[Clang/CodeGen] Prevent crash if destructor class is not accessible

[davide]

Summary

Fixes

struct A {

~A();

};

struct B {

A a;

};

struct C {

union {
  B b;
};

~C() noexcept;

};

C::~C() noexcept {}

[davide]

+ Anders Carlsson who originally wrote the code (apparently he's not
on phabricator).
Also the summary of this review should be read as "Prevent crash if
class destructor is not accessible".

[rnk]

Any reason not to add the test case you have?

[davide]

In D10508#189569, @rnk wrote:

Any reason not to add the test case you have?

No. Missed 'svn add' before sending the review. My bad.

[davide]

Test the right condition, add a comment && a test.

[rnk]

lib/CodeGen/CGClass.cpp

1296–1302

I suspect you want to reorder these checks. The triviality check is a bittest, and getDestructor() may perform lookup.

[davide]

Reordered checks as suggested.
Anders Carlsson LGTM'd the change. Reid, do you think we can go forward and check this in?

[rnk]

Lgtm

Sent from phone

[rjmccall]

Um. I don't mind us being a little more defensive in IR-gen, but the right fix for this code is that it needs to be handled / diagnosed by Sema.

[davide]

Thank you for your reply John. Sorry if I submitted this before hearing back from you.
I can either revert or maybe keep it this way (under the assumption the code added isn't wrong) and add open a bug/put a TODO/FIXME in the code.
Also, I would appreciate any guidance on how this can be correctly diagnosed in Sema.

Thanks!

[rsmith]

cfe/trunk/lib/CodeGen/CGClass.cpp

1345 ↗

(On Diff #28090)

The right place to fix this is here. The corresponding code from Sema is:

// The destructor for an implicit anonymous union member is never invoked.
if (FieldClassDecl->isUnion() && FieldClassDecl->isAnonymousStructOrUnion())
  continue;

Since the AST doesn't explicitly list the field destructor class that a destructor will perform, we need to mirror that check here. (Alternatively, we could change Sema to declare a trivial destructor for an anonymous union member, but that seems a little wasteful.)

[rjmccall]

Richard: this code is already testing hasTrivialDestructor. If you would be comfortable with declaring a trivial destructor except for performance reasons, maybe hasTrivialDestructor should be returning true?

[rjmccall]

I'm fine with saying that the destructor is non-trivial when it's deleted like this; it just seemed inconsistent with your statement that we could make Sema declare a trivial destructor but it would be wasteful. Certainly I don't think we want hasTrivialDestructor and getDestructor()->isTrivial() to return different things.

If we're going to leave hasTrivialDestructor returning false, then I agree that the best fix is to repeat the logic from Sema that completely skips the field.

[rjmccall]

If we do that, we should remove the defensive code from IR-gen, because the less-defensive code is serving the admirable purpose of detecting this problem by causing this assertion.

[davide]

New review: http://reviews.llvm.org/D10626

lib/CodeGen/CGClass.cpp

 static bool
 FieldHasTrivialDestructorBody(ASTContext &Context, const FieldDecl *Field);
 
 static bool
 HasTrivialDestructorBody(ASTContext &Context,
                          const CXXRecordDecl *BaseClassDecl,
                          const CXXRecordDecl *MostDerivedClassDecl)
 {
   // If the destructor is trivial we don't have to check anything else.
   if (BaseClassDecl->hasTrivialDestructor())
     return true;
 
+  // Give up if the destructor is not accessible.
+  if (!BaseClassDecl->getDestructor())
+    return false;
+
   if (!BaseClassDecl->getDestructor()->hasTrivialBody())
     return false;
 
   // Check fields.
   for (const auto *Field : BaseClassDecl->fields())
     if (!FieldHasTrivialDestructorBody(Context, Field))
       return false;
 

test/CodeGenCXX/destructor-crash.cpp

+// RUN: %clang_cc1 %s -emit-llvm -std=c++11 -o %t
+
+struct A {
+  ~A();
+};
+
+struct B {
+  A a;
+};
+
+struct C {
+  union {
+    B b;
+  };
+
+  ~C() noexcept;
+};
+
+C::~C() noexcept {}