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

[Rafael Avila de Espindola via llvm-commits]

Igor Kudrin via Phabricator <reviews at reviews.llvm.org> writes:

> ikudrin 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);
> ----------------
> 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.

The only symbols that we want to remove from the symbol table are
undefined ones, right? I think then that the early calls to
handleDynamicList are fine.

The existing calls are:

ELF/LTO.cpp: only called for prevailing symbols.
ELF/MarkLive.cpp: we only case about defined symbols.
ELF/SymbolTable.cpp: Used to set IsPreemptible, should only care about
                     defined symbols.
ELF/Writer.cpp: Also setting IsPreemptible.
ELF/Writer.cpp: Actually adding a symbol to the dynamic symbol table.

So it seems safe to start with undefined symbols "dead" and mark them
live in gc.

Cheers,
Rafael