[ThinLTO] Appending linkage fixes

[tejohnson]

Summary

Fix import from module with appending var, which cannot be imported. The
first fix is to remove an overly-aggressive error check.

The second fix is to deal with restructuring introduced to the module
linker yesterday in r254418. The handling in linkGlobalValueProto for
not linking in appending variables during function importing is coming
too late, when we are materializing decls. Add the same check to
linkIfNeeded. (Rafael, perhaps the checking in linkGlobalValueProto and
linkIfNeeded should be commoned? Also, I'm concerned about the case
where linkGlobalValueProto decides linkFromSrc==false and there is no
existing dest GV. Previously we would skip linking for that source GV,
but now we will create a decl in the dest module, as the early return
when "!LinkFromSrc && !DGV" has been removed from linkGlobalValueProto
due to the change in callsite. Are there other lurking issues?)

Test by Mehdi Amini.

[• rafael]

Can this be tested with llvm-link instead of opt? LGTM with that.

[tejohnson]

• Change test to be llvm-link based.

[• rafael]

LGTM with a nit.

test/Linker/funcimport_appending_global.ll

6 ↗

(On Diff #41649)

Can you just pass %s to llvm-link and remove the llvm-as run?

[tejohnson]

test/Linker/funcimport_appending_global.ll

6 ↗

(On Diff #41649)

I can't because the .ll file doesn't have the function summary. Running llvm-as with the -function-summary option is required to get that, then llvm-lto creates the combined function index to drive importing.

[mehdi_amini]

lib/Linker/LinkModules.cpp

1867 ↗

(On Diff #41649)

Found another crash:

ModuleLinker::run() will start by iterating over all the variables in the module and call linkIfNeeded(GV). Your fix here only kicks in if there is no "DGV" (why?).

If the destination module already have a global_ctors, DGV won't be null. We may want to fix getLinkedToGlobal() as well.

All of this logic gets more and more complicate, this is a bit scary.

[tejohnson]

lib/Linker/LinkModules.cpp

1867 ↗

(On Diff #41649)

Right, for appending vars we shouldn't import regardless of DGV status. Fixed this, updated test, and removed the old handling in linkGlobalValueProto which is no longer needed as the check is moved here (consistent with other checks that moved from linkGlobalValueProto to linkIfNeeded in the restructuring earlier this week). I don't think the logic is getting more complicated, it has just moved as needed based on the restructuring.

[tejohnson]

• Handle case where dest module already has an appending linkage variable with the same name. Cleanup old handling.

[• rafael]

LGTM. Thanks

llvm/trunk/lib/Linker/LinkModules.cpp

if (doImportAsDefinition(SGV) && !dyn_cast<GlobalAlias>(SGV))
       return GlobalValue::AvailableExternallyLinkage;
     else
       return GlobalValue::ExternalLinkage;
 
   case GlobalValue::AppendingLinkage:
     // It would be incorrect to import an appending linkage variable,
     // since it would cause global constructors/destructors to be
     // executed multiple times. This should have already been handled
-    // by linkGlobalValueProto.
-    llvm_unreachable("Cannot import appending linkage variable");
+    // by linkIfNeeded, and we will assert in shouldLinkFromSource
+    // if we try to import, so we simply return AppendingLinkage here
+    // as this helper is called more widely in getLinkedToGlobal.
+    return GlobalValue::AppendingLinkage;
 
   case GlobalValue::InternalLinkage:
   case GlobalValue::PrivateLinkage:
     // If we are promoting the local to global scope, it is handled
     // similarly to a normal externally visible global.
     if (doPromoteLocalToGlobal(SGV)) {
       if (doImportAsDefinition(SGV) && !dyn_cast<GlobalAlias>(SGV))
         return GlobalValue::AvailableExternallyLinkage;
bool ModuleLinker::shouldLinkFromSource(bool &LinkFromSrc,
   // Should we unconditionally use the Src?
   if (shouldOverrideFromSrc()) {
     LinkFromSrc = true;
     return false;
   }
 
   // We always have to add Src if it has appending linkage.
   if (Src.hasAppendingLinkage()) {
-    // Caller should have already determined that we can't link from source
-    // when importing (see comments in linkGlobalValueProto).
+    // Should have prevented importing for appending linkage in linkIfNeeded.
     assert(!isPerformingImport());
     LinkFromSrc = true;
     return false;
   }
 
   bool SrcIsDeclaration = Src.isDeclarationForLinker();
   bool DestIsDeclaration = Dest.isDeclarationForLinker();
 
Constant *ModuleLinker::linkAppendingVarProto(GlobalVariable *DstGV,
   return Ret;
 }
 
 Constant *ModuleLinker::linkGlobalValueProto(GlobalValue *SGV) {
   GlobalValue *DGV = getLinkedToGlobal(SGV);
 
   // Handle the ultra special appending linkage case first.
   assert(!DGV || SGV->hasAppendingLinkage() == DGV->hasAppendingLinkage());
-  if (SGV->hasAppendingLinkage())
+  if (SGV->hasAppendingLinkage()) {
+    // Should have prevented importing for appending linkage in linkIfNeeded.
+    assert(!isPerformingImport());
     return linkAppendingVarProto(cast_or_null<GlobalVariable>(DGV),
                                  cast<GlobalVariable>(SGV));
+  }
 
   bool LinkFromSrc = true;
   Comdat *C = nullptr;
   bool HasUnnamedAddr = SGV->hasUnnamedAddr();
 
   if (isPerformingImport() && !doImportAsDefinition(SGV)) {
     LinkFromSrc = false;
   } else if (const Comdat *SC = SGV->getComdat()) {

llvm/trunk/test/Linker/Inputs/funcimport_appending_global.ll

+@v = weak global i8 1
[e-mail]_ctors = appending global [1 x { i32, void ()*, i8* }] [{ i32, void ()*, i8* } { i32 65535, void ()* @foo, i8* @v}]
+
+define void @foo() {
+  ret void
+}

llvm/trunk/test/Linker/funcimport_appending_global.ll

+; RUN: llvm-as -function-summary %s -o %t.bc
+; RUN: llvm-as -function-summary %p/Inputs/funcimport_appending_global.ll -o %t2.bc
+; RUN: llvm-lto -thinlto -o %t3 %t.bc %t2.bc
+
+; Do the import now
+; RUN: llvm-link %t.bc -functionindex=%t3.thinlto.bc -import=foo:%t2.bc -S | FileCheck %s
+
+; Ensure that global constructor (appending linkage) is not imported
+; CHECK-NOT: @llvm.global_ctors = {{.*}}@foo
+
+declare void @f()
[e-mail]_ctors = appending global [1 x { i32, void ()*, i8* }] [{ i32, void ()*, i8* } { i32 65535, void ()* @f, i8* null}]
+
+define i32 @main() {
+entry:
+  call void @foo()
+  ret i32 0
+}
+
+declare void @foo()