[PATCH] D72959: Relative VTables ABI on Fuchsia

[John McCall via Phabricator via llvm-commits]

rjmccall added inline comments.


================
Comment at: clang/include/clang/Basic/LangOptions.def:357
+        "allow use of clang's relative C++ ABI "
+        "for classes with vtables on targets that support this")
 
----------------
"Use an ABI-incompatible v-table layout that uses relative references"


================
Comment at: clang/include/clang/CodeGen/ConstantInitBuilder.h:230
+  /// Same as addRelativeOffset(), but instead relative to a position in the
+  /// aggregate being constructed.
+  void addRelativeOffsetToPosition(llvm::IntegerType *type,
----------------
"relative to an element in this aggregate, identified by its index".


================
Comment at: clang/include/clang/CodeGen/ConstantInitBuilder.h:317
+  /// The returned pointer will have type T*, where T is the given
+  /// position.
+  llvm::Constant *getAddrOfPosition(llvm::Type *type, size_t position);
----------------
Hmm, you've borrowed the last sentence from `getAddrOfCurrentPosition`, but it actually doesn't make any sense (in either place).  I think it would be better to say something like "The returned pointer will have type `T*`, where `T` is the given type.  This type can differ from the type of the actual element."


================
Comment at: clang/lib/CodeGen/CGVTables.cpp:638
+  // Examples where there would bo no symbol emitted are available_externally
+  // and private linkages.
+  auto StubLinkage = VTableHasLocalLinkage ? llvm::GlobalValue::InternalLinkage
----------------
Lots of typos


================
Comment at: clang/lib/CodeGen/CGVTables.cpp:646
+
+    // Take offset from stub to the vtable component.
+    llvm::Function *HiddenFunc = M.getFunction(StubName);
----------------
That's not what this block is doing.

I think it would make sense to build this stub in a helper function.

Can you just avoid making the stub entirely if the function is known to be defined in this translation unit (which will include all the internal-linkage cases, but also things like `hidden linkonce_odr`)?


================
Comment at: clang/lib/CodeGen/CGVTables.cpp:649
+    if (!HiddenFunc) {
+      // Make the stub private.
+      llvm::Function *Stub = llvm::Function::Create(Func->getFunctionType(),
----------------
`private` is a specific linkage that you're not actually using.


================
Comment at: clang/lib/CodeGen/CGVTables.cpp:653
+
+      // Propogate byval attributes.
+      Stub->setAttributes(Func->getAttributes());
----------------
You're propagating *all* the attributes (which is good, but the comment should be clearer).


================
Comment at: clang/lib/CodeGen/CGVTables.cpp:722
+    unsigned componentIdx, llvm::Constant *rtti, unsigned &nextVTableThunkIndex,
+    unsigned lastAddrPoint, bool VTableHasLocalLinkage) {
+  auto &component = layout.vtable_components()[componentIdx];
----------------
`lastAddrPoint` is a confusing name for this.


================
Comment at: clang/lib/CodeGen/CGVTables.cpp:728
+  auto addOffsetConstant =
+      UseRelativeLayout ? AddRelativeOffsetConstant : AddOffsetConstant;
 
----------------
Relying on a capitalization difference for this is not a good idea for readability.


================
Comment at: clang/lib/CodeGen/CGVTables.cpp:789
+      // symbol's module. A result of this ABI would make __cxa_pure_virtual
+      // (and it's destructor equivalent) local symbols, and depending on
+      // link order, the comdat groups would resolve to the one with the
----------------
It's not a "destructor" equivalent, it's when the virtual function is defined as deleted.


================
Comment at: clang/lib/CodeGen/CGVTables.cpp:795
+      if (UseRelativeLayout)
+        return llvm::Constant::getNullValue(CGM.Int8PtrTy);
+
----------------
Prefer `llvm::ConstantPointerNull::get`.


================
Comment at: clang/lib/CodeGen/CGVTables.cpp:868
+  else
+    ComponentType = CGM.Int8PtrTy;
   for (unsigned i = 0, e = layout.getNumVTables(); i != e; ++i) {
----------------
Can we make a helper function for this?


================
Comment at: clang/lib/CodeGen/CGVTables.cpp:895
+    size_t thisIndex = layout.getVTableOffset(VTableIdx);
+    size_t nextIndex = thisIndex + layout.getVTableSize(VTableIdx);
+    unsigned LastAddressPoint = thisIndex;
----------------
Please use the same local-variable capitalization conventions as the existing code, and please don't recompute `getNumVTables()` each time through the loop.

I agree that `vtableIndex` is a clearer name than `i`, but if you're going to rename locals for readability, please also rename `thisIndex` and `nextIndex`.


================
Comment at: clang/lib/CodeGen/CGVTables.cpp:899
+         ++ComponentIdx) {
+      // We want to point to the first function in the vtable. This will come
+      // after the typeinfo component, so we can just check if the previous
----------------
"Relative offsets should be relative to the v-table address point, which is the first entry after the RTTI pointer."

Can we just do this scan ahead of time to find the component index of the address point when we're using the relative ABI?  It typically won't be a large scan, and it'll be a lot more obviously correct than what you're doing here.  Also, I think what you're doing here is broken for the RTTI pointer.

You could also add a method on `VTableLayout` to get the address point by table index.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72959/new/

https://reviews.llvm.org/D72959