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 95A373858D38 for ; Wed, 11 Jan 2023 15:08:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 95A373858D38 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 4B980FEC; Wed, 11 Jan 2023 07:09:25 -0800 (PST) Received: from [10.57.37.91] (unknown [10.57.37.91]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 5C7F03F71A; Wed, 11 Jan 2023 07:08:42 -0800 (PST) Message-ID: <8eb85bd3-9d0e-2468-58a8-05399601fad0@foss.arm.com> Date: Wed, 11 Jan 2023 15:08:40 +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: [PATCH 12/15 V5] arm: implement bti injection Content-Language: en-GB To: Andrea Corallo Cc: Kyrylo Tkachov , Richard Earnshaw , nd , Andrea Corallo via Gcc-patches References: <30990d91-0c32-a3b0-7954-846bf1eddc8d@foss.arm.com> <7db3982c-cd42-023f-0dd1-0eb7b7dbfb20@foss.arm.com> From: Richard Earnshaw In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-3488.1 required=5.0 tests=BAYES_00,BODY_8BITS,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,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 22/12/2022 17:13, Andrea Corallo via Gcc-patches wrote: > Richard Earnshaw writes: > >> On 14/12/2022 17:00, Richard Earnshaw via Gcc-patches wrote: >>> On 14/12/2022 16:40, Andrea Corallo via Gcc-patches wrote: >>>> Hi Richard, >>>> >>>> thanks for reviewing. >>>> >>>> Richard Earnshaw writes: >>>> >>>>> On 28/10/2022 17:40, Andrea Corallo via Gcc-patches wrote: >>>>>> Hi all, >>>>>> please find attached the third iteration of this patch addresing >>>>>> review >>>>>> comments. >>>>>> Thanks >>>>>>     Andrea >>>>>> >>>>> >>>>> @@ -23374,12 +23374,6 @@ output_probe_stack_range (rtx reg1, rtx reg2) >>>>>     return ""; >>>>>   } >>>>> >>>>> -static bool >>>>> -aarch_bti_enabled () >>>>> -{ >>>>> -  return false; >>>>> -} >>>>> - >>>>>   /* Generate the prologue instructions for entry into an ARM or Thumb-2 >>>>>      function.  */ >>>>>   void >>>>> @@ -32992,6 +32986,61 @@ arm_current_function_pac_enabled_p (void) >>>>>                 && !crtl->is_leaf)); >>>>>   } >>>>> >>>>> +/* Return TRUE if Branch Target Identification Mechanism is >>>>> enabled.  */ >>>>> +bool >>>>> +aarch_bti_enabled (void) >>>>> +{ >>>>> +  return aarch_enable_bti == 1; >>>>> +} >>>>> >>>>> See comment in earlier patch about the location of this function >>>>> moving.   Can aarch_enable_bti take values other than 0 and 1? >>>> >>>> Yes default is 2. >>> It shouldn't be by this point, because, hopefully you've gone >>> through the equivalent of this hunk (from aarch64) somewhere in >>> arm_override_options: >>>    if (aarch_enable_bti == 2) >>>      { >>>  #ifdef TARGET_ENABLE_BTI >>>        aarch_enable_bti = 1; >>>  #else >>>        aarch_enable_bti = 0; >>>  #endif >>>      } >>> And after this point the '2' should never be seen again.  We use >>> this trick to permit the user to force a default that differs from >>> the configuration. >>> However, I don't see a hunk to do this in patch 3, so perhaps that >>> needs updating to fix this. >> >> I've just remembered that the above is to support a configure-time >> option of the compiler to enable branch protection. But perhaps we >> don't want to have that in AArch32, in which case it would be better >> not to have the default be 2 anyway, just default to off (0). >> >> R. > > Done in 1/15 (needs approval again now). > >>> >>>> [...] >>>> >>>>> +  return GET_CODE (pat) == UNSPEC_VOLATILE && XINT (pat, 1) == >>>>> UNSPEC_BTI_NOP; >>>>> >>>>> I'm not sure where this crept in, but UNSPEC and UNSPEC_VOLATILE have >>>>> separate enums in the backend, so UNSPEC_BIT_NOP should really be >>>>> VUNSPEC_BTI_NOP and defined in the enum "unspecv". >>>> >>>> Done >>>> >>>>> +aarch_pac_insn_p (rtx x) >>>>> +{ >>>>> +  if (!x || !INSN_P (x)) >>>>> +    return false; >>>>> + >>>>> +  rtx pat = PATTERN (x); >>>>> + >>>>> +  if (GET_CODE (pat) == SET) >>>>> +    { >>>>> +      rtx tmp = XEXP (pat, 1); >>>>> +      if (tmp >>>>> +      && GET_CODE (tmp) == UNSPEC >>>>> +      && (XINT (tmp, 1) == UNSPEC_PAC_NOP >>>>> +          || XINT (tmp, 1) == UNSPEC_PACBTI_NOP)) >>>>> +    return true; >>>>> +    } >>>>> + >>>>> >>>>> This will also need updating (see review on earlier patch) because >>>>> PACBTI needs to be unspec_volatile, while PAC doesn't. >>>> >>>> Done >>>> >>>>> +/* The following two functions are for code compatibility with aarch64 >>>>> +   code, this even if in arm we have only one bti instruction.  */ >>>>> + >>>>> >>>>> I'd just write >>>>>   /* Target specific mapping for aarch_gen_bti_c and >>>>>   aarch_gen_bti_j. For Arm, both of these map to a simple BTI >>>>> instruction.  */ >>>> >>>> Done >>>> >>>>> >>>>> @@ -162,6 +162,7 @@ (define_c_enum "unspec" [ >>>>>     UNSPEC_PAC_NOP    ; Represents PAC signing LR >>>>>     UNSPEC_PACBTI_NOP    ; Represents PAC signing LR + valid landing pad >>>>>     UNSPEC_AUT_NOP    ; Represents PAC verifying LR >>>>> +  UNSPEC_BTI_NOP    ; Represent BTI >>>>>   ]) >>>>> >>>>> BTI is an unspec volatile, so this should be in the "vunspec" enum and >>>>> renamed accordingly (see above). >>>> >>>> Done. >>>> >>>> Please find attached the updated version of this patch. >>>> >>>> BR >>>> >>>>    Andrea >>>> >>> Apart from that, this is OK. >>> R. > > Cool, attached the updated patch. > > Also I added some error handling not to run the bti pass if the march > selected does not support bti. > > BR > > Andrea > OK. R.