[SemaCXX] Mark destructor as referenced
Needs ReviewPublic

Authored by ahatanak on Apr 20 2018, 12:48 PM.

Details

Summary

If an initializer in a braced-init-list is a C++ class that has a non-trivial destructor, mark the destructor as referenced. This fixes a crash in CodeGenFunction::destroyCXXObject that occurs when it tries to emit a call to the destructor on the stack unwinding path but the CXXRecordDecl of the class doesn't have a CXXDestructorDecl for the destructor.

Diff Detail

Repository
rC Clang
ahatanak created this revision.Apr 20 2018, 12:48 PM
rjmccall added inline comments.
lib/Sema/SemaInit.cpp
7090

If we're actually using the destructor, we can't just look it up and mark it referenced; we have to call CheckDestructorAccess and DiagnoseUseOfDecl.

Walking the syntactic initializers like this seems wrong:

  • I don't know if we actually promise to rewrite these expressions to the actual recursive element initialization expression if the expression requires

conversion.

  • This is going to miss implicitly-initialized elements, which can happen for a variety of reasons including (1) running out of explicit initializers and (2) designated initializers skipping a field.
  • We'll end up with redundant diagnostics from CheckDestructorAccess/DiagnoseUseOfDecl if the expression is an already-materialized temporary.
  • We'll end up with unwanted diagnostics from CheckDestructorAccess/DiagnoseUseOfDecl if the expression is a gl-value and actually part of a reference-initialization, where we don't actually use the destructor.

I think the right solution is probably that list-initialization needs to recognize that the initialization of a record needs to be treated as a potential use of the destructor even if we aren't supposed to bind it as a temporary, because once an object is initialized in the local context, there are all sorts of reasons why we might need to call its destructor before the initialization completes/transitions. Basically every call to shouldBindAsTemporary in this file is suspect, because they're all places where we might need to check the destructor use even if we didn't make a temporary.

We seem to get this right in SK_UserConversion where we call shouldDestroyEntity, and the comment there is spot-on — it doesn't make sense for this code to be specific to SK_UserConversion at all. My tentative suggestion would be to (1) make a helper function that does that exact "if should-bind then bind else if should-destroy then destroy" dance and then (2) look at all the should-bind checks and try to use your helper function instead. But I'd really like Richard and/or Doug to weigh in.

test/CodeGenObjCXX/arc-list-init-destruct.mm
2

Does the corresponding C++ test case (replacing Class0 *f; with HasExplicitNonTrivialDestructor f;) not reproduce the problem?

Richard and Doug, do you have any thoughts on John's suggestion?

test/CodeGenObjCXX/arc-list-init-destruct.mm
2

I wasn't able to reproduce the problem by changing the type of field 'f' to a C++ class with a non-trivial destructor because, if I make that change, Class1's destructor declaration gets added in Sema::AddImplicitlyDeclaredMembersToClass. I don't fully understand the reason behind it, but Class1's destructor declaration is added when the type of one of its subobject has a user-declared destructor.

As it happens, the C++ committee fixed the language wording hole here very recently. The new rule can be found here: http://wg21.link/p0968r0#2227
In summary: we should to consider the destructor for all elements of the aggregate to be potentially-invoked.

@rjmccall Were you suggesting that we should also consider the destructor of the aggregate itself to be potentially-invoked, or just the elements? In the new-expression case at least, I don't think the former is permitted.

I agree that the new-expression case doesn't use the destructor, and all the other cases of list-initialization presumably use the destructor for the initialized type for separate reasons. Ok.

test/CodeGenObjCXX/arc-list-init-destruct.mm
2

Interesting, alright.

As it happens, the C++ committee fixed the language wording hole here very recently. The new rule can be found here: http://wg21.link/p0968r0#2227
In summary: we should to consider the destructor for all elements of the aggregate to be potentially-invoked.

It doesn't mean that clang should reject the following code, does it?

// This should compile fine as long as 'Deleted7d d7d' is commented out.
struct DeletedDtor { ~DeletedDtor() = delete; };
struct Deleted7d { DeletedDtor a = {}; }; 
//Deleted7d d7d;

I tried making a helper function out of the code in SK_UserConversion and using it in "case SK_ListInitialization". That doesn't seem to work because DiagnoseUseOfDecl rejects the code (with error message "error: attempt to use a deleted function") above even though the destructor isn't needed (because 'd7d' is commented out), so I guess that check should be done in another place.

ahatanak updated this revision to Diff 153779.Mon, Jul 2, 2:47 PM

Implement the new rule defined here: http://wg21.link/p0968r0#2227. Produce a diagnostic if a class is initialized with aggregate initialization and one of its element's destructor is not accessible.

LGTM, but I'd like Richard to sign off, too.