From 7eee2a2d4401813cd485ae708e8cb0f94469e037 Mon Sep 17 00:00:00 2001 From: Ben Dunbobbin Date: Fri, 30 Sep 2022 00:21:31 +0100 Subject: [PATCH] [IR] Don't allow DLL storage-class and local linkage 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 --- clang/lib/CodeGen/CodeGenModule.cpp | 11 ++- llvm/docs/LangRef.rst | 3 + llvm/include/llvm/IR/GlobalValue.h | 10 ++- llvm/lib/AsmParser/LLParser.cpp | 16 ++++ llvm/lib/Bitcode/Reader/BitcodeReader.cpp | 31 ++++++-- .../Assembler/dll-storage-class-local-linkage.ll | 88 ++++++++++++++++++++++ .../Transforms/GlobalOpt/alias-used-section.ll | 4 +- 7 files changed, 147 insertions(+), 16 deletions(-) create mode 100644 llvm/test/Assembler/dll-storage-class-local-linkage.ll diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp index 8feb673..92920da 100644 --- 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 index 5b66ac0..1778a0a 100644 --- 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 index 63c5d1b..db29092 100644 --- a/llvm/include/llvm/IR/GlobalValue.h +++ b/llvm/include/llvm/IR/GlobalValue.h @@ -275,7 +275,11 @@ public: 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 @@ public: } 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 index 23445f4a..1b072f8 100644 --- 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 index 0ed0614..bfce79c 100644 --- 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 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 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 new file mode 100644 index 0000000..67429ef --- /dev/null +++ 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 index 389d77a..e0ef39d8 100644 --- 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" -- 2.7.4