[ThinLTO] Remove dllimport attribute from locally defined symbols

[ormris]

Summary

The LTO/ThinLTO driver currently creates invalid bitcode by setting
symbols marked dllimport as dso_local. The compiler often has access
to the definition (often dllexport) and the declaration (often
dllimport) of an object at link-time, leading to a conflicting
declaration. This patch resolves the inconsistency by removing the
dllimport attribute.

[rnk]

Why add dllexport instead of just removing dllimport as was done before? This often happens in situations involving inline methods of templates, and I don't think it would be correct to export such inline functions. I also wouldn't want LTO to suddenly export a bunch of things that the native linker wouldn't export. By the way, this situation is covered by the LNK4049 and LNK4217 warnings in the Microsoft linker.

Here's an example of a situation where you might get accidental incorrect imports: https://gcc.godbolt.org/z/VAl5Kn

void g(int);
template <typename T>
struct Foo {
  static void f() { g(sizeof(T)); }
};
// Uncomment Bar to see Foo<int>::f get imported
//struct __declspec(dllimport) Bar : Foo<int> {};
void h() { Foo<int>::f(); }

I don't think it would be correct to upgrade Foo<int>::f to dllexport.

[ormris]

That makes sense. Here's an updated diff.

Changelog:

• Remove dllimport instead of replacing it.
• Remove unnecessary test change.

[rnk]

include/llvm/IR/GlobalValue.h

198–199 ↗

(On Diff #178784)

This change doesn't seem quite right. On the one hand, if a GlobalValue has internal linkage, it probably shouldn't be marked dllimport. However, we really shouldn't look at visibility or external weak linkage, which are concepts that don't exist for COFF. And we probably shouldn't adjust the dll storage class from a function named maybeSetDsoLocal.

lib/LTO/LTO.cpp

698–700

This looks like the important functional change. Would the test still pass if you reverted the changes other than this one?

[ormris]

I've trimmed the patch, as suggested.

[rnk]

lgtm, thanks!

[ormris]

Thanks for the review! I'll go ahead and commit this.

lib/LTO/LTO.cpp

if (GlobalValue *GV = Msym.dyn_cast<GlobalValue *>()) {
         Mod.Keep.push_back(GV);
         GV->setLinkage(GlobalValue::AvailableExternallyLinkage);
         if (GV->hasComdat())
           NonPrevailingComdats.insert(GV->getComdat());
         cast<GlobalObject>(GV)->setComdat(nullptr);
       }
 
       // Set the 'local' flag based on the linker resolution for this symbol.
-      if (Res.FinalDefinitionInLinkageUnit)
+      if (Res.FinalDefinitionInLinkageUnit) {
         GV->setDSOLocal(true);
+        if (GV->hasDLLImportStorageClass())
+          GV->setDLLStorageClass(GlobalValue::DLLStorageClassTypes::
+                                 DefaultStorageClass);
+      }
     }
     // Common resolution: collect the maximum size/alignment over all commons.
     // We also record if we see an instance of a common as prevailing, so that
     // if none is prevailing we can ignore it later.
     if (Sym.isCommon()) {
       // FIXME: We should figure out what to do about commons defined by asm.
       // For now they aren't reported correctly by ModuleSymbolTable.
       auto &CommonRes = RegularLTO.Commons[Sym.getIRName()];

test/LTO/Resolution/X86/local-def-dllimport.ll

+; RUN: opt -thinlto-bc -o %t0.bc %s
+; RUN: llvm-lto2 run -r %t0.bc,__imp_f,l \
+; RUN:               -r %t0.bc,g,p \
+; RUN:               -r %t0.bc,g,l \
+; RUN:               -r %t0.bc,e,l \
+; RUN:               -r %t0.bc,main,x \
+; RUN:               -save-temps -o %t1 %t0.bc
+; RUN: llvm-dis %t1.1.3.import.bc -o - | FileCheck %s
+source_filename = "test.cpp"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+$g = comdat any
+@g = global i8 42, comdat, !type !0
+
+; CHECK: define
+; CHECK-NOT: dllimport
+; CHECK-SAME: @f
+define available_externally dllimport i8* @f() {
+  ret i8* @g
+}
+
+define i8* @e() {
+  ret i8* @g
+}
+
+define i32 @main() {
+  %1 = call i8* @f()
+  %2 = ptrtoint i8* %1 to i32
+  ret i32 %2
+}
+!0 = !{i32 0, !"typeid"}