Re: [Patch 4/7, Arm. GCC] Implement target feature macros for PACBTI.

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

On 08/10/2021 13:18, Tejas Belagod via Gcc-patches wrote:
> Hi,
> 
> This patch implements target feature macros when PACBTI is
> enabled through the -march option or -mbranch-protection.
> 
> Tested on arm-none-eabi. OK for trunk?
> 
> 2021-10-04  Tejas Belagod  <tbelagod@arm.com>
> 
> gcc/ChangeLog:
> 
> 	* config/arm/arm-c.c (arm_cpu_builtins): Define
> 	__ARM_FEATURE_BTI_DEFAULT and __ARM_FEATURE_PAC_DEFAULT.
> 
> 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.
> 

I presume the specification for this is ACLE - please say so rather than 
making me guess.


+  cpp_undef (pfile, "__ARM_FEATURE_BTI_DEFAULT");
+  cpp_undef (pfile, "__ARM_FEATURE_PAC_DEFAULT");
+  if (TARGET_HAVE_PACBTI)
+    {
+      builtin_define_with_int_value ("__ARM_FEATURE_BTI_DEFAULT",
+				     arm_enable_pacbti & 0x1);

My reading of the ACLE specification would suggest this shouldn't be 
defined if it would have a value of 0, but that's not what this code 
does.  I think it would be better to move this outside the 
TARGET_HAVE_PACBTI and use the def_or_undef approach.

+      builtin_define_with_int_value ("__ARM_FEATURE_PAC_DEFAULT",
+				     arm_enable_pacbti >> 1);

This one is less clear, could the value ever be zero?  I guess exactly 
one of a-key and b-key must be defined and each has a separate bit.

+    }
+
+

Not more than one blank line at the end of a block.


diff --git a/gcc/testsuite/gcc.target/arm/acle/pacbti-m-predef-2.c 
b/gcc/testsuite/gcc.target/arm/acle/pacbti-m-predef-2.c


Given what I've said above, I think you need to also test that 
__ARM_FEATURE_BTI_DEFAULT is defined before testing the value (and 
emitting #error if it isn't).

R.