[clang][Sema] Diagnose passing function pointer to __builtin_assume_aligned #153552
Conversation
llvmbot added clang 
| @llvm/pr-subscribers-clang Author: Victor Chernyakin (localspook) ChangesThe first parameter of the builtin is Full diff: https://github.com/llvm/llvm-project/pull/153552.diff 3 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 116341f4b66d5..f7f26ed6062b3 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -12829,7 +12829,8 @@ def err_builtin_launder_invalid_arg : Error< "%select{non-pointer|function pointer|void pointer}0 argument to " "'__builtin_launder' is not allowed">; def err_builtin_assume_aligned_invalid_arg : Error< - "non-pointer argument to '__builtin_assume_aligned' is not allowed">; + "%select{non-pointer|function pointer}0 argument to " + "'__builtin_assume_aligned' is not allowed">; def err_builtin_is_within_lifetime_invalid_arg : Error< "%select{non-|function }0pointer argument to '__builtin_is_within_lifetime' " diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp index 9ecee18661340..d8ad9f65c1995 100644 --- a/clang/lib/Sema/SemaChecking.cpp +++ b/clang/lib/Sema/SemaChecking.cpp @@ -5660,9 +5660,10 @@ bool Sema::BuiltinAssumeAligned(CallExpr *TheCall) { { ExprResult FirstArgResult = DefaultFunctionArrayLvalueConversion(FirstArg); - if (!FirstArgResult.get()->getType()->isPointerType()) { + QualType FirstArgType = FirstArgResult.get()->getType(); + if (!FirstArgType->isObjectPointerType()) { Diag(TheCall->getBeginLoc(), diag::err_builtin_assume_aligned_invalid_arg) - << TheCall->getSourceRange(); + << FirstArgType->isFunctionPointerType() << TheCall->getSourceRange(); return true; } TheCall->setArg(0, FirstArgResult.get()); diff --git a/clang/test/Sema/builtin-assume-aligned.c b/clang/test/Sema/builtin-assume-aligned.c index 57378a3426524..c6d1197fe8ab4 100644 --- a/clang/test/Sema/builtin-assume-aligned.c +++ b/clang/test/Sema/builtin-assume-aligned.c @@ -77,18 +77,19 @@ int test14(int *a, int b) { a = (int *)__builtin_assume_aligned(b, 32); // expected-error {{non-pointer argument to '__builtin_assume_aligned' is not allowed}} } -int test15(int *b) { - int arr[3] = {1, 2, 3}; - b = (int *)__builtin_assume_aligned(arr, 32); - return b[0]; +void test15(void (*f)()) { + f = (void (*)())__builtin_assume_aligned(f, 32); // expected-error {{function pointer argument to '__builtin_assume_aligned' is not allowed}} } -int val(int x) { - return x; +void foo(); + +void test16(void (*f)()) { + f = (void (*)())__builtin_assume_aligned(foo, 32); // expected-error {{function pointer argument to '__builtin_assume_aligned' is not allowed}} } -int test16(int *b) { - b = (int *)__builtin_assume_aligned(val, 32); +int test17(int *b) { + int arr[3] = {1, 2, 3}; + b = (int *)__builtin_assume_aligned(arr, 32); return b[0]; } |
AaronBallman left a comment
AaronBallman left a comment There was a problem hiding this comment.
Thank you for working on this! However, this isn't quite the correct change IMO. GCC diagnoses function pointer to void * conversions in C++ mode in general. Clang does as well: https://godbolt.org/z/jcW718EY6
I think you should be calling checkBuiltinArgument() to initialize the parameter. That should go through the usual initialization sequence logic and emit the correct diagnostics in both C++ and C.
Also, the changes should come with a release note in clang/docs/ReleaseNotes.rst so users know about the fix.
| I've switched to |
| @@ -1,9 +0,0 @@ | |||
| // RUN: %clang_cc1 -fsyntax-only -Wno-int-conversion -triple x86_64-linux -verify %s | |||
There was a problem hiding this comment.
The pointer is now always converted to const void *, so we no longer need to disallow these integer conversions.
CC @zygoloid @vitalybuka for opinions |
AaronBallman left a comment
AaronBallman left a comment There was a problem hiding this comment.
Thanks! The changes LGTM but I'd like to hear from @zygoloid or @vitalybuka about the change in sanitizer before before we land the changes.
| It looks like precommit CI found relevant failures though: |
3 participants
The first parameter of the builtin is
const void *, and function pointers aren't convertible to that. GCC diagnoses this.