From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id 5B71A383FD78 for ; Tue, 6 Dec 2022 17:58:44 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 5B71A383FD78 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=foss.arm.com Authentication-Results: sourceware.org; spf=none smtp.mailfrom=foss.arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id B043023A; Tue, 6 Dec 2022 09:58:50 -0800 (PST) Received: from [10.2.78.76] (unknown [10.2.78.76]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 6477C3F73B; Tue, 6 Dec 2022 09:58:43 -0800 (PST) Message-ID: Date: Tue, 6 Dec 2022 17:58:42 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.2 Subject: Re: [GCC][PATCH v2] arm: Add pacbti related multilib support for armv8.1-m.main. Content-Language: en-GB To: Srinath Parvathaneni , gcc-patches@gcc.gnu.org Cc: richard.earnshaw@arm.com References: <32a435b2-b9c4-4b51-b1f0-d9f304ddf8c7@AZ-NEU-EX04.Arm.com> From: Richard Earnshaw In-Reply-To: <32a435b2-b9c4-4b51-b1f0-d9f304ddf8c7@AZ-NEU-EX04.Arm.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3489.9 required=5.0 tests=BAYES_00,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,KAM_SHORT,NICE_REPLY_A,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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 > > * 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 > > * 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.