[clang][PAC] ptrauth_qualifier must be considered a feature #153291
Conversation
…sion Under `-pedantic-errors` extensions (as detected by __has_extension) are not enabled (or rather the tests return false). As the ptrauth qualifier impacts the ABI this is not sound, so this PR returns the ptrauth qualifier to being detected through a feature check.
ojhunt
requested review from
ahmedbougacha,
rjmccall,
cor3ntin,
AaronBallman and
ahatanak
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Oliver Hunt (ojhunt) ChangesDuring the upstreaming process the ptrauth qualifier test was changed from a feature Full diff: https://github.com/llvm/llvm-project/pull/153291.diff 4 Files Affected:
diff --git a/clang/include/clang/Basic/Features.def b/clang/include/clang/Basic/Features.def
index c58e3f2400adc..404ae9e1fca14 100644
--- a/clang/include/clang/Basic/Features.def
+++ b/clang/include/clang/Basic/Features.def
@@ -148,7 +148,7 @@ FEATURE(thread_sanitizer, LangOpts.Sanitize.has(SanitizerKind::Thread))
FEATURE(dataflow_sanitizer, LangOpts.Sanitize.has(SanitizerKind::DataFlow))
FEATURE(scudo, LangOpts.Sanitize.hasOneOf(SanitizerKind::Scudo))
FEATURE(ptrauth_intrinsics, LangOpts.PointerAuthIntrinsics)
-EXTENSION(ptrauth_qualifier, LangOpts.PointerAuthIntrinsics)
+FEATURE(ptrauth_qualifier, LangOpts.PointerAuthIntrinsics)
FEATURE(ptrauth_calls, LangOpts.PointerAuthCalls)
FEATURE(ptrauth_returns, LangOpts.PointerAuthReturns)
FEATURE(ptrauth_vtable_pointer_address_discrimination, LangOpts.PointerAuthVTPtrAddressDiscrimination)
diff --git a/clang/test/Preprocessor/ptrauth_extension.c b/clang/test/Preprocessor/ptrauth_extension.c
index d6b79187ba62d..5a0fbaa9e0e2d 100644
--- a/clang/test/Preprocessor/ptrauth_extension.c
+++ b/clang/test/Preprocessor/ptrauth_extension.c
@@ -1,10 +1,16 @@
// RUN: %clang_cc1 -E %s -triple=aarch64 -fptrauth-intrinsics | \
// RUN: FileCheck %s --check-prefixes=INTRIN
+// RUN: %clang_cc1 -E %s -triple=aarch64 -fptrauth-intrinsics -pedantic-errors | \
+// RUN: FileCheck %s --check-prefixes=INTRIN
+
// RUN: %clang_cc1 -E %s -triple=aarch64 -fptrauth-calls | \
// RUN: FileCheck %s --check-prefixes=NOINTRIN
-#if __has_extension(ptrauth_qualifier)
+// RUN: %clang_cc1 -E %s -triple=aarch64 -fptrauth-calls -pedantic-errors | \
+// RUN: FileCheck %s --check-prefixes=NOINTRIN
+
+#if __has_feature(ptrauth_qualifier)
// INTRIN: has_ptrauth_qualifier
void has_ptrauth_qualifier() {}
#else
diff --git a/clang/test/Sema/ptrauth-qualifier.c b/clang/test/Sema/ptrauth-qualifier.c
index 5d932b724f07a..18fada51bcbbe 100644
--- a/clang/test/Sema/ptrauth-qualifier.c
+++ b/clang/test/Sema/ptrauth-qualifier.c
@@ -1,7 +1,8 @@
// RUN: %clang_cc1 -triple arm64-apple-ios -std=c23 -fsyntax-only -verify -fptrauth-intrinsics %s
// RUN: %clang_cc1 -triple aarch64-linux-gnu -std=c23 -fsyntax-only -verify -fptrauth-intrinsics %s
+// RUN: %clang_cc1 -triple aarch64-linux-gnu -std=c23 -fsyntax-only -verify -fptrauth-intrinsics -pedantic-errors -DPEDANTIC_ERRORS %s
-#if !__has_extension(ptrauth_qualifier)
+#if !__has_feature(ptrauth_qualifier)
// This error means that the __ptrauth qualifier availability test says that it
// is not available. This error is not expected in the output, if it is seen
// there is a feature detection regression.
@@ -66,6 +67,7 @@ intp redeclaration3 = 0; // expected-error{{redefinition o
void illegal0(intp __ptrauth(VALID_DATA_KEY)); // expected-error {{parameter type may not be qualified with '__ptrauth'; type is '__ptrauth(2,0,0) intp' (aka 'int *__ptrauth(2,0,0)')}}
intp __ptrauth(VALID_DATA_KEY) illegal1(void); // expected-error {{return type may not be qualified with '__ptrauth'; type is '__ptrauth(2,0,0) intp' (aka 'int *__ptrauth(2,0,0)')}}
+#ifndef PEDANTIC_ERRORS
static_assert(_Generic(typeof(valid0), int * __ptrauth(VALID_DATA_KEY) : 1, int * : 0, default : 0));
static_assert(_Generic(typeof(valid0), int * __ptrauth(VALID_CODE_KEY) : 0, default : 1));
static_assert(_Generic(typeof_unqual(valid0), int * __ptrauth(VALID_DATA_KEY) : 0, int * : 1, default : 0));
@@ -73,6 +75,7 @@ static_assert(_Generic(valid0, int * __ptrauth(VALID_DATA_KEY) : 0, int * : 1, d
static_assert(_Generic(array0, int * __ptrauth(VALID_DATA_KEY) * : 1, default : 0));
static_assert(_Generic(*array1, int * : 1, default : 0));
+#endif
void test_code(intp p) {
p = (intp __ptrauth(VALID_DATA_KEY)) 0; // expected-error {{cannot cast to '__ptrauth'-qualified type '__ptrauth(2,0,0) intp' (aka 'int *__ptrauth(2,0,0)')}}
@@ -99,8 +102,10 @@ void test_array(void) {
__attribute__((overloadable)) int overload_func(int **);
__attribute__((overloadable)) float overload_func(int * __ptrauth(VALID_DATA_KEY) *);
+#ifndef PEDANTIC_ERRORS
static_assert(_Generic(typeof(overload_func(&ptr0)), int : 1, default : 0));
static_assert(_Generic(typeof(overload_func(&valid0)), float : 1, default : 0));
+#endif
void func(int array[__ptrauth(VALID_DATA_KEY) 10]); // expected-error {{'__ptrauth' qualifier only applies to pointer or pointer sized integer types; 'int[10]' is invalid}}
diff --git a/clang/test/SemaObjC/ptrauth-qualifier.m b/clang/test/SemaObjC/ptrauth-qualifier.m
index 74bbe6f09899b..bd64dc85d21a5 100644
--- a/clang/test/SemaObjC/ptrauth-qualifier.m
+++ b/clang/test/SemaObjC/ptrauth-qualifier.m
@@ -1,7 +1,8 @@
// RUN: %clang_cc1 -triple arm64-apple-ios -fsyntax-only -verify -fptrauth-intrinsics %s
// RUN: %clang_cc1 -triple aarch64-linux-gnu -fsyntax-only -verify -fptrauth-intrinsics %s
+// RUN: %clang_cc1 -triple aarch64-linux-gnu -fsyntax-only -verify -fptrauth-intrinsics -pedantic-errors %s
-#if !__has_extension(ptrauth_qualifier)
+#if !__has_feature(ptrauth_qualifier)
// This error means that the __ptrauth qualifier availability test says that it
// is not available. This error is not expected in the output, if it is seen
// there is a feature detection regression.
|
Also update to __has_feature, though that is blocked on #153291
There was a problem hiding this comment.
No, ptrauth is not a feature, it is an extension and should be reported as such. The real bug here is that you're missing an extension warning when __ptrauth is used: https://godbolt.org/z/4Mb8f4W94
That code needs to issue an extension warning.
That means all code targeting arm64e/armv8.3 would produce this warning. Pointer authentication is platform ABI, so this warning would be present in every build (if nothing else due to system headers) and as a result would be disabled anyway, rendering it moot. Then because I understand that the semantics of The ODR violation argument doesn't really help: by virtue of breaking the ability to detect support for the qualifier code cannot match the ABI without requiring an external check followed by a macro define that they can test on instead, but that is not an option for the ABI definitions as a system headers can't rely on that, so -pedantic-errors breaks the ABI and causes ODR violations. i.e by requiring the ptrauth qualifier be an extension:
That would be the behavior in the llvm.org tree, every platform that supports pointer auth would in all likelihood patch this into a feature to avoid these problems: breaking code that uses -pedantic-errors isn't acceptable, and neither is producing warnings in all code targeting that platform, especially given those warnings would quite literally be a warning about the target: user chooses to target a platform that requires pointer authentication, code compiles and produces a warning that they are targeting a platform that uses pointer authentication. All code compiling with -Werror then also needs to add -Wno- to disable the warning that they are compiling to the target they have chosen to target. |
I thought our general policy was that we don't trigger pedantic warnings for reserved keywords (like anything with a |
the problem is that The more I think about this the more weird the behavior is - you can still use the extensions (I think you can simply suppress the warnings for the cases where extensions do actually produce them), you just can't check for them :-/ An alternative approach - that would require significant code updates - would be to blast down a pile of implicit macros for all the current feature tests, which would at least provide a non- Failing that we could add a flag to the internal extension model so that some extensions will return the correct value even if -pedantic-errors is set. |
|
Right, so in macro expansion we have // If the use of an extension results in an error diagnostic, extensions are
// effectively unavailable, so just return false here.
if (PP.getDiagnostics().getExtensionHandlingBehavior() >=
diag::Severity::Error)
return false;But if you have |
|
*that said this behavior does match gcc's :-/ |
|
Can we just drop the check from It looks like that check dates back to the introduction of __has_extension (d5d410f). But it's a terrible idea for a lot of reasons: it adds a semantic effect to a flag which otherwise doesn't have any semantic effects, and even for extensions that are actually extensions, there are plenty of ways to suppress the diagnostic even in (And as a practical matter, some large fraction of codebases that care about pedantic warnings probably use |
Yeah, that's what I was thinking -- it honestly feels like an optimization that may have been inherited from gcc if that came first, but the optimization is based on an assumption that all use of any extensions will produce a warning that gets promoted to an error which is not accurate. I would be concerned about changing this specific behavior for llvm21 though, but I have an idea for a more targeted change |
|
@efriedma-quic @AaronBallman I just posted a proof of concept version of a restricted " (Side note, it might be nice to be able to create a way to deprecate __has_feature tests so that code using __has_feature can be warned and migrate to __has_extension) |
During the upstreaming process the ptrauth qualifier test was changed from a feature
test to an extension test. Unfortunately under
pedantic-errorsthis results in theptrauth qualifier being reported as disabled. As the ptrauth qualifier impacts the ABI
this is not sound, so this PR returns the ptrauth qualifier to being detected through a
feature check.