[libcxx-commits] [PATCH] D72959: Relative VTables ABI on Fuchsia

[Leonard Chan via Phabricator via libcxx-commits]

leonardchan marked an inline comment as done.
leonardchan added inline comments.


================
Comment at: clang/lib/CodeGen/CGVTables.cpp:623
+    llvm::Constant *C, llvm::GlobalVariable *VTable, unsigned vtableIdx,
+    unsigned lastAddrPoint) const {
+  // No need to get the offset of a nullptr.
----------------
leonardchan wrote:
> rjmccall wrote:
> > There's already an `addRelativeOffset` on `ConstantArrayBuilder`; is that insufficient for some reason?  I think that, if v-table building were refactored so that the places that build components also add them to the v-table, we'd end up with a lot more flexibility for the ABIs.  We needed a similar sort of change for pointer authentication, which we haven't upstreamed to LLVM yet, but which you can see here:
> > 
> > https://github.com/apple/llvm-project/blob/apple/master/clang/lib/CodeGen/CGVTables.cpp
> > 
> > 
> I actually did not know about this method, but it does seem to boil down to the same arithmetic used here. Will update to see if I can use the existing builders instead.
Ah, so it seems `addRelativeOffset` operates differently than I thought. Initially I thought it just took the offset relative to the start of the array being built, but it actually seems to be relative to the component that would be added to the builder. This is slightly different from the current implementation where the offset is instead taken relative to the address point of the current vtable.

We could technically still achieve the desired effect of offsets in the vtable if we were to use `addTaggedRelativeOffset` and subtract a constantaly increasing offset as more virtual function components are added, although I'm not sure how much more benefit that would offer.

An alternative approach could also be to just use `addRelativeOffset` for offsets relative to the component slot and update instances we access the vtable to adjust for this new arithmetic. I think this would just transform instances of `llvm.load.relative(%vtable, %func_offset)` to `llvm.load.relative(%vtable + %func_offset , 0)`, which seems differerent from how it was intended to be used, but technically might work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72959