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

[bd1976llvm]

Summary

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.

[MaskRay]

clang/lib/CodeGen/CodeGenModule.cpp

6016

Prevent local linkage globals from ... or just "Don't assign ... to local linkage ..."

6017

llvm::GlobalValue::isLocalLinkage(Linkage) ?

This change is to make CodeGenCXX/reference-temporary-ms.cpp work

llvm/include/llvm/IR/GlobalValue.h

279

[bd1976llvm]

Thanks for the review. I addressed your comments and also consolidated the tests into a single test using split-file.

clang/lib/CodeGen/CodeGenModule.cpp

6017

Thanks. Yes, CodeGenCXX/reference-temporary-ms.cpp will fail without this change. It also just makes sense that we don't need to resolve the properties of a local linkage GV.

[MaskRay]

LGTM.
For LLVM IR, the more rigid form helps.
For Clang, it seems that MSVC allows dllspec on a local linkage variable, so that's why we have to add it to CodeGen instead of rejecting it in Sema.

llvm/lib/AsmParser/LLParser.cpp

1182

Should Storage be storage?

must not or cannot ?

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

1301

cannot be used together with

or

A GlobalValue of a local linkage cannot have a ...

3771

ditto

3774

ensure both then and else branches have braces

3941

ditto

[bd1976llvm]

In D134784#3821336, @MaskRay wrote:

For Clang, it seems that MSVC allows dllspec on a local linkage variable, so that's why we have to add it to CodeGen instead of rejecting it in Sema.

Can you give an example? MSVC certainly rejects simple cases e.g. https://godbolt.org/z/bG3M7oj5W. I am worried that I have missed something...

[MaskRay]

In D134784#3822303, @bd1976llvm wrote:

In D134784#3821336, @MaskRay wrote:

For Clang, it seems that MSVC allows dllspec on a local linkage variable, so that's why we have to add it to CodeGen instead of rejecting it in Sema.

Can you give an example? MSVC certainly rejects simple cases e.g. https://godbolt.org/z/bG3M7oj5W. I am worried that I have missed something...

__declspec(dllexport) const int foo = 10; foo with internal linkage is accepted.

[rnk]

In D134784#3822333, @MaskRay wrote:

__declspec(dllexport) const int foo = 10; foo with internal linkage is accepted.

I believe in this case MSVC will make the symbol external, so it's not actually internal. The dll attributes tend to imply extern in many cases.

[bd1976llvm]

In D134784#3822333, @MaskRay wrote:

In D134784#3822303, @bd1976llvm wrote:

In D134784#3821336, @MaskRay wrote:

For Clang, it seems that MSVC allows dllspec on a local linkage variable, so that's why we have to add it to CodeGen instead of rejecting it in Sema.

Can you give an example? MSVC certainly rejects simple cases e.g. https://godbolt.org/z/bG3M7oj5W. I am worried that I have missed something...

__declspec(dllexport) const int foo = 10; foo with internal linkage is accepted.

Thanks. I have reviewed the discussion here: https://reviews.llvm.org/D45978 and I experimented with the MSVC toolchain.

In D134784#3824309, @rnk wrote:

In D134784#3822333, @MaskRay wrote:

__declspec(dllexport) const int foo = 10; foo with internal linkage is accepted.

I believe in this case MSVC will make the symbol external, so it's not actually internal. The dll attributes tend to imply extern in many cases.

Thanks. Interestingly, MinGW seems to reject such constructs which is also the behaviour that we implement for PlayStation.

In conclusion, as MaskRay alluded to upthread, this anomaly means that this restriction can't be applied during Sema. However, it does make sense for the IR.

[bd1976llvm]

Address final review comments.

clang/lib/CodeGen/CodeGenModule.cpp

if (VD->isStaticDataMember() && VD->getAnyInitializer(InitVD) &&
       Linkage = llvm::GlobalVariable::InternalLinkage;
     }
   }
   auto TargetAS = getContext().getTargetAddressSpace(AddrSpace);
   auto *GV = new llvm::GlobalVariable(
       getModule(), Type, Constant, Linkage, InitialValue, Name.c_str(),
       /*InsertBefore=*/nullptr, llvm::GlobalVariable::NotThreadLocal, TargetAS);
   if (emitter) emitter->finalize(GV);
+  // Prevent InternalLinkage) globals getting assigned a dllimport or dllexport.
+  if (Linkage != llvm::GlobalVariable::InternalLinkage) {
     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()));
   if (VD->getTLSKind())
     setTLSMode(GV, *VD);
   llvm::Constant *CV = GV;
   if (AddrSpace != LangAS::Default)
     CV = getTargetCodeGenInfo().performAddrSpaceCast(

llvm/docs/LangRef.rst

``dllexport``
     ``dllimport`` attribute. the pointer name is formed by combining ``__imp_``
     and the function or variable name. On XCOFF targets, ``dllexport`` indicates
     that the symbol will be made visible to other modules using "exported"
     visibility and thus placed by the linker in the loader section symbol table.
     Since this storage class exists for defining a dll interface, the compiler,
     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
 ---------------------------
 
 A variable may be defined as ``thread_local``, which means that it will
 not be shared by threads (each thread will have a separated copy of the
 variable). Not all targets support thread-local variables. Optionally, a

llvm/include/llvm/IR/GlobalValue.h

DLLStorageClassTypes getDLLStorageClass() const {
     return DLLStorageClassTypes(DllStorageClass);
   }
   bool hasDLLImportStorageClass() const {
     return DllStorageClass == DLLImportStorageClass;
   }
   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;
 
   /// Global values are always pointers.
   PointerType *getType() const { return cast<PointerType>(User::getType()); }
 
   Type *getValueType() const { return ValueType; }
bool hasExternalWeakLinkage() const {
     return isExternalWeakLinkage(getLinkage());
   }
   bool hasCommonLinkage() const { return isCommonLinkage(getLinkage()); }
   bool hasValidDeclarationLinkage() const {
     return isValidDeclarationLinkage(getLinkage());
   }
 
   void setLinkage(LinkageTypes LT) {
-    if (isLocalLinkage(LT))
+    if (isLocalLinkage(LT)) {
       Visibility = DefaultVisibility;
+      DllStorageClass = DefaultStorageClass;
+    }
     Linkage = LT;
     if (isImplicitDSOLocal())
       setDSOLocal(true);
   }
   LinkageTypes getLinkage() const { return LinkageTypes(Linkage); }
 
   bool isDiscardableIfUnused() const {
     return isDiscardableIfUnused(getLinkage());

llvm/lib/AsmParser/LLParser.cpp

bool LLParser::parseSummaryEntry() {
   Lex.setIgnoreColonInIdentifiers(false);
   return result;
 }
 
 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.
 static void maybeSetDSOLocal(bool DSOLocal, GlobalValue &GV) {
   if (DSOLocal)
     GV.setDSOLocal(true);
 }
 
bool LLParser::parseAliasOrIFunc(const std::string &Name, LocTy NameLoc,
 
   if(IsAlias && !GlobalAlias::isValidLinkage(Linkage))
     return error(NameLoc, "invalid linkage type for alias");
 
   if (!isValidVisibilityForLinkage(Visibility, L))
     return error(NameLoc,
                  "symbol with local linkage must have default visibility");
 
+  if (!isValidDLLStorageClassForLinkage(DLLStorageClass, L))
+    return error(NameLoc,
+                 "symbol with local linkage must not have a DLL Storage class");
+
   Type *Ty;
   LocTy ExplicitTypeLoc = Lex.getLoc();
   if (parseType(Ty) ||
       parseToken(lltok::comma, "expected comma after alias or ifunc's type"))
     return true;
 
   Constant *Aliasee;
   LocTy AliaseeLoc = Lex.getLoc();
bool LLParser::parseGlobal(const std::string &Name, LocTy NameLoc,
                            unsigned Linkage, bool HasLinkage,
                            unsigned Visibility, unsigned DLLStorageClass,
                            bool DSOLocal, GlobalVariable::ThreadLocalMode TLM,
                            GlobalVariable::UnnamedAddr UnnamedAddr) {
   if (!isValidVisibilityForLinkage(Visibility, Linkage))
     return error(NameLoc,
                  "symbol with local linkage must have default visibility");
 
+  if (!isValidDLLStorageClassForLinkage(DLLStorageClass, Linkage))
+    return error(NameLoc,
+                 "symbol with local linkage must not have a DLL Storage class");
+
   unsigned AddrSpace;
   bool IsConstant, IsExternallyInitialized;
   LocTy IsExternallyInitializedLoc;
   LocTy TyLoc;
 
   Type *Ty = nullptr;
   if (parseOptionalAddrSpace(AddrSpace) ||
       parseOptionalToken(lltok::kw_externally_initialized,
bool LLParser::parseFunctionHeader(Function *&Fn, bool IsDefine) {
   case GlobalValue::CommonLinkage:
     return error(LinkageLoc, "invalid function linkage type");
   }
 
   if (!isValidVisibilityForLinkage(Visibility, Linkage))
     return error(LinkageLoc,
                  "symbol with local linkage must have default visibility");
 
+  if (!isValidDLLStorageClassForLinkage(DLLStorageClass, Linkage))
+    return error(LinkageLoc,
+                 "symbol with local linkage must not have a DLL Storage class");
+
   if (!FunctionType::isValidReturnType(RetType))
     return error(RetTypeLoc, "invalid function return type");
 
   LocTy NameLoc = Lex.getLoc();
 
   std::string FunctionName;
   if (Lex.getKind() == lltok::GlobalVar) {
     FunctionName = Lex.getStrVal();

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

static FastMathFlags getDecodedFastMathFlags(unsigned Val) {
   if (0 != (Val & bitc::AllowContract))
     FMF.setAllowContract(true);
   if (0 != (Val & bitc::ApproxFunc))
     FMF.setApproxFunc();
   return FMF;
 }
 
 static void upgradeDLLImportExportLinkage(GlobalValue *GV, unsigned Val) {
+  // Local linkage must not 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;
   }
 }
 
 Type *BitcodeReader::getTypeByID(unsigned ID) {
   // The type table size is always specified correctly.
GlobalVariable *NewGV =
       new GlobalVariable(*TheModule, Ty, isConstant, Linkage, nullptr, Name,
                          nullptr, TLM, AddressSpace, ExternallyInitialized);
   NewGV->setAlignment(Alignment);
   if (!Section.empty())
     NewGV->setSection(Section);
   NewGV->setVisibility(Visibility);
   NewGV->setUnnamedAddr(UnnamedAddr);
 
-  if (Record.size() > 10)
+  if (Record.size() > 10) {
+    // Local linkage should not have a DLL Storage class.
+    if (!NewGV->hasLocalLinkage())
       NewGV->setDLLStorageClass(getDecodedDLLStorageClass(Record[10]));
-  else
+  } else
     upgradeDLLImportExportLinkage(NewGV, RawLinkage);
 
   ValueList.push_back(NewGV, getVirtualTypeID(NewGV->getType(), TyID));
 
   // Remember which value to use for the global initializer.
   if (unsigned InitID = Record[2])
     GlobalInits.push_back(std::make_pair(NewGV, InitID - 1));
 
Error BitcodeReader::parseFunctionRecord(ArrayRef<uint64_t> Record) {
   if (Record.size() > 9)
     UnnamedAddr = getDecodedUnnamedAddrType(Record[9]);
   Func->setUnnamedAddr(UnnamedAddr);
 
   FunctionOperandInfo OperandInfo = {Func, 0, 0, 0};
   if (Record.size() > 10)
     OperandInfo.Prologue = Record[10];
 
-  if (Record.size() > 11)
+  if (Record.size() > 11) {
+    // Local linkage must not have a DLL Storage class.
+    if (!Func->hasLocalLinkage())
       Func->setDLLStorageClass(getDecodedDLLStorageClass(Record[11]));
-  else
+  } else
     upgradeDLLImportExportLinkage(Func, RawLinkage);
 
   if (Record.size() > 12) {
     if (unsigned ComdatID = Record[12]) {
       if (ComdatID > ComdatList.size())
         return error("Invalid function comdat ID");
       Func->setComdat(ComdatList[ComdatID - 1]);
     }
   } else if (hasImplicitComdat(RawLinkage)) {
Error BitcodeReader::parseGlobalIndirectSymbolRecord(
   // auto-upgrade `hidden` and `protected` for old bitcode.
   if (OpNum != Record.size()) {
     auto VisInd = OpNum++;
     if (!NewGA->hasLocalLinkage())
       NewGA->setVisibility(getDecodedVisibility(Record[VisInd]));
   }
   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++];
+      // Local linkage must not have a DLL Storage class.
+      if (!NewGA->hasLocalLinkage())
+        NewGA->setDLLStorageClass(getDecodedDLLStorageClass(S));
+    }
     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())

llvm/test/Assembler/internal-dllexport-alias.ll

+; RUN: not llvm-as < %s -o /dev/null 2>&1 | FileCheck %s
+
+@global = global i32 0
+
+@alias = internal dllexport alias i32, i32* @global
+; CHECK: symbol with local linkage must not have a DLL Storage class

llvm/test/Assembler/internal-dllexport-function.ll

+; RUN: not llvm-as < %s -o /dev/null 2>&1 | FileCheck %s
+
+define internal dllexport void @function() {
+; CHECK: symbol with local linkage must not have a DLL Storage class
+entry:
+  ret void
+}

llvm/test/Assembler/internal-dllexport-variable.ll

+; RUN: not llvm-as < %s -o /dev/null 2>&1 | FileCheck %s
+
+@var = internal dllexport global i32 0
+; CHECK: symbol with local linkage must not have a DLL Storage class

llvm/test/Assembler/internal-dllimport-function.ll

+; RUN: not llvm-as < %s -o /dev/null 2>&1 | FileCheck %s
+
+define internal dllimport void @function() {
+; CHECK: symbol with local linkage must not have a DLL Storage class
+entry:
+  ret void
+}

llvm/test/Assembler/private-dllexport-alias.ll

+; RUN: not llvm-as < %s -o /dev/null 2>&1 | FileCheck %s
+
+@global = global i32 0
+
+@alias = private dllexport alias i32, i32* @global
+; CHECK: symbol with local linkage must not have a DLL Storage class

llvm/test/Assembler/private-dllexport-function.ll

+; RUN: not llvm-as < %s -o /dev/null 2>&1 | FileCheck %s
+
+define private dllexport void @function() {
+; CHECK: symbol with local linkage must not have a DLL Storage class
+entry:
+  ret void
+}

llvm/test/Assembler/private-dllexport-variable.ll

+; RUN: not llvm-as < %s -o /dev/null 2>&1 | FileCheck %s
+
+@var = private dllexport global i32 0
+; CHECK: symbol with local linkage must not have a DLL Storage class

llvm/test/Transforms/GlobalOpt/alias-used-section.ll

 ; 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"