ThinLTOBitcodeWriter: drop dso_local when a GlobalVariable is converted to a declaration

[MaskRay]

Summary

If we infer the dso_local flag for -fpic, dso_local should be dropped
when we convert a GlobalVariable a declaration. dso_local causes the
generation of direct access (e.g. R_X86_64_PC32). Such relocations referencing
STB_GLOBAL STV_DEFAULT objects are not allowed in a -shared link.

[tejohnson]

Should there be a verifier check that there is no dso_local on declarations (after this and D74751 go in)?

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

186 ↗

(On Diff #245064)

Better to do this inside convertToDeclaration, since there are other places we use that to drop definitions. Also logically it belongs there.

llvm/test/Transforms/ThinLTOBitcodeWriter/split-dsolocal.ll

9

This checking seems unnecessary in this test.

[MaskRay]

Address comments

[tejohnson]

llvm/lib/Transforms/IPO/FunctionImport.cpp

993

Don't we also need it here (alias case)?

[MaskRay]

llvm/lib/Transforms/IPO/FunctionImport.cpp

993

GlobalAlias does not need it, but I'll add a test case that an alias works.

I will also add a hidden GlobalVariable case. If we don't check if (!GV.isImplicitDSOLocal()), setting a hidden GlobalVariable declaration dso_preemptable will trigger a verifier error (IR/Verifier.cpp:visitGlobalValue).

[MaskRay]

Improve tests

[MaskRay]

Ping :)

[MaskRay]

Fix not-prevailing-variables.ll (this patch fixes a copy relocation)

[tejohnson]

LGTM

llvm/lib/Transforms/IPO/FunctionImport.cpp

993

Oh ic why, because we create a new decl without it in the alias case. Nevermind.

[hoyFB]

Hello, It looks like this change is breaking the test llvm/test/tools/gold/X86/thinlto_weak_library.ll . Can you please verify? Thanks!

[MaskRay]

In D74749#1910258, @hoyFB wrote:

Hello, It looks like this change is breaking the test llvm/test/tools/gold/X86/thinlto_weak_library.ll . Can you please verify? Thanks!

My other llvm-objdump -d change also caused a disassembly test to fail.

Fixed by c3de1d0b1f99926f4a2665756b29b2d35a8359b8

[hoyFB]

In D74749#1910352, @MaskRay wrote:

In D74749#1910258, @hoyFB wrote:

Hello, It looks like this change is breaking the test llvm/test/tools/gold/X86/thinlto_weak_library.ll . Can you please verify? Thanks!

My other llvm-objdump -d change also caused a disassembly test to fail.

Fixed by c3de1d0b1f99926f4a2665756b29b2d35a8359b8

Thanks for the fast turnaround!

llvm/lib/Transforms/IPO/FunctionImport.cpp

else
       NewGV =
           new GlobalVariable(*GV.getParent(), GV.getValueType(),
                              /*isConstant*/ false, GlobalValue::ExternalLinkage,
                              /*init*/ nullptr, "",
                              /*insertbefore*/ nullptr, GV.getThreadLocalMode(),
                              GV.getType()->getAddressSpace());
     NewGV->takeName(&GV);
     GV.replaceAllUsesWith(NewGV);
     return false;
   }
+  if (!GV.isImplicitDSOLocal())
+    GV.setDSOLocal(false);
   return true;
 }
 
 /// Fixup prevailing symbol linkages in \p TheModule based on summary analysis.
 void llvm::thinLTOResolvePrevailingInModule(
     Module &TheModule, const GVSummaryMapTy &DefinedGlobals) {
   auto updateLinkage = [&](GlobalValue &GV) {
     // See if the global summary analysis computed a new resolved linkage.

llvm/test/LTO/Resolution/X86/not-prevailing-variables.ll

 ; RUN: opt -module-summary %s -o %t1.o
 ; RUN: llvm-lto2 run -save-temps -o %t2.o %t1.o      \
 ; RUN:   -r %t1.o,testVar1,plx -r %t1.o,testVar2,plx \
 ; RUN:   -r %t1.o,var1,pl -r %t1.o,var2,lx
 
 ; Test contains two retainedNodes: var1 and var2.
 ; var2 is not prevailing and here we check it is not inlined.
 
 ; Check 'var2' was not inlined.
-; RUN: llvm-objdump -d %t2.o.1 | FileCheck %s
+; RUN: llvm-objdump -d -r %t2.o.1 | FileCheck %s
 ; CHECK:      <testVar1>:
 ; CHECK-NEXT:   movl $10, %eax
 ; CHECK-NEXT:   retq
 ; CHECK:      <testVar2>:
-; CHECK-NEXT:   movl  (%rip), %eax
+; CHECK-NEXT:   movq  (%rip), %rax
+; CHECK-NEXT:     R_X86_64_GOTPCREL var2-0x4
+; CHECK-NEXT:   movl  (%rax), %eax
 ; CHECK-NEXT:   retq
 
 ; Check 'var2' is undefined.
 ; RUN: llvm-readelf --symbols %t2.o.1 | FileCheck %s --check-prefix=UND
 ; UND: NOTYPE  GLOBAL DEFAULT UND var2
 
 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"

llvm/test/Transforms/ThinLTOBitcodeWriter/split-dsolocal.ll

+; RUN: opt -thinlto-bc -thinlto-split-lto-unit -o %t %s
+; RUN: llvm-modextract -b -n 0 -o %t0.bc %t
+; RUN: llvm-modextract -b -n 1 -o %t1.bc %t
+; RUN: llvm-dis -o - %t0.bc | FileCheck --check-prefix=M0 %s
+; RUN: llvm-dis -o - %t1.bc | FileCheck --check-prefix=M1 %s
+
+; M0: @default = external constant [1 x i8]
+; M0: @hidden = external hidden constant [1 x i8]
+; M0: @al = external global [1 x i8]
+
+; M1: @default = dso_local constant [1 x i8] c"0", !type !0
+; M1: @hidden = hidden constant [1 x i8] c"0", !type !0
+; M1: @al = dso_local alias [1 x i8], [1 x i8]* @default
+@default = dso_local constant [1 x i8] c"0", !type !0
+@hidden = dso_local hidden constant [1 x i8] c"0", !type !0
+
+@al = dso_local alias [1 x i8], [1 x i8]* @default
+
+define [1 x i8]* @f_default() { ret [1 x i8]* @default }
+define [1 x i8]* @f_hidden() { ret [1 x i8]* @hidden }
+define [1 x i8]* @f_al() { ret [1 x i8]* @al }
+
+!0 = !{i32 0, !"typeid"}