[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 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:
> > 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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124143