[LLD][ThinLTO] Handle GUID collision in import global processing

[the_jk]

Summary

If there are a GUID collision between two globals checking the
summarylist from the import index to make assumption can be dangerous.

Do not assume that a GlobalValue that has a GlobalVarSummary
actually is a GlobalVariable as it can be another GlobalValue with
the same GUID that the summary is connected to.

[the_jk]

Bug report: https://bugs.llvm.org/show_bug.cgi?id=43250

[evgeny777]

Sorry, I somehow missed this. My understanding is that this is an ODR violation and should trigger linker error, instead of trying to be smart.
Adding @tejohnson

llvm/lib/Transforms/Utils/FunctionImportUtils.cpp

247

Why this check is needed?

[tejohnson]

llvm/lib/Transforms/Utils/FunctionImportUtils.cpp

247

I suppose it is because in e.g. the test case the two local F values have the same GUID and we are attempting to mark only the variable in this module.

There is already a facility for doing this - see ModuleSummaryIndex::findSummaryInModule(ValueInfo VI, StringRef ModuleId)

[evgeny777]

I wonder how such thing happened in the first place. Can linker produce working image even if we circumvent the crash?

[tejohnson]

In D67322#1704084, @evgeny777 wrote:

I wonder how such thing happened in the first place. Can linker produce working image even if we circumvent the crash?

There isn't an ODR issue since they are both locals. The GUID is the same because, since it is a local, the name is created by prepending the source_filename, which is "-" in both cases, which defeats our normal mechanism for creating a unique GUID for locals. It turned out this was due to the way clang was being invoked by icecc (see the bug for details). It was also addressed separately by D67592, but this is still good to fix.

[evgeny777]

Is this the only place where bad things can happen? May be simply raise an error in addGlobalValueSummary when new summary type is different from that of SummaryList[0]?

[tejohnson]

In D67322#1704278, @evgeny777 wrote:

Is this the only place where bad things can happen? May be simply raise an error in addGlobalValueSummary when new summary type is different from that of SummaryList[0]?

We should probably have more testing of this situation. But if we can make it work we should wherever possible.

[the_jk]

Updated to latest master.

[the_jk]

Another place that already handles multiple locals with the same GUID is in the same file: bool FunctionImportGlobalProcessing::shouldPromoteLocalToGlobal(), from a comment there:

// When exporting, consult the index. We can have more than one local
// with the same GUID, in the case of same-named locals in different but
// same-named source files that were compiled in their respective directories
// (so the source file name and resulting GUID is the same). Find the one
// in this module.

so I think raising an error would cause surprises for different setups.
I'll gladly add more testcases if you can give me some ideas/tips on possible problems.

[tejohnson]

In D67322#1707478, @the_jk wrote:

Another place that already handles multiple locals with the same GUID is in the same file: bool FunctionImportGlobalProcessing::shouldPromoteLocalToGlobal(), from a comment there:

// When exporting, consult the index. We can have more than one local
// with the same GUID, in the case of same-named locals in different but
// same-named source files that were compiled in their respective directories
// (so the source file name and resulting GUID is the same). Find the one
// in this module.

so I think raising an error would cause surprises for different setups.
I'll gladly add more testcases if you can give me some ideas/tips on possible problems.

I think the testing you have here is good for this patch. But please implement my suggestion to use findSummaryInModule, and also add a comment similar to this one.

[the_jk]

Sorry, I somehow missed the reference to findSummaryInModule the first time. Updated diff.

[tejohnson]

LGTM. Small suggestion to expand comment a bit.

llvm/lib/Transforms/Utils/FunctionImportUtils.cpp

243

Please add note about this being because of same named but different source files compiled without distinguishing path context.

[the_jk]

Include all of the comment.

[the_jk]

I don't have commit access so I'd need someone to push this, thanks.

[tejohnson]

In D67322#1721220, @the_jk wrote:

I don't have commit access so I'd need someone to push this, thanks.

I'll take care of that for you today.

[tejohnson]

In D67322#1721221, @tejohnson wrote:

In D67322#1721220, @the_jk wrote:

I don't have commit access so I'd need someone to push this, thanks.

I'll take care of that for you today.

Actually, I don't believe I have commit access at the moment either (didn't add my git username before the migration this week). I'll commit this as soon as I can get access...

[tejohnson]

I had to revert this as it was causing a failure in my testing. Looking into it...

[tejohnson]

Recommitted in 16ec00eee7e348767f4393f189044f87f6374031 with fix for the failure I saw.

llvm/lib/Transforms/Utils/FunctionImportUtils.cpp

void FunctionImportGlobalProcessing::processGlobalForThinLTO(GlobalValue &GV) {
   // to link variable definitions to their external declarations during
   // ThinLTO import. We'll internalize read-only variables later, after
   // import is finished. See internalizeGVsAfterImport.
   //
   // If global value dead stripping is not enabled in summary then
   // propagateConstants hasn't been run. We can't internalize GV
   // in such case.
   if (!GV.isDeclaration() && VI && ImportIndex.withGlobalValueDeadStripping()) {
-    const auto &SL = VI.getSummaryList();
-    auto *GVS = SL.empty() ? nullptr : dyn_cast<GlobalVarSummary>(SL[0].get());
+    if (GlobalVariable *V = dyn_cast<GlobalVariable>(&GV)) {
+      // We can have more than one local with the same GUID, find the one
+      // in this module.
+      auto* GVS = dyn_cast<GlobalVarSummary>(
+          ImportIndex.findSummaryInModule(VI, M.getModuleIdentifier()));
       // At this stage "maybe" is "definitely"
       if (GVS && (GVS->maybeReadOnly() || GVS->maybeWriteOnly()))
-      cast<GlobalVariable>(&GV)->addAttribute("thinlto-internalize");
+        V->addAttribute("thinlto-internalize");
+    }
   }
 
   bool DoPromote = false;
   if (GV.hasLocalLinkage() &&
       ((DoPromote = shouldPromoteLocalToGlobal(&GV)) || isPerformingImport())) {
     // Save the original name string before we rename GV below.
     auto Name = GV.getName().str();
     // Once we change the name or linkage it is difficult to determine

llvm/test/ThinLTO/X86/Inputs/guid_collision.ll

+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-linux-gnu"
+
+; The source for the GUID for this symbol will be -:F
+source_filename = "-"
+define internal fastcc i64 @F() {
+  ret i64 0
+}
+
+; Needed to give llvm-lto2 something to do
+@dummy2 = global i32 0
+

llvm/test/ThinLTO/X86/guid_collision.ll

+; Make sure LTO succeeds even if %t.bc contains a GlobalVariable F and
+; %t2.bc cointains a Function F with the same GUID.
+;
+; RUN: opt -module-summary %s -o %t.bc
+; RUN: opt -module-summary %p/Inputs/guid_collision.ll -o %t2.bc
+; RUN: llvm-lto2 run %t.bc %t2.bc -o %t.out \
+; RUN: -r=%t.bc,dummy,px -r=%t2.bc,dummy2,px
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-linux-gnu"
+
+; The source for the GUID for this symbol will be -:F
+source_filename = "-"
+@F = internal constant i8 0
+
+; Needed to give llvm-lto2 something to do
+@dummy = global i32 0