Re: [GCC][PATCH v2] arm: Add pacbti related multilib support for armv8.1-m.main.

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

On 31/10/2022 15:36, Srinath Parvathaneni via Gcc-patches wrote:
> Hi,
> 
> This patch adds the support for pacbti multlilib linking by making
> "-mbranch-protection=none" as default in the command line for all M-profile
> targets and uses "-mbranch-protection=none" for multilib matching. If any
> valid value is passed to "-mbranch-protection" in the command line, this
> new value overwrites the default value in the command line and uses
> "-mbranch-protection=standard" for multilib matching.
> 
> Eg 1.
> 
> If the passed command line flags are:
> a) -march=armv8.1-m.main+mve -mfloat-abi=hard -mfpu=auto
> b) -mcpu=cortex-m85+nopacbti -mfloat-abi=hard -mfpu=auto
> 
> After this patch the command line flags the compiler receives will be:
> a) -march=armv8.1-m.main+mve -mfloat-abi=hard -mfpu=auto -mbranch-protection=none
> b) -mcpu=cortex-m85+nopacbti -mfloat-abi=hard -mfpu=auto -mbranch-protection=none
> 
> "-mbranch-protection=none" will be used in the multilib matching.
> 
> Eg 2.
> 
> If the passed command line flags are:
> a) -march=armv8.1-m.main+mve+pacbti -mfloat-abi=hard -mfpu=auto  -mbranch-protection=pac-ret
> b) -mcpu=cortex-m85 -mfloat-abi=hard -mfpu=auto  -mbranch-protection=pac-ret+bti
> 
> After this patch the command line flags the compiler receives will be:
> a) -march=armv8.1-m.main+mve+pacbti -mfloat-abi=hard -mfpu=auto -mbranch-protection=pac-ret
> b) -mcpu=cortex-m85 -mfloat-abi=hard -mfpu=auto -mbranch-protection=pac-ret+bti
> 
> "-mbranch-protection=standard" will be used in the multilib matching.
> 
> Eg 3.
> 
> For A-profile target, if the passed command line flags are:
> -march=armv8-a+simd -mfloat-abi=hard -mfpu=auto
> 
> Even after this patch the command line flags compiler receives will remain the same:
> -march=armv8-a+simd -mfloat-abi=hard -mfpu=auto
> 
> Regression tested on arm-none-eabi and bootstrapped on arm-none-linux-gnueabihf.
> 
> Ok for master?
> 
> Regards,
> Srinath.
> 
> gcc/ChangeLog:
> 
> 2022-10-28  Srinath Parvathaneni  <srinath.parvathaneni@arm.com>
> 
>          * common/config/arm/arm-common.cc
>          (arm_canon_branch_protection_option): Define new function.
>          * config/arm/arm-cpus.in (armv8.1-m.main): Move dsp option below pacbti
>          option.
>          * config/arm/arm.h (arm_canon_branch_protection_option): Define function
>          prototype.
>          (CANON_BRANCH_PROTECTION_SPEC_FUNCTION): Define macro.
>          (MBRANCH_PROTECTION_SPECS): Likewise.
>          * config/arm/t-rmprofile (MULTI_ARCH_OPTS_RM): Add new options.
>          (MULTI_ARCH_DIRS_RM): Add new directories.
>          (MULTILIB_REQUIRED): Add new option.
>          (MULTILIB_REUSE): Reuse existing multlibs.
>          (MULTILIB_MATCHES): Match multilib strings.
> 
> gcc/testsuite/ChangeLog:
> 
> 2022-10-28  Srinath Parvathaneni  <srinath.parvathaneni@arm.com>
> 
>          * gcc.target/arm/multilib.exp (multilib_config "rmprofile"): Update
>          tests.
>          * gcc.target/arm/pac-10.c: New test.
>          * gcc.target/arm/pac-11.c: Likewise.
>          * gcc.target/arm/pac-12.c: Likewise.

Your attachment this time is gzipped, which is almost as bad as 
octet-stream.  Please use text/plain attachments.

--- a/gcc/config/arm/arm-cpus.in
+++ b/gcc/config/arm/arm-cpus.in
@@ -746,8 +746,8 @@ begin arch armv8.1-m.main
   profile M
   isa ARMv8_1m_main
  # fp => FPv5-sp-d16; fp.dp => FPv5-d16
- option dsp add armv7em
   option pacbti add pacbti
+ option dsp add armv7em

Why is this needed?  It looks completely unnecessary.

+/* Automatically add -mbranch-protection=none for M-profile targets if
+   -mbranch-protection value isn't specified via the command line.  */
+#define MBRANCH_PROTECTION_SPECS				\
+  "%{!mbranch-protection=*:%:canon_branch_protection(%{march=*:arch %*;" \
+  "mcpu=*:cpu %*;:})}"
+

This doesn't canonicalize the branch-protection option, it provides a 
default if none was specified.  So if we really need this operation (see 
below) it should be renamed accordingly.

  MULTI_ARCH_OPTS_RM	+= mbranch-protection=standard
-MULTI_ARCH_DIRS_RM	+= mbranch-protection
+MULTI_ARCH_DIRS_RM	+= branch_protection_on
+MULTI_ARCH_OPTS_RM	+= mbranch-protection=none
+MULTI_ARCH_DIRS_RM	+= branch_protection_off

These options are related (you'll never need both), so it should be 
written as

  MULTI_ARCH_OPTS_RM	+= mbranch-protection=standard/mbranch-protection=none
and then add
MULTI_ARCH_DIRS_RM	+= mbranch_protection_on mbranch_protection_off

as a single line.  But I think it would be better to rename the 
directory names as "bp" and "bp_off".

Except that...

Why do we need a separate bp_off multilib at all? That's the default and 
the multilib framework can be told how to handle that ...

Firstly, create a new header, lets call it arm-mlib.h, containing

#define MULTILIB_DEFAULTS { "mbranch-protection=none" }

And then arrange to add this new header when the multilib framework is 
enabled via config.gcc.  You'll need to add this to ${tm_file} when we 
add the extra multilibs (search for "aprofile|rmprofile" in config.gcc).

Now you no longer need to handle this case at all in the multilib 
framework, since the compiler now knows that this is equivalent to 
omitting the option entirely.  That should eliminate a lot of the code 
you're adding, along with the canonicalization step.

Secondly, you appear to be missing support for the other variants of 
-mbranch-protection.  The manual says that we have
none, standard, pac-ret, standard+leaf and pac-ret+leaf and we need 
MULTILIB_MATCHES rules to reuse the standard variant for these cases.

Finally, please make sure this is tested and works as expected when you 
have both a-profile and rm-profile multilibs in the same build.

R.