[CodeGen] Disable structor optimizations at -O0

[labath]

Summary

Merging of complete and base structors can degrade debug quality as it
will leave the debugger unable to locate the full object structor. This
is apparent when evaluating an expression in the debugger which requires
constructing an object of class which has had this optimization applied
to it. When compiling the expression, we pretend that the class and
its methods have been defined in another compilation unit, so the
expression compiler assumes the structor definition must be available,
whereas in reality the complete structor may have been omitted and all
it's uses replaced by the base structor.

This improves debug quality on non-darwin platforms (darwin does not
have -mconstructor-aliases on by default, so it is spared these
problems) and enable us to remove some workarounds from LLDB which attempt to
mitigate this issue.

This is tested by strenghtening the check in the existing
ctor-dtor-alias test. In the other aliasing tests I add -O1 to compiler
options to make sure the aliasing kicks in.

PS: Of the three ways we can do structor optimizations, only the RAUW
actually causes problems as it makes the symbol completely disappear, so
technically it should be sufficient to weaken RAUW to one of the other two
for -O0, but disabling optimizations completely looked like a more
principled solution.

[rjmccall]

Can we suppress this optimization only when we can't emit an alias? An alias shouldn't degrade debugging experience, and it's good to emit less code at -O0.

[labath]

Yes we can do that. Only the RAUW optimization is nasty, I've checked that the
debugger is able to use the alias with no problem.

Updated the patch the reflect that.

[rjmccall]

LGTM, thanks.

[labath]

Thank you for the quick review.

[labath]

I've reverted the patch as it has caused miscompiles on non-trivial inputs. I'll update the patch when I gain a better understanding of what is going on.

[labath]

Updating with a new version of the patch. The problem with the previous version
was that it caused us to use C5/D5 comdats very aggressively (at -O0), and this
is not always correct. This uses a more nuanced approach:

• for local symbols we can use aliases because we don't have to worry about comdats or other compilation units.
• for linkonce symbols, we must emit each structor separately. Theoretically it would be possible to emit a C5 comdat, but then we would need to make sure it contains both (in case of destructors, all three) symbols. As linkonce symbols are emitted lazily, I did not find an easy way to ensure that this is always the case.
• for available_externally, I also emit the structor, as the code seemed to be worried about emitting an alias in this case.
• other linkage types are not affected by the optimization level. They either get put into a comdat (weak) or get aliased (external).

I also add more thorough tests for this behavior.

Overall I am not sure if this extra complexity is worth the few extra aliases we
can emit this way at -O0. Let me know what you think.

[rjmccall]

Well, internal and external types are important cases. I'm fine with this. It's a pity that we can't express what we want with aliases, though.

[labath]

This was reverted in r333482 because it was causing "definition with same mangled name as another definition" errors in some internal google builds.

It turned out this uncovered an (unrelated) issue in module importing. This has now been fixed (r336240), so I'm planning to resubmit this patch. Let me know if you have any concerns.

lib/CodeGen/ItaniumCXXABI.cpp

static StructorCodegen getCodegenToUse(CodeGenModule &CGM,
   if (const auto *DD = dyn_cast<CXXDestructorDecl>(MD)) {
     AliasDecl = GlobalDecl(DD, Dtor_Complete);
   } else {
     const auto *CD = cast<CXXConstructorDecl>(MD);
     AliasDecl = GlobalDecl(CD, Ctor_Complete);
   }
   llvm::GlobalValue::LinkageTypes Linkage = CGM.getFunctionLinkage(AliasDecl);
 
-  if (llvm::GlobalValue::isDiscardableIfUnused(Linkage))
-    return StructorCodegen::RAUW;
+  // All discardable structors can be RAUWed, but we don't want to do that in
+  // unoptimized code, as that makes complete structor symbol disappear
+  // completely, which degrades debugging experience.
+  // Symbols with private linkage can be safely aliased, so we special case them
+  // here.
+  if (llvm::GlobalValue::isLocalLinkage(Linkage))
+    return CGM.getCodeGenOpts().OptimizationLevel > 0 ? StructorCodegen::RAUW
+                                                      : StructorCodegen::Alias;
 
+  // Linkonce structors cannot be aliased nor placed in a comdat, so these need
+  // to be emitted separately.
   // FIXME: Should we allow available_externally aliases?
-  if (!llvm::GlobalAlias::isValidLinkage(Linkage))
-    return StructorCodegen::RAUW;
+  if (llvm::GlobalValue::isDiscardableIfUnused(Linkage) ||
+      !llvm::GlobalAlias::isValidLinkage(Linkage))
+    return CGM.getCodeGenOpts().OptimizationLevel > 0 ? StructorCodegen::RAUW
+                                                      : StructorCodegen::Emit;
 
   if (llvm::GlobalValue::isWeakForLinker(Linkage)) {
     // Only ELF and wasm support COMDATs with arbitrary names (C5/D5).
     if (CGM.getTarget().getTriple().isOSBinFormatELF() ||
         CGM.getTarget().getTriple().isOSBinFormatWasm())
       return StructorCodegen::COMDAT;
     return StructorCodegen::Emit;
   }

test/CodeGenCXX/ctor-dtor-alias.cpp

-// RUN: %clang_cc1 %s -triple i686-linux -emit-llvm -o - -mconstructor-aliases | FileCheck --check-prefix=NOOPT %s
-
+// RUN: %clang_cc1 %s -triple i686-linux -emit-llvm -o - -mconstructor-aliases > %t
+// RUN: FileCheck --check-prefix=NOOPT1 --input-file=%t %s
+// RUN: FileCheck --check-prefix=NOOPT2 --input-file=%t %s
+// RUN: FileCheck --check-prefix=NOOPT3 --input-file=%t %s
 // RUN: %clang_cc1 %s -triple i686-linux -emit-llvm -o - -mconstructor-aliases -O1 -disable-llvm-passes > %t
 // RUN: FileCheck --check-prefix=CHECK1 --input-file=%t %s
 // RUN: FileCheck --check-prefix=CHECK2 --input-file=%t %s
 // RUN: FileCheck --check-prefix=CHECK3 --input-file=%t %s
 // RUN: FileCheck --check-prefix=CHECK4 --input-file=%t %s
 // RUN: FileCheck --check-prefix=CHECK5 --input-file=%t %s
 // RUN: FileCheck --check-prefix=CHECK6 --input-file=%t %s
 
 // RUN: %clang_cc1 %s -triple i686-pc-windows-gnu -emit-llvm -o - -mconstructor-aliases -O1 -disable-llvm-passes | FileCheck --check-prefix=COFF %s
 
 namespace test1 {
 // Test that we produce the appropriate comdats when creating aliases to
 // weak_odr constructors and destructors.
 
 // CHECK1: @_ZN5test16foobarIvEC1Ev = weak_odr alias void {{.*}} @_ZN5test16foobarIvEC2Ev
 // CHECK1: @_ZN5test16foobarIvED1Ev = weak_odr alias void (%"struct.test1::foobar"*), void (%"struct.test1::foobar"*)* @_ZN5test16foobarIvED2Ev
 // CHECK1: define weak_odr void @_ZN5test16foobarIvEC2Ev({{.*}} comdat($_ZN5test16foobarIvEC5Ev)
 // CHECK1: define weak_odr void @_ZN5test16foobarIvED2Ev({{.*}} comdat($_ZN5test16foobarIvED5Ev)
 // CHECK1: define weak_odr void @_ZN5test16foobarIvED0Ev({{.*}} comdat($_ZN5test16foobarIvED5Ev)
 // CHECK1-NOT: comdat
 
+// This should happen regardless of the opt level.
+// NOOPT1: @_ZN5test16foobarIvEC1Ev = weak_odr alias void {{.*}} @_ZN5test16foobarIvEC2Ev
+// NOOPT1: @_ZN5test16foobarIvED1Ev = weak_odr alias void (%"struct.test1::foobar"*), void (%"struct.test1::foobar"*)* @_ZN5test16foobarIvED2Ev
+// NOOPT1: define weak_odr void @_ZN5test16foobarIvEC2Ev({{.*}} comdat($_ZN5test16foobarIvEC5Ev)
+// NOOPT1: define weak_odr void @_ZN5test16foobarIvED2Ev({{.*}} comdat($_ZN5test16foobarIvED5Ev)
+// NOOPT1: define weak_odr void @_ZN5test16foobarIvED0Ev({{.*}} comdat($_ZN5test16foobarIvED5Ev)
+
 // COFF doesn't support comdats with arbitrary names (C5/D5).
 // COFF: define weak_odr {{.*}} void @_ZN5test16foobarIvEC2Ev({{.*}} comdat align
 // COFF: define weak_odr {{.*}} void @_ZN5test16foobarIvEC1Ev({{.*}} comdat align
 // COFF: define weak_odr {{.*}} void @_ZN5test16foobarIvED2Ev({{.*}} comdat align
 // COFF: define weak_odr {{.*}} void @_ZN5test16foobarIvED0Ev({{.*}} comdat align
 
 template <typename T>
 struct foobar {
   foobar() {}
   virtual ~foobar() {}
 };
 
 template struct foobar<void>;
 }
 
 namespace test2 {
-// test that when the destrucor is linkonce_odr we just replace every use of
+// test that when the destructor is linkonce_odr we just replace every use of
 // C1 with C2.
 
 // CHECK1: define internal void @__cxx_global_var_init()
 // CHECK1: call void @_ZN5test26foobarIvEC2Ev
 // CHECK1: define linkonce_odr void @_ZN5test26foobarIvEC2Ev({{.*}} comdat align
+
+// At -O0, we should still emit the complete constructor.
+// NOOPT1: define internal void @__cxx_global_var_init()
+// NOOPT1: call void @_ZN5test26foobarIvEC1Ev
+// NOOPT1: define linkonce_odr void @_ZN5test26foobarIvEC1Ev({{.*}} comdat align
 void g();
 template <typename T> struct foobar {
   foobar() { g(); }
 };
 foobar<void> x;
 }
 
 namespace test3 {
 // test that instead of an internal alias we just use the other destructor
 // directly.
 
 // CHECK1: define internal void @__cxx_global_var_init.1()
 // CHECK1: call i32 @__cxa_atexit{{.*}}_ZN5test312_GLOBAL__N_11AD2Ev
 // CHECK1: define internal void @_ZN5test312_GLOBAL__N_11AD2Ev(
+
+// We can use an alias for internal symbols at -O0.
+// NOOPT2: _ZN5test312_GLOBAL__N_11BD1Ev = internal alias void {{.*}} @_ZN5test312_GLOBAL__N_11BD2Ev
+// NOOPT2: define internal void @__cxx_global_var_init.1()
+// NOOPT2: call i32 @__cxa_atexit{{.*}}_ZN5test312_GLOBAL__N_11BD1Ev
 namespace {
 struct A {
   ~A() {}
 };
 
 struct B : public A {};
 }
 
 B x;
 }
 
 namespace test4 {
   // Test that we don't produce aliases from B to A. We cannot because we cannot
   // guarantee that they will be present in every TU. Instead, we just call
   // A's destructor directly.
 
   // CHECK1: define internal void @__cxx_global_var_init.2()
   // CHECK1: call i32 @__cxa_atexit{{.*}}_ZN5test41AD2Ev
   // CHECK1: define linkonce_odr void @_ZN5test41AD2Ev({{.*}} comdat align
 
-  // test that we don't do this optimization at -O0 so that the debugger can
-  // see both destructors.
-  // NOOPT: define internal void @__cxx_global_var_init.2()
-  // NOOPT: call i32 @__cxa_atexit{{.*}}@_ZN5test41BD2Ev
-  // NOOPT: define linkonce_odr void @_ZN5test41BD2Ev({{.*}} comdat align
+  // Test that we don't do this optimization at -O0 and call the complete
+  // destructor for B instead. This enables the debugger to see both
+  // destructors.
+  // NOOPT2: define internal void @__cxx_global_var_init.2()
+  // NOOPT2: call i32 @__cxa_atexit{{.*}}@_ZN5test41BD1Ev
+  // NOOPT2: define linkonce_odr void @_ZN5test41BD1Ev({{.*}} comdat align
   struct A {
     virtual ~A() {}
   };
   struct B : public A{
     ~B() {}
   };
   B X;
 }
namespace test6 {
   // CHECK3: call i32 @__cxa_atexit({{.*}}@_ZN5test61AD2Ev
 }
 
 namespace test7 {
   // Test that we don't produce an alias from ~B to ~A<int> (or crash figuring
   // out if we should).
   // pr17875.
   // CHECK3: define void @_ZN5test71BD2Ev
+
+  // At -O0, we should emit both destructors, the complete can be an alias to
+  // the base one.
+  // NOOPT3: @_ZN5test71BD1Ev = alias void {{.*}} @_ZN5test71BD2Ev
+  // NOOPT3: define void @_ZN5test71BD2Ev
   template <typename> struct A {
     ~A() {}
   };
   class B : A<int> {
     ~B();
   };
   template class A<int>;
   B::~B() {}

test/CodeGenCXX/float16-declarations.cpp

// CHECK-DAG: store half 0xH63D0, half* %{{.*}}, align 2
   _Float16 f2l = -0.f16;
 // CHECK-DAG: store half 0xH8000, half* %{{.*}}, align 2
 
   _Float16 f3l = 1.000976562;
 // CHECK-DAG: store half 0xH3C01, half* %{{.*}}, align 2
 
   C1 c1(f1l);
 // CHECK-DAG:  [[F1L:%[a-z0-9]+]] = load half, half* %{{.*}}, align 2
-// CHECK-DAG:  call void @_ZN2C1C2EDF16_(%class.C1* %{{.*}}, half %{{.*}})
+// CHECK-DAG:  call void @_ZN2C1C1EDF16_(%class.C1* %{{.*}}, half %{{.*}})
 
   S1<_Float16> s1 = { 132.f16 };
 // CHECK-DAG: @_ZZ4mainE2s1 = private unnamed_addr constant %struct.S1 { half 0xH5820 }, align 2
 // CHECK-DAG:  [[S1:%[0-9]+]] = bitcast %struct.S1* %{{.*}} to i8*
 // CHECK-DAG: call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 2 [[S1]], i8* align 2 bitcast (%struct.S1* @_ZZ4mainE2s1 to i8*), i64 2, i1 false)
 
   _Float16 f4l = func1n(f1l)  + func1f(f2l) + c1.func1c(f3l) + c1.func2c(f1l) +
     func1t(f1l) + s1.mem2 - f1n + f2n;