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