From 49135bb76cba6792ad8abd52927a87049b730a4a Mon Sep 17 00:00:00 2001 From: "Duncan P. N. Exon Smith" Date: Tue, 12 Aug 2014 16:46:37 +0000 Subject: [PATCH] Don't upgrade global constructors when reading bitcode An optional third field was added to `llvm.global_ctors` (and `llvm.global_dtors`) in r209015. Most of the code has been changed to deal with both versions of the variables. Users of the C API might create either version, the helper functions in LLVM create the two-field version, and clang now creates the three-field version. However, the BitcodeReader was changed to always upgrade to the three-field version. This created an unnecessary inconsistency in the IR before/after serializing to bitcode. This commit resolves the inconsistency by making the third field truly optional (and not upgrading in the bitcode reader). Since `llvm-link` was relying on this upgrade code, rather than deleting it I've moved it into `ModuleLinker`, where it upgrades these arrays as necessary to resolve inconsistencies between modules. The ideal resolution would be to remove the 2-field version and make the third field required. I filed PR20506 to track that. I changed `test/Bitcode/upgrade-global-ctors.ll` to a negative test and duplicated the `llvm-link` check in `test/Linker/global_ctors.ll` to check both upgrade directions. Since I came across this as part of PR5680 (serializing use-list order), I've also added the missing `verify-uselistorder` RUN line to `test/Bitcode/metadata-2.ll`. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@215457 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/IR/AutoUpgrade.cpp | 55 ------------------------ lib/Linker/LinkModules.cpp | 83 ++++++++++++++++++++++++++++++++++++ test/Bitcode/metadata-2.ll | 1 + test/Bitcode/upgrade-global-ctors.ll | 3 +- test/Linker/global_ctors.ll | 1 + 5 files changed, 87 insertions(+), 56 deletions(-) diff --git a/lib/IR/AutoUpgrade.cpp b/lib/IR/AutoUpgrade.cpp index 459bd880ccb..2b62ea24cec 100644 --- a/lib/IR/AutoUpgrade.cpp +++ b/lib/IR/AutoUpgrade.cpp @@ -173,62 +173,7 @@ bool llvm::UpgradeIntrinsicFunction(Function *F, Function *&NewFn) { return Upgraded; } -static bool UpgradeGlobalStructors(GlobalVariable *GV) { - ArrayType *ATy = dyn_cast(GV->getType()->getElementType()); - StructType *OldTy = - ATy ? dyn_cast(ATy->getElementType()) : nullptr; - - // Only upgrade an array of a two field struct with the appropriate field - // types. - if (!OldTy || OldTy->getNumElements() != 2) - return false; - - // Get the upgraded 3 element type. - PointerType *VoidPtrTy = Type::getInt8Ty(GV->getContext())->getPointerTo(); - Type *Tys[3] = { - OldTy->getElementType(0), - OldTy->getElementType(1), - VoidPtrTy - }; - StructType *NewTy = - StructType::get(GV->getContext(), Tys, /*isPacked=*/false); - - // Build new constants with a null third field filled in. - Constant *OldInitC = GV->getInitializer(); - ConstantArray *OldInit = dyn_cast(OldInitC); - if (!OldInit && !isa(OldInitC)) - return false; - std::vector Initializers; - if (OldInit) { - for (Use &U : OldInit->operands()) { - ConstantStruct *Init = cast(&U); - Constant *NewInit = - ConstantStruct::get(NewTy, Init->getOperand(0), Init->getOperand(1), - Constant::getNullValue(VoidPtrTy), nullptr); - Initializers.push_back(NewInit); - } - } - assert(Initializers.size() == ATy->getNumElements()); - - // Replace the old GV with a new one. - ATy = ArrayType::get(NewTy, Initializers.size()); - Constant *NewInit = ConstantArray::get(ATy, Initializers); - GlobalVariable *NewGV = new GlobalVariable( - *GV->getParent(), ATy, GV->isConstant(), GV->getLinkage(), NewInit, "", - GV, GV->getThreadLocalMode(), GV->getType()->getAddressSpace(), - GV->isExternallyInitialized()); - NewGV->copyAttributesFrom(GV); - NewGV->takeName(GV); - assert(GV->use_empty() && "program cannot use initializer list"); - GV->eraseFromParent(); - return true; -} - bool llvm::UpgradeGlobalVariable(GlobalVariable *GV) { - if (GV->getName() == "llvm.global_ctors" || - GV->getName() == "llvm.global_dtors") - return UpgradeGlobalStructors(GV); - // Nothing to do yet. return false; } diff --git a/lib/Linker/LinkModules.cpp b/lib/Linker/LinkModules.cpp index c83774f8d03..1970e7fa069 100644 --- a/lib/Linker/LinkModules.cpp +++ b/lib/Linker/LinkModules.cpp @@ -469,6 +469,9 @@ namespace { void computeTypeMapping(); + void upgradeMismatchedGlobalArray(StringRef Name); + void upgradeMismatchedGlobals(); + bool linkAppendingVarProto(GlobalVariable *DstGV, GlobalVariable *SrcGV); bool linkGlobalProto(GlobalVariable *SrcGV); bool linkFunctionProto(Function *SrcF); @@ -816,6 +819,83 @@ void ModuleLinker::computeTypeMapping() { TypeMap.linkDefinedTypeBodies(); } +static void upgradeGlobalArray(GlobalVariable *GV) { + ArrayType *ATy = cast(GV->getType()->getElementType()); + StructType *OldTy = cast(ATy->getElementType()); + assert(OldTy->getNumElements() == 2 && "Expected to upgrade from 2 elements"); + + // Get the upgraded 3 element type. + PointerType *VoidPtrTy = Type::getInt8Ty(GV->getContext())->getPointerTo(); + Type *Tys[3] = {OldTy->getElementType(0), OldTy->getElementType(1), + VoidPtrTy}; + StructType *NewTy = StructType::get(GV->getContext(), Tys, false); + + // Build new constants with a null third field filled in. + Constant *OldInitC = GV->getInitializer(); + ConstantArray *OldInit = dyn_cast(OldInitC); + if (!OldInit && !isa(OldInitC)) + // Invalid initializer; give up. + return; + std::vector Initializers; + if (OldInit && OldInit->getNumOperands()) { + Value *Null = Constant::getNullValue(VoidPtrTy); + for (Use &U : OldInit->operands()) { + ConstantStruct *Init = cast(U.get()); + Initializers.push_back(ConstantStruct::get( + NewTy, Init->getOperand(0), Init->getOperand(1), Null, nullptr)); + } + } + assert(Initializers.size() == ATy->getNumElements() && + "Failed to copy all array elements"); + + // Replace the old GV with a new one. + ATy = ArrayType::get(NewTy, Initializers.size()); + Constant *NewInit = ConstantArray::get(ATy, Initializers); + GlobalVariable *NewGV = new GlobalVariable( + *GV->getParent(), ATy, GV->isConstant(), GV->getLinkage(), NewInit, "", + GV, GV->getThreadLocalMode(), GV->getType()->getAddressSpace(), + GV->isExternallyInitialized()); + NewGV->copyAttributesFrom(GV); + NewGV->takeName(GV); + assert(GV->use_empty() && "program cannot use initializer list"); + GV->eraseFromParent(); +} + +void ModuleLinker::upgradeMismatchedGlobalArray(StringRef Name) { + // Look for the global arrays. + auto *DstGV = dyn_cast_or_null(DstM->getNamedValue(Name)); + if (!DstGV) + return; + auto *SrcGV = dyn_cast_or_null(SrcM->getNamedValue(Name)); + if (!SrcGV) + return; + + // Check if the types already match. + auto *DstTy = cast(DstGV->getType()->getElementType()); + auto *SrcTy = + cast(TypeMap.get(SrcGV->getType()->getElementType())); + if (DstTy == SrcTy) + return; + + // Grab the element types. We can only upgrade an array of a two-field + // struct. Only bother if the other one has three-fields. + auto *DstEltTy = cast(DstTy->getElementType()); + auto *SrcEltTy = cast(SrcTy->getElementType()); + if (DstEltTy->getNumElements() == 2 && SrcEltTy->getNumElements() == 3) { + upgradeGlobalArray(DstGV); + return; + } + if (DstEltTy->getNumElements() == 3 && SrcEltTy->getNumElements() == 2) + upgradeGlobalArray(SrcGV); + + // We can't upgrade any other differences. +} + +void ModuleLinker::upgradeMismatchedGlobals() { + upgradeMismatchedGlobalArray("llvm.global_ctors"); + upgradeMismatchedGlobalArray("llvm.global_dtors"); +} + /// linkAppendingVarProto - If there were any appending global variables, link /// them together now. Return true on error. bool ModuleLinker::linkAppendingVarProto(GlobalVariable *DstGV, @@ -1454,6 +1534,9 @@ bool ModuleLinker::run() { ComdatsChosen[&C] = std::make_pair(SK, LinkFromSrc); } + // Upgrade mismatched global arrays. + upgradeMismatchedGlobals(); + // Insert all of the globals in src into the DstM module... without linking // initializers (which could refer to functions not yet mapped over). for (Module::global_iterator I = SrcM->global_begin(), diff --git a/test/Bitcode/metadata-2.ll b/test/Bitcode/metadata-2.ll index 4055f921c33..06f844283ff 100644 --- a/test/Bitcode/metadata-2.ll +++ b/test/Bitcode/metadata-2.ll @@ -1,4 +1,5 @@ ; RUN: llvm-as < %s | llvm-dis -disable-output +; RUN: verify-uselistorder < %s -preserve-bc-use-list-order %0 = type { %object.ModuleInfo.__vtbl*, i8*, %"byte[]", %1, %"ClassInfo[]", i32, void ()*, void ()*, void ()*, i8*, void ()* } ; type %0 %1 = type { i64, %object.ModuleInfo* } ; type %1 %2 = type { i32, void ()* } ; type %2 diff --git a/test/Bitcode/upgrade-global-ctors.ll b/test/Bitcode/upgrade-global-ctors.ll index 9ba422fe491..4816f0dc294 100644 --- a/test/Bitcode/upgrade-global-ctors.ll +++ b/test/Bitcode/upgrade-global-ctors.ll @@ -1,4 +1,5 @@ ; RUN: llvm-dis < %s.bc| FileCheck %s ; RUN: verify-uselistorder < %s.bc -preserve-bc-use-list-order -; CHECK: @llvm.global_ctors = appending global [0 x { i32, void ()*, i8* }] zeroinitializer +; Global constructors should no longer be upgraded when reading bitcode. +; CHECK: @llvm.global_ctors = appending global [0 x { i32, void ()* }] zeroinitializer diff --git a/test/Linker/global_ctors.ll b/test/Linker/global_ctors.ll index 541f0d4f91b..49df81a0075 100644 --- a/test/Linker/global_ctors.ll +++ b/test/Linker/global_ctors.ll @@ -1,5 +1,6 @@ ; RUN: llvm-as %s -o %t.new.bc ; RUN: llvm-link %t.new.bc %S/Inputs/old_global_ctors.3.4.bc | llvm-dis | FileCheck %s +; RUN: llvm-link %S/Inputs/old_global_ctors.3.4.bc %t.new.bc | llvm-dis | FileCheck %s ; old_global_ctors.3.4.bc contains the following LLVM IL, assembled into ; bitcode by llvm-as from 3.4. It uses a two element @llvm.global_ctors array. -- 2.11.0