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