[PATCH] D41113: [LLVMgold] Don't set resolutions for undefined symbols to 'Prevailing'

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D41113#953393, @evgeny777 wrote:

> Thanks for sharing this! Here are results of my brief investigation:
>
> IR symbol loading (through IRSymtab) happens absolutely identically in case of gold plugin and lld. In both cases undefined asm counterpart of 'foo' gets FB_used bit (so isUsed() returns true).
>  The difference, however, shows up when we process symbol resolutions in lto::addModuleToGlobalRes. The irsymtab::storage::Symbol has two name fields, used for different purposes
>
> - Name. This is mangled name used to map GlobalResolution to specific IR symbol (see lto::addModuleToGlobalRes)
> - IRName. This is unmangled name used to compute GUID (hash). For asm undefined symbols IRName is an empty string as they aren't IR symbols. This is being done in irsymtab::Builder::addSymbol
>
>   Now to your question (why is the handling added in https://reviews.llvm.org/D32544 not covering this?):
>
>   The lto::addModuleToGlobalRes relies on correct setting of 'Prevailing' in SymbolResolution. The logic behind it assumes that prevailing symbol always has valid IRName, so there are following lines of code:
>
>   ``` if (Res.Prevailing) GlobalRes.IRName = Sym.getIRName(); ``` When we see asm undefined symbol with Prevailing == true (gold plugin) we set GlobalRes.IRName to an empty string. After that we are unable to correctly compute hash and set its IR counterpart as GC root. This in turn causes incorrect symbol internalization/DCE.


Thanks for digging! +pcc for thoughts, as this seems like a subtle issue. I think we should proceed with this patch as it seems correct - an undef shouldn't be prevailing. And presumably the IR copy is also marked prevailing, but the asm copy is causing the IRName in the shared GlobalRes to be overwritten with ""? Can you add an assert for this (i.e. assert when we overwrite a non-empty GlobalRes.IRName with an empty Sym.getIRName()) in this patch, and check that it fires

But it raises some other issues. It isn't just ThinLTO internalization code that is going to have an issue. Dead stripping (both full LTO and ThinLTO) run the following code:

  if (Res.second.VisibleOutsideSummary &&
      // IRName will be defined if we have seen the prevailing copy of
      // this value. If not, no need to preserve any ThinLTO copies.
      !Res.second.IRName.empty())
    GUIDPreservedSymbols.insert(GlobalValue::getGUID(
        GlobalValue::dropLLVMManglingEscape(Res.second.IRName)));

What happens if the asm does have the prevailing def? Isn't that possible? In that case the IRName will never be set.


https://reviews.llvm.org/D41113