Check if First argument in _builtin_assume_aligned_ is of pointer type

[Ris-Bali]

Summary

Currently clang doesn't verify if the first argument in _builtin_assume_aligned_ is of pointer type. This leads to an assertion build failure. This patch aims to add a check if the first argument is of pointer type or not and diagnose it with diag::err_typecheck_convert_incompatible if its not of pointer type.

[Ris-Bali]

Clang-format fix

[Ris-Bali]

QualType error fix

[shafik]

Thank you for the fix, you need to add a test that exercises the diagnostic you added, I think clang/test/Sema/builtin-assume-aligned.c would be the right place to add it.

Also please add a release note in clang/docs/ReleaseNotes.rst

[Ris-Bali]

Added test and made changes in ReleaseNotes.rst

[Ris-Bali]

Removed __builtin_assume_aligned from ignoredbuiltinstest() in clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-vararg.cpp.

[barannikov88]

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-vararg.cpp

55 ↗

(On Diff #518274)

The test should not be removed, it is here for a reason.
In fact, passing zero to a pointer parameter is valid in C and is accepted in C++ mode with a diagnostic (-Wzero-as-null-pointer-constant, disabled by default).

clang/lib/Sema/SemaChecking.cpp

7984

Variables should have CamelCase names.

7986

Instead of directly checking for the pointer type, it is better to call checkBuiltinArgument, which will handle many corner cases.
See the uses of this function in this file.

clang/test/Sema/builtin-assume-aligned.c

75

The expected type is not int *, it is cost void * (according to the definition of the builtin in Builtins.def).

77

Please add a test that passes an array to the builtin. It should pass.

[barannikov88]

clang/lib/Sema/SemaChecking.cpp

7986

It looks like this whole block can be replaced by a single function call.

[Ris-Bali]

Requested changes

[Ris-Bali]

Array test added

[barannikov88]

clang/lib/Sema/SemaChecking.cpp

7982

This check is likely also done by checkBuiltinArgument.

7989

checkBuiltinArgument already issues an error if the argument does not have the correct type.
Please also make sure that other tests pass, they may need some adjustments (ninja check-clang).
Ask for a guidance if you don't know how to fix them.

[Ris-Bali]

Requested changes

[Ris-Bali]

Removed redundant code

[Ris-Bali]

I have made the requested changes. Please do let me know if any other changes are required

[yronglin]

Looks good from my side, but need @rsmith and @rjmccall also take a look with any concerns

clang/lib/Sema/SemaChecking.cpp

7993

Can we add a comments here that we ignored the in-place updating for 1st arg from checkBuiltinArgument?

clang/test/Sema/builtin-assume-aligned.c

79

Please add a test for function type

[aaron.ballman]

Generally looks good to me, but there were a few minor things. Do you need someone to commit on your behalf once those are addressed? If so, what name and email address would you like used for patch attribution?

clang/docs/ReleaseNotes.rst

343

clang/lib/Sema/SemaChecking.cpp

7982–7992

The comment doesn't really add too much value, so removing it.

clang/test/Sema/builtin-assume-aligned.c

75

The GCC documentation for this builtin says const void *, Clang has no documentation for this one specifically.

79

+1, test to make sure function to pointer decay happens.

[barannikov88]

clang/lib/Sema/SemaChecking.cpp

7993

This looks suspicious to me, but I'm no clang expert.
Is it safe to ignore casts inserted by checkBuiltinArgument, or, in other words,
is it correct to pass an argument of incompatible type?

[Ris-Bali]

Added test for function type

[rsmith]

There's some corresponding code in lib/CodeGen that can be deleted too:

   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;

clang/lib/Sema/SemaChecking.cpp

7979–7994

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?)

[yronglin]

clang/lib/Sema/SemaChecking.cpp

7979–7994

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?

[barannikov88]

clang/lib/Sema/SemaChecking.cpp

7979–7994

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.

[tbaeder]

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?

[aaron.ballman]

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

clang/lib/Sema/SemaChecking.cpp

7979–7994

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.

[Ris-Bali]

Requested Changes

[Ris-Bali]

Rebase

[Ris-Bali]

I have made the requested changes. Please do let me know if other changes are required. Also I would like to thank everyone for their help and guidance.

[aaron.ballman]

LGTM, thank you! Do you need someone to commit on your behalf? If so, what name and email address would you like used for patch attribution?

[Ris-Bali]

Name : Rishabh Bali
Github Username : Ris-Bali
email-id : [e-mail]

clang/docs/ReleaseNotes.rst

- Fix crash when handling undefined template partial specialization
   (`#61356 <https://github.com/llvm/llvm-project/issues/61356>`_)
 - Fix a crash caused by incorrectly evaluating constraints on an inheriting
   constructor declaration.
   (`#62361 <https://github.com/llvm/llvm-project/issues/62361>`_)
   (`#62362 <https://github.com/llvm/llvm-project/issues/62362>`_)
 - Fix crash when attempting to perform parenthesized initialization of an
   aggregate with a base class with only non-public constructors.
   (`#62296 <https://github.com/llvm/llvm-project/issues/62296>`_)
+- Fix crash when attempting to pass a non-pointer type as first argument of
+  ``__builtin_assume_aligned``.
+  (`#62305 <https://github.com/llvm/llvm-project/issues/62305>`)
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Bug Fixes to Attribute Support
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - Fixed a bug where attribute annotations on type specifiers (enums, classes,
   structs, unions, and scoped enums) were not properly ignored, resulting in

clang/lib/Sema/SemaChecking.cpp

 /// 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);
 
   {
     ExprResult FirstArgResult =
         DefaultFunctionArrayLvalueConversion(FirstArg);
     if (FirstArgResult.isInvalid())
       return true;
+    QualType FirstArgType = FirstArgResult.get()->getType();
+
+    if (checkBuiltinArgument(*this, TheCall, 0)) {
+      const QualType &ConstVoidPointerType =
+          Context.getPointerType(Context.VoidTy.withConst());
+      return Diag(FirstArg->getBeginLoc(),
+                  diag::err_typecheck_convert_incompatible)
+             << FirstArgType << ConstVoidPointerType << 1 << 0 << 0;
+    }
     TheCall->setArg(0, FirstArgResult.get());
   }
 
   // The alignment must be a constant integer.
   Expr *SecondArg = TheCall->getArg(1);
 
   // We can't check the value of a dependent argument.
   if (!SecondArg->isValueDependent()) {
     llvm::APSInt Result;
     if (SemaBuiltinConstantArg(TheCall, 1, Result))

clang/test/Sema/builtin-assume-aligned.c

 }
 #endif
 
 int test13(int *a) {
   a = (int *)__builtin_assume_aligned(a, 2 * 2.0); // expected-error {{argument to '__builtin_assume_aligned' must be a constant integer}}
   return a[0];
 }
 
+int test14(int *a, int b) {
+  a = (int *)__builtin_assume_aligned(b, 32); // expected-error {{incompatible integer to pointer conversion passing 'int' to parameter of type 'const void *}}
+  return a[0];
+}
+
+int test15(int *b) {
+  int arr[3] = {1, 2, 3};
+  b = (int *)__builtin_assume_aligned(arr, 32); 
+  return b[0];
+}
+
 void test_void_assume_aligned(void) __attribute__((assume_aligned(32))); // expected-warning {{'assume_aligned' attribute only applies to return values that are pointers}}
 int test_int_assume_aligned(void) __attribute__((assume_aligned(16))); // expected-warning {{'assume_aligned' attribute only applies to return values that are pointers}}
 void *test_ptr_assume_aligned(void) __attribute__((assume_aligned(64))); // no-warning
 void *test_ptr_assume_aligned(void) __attribute__((assume_aligned(8589934592))); // expected-warning {{requested alignment must be 4294967296 bytes or smaller; maximum alignment assumed}}
 
 int j __attribute__((assume_aligned(8))); // expected-warning {{'assume_aligned' attribute only applies to Objective-C methods and functions}}
 void *test_no_fn_proto() __attribute__((assume_aligned(32))); // no-warning
 void *test_with_fn_proto(void) __attribute__((assume_aligned(128))); // no-warning
 
 void *test_no_fn_proto() __attribute__((assume_aligned(31))); // expected-error {{requested alignment is not a power of 2}}
 void *test_no_fn_proto() __attribute__((assume_aligned(32, 73))); // no-warning
 
 void *test_no_fn_proto() __attribute__((assume_aligned)); // expected-error {{'assume_aligned' attribute takes at least 1 argument}}
 void *test_no_fn_proto() __attribute__((assume_aligned())); // expected-error {{'assume_aligned' attribute takes at least 1 argument}}
 void *test_no_fn_proto() __attribute__((assume_aligned(32, 45, 37))); // expected-error {{'assume_aligned' attribute takes no more than 2 arguments}}