[Clang] Avoid __builtin_assume_aligned crash when the 1st arg is array type

[yronglin]

Summary

Avoid __builtin_assume_aligned crash when the 1st arg is array type(or string literal).

Open issue: https://github.com/llvm/llvm-project/issues/57169

[yronglin]

This comment was removed by yronglin.

[yronglin]

Format code

[rjmccall]

Other than the one thing, this looks good.

clang/lib/Sema/SemaChecking.cpp

7697

This should be:

Expr *SecondArg = TheCall->getArg(1);
if (convertArgumentToType(*this, SecondArg, Context.getSizeType()))
  return true;
TheCall->setArg(1, SecondArg);

if (!SecondArg->isValueDependent()) {
  llvm::APSInt Result;
  ....
}

Test case is to pass a floating-point expression or something like that.

[yronglin]

Update patch with john's comments

clang/lib/Sema/SemaChecking.cpp

7697

+1

[rjmccall]

Thanks, LGTM

[yronglin]

This comment was removed by yronglin.

[vitalybuka]

Broken by the patch https://lab.llvm.org/buildbot/#/builders/127/builds/35304

[yronglin]

In D133202#3768770, @vitalybuka wrote:

Broken by the patch https://lab.llvm.org/buildbot/#/builders/127/builds/35304

Thanks @vitalybuka , seems intrin0.inl.h have a forward declaration __MACHINE(void * __cdecl __builtin_assume_aligned(const void *, size_t, ...) noexcept), should we use nc in BUILTIN(__builtin_assume_aligned, "v*vC*z.", "nct") but not nct ?

[yronglin]

Fix windows build bolt broken https://lab.llvm.org/buildbot/#/builders/127/builds/35304

[aaron.ballman]

LGTM, but can you add a release note to clang/docs/ReleaseNotes.rst for the fix? Also, do you need someone to commit this on your behalf? If so, what name and email address would you like used for patch attribution?

[yronglin]

Update ReleaseNotes

[yronglin]

Thanks for your review @aaron.ballman , I've updated ReleaseNotes. I don’t have commit access, can you land this patch for me?Please use 'yronglin <[e-mail]>' to commit the change.

[yronglin]

ping~

[aaron.ballman]

I've pushed the changes up for you, thanks!

In D133202#3774801, @yronglin wrote:

ping~

Something to keep in mind for the future: we typically only "ping" a review after about a week of no activity on it (just because folks end up being busy, taking vacation, want to contemplate the review for a while, etc).

[yronglin]

In D133202#3774905, @aaron.ballman wrote:

I've pushed the changes up for you, thanks!

In D133202#3774801, @yronglin wrote:

ping~

Something to keep in mind for the future: we typically only "ping" a review after about a week of no activity on it (just because folks end up being busy, taking vacation, want to contemplate the review for a while, etc).

Thanks a lot Aaron, I will remember what you said.

[rsmith]

clang/lib/Sema/SemaChecking.cpp

7685–7686

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?

[rjmccall]

clang/lib/Sema/SemaChecking.cpp

7685–7686

Somehow I missed this in my review. Yes, this line should be unnecessary, and as you say, it is definitely wrong.

[yronglin]

Thanks a lot for your comments @rsmith @rjmccall .

Firstly, as far as I know, turning on the -fsanitizer=alignment options and calling __builtin_assume_aligned, Clang will emit call void @__ubsan_handle_alignment_assumption(...) in CodeGen, and CodeGen need user-written-type for 1st arg to generate correct TypeDescriptor (this class in compiler-rt/UBSan).

Secondly, before this patch, clang::CodeGen::CodeGenFunction::emitAlignmentAssumption use CastExpr->getSubExprAsWritten to get user-written-type in CodeGen. In Diff 457643 , we use custom sema checking.and just do DefaultFunctionArrayLvalueConversion on 1st arg, but not implicit cast 1st arg to const void *(We expect pass user-written-type to CodeGen).

Unfortunately, Diff 457643 broken windows sanitize test, because there have a forward declaration __MACHINE(void * __cdecl __builtin_assume_aligned(const void *, size_t, ...) noexcept)in intrin0.inl.h, I think the reason for this problem is we use nct in BUILTIN(__builtin_assume_aligned, "v*vC*z.", "nct"), I try to find a solution based on Diff 457643, what do you all think about?

[yronglin]

clang/lib/Sema/SemaChecking.cpp

7685–7686

CodeGen need real user-written-type to generate __ubsan_handle_alignment_assumption 's arg, but not const void *

[yronglin]

Reproduce windows broken test:

ignore.cpp

#include <stddef.h>

void *__builtin_assume_aligned(const void *, size_t, ...) noexcept;
void foo() {
  int a;
  (void) __builtin_assume_aligned(&a, 4);
}

FunctionDecl 0x14a80e480 <./ignore.cpp:3:1, col:7> col:7 __builtin_assume_aligned 'void *(const void *, size_t, ...) noexcept'
|-ParmVarDecl 0x14a80df80 <col:32, col:43> col:44 'const void *'
`-ParmVarDecl 0x14a80e050 <col:46> col:52 'size_t':'unsigned long'
FunctionDecl 0x14a80e210 <./ignore.cpp:3:7> col:7 implicit __builtin_assume_aligned 'void *(const void *, unsigned long, ...) noexcept' extern
|-ParmVarDecl 0x14a80e308 <<invalid sloc>> <invalid sloc> 'const void *'
|-ParmVarDecl 0x14a80e370 <<invalid sloc>> <invalid sloc> 'unsigned long'
|-BuiltinAttr 0x14a80e2b0 <<invalid sloc>> Implicit 402
|-NoThrowAttr 0x14a80e3e8 <col:7> Implicit
`-ConstAttr 0x14a80e410 <col:7> Implicit

./ignore.cpp:3:7: error: cannot redeclare builtin function '__builtin_assume_aligned'
void *__builtin_assume_aligned(const void *, size_t, ...) noexcept;
      ^
./ignore.cpp:3:7: note: '__builtin_assume_aligned' is a builtin with type 'void *(const void *, unsigned long, ...) noexcept'
1 error generated.

[yronglin]

Seems that a builtin can't be redeclared which has custom type checking
https://github.com/llvm/llvm-project/blob/ec8f2905a33ba970543c8edb4141c47f30d325f7/clang/lib/Basic/Builtins.cpp#L210-L214

bool Builtin::Context::canBeRedeclared(unsigned ID) const {
  return ID == Builtin::NotBuiltin || ID == Builtin::BI__va_start ||
         (!hasReferenceArgsOrResult(ID) && !hasCustomTypechecking(ID)) ||
         isInStdNamespace(ID);
}

[rjmccall]

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.

clang/lib/Sema/SemaChecking.cpp

7685–7686

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.

[rsmith]

clang/lib/Sema/SemaChecking.cpp

7685–7686

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);
}

[yronglin]

Thanks very much for your comments @rjmccall @rsmith , I've take a look at D45383, I believe that user code isn't allowed to declare __builtin_*, but seems intrin0.inl.h is a system header on windows, should we keep compatibility(like __va_start in D45383) with it? or breaking it, but I'd like some further guidance. can you help me make that decision? I think you guys are experts in this area and have more experience than me, I'll see if I can accomplish soon! (maybe I should submit a new patch).

[yronglin]

I've a new patch D133583 , please can you guys take a look

[yronglin]

clang/lib/Sema/SemaChecking.cpp

7685–7686

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

clang/include/clang/Basic/Builtins.def

 BUILTIN(__builtin_constant_p, "i.", "nctu")
 BUILTIN(__builtin_classify_type, "i.", "nctu")
 BUILTIN(__builtin___CFStringMakeConstantString, "FC*cC*", "nc")
 BUILTIN(__builtin___NSStringMakeConstantString, "FC*cC*", "nc")
 BUILTIN(__builtin_va_start, "vA.", "nt")
 BUILTIN(__builtin_va_end, "vA", "n")
 BUILTIN(__builtin_va_copy, "vAA", "n")
 BUILTIN(__builtin_stdarg_start, "vA.", "nt")
-BUILTIN(__builtin_assume_aligned, "v*vC*z.", "nc")
+BUILTIN(__builtin_assume_aligned, "v*vC*z.", "nct")
 BUILTIN(__builtin_bcmp, "ivC*vC*z", "Fn")
 BUILTIN(__builtin_bcopy, "vv*v*z", "n")
 BUILTIN(__builtin_bzero, "vv*z", "nF")
 BUILTIN(__builtin_fprintf, "iP*cC*.", "Fp:1:")
 BUILTIN(__builtin_free, "vv*", "nF")
 BUILTIN(__builtin_malloc, "v*z", "nF")
 BUILTIN(__builtin_memchr, "v*vC*iz", "nF")
 BUILTIN(__builtin_memcmp, "ivC*vC*z", "nF")

clang/lib/CodeGen/CGBuiltin.cpp

Function *FnExpect =
         CGM.getIntrinsic(Intrinsic::expect_with_probability, ArgType);
     Value *Result = Builder.CreateCall(
         FnExpect, {ArgValue, ExpectedValue, Confidence}, "expval");
     return RValue::get(Result);
   }
   case Builtin::BI__builtin_assume_aligned: {
     const Expr *Ptr = E->getArg(0);
     Value *PtrValue = EmitScalarExpr(Ptr);
+    if (PtrValue->getType() != VoidPtrTy)
+      PtrValue = EmitCastToVoidPtr(PtrValue);
     Value *OffsetValue =
       (E->getNumArgs() > 2) ? EmitScalarExpr(E->getArg(2)) : nullptr;
 
     Value *AlignmentValue = EmitScalarExpr(E->getArg(1));
     ConstantInt *AlignmentCI = cast<ConstantInt>(AlignmentValue);
     if (AlignmentCI->getValue().ugt(llvm::Value::MaximumAlignment))
       AlignmentCI = ConstantInt::get(AlignmentCI->getType(),
                                      llvm::Value::MaximumAlignment);

clang/lib/CodeGen/CodeGenFunction.cpp

emitAlignmentAssumptionCheck(PtrValue, Ty, Loc, AssumptionLoc, Alignment,
                                OffsetValue, TheCheck, Assumption);
 }
 
 void CodeGenFunction::emitAlignmentAssumption(llvm::Value *PtrValue,
                                               const Expr *E,
                                               SourceLocation AssumptionLoc,
                                               llvm::Value *Alignment,
                                               llvm::Value *OffsetValue) {
-  if (auto *CE = dyn_cast<CastExpr>(E))
-    E = CE->getSubExprAsWritten();
   QualType Ty = E->getType();
   SourceLocation Loc = E->getExprLoc();
 
   emitAlignmentAssumption(PtrValue, Ty, Loc, AssumptionLoc, Alignment,
                           OffsetValue);
 }
 
 llvm::Value *CodeGenFunction::EmitAnnotationCall(llvm::Function *AnnotationFn,

clang/lib/Sema/SemaChecking.cpp

static bool checkArgCountAtLeast(Sema &S, CallExpr *Call,
   if (ArgCount >= MinArgCount)
     return false;
 
   return S.Diag(Call->getEndLoc(), diag::err_typecheck_call_too_few_args)
          << 0 /*function call*/ << MinArgCount << ArgCount
          << Call->getSourceRange();
 }
 
+/// Checks that a call expression's argument count is at most the desired
+/// number. This is useful when doing custom type-checking on a variadic
+/// function. Returns true on error.
+static bool checkArgCountAtMost(Sema &S, CallExpr *Call, unsigned MaxArgCount) {
+  unsigned ArgCount = Call->getNumArgs();
+  if (ArgCount <= MaxArgCount)
+    return false;
+  return S.Diag(Call->getEndLoc(),
+                diag::err_typecheck_call_too_many_args_at_most)
+         << 0 /*function call*/ << MaxArgCount << ArgCount
+         << Call->getSourceRange();
+}
+
+/// Checks that a call expression's argument count is in the desired range. This
+/// is useful when doing custom type-checking on a variadic function. Returns
+/// true on error.
+static bool checkArgCountRange(Sema &S, CallExpr *Call, unsigned MinArgCount,
+                               unsigned MaxArgCount) {
+  return checkArgCountAtLeast(S, Call, MinArgCount) ||
+         checkArgCountAtMost(S, Call, MaxArgCount);
+}
+
 /// Checks that a call expression's argument count is the desired number.
 /// This is useful when doing custom type-checking.  Returns true on error.
 static bool checkArgCount(Sema &S, CallExpr *Call, unsigned DesiredArgCount) {
   unsigned ArgCount = Call->getNumArgs();
   if (ArgCount == DesiredArgCount)
     return false;
 
   if (checkArgCountAtLeast(S, Call, DesiredArgCount))
     return true;
   assert(ArgCount > DesiredArgCount && "should have diagnosed this");
 
   // Highlight all the excess arguments.
   SourceRange Range(Call->getArg(DesiredArgCount)->getBeginLoc(),
                     Call->getArg(ArgCount - 1)->getEndLoc());
 
   return S.Diag(Range.getBegin(), diag::err_typecheck_call_too_many_args)
          << 0 /*function call*/ << DesiredArgCount << ArgCount
          << Call->getArg(1)->getSourceRange();
 }
 
+static bool convertArgumentToType(Sema &S, Expr *&Value, QualType Ty) {
+  if (Value->isTypeDependent())
+    return false;
+
+  InitializedEntity Entity =
+    InitializedEntity::InitializeParameter(S.Context, Ty, false);
+  ExprResult Result =
+    S.PerformCopyInitialization(Entity, SourceLocation(), Value);
+  if (Result.isInvalid())
+    return true;
+  Value = Result.get();
+  return false;
+}
+
 /// Check that the first argument to __builtin_annotation is an integer
 /// and the second argument is a non-wide string literal.
 static bool SemaBuiltinAnnotation(Sema &S, CallExpr *TheCall) {
   if (checkArgCount(S, TheCall, 2))
     return true;
 
   // First argument should be an integer.
   Expr *ValArg = TheCall->getArg(0);
bool Sema::SemaBuiltinAllocaWithAlign(CallExpr *TheCall) {
   }
 
   return false;
 }
 
 /// Handle __builtin_assume_aligned. This is declared
 /// as (const void*, size_t, ...) and can take one optional constant int arg.
 bool Sema::SemaBuiltinAssumeAligned(CallExpr *TheCall) {
+  if (checkArgCountRange(*this, TheCall, 2, 3))
+    return true;
+
   unsigned NumArgs = TheCall->getNumArgs();
+  Expr *FirstArg = TheCall->getArg(0);
   
-  if (NumArgs > 3)
-    return Diag(TheCall->getEndLoc(),
-                diag::err_typecheck_call_too_many_args_at_most)
-           << 0 /*function call*/ << 3 << NumArgs << TheCall->getSourceRange();
+  {
+    ExprResult FirstArgResult = DefaultFunctionArrayLvalueConversion(FirstArg);
+    if (FirstArgResult.isInvalid())
+      return true;
+    TheCall->setArg(0, FirstArgResult.get());
+  }
 
   // The alignment must be a constant integer.
-  Expr *Arg = TheCall->getArg(1);
+  Expr *SecondArg = TheCall->getArg(1);
 
   // We can't check the value of a dependent argument.
-  if (!Arg->isTypeDependent() && !Arg->isValueDependent()) {
+  if (!SecondArg->isTypeDependent() && !SecondArg->isValueDependent()) {
     llvm::APSInt Result;
     if (SemaBuiltinConstantArg(TheCall, 1, Result))
       return true;
 
     if (!Result.isPowerOf2())
       return Diag(TheCall->getBeginLoc(), diag::err_alignment_not_power_of_two)
-             << Arg->getSourceRange();
+             << SecondArg->getSourceRange();
 
     if (Result > Sema::MaximumAlignment)
       Diag(TheCall->getBeginLoc(), diag::warn_assume_aligned_too_great)
-          << Arg->getSourceRange() << Sema::MaximumAlignment;
+          << SecondArg->getSourceRange() << Sema::MaximumAlignment;
   }
 
   if (NumArgs > 2) {
-    ExprResult Arg(TheCall->getArg(2));
-    InitializedEntity Entity = InitializedEntity::InitializeParameter(Context,
-      Context.getSizeType(), false);
-    Arg = PerformCopyInitialization(Entity, SourceLocation(), Arg);
-    if (Arg.isInvalid()) return true;
-    TheCall->setArg(2, Arg.get());
+    Expr *ThirdArg = TheCall->getArg(2);
+    if (convertArgumentToType(*this, ThirdArg, Context.getSizeType()))
+      return true;
+    TheCall->setArg(2, ThirdArg);
   }
 
   return false;
 }
 
 bool Sema::SemaBuiltinOSLogFormat(CallExpr *TheCall) {
   unsigned BuiltinID =
       cast<FunctionDecl>(TheCall->getCalleeDecl())->getBuiltinID();

clang/test/CodeGen/catch-alignment-assumption-array.c

+// RUN: %clang_cc1 -no-opaque-pointers -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s
+// RUN: %clang_cc1 -no-opaque-pointers -fsanitize=alignment -fno-sanitize-recover=alignment -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s -implicit-check-not="call void @__ubsan_handle_alignment_assumption" --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-NORECOVER,CHECK-SANITIZE-UNREACHABLE
+// RUN: %clang_cc1 -no-opaque-pointers -fsanitize=alignment -fsanitize-recover=alignment -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s -implicit-check-not="call void @__ubsan_handle_alignment_assumption" --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-RECOVER
+// RUN: %clang_cc1 -no-opaque-pointers -fsanitize=alignment -fsanitize-trap=alignment -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s -implicit-check-not="call void @__ubsan_handle_alignment_assumption" --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-TRAP,CHECK-SANITIZE-UNREACHABLE
+
+// CHECK-SANITIZE-ANYRECOVER: @[[CHAR:.*]] = {{.*}} c"'char *'\00" }
+// CHECK-SANITIZE-ANYRECOVER: @[[ALIGNMENT_ASSUMPTION:.*]] = {{.*}}, i32 31, i32 35 }, {{.*}}* @[[CHAR]] }
+
+void *caller(void) {
+  char str[] = "";
+  // CHECK:                           define{{.*}}
+  // CHECK-NEXT:                      entry:
+  // CHECK-NEXT:                        %[[STR:.*]] = alloca [1 x i8], align 1
+  // CHECK-NEXT:                        %[[BITCAST:.*]] = bitcast [1 x i8]* %[[STR]] to i8*
+  // CHECK-NEXT:                        call void @llvm.memset.p0i8.i64(i8* align 1 %[[BITCAST]], i8 0, i64 1, i1 false)
+  // CHECK-NEXT:                        %[[ARRAYDECAY:.*]] = getelementptr inbounds [1 x i8], [1 x i8]* %[[STR]], i64 0, i64 0
+  // CHECK-SANITIZE-NEXT:               %[[PTRINT:.*]] = ptrtoint i8* %[[ARRAYDECAY]] to i64
+  // CHECK-SANITIZE-NEXT:               %[[MASKEDPTR:.*]] = and i64 %[[PTRINT]], 0
+  // CHECK-SANITIZE-NEXT:               %[[MASKCOND:.*]] = icmp eq i64 %[[MASKEDPTR]], 0
+  // CHECK-SANITIZE-NEXT:               %[[PTRINT_DUP:.*]] = ptrtoint i8* %[[ARRAYDECAY]] to i64, !nosanitize
+  // CHECK-SANITIZE-NEXT:               br i1 %[[MASKCOND]], label %[[CONT:.*]], label %[[HANDLER_ALIGNMENT_ASSUMPTION:[^,]+]],{{.*}} !nosanitize
+  // CHECK-SANITIZE:                  [[HANDLER_ALIGNMENT_ASSUMPTION]]:
+  // CHECK-SANITIZE-NORECOVER-NEXT:     call void @__ubsan_handle_alignment_assumption_abort(i8* bitcast ({ {{{.*}}}, {{{.*}}}, {{{.*}}}* }* @[[ALIGNMENT_ASSUMPTION]] to i8*), i64 %[[PTRINT_DUP]], i64 1, i64 0){{.*}}, !nosanitize
+  // CHECK-SANITIZE-RECOVER-NEXT:       call void @__ubsan_handle_alignment_assumption(i8* bitcast ({ {{{.*}}}, {{{.*}}}, {{{.*}}}* }* @[[ALIGNMENT_ASSUMPTION]] to i8*), i64 %[[PTRINT_DUP]], i64 1, i64 0){{.*}}, !nosanitize
+  // CHECK-SANITIZE-TRAP-NEXT:          call void @llvm.ubsantrap(i8 23){{.*}}, !nosanitize
+  // CHECK-SANITIZE-UNREACHABLE-NEXT:   unreachable, !nosanitize
+  // CHECK-SANITIZE:                  [[CONT]]:
+  // CHECK-NEXT:                        call void @llvm.assume(i1 true) [ "align"(i8* %[[ARRAYDECAY]], i64 1) ] 
+  // CHECK-NEXT:                        ret i8* %[[ARRAYDECAY]]
+  // CHECK-NEXT:                      }
+  return __builtin_assume_aligned(str, 1);
+}

clang/test/CodeGen/catch-alignment-assumption-ignorelist.c

void *dont_ignore_volatile_ptrs(void * volatile x) {
   // CHECK: call void @__ubsan_handle_alignment_assumption(
   return __builtin_assume_aligned(x, 1);
 }
 
 // CHECK-LABEL: ignore_volatiles
 void *ignore_volatiles(volatile void * x) {
   return __builtin_assume_aligned(x, 1);
 }
+
+// CHECK-LABEL: ignore_array_volatiles
+void *ignore_array_volatiles() {
+  volatile int arr[] = {1};
+  return __builtin_assume_aligned(arr, 4);
+}