rjmccall added a comment.

Please look into the history of that test case to see why we're specifically 
testing for the ability to redeclare this builtin.

We could certainly allow this builtin to be redeclared using the `const void*` 
type — that doesn't really reflect the generic nature of the builtin, but it 
would work for compatibility with other compilers in the vast preponderance of 
code — but if we don't really need to do it, let's not go down that road.



================
Comment at: clang/lib/Sema/SemaChecking.cpp:7651-7652
+  Expr *FirstArg = TheCall->getArg(0);
+  if (auto *CE = dyn_cast<CastExpr>(FirstArg))
+    FirstArg = CE->getSubExprAsWritten();
 
----------------
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.


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

Reply via email to