[CodeGen] Add cleanup scope to EmitInlinedInheritingCXXConstructorCall

[smeenai]

Summary

EmitInlinedInheritingCXXConstructorCall may result in a CallBaseDtor
cleanup being pushed. That cleanup would then be popped when the CGF's
CurCodeDecl no longer points to the method which triggered the cleanup,
leading to a failed cast. Create a new cleanup scope to ensure that the
cleanup gets popped while the CurCodeDecl still points to the method.
Fixes PR36748.

[smeenai]

Argh, this fixes my test case, but causes failures in some other ones. Back to the drawing board, I guess.

[rnk]

Code looks good, but we should make the test more robust and self-documenting.

test/CodeGenCXX/inheriting-constructor-cleanup.cpp

22

The landingpad should be trivially dead, since T has nothing that throws in it, right? Clang is usually pretty smart about not emitting unused exceptional-only destructors, so I'd try to defend against it getting smart in the future. I'd put a function call foo in T(int, ...), then CHECK for the invoke of it, and that it unwinds to a landingpad preceding this destructor call.

[smeenai]

Thanks Reid. I still need to look into why this is causing some existing tests to crash, but I'll also adjust the test.

[rsmith]

lib/CodeGen/CGClass.cpp

2183

Please add a test to ensure that we still destroy function parameters in the right order and at the right times (for both the exceptional and non-exceptional cleanup cases).

test/CodeGenCXX/inheriting-constructor-cleanup.cpp

22

Maybe remove some of the function bodies so that we can't statically tell there is no possibility of unwind?

[smeenai]

Finally got a chance to dig into some of the failures this patch is causing. Here's an example crasher (run with clang -cc1 -triple i686-windows -emit-llvm):

struct Q {
  Q(int);
  ~Q();
};
struct Z {};
struct A {
  A(Q);
};
struct B : Z, A {
  using A::A;
};
B b(Q(1));

What's happening here is that we emit the inlined inherited constructor call (B's using A::A), which in turn emits the constructor call (A::A), which eventually winds up in CodeGenFunction::EmitCall, which calls deactivateArgCleanupsBeforeCall, which cleans the cleanup stack. However, the RunCleanupsScope that I added thinks it has cleanups to do, because the EHStack's stable_begin iterator holds a different (empty) value at its destruction than it did at its construction. Of course, the cleanup stack is actually empty at this point, so attempting to run cleanups just asserts.

I guess what I want is for my added scope to only apply to the cleanups added for base destructor calls (since any cleanups for arguments will be handled by deactivateArgCleanupsBeforeCall), but I'm not sure how best to accomplish that.

CodeGenFunction::EmitConstructorBody also wraps a RunCleanupsScope around its EmitCtorPrologue call, so in theory it should be affected by the same problem. I'll try to craft a test.

[smeenai]

This doesn't pass all tests right now, and I'll also need to change the test in accordance with the review comments.

[smeenai]

https://reviews.llvm.org/D55543 does the same thing

lib/CodeGen/CGClass.cpp

 }
 
 void CodeGenFunction::EmitInlinedInheritingCXXConstructorCall(
     const CXXConstructorDecl *Ctor, CXXCtorType CtorType, bool ForVirtualBase,
     bool Delegating, CallArgList &Args) {
   GlobalDecl GD(Ctor, CtorType);
   InlinedInheritingConstructorScope Scope(*this, GD);
   ApplyInlineDebugLocation DebugScope(*this, GD);
+  RunCleanupsScope CleanupScope(*this);
 
   // Save the arguments to be passed to the inherited constructor.
   CXXInheritedCtorInitExprArgs = Args;
 
   FunctionArgList Params;
   QualType RetType = BuildFunctionArgList(CurGD, Params);
   FnRetTy = RetType;
 

test/CodeGenCXX/inheriting-constructor-cleanup.cpp

+// RUN: %clang_cc1 -triple x86_64-linux -fexceptions -emit-llvm -o - %s | FileCheck %s
+
+// PR36748
+struct S {
+  ~S() {}
+};
+
+struct T {
+  T(int, ...) {}
+};
+
+struct U : public S, public T {
+  using T::T;
+};
+
+void f() {
+  U(0);
+}
+
+// ~S cleanup should be emitted rather than crashing
+// CHECK-LABEL: define void @_Z1fv
+// CHECK: call void @_ZN1SD2Ev