[MS ABI] Detect and diagnose vftables which cannot be created

The MSVC ABI has a bug introduced by appending to the end of vftables
which come from virtual bases: covariant thunks introduces via
non-overlapping regions of the inheritance lattice both append to the
same slot in the vftable.

It is possible to generate correct vftables in cases where one node in
the lattice completely dominates the other on the way to the base with
the vfptr; in all other cases, we must raise a diagnostic in order to
prevent the illusion that we succeeded in laying out the vftable.

This fixes PR16759.

llvm-svn: 236354

diff --git a/clang/include/clang/Basic/DiagnosticASTKinds.td b/clang/include/clang/Basic/DiagnosticASTKinds.td
index d353b45..0b37030 100644 (file)
--- a/clang/include/clang/Basic/DiagnosticASTKinds.td
+++ b/clang/include/clang/Basic/DiagnosticASTKinds.td
@@ -168,6 +168,14 @@ let CategoryName = "Inline Assembly Issue" in {
     "invalid operand number in inline asm string">;
 }
 
+// vtable related.
+let CategoryName = "VTable ABI Issue" in {
+  def err_vftable_ambiguous_component : Error<
+    "ambiguous vftable component for %0 introduced via covariant thunks; "
+    "this is an inherent limitation of the ABI">;
+  def note_covariant_thunk : Note<
+    "covariant thunk required by %0">;
+}
 
 // Importing ASTs
 def err_odr_variable_type_inconsistent : Error<

diff --git a/clang/include/clang/Basic/DiagnosticIDs.h b/clang/include/clang/Basic/DiagnosticIDs.h
index 99b469d..a675dfa 100644 (file)
--- a/clang/include/clang/Basic/DiagnosticIDs.h
+++ b/clang/include/clang/Basic/DiagnosticIDs.h
@@ -34,7 +34,7 @@ namespace clang {
       DIAG_START_LEX           = DIAG_START_SERIALIZATION   +  120,
       DIAG_START_PARSE         = DIAG_START_LEX             +  300,
       DIAG_START_AST           = DIAG_START_PARSE           +  500,
-      DIAG_START_COMMENT       = DIAG_START_AST             +  100,
+      DIAG_START_COMMENT       = DIAG_START_AST             +  110,
       DIAG_START_SEMA          = DIAG_START_COMMENT         +  100,
       DIAG_START_ANALYSIS      = DIAG_START_SEMA            + 3000,
       DIAG_UPPER_LIMIT         = DIAG_START_ANALYSIS        +  100

diff --git a/clang/lib/AST/VTableBuilder.cpp b/clang/lib/AST/VTableBuilder.cpp
index de0c11a..5c7b9d2 100644 (file)
--- a/clang/lib/AST/VTableBuilder.cpp
+++ b/clang/lib/AST/VTableBuilder.cpp
@@ -13,6 +13,7 @@
 
 #include "clang/AST/VTableBuilder.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/ASTDiagnostic.h"
 #include "clang/AST/CXXInheritance.h"
 #include "clang/AST/RecordLayout.h"
 #include "clang/Basic/TargetInfo.h"
@@ -3482,9 +3483,24 @@ static bool findPathForVPtr(ASTContext &Context,
 
   CXXBasePaths Paths(/*FindAmbiguities=*/false, /*RecordPaths=*/false,
                      /*DetectVirtual=*/true);
+#if 0
+  std::vector<const CXXRecordDecl *> BaseLists;
+  for (const auto &B : RD->bases()) {
+    const CXXRecordDecl *Base = B.getType()->getAsCXXRecordDecl();
+    Paths.clear();
+    if (!Base->isDerivedFrom(Info->BaseWithVPtr, Paths))
+      continue;
+    for (const auto &Chain : BaseLists) {
+      const CXXRecordDecl *MostDerived = Chain.front();
+    }
+  }
+#endif
   // All virtual bases which are on the path to the BaseWithVPtr are not equal.
   // Specifically, virtual paths which introduce additional covariant thunks
   // must be preferred over paths which do not introduce such thunks.
+  const CXXRecordDecl *Base = nullptr;
+  CharUnits NewOffset;
+  const CXXMethodDecl *CovariantMD = nullptr;
   for (const auto *MD : Info->BaseWithVPtr->methods()) {
     if (!MD->isVirtual())
       continue;
@@ -3501,10 +3517,10 @@ static bool findPathForVPtr(ASTContext &Context,
 
     // Ok, let's iterate through our virtual bases looking for a base which
     // provides a return adjusting overrider for this method.
-    const CXXRecordDecl *Base = nullptr;
-    CharUnits NewOffset;
     for (const auto &B : RD->bases()) {
       const CXXRecordDecl *VBase = B.getType()->getAsCXXRecordDecl();
+      if (Base == VBase)
+        continue;
       // There might be a vbase which derives from a vbase which provides a
       // covariant override for the method *and* provides its own covariant
       // override.
@@ -3512,7 +3528,7 @@ static bool findPathForVPtr(ASTContext &Context,
       // looking for the most derived virtual base which provides a covariant
       // override for the method.
       Paths.clear();
-      if (!VBase->isDerivedFrom(Base ? Base : Info->BaseWithVPtr, Paths) ||
+      if (!VBase->isDerivedFrom(Info->BaseWithVPtr, Paths) ||
           !Paths.getDetectedVirtual())
         continue;
       const CXXMethodDecl *VBaseMD = MD->getCorrespondingMethodInClass(VBase);
@@ -3525,17 +3541,33 @@ static bool findPathForVPtr(ASTContext &Context,
       // Skip any overriders which are not return adjusting.
       if (BO.isEmpty() || !BO.VirtualBase)
         continue;
-      Base = VBase;
-      NewOffset = VBaseNewOffset;
+      Paths.clear();
+      if (!Base || VBase->isDerivedFrom(Base, Paths)) {
+        assert(!Base || Paths.getDetectedVirtual());
+        Base = VBase;
+        NewOffset = VBaseNewOffset;
+        CovariantMD = VBaseMD;
+      } else {
+        Paths.clear();
+        if (!Base->isDerivedFrom(VBase, Paths)) {
+          DiagnosticsEngine &Diags = Context.getDiagnostics();
+          Diags.Report(RD->getLocation(), diag::err_vftable_ambiguous_component)
+              << RD;
+          Diags.Report(CovariantMD->getLocation(), diag::note_covariant_thunk)
+              << CovariantMD;
+          Diags.Report(VBaseMD->getLocation(), diag::note_covariant_thunk)
+              << VBaseMD;
+        }
+      }
     }
-
-    if (Base && Recurse(Base, NewOffset))
-      return true;
   }
 
+  if (Base && Recurse(Base, NewOffset))
+    return true;
+
   for (const auto &B : RD->bases()) {
-    const CXXRecordDecl *Base = B.getType()->getAsCXXRecordDecl();
-    CharUnits NewOffset = GetBaseOffset(B);
+    Base = B.getType()->getAsCXXRecordDecl();
+    NewOffset = GetBaseOffset(B);
     if (Recurse(Base, NewOffset))
       return true;
   }