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.parvathan...@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.parvathan...@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.
