[PATCH] D15686: PR25910: clang allows two var definitions with the same mangled name

[Artem Belevich via cfe-commits]

tra added a comment.

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.


================
Comment at: lib/CodeGen/CodeGenModule.cpp:1235-1236
@@ -1235,7 +1234,4 @@
     // 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::Constant *GVC = GetAddrOfGlobal(D, /*IsForDefinition=*/true);
+    llvm::GlobalValue *GV = dyn_cast<llvm::GlobalValue>(GVC);
+
----------------
andreybokhanko wrote:
> You are right -- I missed this case. Fixed.
> 
> Patch updated.
Minor nit -- you could just do dyn_cast<...>(GetAddrOfGlobal()), considering that GVC is not used anywhere else.

================
Comment at: lib/CodeGen/CodeGenModule.cpp:1250
@@ -1248,3 +1249,3 @@
     // ignore these cases.
-    if (GV && !GV->isDeclaration())
+    if (!GV->isDeclaration())
       continue;
----------------
GetGlobalValue() may return nullptr and existing code did check for that.
I'd add an assert(GV).

================
Comment at: lib/CodeGen/CodeGenModule.cpp:2184-2188
@@ -2140,7 +2183,7 @@
+
   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;
----------------
This may be collapsed into a single if() now.

================
Comment at: lib/CodeGen/CodeGenModule.cpp:2197-2198
@@ +2196,4 @@
+  // definition).
+  if (GV && !GV->isDeclaration())
+    return;
+
----------------
This can be moved above if (!MustBeEmitted()) -- no point calling MustBeEmitted() in this case if GV != nullptr.

================
Comment at: lib/CodeGen/CodeGenModule.h:704
@@ -704,1 +703,3 @@
+                                     llvm::Type *Ty = nullptr,
+                                     bool IsForDefinition = false);
 
----------------
It would be good to document what IsForDefinition does.


================
Comment at: lib/CodeGen/CodeGenModule.h:1140
@@ -1139,1 +1139,3 @@
+                                        const VarDecl *D,
+                                        bool IsForDefinition = false);
 
----------------
Ditto here.

================
Comment at: lib/CodeGen/CodeGenModule.h:1151
@@ -1148,3 +1150,3 @@
   void EmitGlobalFunctionDefinition(GlobalDecl GD, llvm::GlobalValue *GV);
-  void EmitGlobalVarDefinition(const VarDecl *D);
+  void EmitGlobalVarDefinition(const VarDecl *D, bool IsTentative = false);
   void EmitAliasDefinition(GlobalDecl GD);
----------------
And for IsTentative, too.



http://reviews.llvm.org/D15686