[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

yihanaa added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:7671 + AllArgs, CallType)) +return true; + rjmccall wrote: > yihanaa wrote: > > rjmccall wrote: > > > yihanaa wrote: > > > > rjmccall wrote: > > > > > You c

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:7671 + AllArgs, CallType)) +return true; + yihanaa wrote: > rjmccall wrote: > > yihanaa wrote: > > > rjmccall wrote: > > > > You can just pull the argument

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

yihanaa added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:7671 + AllArgs, CallType)) +return true; + rjmccall wrote: > yihanaa wrote: > > rjmccall wrote: > > > You can just pull the argument expressions out of the

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:7671 + AllArgs, CallType)) +return true; + yihanaa wrote: > rjmccall wrote: > > You can just pull the argument expressions out of the `CallExpr`; you don't

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

yihanaa added a comment. Thanks for your review John, I'll try to fix these problem later which you point out. Comment at: clang/lib/Sema/SemaChecking.cpp:7659 + << 0 << 2 << TheCall->getNumArgs() << Callee->getSourceRange(); + } + rjmccall wrote: >

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

rjmccall added a comment. Thanks. Just a few small requests to re-use more of the existing logic here. Comment at: clang/lib/Sema/SemaChecking.cpp:7659 + << 0 << 2 << TheCall->getNumArgs() << Callee->getSourceRange(); + } + It looks like this should

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

yihanaa added a comment. Now that parameter checking is generic, I think it would be better to use both common seam checking and custom sema checking, like Diff 456142 (https://reviews.llvm.org/differential/diff/456142/), what do you think about? Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

yihanaa added a comment. In D131979#3756474 , @rjmccall wrote: > In D131979#3756067 , @yihanaa wrote: > >> In D131979#3756006 , @rjmccall >> wrote: >> >>> In D131979#3753

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

yihanaa updated this revision to Diff 456646. yihanaa added a comment. Use custom seam checking on __builtin_assume_aligned. C++ not allow implicit cast from volatile void * to const void *, but C allowed and only emit a warning. This is a general case, volatile in __builtin_assume_aligned just

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

rjmccall added a comment. In D131979#3756067 , @yihanaa wrote: > In D131979#3756006 , @rjmccall > wrote: > >> In D131979#3753358 , @yihanaa >> wrote: >> >>> In D131979#3

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

yihanaa added a comment. In D131979#3753706 , @yihanaa wrote: > Keep same behavior with current clang, don't emit an error when 1st arg is > volatile qualified. > > Ignore the implicit cast from user-written-type to 'const void *' on the 1st > arg, beca

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

yihanaa added a comment. In D131979#3756006 , @rjmccall wrote: > In D131979#3753358 , @yihanaa wrote: > >> In D131979#3752208 , @rjmccall >> wrote: >> >>> From the test c

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

rjmccall added a comment. In D131979#3753358 , @yihanaa wrote: > In D131979#3752208 , @rjmccall > wrote: > >> From the test case, it looks like the builtin just ignores pointers to >> volatile types, which shoul

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

yihanaa added a comment. pin～ 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

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

yihanaa added a comment. pin~ 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

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

yihanaa updated this revision to Diff 456142. yihanaa added a comment. Format code and recovery builtin-functions.cpp(Wrongly modified in the last update) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131979/new/ https://reviews.llvm.org/D131979

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

yihanaa updated this revision to Diff 456140. yihanaa added a comment. Keep same behavior with current clang, don't emit an error when 1st arg is volatile qualified. Ignore the implicit cast from user-written-type to 'const void *' on the 1st arg, because CodeGen need user-written-type to gener

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

yihanaa added a comment. In 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 ab

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

rjmccall added a comment. From the test case, it looks like the builtin just ignores pointer to volatile, 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()->isVolatile()`. Repository: rG LLVM Github Mo

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

yihanaa added a comment. In D131979#3752131 , @rjmccall wrote: > I'm a little worried about the shift to emit an unconditional error on > `volatile`. Can we at least separate that into its own patch and just fix > the bug in this one? And please try t

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

rjmccall added a comment. I'm a little worried about the shift to emit an unconditional error on `volatile`. Can we at least separate that into its own patch and just fix the bug in this one? And please try to reach out whoever originally added this feature to see if that restriction is okay

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

yihanaa updated this revision to Diff 455918. yihanaa added a comment. Thanks John and Erich for your comments. In this update: Check args in Sema, and both in C and C++, emit an error if the 1st arg of __builtin_assume_aligned is volatile qualified or 1st arg not a pointer type. Remove getSubE

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

yihanaa added a comment. In D131979#3743127 , @rjmccall wrote: > In D131979#3742399 , @yihanaa wrote: > >> As far as I know, turning on the -fsanitizer=alignment options when calling >> __builtin_assume_aligned i

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

rjmccall added a comment. In D131979#3742399 , @yihanaa wrote: > As far as I know, turning on the -fsanitizer=alignment options when calling > __builtin_assume_aligned in C code, if > the 1st arg has volatile qualifier, Clang will emit "call void > @__u

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

yihanaa added a comment. As far as I know, turning on the -fsanitizer=alignment options when calling __builtin_assume_aligned in C code, if the 1st arg has volatile qualifier, Clang will emit "call void @__ubsan_handle_alignment_assumption(...)" in CodeGen, else Clang will emit an warning and ig

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

yihanaa added a comment. Thanks for your suggestion @erichkeane @rjmccall , I will try to do that Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131979/new/ https://reviews.llvm.org/D131979 ___ cfe-commit

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

rjmccall added a comment. `getSubExprAsWritten`, as the name suggests, is a query for exploring the source code as written; it should generally not be used in CodeGen, which should be respecting the semantics of the AST. If Sema is applying implicit conversions that aren't desirable, we should

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

yihanaa added a comment. In D131979#3726686 , @erichkeane wrote: > I think the correct answer here is instead to prohibit using this on > array/string literal types in Sema instead of just ignoring the call. Thanks for you replay @erichkeane , in clan

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

erichkeane added a comment. I think the correct answer here is instead to prohibit using this on array/string literal types in Sema instead of just ignoring the call. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131979/new/ https://reviews.llvm.o

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

yihanaa created this revision. yihanaa added reviewers: rjmccall, aaron.ballman, erichkeane, lebedev.ri. yihanaa added a project: clang. Herald added a project: All. yihanaa requested review of this revision. Herald added a subscriber: cfe-commits. Clang will crash when __builtin_assume_aligned's