rjmccall added a comment. In D131979#3756067 <https://reviews.llvm.org/D131979#3756067>, @yihanaa wrote:
> In D131979#3756006 <https://reviews.llvm.org/D131979#3756006>, @rjmccall > wrote: > >> In D131979#3753358 <https://reviews.llvm.org/D131979#3753358>, @yihanaa >> wrote: >> >>> In D131979#3752208 <https://reviews.llvm.org/D131979#3752208>, @rjmccall >>> wrote: >>> >>>> From the test case, it looks like the builtin just ignores pointers to >>>> volatile types, which should be preserved by the conversions you're now >>>> doing in Sema. That is, you should be able to just check >>>> `Ptr->getType()->castAs<PointerType>()->getPointeeType().isVolatile()`. >>>> >>>> It would be different if it ignored pointers loaded out of volatile >>>> l-values or something, but that's explicitly not what it's doing. >>> >>> Thanks for your tips John, form the test case global constants and >>> compiler-rt ubsan class TypeDescriptor, it looks like pass an type >>> description to __ubsan_handle_alignment_assumption, so, we have to use >>> getSubExprAsWritten to get the origin(user written arg type) type >>> description of 1st arg, but not 'const void *', for example, in >>> catch-alignment-assumption-builtin_assume_aligned-three-params.cpp, the 1st >>> arg type description is 'char **', but if we don't use getSubExprAsWritten, >>> all the type description will be 'const void *' >> >> The version of this patch which changed the builtin to use custom >> type-checking didn't need to worry about this because doing that will stop >> Sema from converting to `const void *`. You seem to have taken all of that >> code out and reverted to an earlier approach, though, and I don't see >> anything in this review explaining why. >> >> Also, please don't ping for review on Monday when you submitted a new >> version of the patch on Saturday. Not only is that a very short period to >> be demanding review during, but a lot of reviewers keep up with review as >> part of their normal workday and cannot be expected to review over the >> weekend. > > Thanks for your reply John, the patch of the previous version which will emit > an error when 1st arg is volatile-qualified, we're worried about the shift > to emit an unconditional error on `volatile`. So, I'll try to separate that > into its own patch and just fix the bug in this one. and I'm so sorry about > have 'pin' too often, this is due to time zone, I will pay attention to it > later, and thanks for your tips! Hmm. I think you can take the previous version of the patch and either (1) remove the place where it explicitly emits an error or (2) make it only emit an error in C++ mode. I think the type descriptor for something that starts as an array reference should probably be a pointer, i.e. you should apply array decay and then derive the type from that. That's one of the things that'll just fall out naturally if you do what the previous version of the patch was doing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131979/new/ https://reviews.llvm.org/D131979 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
