[PATCH] D109841: Fix vtbl field addr space

Wed Sep 15 13:43:25 PDT 2021

yaxunl marked 2 inline comments as done.
yaxunl added a comment.

In D109841#3002404 <https://reviews.llvm.org/D109841#3002404>, @arichardson wrote:

> 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`.

will do

================
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(
----------------
arichardson wrote:
> Maybe this is clearer?
will do

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

----------------
arichardson wrote:
> As noted by @rjmccall, changing this line still allows all tests to pass (including our newly added out-of-tree CHERI ones).
will do. I think we can also make changes so that

```
VTableField = Builder.CreateBitCast(
      VTableField, VTablePtrTy->getPointerTo(ThisAddrSpace));
```

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

https://reviews.llvm.org/D109841