[PATCH] D154270: [clang][CodeGen] Fix global variables initialized with an inheriting constructor.

[Eli Friedman via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGb4bae3fd8ede: [clang][CodeGen] Fix global variables 
initialized with an inheriting… (authored by efriedma).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154270/new/

https://reviews.llvm.org/D154270

Files:
  clang/lib/CodeGen/CGDecl.cpp
  clang/test/CodeGenCXX/inheriting-constructor.cpp


Index: clang/test/CodeGenCXX/inheriting-constructor.cpp
===
--- clang/test/CodeGenCXX/inheriting-constructor.cpp
+++ clang/test/CodeGenCXX/inheriting-constructor.cpp
@@ -4,6 +4,24 @@
 // RUN: %clang_cc1 -no-enable-noundef-analysis -std=c++11 -triple i386-windows 
-emit-llvm -o - %s | FileCheck %s --check-prefix=MSABI --check-prefix=WIN32
 // RUN: %clang_cc1 -no-enable-noundef-analysis -std=c++11 -triple 
x86_64-windows -emit-llvm -o - %s | FileCheck %s --check-prefix=MSABI 
--check-prefix=WIN64
 
+// PR63618: make sure we generate definitions for all the globals defined in 
the test
+// MSABI: @"?b@@3UB@@A" = {{.*}} zeroinitializer
+// MSABI: @"?d@@3UD@@A" = {{.*}} zeroinitializer
+// MSABI: @"?b@noninline_nonvirt@@3UB@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?c@noninline_nonvirt@@3UC@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?b@noninline_virt@@3UB@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?c@noninline_virt@@3UC@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?b@inalloca_nonvirt@@3UB@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?c@inalloca_nonvirt@@3UC@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?b@inalloca_virt@@3UB@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?c@inalloca_virt@@3UC@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?b@inline_nonvirt@@3UB@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?c@inline_nonvirt@@3UC@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?b@inline_virt@@3UB@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?c@inline_virt@@3UC@1@A" = {{.*}} zeroinitializer
+
+// MSABI-NOT: @this
+
 // PR12219
 struct A { A(int); virtual ~A(); };
 struct B : A { using A::A; ~B(); };
Index: clang/lib/CodeGen/CGDecl.cpp
===
--- clang/lib/CodeGen/CGDecl.cpp
+++ clang/lib/CodeGen/CGDecl.cpp
@@ -2453,7 +2453,10 @@
   assert((isa(D) || isa(D)) &&
  "Invalid argument to EmitParmDecl");
 
-  Arg.getAnyValue()->setName(D.getName());
+  // Set the name of the parameter's initial value to make IR easier to
+  // read. Don't modify the names of globals.
+  if (!isa(Arg.getAnyValue()))
+Arg.getAnyValue()->setName(D.getName());
 
   QualType Ty = D.getType();
 


Index: clang/test/CodeGenCXX/inheriting-constructor.cpp
===
--- clang/test/CodeGenCXX/inheriting-constructor.cpp
+++ clang/test/CodeGenCXX/inheriting-constructor.cpp
@@ -4,6 +4,24 @@
 // RUN: %clang_cc1 -no-enable-noundef-analysis -std=c++11 -triple i386-windows -emit-llvm -o - %s | FileCheck %s --check-prefix=MSABI --check-prefix=WIN32
 // RUN: %clang_cc1 -no-enable-noundef-analysis -std=c++11 -triple x86_64-windows -emit-llvm -o - %s | FileCheck %s --check-prefix=MSABI --check-prefix=WIN64
 
+// PR63618: make sure we generate definitions for all the globals defined in the test
+// MSABI: @"?b@@3UB@@A" = {{.*}} zeroinitializer
+// MSABI: @"?d@@3UD@@A" = {{.*}} zeroinitializer
+// MSABI: @"?b@noninline_nonvirt@@3UB@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?c@noninline_nonvirt@@3UC@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?b@noninline_virt@@3UB@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?c@noninline_virt@@3UC@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?b@inalloca_nonvirt@@3UB@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?c@inalloca_nonvirt@@3UC@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?b@inalloca_virt@@3UB@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?c@inalloca_virt@@3UC@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?b@inline_nonvirt@@3UB@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?c@inline_nonvirt@@3UC@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?b@inline_virt@@3UB@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?c@inline_virt@@3UC@1@A" = {{.*}} zeroinitializer
+
+// MSABI-NOT: @this
+
 // PR12219
 struct A { A(int); virtual ~A(); };
 struct B : A { using A::A; ~B(); };
Index: clang/lib/CodeGen/CGDecl.cpp
===
--- clang/lib/CodeGen/CGDecl.cpp
+++ clang/lib/CodeGen/CGDecl.cpp
@@ -2453,7 +2453,10 @@
   assert((isa(D) || isa(D)) &&
  "Invalid argument to EmitParmDecl");
 
-  Arg.getAnyValue()->setName(D.getName());
+  // Set the name of the parameter's initial value to make IR easier to
+  // read. Don't modify the names of globals.
+  if (!isa(Arg.getAnyValue()))
+Arg.getAnyValue()->setName(D.getName());
 
   QualType Ty = D.getType();
 
___
cfe-commits mailing list

[PATCH] D154270: [clang][CodeGen] Fix global variables initialized with an inheriting constructor.

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/CodeGen/CGDecl.cpp:2459
+  if (!isa(Arg.getAnyValue()))
+Arg.getAnyValue()->setName(D.getName());
 

rsmith wrote:
> Is it feasible to only set the name if the value doesn't already have a name, 
> or do we give values bad names often enough that it's better to overwrite an 
> existing name here?
Outside the EmitInlinedInheritingCXXConstructorCall case, the arguments are 
generally either llvm::Argument, or llvm::Instruction generated by calling 
convention code.  It might make sense to just make the calling convention code 
name the values itself (it already names some of the values it generates).

But I'll stick with the conservative fix here, and leave that for a followup.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154270/new/

https://reviews.llvm.org/D154270

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154270: [clang][CodeGen] Fix global variables initialized with an inheriting constructor.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

Wow, what an amazing bug :)




Comment at: clang/lib/CodeGen/CGDecl.cpp:2459
+  if (!isa(Arg.getAnyValue()))
+Arg.getAnyValue()->setName(D.getName());
 

Is it feasible to only set the name if the value doesn't already have a name, 
or do we give values bad names often enough that it's better to overwrite an 
existing name here?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154270/new/

https://reviews.llvm.org/D154270

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154270: [clang][CodeGen] Fix global variables initialized with an inheriting constructor.

[Eli Friedman via Phabricator via cfe-commits]

efriedma created this revision.
efriedma added reviewers: rsmith, rnk, rjmccall, akhuang.
Herald added a subscriber: kristof.beyls.
Herald added a project: All.
efriedma requested review of this revision.
Herald added a project: clang.

For inheriting constructors which have to be emitted inline, we use 
CodeGenFunction::EmitInlinedInheritingCXXConstructorCall to emit the required 
code.  This code uses EmitParmDecl to emit the "this" argument of the call.  
EmitParmDecl then helpfully calls llvm::Value::setName to name the parameter... 
which renames the global variable to "this".

To fix the issue, skip the setName call on globals.

The renaming still has slightly weird results in other cases (it renames all 
local variables initialized with an inlined inheriting constructor to "this"), 
but the result isn't actually wrong in those cases, so I'm just going to leave 
it for now.

Fixes https://github.com/llvm/llvm-project/issues/63618


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D154270

Files:
  clang/lib/CodeGen/CGDecl.cpp
  clang/test/CodeGenCXX/inheriting-constructor.cpp


Index: clang/test/CodeGenCXX/inheriting-constructor.cpp
===
--- clang/test/CodeGenCXX/inheriting-constructor.cpp
+++ clang/test/CodeGenCXX/inheriting-constructor.cpp
@@ -4,6 +4,24 @@
 // RUN: %clang_cc1 -no-enable-noundef-analysis -std=c++11 -triple i386-windows 
-emit-llvm -o - %s | FileCheck %s --check-prefix=MSABI --check-prefix=WIN32
 // RUN: %clang_cc1 -no-enable-noundef-analysis -std=c++11 -triple 
x86_64-windows -emit-llvm -o - %s | FileCheck %s --check-prefix=MSABI 
--check-prefix=WIN64
 
+// PR63618: make sure we generate definitions for all the globals defined in 
the test
+// MSABI: @"?b@@3UB@@A" = {{.*}} zeroinitializer
+// MSABI: @"?d@@3UD@@A" = {{.*}} zeroinitializer
+// MSABI: @"?b@noninline_nonvirt@@3UB@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?c@noninline_nonvirt@@3UC@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?b@noninline_virt@@3UB@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?c@noninline_virt@@3UC@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?b@inalloca_nonvirt@@3UB@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?c@inalloca_nonvirt@@3UC@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?b@inalloca_virt@@3UB@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?c@inalloca_virt@@3UC@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?b@inline_nonvirt@@3UB@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?c@inline_nonvirt@@3UC@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?b@inline_virt@@3UB@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?c@inline_virt@@3UC@1@A" = {{.*}} zeroinitializer
+
+// MSABI-NOT: @this
+
 // PR12219
 struct A { A(int); virtual ~A(); };
 struct B : A { using A::A; ~B(); };
Index: clang/lib/CodeGen/CGDecl.cpp
===
--- clang/lib/CodeGen/CGDecl.cpp
+++ clang/lib/CodeGen/CGDecl.cpp
@@ -2453,7 +2453,10 @@
   assert((isa(D) || isa(D)) &&
  "Invalid argument to EmitParmDecl");
 
-  Arg.getAnyValue()->setName(D.getName());
+  // Set the name of the parameter's initial value to make IR easier to
+  // read. Don't modify the names of globals.
+  if (!isa(Arg.getAnyValue()))
+Arg.getAnyValue()->setName(D.getName());
 
   QualType Ty = D.getType();
 


Index: clang/test/CodeGenCXX/inheriting-constructor.cpp
===
--- clang/test/CodeGenCXX/inheriting-constructor.cpp
+++ clang/test/CodeGenCXX/inheriting-constructor.cpp
@@ -4,6 +4,24 @@
 // RUN: %clang_cc1 -no-enable-noundef-analysis -std=c++11 -triple i386-windows -emit-llvm -o - %s | FileCheck %s --check-prefix=MSABI --check-prefix=WIN32
 // RUN: %clang_cc1 -no-enable-noundef-analysis -std=c++11 -triple x86_64-windows -emit-llvm -o - %s | FileCheck %s --check-prefix=MSABI --check-prefix=WIN64
 
+// PR63618: make sure we generate definitions for all the globals defined in the test
+// MSABI: @"?b@@3UB@@A" = {{.*}} zeroinitializer
+// MSABI: @"?d@@3UD@@A" = {{.*}} zeroinitializer
+// MSABI: @"?b@noninline_nonvirt@@3UB@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?c@noninline_nonvirt@@3UC@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?b@noninline_virt@@3UB@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?c@noninline_virt@@3UC@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?b@inalloca_nonvirt@@3UB@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?c@inalloca_nonvirt@@3UC@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?b@inalloca_virt@@3UB@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?c@inalloca_virt@@3UC@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?b@inline_nonvirt@@3UB@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?c@inline_nonvirt@@3UC@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?b@inline_virt@@3UB@1@A" = {{.*}} zeroinitializer
+// MSABI: @"?c@inline_virt@@3UC@1@A" = {{.*}} zeroinitializer
+
+// MSABI-NOT: @this
+
 // PR12219
 struct A { A(int); virtual ~A(); };
 struct B : A { using A::A; ~B(); };
Index: