This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by vsapsai on Dec 10 2018, 7:35 PM.

Details

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

Event Timeline

vsapsai created this revision.Dec 10 2018, 7:35 PM

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?

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

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.

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 planned changes to this revision.Dec 12 2018, 7:48 PM

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 updated this revision to Diff 178563.Dec 17 2018, 4:58 PM
  • 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 added inline comments.Dec 17 2018, 9:22 PM
clang/test/CodeGenObjCXX/inheriting-constructor-cleanup.mm
11

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 updated this revision to Diff 178746.Dec 18 2018, 11:42 AM
  • [ObjC++] Pass struct with __strong field as a value, not as a reference.
rjmccall added inline comments.Dec 18 2018, 12:03 PM
clang/test/CodeGenObjCXX/inheriting-constructor-cleanup.mm
26

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

vsapsai updated this revision to Diff 178921.Dec 19 2018, 10:57 AM
  • [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.

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.

rjmccall accepted this revision.Dec 19 2018, 11:45 AM
This revision is now accepted and ready to land.Dec 19 2018, 11:45 AM
This revision was automatically updated to reflect the committed changes.

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