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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith 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();
 
----------------
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);
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133202