[PATCH] D109841: Fix vtbl field addr space

Wed Sep 15 12:57:46 PDT 2021

arichardson accepted this revision.
arichardson added a comment.
This revision is now accepted and ready to land.

Thanks, I can confirm that this fixes the assertions we were seeing after merging D103835 <https://reviews.llvm.org/D103835>. A few minor suggestions below:

There's a typo in the commit message, I'd maybe change it to:
`Storing the vtable field of an object should use the same address space as the this pointer.`
And maybe change `This caused issue for some out of tree project.` to something like `This assumption (added in 054cc3b1b469de4b0cb25d1dc3af43c679c5dc44) caused issues for the out-of-tree CHERI targets`.

================
Comment at: clang/lib/CodeGen/CGClass.cpp:2522-2523
+  // vtable field is is derived from `this` pointer, therefore they should be in
+  // the same addr space. Note it may not be the default address space of LLVM
+  // IR.
   VTableField = Builder.CreatePointerBitCastOrAddrSpaceCast(
----------------
Maybe this is clearer?

================
Comment at: clang/lib/CodeGen/CGClass.cpp:2526-2527
+      VTableField, VTablePtrTy->getPointerTo(ThisAddrSpace));
   VTableAddressPoint = Builder.CreatePointerBitCastOrAddrSpaceCast(
       VTableAddressPoint, VTablePtrTy);

----------------
As noted by @rjmccall, changing this line still allows all tests to pass (including our newly added out-of-tree CHERI ones).

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

https://reviews.llvm.org/D109841