Page MenuHomePhabricator

[SemaCXX] Mark destructor as referenced
ClosedPublic

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

Event Timeline

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

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.Jul 2 2018, 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.

Does this check destructors of nested aggregate initializations in the case where brace elision occurs? I don't think just checking the top level of the SK_ListInitialization is enough. Perhaps it would make more sense to mark the dtors used from InitListChecker (in non-VerifyOnly mode).

ahatanak updated this revision to Diff 157373.Jul 25 2018, 2:53 PM

Check if destructors are accessible from InitListChecker's constructor. Add test cases for designated initializer and brace elision.

rsmith added inline comments.Aug 30 2018, 6:57 PM
lib/Sema/SemaInit.cpp
829

You also need to check base classes (since C++17, aggregates can have base classes).

829–833

I think this now checks too much: only those subobjects for which we formed an InitListExpr should be checked. Testcase:

struct A { friend struct B; private: ~A(); };
struct B { B(); A a; };
struct C { B b; };
C c = { B(); };

Here, we must not check that A's destructor is accessible from the context of C's initialization.

I think the best thing to do is probably to move the call to this function into InitListChecker::CheckStructUnionTypes, and make it just check one level.

830

Do we need a getBaseElementType() here? (What if the member is of array-of-class type?)

ahatanak updated this revision to Diff 163612.Aug 31 2018, 5:36 PM
ahatanak marked 3 inline comments as done.

Move the call that checks the element's destructor into InitListChecker::CheckStructUnionTypes.

rsmith accepted this revision.Aug 31 2018, 6:37 PM
rsmith added inline comments.
lib/Sema/SemaInit.cpp
1827–1829

Usual Clang convention is to return true on error.

1862

Hmm, I don't think we considered this case when working on the relevant DR. It doesn't make much sense to check the destructors of the inactive union members. I'll take this back to WG21, but let's go with the wording as-is for now.

1869

It's a minor thing, but I think it'd be preferable to point the diagnostic at the relevant init list element, or at the close brace if the initializer was omitted.

This revision is now accepted and ready to land.Aug 31 2018, 6:37 PM
ahatanak updated this revision to Diff 163871.Sep 4 2018, 11:05 AM
ahatanak marked 2 inline comments as done.

Point the diagnostic at either the relevant init list element or at the close brace.

ahatanak added inline comments.Sep 4 2018, 11:07 AM
lib/Sema/SemaInit.cpp
1827–1829

I also renamed the function to hasAccessibleDestructor to make it clearer what the function does.

1869

The function that checks the destructor (hasAccessibleDestructor) is called in five different places now.

This revision was automatically updated to reflect the committed changes.
orivej added a subscriber: orivej.Sat, Dec 8, 3:49 PM

The committed test does not crash Clang 7, but the following test does, yet it compiles without any warnings by the current Clang trunk thanks to this fix.

struct A { ~A(); };
struct B : A {};
struct C { C();  B b; };
struct D { C c, d; };
D f() { return {}; }

Always worth adding more tests. Mind writing that one up as a commit?

orivej added a comment.Sat, Dec 8, 4:29 PM

Actually, arc-list-init-destruct.mm crashes Clang 7 with the same backtrace as this test case, and Clang trunk generates similar assembly (to the the point that I could essentially copy the // CHECK comments from the .mm test to a .cpp test), so I'm not sure if adding a .cpp test is valuable…

Yeah, if we weren't exercising the code in our test suite, that would be one thing, but I think a language-mode-specific test probably isn't too valuable.

orivej added a comment.Sun, Dec 9, 4:26 AM

I have noticed that this change breaks seemingly valid code:

class A { protected: ~A(); };
struct B : A {};
B f() { return B(); }
B g() { return {}; }

f compiles, but g fails with temporary of type 'A' has protected destructor. (g++ 8.2 compiles this file.)

I have noticed that this change breaks seemingly valid code:

class A { protected: ~A(); };
struct B : A {};
B f() { return B(); }
B g() { return {}; }

f compiles, but g fails with temporary of type 'A' has protected destructor. (g++ 8.2 compiles this file.)

We talked about this in the analysis for https://reviews.llvm.org/D53860, and Richard decided that it is indeed invalid under the standard.

As a language designer I'm not sure that's a good language rule, but it's the realm of a standard defect, not a compiler bug.