[PATCH] D133202: [Clang] Avoid __builtin_assume_aligned crash when the 1st arg is array type

[Yurong via Phabricator via cfe-commits]

yronglin marked 2 inline comments as done.
yronglin added inline comments.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:7651-7652
+  Expr *FirstArg = TheCall->getArg(0);
+  if (auto *CE = dyn_cast<CastExpr>(FirstArg))
+    FirstArg = CE->getSubExprAsWritten();
 
----------------
rsmith wrote:
> rjmccall wrote:
> > yronglin wrote:
> > > rjmccall wrote:
> > > > rsmith wrote:
> > > > > This looks very suspicious to me: this will remove a cast expression that was written in the source code from the AST. That loses source fidelity, can give the wrong answer if the cast changed the value (such as a C++ derived-to-base conversion to a non-primary base class), and in any case this is only done once where there could be multiple explicit casts written on an argument to the builtin, so if it's necessary, then it's not being done fully.
> > > > > 
> > > > > Can this be removed?
> > > > Somehow I missed this in my review.  Yes, this line should be unnecessary, and as you say, it is definitely wrong.
> > > CodeGen need real `user-written-type` to generate `__ubsan_handle_alignment_assumption ` 's arg, but not `const void *`
> > Because we're using custom type-checking here, there is no conversion to `const void *` in the first place; that conversion was an artifact of the earlier implementation.
> > 
> > Also, if the previous code in CodeGen looked like this, then it was buggy: it is incorrect in the case that someone wrote `(const void*) foo` as the argument, because it would inappropriately look through the explicit, user-provided cast.
> The old implementation could get the pointer value wrong, not only the alignment. For example:
> ```
> struct A { virtual ~A(); };
> 
> void *f(A *a) {
>   // Incorrectly returns `a` rather than the address of the complete object.
>   return __builtin_assume_aligned(dynamic_cast<void*>(a), 8);
> }
> ```
> The new implementation gets the pointer value wrong in more cases, such as:
> ```
> struct A { int n; };
> struct B { int n; };
> struct C : A, B {};
> 
> void *f(C *c) {
>   // Incorrectly returns `c` rather than the address of the B base class.
>   return __builtin_assume_aligned((B*)c, 8);
> }
> ```
Thanks for your tips, and is this an error in clang, https://godbolt.org/z/91qxq73Mb, if lose return 0 in main, ubsan will emit an error, but it will pass when there has a return statement. and I have submit an issue https://github.com/llvm/llvm-project/issues/62478


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133202/new/

https://reviews.llvm.org/D133202