[PATCH] D38790: [ELF] Avoid keeping useless undefined symbols in .dynsym.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/Writer.cpp:1281-1291
     if (InX::DynSymTab && S->includeInDynsym()) {
+      if (Body->isUndefined() && !Body->IsUsedInReloc)
+        // This symbol was most probably used in a section, which was collected
+        // in the GC phase. We don't need such symbols in the dynamic symbols
+        // table because they solely introduce useless dependencies.
+        continue;
       InX::DynSymTab->addSymbol(Body);
----------------
ikudrin wrote:
> ruiu wrote:
> > I don't think this is logically correct. If `includeInDynsym` returns true, the symbol should be included in a .dynsym regardless of other conditions. You should modify other place so that `includeInDynsym` works correctly.
> `includeInDynsym` is used when computing `IsPreemptible`. This flag is calculated before calling `scanRelocations` because it is used there. And with my approach, we can say that a symbol is useless only after `scanRelocations` is finished and the symbol has not been used anywhere.
> 
> `includeInDynsym` is also called in `SymbolTable::handleDynamicList`, even before GC. At that point, we can't say if a symbol is going to become unreferenced after GC or not.
> 
> What if we just rename `includeInDynsym` to something like `maybeIncludeInDynsym`?
Still it doesn't feel right. It feels like is a violation of constraints each step in lld should maintain. This complex and subtle decision shouldn't be made at this late stage.

You are fixing an issue when -gc-section is given. In the garbage collector, you visit all relocations to see whether something is alive or not. So, can't you do everything in that phase?


https://reviews.llvm.org/D38790