[clang] Frontend components for the relative vtables ABI

[leonardchan]

Summary

This patch contains all of the clang changes from D72959.

• Generalized the relative vtables ABI such that it can be used by other targets.
• Adds an enum VTableComponentLayout which controls whether components in the vtable should be pointers to other structs or relative offsets to those structs. Other ABIs can change this enum to restructure how components in the vtable are laid out/accessed.
• Adds methods to ConstantInitBuilder for inserting relative offsets to a specified position in the aggregate being constructed.

See D72959 for background info.

[rjmccall]

This is a weird point to allow further ABI customization of. I understand why you want to customize this, but I wonder if it's actually worthwhile to make a virtual function for it vs. just checking some sort of flag in the builder. Isn't there quite a lot of structure you're going to have to duplicate just to call addRelativeAddress at the leaves?

[leonardchan]

In D77592#1975629, @rjmccall wrote:

This is a weird point to allow further ABI customization of. I understand why you want to customize this, but I wonder if it's actually worthwhile to make a virtual function for it vs. just checking some sort of flag in the builder. Isn't there quite a lot of structure you're going to have to duplicate just to call addRelativeAddress at the leaves?

Based off the work I've done so far, I wasn't necessarily duplicating existing code, but more moving it into virtual functions so I can override them with new logic. I think I'd end up with the same result regardless of if I use virtual functions or check a flag somewhere since, in the end, I'll need a switch somewhere that says I should use this new vtable logic in specific areas of the code. Where that switch would be (through virtual functions/a compiler flag/a cmake flag), I don't have strong opinions.

The main thing that I'm unsure of though is if it's ok to have all these customisations. Based off what I have so far, I'd like to add a few more virtual functions (~8 that FuchsiaCXXABI overrides), so it would be checking one switch in many different areas.

[rjmccall]

This is a global switch, right, not something that's mix-and-match between hierarchies or even between classes? I think I would prefer that the ABI object just tell us the expected layout of a v-table entry (as an enum) rather than forcing a bunch of different callbacks. Seems more composable, among other things.

[leonardchan]

In D77592#1979030, @rjmccall wrote:

This is a global switch, right, not something that's mix-and-match between hierarchies or even between classes? I think I would prefer that the ABI object just tell us the expected layout of a v-table entry (as an enum) rather than forcing a bunch of different callbacks. Seems more composable, among other things.

I see. Yeah, this isn't meant to be mix-and-matched. My initial reasoning for making it virtual was so that it could be specific to Fuchsia for now, then if other platforms wanted to use it, we could abstract it out then other ABIs can just inherit from it. But it seems that the same effect can be done without a set of callbacks.

Will update such that the ABI selects the vtable entry layout.

[rjmccall]

Thanks. Having an enum like this would also be important if we wanted to implement the Itanium rule (as in, the actual Itanium architecture, not the generic C++ ABI) that v-tables contain "inline" function descriptors rather than pointers to them. (On Itanium, a C function pointer is actually a pointer to a global descriptor object that's a pair of a function address and some other metadata necessary for the call.) So it's a generalizable concept.

[leonardchan]

I'm not sure how to test it yet without adding all the mechanisms for actually laying out the components, but at the very least this adds the switch for it.

If you don't mind the patch getting bigger, I could include the cc1 option in this patch for selecting the enum and some parts of codegen so we can test this.

[rjmccall]

I'm not sure if the AST-level v-table layout abstraction really cares about these differences. I don't think it vends byte offsets into the v-table, just slot indices (i.e. word offsets).

clang/include/clang/AST/VTableBuilder.h

380 ↗

(On Diff #257786)

The only "struct" pointed to by the v-table is the type_info object. How are you planning to handle that? The standard ABI makes the type_info a vague-linkage symbol in most cases, so you won't be able to have a direct relative reference to it. If you adopt the Apple ARM64 modification for type_info equality, you can rely on the type_info being defined within the same linkage unit unless the v-table is being emitted as an optimization for a type defined with a key function in a different linkage unit. You could handle that by making this an "inidirectable" relative reference, where it's either a direct relative reference or a relative reference to a GOT entry, as specified by the low bit in the reference. But you need *some* solution for this.

[leonardchan]

In D77592#1985740, @rjmccall wrote:

I'm not sure if the AST-level v-table layout abstraction really cares about these differences. I don't think it vends byte offsets into the v-table, just slot indices (i.e. word offsets).

The primary reason for why I added it in AST is because down the line, to support the changes to the RTTI component, I'll need to edit VCallAndVBaseOffsetBuilder::getCurrentOffsetOffset():

CharUnits VCallAndVBaseOffsetBuilder::getCurrentOffsetOffset() const {
  // OffsetIndex is the index of this vcall or vbase offset, relative to the
  // vtable address point. (We subtract 3 to account for the information just
  // above the address point, the RTTI info, the offset to top, and the
  // vcall offset itself).
  int64_t OffsetIndex = -(int64_t)(3 + Components.size());

  CharUnits PointerWidth =
    Context.toCharUnitsFromBits(Context.getTargetInfo().getPointerWidth(0));
  CharUnits OffsetOffset = PointerWidth * OffsetIndex;
  return OffsetOffset;
}

Right now, it assumes that the offset is 3 pointer widths below the vtable address point, but since we want to make the RTTI component a 32-bit int, we'll need to add an extra 4 CharUnits to the result. The enum would need to be exposed on the AST level so we can check it here. If we move the enum into ItaniumCXXABI or somewhere under codegen, then we will instead need to check every instance vcall or vbase offsets are used in codegen. I tried playing around with this idea, but I gave up since there seemed to be too many instances I didn't catch since my tests kept failing. I figured this was the easiest way.

This is just my experience trying to move things around, but it could be there's a better place to put this that I'm not seeing.

clang/include/clang/AST/VTableBuilder.h

380 ↗

(On Diff #257786)

The plan I have is to add the "indirectable" relative reference you mention. In IR, we emit a dso_local global that points to the potentially external type_info object, and the relative reference is taken on this dso_local symbol. This IR seems to get lowered to a relative reference to a GOT entry.

[rjmccall]

In D77592#1987677, @leonardchan wrote:

In D77592#1985740, @rjmccall wrote:

I'm not sure if the AST-level v-table layout abstraction really cares about these differences. I don't think it vends byte offsets into the v-table, just slot indices (i.e. word offsets).

The primary reason for why I added it in AST is because down the line, to support the changes to the RTTI component, I'll need to edit VCallAndVBaseOffsetBuilder::getCurrentOffsetOffset():

CharUnits VCallAndVBaseOffsetBuilder::getCurrentOffsetOffset() const {
  // OffsetIndex is the index of this vcall or vbase offset, relative to the
  // vtable address point. (We subtract 3 to account for the information just
  // above the address point, the RTTI info, the offset to top, and the
  // vcall offset itself).
  int64_t OffsetIndex = -(int64_t)(3 + Components.size());

  CharUnits PointerWidth =
    Context.toCharUnitsFromBits(Context.getTargetInfo().getPointerWidth(0));
  CharUnits OffsetOffset = PointerWidth * OffsetIndex;
  return OffsetOffset;
}

Right now, it assumes that the offset is 3 pointer widths below the vtable address point, but since we want to make the RTTI component a 32-bit int, we'll need to add an extra 4 CharUnits to the result.

Okay, sure, if there are already offsets in bytes already in the AST-level layout, I agree that we should be able to compute all of the byte offsets at that level.

How are you planning to handle alignments here? Currently alignment doesn't affect the layout because all the entries are uniformly pointer-sized. If you're planning to make the offset-to-top and vcall offsets 64-bit on 64-bit architectures, you're going to either have interior padding or those offsets might not be naturally-aligned.

Personally, I would just use 32-bit offsets unless you really think you need to support types with base adjustments more than ±2GB. And that has the nice impact of making all the offsets within the v-table uniformly scaled again, just by 4 bytes instead of the pointer size.

Different topic: I'm still not sure why the places you've added TODOs here are actually places you'd need to modify.

[leonardchan]

Okay, sure, if there are already offsets in bytes already in the AST-level layout, I agree that we should be able to compute all of the byte offsets at that level.

How are you planning to handle alignments here? Currently alignment doesn't affect the layout because all the entries are uniformly pointer-sized. If you're planning to make the offset-to-top and vcall offsets 64-bit on 64-bit architectures, you're going to either have interior padding or those offsets might not be naturally-aligned.

Personally, I would just use 32-bit offsets unless you really think you need to support types with base adjustments more than ±2GB. And that has the nice impact of making all the offsets within the v-table uniformly scaled again, just by 4 bytes instead of the pointer size.

My initial idea was to insert padding, but it seems more straightforward and cleaner to just make the entire struct use 32 bit offsets as you said. There's probably a bigger issue if we're actually using types that require more than a 2GB adjustment, and we could perhaps add a check downt he line that prints an error if this somehow happens. I'll update D72959 then this one.

Different topic: I'm still not sure why the places you've added TODOs here are actually places you'd need to modify.

My initial proposal had the idea of calling a separate method CodeGenVTables::createRelativeVTableInitializer() which was structured a lot differently from CodeGenVTables::createVTableInitializer(), but if I'm instead moving away from virtual methods to checking against this enum, and using an array of i32s instead of a struct with mixed i32s and i8*s, then I may not need this new method in the first place and can reuse a lot of the existing mechanisms in CodeGenVTables::createVTableInitializer().

This particular TODO though will be necessary for selecting the Relative enum:

// TODO: Specify that we want to use the Relative VTableComponentLayout
// here once we add the option for selecting it for Fuchsia.
VTContext.reset(new ItaniumVTableContext(*this));

The idea I had here is to instead construct new ItaniumVTableContext(*this, /*ComponentLayout=*/Relative)) once I add the flag for turning this on across Fuchsia.

[leonardchan]

Updated to reflect the changes in D72959 which make the vtable 4-byte aligned under the Relative layout.

[rjmccall]

Okay. The plan sounds good to me. So you want to roll out this enum in this patch, but not really use it for much until the follow-up?

[leonardchan]

In D77592#2002148, @rjmccall wrote:

Okay. The plan sounds good to me. So you want to roll out this enum in this patch, but not really use it for much until the follow-up?

That was the plan, but I can also add include the cc1 flag for selecting the Relative layout and some initial vtable changes in this patch so it can be tested. The first patch after this that would be using it will be large regardless.

[leonardchan]

I decided to just include all the frontend changes here since most of them are tightly coupled. If this patch should be split up more, I don't mind splitting it again.

[leonardchan]

@rjmccall Would it be fine to submit this since you gave LGTM on D72959?

[rjmccall]

Yes, alright.

[erik.pilkington]

clang/lib/CodeGen/ConstantInitBuilder.cpp

132 ↗

(On Diff #269954)

Shouldn't this be Builder.Buffer.size() - Begin instead of Builder.SelfReferences.size()? If a record had a mix of relative pointers and other fields they wouldn't necessarily be the same.

clang/lib/CodeGen/CGCXXABI.h

EmitMemberPointerComparison(CodeGenFunction &CGF,
                               bool Inequality);
 
   /// Determine if a member pointer is non-null.  Returns an i1.
   virtual llvm::Value *
   EmitMemberPointerIsNotNull(CodeGenFunction &CGF,
                              llvm::Value *MemPtr,
                              const MemberPointerType *MPT);
 
+  /// Create and set the initializer for the vtable.
+  virtual void SetVTableInitializer(CodeGenVTables &CGVT,
+                                    llvm::GlobalVariable *VTable,
+                                    const VTableLayout &VTLayout,
+                                    llvm::Constant *RTTI);
+
 protected:
   /// A utility method for computing the offset required for the given
   /// base-to-derived or derived-to-base member-pointer conversion.
   /// Does not handle virtual conversions (in case we ever fully
   /// support an ABI that allows this).  Returns null if no adjustment
   /// is required.
   llvm::Constant *getMemberPointerAdjustment(const CastExpr *E);
 

clang/lib/CodeGen/CGCXXABI.cpp

 //===----- CGCXXABI.cpp - Interface to C++ ABIs ---------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This provides an abstract class for C++ code generation. Concrete subclasses
 // of this implement code generation for specific C++ ABIs.
 //
 //===----------------------------------------------------------------------===//
 
 #include "CGCXXABI.h"
 #include "CGCleanup.h"
 #include "clang/AST/Attr.h"
+#include "clang/CodeGen/ConstantInitBuilder.h"
 
 using namespace clang;
 using namespace CodeGen;
 
 CGCXXABI::~CGCXXABI() { }
 
 void CGCXXABI::ErrorUnsupportedABI(CodeGenFunction &CGF, StringRef S) {
   DiagnosticsEngine &Diags = CGF.CGM.getDiags();

 llvm::Value *
 CGCXXABI::EmitMemberPointerIsNotNull(CodeGenFunction &CGF,
                                      llvm::Value *MemPtr,
                                      const MemberPointerType *MPT) {
   ErrorUnsupportedABI(CGF, "member function pointer null testing");
   return CGF.Builder.getFalse();
 }
 
+void CGCXXABI::SetVTableInitializer(CodeGenVTables &CGVT,
+                                    llvm::GlobalVariable *VTable,
+                                    const VTableLayout &VTLayout,
+                                    llvm::Constant *RTTI) {
+  ConstantInitBuilder builder(CGM);
+  auto components = builder.beginStruct();
+  CGVT.createVTableInitializer(components, VTLayout, RTTI);
+  components.finishAndSetAsInitializer(VTable);
+}
+
 llvm::Constant *
 CGCXXABI::EmitNullMemberPointer(const MemberPointerType *MPT) {
   return GetBogusMemberPointer(QualType(MPT, 0));
 }
 
 llvm::Constant *CGCXXABI::EmitMemberFunctionPointer(const CXXMethodDecl *MD) {
   return GetBogusMemberPointer(CGM.getContext().getMemberPointerType(
       MD->getType(), MD->getParent()->getTypeForDecl()));

clang/lib/CodeGen/CGVTables.cpp

CodeGenVTables::GenerateConstructionVTable(const CXXRecordDecl *RD,
 
   // V-tables are always unnamed_addr.
   VTable->setUnnamedAddr(llvm::GlobalValue::UnnamedAddr::Global);
 
   llvm::Constant *RTTI = CGM.GetAddrOfRTTIDescriptor(
       CGM.getContext().getTagDeclType(Base.getBase()));
 
   // Create and set the initializer.
-  ConstantInitBuilder builder(CGM);
-  auto components = builder.beginStruct();
-  createVTableInitializer(components, *VTLayout, RTTI);
-  components.finishAndSetAsInitializer(VTable);
+  CGM.getCXXABI().SetVTableInitializer(*this, VTable, *VTLayout, RTTI);
 
   // Set properties only after the initializer has been set to ensure that the
   // GV is treated as definition and not declaration.
   assert(!VTable->isDeclaration() && "Shouldn't set properties on declaration");
   CGM.setGVProperties(VTable, RD);
 
   CGM.EmitVTableTypeMetadata(RD, VTable, *VTLayout.get());
 

clang/lib/CodeGen/ItaniumCXXABI.cpp

public:
   bool shouldRTTIBeUnique() const override { return false; }
 };
 
 class FuchsiaCXXABI final : public ItaniumCXXABI {
 public:
   explicit FuchsiaCXXABI(CodeGen::CodeGenModule &CGM)
       : ItaniumCXXABI(CGM) {}
 
+  void SetVTableInitializer(CodeGenVTables &CGVT, llvm::GlobalVariable *VTable,
+                            const VTableLayout &VTLayout,
+                            llvm::Constant *RTTI) override {
+    ConstantInitBuilder builder(CGM);
+    auto components = builder.beginStruct();
+
+    // TODO: Replace this with CodeGenVTables::createRelativeVTableInitializer()
+    // once it is implemented. This will be the helper for the relative-vtables
+    // ABI.
+    CGVT.createVTableInitializer(components, VTLayout, RTTI);
+
+    components.finishAndSetAsInitializer(VTable);
+  }
+
 private:
   bool HasThisReturn(GlobalDecl GD) const override {
     return isa<CXXConstructorDecl>(GD.getDecl()) ||
            (isa<CXXDestructorDecl>(GD.getDecl()) &&
             GD.getDtorType() != Dtor_Deleting);
   }
 };
 
void ItaniumCXXABI::emitVTableDefinitions(CodeGenVTables &CGVT,
 
   ItaniumVTableContext &VTContext = CGM.getItaniumVTableContext();
   const VTableLayout &VTLayout = VTContext.getVTableLayout(RD);
   llvm::GlobalVariable::LinkageTypes Linkage = CGM.getVTableLinkage(RD);
   llvm::Constant *RTTI =
       CGM.GetAddrOfRTTIDescriptor(CGM.getContext().getTagDeclType(RD));
 
   // Create and set the initializer.
-  ConstantInitBuilder Builder(CGM);
-  auto Components = Builder.beginStruct();
-  CGVT.createVTableInitializer(Components, VTLayout, RTTI);
-  Components.finishAndSetAsInitializer(VTable);
+  SetVTableInitializer(CGVT, VTable, VTLayout, RTTI);
 
   // Set the correct linkage.
   VTable->setLinkage(Linkage);
 
   if (CGM.supportsCOMDAT() && VTable->isWeakForLinker())
     VTable->setComdat(CGM.getModule().getOrInsertComdat(VTable->getName()));
 
   // Set the right visibility.

clang/lib/CodeGen/MicrosoftCXXABI.cpp

for (const std::unique_ptr<VPtrInfo>& Info : VFPtrs) {
     const VTableLayout &VTLayout =
       VFTContext.getVFTableLayout(RD, Info->FullOffsetInMDC);
 
     llvm::Constant *RTTI = nullptr;
     if (any_of(VTLayout.vtable_components(),
                [](const VTableComponent &VTC) { return VTC.isRTTIKind(); }))
       RTTI = getMSCompleteObjectLocator(RD, *Info);
 
-    ConstantInitBuilder Builder(CGM);
-    auto Components = Builder.beginStruct();
-    CGVT.createVTableInitializer(Components, VTLayout, RTTI);
-    Components.finishAndSetAsInitializer(VTable);
+    SetVTableInitializer(CGVT, VTable, VTLayout, RTTI);
 
     emitVTableTypeMetadata(*Info, RD, VTable);
   }
 }
 
 bool MicrosoftCXXABI::isVirtualOffsetNeededForVTableField(
     CodeGenFunction &CGF, CodeGenFunction::VPtr Vptr) {
   return Vptr.NearestVBase != nullptr;