Re: [PATCH 5/12 V2] arm: Implement target feature macros for PACBTI

[Richard Earnshaw <Richard.Earnshaw@foss.arm.com>]

On 12/07/2022 16:45, Andrea Corallo via Gcc-patches wrote:
> Richard Earnshaw <Richard.Earnshaw@foss.arm.com> writes:
> 
>> On 28/04/2022 10:42, Andrea Corallo via Gcc-patches wrote:
>>> This patch implements target feature macros when PACBTI is enabled
>>> through the -march option or -mbranch-protection.  The target feature
>>> macros __ARM_FEATURE_PAC_DEFAULT and __ARM_FEATURE_BTI_DEFAULT are
>>> specified in ARM ACLE
>>> <https://developer.arm.com/documentation/101028/0012/5--Feature-test-macros?lang=en>
>>> __ARM_FEATURE_PAUTH and __ARM_FEATURE_BTI are specified in the
>>> pull-request <https://github.com/ARM-software/acle/pull/55>.
>>> Approved here
>>> <https://gcc.gnu.org/pipermail/gcc-patches/2021-December/586334.html>.
>>> gcc/ChangeLog:
>>> 	* config/arm/arm-c.c (arm_cpu_builtins): Define
>>> 	__ARM_FEATURE_BTI_DEFAULT, __ARM_FEATURE_PAC_DEFAULT,
>>> 	__ARM_FEATURE_PAUTH and __ARM_FEATURE_BTI.
>>
>> This bit is OK.
>>
>>> gcc/testsuite/ChangeLog:
>>> 	* gcc.target/arm/acle/pacbti-m-predef-2.c: New test.
>>> 	* gcc.target/arm/acle/pacbti-m-predef-4.c: New test.
>>> 	* gcc.target/arm/acle/pacbti-m-predef-5.c: New test.
>>>
>>
>> These are all execution tests.  I think we also need some compile-only
>> tests so that we get better coverage when the target does not directly
>> support PACBTI.
>>
>> We also need some tests for the defines when targetting armv8-m.main
>> and some tests for checking __ARM_FEATURE_BTI and __ARM_FEATURE_PAC
>> (the tests here check only the '..._DEFAULT' macros.
> 
> Hi Richard & all,
> 
> please find attached the updated version of this patch.
> 
> Best Regards
> 
>    Andrea
> 
> gcc/ChangeLog:
> 
> 	* config/arm/arm-c.c (arm_cpu_builtins): Define
> 	__ARM_FEATURE_BTI_DEFAULT, __ARM_FEATURE_PAC_DEFAULT,
> 	__ARM_FEATURE_PAUTH and __ARM_FEATURE_BTI.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gcc.target/arm/acle/pacbti-m-predef-2.c: New test.
> 	* gcc.target/arm/acle/pacbti-m-predef-4.c: Likewise.
> 	* gcc.target/arm/acle/pacbti-m-predef-5.c: Likewise.
> 	* gcc.target/arm/acle/pacbti-m-predef-8.c: Likewise.
> 	* gcc.target/arm/acle/pacbti-m-predef-9.c: Likewise.
> 	* gcc.target/arm/acle/pacbti-m-predef-10.c: Likewise.
> 	* gcc.target/arm/acle/pacbti-m-predef-11.c: Likewise.
> 	* gcc.target/arm/acle/pacbti-m-predef-12.c: Likewise.
> 

diff --git a/gcc/testsuite/gcc.target/arm/acle/pacbti-m-predef-10.c 
b/gcc/testsuite/gcc.target/arm/acle/pacbti-m-predef-10.c
new file mode 100644
index 00000000000..311cf572dd9
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/acle/pacbti-m-predef-10.c
@@ -0,0 +1,10 @@
+/* { dg-do compile } */
+/* { dg-additional-options " -mbranch-protection=bti+pac-ret" } */

This is not enough.  For example, if the testsuite is being run with 
"-march=armv6-m" as the testrun options, we'll get an error that will 
cause a test failure.  You need to run a pre-test rule that validates 
that adding -mbranch-protection is safe.

+++ b/gcc/testsuite/gcc.target/arm/acle/pacbti-m-predef-11.c
@@ -0,0 +1,10 @@
+/* { dg-do compile } */
+/* { dg-options "-march=armv8.1-m.main+pacbti" } */

Similarly here, this would conflict with, for example, "-marm" as test 
options.

+++ b/gcc/testsuite/gcc.target/arm/acle/pacbti-m-predef-5.c
@@ -0,0 +1,24 @@
+
+/* { dg-do run } */

Blank line at the start of the test.

The other tests have similar issues.

R.