Page MenuHomePhabricator

[SemaCXX] Don't check base's dtor is accessible
AbandonedPublic

Authored by modocache on Oct 29 2018, 9:57 PM.

Details

Summary

In https://reviews.llvm.org/D45898?id=157373, @rsmith advises to move a
checkMemberDestructor into InitListChecker::CheckStructUnionTypes,
to check base classes for an accessible destructor when performing
aggregate initialization. However, in so doing, the following C++ code
now fails to compile:

class Base { protected: ~Base() = default; };
struct Derived : Base {};
void foo() { Derived d = {}; }

GCC 8.2 and Clang 7.0.0 both treat this C++ code as valid, whereas
Clang trunk emits the following error diagnostic:

<source>:3:27: error: temporary of type 'Base' has protected destructor
void foo() { Derived d = {}; }
                          ^
<source>:1:25: note: declared protected here
class Base { protected: ~Base() = default; };
                        ^

I think Clang trunk is wrong about the validity of this C++ code: my
understanding of the C++17 standard is that the Base destructor need
only be accessible from within the context of Derived, and not at the
context in which Derived is aggregate initialized within the foo
function.

Add a test for the case above, and modify the destructor access check
within InitListChecker::CheckStructUnionTypes to check only that the
derived class has an accessible destructor.

Test Plan: check-clang

Diff Detail

Event Timeline

modocache created this revision.Oct 29 2018, 9:57 PM

Here's a compiler explorer link demonstrating that GCC 8.2 and Clang 7.0.0 compile the example code, but Clang trunk emits an error: https://godbolt.org/z/l3baI_

I realize the proposed "fix" here isn't likely to be exactly correct, but I thought it could be a good starting point. I'd also like to confirm that my example is, in fact, valid C++ code.

Any and all reviews appreciated!

That's interesting. If you think of a list-initialization of an aggregate as effectively defining an *ad hoc* constructor for it, then yes, we clearly ought to have access to protected destructors of base classes. And that aligns with the intuition that people make their destructors protected in order to prevent types from being constructed except as base sub-objects, which is still valid here. But at the same time, at a low level, we are directly accessing a protected destructor from a context that is not code actually defined in a subclass.

http://wg21.link/p0968r0#2227 says:

The destructor for each element of class type is potentially invoked (15.4 [class.dtor]) from the context where the aggregate initialization occurs. [Note: This provision ensures that destructors can be called for fully-constructed subobjects in case an exception is thrown (18.2 [except.ctor]). —end note]

And '15.4 Destructors' has this sentence:
A program is ill-formed if a destructor that is potentially invoked is deleted or not accessible from the context of the invocation.

I'm not sure what "context" is in the case above (foo or Derived?), but if it's foo, it seems like clang is correct.

twoh added a subscriber: twoh.Oct 30 2018, 12:56 PM

I think the context is Derived here. My understanding of http://wg21.link/p0968r0#2227 (in this patch's context) is that when Derived is aggregate initialized, the destructor for each element of Base is potentially invoked as well.

For me it seems that the bug is checking destructor accessibility of Base itself, not fields of Base. I think the right fix for 1956-1960 is

if (!VerifyOnly) {
  auto *BaseRD = Base.getType()->getAs<RecordType>()->getDecl();
  for (FieldDecl *FD : BaseRD->fields()) {
    QualType ET = SemaRef.Context.getBaseElementType(FD->getType());
    if (hasAccessibleDestructor(ET, InitLoc, SemaRef)) {
      hadError = true;
      return;
    }
  }
}

For me it seems that the bug is checking destructor accessibility of Base itself, not fields of Base.

The code is correct (or at least, following the current standard rule) as-is. The base class subobject itself is an element of the aggregate. Note that aggregate-initialization of Derived does not require Base to be an aggregate at all, so recursing to the fields of Base would be inappropriate.

If this is causing significant problems for real code, we can certainly try to take this back to the C++ committee and request a do-over, but I think it's unlikely the rule would be changed: for aggregate initialization, direct base class subobjects are (by design) treated exactly the same as fields, and the destructors for all such subobjects are potentially invoked by aggregate initialization (because if an exception is thrown after an element is initialized and before initialization of the complete object finishes, the element's destructor will be invoked). You could argue that the context for the access check should be the derived class and not the context in which aggregate initialization is performed. I've checked the minutes, and we did indeed discuss that when resolving C++ DR 2227, and decided that the context for the access check should be the context of the aggregate initialization, not the context of the class definition.

For what it's worth, this is not the only way that the C++17 extensions to aggregate initialization break existing code. There are easy workarounds: make sure that your class is not an aggregate (add a constructor), or don't use list-initialization.

twoh added a comment.Oct 30 2018, 6:48 PM

@rsmith I see. Thank you for the clarification!

modocache abandoned this revision.Oct 30 2018, 7:35 PM

Yes, thanks @rsmith! And sorry @ahatanak for the trouble of explaining your code here.

The new standards behavior does impact @twoh and I's codebase in a few places, but as you explained we can simply change the source to not use list-initialization.

Thanks all!

orivej added a subscriber: orivej.Dec 9 2018, 11:57 AM