[PATCH] D124143: [lld-macho] Allow dead_strip to work with autohide symbols that cannot be exported

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/MarkLive.cpp:234
         // explicitUndefineds code below would handle this automatically.
-        assert(!defined->privateExtern &&
-               "should have been rejected by driver");
         marker->addSym(defined);
         continue;
----------------
thevinster wrote:
> thevinster wrote:
> > oontvoo wrote:
> > > thevinster wrote:
> > > > oontvoo wrote:
> > > > > thakis wrote:
> > > > > > Should we call addSym for privateExterns? That seems wrong (?)
> > > > > > Should we call addSym for privateExterns 
> > > > > 
> > > > > normally we shouldn't but if a symbol is both private extern and autohide, then i think it should be ok,
> > > > > 
> > > > > relatedly, stray comment should be deleted or moved?
> > > > I thought the comment was referring to the `addSym`, but I don't think it will be changed so I'm happy to remove it. 
> > > > 
> > > > > if a symbol is both private extern and autohide, then i think it should be ok,
> > > > 
> > > > This is what I found from testing as well. Happy to re-add the assert back and only skip it if it is both privateExtern and autohide. I think that's the one I'm fixing in my local builds.
> > > > I thought the comment was referring to the addSym, but I don't think it will be changed so I'm happy to remove it.
> > > 
> > > The comment was referring to the fact that we shouldn't see "privateExtern" on a symbol from symtab and that such check could have been done in Driver (when we process the exported  symbols).
> > > Now that you're removing the assert, the comment can either move to Driver or just delete it.
> > > 
> > > (btw, the comment was lacking the case where a symbol is both privateExtern and autohide)
> > So with some further testing, this is looking fairly strange. I think ld64 is just preserving all the symbols that are exported even if they cannot be exported. It does feel strange to preserve the symbol if it's privateExtern... I can skip "unable to be exported syms" from being marked live which seems like the correct behavior, but this will deviate from the ld64's behavior. What do y'all think should be the correct approach here?
> > 
> > @int3 @oontvoo @thakis 
> We have some pretty convoluted logic with privateExterns and autohides. For symbols that are both privateExterns and autohide, we end up nullifying the flag. We've effectively removed lost that information and can't distinguish if a particular symbol was originally privateExtern and autohide which makes checking for that case a harder. Although, from empirical testing w/ ld64, it looks like it just preserves them when symbols from the exported list cannot be exported. 
it looks like ld64 does treat a private extern symbol as a liveness root if it's exported, regardless of whether it is autohide. Let's just leave a comment to that effect and follow ld64's behavior. It doesn't cost us in terms of code complexity and it is one less thing for new adopters of LLD to worry about


================
Comment at: lld/test/MachO/export-options.s:148-149
+# RUN: %no-fatal-warnings-lld -dylib -exported_symbol "_foo" %t/autohide-private-extern.o \
+# RUN:   -dead_strip -o %t/no_crash
+# RUN: llvm-nm -m %t/no_crash | FileCheck %s --check-prefix=NO-CRASH
+
----------------
it's better to describe what the test should cover (`exported-hidden`) rather than what it shouldn't do (`no_crash`). (The comment itself can note that this used to crash.)

let's also mention explicitly that we expect `_foo` to be treated as a liveness root due to `-exported_symbol`

also nit: use hyphens instead of underscores for the file names


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124143/new/

https://reviews.llvm.org/D124143