<div dir="ltr">Sorry, this is still resulting in problems: we're seeing miscompiles and "definition with same mangled name as another definition" errors after this change. It seems plausible that this is a pre-existing issue that's just being exposed by this change, but either way I've temporarily reverted this in r333482. I'll get you more details, including steps to reproduce this, offline.</div><div class="gmail_extra"><br><div class="gmail_quote">On 21 May 2018 at 04:47, Pavel Labath via cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: labath<br>
Date: Mon May 21 04:47:45 2018<br>
New Revision: 332839<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=332839&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project?rev=332839&view=rev</a><br>
Log:<br>
[CodeGen] Disable aggressive structor optimizations at -O0, take 2<br>
<br>
The first version of the patch (r332228) was flawed because it was<br>
putting structors into C5/D5 comdats very eagerly. This is correct only<br>
if we can ensure the comdat contains all required versions of the<br>
structor (which wasn't the case). This version uses a more nuanced<br>
approach:<br>
- for local structor symbols we use an alias because we don't have to<br>
worry about comdats or other compilation units.<br>
- linkonce symbols are emitted separately, as we cannot guarantee we<br>
will have all symbols we need to form a comdat (they are emitted<br>
lazily, only when referenced).<br>
- available_externally symbols are also emitted separately, as the code<br>
seemed to be worried about emitting an alias in this case.<br>
- other linkage types are not affected by the optimization level. They<br>
either get put into a comdat (weak) or get aliased (external).<br>
<br>
Reviewers: rjmccall, aprantl<br>
<br>
Subscribers: cfe-commits<br>
<br>
Differential Revision: <a href="https://reviews.llvm.org/D46685" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D46685</a><br>
<br>
Modified:<br>
cfe/trunk/lib/CodeGen/<wbr>ItaniumCXXABI.cpp<br>
cfe/trunk/test/CodeGenCXX/<wbr>ctor-dtor-alias.cpp<br>
cfe/trunk/test/CodeGenCXX/<wbr>float16-declarations.cpp<br>
<br>
Modified: cfe/trunk/lib/CodeGen/<wbr>ItaniumCXXABI.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp?rev=332839&r1=332838&r2=332839&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/lib/CodeGen/<wbr>ItaniumCXXABI.cpp?rev=332839&<wbr>r1=332838&r2=332839&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/lib/CodeGen/<wbr>ItaniumCXXABI.cpp (original)<br>
+++ cfe/trunk/lib/CodeGen/<wbr>ItaniumCXXABI.cpp Mon May 21 04:47:45 2018<br>
@@ -3628,12 +3628,22 @@ static StructorCodegen getCodegenToUse(C<br>
}<br>
llvm::GlobalValue::<wbr>LinkageTypes Linkage = CGM.getFunctionLinkage(<wbr>AliasDecl);<br>
<br>
- if (llvm::GlobalValue::<wbr>isDiscardableIfUnused(Linkage)<wbr>)<br>
- return StructorCodegen::RAUW;<br>
+ // All discardable structors can be RAUWed, but we don't want to do that in<br>
+ // unoptimized code, as that makes complete structor symbol disappear<br>
+ // completely, which degrades debugging experience.<br>
+ // Symbols with private linkage can be safely aliased, so we special case them<br>
+ // here.<br>
+ if (llvm::GlobalValue::<wbr>isLocalLinkage(Linkage))<br>
+ return CGM.getCodeGenOpts().<wbr>OptimizationLevel > 0 ? StructorCodegen::RAUW<br>
+ : StructorCodegen::Alias;<br>
<br>
+ // Linkonce structors cannot be aliased nor placed in a comdat, so these need<br>
+ // to be emitted separately.<br>
// FIXME: Should we allow available_externally aliases?<br>
- if (!llvm::GlobalAlias::<wbr>isValidLinkage(Linkage))<br>
- return StructorCodegen::RAUW;<br>
+ if (llvm::GlobalValue::<wbr>isDiscardableIfUnused(Linkage) ||<br>
+ !llvm::GlobalAlias::<wbr>isValidLinkage(Linkage))<br>
+ return CGM.getCodeGenOpts().<wbr>OptimizationLevel > 0 ? StructorCodegen::RAUW<br>
+ : StructorCodegen::Emit;<br>
<br>
if (llvm::GlobalValue::<wbr>isWeakForLinker(Linkage)) {<br>
// Only ELF and wasm support COMDATs with arbitrary names (C5/D5).<br>
<br>
Modified: cfe/trunk/test/CodeGenCXX/<wbr>ctor-dtor-alias.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/ctor-dtor-alias.cpp?rev=332839&r1=332838&r2=332839&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/test/<wbr>CodeGenCXX/ctor-dtor-alias.<wbr>cpp?rev=332839&r1=332838&r2=<wbr>332839&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/test/CodeGenCXX/<wbr>ctor-dtor-alias.cpp (original)<br>
+++ cfe/trunk/test/CodeGenCXX/<wbr>ctor-dtor-alias.cpp Mon May 21 04:47:45 2018<br>
@@ -1,5 +1,7 @@<br>
-// RUN: %clang_cc1 %s -triple i686-linux -emit-llvm -o - -mconstructor-aliases | FileCheck --check-prefix=NOOPT %s<br>
-<br>
+// RUN: %clang_cc1 %s -triple i686-linux -emit-llvm -o - -mconstructor-aliases > %t<br>
+// RUN: FileCheck --check-prefix=NOOPT1 --input-file=%t %s<br>
+// RUN: FileCheck --check-prefix=NOOPT2 --input-file=%t %s<br>
+// RUN: FileCheck --check-prefix=NOOPT3 --input-file=%t %s<br>
// RUN: %clang_cc1 %s -triple i686-linux -emit-llvm -o - -mconstructor-aliases -O1 -disable-llvm-passes > %t<br>
// RUN: FileCheck --check-prefix=CHECK1 --input-file=%t %s<br>
// RUN: FileCheck --check-prefix=CHECK2 --input-file=%t %s<br>
@@ -21,6 +23,13 @@ namespace test1 {<br>
// CHECK1: define weak_odr void @_ZN5test16foobarIvED0Ev({{.*}<wbr>} comdat($_<wbr>ZN5test16foobarIvED5Ev)<br>
// CHECK1-NOT: comdat<br>
<br>
+// This should happen regardless of the opt level.<br>
+// NOOPT1: @_ZN5test16foobarIvEC1Ev = weak_odr alias void {{.*}} @_ZN5test16foobarIvEC2Ev<br>
+// NOOPT1: @_ZN5test16foobarIvED1Ev = weak_odr alias void (%"struct.test1::foobar"*), void (%"struct.test1::foobar"*)* @_ZN5test16foobarIvED2Ev<br>
+// NOOPT1: define weak_odr void @_ZN5test16foobarIvEC2Ev({{.*}<wbr>} comdat($_<wbr>ZN5test16foobarIvEC5Ev)<br>
+// NOOPT1: define weak_odr void @_ZN5test16foobarIvED2Ev({{.*}<wbr>} comdat($_<wbr>ZN5test16foobarIvED5Ev)<br>
+// NOOPT1: define weak_odr void @_ZN5test16foobarIvED0Ev({{.*}<wbr>} comdat($_<wbr>ZN5test16foobarIvED5Ev)<br>
+<br>
// COFF doesn't support comdats with arbitrary names (C5/D5).<br>
// COFF: define weak_odr {{.*}} void @_ZN5test16foobarIvEC2Ev({{.*}<wbr>} comdat align<br>
// COFF: define weak_odr {{.*}} void @_ZN5test16foobarIvEC1Ev({{.*}<wbr>} comdat align<br>
@@ -37,12 +46,17 @@ template struct foobar<void>;<br>
}<br>
<br>
namespace test2 {<br>
-// test that when the destrucor is linkonce_odr we just replace every use of<br>
+// test that when the destructor is linkonce_odr we just replace every use of<br>
// C1 with C2.<br>
<br>
// CHECK1: define internal void @__cxx_global_var_init()<br>
// CHECK1: call void @_ZN5test26foobarIvEC2Ev<br>
// CHECK1: define linkonce_odr void @_ZN5test26foobarIvEC2Ev({{.*}<wbr>} comdat align<br>
+<br>
+// At -O0, we should still emit the complete constructor.<br>
+// NOOPT1: define internal void @__cxx_global_var_init()<br>
+// NOOPT1: call void @_ZN5test26foobarIvEC1Ev<br>
+// NOOPT1: define linkonce_odr void @_ZN5test26foobarIvEC1Ev({{.*}<wbr>} comdat align<br>
void g();<br>
template <typename T> struct foobar {<br>
foobar() { g(); }<br>
@@ -57,6 +71,11 @@ namespace test3 {<br>
// CHECK1: define internal void @__cxx_global_var_init.1()<br>
// CHECK1: call i32 @__cxa_atexit{{.*}}_<wbr>ZN5test312_GLOBAL__N_11AD2Ev<br>
// CHECK1: define internal void @_ZN5test312_GLOBAL__N_<wbr>11AD2Ev(<br>
+<br>
+// We can use an alias for internal symbols at -O0.<br>
+// NOOPT2: _ZN5test312_GLOBAL__N_11BD1Ev = internal alias void {{.*}} @_ZN5test312_GLOBAL__N_11BD2Ev<br>
+// NOOPT2: define internal void @__cxx_global_var_init.1()<br>
+// NOOPT2: call i32 @__cxa_atexit{{.*}}_<wbr>ZN5test312_GLOBAL__N_11BD1Ev<br>
namespace {<br>
struct A {<br>
~A() {}<br>
@@ -77,11 +96,12 @@ namespace test4 {<br>
// CHECK1: call i32 @__cxa_atexit{{.*}}_<wbr>ZN5test41AD2Ev<br>
// CHECK1: define linkonce_odr void @_ZN5test41AD2Ev({{.*}} comdat align<br>
<br>
- // test that we don't do this optimization at -O0 so that the debugger can<br>
- // see both destructors.<br>
- // NOOPT: define internal void @__cxx_global_var_init.2()<br>
- // NOOPT: call i32 @__cxa_atexit{{.*}}@_<wbr>ZN5test41BD2Ev<br>
- // NOOPT: define linkonce_odr void @_ZN5test41BD2Ev({{.*}} comdat align<br>
+ // Test that we don't do this optimization at -O0 and call the complete<br>
+ // destructor for B instead. This enables the debugger to see both<br>
+ // destructors.<br>
+ // NOOPT2: define internal void @__cxx_global_var_init.2()<br>
+ // NOOPT2: call i32 @__cxa_atexit{{.*}}@_<wbr>ZN5test41BD1Ev<br>
+ // NOOPT2: define linkonce_odr void @_ZN5test41BD1Ev({{.*}} comdat align<br>
struct A {<br>
virtual ~A() {}<br>
};<br>
@@ -129,6 +149,11 @@ namespace test7 {<br>
// out if we should).<br>
// pr17875.<br>
// CHECK3: define void @_ZN5test71BD2Ev<br>
+<br>
+ // At -O0, we should emit both destructors, the complete can be an alias to<br>
+ // the base one.<br>
+ // NOOPT3: @_ZN5test71BD1Ev = alias void {{.*}} @_ZN5test71BD2Ev<br>
+ // NOOPT3: define void @_ZN5test71BD2Ev<br>
template <typename> struct A {<br>
~A() {}<br>
};<br>
<br>
Modified: cfe/trunk/test/CodeGenCXX/<wbr>float16-declarations.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/float16-declarations.cpp?rev=332839&r1=332838&r2=332839&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/test/<wbr>CodeGenCXX/float16-<wbr>declarations.cpp?rev=332839&<wbr>r1=332838&r2=332839&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/test/CodeGenCXX/<wbr>float16-declarations.cpp (original)<br>
+++ cfe/trunk/test/CodeGenCXX/<wbr>float16-declarations.cpp Mon May 21 04:47:45 2018<br>
@@ -103,7 +103,7 @@ int main(void) {<br>
<br>
C1 c1(f1l);<br>
// CHECK-DAG: [[F1L:%[a-z0-9]+]] = load half, half* %{{.*}}, align 2<br>
-// CHECK-DAG: call void @_ZN2C1C2EDF16_(%class.C1* %{{.*}}, half %{{.*}})<br>
+// CHECK-DAG: call void @_ZN2C1C1EDF16_(%class.C1* %{{.*}}, half %{{.*}})<br>
<br>
S1<_Float16> s1 = { 132.f16 };<br>
// CHECK-DAG: @_ZZ4mainE2s1 = private unnamed_addr constant %struct.S1 { half 0xH5820 }, align 2<br>
<br>
<br>
______________________________<wbr>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div>