[PATCH] D149514: Check if First argument in _builtin_assume_aligned_ is of pointer type

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D149514#4317734 <https://reviews.llvm.org/D149514#4317734>, @tbaeder wrote:

> This patch was submitted by a beginner, because https://github.com/llvm/llvm-project/issues/62305 has the "good first issue" label. From the last few comments, I'm not sure they know how to proceed. Could you summarize what the next steps are?

Thank you for pointing this out! FWIW, I think this is good incremental progress because it fixes a crash. We can leave the AST fidelity work to a follow up. However, I think the comment from @rsmith (https://reviews.llvm.org/D149514#4310865) can be addressed as part of this patch.



================
Comment at: clang/lib/Sema/SemaChecking.cpp:7979-7986
   {
     ExprResult FirstArgResult =
         DefaultFunctionArrayLvalueConversion(FirstArg);
-    if (FirstArgResult.isInvalid())
+    if (checkBuiltinArgument(*this, TheCall, 0))
       return true;
+    /// In-place updation of FirstArg by checkBuiltinArgument is ignored.
     TheCall->setArg(0, FirstArgResult.get());
----------------
barannikov88 wrote:
> yronglin wrote:
> > rsmith wrote:
> > > This still seems to be more complex than necessary. If we can't just do this, we should have a comment explaining why. (Eg, does CodeGen really want the final conversion from `T*` to `const void*` to not be in the AST?)
> > As far as I know, alignment sanitizer need a user written type descriptor (but not `const void *`) to emit diagnostic message, Eg:
> > ```
> > /app/example.cpp:7:35: runtime error: assumption of 8 byte alignment for pointer of type 'B *' failed
> > 
> > ```
> > So, not only convert `T *` to `const void *`, but also we need keep the user written type info. Previously, the solution to this problem was to use below code in codegen:
> > ```
> > if (auto *CE = dyn_cast<CastExpr>(E))
> >     E = CE->getSubExprAsWritten();
> > ```
> > since D133202 and D133583, we only do de default array/function conversion in sema, and convert the 1st arg to `const void *` in codegen. And I think the current solution is not very good, do you have any other better solutions? Should we keep the original type info in somewhere? Then we can simplify our work in sema. WDYT?
> I'm not qualified enough in this area to suggest anything, but I'd assume that the type should live through to CodeGen, where implicit casts should be ignored, if this is necessary.
> 
> > assumption of 8 byte alignment for pointer of type 'B *' failed
> I'm not sure what's value of mentioning the type here. It already gives the precise location, but whatever.
> 
> If this needs to be addressed, this should be done in a different patch.
> 
I think we'd want the AST to have full fidelity, and so that would retain the type through to CodeGen and then ignore the implicit cast from there. However, because this is fixing a crash, I think it's fine to leave that work to follow-up efforts if someone wants to tackle that.


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

https://reviews.llvm.org/D149514