yihanaa added a comment. In D131979#3756474 <https://reviews.llvm.org/D131979#3756474>, @rjmccall wrote:
> 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. Thanks for your suggestions John. As far as I know, ‘warning: passing 'volatile char *' to parameter of type 'const void *' discards qualifiers’ is a common case, so I use GatherArgumentsForCall to handle these common cases. 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
