List implicit operator== after implicit destructors in a vtable.

[rsmith]

Summary

We previously listed first declared members, then implicit operator=,
then implicit operator==, then implicit destructors. Per discussion on
https://github.com/itanium-cxx-abi/cxx-abi/issues/88, put the implicit
equality comparison operators at the very end, after all special member
functions.

[rjmccall]

I see two reasonable approaches here: we can rely on the implicit members being added in the right order by Sema, as this patch seems to do, or we can broaden the special treatment of implicit virtual functions in the v-table layout code. The latter is more complex (we'd basically need to collect and sort all the implicit virtual functions), but it reliably isolates the ABI from implementation decisions like the order in which Sema processes implicit members and whether some lookup might have triggered an implicit declaration at a point we didn't expect.

I know that Sema used to have a lot of issues with implicitly declaring special members, but I also know that that was bound up with how we tracked special state for the class, and that code has changed a lot. If you feel that that's an unlikely problem now, and you're comfortable with potentially having to generate implicit declarations of special members in different orders to satisfy different ABIs, I can accept this approach. (It would be good to know what order MSVC will emit these members in, though.)

[rsmith]

In D72897#1825791, @rjmccall wrote:

I know that Sema used to have a lot of issues with implicitly declaring special members, but I also know that that was bound up with how we tracked special state for the class, and that code has changed a lot. If you feel that that's an unlikely problem now, and you're comfortable with potentially having to generate implicit declarations of special members in different orders to satisfy different ABIs, I can accept this approach.

I'm not particularly overjoyed by having an ABI decision determined by the order in which Sema chooses to implicitly declare these members. I think it's unlikely to have stability problems (we explicitly force declaration of virtual implicit members at the end of the class before they can have been lazily declared), but avoiding a reliance on Sema's behavior would be nice. It'd be hard to avoid relying on Sema getting the order of implicit operator== functions right, though, as I don't think we want vtable layout to be responsible for figuring out the correspondence between implicit operator== functions and defaulted operator<=> functions. Please take a look at this alternative approach and see what you think.

[rsmith]

• Explicitly put implicit virtual functions in the right order, rather

[rjmccall]

It wouldn't be a bad idea to track that correspondence in the AST just on the general principle that it's useful information (for diagnostics, tooling, etc.) that's otherwise hard to recreate. But I agree that it's also not problematic to expect Sema to add them in the proper order, and if we don't have an ABI that needs the correspondence directly (e.g. because it wants to emit the operators in pairs), it's fine to rely on it.

Putting the operators at the end makes sense to me. It seems likely that an arbitrary implementation is always more likely to need to process the implicit members first.

I'm much happier with this implementation; LGTM.

[RKSimon]

@rsmith This is failing on http://lab.llvm.org:8011/builders/llvm-clang-win-x-armv7l/builds/3169 - please can you take a look?

[peter.smith]

It is also failing on the other 32-bit arm bots, example http://lab.llvm.org:8011/builders/clang-cmake-armv7-quick/builds/13052/steps/ninja%20check%201/logs/FAIL%3A%20Clang%3A%3Avirtual-compare.cpp
Looking at the failure

// CHECK-SAME: @_ZThn8_N1AD1Ev

and the output on an Arm machine

$_ZThn4_N1AD1Ev = comdat any

$_ZThn4_N1AD0Ev = comdat any

$_ZThn4_N1AaSERKS_ = comdat any

It might be a 32-bit/64-bit specific expectation.

[alanphipps]

This test is still failing on the arm bots and also with my downstream ARM compiler validation.

[vvereschaka]

Hi Richard,

http://lab.llvm.org:8011/builders/llvm-clang-win-x-armv7l builder has been broken by this patch during few days (failed "Clang::virtual-compare.cpp" test).
Sorry, but I'm going to revert these changes.

[rsmith]

In D72897#1832234, @vvereschaka wrote:

Hi Richard,

http://lab.llvm.org:8011/builders/llvm-clang-win-x-armv7l builder has been broken by this patch during few days (failed "Clang::virtual-compare.cpp" test).
Sorry, but I'm going to revert these changes.

Thanks for the revert; fixed and re-landed.

clang/lib/AST/VTableBuilder.cpp

if (const CXXRecordDecl *PrimaryBase = Layout.getPrimaryBase()) {
     AddMethods(BaseSubobject(PrimaryBase, PrimaryBaseOffset),
                PrimaryBaseOffsetInLayoutClass, FirstBaseInPrimaryBaseChain,
                FirstBaseOffsetInLayoutClass, PrimaryBases);
 
     if (!PrimaryBases.insert(PrimaryBase))
       llvm_unreachable("Found a duplicate primary base!");
   }
 
-  const CXXDestructorDecl *ImplicitVirtualDtor = nullptr;
-
   typedef llvm::SmallVector<const CXXMethodDecl *, 8> NewVirtualFunctionsTy;
   NewVirtualFunctionsTy NewVirtualFunctions;
 
+  llvm::SmallVector<const CXXMethodDecl*, 4> NewImplicitVirtualFunctions;
+
   // Now go through all virtual member functions and add them.
   for (const auto *MD : RD->methods()) {
     if (!MD->isVirtual())
       continue;
     MD = MD->getCanonicalDecl();
 
     // Get the final overrider.
     FinalOverriders::OverriderInfo Overrider =
if (const CXXMethodDecl *OverriddenMD =
                      ThunkInfo(ThisAdjustment, ReturnAdjustment));
           }
         }
 
         continue;
       }
     }
 
-    if (const CXXDestructorDecl *DD = dyn_cast<CXXDestructorDecl>(MD)) {
-      if (MD->isImplicit()) {
-        // Itanium C++ ABI 2.5.2:
-        //   If a class has an implicitly-defined virtual destructor,
-        //   its entries come after the declared virtual function pointers.
-
-        assert(!ImplicitVirtualDtor &&
-               "Did already see an implicit virtual dtor!");
-        ImplicitVirtualDtor = DD;
-        continue;
-      }
-    }
-
+    if (MD->isImplicit())
+      NewImplicitVirtualFunctions.push_back(MD);
+    else
       NewVirtualFunctions.push_back(MD);
   }
 
-  if (ImplicitVirtualDtor)
-    NewVirtualFunctions.push_back(ImplicitVirtualDtor);
+  std::stable_sort(
+      NewImplicitVirtualFunctions.begin(), NewImplicitVirtualFunctions.end(),
+      [](const CXXMethodDecl *A, const CXXMethodDecl *B) {
+        if (A->isCopyAssignmentOperator() != B->isCopyAssignmentOperator())
+          return A->isCopyAssignmentOperator();
+        if (A->isMoveAssignmentOperator() != B->isMoveAssignmentOperator())
+          return A->isMoveAssignmentOperator();
+        if (isa<CXXDestructorDecl>(A) != isa<CXXDestructorDecl>(B))
+          return isa<CXXDestructorDecl>(A);
+        assert(A->getOverloadedOperator() == OO_EqualEqual &&
+               B->getOverloadedOperator() == OO_EqualEqual &&
+               "unexpected or duplicate implicit virtual function");
+        // We rely on Sema to have declared the operator== members in the
+        // same order as the corresponding operator<=> members.
+        return false;
+      });
+  NewVirtualFunctions.append(NewImplicitVirtualFunctions.begin(),
+                             NewImplicitVirtualFunctions.end());
 
   for (const CXXMethodDecl *MD : NewVirtualFunctions) {
     // Get the final overrider.
     FinalOverriders::OverriderInfo Overrider =
       Overriders.getOverrider(MD, Base.getBaseOffset());
 
     // Insert the method info for this method.
     MethodInfo MethodInfo(Base.getBaseOffset(), BaseOffsetInLayoutClass,

clang/test/CodeGenCXX/virtual-compare.cpp

+// RUN: %clang_cc1 -std=c++2a -triple %itanium_abi_triple -emit-llvm %s -o - | FileCheck %s
+
+#include "Inputs/std-compare.h"
+
+// CHECK: @_ZTV1A =
+struct A;
+struct X {
+  // CHECK-SAME: @_ZN1X1xEv
+  virtual void x();
+  friend auto operator<=>(X, X) = default;
+};
+struct Y {
+  virtual ~Y();
+  virtual A &operator=(const A &);
+  friend auto operator<=>(Y, Y) = default;
+};
+struct A : X, Y {
+  // CHECK-SAME: @_ZN1A1fEv
+  virtual void f();
+  // CHECK-SAME: @_ZNKR1AssERKS_
+  virtual std::strong_ordering operator<=>(const A &) const & = default;
+  // CHECK-SAME: @_ZN1A1gEv
+  virtual void g();
+  // CHECK-SAME: @_ZNKO1AssERKS_
+  virtual std::strong_ordering operator<=>(const A &) const && = default;
+  // CHECK-SAME: @_ZN1A1hEv
+  virtual void h();
+
+  // CHECK-SAME: @_ZN1AaSERKS_
+  // implicit virtual A &operator=(const A&) = default;
+
+  // CHECK-SAME: @_ZN1AD1Ev
+  // CHECK-SAME: @_ZN1AD0Ev
+  // implicit virtual ~A();
+
+  // CHECK-SAME: @_ZNKR1AeqERKS_
+  // implicit virtual A &operator==(const A&) const & = default;
+
+  // CHECK-SAME: @_ZNKO1AeqERKS_
+  // implicit virtual A &operator==(const A&) const && = default;
+};
+
+// For Y:
+// CHECK-SAME: @_ZTI1A
+
+// CHECK-SAME: @_ZThn8_N1AD1Ev
+// CHECK-SAME: @_ZThn8_N1AD0Ev
+// virtual ~Y();
+
+// CHECK-SAME: @_ZThn8_N1AaSERKS_
+// virtual A &operator=(const A &);
+
+void A::f() {}