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 <richard.earns...@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.
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.
[...]
+ 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.
