[ThinLTO] Respect ClearDSOLocalOnDeclarations for unimported functions

[MaskRay]

Summary

D74751 added ClearDSOLocalOnDeclarations and dropped dso_local for
isDeclarationForLinker GlobalValues. It missed a case for imported
declarations (doImportAsDefinition is false while isPerformingImport is
true). This can lead to a linker error for a default visibility symbol in
ld.lld -shared.

When ClearDSOLocalOnDeclarations is true, we check
isPerformingImport() && !doImportAsDefinition(&GV) along with
GV.isDeclarationForLinker(). The new condition checks an imported declaration.

This patch fixes a LLVMPolly.so link error using a trunk clang -DLLVM_ENABLE_LTO=Thin.

[MaskRay]

Ping:)

13.0.0 will be branched this month and I hope this can be addressed before that. (I could also drop -fno-semantic-interposition for Clang and keep it just for GCC but this issue should be addressed anyway)

[tejohnson]

llvm/lib/Transforms/Utils/FunctionImportUtils.cpp

130

I don't follow what is happening with and without this change. If we aren't planning to import something as a definition, why do we need to mark it available_externally in the source module? How does this change result in dropping the dso local flag?

From your summary:

i.e. when isPerformingImport, an unimported define dso_local @foo should be treated as an available_externally function, instead of an ExternalLinkage definition.

It should eventually end up as an external declaration. Is that not happening? Or is the available_externally marking just a temporary adjustment to trigger subsequent dso_local clearing?

llvm/test/ThinLTO/X86/import-dsolocal.ll

63

Should this have linkonce_odr linkage?

68

Ditto

[MaskRay]

Address comments

llvm/lib/Transforms/Utils/FunctionImportUtils.cpp

130

I got confused. The few cases where getLinkage returns ExternalLinkage means this is an imported declaration (I thought it means an unimported definition).

Returning AvailableExternallyLinkage is not good - we will get declare available_externally and available_externally shouldn't be used on declarations. This is benign because bitcode writer treats available_externally declaration like an external declaration.

I've reverted the part.

[MaskRay]

Test weak/linkonce

[tejohnson]

llvm/lib/Transforms/Utils/FunctionImportUtils.cpp

134

Is the change of the return linkage type needed for this fix, or is it just the returning of IsImportedDeclaration?

Why not just invoke doImportAsDefinition() in the caller to get this info rather than setting IsImportedDeclaration here? There are also other cases below for other linkage types where we are importing as a declaration but the new var isn't set.

[MaskRay]

use doImportAsDefinition

[tejohnson]

lgtm with test fix noted below.

llvm/test/ThinLTO/X86/import-dsolocal.ll

34

Missing checks for linkonceodr[_aux].

[MaskRay]

improve tests

[MaskRay]

llvm/lib/Transforms/Utils/FunctionImportUtils.cpp

134

I think invoking doImportAsDefinition is superior. Switched the implementation.

llvm/lib/Transforms/Utils/FunctionImportUtils.cpp

if (isModuleExporting()) {
     return SGV->getLinkage();
   }
 
   // Otherwise, if we aren't importing, no linkage change is needed.
   if (!isPerformingImport())
     return SGV->getLinkage();
 
   switch (SGV->getLinkage()) {
-  case GlobalValue::LinkOnceODRLinkage:
   case GlobalValue::ExternalLinkage:
-    // External and linkonce definitions are converted to available_externally
-    // definitions upon import, so that they are available for inlining
-    // and/or optimization, but are turned into declarations later
+  case GlobalValue::LinkOnceODRLinkage:
+  case GlobalValue::WeakODRLinkage:
+    // External and linkonce_odr definitions are converted to
+    // available_externally definitions upon import, so that they are available
+    // for inlining and/or optimization, but are turned into declarations later
     // during the EliminateAvailableExternally pass.
-    if (doImportAsDefinition(SGV) && !isa<GlobalAlias>(SGV))
+    //
+    // For weak_odr linkage, there is a guarantee that all copies will be
+    // equivalent, so the issue described below for weak_any does not exist,
+    // and the definition can be imported. It can be treated similarly
+    // to an imported externally visible global value.
+    if (!SGV->isDeclarationForLinker() && !isa<GlobalAlias>(SGV))
       return GlobalValue::AvailableExternallyLinkage;
     // An imported external declaration stays external.
-    return SGV->getLinkage();
+    return GlobalValue::ExternalLinkage;
 
   case GlobalValue::AvailableExternallyLinkage:
     // An imported available_externally definition converts
     // to external if imported as a declaration.
     if (!doImportAsDefinition(SGV))
       return GlobalValue::ExternalLinkage;
     // An imported available_externally declaration stays that way.
     return SGV->getLinkage();
 
   case GlobalValue::LinkOnceAnyLinkage:
   case GlobalValue::WeakAnyLinkage:
     // Can't import linkonce_any/weak_any definitions correctly, or we might
     // change the program semantics, since the linker will pick the first
     // linkonce_any/weak_any definition and importing would change the order
     // they are seen by the linker. The module linking caller needs to enforce
     // this.
     assert(!doImportAsDefinition(SGV));
     // If imported as a declaration, it becomes external_weak.
     return SGV->getLinkage();
 
-  case GlobalValue::WeakODRLinkage:
-    // For weak_odr linkage, there is a guarantee that all copies will be
-    // equivalent, so the issue described above for weak_any does not exist,
-    // and the definition can be imported. It can be treated similarly
-    // to an imported externally visible global value.
-    if (doImportAsDefinition(SGV) && !isa<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 linkIfNeeded, and we will assert in shouldLinkFromSource
     // if we try to import, so we simply return AppendingLinkage.
     return GlobalValue::AppendingLinkage;
 

llvm/test/ThinLTO/X86/import-dsolocal.ll

+; RUN: split-file %s %t
+; RUN: opt -module-summary %t/a.ll -o %t/a.bc
+; RUN: opt -module-summary %t/b.ll -o %t/b.bc
+
+;; With a small limit, *_aux are not imported. Check we discard dso_local.
+; RUN: llvm-lto2 run %t/a.bc %t/b.bc -o %t1 -save-temps -import-instr-limit=3 \
+; RUN:   -r=%t/a.bc,main,plx -r=%t/a.bc,extern, -r=%t/a.bc,linkonceodr, -r=%t/a.bc,weakodr, \
+; RUN:   -r=%t/b.bc,a,pl -r=%t/b.bc,b,pl -r=%t/b.bc,extern,pl -r=%t/b.bc,extern_aux,pl \
+; RUN:   -r=%t/b.bc,linkonceodr,pl -r=%t/b.bc,linkonceodr_aux,pl -r=%t/b.bc,weakodr,pl -r=%t/b.bc,weakodr_aux,pl
+; RUN: llvm-dis %t1.1.3.import.bc -o - | FileCheck %s --check-prefixes=DEST,DEST1
+
+;; With a large limit, *_aux are imported. Check we still discard dso_local.
+; RUN: llvm-lto2 run %t/a.bc %t/b.bc -o %t2 -save-temps -import-instr-limit=10 \
+; RUN:   -r=%t/a.bc,main,plx -r=%t/a.bc,extern, -r=%t/a.bc,linkonceodr, -r=%t/a.bc,weakodr, \
+; RUN:   -r=%t/b.bc,a,pl -r=%t/b.bc,b,pl -r=%t/b.bc,extern,pl -r=%t/b.bc,extern_aux,pl \
+; RUN:   -r=%t/b.bc,linkonceodr,pl -r=%t/b.bc,linkonceodr_aux,pl -r=%t/b.bc,weakodr,pl -r=%t/b.bc,weakodr_aux,pl
+; RUN: llvm-dis %t2.1.3.import.bc -o - | FileCheck %s --check-prefixes=DEST,DEST2
+
+; DEST:      @a = available_externally global i32 42, align 4
+; DEST-NEXT: @b = external global i32*, align 8
+; DEST:      define dso_local i32 @main()
+; DEST:      define available_externally void @extern()
+; DEST1:     declare i32 @extern_aux(i32*, i32**)
+; DEST2:     define available_externally i32 @extern_aux(i32* %a, i32** %b)
+; DEST:      define available_externally void @weakodr()
+; DEST1:     declare i32 @weakodr_aux(i32*, i32**)
+; DEST2:     define available_externally i32 @weakodr_aux(i32* %a, i32** %b)
+
+;--- a.ll
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+declare void @extern()
+declare void @linkonceodr()
+declare void @weakodr()
+
+define i32 @main() {
+  call void @extern()
+  call void @linkonceodr()
+  call void @weakodr()
+  ret i32 0
+}
+
+;--- b.ll
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@a = dso_local global i32 42, align 4
+@b = dso_local global i32* @a, align 8
+
+define dso_local void @extern() {
+  call i32 @extern_aux(i32* @a, i32** @b)
+  ret void
+}
+
+define dso_local i32 @extern_aux(i32* %a, i32** %b) {
+  %p = load i32*, i32** %b, align 8
+  store i32 33, i32* %p, align 4
+  %v = load i32, i32* %a, align 4
+  ret i32 %v
+}
+
+define weak_odr dso_local void @linkonceodr() {
+  call i32 @linkonceodr_aux(i32* @a, i32** @b)
+  ret void
+}
+
+define weak_odr i32 @linkonceodr_aux(i32* %a, i32** %b) {
+  %p = load i32*, i32** %b, align 8
+  store i32 33, i32* %p, align 4
+  %v = load i32, i32* %a, align 4
+  ret i32 %v
+}
+
+define weak_odr dso_local void @weakodr() {
+  call i32 @weakodr_aux(i32* @a, i32** @b)
+  ret void
+}
+
+define weak_odr i32 @weakodr_aux(i32* %a, i32** %b) {
+  %p = load i32*, i32** %b, align 8
+  store i32 33, i32* %p, align 4
+  %v = load i32, i32* %a, align 4
+  ret i32 %v
+}