Hi Richard,

thanks for reviewing.

Richard Earnshaw <Richard.Earnshaw@foss.arm.com> 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.

[...]

> +  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