[PATCH] D92001: [ubsan] Fix crash on __builtin_assume_aligned

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri requested changes to this revision.
lebedev.ri added a comment.
This revision now requires changes to proceed.

@orivej please can you look where we create AST nodes for these builtins and ensure that the "pointer" argument is actually converted into a pointer beforehand?
I'm afraid i can't readily point at the examples, but it should be easy-ish..



================
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:2524
   // defined.
-  if (Ty->getPointeeType().isVolatileQualified())
+  if (!Ty->getPointeeType().isNull() && Ty->getPointeeType().isVolatileQualified())
     return;
----------------
aaron.ballman wrote:
> lebedev.ri wrote:
> > vsk wrote:
> > > Is the pointee type associated with a PointerType QualType ever supposed to be null? I wonder why this happens, and whether it can cause problems in other places.
> > Basically, we can't just use `getPointeeType()` here, but i'm not sure what should be used instead.
> I'm not super familiar with `__builtin_assume_aligned` but from the GCC docs, it looks like the code from the test case is valid code (so we don't need to add semantic checking that would ensure we don't reach this code path). However, it seems a bit odd to me that we'd get an array type here as opposed to a decayed type which would actually be a pointer.
> 
> I think the issue is further up the call chain, perhaps. `EmitBuiltinExpr()` gets the argument expression and passes it to `emitAlignmentAssumption()` which pulls the type directly out of the expression. It seems like there's an lvalue to rvalue conversion step missing to adjust the type.
Aha, yes, that does sound like the underying problem.
I was thinking about that beforehand, but wasn't sure.


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

https://reviews.llvm.org/D92001