From e49df9b58fa98543c487e016a3d7771647b54ff4 Mon Sep 17 00:00:00 2001 From: Daniel Dunbar Date: Wed, 30 Jul 2008 16:32:24 +0000 Subject: [PATCH] Change CodeGenModule GlobalDeclMap to directly reference globals instead of mapping the decl to a bitcast of the global to the correct type. - GetAddrOf{Function,GlobalVar} introduce the bitcast on every use now. - This solves a problem where a dangling pointer could be introduced by the RAUW done when replacing a forward or tentative definition. See testcase for more details. - Fixes llvm-svn: 54211 --- clang/lib/CodeGen/CodeGenModule.cpp | 87 ++++++++++--------- clang/lib/CodeGen/CodeGenModule.h | 9 +- .../2008-07-30-redef-of-bitcasted-decl.c | 28 ++++++ 3 files changed, 83 insertions(+), 41 deletions(-) create mode 100644 clang/test/CodeGen/2008-07-30-redef-of-bitcasted-decl.c diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp index 1e91ac2f2526..a65c2b3807d6 100644 --- a/clang/lib/CodeGen/CodeGenModule.cpp +++ b/clang/lib/CodeGen/CodeGenModule.cpp @@ -476,9 +476,9 @@ llvm::Constant *CodeGenModule::EmitAnnotateAttr(llvm::GlobalValue *GV, /// ReplaceMapValuesWith - This is a really slow and bad function that /// searches for any entries in GlobalDeclMap that point to OldVal, changing /// them to point to NewVal. This is badbadbad, FIXME! -void CodeGenModule::ReplaceMapValuesWith(llvm::Constant *OldVal, - llvm::Constant *NewVal) { - for (llvm::DenseMap::iterator +void CodeGenModule::ReplaceMapValuesWith(llvm::GlobalValue *OldVal, + llvm::GlobalValue *NewVal) { + for (llvm::DenseMap::iterator I = GlobalDeclMap.begin(), E = GlobalDeclMap.end(); I != E; ++I) if (I->second == OldVal) I->second = NewVal; } @@ -525,38 +525,41 @@ void CodeGenModule::EmitGlobalDefinition(const ValueDecl *D) { } } -llvm::Constant *CodeGenModule::GetAddrOfGlobalVar(const VarDecl *D) { + llvm::Constant *CodeGenModule::GetAddrOfGlobalVar(const VarDecl *D) { assert(D->hasGlobalStorage() && "Not a global variable"); - // See if it is already in the map. - llvm::Constant *&Entry = GlobalDeclMap[D]; - if (Entry) return Entry; - QualType ASTTy = D->getType(); const llvm::Type *Ty = getTypes().ConvertTypeForMem(ASTTy); + const llvm::Type *PTy = llvm::PointerType::get(Ty, ASTTy.getAddressSpace()); - // Check to see if the global already exists. - llvm::GlobalVariable *GV = getModule().getGlobalVariable(D->getName(), true); + // See if it is already in the map. + llvm::GlobalValue *&Entry = GlobalDeclMap[D]; - // If it doesn't already exist, just create and return an entry. - if (GV == 0) { - return Entry = new llvm::GlobalVariable(Ty, false, - llvm::GlobalValue::ExternalLinkage, - 0, D->getName(), &getModule(), 0, - ASTTy.getAddressSpace()); + // If not look for an existing global (if this decl shadows another + // one) or lazily create a forward declaration. + if (!Entry) { + // Check to see if the global already exists. + llvm::GlobalVariable *GV = getModule().getGlobalVariable(D->getName(), true); + + // Create it if not. + if (!GV) + GV = new llvm::GlobalVariable(Ty, false, + llvm::GlobalValue::ExternalLinkage, + 0, D->getName(), &getModule(), 0, + ASTTy.getAddressSpace()); + + // Cache the entry. + Entry = GV; } - // Otherwise, it already exists; return the existing version - llvm::PointerType *PTy = llvm::PointerType::get(Ty, ASTTy.getAddressSpace()); - return Entry = llvm::ConstantExpr::getBitCast(GV, PTy); + // Make sure the result is of the correct type. + return llvm::ConstantExpr::getBitCast(Entry, PTy); } void CodeGenModule::EmitGlobalVarDefinition(const VarDecl *D) { llvm::Constant *Init = 0; QualType ASTTy = D->getType(); const llvm::Type *VarTy = getTypes().ConvertTypeForMem(ASTTy); - const llvm::Type *VarPtrTy = - llvm::PointerType::get(VarTy, ASTTy.getAddressSpace()); if (D->getInit() == 0) { // This is a tentative definition; tentative definitions are @@ -617,13 +620,13 @@ void CodeGenModule::EmitGlobalVarDefinition(const VarDecl *D) { // Make sure we don't keep around any stale references to globals // FIXME: This is really slow; we need a better way to walk all // the decls with the same name - ReplaceMapValuesWith(OldGV, NewPtrForOldDecl); + ReplaceMapValuesWith(OldGV, GV); // Erase the old global, since it is no longer used. OldGV->eraseFromParent(); } - GlobalDeclMap[D] = llvm::ConstantExpr::getBitCast(GV, VarPtrTy); + GlobalDeclMap[D] = GV; if (const AnnotateAttr *AA = D->getAttr()) { SourceManager &SM = Context.getSourceManager(); @@ -713,26 +716,32 @@ CodeGenModule::EmitForwardFunctionDefinition(const FunctionDecl *D) { } llvm::Constant *CodeGenModule::GetAddrOfFunction(const FunctionDecl *D) { - // See if it is already in the map. If so, just return it. - llvm::Constant *&Entry = GlobalDeclMap[D]; - if (Entry) return Entry; - - // Check to see if the function already exists; this occurs when - // this decl shadows a previous one. If it exists we bitcast it to - // the proper type for this decl and return. - llvm::Function *F = getModule().getFunction(D->getName()); - if (F) { - const llvm::Type *Ty = getTypes().ConvertType(D->getType()); - llvm::Type *PFTy = llvm::PointerType::getUnqual(Ty); - return Entry = llvm::ConstantExpr::getBitCast(F, PFTy); + QualType ASTTy = D->getType(); + const llvm::Type *Ty = getTypes().ConvertTypeForMem(ASTTy); + const llvm::Type *PTy = llvm::PointerType::get(Ty, ASTTy.getAddressSpace()); + + // See if it is already in the map. + llvm::GlobalValue *&Entry = GlobalDeclMap[D]; + + // If not look for an existing global (if this decl shadows another + // one) or lazily create a forward declaration. + if (!Entry) { + // Check to see if the global already exists. + llvm::GlobalValue *GV = getModule().getFunction(D->getName()); + + // Create it if not. + if (!GV) + GV = EmitForwardFunctionDefinition(D); + + // Cache the entry. + Entry = GV; } - // It doesn't exist; create and return an entry. - return Entry = EmitForwardFunctionDefinition(D); + return llvm::ConstantExpr::getBitCast(Entry, PTy); } void CodeGenModule::EmitGlobalFunctionDefinition(const FunctionDecl *D) { - llvm::Constant *&Entry = GlobalDeclMap[D]; + llvm::GlobalValue *&Entry = GlobalDeclMap[D]; const llvm::Type *Ty = getTypes().ConvertType(D->getType()); const llvm::FunctionType *FTy = cast(Ty); @@ -770,7 +779,7 @@ void CodeGenModule::EmitGlobalFunctionDefinition(const FunctionDecl *D) { // FIXME: Update the globaldeclmap for the previous decl of this name. We // really want a way to walk all of these, but we don't have it yet. This // is incredibly slow! - ReplaceMapValuesWith(F, NewPtrForOldDecl); + ReplaceMapValuesWith(F, NewFn); // Ok, delete the old function now, which is dead. assert(F->isDeclaration() && "Shouldn't replace non-declaration"); diff --git a/clang/lib/CodeGen/CodeGenModule.h b/clang/lib/CodeGen/CodeGenModule.h index 57363cddee0b..3a39eb3f79d3 100644 --- a/clang/lib/CodeGen/CodeGenModule.h +++ b/clang/lib/CodeGen/CodeGenModule.h @@ -66,7 +66,12 @@ class CodeGenModule { llvm::Function *MemCpyFn; llvm::Function *MemMoveFn; llvm::Function *MemSetFn; - llvm::DenseMap GlobalDeclMap; + + /// GlobalDeclMap - Mapping of decls to global variables we have + /// already emitted. Note that the entries in this map are the + /// actual global and therefore may not be of the same type as the + /// decl, they should be bitcasted on retrieval. + llvm::DenseMap GlobalDeclMap; /// List of static global for which code generation is delayed. When /// the translation unit has been fully processed we will lazily @@ -155,7 +160,7 @@ private: /// ReplaceMapValuesWith - This is a really slow and bad function that /// searches for any entries in GlobalDeclMap that point to OldVal, changing /// them to point to NewVal. This is badbadbad, FIXME! - void ReplaceMapValuesWith(llvm::Constant *OldVal, llvm::Constant *NewVal); + void ReplaceMapValuesWith(llvm::GlobalValue *OldVal, llvm::GlobalValue *NewVal); void SetFunctionAttributes(const FunctionDecl *FD, llvm::Function *F, diff --git a/clang/test/CodeGen/2008-07-30-redef-of-bitcasted-decl.c b/clang/test/CodeGen/2008-07-30-redef-of-bitcasted-decl.c new file mode 100644 index 000000000000..f7c2908d98ff --- /dev/null +++ b/clang/test/CodeGen/2008-07-30-redef-of-bitcasted-decl.c @@ -0,0 +1,28 @@ +// RUN: clang --emit-llvm -o - %s +// + +/* For posterity, the issue here begins initial "char []" decl for + * s. This is a tentative definition and so a global was being + * emitted, however the mapping in GlobalDeclMap referred to a bitcast + * of this global. + * + * The problem was that later when the correct definition for s is + * emitted we were doing a RAUW on the old global which was destroying + * the bitcast in the GlobalDeclMap (since it cannot be replaced + * properly), leaving a dangling pointer. + * + * The purpose of bar is just to trigger a use of the old decl + * sometime after the dangling pointer has been introduced. + */ + +char s[]; + +static void bar(void *db) { + eek(s); +} + +char s[5] = "hi"; + +int foo() { + bar(0); +}