[clang][ubsan] Fix __builtin_assume_aligned incorrect type descriptor and C++ object polymorphic address

[yronglin]

Summary

Fix __builtin_assume_aligned incorrect type descriptor

example from @rsmith

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]

Hi, follow D133202 , should I both fix alignment in this patch or in another separate patch? (this seems have different behavior with gcc https://godbolt.org/z/7dvM8zhnh )

[yronglin]

format

[aaron.ballman]

In D133583#3780460, @yronglin wrote:

Hi, follow D133202 , should I both fix alignment in this patch or in another separate patch? (this seems have different behavior with gcc https://godbolt.org/z/7dvM8zhnh )

I think that's a separate patch -- this one is fixing a mistake with the type-checking related crash fix and the other is about the behavior of the expression itself when it's valid.

The current patch looks reasonable to me, but I'd love a second opinion.

clang/test/Sema/builtin-redecl.cpp

5–6

We can be tricky instead of including a header file.

[yronglin]

In D133583#3792625, @aaron.ballman wrote:

In D133583#3780460, @yronglin wrote:

Hi, follow D133202 , should I both fix alignment in this patch or in another separate patch? (this seems have different behavior with gcc https://godbolt.org/z/7dvM8zhnh )

I think that's a separate patch -- this one is fixing a mistake with the type-checking related crash fix and the other is about the behavior of the expression itself when it's valid.

The current patch looks reasonable to me, but I'd love a second opinion.

Thanks for your comments @aaron.ballman

clang/test/Sema/builtin-redecl.cpp

5–6

Use decltype not works in C , can we use typedef unsigned long size_t instead?

[aaron.ballman]

clang/test/Sema/builtin-redecl.cpp

5–6

Oh good catch, I had missed the -x c on a RUN line and was looking at the file extension alone. :-) We can't use that typedef because there are some platforms where that type is wrong, but we can be tricky in a different way: typedef __typeof__(sizeof(0)) size_t;

[yronglin]

Add test case for C++ polymorphism class
Use typedef __typeof__(sizeof(0)) size_t; instead of #include <stddef.h>

[yronglin]

Thanks for your suggestion @aaron.ballman , also I have added a new test case for C++ polymorphism classes

clang/test/Sema/builtin-redecl.cpp

5–6

+1

[aaron.ballman]

LGTM! Please give @rjmccall another day to chime in with any concerns he has.

[rjmccall]

LGTM

[yronglin]

Thanks for take a review @aaron.ballman @rjmccall , can you land this patch for me? Please use 'yronglin <[e-mail]>' to commit the change.

[aaron.ballman]

In D133583#3802451, @yronglin wrote:

Thanks for take a review @aaron.ballman @rjmccall , can you land this patch for me? Please use 'yronglin <[e-mail]>' to commit the change.

Happy to do so! I've landed on your behalf in https://github.com/llvm/llvm-project/commit/8392f1cc78270c7039970b413dfd836bf4def602

[yronglin]

In D133583#3803002, @aaron.ballman wrote:

In D133583#3802451, @yronglin wrote:

Thanks for take a review @aaron.ballman @rjmccall , can you land this patch for me? Please use 'yronglin <[e-mail]>' to commit the change.

Happy to do so! I've landed on your behalf in https://github.com/llvm/llvm-project/commit/8392f1cc78270c7039970b413dfd836bf4def602

Thanks very much!

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/Basic/Builtins.cpp

bool Builtin::Context::performsCallback(unsigned ID,
   }
 
   assert(*EndPos == '>' && "Callback callee specifier must end with a '>'");
   return true;
 }
 
 bool Builtin::Context::canBeRedeclared(unsigned ID) const {
   return ID == Builtin::NotBuiltin || ID == Builtin::BI__va_start ||
+         ID == Builtin::BI__builtin_assume_aligned ||
          (!hasReferenceArgsOrResult(ID) && !hasCustomTypechecking(ID)) ||
          isInStdNamespace(ID);
 }
 
 bool Builtin::evaluateRequiredTargetFeatures(
     StringRef RequiredFeatures, const llvm::StringMap<bool> &TargetFetureMap) {
   // Return true if the builtin doesn't have any required features.
   if (RequiredFeatures.empty())
     return true;
   assert(!RequiredFeatures.contains(' ') && "Space in feature list");
 
   TargetFeatures TF(TargetFetureMap);
   return TF.hasRequiredFeatures(RequiredFeatures);
 }

clang/lib/Sema/SemaChecking.cpp

static bool checkArgCountAtMost(Sema &S, CallExpr *Call, unsigned MaxArgCount) {
   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))
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 (checkArgCountAtMost(*this, TheCall, 3))
+  if (checkArgCountRange(*this, TheCall, 2, 3))
     return true;
 
   unsigned NumArgs = TheCall->getNumArgs();
   Expr *FirstArg = TheCall->getArg(0);
-  if (auto *CE = dyn_cast<CastExpr>(FirstArg))
-    FirstArg = CE->getSubExprAsWritten();
 
   {
     ExprResult FirstArgResult =
-        DefaultFunctionArrayLvalueConversion(FirstArg, /*Diagnose=*/false);
+        DefaultFunctionArrayLvalueConversion(FirstArg);
     if (FirstArgResult.isInvalid())
       return true;
     TheCall->setArg(0, FirstArgResult.get());
   }
 
   // The alignment must be a constant integer.
   Expr *SecondArg = TheCall->getArg(1);
 

clang/test/CodeGen/catch-alignment-assumption-builtin_assume_aligned-polymorphism.cpp

+// 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"'B *'\00" }
+// CHECK-SANITIZE-ANYRECOVER: @[[LINE_100_ALIGNMENT_ASSUMPTION:.*]] = {{.*}}, i32 100, i32 35 }, {{.*}}* @[[CHAR]] }
+
+struct A { int n; };
+struct B { int n; };
+struct C : A, B {};
+
+void *f(C *c) {
+  // CHECK:                             define {{.*}} i8* @{{.*}}(%struct.C* noundef %[[C:.*]]) {{.*}} {
+  // CHECK-NEXT:                        [[ENTRY:.*]]:
+  // CHECK-NEXT:                          %[[C_ADDR:.*]] = alloca %struct.C*
+  // CHECK-NEXT:                          store %struct.C* %[[C]], %struct.C** %[[C_ADDR]]
+  // CHECK-NEXT:                          %[[C_RELOAD:.*]] = load %struct.C*, %struct.C** %[[C_ADDR]]
+  // CHECK-NEXT:                          %[[IS_NULL:.*]] = icmp eq %struct.C* %[[C_RELOAD]], null
+  // CHECK-NEXT:                          br i1 %[[IS_NULL]], label %[[CAST_END:[^,]+]], label %[[CAST_NOT_NULL:[^,]+]]
+  // CHECK:                             [[CAST_NOT_NULL]]:
+  // CHECK-NOSANITIZE-NEXT:               %[[BITCAST:.*]] = bitcast %struct.C* %[[C_RELOAD]] to i8*
+  // CHECK-NOSANITIZE-NEXT:               %[[ADD_PTR:.*]] = getelementptr inbounds i8, i8* %[[BITCAST]], i64 4
+  // CHECK-NOSANITIZE-NEXT:               %[[BITCAST2:.*]] = bitcast i8* %[[ADD_PTR]] to %struct.B*
+  // CHECK-NOSANITIZE-NEXT:               br label %[[CAST_END]]
+  // CHECK-SANITIZE-NEXT:                 %[[PTRTOINT:.*]] = ptrtoint %struct.C* %[[C_RELOAD]] to i64, !nosanitize
+  // CHECK-SANITIZE-NEXT:                 %[[MASKEDPTR:.*]] = and i64 %[[PTRTOINT]], 3, !nosanitize
+  // CHECK-SANITIZE-NEXT:                 %[[MASKCOND:.*]] = icmp eq i64 %[[MASKEDPTR]], 0, !nosanitize
+  // CHECK-SANITIZE-NEXT:                 br i1 %[[MASKCOND]], label %[[CONT:[^,]+]], label %[[HANDLER_TYPE_MISMATCH:[^,]+]]
+  // CHECK-SANITIZE:                    [[HANDLER_TYPE_MISMATCH]]:
+  // CHECK-SANITIZE-NORECOVER-NEXT:       call void @__ubsan_handle_type_mismatch_v1_abort(
+  // CHECK-SANITIZE-RECOVER-NEXT:         call void @__ubsan_handle_type_mismatch_v1(
+  // CHECK-SANITIZE-TRAP-NEXT:            call void @llvm.ubsantrap(
+  // CHECK-SANITIZE-UNREACHABLE-NEXT:     unreachable, !nosanitize
+  // CHECK-SANITIZE:                    [[CONT]]:
+  // CHECK-SANITIZE-NEXT:                 %[[BITCAST:.*]] = bitcast %struct.C* %[[C_RELOAD]] to i8*
+  // CHECK-SANITIZE-NEXT:                 %[[ADD_PTR:.*]] = getelementptr inbounds i8, i8* %[[BITCAST]], i64 4
+  // CHECK-SANITIZE-NEXT:                 %[[BITCAST2:.*]] = bitcast i8* %[[ADD_PTR]] to %struct.B*
+  // CHECK-SANITIZE-NEXT:                 br label %[[CAST_END]]
+  // CHECK:                             [[CAST_END]]:
+  // CHECK-NOSANITIZE-NEXT:               %[[CAST_RESULT:.*]] = phi %struct.B* [ %[[BITCAST2]], %[[CAST_NOT_NULL]] ], [ null, %[[ENTRY]] ]
+  // CHECK-NOSANITIZE-NEXT:               %[[CAST_END_BITCAST:.*]] = bitcast %struct.B* %[[CAST_RESULT]] to i8*
+  // CHECK-NOSANITIZE-NEXT:               call void @llvm.assume(i1 true) [ "align"(i8* %[[CAST_END_BITCAST]], i64 8) ]
+  // CHECK-NOSANITIZE-NEXT:               ret i8* %[[CAST_END_BITCAST]]
+  // CHECK-NOSANITIZE-NEXT:              }
+  // CHECK-SANITIZE-NEXT:                 %[[CAST_RESULT:.*]] = phi %struct.B* [ %[[BITCAST2]], %[[CONT]] ], [ null, %[[ENTRY]] ]
+  // CHECK-SANITIZE-NEXT:                 %[[CAST_END_BITCAST:.*]] = bitcast %struct.B* %[[CAST_RESULT]] to i8*
+  // CHECK-SANITIZE-NEXT:                 %[[PTRINT:.*]] = ptrtoint i8* %[[CAST_END_BITCAST]] to i64
+  // CHECK-SANITIZE-NEXT:                 %[[MASKEDPTR:.*]] = and i64 %[[PTRINT]], 7
+  // CHECK-SANITIZE-NEXT:                 %[[MASKCOND:.*]] = icmp eq i64 %[[MASKEDPTR]], 0
+  // CHECK-SANITIZE-NEXT:                 %[[PTRINT_DUP:.*]] = ptrtoint i8* %[[CAST_END_BITCAST]] to i64, !nosanitize
+  // CHECK-SANITIZE-NEXT:                 br i1 %[[MASKCOND]], label %[[CONT1:.*]], label %[[HANDLER_ALIGNMENT_ASSUMPTION:[^,]+]],{{.*}} !nosanitize
+  // CHECK-SANITIZE:                    [[HANDLER_ALIGNMENT_ASSUMPTION]]:
+  // CHECK-SANITIZE-NORECOVER-NEXT:       call void @__ubsan_handle_alignment_assumption_abort(i8* bitcast ({ {{{.*}}}, {{{.*}}}, {{{.*}}}* }* @[[LINE_100_ALIGNMENT_ASSUMPTION]] to i8*), i64 %[[PTRINT_DUP]], i64 8, i64 0){{.*}}, !nosanitize
+  // CHECK-SANITIZE-RECOVER-NEXT:         call void @__ubsan_handle_alignment_assumption(i8* bitcast ({ {{{.*}}}, {{{.*}}}, {{{.*}}}* }* @[[LINE_100_ALIGNMENT_ASSUMPTION]] to i8*), i64 %[[PTRINT_DUP]], i64 8, i64 0){{.*}}, !nosanitize
+  // CHECK-SANITIZE-TRAP-NEXT:            call void @llvm.ubsantrap(i8 23){{.*}}, !nosanitize
+  // CHECK-SANITIZE-UNREACHABLE-NEXT:     unreachable, !nosanitize
+  // CHECK-SANITIZE:                    [[CONT1]]:
+  // CHECK-SANITIZE-NEXT:                 call void @llvm.assume(i1 true) [ "align"(i8* %[[CAST_END_BITCAST]], i64 8) ]
+  // CHECK-SANITIZE-NEXT:                 ret i8* %[[CAST_END_BITCAST]]
+  // CHECK-SANITIZE-NEXT:                }
+#line 100
+  return __builtin_assume_aligned((B*)c, 8);
+}

clang/test/Sema/builtin-redecl.cpp

 // RUN: %clang_cc1 %s -fsyntax-only -verify 
 // RUN: %clang_cc1 %s -fsyntax-only -verify -x c
 // RUN: %clang_cc1 %s -fsyntax-only -verify -fms-compatibility
 
+typedef __typeof__(sizeof(0)) size_t;
+
 // Redeclaring library builtins is OK.
 void exit(int);
 
 // expected-error@+2 {{cannot redeclare builtin function '__builtin_va_copy'}}
 // expected-note@+1 {{'__builtin_va_copy' is a builtin with type}}
 void __builtin_va_copy(double d);
 
 // expected-error@+2 {{cannot redeclare builtin function '__builtin_va_end'}}
 // expected-note@+1 {{'__builtin_va_end' is a builtin with type}}
 void __builtin_va_end(__builtin_va_list);
 // RUN: %clang_cc1 %s -fsyntax-only -verify 
 // RUN: %clang_cc1 %s -fsyntax-only -verify -x c
 
 void __va_start(__builtin_va_list*, ...);
+
+#ifdef __cplusplus
+void *__builtin_assume_aligned(const void *, size_t, ...) noexcept;
+#else
+void *__builtin_assume_aligned(const void *, size_t, ...);
+#endif