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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to