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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
