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 ↗

(On Diff #518274)

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

77 ↗

(On Diff #518274)

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

7992

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 ↗

(On Diff #518348)

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 ↗

(On Diff #518425)

clang/lib/Sema/SemaChecking.cpp

7982–7991

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

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

79 ↗

(On Diff #518348)

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

75 ↗

(On Diff #518274)

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

[barannikov88]

clang/lib/Sema/SemaChecking.cpp

7992

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–7993

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–7993

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–7993

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–7993

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/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 (!firstArgType->isAnyPointerType()) {
+      QualType expectedType = Context.getPointerType(firstArgType);
+      return Diag(FirstArg->getBeginLoc(),
+                  diag::err_typecheck_convert_incompatible)
+             << firstArgType << expectedType << 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))