[12/15,V4] arm: implement bti injection
Commit Message
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
From 5823333b5e4e4fe089f6865cc3e0360afd1ff168 Mon Sep 17 00:00:00 2001
From: Andrea Corallo <andrea.corallo@arm.com>
Date: Thu, 7 Apr 2022 11:51:56 +0200
Subject: [PATCH] [PATCH 12/15] arm: implement bti injection
Hi all,
this patch enables Branch Target Identification Armv8.1-M Mechanism
[1].
This is achieved by using the bti pass made common with Aarch64.
The pass iterates through the instructions and adds the necessary BTI
instructions at the beginning of every function and at every landing
pads targeted by indirect jumps.
Best Regards
Andrea
[1]
<https://community.arm.com/developer/ip-products/processors/b/processors-ip-blog/posts/armv8-1-m-pointer-authentication-and-branch-target-identification-extension>
gcc/ChangeLog
2022-04-07 Andrea Corallo <andrea.corallo@arm.com>
* config.gcc (arm*-*-*): Add 'aarch-bti-insert.o' object.
* config/arm/arm-protos.h: Update.
* config/arm/arm.cc (aarch_bti_enabled) Update.
(aarch_bti_j_insn_p, aarch_pac_insn_p, aarch_gen_bti_c)
(aarch_gen_bti_j): New functions.
* config/arm/arm.md (bti_nop): New insn.
* config/arm/t-arm (PASSES_EXTRA): Add 'arm-passes.def'.
(aarch-bti-insert.o): New target.
* config/arm/unspecs.md (VUNSPEC_BTI_NOP): New unspec.
* config/arm/aarch-bti-insert.cc (rest_of_insert_bti): Verify arch
compatibility.
* config/arm/arm-passes.def: New file.
gcc/testsuite/ChangeLog
2022-04-07 Andrea Corallo <andrea.corallo@arm.com>
* gcc.target/arm/bti-1.c: New testcase.
* gcc.target/arm/bti-2.c: Likewise.
---
gcc/config.gcc | 2 +-
gcc/config/arm/arm-passes.def | 21 ++++++++++
gcc/config/arm/arm-protos.h | 2 +
gcc/config/arm/arm.cc | 53 ++++++++++++++++++++++++-
gcc/config/arm/arm.md | 7 ++++
gcc/config/arm/t-arm | 10 +++++
gcc/config/arm/unspecs.md | 1 +
gcc/testsuite/gcc.target/arm/bti-1.c | 12 ++++++
gcc/testsuite/gcc.target/arm/bti-2.c | 58 ++++++++++++++++++++++++++++
9 files changed, 163 insertions(+), 3 deletions(-)
create mode 100644 gcc/config/arm/arm-passes.def
create mode 100644 gcc/testsuite/gcc.target/arm/bti-1.c
create mode 100644 gcc/testsuite/gcc.target/arm/bti-2.c
Comments
On 14/12/2022 16:40, Andrea Corallo via Gcc-patches wrote:
> 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.
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.
> [...]
>
>> + 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.
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.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.
>
> 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.
@@ -351,7 +351,7 @@ arc*-*-*)
;;
arm*-*-*)
cpu_type=arm
- extra_objs="arm-builtins.o arm-mve-builtins.o aarch-common.o"
+ extra_objs="arm-builtins.o arm-mve-builtins.o aarch-common.o aarch-bti-insert.o"
extra_headers="mmintrin.h arm_neon.h arm_acle.h arm_fp16.h arm_cmse.h arm_bf16.h arm_mve_types.h arm_mve.h arm_cde.h"
target_type_format_char='%'
c_target_objs="arm-c.o"
new file mode 100644
@@ -0,0 +1,21 @@
+/* Arm-specific passes declarations.
+ Copyright (C) 2022 Free Software Foundation, Inc.
+ Contributed by Arm Ltd.
+
+ This file is part of GCC.
+
+ GCC is free software; you can redistribute it and/or modify it
+ under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3, or (at your option)
+ any later version.
+
+ GCC is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with GCC; see the file COPYING3. If not see
+ <http://www.gnu.org/licenses/>. */
+
+INSERT_PASS_BEFORE (pass_shorten_branches, 1, pass_insert_bti);
@@ -24,6 +24,8 @@
#include "sbitmap.h"
+rtl_opt_pass *make_pass_insert_bti (gcc::context *ctxt);
+
extern enum unwind_info_type arm_except_unwind_info (struct gcc_options *);
extern int use_return_insn (int, rtx);
extern bool use_simple_return_p (void);
@@ -33109,12 +33109,61 @@ arm_current_function_pac_enabled_p (void)
}
/* Return TRUE if Branch Target Identification Mechanism is enabled. */
-static bool
-aarch_bti_enabled ()
+bool
+aarch_bti_enabled (void)
+{
+ return aarch_enable_bti == 1;
+}
+
+/* Check if INSN is a BTI J insn. */
+bool
+aarch_bti_j_insn_p (rtx_insn *insn)
{
+ if (!insn || !INSN_P (insn))
+ return false;
+
+ rtx pat = PATTERN (insn);
+ return GET_CODE (pat) == UNSPEC_VOLATILE && XINT (pat, 1) == VUNSPEC_BTI_NOP;
+}
+
+/* Check if X (or any sub-rtx of X) is a PACIASP/PACIBSP instruction. */
+bool
+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)
+ || (GET_CODE (tmp) == UNSPEC_VOLATILE
+ && XINT (tmp, 1) == VUNSPEC_PACBTI_NOP)))
+ return true;
+ }
+
return false;
}
+ /* Target specific mapping for aarch_gen_bti_c and aarch_gen_bti_j.
+ For Arm, both of these map to a simple BTI instruction. */
+
+rtx
+aarch_gen_bti_c (void)
+{
+ return gen_bti_nop ();
+}
+
+rtx
+aarch_gen_bti_j (void)
+{
+ return gen_bti_nop ();
+}
+
/* Implement TARGET_SCHED_CAN_SPECULATE_INSN. Return true if INSN can be
scheduled for speculative execution. Reject the long-running division
and square-root instructions. */
@@ -13001,6 +13001,13 @@ (define_insn "aut_nop"
"aut\t%|ip, %|lr, %|sp"
[(set_attr "conds" "unconditional")])
+(define_insn "bti_nop"
+ [(unspec_volatile [(const_int 0)] VUNSPEC_BTI_NOP)]
+ "arm_arch8m_main"
+ "bti"
+ [(set_attr "conds" "unconditional")
+ (set_attr "type" "nop")])
+
;; Vector bits common to IWMMXT, Neon and MVE
(include "vec-common.md")
;; Load the Intel Wireless Multimedia Extension patterns
@@ -175,3 +175,13 @@ arm-d.o: $(srcdir)/config/arm/arm-d.cc
arm-common.o: arm-cpu-cdata.h
driver-arm.o: arm-native.h
+
+PASSES_EXTRA += $(srcdir)/config/arm/arm-passes.def
+
+aarch-bti-insert.o: $(srcdir)/config/arm/aarch-bti-insert.cc \
+ $(CONFIG_H) $(SYSTEM_H) $(TM_H) $(REGS_H) insn-config.h $(RTL_BASE_H) \
+ dominance.h cfg.h cfganal.h $(BASIC_BLOCK_H) $(INSN_ATTR_H) $(RECOG_H) \
+ output.h hash-map.h $(DF_H) $(OBSTACK_H) $(TARGET_H) $(RTL_H) \
+ $(CONTEXT_H) $(TREE_PASS_H) regrename.h
+ $(COMPILER) -c $(ALL_COMPILERFLAGS) $(ALL_CPPFLAGS) $(INCLUDES) \
+ $(srcdir)/config/arm/aarch-bti-insert.cc
@@ -257,6 +257,7 @@ (define_c_enum "unspecv" [
; instruction.
VUNSPEC_PACBTI_NOP ; Represents PAC signing LR + valid landing pad
VUNSPEC_AUT_NOP ; Represents PAC verifying LR
+ VUNSPEC_BTI_NOP ; Represent BTI
])
;; Enumerators for NEON unspecs.
new file mode 100644
@@ -0,0 +1,12 @@
+/* Check that GCC does bti instruction. */
+/* { dg-do compile } */
+/* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-marm" "-mcpu=*" } } */
+/* { dg-options "-march=armv8.1-m.main -mthumb -mfloat-abi=softfp -mbranch-protection=bti --save-temps" } */
+
+int
+main (void)
+{
+ return 0;
+}
+
+/* { dg-final { scan-assembler "bti" } } */
new file mode 100644
@@ -0,0 +1,58 @@
+/* { dg-do compile } */
+/* -Os to create jump table. */
+/* { dg-options "-Os" } */
+/* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-marm" "-mcpu=*" } } */
+/* { dg-options "-march=armv8.1-m.main -mthumb -mfloat-abi=softfp -mbranch-protection=bti --save-temps" } */
+
+extern int f1 (void);
+extern int f2 (void);
+extern int f3 (void);
+extern int f4 (void);
+extern int f5 (void);
+extern int f6 (void);
+extern int f7 (void);
+extern int f8 (void);
+extern int f9 (void);
+extern int f10 (void);
+
+int (*ptr) (void);
+
+int
+f_jump_table (int y, int n)
+{
+ int i;
+ for (i = 0; i < n ;i ++)
+ {
+ switch (y)
+ {
+ case 0 : ptr = f1; break;
+ case 1 : ptr = f2; break;
+ case 2 : ptr = f3; break;
+ case 3 : ptr = f4; break;
+ case 4 : ptr = f5; break;
+ case 5 : ptr = f6; break;
+ case 6 : ptr = f7; break;
+ case 7 : ptr = f8; break;
+ case 8 : ptr = f9; break;
+ case 9 : ptr = f10; break;
+ default: break;
+ }
+ y += ptr ();
+ }
+ return (y == 0)? y+1:4;
+}
+
+int
+f_label_address ()
+{
+ static void * addr = &&lab1;
+ goto *addr;
+lab1:
+ addr = &&lab2;
+ return 1;
+lab2:
+ addr = &&lab1;
+ return 2;
+}
+
+/* { dg-final { scan-assembler-times "bti" 15 } } */