[CodeGen] Fix assertion on throwing object with inlined inherited constructor and non-trivial destructor.

[vsapsai]

Summary

Fixes assertion

Assertion failed: (isa<X>(Val) && "cast<Ty>() argument of incompatible type!"), function cast, file llvm/Support/Casting.h, line 255.

It was triggered by trying to cast FunctionDecl to CXXMethodDecl as
CGF.CurCodeDecl in CallBaseDtor::Emit. It was happening because
cleanups were emitted in ScalarExprEmitter::VisitExprWithCleanups
after destroying InlinedInheritingConstructorScope, so
CodeGenFunction.CurCodeDecl didn't correspond to expected cleanup decl.

Fix the assertion by emitting cleanups before leaving
InlinedInheritingConstructorScope and changing CurCodeDecl.

rdar://problem/45805151

[rjmccall]

Nice catch. This also fixes problems where there might be cleanups entered by EmitParmDecl, e.g. in ObjC++ with a parameter of type struct A { __strong id x; }; could you please add a test for that and verify that the parameter is destroyed at an appropriate point in the inlined call?

[smeenai]

I'd tried this exact same patch back in https://reviews.llvm.org/D44619, but I was running into a bunch of check-clang failures with it, and I was never able to figure them out. It looks like it works now though, so I'm glad :) Richard's comment from that diff might still be relevant:

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).

This will also resolve https://bugs.llvm.org/show_bug.cgi?id=36748

[vsapsai]

In D55543#1328025, @rjmccall wrote:

Nice catch. This also fixes problems where there might be cleanups entered by EmitParmDecl, e.g. in ObjC++ with a parameter of type struct A { __strong id x; }; could you please add a test for that and verify that the parameter is destroyed at an appropriate point in the inlined call?

Aha, I see we have at least EHStack.pushCleanup<ConsumeARCParameter> in EmitParmDecl. Will make sure to add a test that executes this code path.

[vsapsai]

In D55543#1328034, @smeenai wrote:

I'd tried this exact same patch back in https://reviews.llvm.org/D44619, but I was running into a bunch of check-clang failures with it, and I was never able to figure them out. It looks like it works now though, so I'm glad :) Richard's comment from that diff might still be relevant:

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).

This will also resolve https://bugs.llvm.org/show_bug.cgi?id=36748

Thanks, Shoaib, for extra information. Maybe I don't see any failing tests because I'm not testing on Windows. At least I want to try a sample you've provided in https://reviews.llvm.org/D44619#1056458

[vsapsai]

In D55543#1329288, @vsapsai wrote:

In D55543#1328025, @rjmccall wrote:

Nice catch. This also fixes problems where there might be cleanups entered by EmitParmDecl, e.g. in ObjC++ with a parameter of type struct A { __strong id x; }; could you please add a test for that and verify that the parameter is destroyed at an appropriate point in the inlined call?

Aha, I see we have at least EHStack.pushCleanup<ConsumeARCParameter> in EmitParmDecl. Will make sure to add a test that executes this code path.

The way I understand the comment, we cannot make a parameter of type struct A to have a cleanup specific to CodeGenFunction::EmitInlinedInheritingCXXConstructorCall. It calls EmitParmDecl but does it only for implicit parameters. And I am struggling to make struct A an implicit parameter.

Nevertheless, my change causes different exception-handling behaviour. Need to check further if it is ARC-related or inherited constructor-related.

[vsapsai]

• Test order of destroying parameters to inlined inherited constructor.
• [ObjC++] Add test using struct with __strong field as a parameter for inlined inherited constructor.

[rjmccall]

clang/test/CodeGenObjCXX/inheriting-constructor-cleanup.mm

10 ↗

(On Diff #178563)

To test what I'd like to see, this needs to take Strong by value, not by reference. There won't be a cleanup for the inlined parameter if it's taken by reference.

[vsapsai]

• [ObjC++] Pass struct with __strong field as a value, not as a reference.

[rjmccall]

clang/test/CodeGenObjCXX/inheriting-constructor-cleanup.mm

25 ↗

(On Diff #178746)

Can you include checks for all the calls in this function, with CHECK-NOTs to prove that there aren't any other calls?

[vsapsai]

• [ObjC++] Verify there are no unexpected calls.

--implicit-check-not seems reasonable approach in this case. It causes adding
Inheritor destructors but gives higher confidence there are no unexpected
calls in unexpected places.

Escaping a space after check-not pattern to avoid matching attribute
disable-tail-calls.

[rjmccall]

Thanks, LGTM. Somewhat surprised that we don't have an implicit copy of the Strong argument, but it's not a bad thing that we don't.

[vsapsai]

Thanks for the review, John. Committed the change and will watch the buildbots on non-Darwin platform.

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 RunCleanups(*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-darwin -std=c++11 -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-darwin -std=c++11 -fcxx-exceptions -fexceptions -emit-llvm -o - %s | FileCheck %s --check-prefix=EXCEPTIONS
+
+// PR36748
+// rdar://problem/45805151
+
+// Classes to verify order of destroying function parameters.
+struct S1 {
+  ~S1();
+};
+struct S2 {
+  ~S2();
+};
+
+struct Base {
+  // Use variadic args to cause inlining the inherited constructor.
+  Base(const S1&, const S2&, const char *fmt, ...) {}
+};
+
+struct NonTrivialDtor {
+  ~NonTrivialDtor() {}
+};
+struct Inheritor : public NonTrivialDtor, public Base {
+  using Base::Base;
+};
+
+void f() {
+  Inheritor(S1(), S2(), "foo");
+  // CHECK-LABEL: define void @_Z1fv
+  // CHECK: %[[TMP1:.*]] = alloca %struct.S1
+  // CHECK: %[[TMP2:.*]] = alloca %struct.S2
+  // CHECK: call void (%struct.Base*, %struct.S1*, %struct.S2*, i8*, ...) @_ZN4BaseC2ERK2S1RK2S2PKcz(%struct.Base* {{.*}}, %struct.S1* dereferenceable(1) %[[TMP1]], %struct.S2* dereferenceable(1) %[[TMP2]], i8* {{.*}})
+  // CHECK-NEXT: call void @_ZN9InheritorD1Ev(%struct.Inheritor* {{.*}})
+  // CHECK-NEXT: call void @_ZN2S2D1Ev(%struct.S2* %[[TMP2]])
+  // CHECK-NEXT: call void @_ZN2S1D1Ev(%struct.S1* %[[TMP1]])
+
+  // EXCEPTIONS-LABEL: define void @_Z1fv
+  // EXCEPTIONS: %[[TMP1:.*]] = alloca %struct.S1
+  // EXCEPTIONS: %[[TMP2:.*]] = alloca %struct.S2
+  // EXCEPTIONS: invoke void (%struct.Base*, %struct.S1*, %struct.S2*, i8*, ...) @_ZN4BaseC2ERK2S1RK2S2PKcz(%struct.Base* {{.*}}, %struct.S1* dereferenceable(1) %[[TMP1]], %struct.S2* dereferenceable(1) %[[TMP2]], i8* {{.*}})
+  // EXCEPTIONS-NEXT: to label %[[CONT:.*]] unwind label %[[LPAD:.*]]
+
+  // EXCEPTIONS: [[CONT]]:
+  // EXCEPTIONS-NEXT: call void @_ZN9InheritorD1Ev(%struct.Inheritor* {{.*}})
+  // EXCEPTIONS-NEXT: call void @_ZN2S2D1Ev(%struct.S2* %[[TMP2]])
+  // EXCEPTIONS-NEXT: call void @_ZN2S1D1Ev(%struct.S1* %[[TMP1]])
+
+  // EXCEPTIONS: [[LPAD]]:
+  // EXCEPTIONS: call void @_ZN14NonTrivialDtorD2Ev(%struct.NonTrivialDtor* {{.*}})
+  // EXCEPTIONS-NEXT: call void @_ZN2S2D1Ev(%struct.S2* %[[TMP2]])
+  // EXCEPTIONS-NEXT: call void @_ZN2S1D1Ev(%struct.S1* %[[TMP1]])
+}

test/CodeGenObjCXX/inheriting-constructor-cleanup.mm

+// RUN: %clang_cc1 -triple x86_64-darwin -std=c++11 -fobjc-arc -emit-llvm -o - %s | FileCheck %s --implicit-check-not "call\ "
+// rdar://problem/45805151
+
+struct Strong {
+  __strong id x;
+};
+
+struct Base {
+  // Use variadic args to cause inlining the inherited constructor.
+  Base(Strong s, ...) {}
+};
+
+struct NonTrivialDtor {
+  ~NonTrivialDtor() {}
+};
+struct Inheritor : public NonTrivialDtor, public Base {
+  using Base::Base;
+};
+
+id g(void);
+void f() {
+  Inheritor({g()});
+}
+// CHECK-LABEL: define void @_Z1fv
+// CHECK:       %[[TMP:.*]] = call i8* @_Z1gv()
+// CHECK:       {{.*}} = call i8* @objc_retainAutoreleasedReturnValue(i8* %[[TMP]])
+// CHECK:       call void (%struct.Base*, i8*, ...) @_ZN4BaseC2E6Strongz(%struct.Base* {{.*}}, i8* {{.*}})
+// CHECK-NEXT:  call void @_ZN9InheritorD1Ev(%struct.Inheritor* {{.*}})
+
+// CHECK-LABEL: define linkonce_odr void @_ZN4BaseC2E6Strongz(%struct.Base* {{.*}}, i8* {{.*}}, ...)
+// CHECK:       call void @_ZN6StrongD1Ev(%struct.Strong* {{.*}})
+
+// CHECK-LABEL: define linkonce_odr void @_ZN9InheritorD1Ev(%struct.Inheritor* {{.*}})
+// CHECK:       call void @_ZN9InheritorD2Ev(%struct.Inheritor* {{.*}})
+
+// CHECK-LABEL: define linkonce_odr void @_ZN6StrongD1Ev(%struct.Strong* {{.*}})
+// CHECK:       call void @_ZN6StrongD2Ev(%struct.Strong* {{.*}})
+
+// CHECK-LABEL: define linkonce_odr void @_ZN6StrongD2Ev(%struct.Strong* {{.*}})
+// CHECK:       call void @objc_storeStrong(i8** {{.*}}, i8* null)
+
+// CHECK-LABEL: define linkonce_odr void @_ZN9InheritorD2Ev(%struct.Inheritor* {{.*}})
+// CHECK:       call void @_ZN14NonTrivialDtorD2Ev(%struct.NonTrivialDtor* {{.*}})