Devirtualize destructor of final class.

[hjyamauchi]

Summary

Take advantage of the final keyword to devirtualize destructor calls.

Fix https://bugs.llvm.org/show_bug.cgi?id=21368

[rsmith]

lib/CodeGen/CGExprCXX.cpp

1867–1868 ↗

(On Diff #204143)

Can we use getDevirtualizedMethod here instead? That catches a few other cases that just checking for final will miss, and disables this for cases where it will currently do the wrong thing (eg, -fapple-kext). See EmitCXXMemberOrOperatorMemberCallExpr for where we do this for other virtual calls.

As a particularly terrible example:

struct A { virtual ~A() final = 0; };
void evil(A *p) { delete p; }

is (amazingly) valid, and must not be devirtualized because there is no requirement that the destructor of A have a definition in this program. (It would, however, be correct to optimize out the entire delete expression in this case, on the basis that p must always be nullptr whenever it's reached. But that doesn't really seem worthwhile.)

[hjyamauchi]

Using getDevirtualizedMethod.

[hjyamauchi]

lib/CodeGen/CGExprCXX.cpp

1867–1868 ↗

(On Diff #204143)

Updated. Hopefully this does the right thing.

[rsmith]

lib/CodeGen/CGExprCXX.cpp

1871 ↗

(On Diff #204365)

Dtor could be the destructor for a type derived from ElementPtr. We'll either need to somehow emit a cast to the correct derived type here, or just abort the devirtualization and emit a virtual call in that case. (The latter is what EmitCXXMemberOrOperatorMemberCallExpr does -- see its checks on getCXXRecord(Base) and getCXXRecord(Inner).)

Eg, for:

struct SomethingElse { virtual void f(); };
struct A {
  virtual ~A();
};
struct B : SomethingElse, A {
  ~B() final;
};
void f(B *p) {
  delete (A*)p;
}

... Ptr will be a pointer to an A subobject of a B object, but getDevirtualizedMethod will still be able to work out that B::~B is the right thing to call here (by looking through the cast and finding that the pointer must actually point to a B object whose destructor is final). We need to either convert Ptr back from an A* to a B* or just not devirtualize this call.

[hjyamauchi]

Addressed comment.

[hjyamauchi]

lib/CodeGen/CGExprCXX.cpp

1871 ↗

(On Diff #204365)

Should be good now.

[rsmith]

lib/CodeGen/CGExprCXX.cpp

1887 ↗

(On Diff #204852)

In this case we'll emit the inner expression (including its side-effects) twice.

The simplest thing to do would be to just drop this else if case for now and add a FIXME.

1867–1868 ↗

(On Diff #204143)

Please can you add that test to your testcases too (eg, with a CHECK-NOT to ensure we don't reference the destructor of A)? It's subtle enough that I could imagine us getting that wrong in the future.

[hjyamauchi]

lib/CodeGen/CGExprCXX.cpp

1887 ↗

(On Diff #204852)

I'd go with that. I assume there isn't a simple way to adjust the this pointer of a base class given a derived class? Sort of like CodeGenFunction::GetAddressOfDerivedClass?

[rsmith]

lib/CodeGen/CGExprCXX.cpp

1887 ↗

(On Diff #204852)

I don't think we have anything quite like that. This won't be possible in the general case: there could be more than one base class of the current type within the derived class, so you'd need to take into account the path that the base expression took to reach the inner expression with the known dynamic type. (We could opportunistically do this in the common case where the derived class has only one base class of the base type, and it's a non-virtual base, but I don't think we have any existing code to do that either.)

[hjyamauchi]

Updated.

[rsmith]

Thanks! Minor comment then feel free to commit.

lib/CodeGen/CGExprCXX.cpp

1877 ↗

(On Diff #205381)

There's no guarantee that you get the same declaration of the class in both cases; use declaresSameEntity here instead of == to compare whether you have the same class.

[rsmith]

test/CodeGenCXX/devirtualize-dtor-final.cpp

20 ↗

(On Diff #205381)

Missing space after void here

[hjyamauchi]

Comments addressed.

[xbolva00]

https://reviews.llvm.org/rL364359

Why this was reverted ?

[xbolva00]

You, @rdhindsa, should report here why you reverted this commit.

It is really weird to revert random commits without more information.

[hjyamauchi]

This was reverted due to some internal test failure. But it turned out a false alarm. I'll work on recommitting it.

[hjyamauchi]

Recommitted as https://reviews.llvm.org/rL365509

cfe/trunk/lib/CodeGen/CGExprCXX.cpp

static void EmitObjectDelete(CodeGenFunction &CGF,
   // destructor is virtual, we'll just emit the vcall and return.
   const CXXDestructorDecl *Dtor = nullptr;
   if (const RecordType *RT = ElementType->getAs<RecordType>()) {
     CXXRecordDecl *RD = cast<CXXRecordDecl>(RT->getDecl());
     if (RD->hasDefinition() && !RD->hasTrivialDestructor()) {
       Dtor = RD->getDestructor();
 
       if (Dtor->isVirtual()) {
+        bool UseVirtualCall = true;
+        const Expr *Base = DE->getArgument();
+        if (auto *DevirtualizedDtor =
+                dyn_cast_or_null<const CXXDestructorDecl>(
+                    Dtor->getDevirtualizedMethod(
+                        Base, CGF.CGM.getLangOpts().AppleKext))) {
+          UseVirtualCall = false;
+          const CXXRecordDecl *DevirtualizedClass =
+              DevirtualizedDtor->getParent();
+          if (declaresSameEntity(getCXXRecord(Base), DevirtualizedClass)) {
+            // Devirtualized to the class of the base type (the type of the
+            // whole expression).
+            Dtor = DevirtualizedDtor;
+          } else {
+            // Devirtualized to some other type. Would need to cast the this
+            // pointer to that type but we don't have support for that yet, so
+            // do a virtual call. FIXME: handle the case where it is
+            // devirtualized to the derived type (the type of the inner
+            // expression) as in EmitCXXMemberOrOperatorMemberCallExpr.
+            UseVirtualCall = true;
+          }
+        }
+        if (UseVirtualCall) {
           CGF.CGM.getCXXABI().emitVirtualObjectDelete(CGF, DE, Ptr, ElementType,
                                                       Dtor);
           return;
         }
       }
     }
+  }
 
   // Make sure that we call delete even if the dtor throws.
   // This doesn't have to a conditional cleanup because we're going
   // to pop it off in a second.
   CGF.EHStack.pushCleanup<CallObjectDelete>(NormalAndEHCleanup,
                                             Ptr.getPointer(),
                                             OperatorDelete, ElementType);
 

cfe/trunk/test/CodeGenCXX/devirtualize-dtor-final.cpp

+// RUN: %clang_cc1 -triple i386-unknown-unknown -std=c++11 %s -emit-llvm -o - | FileCheck %s
+
+namespace Test1 {
+  struct A { virtual ~A() {} };
+  struct B final : A {};
+  struct C : A { virtual ~C() final {} };
+  struct D { virtual ~D() final = 0; };
+  // CHECK-LABEL: define void @_ZN5Test13fooEPNS_1BE
+  void foo(B *b) {
+    // CHECK: call void @_ZN5Test11BD1Ev
+    delete b;
+  }
+  // CHECK-LABEL: define void @_ZN5Test14foo2EPNS_1CE
+  void foo2(C *c) {
+    // CHECK: call void @_ZN5Test11CD1Ev
+    delete c;
+  }
+  // CHECK-LABEL: define void @_ZN5Test14evilEPNS_1DE
+  void evil(D *p) {
+    // CHECK-NOT: call void @_ZN5Test11DD1Ev
+    delete p;
+  }
+}