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

[evgeny777]

Summary

D40970 actually breaks one of the gold plugin test cases, because gold plugin marks resolution of asm undefined symbol as prevailing.
This patch fixes it (according to gold sources LDPR_PREVAILING_DEF_IRONLY is not set for symbol when it is referenced from outside of the module)

[tejohnson]

tools/gold/gold-plugin.cpp

659

This seems like a bug in gold? The description of LDPR_PREVAILING_DEF_IRONLY from plugin-api.h says it should be used for definitions:

/* This is the prevailing definition of the symbol, with no
   references from regular objects.  It is only referenced from IR
   code.  */
LDPR_PREVAILING_DEF_IRONLY,

Wondering why it isn't LDPR_UNDEF. What does the test case look like?

[evgeny777]

tools/gold/gold-plugin.cpp

659

Gold sets LDPR_UNDEF only for regular undefined symbols. IR symbols with LDPR_PREVAILING_DEF_IRONLY can be either LDPK_DEF or LDPK_UNDEF (and also LDPK_WEAKUNDEF). Gold plugin sets all IRONLY resolutions to prevailing, no matter if they are defined or not. This, by the way doesn't correspond to lld behavior, where we have this (ELF/LTO.cpp):

R.Prevailing = !ObjSym.isUndefined() && Sym->File == &F;

What does the test case look like?

Like I said this is an addition to D40970 (reverted), which fixes broken gold plugin test (asm_undefined2.ll)

[tejohnson]

tools/gold/gold-plugin.cpp

659

Ok I did some archaeology since this particular test case rang a bell. Your change here isn't necessarily wrong, but I'd like to understand why the existing mechanisms in the thin link aren't handling this correctly. Background below, but see the very end of my long comment for how I *think* this should be currently handled by the thin link so that we don't incorrectly mark this as internalized in the index during that phase.

The test case was added in https://reviews.llvm.org/D24617, along with some code in the LTO backends to add these to the llvm.compiler.used which should prevent internalization. This patch was added in response to discussion on the subsequently abandoned D24595 (the relevant discussion is on the llvm-dev mailing list and not Phab, between Rafael, Mehdi and myself). In fact, the implication seems to be that D24617 should have avoided the need for the same handling in thinLtoInternalizeModule that you are trying to remove (but it never got cleaned up - likely my bad).

But later, this handling to add these to the llvm.compiler.used was removed by pcc, who replaced this with IRSymtab logic in D32544, that was also supposed to be preventing the internalization of foo in this particular test case.

So I guess the follow-on question is why is the handling added in D32544 not covering this? AFAICT that patch should have caused the isUsed() flag to be true for the use of foo in the asm, and the following existing logic in LTO::addSymbolToGlobalRes (now called LTO::addModuleToGlobalRes) should have caused the partition to be marked as External, and should be enough to prevent internalization:

// Set the partition to external if we know it is used elsewhere, e.g.
// it is visible to a regular object, is referenced from llvm.compiler_used,
// or was already recorded as being referenced from a different partition.
if (Res.VisibleToRegularObj || Sym.isUsed() ||
    (GlobalRes.Partition != GlobalResolution::Unknown &&
     GlobalRes.Partition != Partition)) {
  GlobalRes.Partition = GlobalResolution::External;

Can you double check whether isUsed() is being set properly for the use of foo in the asm in that test case, and if so, why is it being internalized during the thin link (i.e. why isn't it being added to ExportedGUIDs during runThinLTO)?

[evgeny777]

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

[tejohnson]

In 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 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.

[evgeny777]

Can you add an assert for this

check-all passes with this, but we can explicitly mark asm undefined symbol as prevailing using llvm-lto(2) and trigger this assert. Is this OK?

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

When you have asm def as prevailing (gold plugin) we mark foo as dead in ModuleSummaryIndex. It's only check for

if (AsmUndefinedRefs.count(GV.getName()))
  return true;

in thinLTOInternalizeModule which prevents DCE.

[tejohnson]

In D41113#953964, @evgeny777 wrote:

Can you add an assert for this

check-all passes with this, but we can explicitly mark asm undefined symbol as prevailing using llvm-lto(2) and trigger this assert. Is this OK?

Not sure what you mean about marking via llvm-lto2 - I meant that the asm_undefined2.ll test should cause this assert to fire without the fix in this patch (i.e. suppress the fix temporarily to make sure it fires there).

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

When you have asm def as prevailing (gold plugin) we mark foo as dead in ModuleSummaryIndex. It's only check for

if (AsmUndefinedRefs.count(GV.getName()))
  return true;

in thinLTOInternalizeModule which prevents DCE.

Yeah, and that's the code that we were talking separately about removing. There's even a probable current issue - if the prevailing def in the asm is marked dead, anything reachable from it might be marked dead as well (i.e. if the only use is in that def).

[evgeny777]

I meant that the asm_undefined2.ll test should cause this assert without the fix in this patch

Without modification to gold-plugin.cpp assertion fires. I've added these lines of code:

if (Res.Prevailing) {
  assert(GlobalRes.IRName.empty() || !Sym.getIRName().empty());
  GlobalRes.IRName = Sym.getIRName();
}

Not sure what you mean about marking via llvm-lto2

I meant that this assertion may also fire (and actually does) when you set symbol resolutions manually, which is possible to do using llvm-lto and llvm-lto2.
This is the reason why it breaks one of llvm-lto test cases.

[tejohnson]

In D41113#954049, @evgeny777 wrote:

I meant that the asm_undefined2.ll test should cause this assert without the fix in this patch

Without modification to gold-plugin.cpp assertion fires.

Great, that's what I was expecting. Go ahead and add the assert to this patch.

I've added these lines of code:

if (Res.Prevailing) {
  assert(GlobalRes.IRName.empty() || !Sym.getIRName().empty());

I wonder if this can just be "assert(GlobalRes.IRName.empty())" in fact. Can you give that a try?

GlobalRes.IRName = Sym.getIRName();

}

> Not sure what you mean about marking via llvm-lto2

I meant that this assertion may also fire (and actually does) when you set symbol resolutions manually, which is possible to do using llvm-lto and llvm-lto2.
This is the reason why it breaks one of llvm-lto test cases.

Ah got it. No need to add a new test, the fact that the existing test would fail this new assertion without your fix is good enough.

Hoping pcc has a chance to look today. I'd like to get his thoughts on what should be happening here in the prevailing asm def case.

[evgeny777]

the fact that the existing test would fail this new assertion without your fix is good enough.

Unfortunately it will fail both with and w/o the fix (in case assertion is added), because llvm-lto doesn't use gold plugin in any way. One can explicitly set both ASM undef and IR def as prevailing by simply giving this resolution file:

asm_undefined2.ll.tmp.o
-r=asm_undefined2.ll.tmp.o,patatino,pl
-r=asm_undefined2.ll.tmp.o,foo,pl
-r=asm_undefined2.ll.tmp.o,patatino,pl
-r=asm_undefined2.ll.tmp.o,foo,pl

to llvm-lto2.

I now wonder if we can simply ignore empty symbol name with this (check-all passes flawlessly as well):

if (Res.Prevailing && !Sym.getIRName().empty())
  GlobalRes.IRName = Sym.getIRName();

In such case there is no need to modify gold plugin. Hope @pcc will bring some light.

[pcc]

This seems fine to me with the assert.

There's even a probable current issue - if the prevailing def in the asm is marked dead, anything reachable from it might be marked dead as well (i.e. if the only use is in that def).

Module asm definitions do not refer to anything at the summary level, so it does not matter if we mark them as dead. Any symbols that are referenced from module inline asm are GC roots because of D32544.

[evgeny777]

@pcc What about case when assertion is triggered in llvm-lto2 (symbol resolutions are set manually with -r=...) ? How to deal with it?

[pcc]

I now wonder if we can simply ignore empty symbol name with this

Doesn't seem like a good idea to me. It is an error for a linker to mark more than one copy of a symbol as prevailing, and the assert would allow us to check for that property in most cases.

[pcc]

In D41113#954158, @evgeny777 wrote:

@pcc What about case when assertion is triggered in llvm-lto2 (symbol resolutions are set manually with -r=...) ? How to deal with it?

In that case, the user of the llvm-lto2 tool has made a mistake. If we wanted, we could add a check to llvm-lto2 that the user does not mark more than one copy of a symbol as prevailing.

[evgeny777]

In that case, the user of the llvm-lto2 tool has made a mistake.

I see. This assertion currently breaks one of llvm-lto test cases. I'll fix it than.

[tejohnson]

In D41113#954157, @pcc wrote:

This seems fine to me with the assert.

There's even a probable current issue - if the prevailing def in the asm is marked dead, anything reachable from it might be marked dead as well (i.e. if the only use is in that def).

Module asm definitions do not refer to anything at the summary level, so it does not matter if we mark them as dead. Any symbols that are referenced from module inline asm are GC roots because of D32544.

Ok right, any use in module asm that is defined in IR will get an IRName from the prevailing def there, and be marked as VisibleOutsideSummary.
Any def in module asm that is used in IR must have a declaration in IR - presumably we won't end up with a Prevailing def that sets the IRName, but it will just look like something defined outside of the index, and we won't have a summary to mark dead or internalized or anything incorrect (the module summary builder skips declarations).

So with the new assert ensuring that we don't overwrite a non-empty IRName, this LGTM.

[tejohnson]

llvm/trunk/lib/LTO/LTO.cpp

420 ↗

(On Diff #127077)

I had suggested changing this to simply:

assert(GlobalRes.IRName.empty() && "....

since we should only have one prevailing def. Can you do that?

tools/gold/gold-plugin.cpp

for (ld_plugin_symbol &Sym : F.syms) {
     case LDPR_RESOLVED_EXEC:
     case LDPR_RESOLVED_DYN:
     case LDPR_PREEMPTED_IR:
     case LDPR_PREEMPTED_REG:
     case LDPR_UNDEF:
       break;
 
     case LDPR_PREVAILING_DEF_IRONLY:
-      R.Prevailing = true;
+      R.Prevailing = (Sym.def != LDPK_UNDEF);
       break;
 
     case LDPR_PREVAILING_DEF:
       R.Prevailing = true;
       R.VisibleToRegularObj = true;
       break;
 
     case LDPR_PREVAILING_DEF_IRONLY_EXP: