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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
