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

[Vy Nguyen via Phabricator via llvm-commits]

oontvoo 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.
> 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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124143