[ubsan] Fix crash on __builtin_assume_aligned

[MaskRay]

Summary

Fixes PR45813

[orivej]

Sorry, I don't understand the issue and can't write a better description.
Since this test triggers an assert in getCommonPtr that it "Cannot retrieve a NULL type pointer" of Ty->getPointeeType(), the fix just checks that the pointee is not null before using isVolatileQualified.

[orivej]

Could somebody review this?

[vsk]

clang/lib/CodeGen/CodeGenFunction.cpp

2524

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.

[lebedev.ri]

clang/lib/CodeGen/CodeGenFunction.cpp

2524

Basically, we can't just use getPointeeType() here, but i'm not sure what should be used instead.

[aaron.ballman]

clang/lib/CodeGen/CodeGenFunction.cpp

2524

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.

[lebedev.ri]

@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..

clang/lib/CodeGen/CodeGenFunction.cpp

2524

Aha, yes, that does sound like the underying problem.
I was thinking about that beforehand, but wasn't sure.

[lebedev.ri]

This review seems to be stuck/dead, consider abandoning if no longer relevant.

[MaskRay]

-fsanitize=alignment -c a.c -O[012] no longer crashes for the test. From the comment that the problem was up the call chain, I think we can close this revision now.

clang/lib/CodeGen/CodeGenFunction.cpp

assert(&(Builder.GetInsertBlock()->back()) == Assumption &&
          "Assumption should be the last instruction of the basic block, "
          "since the basic block is still being generated.");
 
   if (!SanOpts.has(SanitizerKind::Alignment))
     return;
 
   // Don't check pointers to volatile data. The behavior here is implementation-
   // defined.
-  if (Ty->getPointeeType().isVolatileQualified())
+  if (!Ty->getPointeeType().isNull() && Ty->getPointeeType().isVolatileQualified())
     return;
 
   // We need to temorairly remove the assumption so we can insert the
   // sanitizer check before it, else the check will be dropped by optimizations.
   Assumption->removeFromParent();
 
   {
     SanitizerScope SanScope(this);

clang/test/CodeGen/ubsan-assume-aligned-crash.c

+// RUN: %clang_cc1 -fsanitize=alignment -emit-llvm %s -o /dev/null
+
+/* Testcase for PR - clang crashes checking isVolatileQualified of isNull pointee. */
+
+__attribute__((aligned(8))) int data[2];
+
+int* test() {
+    return __builtin_assume_aligned(data, 8);
+}