[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);
 
+  // Only use RAUW in optimized code, as it makes the complete structor symbol
+  // disappear completely, which degrades debugging experience.
+  if (CGM.getCodeGenOpts().OptimizationLevel > 0) {
     if (llvm::GlobalValue::isDiscardableIfUnused(Linkage))
       return StructorCodegen::RAUW;
 
     // FIXME: Should we allow available_externally aliases?
     if (!llvm::GlobalAlias::isValidLinkage(Linkage))
       return StructorCodegen::RAUW;
+  }
 
   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

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.
+  // 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.
+  // NOOPT: @_ZN5test41BD1Ev = linkonce_odr alias void {{.*}} @_ZN5test41BD2Ev
   // NOOPT: define internal void @__cxx_global_var_init.2()
-  // NOOPT: call i32 @__cxa_atexit{{.*}}@_ZN5test41BD2Ev
-  // NOOPT: define linkonce_odr void @_ZN5test41BD2Ev({{.*}} comdat align
+  // NOOPT: call i32 @__cxa_atexit{{.*}}@_ZN5test41BD1Ev
   struct A {
     virtual ~A() {}
   };
   struct B : public A{
     ~B() {}
   };
   B X;
 }

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;