[clang] aade5fb - Fix wrong devirtualization when the final overrider in one base class

[Richard Smith via cfe-commits]

Author: Richard Smith
Date: 2020-01-31T17:06:48-08:00
New Revision: aade5fbbfef3e8555df202082bea905deebc2ca5

URL: https://github.com/llvm/llvm-project/commit/aade5fbbfef3e8555df202082bea905deebc2ca5
DIFF: https://github.com/llvm/llvm-project/commit/aade5fbbfef3e8555df202082bea905deebc2ca5.diff

LOG: Fix wrong devirtualization when the final overrider in one base class
overrides the final overrider in a different base class.

Added: 
    

Modified: 
    clang/lib/AST/CXXInheritance.cpp
    clang/lib/AST/DeclCXX.cpp
    clang/test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/AST/CXXInheritance.cpp b/clang/lib/AST/CXXInheritance.cpp
index a3a3794b2edd..0377bd324cb6 100644
--- a/clang/lib/AST/CXXInheritance.cpp
+++ b/clang/lib/AST/CXXInheritance.cpp
@@ -758,6 +758,8 @@ CXXRecordDecl::getFinalOverriders(CXXFinalOverriderMap &FinalOverriders) const {
         return false;
       };
 
+      // FIXME: IsHidden reads from Overriding from the middle of a remove_if
+      // over the same sequence! Is this guaranteed to work?
       Overriding.erase(
           std::remove_if(Overriding.begin(), Overriding.end(), IsHidden),
           Overriding.end());

diff  --git a/clang/lib/AST/DeclCXX.cpp b/clang/lib/AST/DeclCXX.cpp
index 48e310e858b2..227fe80ccab4 100644
--- a/clang/lib/AST/DeclCXX.cpp
+++ b/clang/lib/AST/DeclCXX.cpp
@@ -2038,17 +2038,36 @@ CXXMethodDecl::getCorrespondingMethodInClass(const CXXRecordDecl *RD,
   if (auto *MD = getCorrespondingMethodDeclaredInClass(RD, MayBeBase))
     return MD;
 
+  llvm::SmallVector<CXXMethodDecl*, 4> FinalOverriders;
+  auto AddFinalOverrider = [&](CXXMethodDecl *D) {
+    // If this function is overridden by a candidate final overrider, it is not
+    // a final overrider.
+    for (CXXMethodDecl *OtherD : FinalOverriders) {
+      if (declaresSameEntity(D, OtherD) || recursivelyOverrides(OtherD, D))
+        return;
+    }
+
+    // Other candidate final overriders might be overridden by this function.
+    FinalOverriders.erase(
+        std::remove_if(FinalOverriders.begin(), FinalOverriders.end(),
+                       [&](CXXMethodDecl *OtherD) {
+                         return recursivelyOverrides(D, OtherD);
+                       }),
+        FinalOverriders.end());
+
+    FinalOverriders.push_back(D);
+  };
+
   for (const auto &I : RD->bases()) {
     const RecordType *RT = I.getType()->getAs<RecordType>();
     if (!RT)
       continue;
     const auto *Base = cast<CXXRecordDecl>(RT->getDecl());
-    CXXMethodDecl *T = this->getCorrespondingMethodInClass(Base);
-    if (T)
-      return T;
+    if (CXXMethodDecl *D = this->getCorrespondingMethodInClass(Base))
+      AddFinalOverrider(D);
   }
 
-  return nullptr;
+  return FinalOverriders.size() == 1 ? FinalOverriders.front() : nullptr;
 }
 
 CXXMethodDecl *CXXMethodDecl::Create(ASTContext &C, CXXRecordDecl *RD,
@@ -2105,6 +2124,11 @@ CXXMethodDecl *CXXMethodDecl::getDevirtualizedMethod(const Expr *Base,
   CXXMethodDecl *DevirtualizedMethod =
       getCorrespondingMethodInClass(BestDynamicDecl);
 
+  // If there final overrider in the dynamic type is ambiguous, we can't
+  // devirtualize this call.
+  if (!DevirtualizedMethod)
+    return nullptr;
+
   // If that method is pure virtual, we can't devirtualize. If this code is
   // reached, the result would be UB, not a direct call to the derived class
   // function, and we can't assume the derived class function is defined.

diff  --git a/clang/test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp b/clang/test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp
index 130103de97ae..6f5e844b587e 100644
--- a/clang/test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp
+++ b/clang/test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp
@@ -255,6 +255,49 @@ namespace Test10 {
   }
 }
 
+namespace TestVBase {
+  struct A { virtual void f(); };
+  struct B : virtual A {};
+  struct C : virtual A { void f() override; };
+
+  extern struct BC final : B, C {} &bc;
+  extern struct BCusingA final : B, C { using A::f; } &bc_using_a;
+  extern struct BCusingB final : B, C { using B::f; } &bc_using_b;
+  extern struct BCusingC final : B, C { using C::f; } &bc_using_c;
+
+  extern struct CB final : C, B {} &cb;
+  extern struct CBusingA final : C, B { using A::f; } &cb_using_a;
+  extern struct CBusingB final : C, B { using B::f; } &cb_using_b;
+  extern struct CBusingC final : C, B { using C::f; } &cb_using_c;
+
+  // CHECK-LABEL: @_ZN9TestVBase4testEv(
+  void test() {
+    // FIXME: The 'using A' case can be devirtualized to call A's virtual
+    // adjustment thunk for C::f.
+    // FIXME: The 'using B' case can be devirtualized, but requires us to emit
+    // a derived-to-base or base-to-derived conversion as part of
+    // devirtualization.
+
+    // CHECK: call void @_ZN9TestVBase1C1fEv(
+    bc.f();
+    // CHECK: call void %
+    bc_using_a.f();
+    // CHECK: call void %
+    bc_using_b.f();
+    // CHECK: call void @_ZN9TestVBase1C1fEv(
+    bc_using_c.f();
+
+    // CHECK: call void @_ZN9TestVBase1C1fEv(
+    cb.f();
+    // CHECK: call void %
+    cb_using_a.f();
+    // CHECK: call void %
+    cb_using_b.f();
+    // CHECK: call void @_ZN9TestVBase1C1fEv(
+    cb_using_c.f();
+  }
+}
+
 namespace Test11 {
   // Check that the definitions of Derived's operators are emitted.