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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: tools/gold/gold-plugin.cpp:659
     case LDPR_PREVAILING_DEF_IRONLY:
-      R.Prevailing = true;
+      R.Prevailing = (Sym.def != LDPK_UNDEF);
       break;
----------------
evgeny777 wrote:
> tejohnson wrote:
> > 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?
> 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)
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)?


https://reviews.llvm.org/D41113