[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

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

696–698

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.

include/llvm/IR/GlobalValue.h

bool mayBeDerefined() const {
     case InternalLinkage:
     case PrivateLinkage:
       return isInterposable();
     }
 
     llvm_unreachable("Fully covered switch above!");
   }
 
-  void maybeSetDsoLocal() {
-    if (hasLocalLinkage() ||
-        (!hasDefaultVisibility() && !hasExternalWeakLinkage()))
-      setDSOLocal(true);
-  }
-
 protected:
   /// The intrinsic ID for this subclass (which must be a Function).
   ///
   /// This member is defined by this class, but not used for anything.
   /// Subclasses can use it to store their intrinsic ID, if they have one.
   ///
   /// This is stored here to save space in Function on 64-bit hosts.
   Intrinsic::ID IntID;
public:
   unsigned getAddressSpace() const;
 
   enum class UnnamedAddr {
     None,
     Local,
     Global,
   };
 
+  void maybeSetDsoLocal() {
+    if (hasLocalLinkage() ||
+        (!hasDefaultVisibility() && !hasExternalWeakLinkage())) {
+      setDSOLocal(true);
+      if (hasDLLImportStorageClass())
+        setDLLStorageClass(DLLStorageClassTypes::DLLExportStorageClass);
+    }
+  }
+
   bool hasGlobalUnnamedAddr() const {
     return getUnnamedAddr() == UnnamedAddr::Global;
   }
 
   /// Returns true if this value's address is not significant in this module.
   /// This attribute is intended to be used only by the code generator and LTO
   /// to allow the linker to decide whether the global needs to be in the symbol
   /// table. It should probably not be used in optimizations, as the value may

lib/Bitcode/Reader/BitcodeReader.cpp

if (!UseStrtab) {
     Name = OldFormatName;
   }
   Comdat *C = TheModule->getOrInsertComdat(Name);
   C->setSelectionKind(SK);
   ComdatList.push_back(C);
   return Error::success();
 }
 
-static void inferDSOLocal(GlobalValue *GV) {
-  // infer dso_local from linkage and visibility if it is not encoded.
-  if (GV->hasLocalLinkage() ||
-      (!GV->hasDefaultVisibility() && !GV->hasExternalWeakLinkage()))
-    GV->setDSOLocal(true);
-}
-
 Error BitcodeReader::parseGlobalVarRecord(ArrayRef<uint64_t> Record) {
   // v1: [pointer type, isconst, initid, linkage, alignment, section,
   // visibility, threadlocal, unnamed_addr, externally_initialized,
   // dllstorageclass, comdat, attributes, preemption specifier] (name in VST)
   // v2: [strtab_offset, strtab_size, v1]
   StringRef Name;
   std::tie(Name, Record) = readNameFromStrtab(Record);
 
Error BitcodeReader::parseGlobalVarRecord(ArrayRef<uint64_t> Record) {
   if (Record.size() > 12) {
     auto AS = getAttributes(Record[12]).getFnAttributes();
     NewGV->setAttributes(AS);
   }
 
   if (Record.size() > 13) {
     NewGV->setDSOLocal(getDecodedDSOLocal(Record[13]));
   }
-  inferDSOLocal(NewGV);
+  NewGV->maybeSetDsoLocal();
 
   return Error::success();
 }
 
 Error BitcodeReader::parseFunctionRecord(ArrayRef<uint64_t> Record) {
   // v1: [type, callingconv, isproto, linkage, paramattr, alignment, section,
   // visibility, gc, unnamed_addr, prologuedata, dllstorageclass, comdat,
   // prefixdata,  personalityfn, preemption specifier, addrspace] (name in VST)
if (Record.size() > 13 && Record[13] != 0)
     FunctionPrefixes.push_back(std::make_pair(Func, Record[13] - 1));
 
   if (Record.size() > 14 && Record[14] != 0)
     FunctionPersonalityFns.push_back(std::make_pair(Func, Record[14] - 1));
 
   if (Record.size() > 15) {
     Func->setDSOLocal(getDecodedDSOLocal(Record[15]));
   }
-  inferDSOLocal(Func);
+  Func->maybeSetDsoLocal();
 
   ValueList.push_back(Func);
 
   // If this is a function with a body, remember the prototype we are
   // creating now, so that we can match up the body with them later.
   if (!isProto) {
     Func->setIsMaterializable(true);
     FunctionsWithBodies.push_back(Func);
else
       upgradeDLLImportExportLinkage(NewGA, Linkage);
     if (OpNum != Record.size())
       NewGA->setThreadLocalMode(getDecodedThreadLocalMode(Record[OpNum++]));
     if (OpNum != Record.size())
       NewGA->setUnnamedAddr(getDecodedUnnamedAddrType(Record[OpNum++]));
   }
   if (OpNum != Record.size())
     NewGA->setDSOLocal(getDecodedDSOLocal(Record[OpNum++]));
-  inferDSOLocal(NewGA);
+  NewGA->maybeSetDsoLocal();
 
   ValueList.push_back(NewGA);
   IndirectSymbolInits.push_back(std::make_pair(NewGA, Val));
   return Error::success();
 }
 
 Error BitcodeReader::parseModule(uint64_t ResumeBit,
                                  bool ShouldLazyLoadMetadata) {

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::
+                                 DLLExportStorageClass);
+      }
     }
     // 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()];

lib/Transforms/Utils/FunctionImportUtils.cpp

void FunctionImportGlobalProcessing::processGlobalForThinLTO(GlobalValue &GV) {
   // Check the summaries to see if the symbol gets resolved to a known local
   // definition.
   ValueInfo VI;
   if (GV.hasName()) {
     VI = ImportIndex.getValueInfo(GV.getGUID());
     if (VI && VI.isDSOLocal()) {
       GV.setDSOLocal(true);
       if (GV.hasDLLImportStorageClass())
-        GV.setDLLStorageClass(GlobalValue::DefaultStorageClass);
+        GV.setDLLStorageClass(GlobalValue::DLLExportStorageClass);
     }
   }
 
   // Mark read-only variables which can be imported with specific attribute.
   // We can't internalize them now because IRMover will fail to link variable
   // definitions to their external declarations during ThinLTO import. We'll
   // internalize read-only variables later, after import is finished.
   // See internalizeImmutableGVs.

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-SAME: dllexport
+; 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"}

test/ThinLTO/X86/dsolocal_dllimport.ll

 ; RUN: opt -module-summary %s -o %t1.bc
 ; RUN: opt -module-summary %S/Inputs/dsolocal_dllimport.ll -o %t2.bc
 ; RUN: llvm-lto2 run %t1.bc %t2.bc -o %t -r=%t1.bc,bar,px -r=%t1.bc,__imp_foo, -r=%t2.bc,foo -save-temps
 ; RUN: llvm-dis < %t.1.3.import.bc | FileCheck %s
 
-; If a user (dllimport) is LTOed with a library, check that we replace dllimport with dso_local.
+; If a user (dllimport) is LTOed with a library, check that we replace dllimport with dllexport and mark
+; the function as dso_local.
 
-; CHECK: declare dso_local void @foo()
+; CHECK: declare dso_local dllexport void @foo()
 
 target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-pc-windows-msvc19.11.0"
 define void @bar() {
   call void @foo()
   ret void
 }
 declare dllimport void @foo()