PR25910: clang allows two var definitions with the same mangled name

[andreybokhanko]

Summary

This patch fixes incorrect behavior described in PR25910.

Specifically, when there are two definitions with the same mangled name:

extern "C" {

int _ZN2nm3abcE = 1;

}

namespace nm {

float abc = 2;

}

clang should issue an error, not just emit an arbitrary one of them, as it does now.

It is essentially the same stuff as in http://reviews.llvm.org/D11297, just for variables, not functions.

Yours,

Andrey

Software Engineer
Intel Compiler Team
Intel

[andreybokhanko]

lib/CodeGen/CodeGenModule.cpp

1235–1236

Artem, to address your concern (from http://reviews.llvm.org/rL254195#39417): if IsForDefinition equals to true, it is guaranteed that we get GlobalValue here. We specifically call GetAddrOfGlobal to create (or get) a global with required type, not cast from a declaration with a different type.

[andreybokhanko]

Ping!

[tra]

lib/CodeGen/CodeGenModule.cpp

1235–1236

Empirical evidence suggests that it's possible to get non GlobalValue from GetAddrOfGlobal.
For instance, you may get llvm::UnaryConstantExpr which would be the case when GetAddrOfGlobal returns an addrspacecast which may happen during CUDA or OpenCL compilation.

If you want to reproduce the problem, apply my pending patch http://reviews.llvm.org/D15305 which has to defer emitting some variables. Run clang tests and you will see a lot of CUDA test cases crashing with assertion on this cast.

[andreybokhanko]

Fixed tra's note.

lib/CodeGen/CodeGenModule.cpp

1235–1237

You are right -- I missed this case. Fixed.

Patch updated.

[tra]

A better description of the problem would help. PR itself is somewhat short on details.
If I understand it correctly, the problem is that if we create multiple definitions with the same mangled name, clang does not always report it as an error and only emits one of those instances.

lib/CodeGen/CodeGenModule.cpp

1235–1237

Minor nit -- you could just do dyn_cast<...>(GetAddrOfGlobal()), considering that GVC is not used anywhere else.

1244

GetGlobalValue() may return nullptr and existing code did check for that.
I'd add an assert(GV).

2179–2183

This may be collapsed into a single if() now.

2192–2193

This can be moved above if (!MustBeEmitted()) -- no point calling MustBeEmitted() in this case if GV != nullptr.

lib/CodeGen/CodeGenModule.h

704

It would be good to document what IsForDefinition does.

1140

Ditto here.

1151

And for IsTentative, too.

[andreybokhanko]

Fixed tra's notes. All the fixes are local, logic of the patch is not changed.

[andreybokhanko]

In D15686#319725, @tra wrote:

A better description of the problem would help. PR itself is somewhat short on details.

Sorry for being not clear. I expected my main reviewer to be @rjmccall, who knows exactly what the problem is.

If I understand it correctly, the problem is that if we create multiple definitions with the same mangled name, clang does not always report it as an error and only emits one of those instances.

Yes, exactly.

Patch updated; please re-review.

Yours,
Andrey

lib/CodeGen/CodeGenModule.h

1140

Done (in *.cpp file).

1151

Done (in*.cpp file).

[rnk]

I thought we already addressed this issue with @rjmccall and decided that, if the user intentionally declares extern "C" variables with an _Z prefix, then we know they are intentionally attempting to name some C++ global variable.

I'd rather just warn on all extern "C" declarations starting with _Z, and let users keep the pieces when things break.

[andreybokhanko]

In D15686#325266, @rnk wrote:

I thought we already addressed this issue with @rjmccall and decided that, if the user intentionally declares extern "C" variables with an _Z prefix, then we know they are intentionally attempting to name some C++ global variable.

I'd rather just warn on all extern "C" declarations starting with _Z, and let users keep the pieces when things break.

That doesn't contradicts my patch at all. See the test -- where there is one declaration and one definition:

We expect no warnings here, as there is only declaration of _ZN2nm3abcE
global, no definitions.
extern "C" {

int _ZN2nm3abcE;

}

namespace nm {

float abc = 2;

}
// CHECK: @_ZN2nm3abcE = global float

nothing bad happens.

But if there are two definitions:

extern "C" {

int _ZN2nm3abcE = 1; // expected-note {{previous definition is here}}

}

namespace nm {

float abc = 2; // expected-error {{definition with same mangled name as another definition}}

}

an error is printed, as it should be.

Here is what John wrote himself (http://reviews.llvm.org/D11297#211351):

"I disagree with the conclusions in that thread (and wish you had CC'ed me; I unfortunately do not have time to keep up with cfe-dev). There is roughly zero chance that somebody is declaring an extern "C" symbol with a name that matches an Itanium mangled name with any intent other than to name that entity. We should not be rejecting this code or silently renaming one of the symbols. We should, of course, still diagnose attempts to *define* the same symbol multiple times."

John, may I ask you to take a look at this patch and confirm (or deny :-)) that it follows your understanding of how things should be?

Yours,
Andrey

[rjmccall]

Yes, this seems to be exactly what I wanted, thanks! LGTM.

[andreybokhanko]

@tra, @rnk, @rjmccall, thanks for the review!

Yours,
Andrey

lib/CodeGen/CodeGenModule.h

public:
   /// declarations, not types.
   unsigned GetGlobalVarAddressSpace(const VarDecl *D, unsigned AddrSpace);
 
   /// Return the llvm::Constant for the address of the given global variable.
   /// If Ty is non-null and if the global doesn't exist, then it will be greated
   /// with the specified type instead of whatever the normal requested type
   /// would be.
   llvm::Constant *GetAddrOfGlobalVar(const VarDecl *D,
-                                     llvm::Type *Ty = nullptr);
+                                     llvm::Type *Ty = nullptr,
+                                     bool IsForDefinition = false);
 
   /// Return the address of the given function. If Ty is non-null, then this
   /// function will use the specified type if it has to create it.
   llvm::Constant *GetAddrOfFunction(GlobalDecl GD, llvm::Type *Ty = nullptr,
                                     bool ForVTable = false,
                                     bool DontDefer = false,
                                     bool IsForDefinition = false);
 
private:
   GetOrCreateLLVMFunction(StringRef MangledName, llvm::Type *Ty, GlobalDecl D,
                           bool ForVTable, bool DontDefer = false,
                           bool IsThunk = false,
                           llvm::AttributeSet ExtraAttrs = llvm::AttributeSet(),
                           bool IsForDefinition = false);
 
   llvm::Constant *GetOrCreateLLVMGlobal(StringRef MangledName,
                                         llvm::PointerType *PTy,
-                                        const VarDecl *D);
+                                        const VarDecl *D,
+                                        bool IsForDefinition = false);
 
   void setNonAliasAttributes(const Decl *D, llvm::GlobalObject *GO);
 
   /// Set function attributes for a function declaration.
   void SetFunctionAttributes(GlobalDecl GD, llvm::Function *F,
                              bool IsIncompleteFunction, bool IsThunk);
 
   void EmitGlobalDefinition(GlobalDecl D, llvm::GlobalValue *GV = nullptr);
 
   void EmitGlobalFunctionDefinition(GlobalDecl GD, llvm::GlobalValue *GV);
-  void EmitGlobalVarDefinition(const VarDecl *D);
+  void EmitGlobalVarDefinition(const VarDecl *D, bool IsTentative = false);
   void EmitAliasDefinition(GlobalDecl GD);
   void EmitObjCPropertyImplementations(const ObjCImplementationDecl *D);
   void EmitObjCIvarInitializations(ObjCImplementationDecl *D);
   
   // C++ related functions.
 
   void EmitNamespace(const NamespaceDecl *D);
   void EmitLinkageSpec(const LinkageSpecDecl *D);

lib/CodeGen/CodeGenModule.cpp

void CodeGenModule::EmitDeferred() {
 
   // Grab the list of decls to emit. If EmitGlobalDefinition schedules more
   // work, it will not interfere with this.
   std::vector<DeferredGlobal> CurDeclsToEmit;
   CurDeclsToEmit.swap(DeferredDeclsToEmit);
 
   for (DeferredGlobal &G : CurDeclsToEmit) {
     GlobalDecl D = G.GD;
-    llvm::GlobalValue *GV = G.GV;
     G.GV = nullptr;
 
     // We should call GetAddrOfGlobal with IsForDefinition set to true in order
     // to get GlobalValue with exactly the type we need, not something that
     // might had been created for another decl with the same mangled name but
     // different type.
-    // FIXME: Support for variables is not implemented yet.
-    if (isa<FunctionDecl>(D.getDecl()))
-      GV = cast<llvm::GlobalValue>(GetAddrOfGlobal(D, /*IsForDefinition=*/true));
-    else
-      if (!GV)
-        GV = GetGlobalValue(getMangledName(D));
+    llvm::GlobalValue *GV =
+        cast<llvm::GlobalValue>(GetAddrOfGlobal(D, /*IsForDefinition=*/true));
 
     // Check to see if we've already emitted this.  This is necessary
     // for a couple of reasons: first, decls can end up in the
     // deferred-decls queue multiple times, and second, decls can end
     // up with definitions in unusual ways (e.g. by an extern inline
     // function acquiring a strong function redefinition).  Just
     // ignore these cases.
-    if (GV && !GV->isDeclaration())
+    if (!GV->isDeclaration())
       continue;
 
     // Otherwise, emit the definition and move on to the next one.
     EmitGlobalDefinition(D, GV);
 
     // If we found out that we need to emit more decls, do that recursively.
     // This has the advantage that the decls are emitted in a DFS and related
     // ones are close together, which is convenient for testing.
if (const auto *Method = dyn_cast<CXXMethodDecl>(D)) {
 
       return;
     }
 
     return EmitGlobalFunctionDefinition(GD, GV);
   }
 
   if (const auto *VD = dyn_cast<VarDecl>(D))
-    return EmitGlobalVarDefinition(VD);
+    return EmitGlobalVarDefinition(VD, !VD->hasDefinition());
   
   llvm_unreachable("Invalid argument to EmitGlobalDefinition()");
 }
 
 static void ReplaceUsesOfNonProtoTypeWithRealFunction(llvm::GlobalValue *Old,
                                                       llvm::Function *NewFn);
 
 /// GetOrCreateLLVMFunction - If the specified mangled name is not in the
if (Entry) {
     // Handle dropped DLL attributes.
     if (D && !D->hasAttr<DLLImportAttr>() && !D->hasAttr<DLLExportAttr>())
       Entry->setDLLStorageClass(llvm::GlobalValue::DefaultStorageClass);
 
     // If there are two attempts to define the same mangled name, issue an
     // error.
     if (IsForDefinition && !Entry->isDeclaration()) {
       GlobalDecl OtherGD;
-      // Check that GD is not yet in ExplicitDefinitions is required to make
-      // sure that we issue an error only once.
+      // Check that GD is not yet in DiagnosedConflictingDefinitions is required
+      // to make sure that we issue an error only once.
       if (lookupRepresentativeDecl(MangledName, OtherGD) &&
           (GD.getCanonicalDecl().getDecl() !=
            OtherGD.getCanonicalDecl().getDecl()) &&
           DiagnosedConflictingDefinitions.insert(GD).second) {
         getDiags().Report(D->getLocation(),
                           diag::err_duplicate_mangled_name);
         getDiags().Report(OtherGD.getDecl()->getLocation(),
                           diag::note_previous_definition);

 /// is something in the module with the specified name, return it potentially
 /// bitcasted to the right type.
 ///
 /// If D is non-null, it specifies a decl that correspond to this.  This is used
 /// to set the attributes on the global when it is first created.
 llvm::Constant *
 CodeGenModule::GetOrCreateLLVMGlobal(StringRef MangledName,
                                      llvm::PointerType *Ty,
-                                     const VarDecl *D) {
+                                     const VarDecl *D,
+                                     bool IsForDefinition) {
   // Lookup the entry, lazily creating it if necessary.
   llvm::GlobalValue *Entry = GetGlobalValue(MangledName);
   if (Entry) {
     if (WeakRefReferences.erase(Entry)) {
       if (D && !D->hasAttr<WeakAttr>())
         Entry->setLinkage(llvm::Function::ExternalLinkage);
     }
 
     // Handle dropped DLL attributes.
     if (D && !D->hasAttr<DLLImportAttr>() && !D->hasAttr<DLLExportAttr>())
       Entry->setDLLStorageClass(llvm::GlobalValue::DefaultStorageClass);
 
     if (Entry->getType() == Ty)
       return Entry;
 
+    // If there are two attempts to define the same mangled name, issue an
+    // error.
+    if (IsForDefinition && !Entry->isDeclaration()) {
+      GlobalDecl OtherGD;
+      const VarDecl *OtherD;
+
+      // Check that D is not yet in DiagnosedConflictingDefinitions is required
+      // to make sure that we issue an error only once.
+      if (lookupRepresentativeDecl(MangledName, OtherGD) &&
+          (D->getCanonicalDecl() != OtherGD.getCanonicalDecl().getDecl()) &&
+          (OtherD = dyn_cast<VarDecl>(OtherGD.getDecl())) &&
+          OtherD->hasInit() &&
+          DiagnosedConflictingDefinitions.insert(D).second) {
+        getDiags().Report(D->getLocation(),
+                          diag::err_duplicate_mangled_name);
+        getDiags().Report(OtherGD.getDecl()->getLocation(),
+                          diag::note_previous_definition);
+      }
+    }
+
     // Make sure the result is of the correct type.
     if (Entry->getType()->getAddressSpace() != Ty->getAddressSpace())
       return llvm::ConstantExpr::getAddrSpaceCast(Entry, Ty);
 
+    // Make sure the result is of the correct type.
+    // (If global is requested for a definition, we always need to create a new
+    // global, not just return a bitcast.)
+    if (!IsForDefinition)
       return llvm::ConstantExpr::getBitCast(Entry, Ty);
   }
 
   unsigned AddrSpace = GetGlobalVarAddressSpace(D, Ty->getAddressSpace());
   auto *GV = new llvm::GlobalVariable(
       getModule(), Ty->getElementType(), false,
       llvm::GlobalValue::ExternalLinkage, nullptr, MangledName, nullptr,
       llvm::GlobalVariable::NotThreadLocal, AddrSpace);
 
+  // If we already created a global with the same mangled name (but different
+  // type) before, take its name and remove it from its parent.
+  if (Entry) {
+    GV->takeName(Entry);
+
+    if (!Entry->use_empty()) {
+      llvm::Constant *NewPtrForOldDecl =
+          llvm::ConstantExpr::getBitCast(GV, Entry->getType());
+      Entry->replaceAllUsesWith(NewPtrForOldDecl);
+    }
+
+    Entry->eraseFromParent();
+  }
+
   // This is the first use or definition of a mangled name.  If there is a
   // deferred decl with this name, remember that we need to emit it at the end
   // of the file.
   auto DDI = DeferredDecls.find(MangledName);
   if (DDI != DeferredDecls.end()) {
     // Move the potentially referenced deferred decl to the DeferredDeclsToEmit
     // list, and remove it from DeferredDecls (since we don't need it anymore).
     addDeferredDeclToEmit(GV, DDI->second);
else if (isa<CXXMethodDecl>(GD.getDecl())) {
     return GetAddrOfFunction(GD, Ty, /*ForVTable=*/false, /*DontDefer=*/false,
                              IsForDefinition);
   } else if (isa<FunctionDecl>(GD.getDecl())) {
     const CGFunctionInfo &FI = getTypes().arrangeGlobalDeclaration(GD);
     llvm::FunctionType *Ty = getTypes().GetFunctionType(FI);
     return GetAddrOfFunction(GD, Ty, /*ForVTable=*/false, /*DontDefer=*/false,
                              IsForDefinition);
   } else
-    return GetAddrOfGlobalVar(cast<VarDecl>(GD.getDecl()));
+    return GetAddrOfGlobalVar(cast<VarDecl>(GD.getDecl()), /*Ty=*/nullptr,
+                              IsForDefinition);
 }
 
 llvm::GlobalVariable *
 CodeGenModule::CreateOrReplaceCXXRuntimeVariable(StringRef Name, 
                                       llvm::Type *Ty,
                                       llvm::GlobalValue::LinkageTypes Linkage) {
   llvm::GlobalVariable *GV = getModule().getNamedGlobal(Name);
   llvm::GlobalVariable *OldGV = nullptr;
CodeGenModule::CreateOrReplaceCXXRuntimeVariable(StringRef Name,
   return GV;
 }
 
 /// GetAddrOfGlobalVar - Return the llvm::Constant for the address of the
 /// given global variable.  If Ty is non-null and if the global doesn't exist,
 /// then it will be created with the specified type instead of whatever the
 /// normal requested type would be.
 llvm::Constant *CodeGenModule::GetAddrOfGlobalVar(const VarDecl *D,
-                                                  llvm::Type *Ty) {
+                                                  llvm::Type *Ty,
+                                                  bool IsForDefinition) {
   assert(D->hasGlobalStorage() && "Not a global variable");
   QualType ASTTy = D->getType();
   if (!Ty)
     Ty = getTypes().ConvertTypeForMem(ASTTy);
 
   llvm::PointerType *PTy =
     llvm::PointerType::get(Ty, getContext().getTargetAddressSpace(ASTTy));
 
   StringRef MangledName = getMangledName(D);
-  return GetOrCreateLLVMGlobal(MangledName, PTy, D);
+  return GetOrCreateLLVMGlobal(MangledName, PTy, D, IsForDefinition);
 }
 
 /// CreateRuntimeVariable - Create a new runtime global variable with the
 /// specified type and name.
 llvm::Constant *
 CodeGenModule::CreateRuntimeVariable(llvm::Type *Ty,
                                      StringRef Name) {
   return GetOrCreateLLVMGlobal(Name, llvm::PointerType::getUnqual(Ty), nullptr);
 }
 
 void CodeGenModule::EmitTentativeDefinition(const VarDecl *D) {
   assert(!D->getInit() && "Cannot emit definite definitions here!");
 
+  StringRef MangledName = getMangledName(D);
+  llvm::GlobalValue *GV = GetGlobalValue(MangledName);
+
   if (!MustBeEmitted(D)) {
     // If we have not seen a reference to this variable yet, place it
     // into the deferred declarations table to be emitted if needed
     // later.
-    StringRef MangledName = getMangledName(D);
-    if (!GetGlobalValue(MangledName)) {
+    if (!GV) {
       DeferredDecls[MangledName] = D;
       return;
     }
   }
 
+  // We already have a definition, not declaration, with the same mangled name.
+  // Emitting of declaration is not required (and actually overwrites emitted
+  // definition).
+  if (GV && !GV->isDeclaration())
+    return;
+
   // The tentative definition is the only definition.
   EmitGlobalVarDefinition(D);
 }
 
 CharUnits CodeGenModule::GetTargetTypeStoreSize(llvm::Type *Ty) const {
   return Context.toCharUnitsFromBits(
       getDataLayout().getTypeStoreSizeInBits(Ty));
 }

 
 void CodeGenModule::maybeSetTrivialComdat(const Decl &D,
                                           llvm::GlobalObject &GO) {
   if (!shouldBeInCOMDAT(*this, D))
     return;
   GO.setComdat(TheModule.getOrInsertComdat(GO.getName()));
 }
 
-void CodeGenModule::EmitGlobalVarDefinition(const VarDecl *D) {
+void CodeGenModule::EmitGlobalVarDefinition(const VarDecl *D,
+                                            bool IsTentative) {
   llvm::Constant *Init = nullptr;
   QualType ASTTy = D->getType();
   CXXRecordDecl *RD = ASTTy->getBaseElementTypeUnsafe()->getAsCXXRecordDecl();
   bool NeedsGlobalCtor = false;
   bool NeedsGlobalDtor = RD && !RD->hasTrivialDestructor();
 
   const VarDecl *InitDecl;
   const Expr *InitExpr = D->getAnyInitializer(InitDecl);
if (!Init) {
       // initializer position (just in case this entry was delayed) if we
       // also don't need to register a destructor.
       if (getLangOpts().CPlusPlus && !NeedsGlobalDtor)
         DelayedCXXInitPosition.erase(D);
     }
   }
 
   llvm::Type* InitType = Init->getType();
-  llvm::Constant *Entry = GetAddrOfGlobalVar(D, InitType);
+  llvm::Constant *Entry =
+      GetAddrOfGlobalVar(D, InitType, /*IsForDefinition=*/!IsTentative);
 
   // Strip off a bitcast if we got one back.
   if (auto *CE = dyn_cast<llvm::ConstantExpr>(Entry)) {
     assert(CE->getOpcode() == llvm::Instruction::BitCast ||
            CE->getOpcode() == llvm::Instruction::AddrSpaceCast ||
            // All zero index gep.
            CE->getOpcode() == llvm::Instruction::GetElementPtr);
     Entry = CE->getOperand(0);
if (!GV ||
       GV->getType()->getElementType() != InitType ||
       GV->getType()->getAddressSpace() !=
        GetGlobalVarAddressSpace(D, getContext().getTargetAddressSpace(ASTTy))) {
 
     // Move the old entry aside so that we'll create a new one.
     Entry->setName(StringRef());
 
     // Make a new global with the correct type, this is now guaranteed to work.
-    GV = cast<llvm::GlobalVariable>(GetAddrOfGlobalVar(D, InitType));
+    GV = cast<llvm::GlobalVariable>(
+        GetAddrOfGlobalVar(D, InitType, /*IsForDefinition=*/!IsTentative));
 
     // Replace all uses of the old global with the new global
     llvm::Constant *NewPtrForOldDecl =
         llvm::ConstantExpr::getBitCast(GV, Entry->getType());
     Entry->replaceAllUsesWith(NewPtrForOldDecl);
 
     // Erase the old global, since it is no longer used.
     cast<llvm::GlobalValue>(Entry)->eraseFromParent();

test/CodeGenCXX/duplicate-mangled-name.cpp

 // RUN: %clang_cc1 -triple %itanium_abi_triple -emit-llvm-only %s -verify -DTEST1
-// RUN: %clang_cc1 -triple %itanium_abi_triple -emit-llvm-only %s -verify -DTEST2
+// RUN: %clang_cc1 -triple %itanium_abi_triple -emit-llvm-only %s -verify -DTEST2 -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -triple %itanium_abi_triple -emit-llvm-only %s -verify -DTEST3
+// RUN: %clang_cc1 -triple %itanium_abi_triple -emit-llvm-only %s -verify -DTEST4
 
 #ifdef TEST1
 
 // rdar://15522601
 class MyClass {
  static void meth();
 };
 void MyClass::meth() { } // expected-note {{previous}}
 extern "C" {
   void _ZN7MyClass4methEv() { } // expected-error {{definition with same mangled name as another definition}}
 }
 
 #elif TEST2
 
-// We expect no warnings here, as there is only declaration of _ZN1TD1Ev function, no definitions.
+// expected-no-diagnostics
+
+// We expect no warnings here, as there is only declaration of _ZN1TD1Ev
+// function, no definitions.
 extern "C" void _ZN1TD1Ev();
 struct T {
   ~T() {}
 };
 
-void foo() {
+// We expect no warnings here, as there is only declaration of _ZN2nm3abcE
+// global, no definitions.
+extern "C" {
+  int _ZN2nm3abcE;
+}
+
+namespace nm {
+  float abc = 2;
+}
+// CHECK: @_ZN2nm3abcE = global float
+
+float foo() {
   _ZN1TD1Ev();
+// CHECK: call void bitcast (void (%struct.T*)* @_ZN1TD1Ev to void ()*)()
   T t;
+// CHECK: call void @_ZN1TD1Ev(%struct.T* %t)
+  return _ZN2nm3abcE + nm::abc;
 }
 
+#elif TEST3
+
 extern "C" void _ZN2T2D2Ev() {}; // expected-note {{previous definition is here}}
 
 struct T2 {
   ~T2() {} // expected-error {{definition with same mangled name as another definition}}
 };
 
-void bar() {
+void foo() {
   _ZN2T2D2Ev();
   T2 t;
 }
 
+#elif TEST4
+
+extern "C" {
+  int _ZN2nm3abcE = 1; // expected-note {{previous definition is here}}
+}
+
+namespace nm {
+  float abc = 2; // expected-error {{definition with same mangled name as another definition}}
+}
+
+float foo() {
+  return _ZN2nm3abcE + nm::abc;
+}
+
 #else
 
 #error Unknwon test
 
 #endif