From 5b2d43f6651387a49e6459404c05828200c290e6 Mon Sep 17 00:00:00 2001 From: Matt Pharr Date: Wed, 28 Mar 2012 14:15:49 -0700 Subject: [PATCH] Fix global variable code to correctly handle extern declarations. When we have an "extern" global, now we no longer inadvertently define storage for it. Further, we now successfully do define storage when we encounter a definition following one or more extern declarations. Issues #215 and #217. --- module.cpp | 115 ++++++++++++++++++++++--------- sym.h | 2 +- tests_errors/global-decl-1.ispc | 4 ++ tests_errors/global-decl-2.ispc | 4 ++ tests_errors/global-redef-1.ispc | 4 ++ tests_errors/global-redef.ispc | 4 ++ 6 files changed, 101 insertions(+), 32 deletions(-) create mode 100644 tests_errors/global-decl-1.ispc create mode 100644 tests_errors/global-decl-2.ispc create mode 100644 tests_errors/global-redef-1.ispc create mode 100644 tests_errors/global-redef.ispc diff --git a/module.cpp b/module.cpp index 3f184a42..87cb1a88 100644 --- a/module.cpp +++ b/module.cpp @@ -270,53 +270,106 @@ Module::AddGlobalVariable(Symbol *sym, Expr *initExpr, bool isConst) { Error(sym->pos, "Initializer can't be provided with \"extern\" " "global variable \"%s\".", sym->name.c_str()); } - else if (initExpr != NULL) { - initExpr = TypeCheck(initExpr); + else { if (initExpr != NULL) { - // We need to make sure the initializer expression is - // the same type as the global. (But not if it's an - // ExprList; they don't have types per se / can't type - // convert themselves anyway.) - if (dynamic_cast(initExpr) == NULL) - initExpr = TypeConvertExpr(initExpr, sym->type, "initializer"); - + initExpr = TypeCheck(initExpr); if (initExpr != NULL) { - initExpr = Optimize(initExpr); - // Fingers crossed, now let's see if we've got a - // constant value.. - llvmInitializer = initExpr->GetConstant(sym->type); + // We need to make sure the initializer expression is + // the same type as the global. (But not if it's an + // ExprList; they don't have types per se / can't type + // convert themselves anyway.) + if (dynamic_cast(initExpr) == NULL) + initExpr = TypeConvertExpr(initExpr, sym->type, "initializer"); + + if (initExpr != NULL) { + initExpr = Optimize(initExpr); + // Fingers crossed, now let's see if we've got a + // constant value.. + llvmInitializer = initExpr->GetConstant(sym->type); - if (llvmInitializer != NULL) { - if (sym->type->IsConstType()) - // Try to get a ConstExpr associated with - // the symbol. This dynamic_cast can - // validly fail, for example for types like - // StructTypes where a ConstExpr can't - // represent their values. - sym->constValue = - dynamic_cast(initExpr); + if (llvmInitializer != NULL) { + if (sym->type->IsConstType()) + // Try to get a ConstExpr associated with + // the symbol. This dynamic_cast can + // validly fail, for example for types like + // StructTypes where a ConstExpr can't + // represent their values. + sym->constValue = + dynamic_cast(initExpr); + } + else + Error(initExpr->pos, "Initializer for global variable \"%s\" " + "must be a constant.", sym->name.c_str()); } - else - Error(initExpr->pos, "Initializer for global variable \"%s\" " - "must be a constant.", sym->name.c_str()); } } + + // If no initializer was provided or if we couldn't get a value + // above, initialize it with zeros.. + if (llvmInitializer == NULL) + llvmInitializer = llvm::Constant::getNullValue(llvmType); } - // If no initializer was provided or if we couldn't get a value - // above, initialize it with zeros.. - if (llvmInitializer == NULL) - llvmInitializer = llvm::Constant::getNullValue(llvmType); + Symbol *stSym = symbolTable->LookupVariable(sym->name.c_str()); + llvm::GlobalVariable *oldGV = NULL; + if (stSym != NULL) { + // We've already seen either a declaration or a definition of this + // global. + + // If the type doesn't match with the previous one, issue an error. + if (!Type::Equal(sym->type, stSym->type)) { + Error(sym->pos, "Definition of variable \"%s\" conflicts with " + "definition at %s:%d.", sym->name.c_str(), + stSym->pos.name, stSym->pos.first_line); + return; + } + + llvm::GlobalVariable *gv = + llvm::dyn_cast(stSym->storagePtr); + Assert(gv != NULL); + + // And issue an error if this is a redefinition of a variable + if (gv->hasInitializer() && + sym->storageClass != SC_EXTERN && sym->storageClass != SC_EXTERN_C) { + Error(sym->pos, "Redefinition of variable \"%s\" is illegal. " + "(Previous definition at %s:%d.)", sym->name.c_str(), + stSym->pos.name, stSym->pos.first_line); + return; + } + + // Now, we either have a redeclaration of a global, or a definition + // of a previously-declared global. First, save the pointer to the + // previous llvm::GlobalVariable + oldGV = gv; + + // Now copy over all of the members of the current Symbol to the + // symbol in the symbol table. + *stSym = *sym; + // And copy the pointer of the one in the symbol table to sym, so + // that the operations below update storagePtr for the Symbol + // already in the symbol table. + sym = stSym; + } + else + symbolTable->AddVariable(sym); llvm::GlobalValue::LinkageTypes linkage = (sym->storageClass == SC_STATIC) ? llvm::GlobalValue::InternalLinkage : llvm::GlobalValue::ExternalLinkage; + + // Note that the NULL llvmInitializer is what leads to "extern" + // declarations coming up extern and not defining storage (a bit + // subtle)... sym->storagePtr = new llvm::GlobalVariable(*module, llvmType, isConst, linkage, llvmInitializer, sym->name.c_str()); - symbolTable->AddVariable(sym); - if (diBuilder && (sym->storageClass != SC_EXTERN)) { + // Patch up any references to the previous GlobalVariable (e.g. from a + // declaration of a global that was later defined.) + if (oldGV != NULL) + oldGV->replaceAllUsesWith(sym->storagePtr); + + if (diBuilder) { llvm::DIFile file = sym->pos.GetDIFile(); diBuilder->createGlobalVariable(sym->name, file, diff --git a/sym.h b/sym.h index fa452326..8e14495a 100644 --- a/sym.h +++ b/sym.h @@ -75,7 +75,7 @@ public: std::string MangledName() const; SourcePos pos; /*!< Source file position where the symbol was defined */ - const std::string name; /*!< Symbol's name */ + std::string name; /*!< Symbol's name */ llvm::Value *storagePtr; /*!< For symbols with storage associated with them (i.e. variables but not functions), this member stores a pointer to its diff --git a/tests_errors/global-decl-1.ispc b/tests_errors/global-decl-1.ispc new file mode 100644 index 00000000..6f111bbf --- /dev/null +++ b/tests_errors/global-decl-1.ispc @@ -0,0 +1,4 @@ +// Definition of variable "foo" conflicts with definition at + +extern int foo; +float foo; diff --git a/tests_errors/global-decl-2.ispc b/tests_errors/global-decl-2.ispc new file mode 100644 index 00000000..66647ea7 --- /dev/null +++ b/tests_errors/global-decl-2.ispc @@ -0,0 +1,4 @@ +// Definition of variable "foo" conflicts with definition at + +extern int foo; +extern float foo; diff --git a/tests_errors/global-redef-1.ispc b/tests_errors/global-redef-1.ispc new file mode 100644 index 00000000..7ebb3da7 --- /dev/null +++ b/tests_errors/global-redef-1.ispc @@ -0,0 +1,4 @@ +// Definition of variable "foo" conflicts with definition at + +int foo; +float foo; diff --git a/tests_errors/global-redef.ispc b/tests_errors/global-redef.ispc new file mode 100644 index 00000000..9a2df32f --- /dev/null +++ b/tests_errors/global-redef.ispc @@ -0,0 +1,4 @@ +// Redefinition of variable "foo" is illegal + +int foo; +int foo;