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

[Vincent Lee via Phabricator via llvm-commits]

thevinster added a subscriber: int3.
thevinster 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:
> 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.
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 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124143