[IR] Don't allow DLL storage-class and local linkage

bd1976bris · bd1976bris · commit 7eee2a2d4401 · 2022-09-30T00:26:01.000+01:00
Disallow this meaningless combination. Doing so simplifies analysis of LLVM code w.r.t t DLL storage-class, and prevents mistakes with DLL storage class. - Change the assembler to reject DLL storage class on symbols with local linkage. - Change the bitcode reader to clear the DLL Storage class when the linkage is local for auto-upgrading - Update LangRef. There is an existing restriction on non-default visibility and local linkage which this is modelled on. Differential Review: https://reviews.llvm.org/D134784
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -6013,10 +6013,13 @@ ConstantAddress CodeGenModule::GetAddrOfGlobalTemporary(
       getModule(), Type, Constant, Linkage, InitialValue, Name.c_str(),
       /*InsertBefore=*/nullptr, llvm::GlobalVariable::NotThreadLocal, TargetAS);
   if (emitter) emitter->finalize(GV);
-  setGVProperties(GV, VD);
-  if (GV->getDLLStorageClass() == llvm::GlobalVariable::DLLExportStorageClass)
-    // The reference temporary should never be dllexport.
-    GV->setDLLStorageClass(llvm::GlobalVariable::DefaultStorageClass);
+  // Don't assign dllimport or dllexport to local linkage globals.
+  if (!llvm::GlobalValue::isLocalLinkage(Linkage)) {
+    setGVProperties(GV, VD);
+    if (GV->getDLLStorageClass() == llvm::GlobalVariable::DLLExportStorageClass)
+      // The reference temporary should never be dllexport.
+      GV->setDLLStorageClass(llvm::GlobalVariable::DefaultStorageClass);
+  }
   GV->setAlignment(Align.getAsAlign());
   if (supportsCOMDAT() && GV->isWeakForLinker())
     GV->setComdat(TheModule.getOrInsertComdat(GV->getName()));
diff --git a/llvm/docs/LangRef.rst b/llvm/docs/LangRef.rst
@@ -522,6 +522,9 @@ DLL storage class:
     assembler and linker know it is externally referenced and must refrain from
     deleting the symbol.
 
+A symbol with ``internal`` or ``private`` linkage cannot have a DLL storage
+class.
+
 .. _tls_model:
 
 Thread Local Storage Models
diff --git a/llvm/include/llvm/IR/GlobalValue.h b/llvm/include/llvm/IR/GlobalValue.h
@@ -275,7 +275,11 @@ class GlobalValue : public Constant {
   bool hasDLLExportStorageClass() const {
     return DllStorageClass == DLLExportStorageClass;
   }
-  void setDLLStorageClass(DLLStorageClassTypes C) { DllStorageClass = C; }
+  void setDLLStorageClass(DLLStorageClassTypes C) {
+    assert((!hasLocalLinkage() || C == DefaultStorageClass) &&
+           "local linkage requires DefaultStorageClass");
+    DllStorageClass = C;
+  }
 
   bool hasSection() const { return !getSection().empty(); }
   StringRef getSection() const;
@@ -524,8 +528,10 @@ class GlobalValue : public Constant {
   }
 
   void setLinkage(LinkageTypes LT) {
-    if (isLocalLinkage(LT))
+    if (isLocalLinkage(LT)) {
       Visibility = DefaultVisibility;
+      DllStorageClass = DefaultStorageClass;
+    }
     Linkage = LT;
     if (isImplicitDSOLocal())
       setDSOLocal(true);
diff --git a/llvm/lib/AsmParser/LLParser.cpp b/llvm/lib/AsmParser/LLParser.cpp
@@ -967,6 +967,10 @@ static bool isValidVisibilityForLinkage(unsigned V, unsigned L) {
   return !GlobalValue::isLocalLinkage((GlobalValue::LinkageTypes)L) ||
          (GlobalValue::VisibilityTypes)V == GlobalValue::DefaultVisibility;
 }
+static bool isValidDLLStorageClassForLinkage(unsigned S, unsigned L) {
+  return !GlobalValue::isLocalLinkage((GlobalValue::LinkageTypes)L) ||
+         (GlobalValue::DLLStorageClassTypes)S == GlobalValue::DefaultStorageClass;
+}
 
 // If there was an explicit dso_local, update GV. In the absence of an explicit
 // dso_local we keep the default value.
@@ -1020,6 +1024,10 @@ bool LLParser::parseAliasOrIFunc(const std::string &Name, LocTy NameLoc,
     return error(NameLoc,
                  "symbol with local linkage must have default visibility");
 
+  if (!isValidDLLStorageClassForLinkage(DLLStorageClass, L))
+    return error(NameLoc,
+                 "symbol with local linkage cannot have a DLL storage class");
+
   Type *Ty;
   LocTy ExplicitTypeLoc = Lex.getLoc();
   if (parseType(Ty) ||
@@ -1207,6 +1215,10 @@ bool LLParser::parseGlobal(const std::string &Name, LocTy NameLoc,
     return error(NameLoc,
                  "symbol with local linkage must have default visibility");
 
+  if (!isValidDLLStorageClassForLinkage(DLLStorageClass, Linkage))
+    return error(NameLoc,
+                 "symbol with local linkage cannot have a DLL storage class");
+
   unsigned AddrSpace;
   bool IsConstant, IsExternallyInitialized;
   LocTy IsExternallyInitializedLoc;
@@ -5656,6 +5668,10 @@ bool LLParser::parseFunctionHeader(Function *&Fn, bool IsDefine) {
     return error(LinkageLoc,
                  "symbol with local linkage must have default visibility");
 
+  if (!isValidDLLStorageClassForLinkage(DLLStorageClass, Linkage))
+    return error(LinkageLoc,
+                 "symbol with local linkage cannot have a DLL storage class");
+
   if (!FunctionType::isValidReturnType(RetType))
     return error(RetTypeLoc, "invalid function return type");
 
diff --git a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
@@ -1298,6 +1298,9 @@ static FastMathFlags getDecodedFastMathFlags(unsigned Val) {
 }
 
 static void upgradeDLLImportExportLinkage(GlobalValue *GV, unsigned Val) {
+  // A GlobalValue with local linkage cannot have a DLL storage class.
+  if (GV->hasLocalLinkage())
+    return;
   switch (Val) {
   case 5: GV->setDLLStorageClass(GlobalValue::DLLImportStorageClass); break;
   case 6: GV->setDLLStorageClass(GlobalValue::DLLExportStorageClass); break;
@@ -3764,10 +3767,14 @@ Error BitcodeReader::parseGlobalVarRecord(ArrayRef<uint64_t> Record) {
   NewGV->setVisibility(Visibility);
   NewGV->setUnnamedAddr(UnnamedAddr);
 
-  if (Record.size() > 10)
-    NewGV->setDLLStorageClass(getDecodedDLLStorageClass(Record[10]));
-  else
+  if (Record.size() > 10) {
+    // A GlobalValue with local linkage cannot have a DLL storage class.
+    if (!NewGV->hasLocalLinkage()) {
+      NewGV->setDLLStorageClass(getDecodedDLLStorageClass(Record[10]));
+    }
+  } else {
     upgradeDLLImportExportLinkage(NewGV, RawLinkage);
+  }
 
   ValueList.push_back(NewGV, getVirtualTypeID(NewGV->getType(), TyID));
 
@@ -3928,10 +3935,14 @@ Error BitcodeReader::parseFunctionRecord(ArrayRef<uint64_t> Record) {
   if (Record.size() > 10)
     OperandInfo.Prologue = Record[10];
 
-  if (Record.size() > 11)
-    Func->setDLLStorageClass(getDecodedDLLStorageClass(Record[11]));
-  else
+  if (Record.size() > 11) {
+    // A GlobalValue with local linkage cannot have a DLL storage class.
+    if (!Func->hasLocalLinkage()) {
+      Func->setDLLStorageClass(getDecodedDLLStorageClass(Record[11]));
+    }
+  } else {
     upgradeDLLImportExportLinkage(Func, RawLinkage);
+  }
 
   if (Record.size() > 12) {
     if (unsigned ComdatID = Record[12]) {
@@ -4034,8 +4045,12 @@ Error BitcodeReader::parseGlobalIndirectSymbolRecord(
   }
   if (BitCode == bitc::MODULE_CODE_ALIAS ||
       BitCode == bitc::MODULE_CODE_ALIAS_OLD) {
-    if (OpNum != Record.size())
-      NewGA->setDLLStorageClass(getDecodedDLLStorageClass(Record[OpNum++]));
+    if (OpNum != Record.size()) {
+      auto S = Record[OpNum++];
+      // A GlobalValue with local linkage cannot have a DLL storage class.
+      if (!NewGA->hasLocalLinkage())
+        NewGA->setDLLStorageClass(getDecodedDLLStorageClass(S));
+    }
     else
       upgradeDLLImportExportLinkage(NewGA, Linkage);
     if (OpNum != Record.size())
diff --git a/llvm/test/Assembler/dll-storage-class-local-linkage.ll b/llvm/test/Assembler/dll-storage-class-local-linkage.ll
@@ -0,0 +1,88 @@
+;; Check that an error is emitted for local linkage symbols with DLL storage class.
+
+; RUN: rm -rf %t && split-file %s %t
+
+; RUN: not llvm-as %t/internal_function_dllexport.ll -o /dev/null 2>&1 | FileCheck %s
+; RUN: not llvm-as %t/internal_variable_dllexport.ll -o /dev/null 2>&1 | FileCheck %s
+; RUN: not llvm-as %t/internal_alias_dllexport.ll -o /dev/null 2>&1 | FileCheck %s
+
+; RUN: not llvm-as %t/private_function_dllexport.ll -o /dev/null 2>&1 | FileCheck %s
+; RUN: not llvm-as %t/private_variable_dllexport.ll -o /dev/null 2>&1 | FileCheck %s
+; RUN: not llvm-as %t/private_alias_dllexport.ll -o /dev/null 2>&1 | FileCheck %s
+
+; RUN: not llvm-as %t/internal_function_dllimport.ll -o /dev/null 2>&1 | FileCheck %s
+; RUN: not llvm-as %t/internal_variable_dllimport.ll -o /dev/null 2>&1 | FileCheck %s
+; RUN: not llvm-as %t/internal_alias_dllimport.ll -o /dev/null 2>&1 | FileCheck %s
+
+; RUN: not llvm-as %t/private_function_dllimport.ll -o /dev/null 2>&1 | FileCheck %s
+; RUN: not llvm-as %t/private_variable_dllimport.ll -o /dev/null 2>&1 | FileCheck %s
+; RUN: not llvm-as %t/private_alias_dllimport.ll -o /dev/null 2>&1 | FileCheck %s
+
+
+; CHECK: symbol with local linkage cannot have a DLL storage class
+
+
+;--- internal_function_dllexport.ll
+
+define internal dllexport void @function() {
+entry:
+  ret void
+}
+
+;--- internal_variable_dllexport.ll
+
+@var = internal dllexport global i32 0
+
+;--- internal_alias_dllexport.ll
+
+@global = global i32 0
+@alias = internal dllexport alias i32, i32* @global
+
+;--- private_function_dllexport.ll
+
+define private dllexport void @function() {
+entry:
+  ret void
+}
+
+;--- private_variable_dllexport.ll
+
+@var = private dllexport global i32 0
+
+;--- private_alias_dllexport.ll
+
+@global = global i32 0
+@alias = private dllexport alias i32, i32* @global
+
+
+;--- internal_function_dllimport.ll
+
+define internal dllimport void @function() {
+entry:
+  ret void
+}
+
+;--- internal_variable_dllimport.ll
+
+@var = internal dllimport global i32 0
+
+;--- internal_alias_dllimport.ll
+
+@global = global i32 0
+@alias = internal dllimport alias i32, i32* @global
+
+;--- private_function_dllimport.ll
+
+define private dllimport void @function() {
+entry:
+  ret void
+}
+
+;--- private_variable_dllimport.ll
+
+@var = private dllimport global i32 0
+
+;--- private_alias_dllimport.ll
+
+@global = global i32 0
+@alias = private dllimport alias i32, i32* @global
diff --git a/llvm/test/Transforms/GlobalOpt/alias-used-section.ll b/llvm/test/Transforms/GlobalOpt/alias-used-section.ll
@@ -1,8 +1,8 @@
 ; RUN: opt -S -passes=globalopt < %s | FileCheck %s
 
 @_Z17in_custom_section = internal global i8 42, section "CUSTOM"
-@in_custom_section = internal dllexport alias i8, i8* @_Z17in_custom_section
+@in_custom_section = internal alias i8, i8* @_Z17in_custom_section
 
-; CHECK: @in_custom_section = internal dllexport global i8 42, section "CUSTOM"
+; CHECK: @in_custom_section = internal global i8 42, section "CUSTOM"
 
 @llvm.used = appending global [1 x i8*] [i8* @in_custom_section], section "llvm.metadata"
