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