| Message ID | LV2PR01MB783923BF951C8BDF5B3D5BA9F70BA@LV2PR01MB7839.prod.exchangelabs.com |
|---|---|
| State | New |
| Headers | show |
| Series | arm/aarch64: Add bti for all functions [PR106671] | expand |
Feng Xue OS via Gcc-patches <gcc-patches@gcc.gnu.org> writes: > This patch extends option -mbranch-protection=bti with an optional argument > as bti[+all] to force compiler to unconditionally insert bti for all > functions. Because a direct function call at the stage of compiling might be > rewritten to an indirect call with some kind of linker-generated thunk stub > as invocation relay for some reasons. One instance is if a direct callee is > placed far from its caller, direct BL {imm} instruction could not represent > the distance, so indirect BLR {reg} should be used. For this case, a bti is > required at the beginning of the callee. > > caller() { > bl callee > } > > => > > caller() { > adrp reg, <callee> > add reg, reg, #constant > blr reg > } > > Although the issue could be fixed with a pretty new version of ld, here we > provide another means for user who has to rely on the old ld or other non-ld > linker. I also checked LLVM, by default, it implements bti just as the proposed > -mbranch-protection=bti+all. > > Feng > > --- > gcc/config/aarch64/aarch64.cc | 12 +++++++----- > gcc/config/aarch64/aarch64.opt | 2 +- > gcc/config/arm/aarch-bti-insert.cc | 3 ++- > gcc/config/arm/aarch-common.cc | 22 ++++++++++++++++++---- > gcc/config/arm/aarch-common.h | 18 ++++++++++++++++++ > gcc/config/arm/arm.cc | 4 ++-- > gcc/config/arm/arm.opt | 2 +- > gcc/doc/invoke.texi | 16 ++++++++++------ > gcc/testsuite/gcc.target/aarch64/bti-5.c | 17 +++++++++++++++++ > 9 files changed, 76 insertions(+), 20 deletions(-) > create mode 100644 gcc/testsuite/gcc.target/aarch64/bti-5.c [...] Hi Feng, I think this patch is missing its ChangeLog entry. Also you should specify the state of the testing and regression for this patch, please see [1]. > diff --git a/gcc/testsuite/gcc.target/aarch64/bti-5.c b/gcc/testsuite/gcc.target/aarch64/bti-5.c > new file mode 100644 > index 00000000000..654cd0cce7e > --- /dev/null > +++ b/gcc/testsuite/gcc.target/aarch64/bti-5.c > @@ -0,0 +1,17 @@ > +/* { dg-do run } */ > +/* { dg-options "-O1 -save-temps" } */ > +/* { dg-require-effective-target lp64 } */ > +/* { dg-additional-options "-mbranch-protection=bti+all" { target { ! default_branch_protection } } } */ I see the other bti execution tests we have require "aarch64_bti_hw" as effective target, do you think here is not necessary? If yes why? Thanks Andrea [1] <https://gcc.gnu.org/contribute.html#patches>
Andrea Corallo <andrea.corallo@arm.com> writes: > Feng Xue OS via Gcc-patches <gcc-patches@gcc.gnu.org> writes: > >> This patch extends option -mbranch-protection=bti with an optional argument >> as bti[+all] to force compiler to unconditionally insert bti for all >> functions. Because a direct function call at the stage of compiling might be >> rewritten to an indirect call with some kind of linker-generated thunk stub >> as invocation relay for some reasons. One instance is if a direct callee is >> placed far from its caller, direct BL {imm} instruction could not represent >> the distance, so indirect BLR {reg} should be used. For this case, a bti is >> required at the beginning of the callee. >> >> caller() { >> bl callee >> } >> >> => >> >> caller() { >> adrp reg, <callee> >> add reg, reg, #constant >> blr reg >> } >> >> Although the issue could be fixed with a pretty new version of ld, here we >> provide another means for user who has to rely on the old ld or other non-ld >> linker. I also checked LLVM, by default, it implements bti just as the proposed >> -mbranch-protection=bti+all. >> >> Feng >> >> --- >> gcc/config/aarch64/aarch64.cc | 12 +++++++----- >> gcc/config/aarch64/aarch64.opt | 2 +- >> gcc/config/arm/aarch-bti-insert.cc | 3 ++- >> gcc/config/arm/aarch-common.cc | 22 ++++++++++++++++++---- >> gcc/config/arm/aarch-common.h | 18 ++++++++++++++++++ >> gcc/config/arm/arm.cc | 4 ++-- >> gcc/config/arm/arm.opt | 2 +- >> gcc/doc/invoke.texi | 16 ++++++++++------ >> gcc/testsuite/gcc.target/aarch64/bti-5.c | 17 +++++++++++++++++ >> 9 files changed, 76 insertions(+), 20 deletions(-) >> create mode 100644 gcc/testsuite/gcc.target/aarch64/bti-5.c > > [...] > > Hi Feng, > > I think this patch is missing its ChangeLog entry. Also you should > specify the state of the testing and regression for this patch, please > see [1]. > >> diff --git a/gcc/testsuite/gcc.target/aarch64/bti-5.c b/gcc/testsuite/gcc.target/aarch64/bti-5.c >> new file mode 100644 >> index 00000000000..654cd0cce7e >> --- /dev/null >> +++ b/gcc/testsuite/gcc.target/aarch64/bti-5.c >> @@ -0,0 +1,17 @@ >> +/* { dg-do run } */ >> +/* { dg-options "-O1 -save-temps" } */ >> +/* { dg-require-effective-target lp64 } */ >> +/* { dg-additional-options "-mbranch-protection=bti+all" { target { ! default_branch_protection } } } */ Also an afterthought: given the patch is enabling this feature on arm as well wouldn't be better to have a test case for arm as well? Thanks Andrea
Hi Feng, > -----Original Message----- > From: Gcc-patches <gcc-patches- > bounces+kyrylo.tkachov=arm.com@gcc.gnu.org> On Behalf Of Feng Xue OS > via Gcc-patches > Sent: Wednesday, August 2, 2023 4:49 PM > To: gcc-patches@gcc.gnu.org > Subject: [PATCH] arm/aarch64: Add bti for all functions [PR106671] > > This patch extends option -mbranch-protection=bti with an optional > argument > as bti[+all] to force compiler to unconditionally insert bti for all > functions. Because a direct function call at the stage of compiling might be > rewritten to an indirect call with some kind of linker-generated thunk stub > as invocation relay for some reasons. One instance is if a direct callee is > placed far from its caller, direct BL {imm} instruction could not represent > the distance, so indirect BLR {reg} should be used. For this case, a bti is > required at the beginning of the callee. > > caller() { > bl callee > } > > => > > caller() { > adrp reg, <callee> > add reg, reg, #constant > blr reg > } > > Although the issue could be fixed with a pretty new version of ld, here we > provide another means for user who has to rely on the old ld or other non-ld > linker. I also checked LLVM, by default, it implements bti just as the proposed > -mbranch-protection=bti+all. Apologies for the delay, we had discussed this on and off internally over time. I don't think adding extra complexity in the compiler going forward for the sake of older linkers is a good tradeoffs. So I'd like to avoid this. Thanks, Kyrill > > Feng > > --- > gcc/config/aarch64/aarch64.cc | 12 +++++++----- > gcc/config/aarch64/aarch64.opt | 2 +- > gcc/config/arm/aarch-bti-insert.cc | 3 ++- > gcc/config/arm/aarch-common.cc | 22 ++++++++++++++++++---- > gcc/config/arm/aarch-common.h | 18 ++++++++++++++++++ > gcc/config/arm/arm.cc | 4 ++-- > gcc/config/arm/arm.opt | 2 +- > gcc/doc/invoke.texi | 16 ++++++++++------ > gcc/testsuite/gcc.target/aarch64/bti-5.c | 17 +++++++++++++++++ > 9 files changed, 76 insertions(+), 20 deletions(-) > create mode 100644 gcc/testsuite/gcc.target/aarch64/bti-5.c > > diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc > index 71215ef9fee..a404447c8d0 100644 > --- a/gcc/config/aarch64/aarch64.cc > +++ b/gcc/config/aarch64/aarch64.cc > @@ -8997,7 +8997,8 @@ void aarch_bti_arch_check (void) > bool > aarch_bti_enabled (void) > { > - return (aarch_enable_bti == 1); > + gcc_checking_assert (aarch_enable_bti != AARCH_BTI_FUNCTION_UNSET); > + return (aarch_enable_bti != AARCH_BTI_FUNCTION_NONE); > } > > /* Check if INSN is a BTI J insn. */ > @@ -18454,12 +18455,12 @@ aarch64_override_options (void) > > selected_tune = tune ? tune->ident : cpu->ident; > > - if (aarch_enable_bti == 2) > + if (aarch_enable_bti == AARCH_BTI_FUNCTION_UNSET) > { > #ifdef TARGET_ENABLE_BTI > - aarch_enable_bti = 1; > + aarch_enable_bti = AARCH_BTI_FUNCTION; > #else > - aarch_enable_bti = 0; > + aarch_enable_bti = AARCH_BTI_FUNCTION_NONE; > #endif > } > > @@ -22881,7 +22882,8 @@ aarch64_print_patchable_function_entry (FILE > *file, > basic_block bb = ENTRY_BLOCK_PTR_FOR_FN (cfun)->next_bb; > > if (!aarch_bti_enabled () > - || cgraph_node::get (cfun->decl)->only_called_directly_p ()) > + || (aarch_enable_bti != AARCH_BTI_FUNCTION_ALL > + && cgraph_node::get (cfun->decl)->only_called_directly_p ())) > { > /* Emit the patchable_area at the beginning of the function. */ > rtx_insn *insn = emit_insn_before (pa, BB_HEAD (bb)); > diff --git a/gcc/config/aarch64/aarch64.opt b/gcc/config/aarch64/aarch64.opt > index 025e52d40e5..5571f7e916d 100644 > --- a/gcc/config/aarch64/aarch64.opt > +++ b/gcc/config/aarch64/aarch64.opt > @@ -37,7 +37,7 @@ TargetVariable > aarch64_feature_flags aarch64_isa_flags = 0 > > TargetVariable > -unsigned aarch_enable_bti = 2 > +enum aarch_bti_function_type aarch_enable_bti = > AARCH_BTI_FUNCTION_UNSET > > TargetVariable > enum aarch_key_type aarch_ra_sign_key = AARCH_KEY_A > diff --git a/gcc/config/arm/aarch-bti-insert.cc b/gcc/config/arm/aarch-bti- > insert.cc > index 71a77e29406..babd2490c9f 100644 > --- a/gcc/config/arm/aarch-bti-insert.cc > +++ b/gcc/config/arm/aarch-bti-insert.cc > @@ -164,7 +164,8 @@ rest_of_insert_bti (void) > functions that are already protected by Return Address Signing (PACIASP/ > PACIBSP). For all other cases insert a BTI C at the beginning of the > function. */ > - if (!cgraph_node::get (cfun->decl)->only_called_directly_p ()) > + if (aarch_enable_bti == AARCH_BTI_FUNCTION_ALL > + || !cgraph_node::get (cfun->decl)->only_called_directly_p ()) > { > bb = ENTRY_BLOCK_PTR_FOR_FN (cfun)->next_bb; > insn = BB_HEAD (bb); > diff --git a/gcc/config/arm/aarch-common.cc b/gcc/config/arm/aarch- > common.cc > index 5b96ff4c2e8..7751d40f909 100644 > --- a/gcc/config/arm/aarch-common.cc > +++ b/gcc/config/arm/aarch-common.cc > @@ -666,7 +666,7 @@ static enum aarch_parse_opt_result > aarch_handle_no_branch_protection (char* str, char* rest) > { > aarch_ra_sign_scope = AARCH_FUNCTION_NONE; > - aarch_enable_bti = 0; > + aarch_enable_bti = AARCH_BTI_FUNCTION_NONE; > if (rest) > { > error ("unexpected %<%s%> after %<%s%>", rest, str); > @@ -680,7 +680,7 @@ aarch_handle_standard_branch_protection (char* str, > char* rest) > { > aarch_ra_sign_scope = AARCH_FUNCTION_NON_LEAF; > aarch_ra_sign_key = AARCH_KEY_A; > - aarch_enable_bti = 1; > + aarch_enable_bti = AARCH_BTI_FUNCTION; > if (rest) > { > error ("unexpected %<%s%> after %<%s%>", rest, str); > @@ -718,7 +718,15 @@ static enum aarch_parse_opt_result > aarch_handle_bti_protection (char* str ATTRIBUTE_UNUSED, > char* rest ATTRIBUTE_UNUSED) > { > - aarch_enable_bti = 1; > + aarch_enable_bti = AARCH_BTI_FUNCTION; > + return AARCH_PARSE_OK; > +} > + > +static enum aarch_parse_opt_result > +aarch_handle_bti_all (char* str ATTRIBUTE_UNUSED, > + char* rest ATTRIBUTE_UNUSED) > +{ > + aarch_enable_bti = AARCH_BTI_FUNCTION_ALL; > return AARCH_PARSE_OK; > } > > @@ -728,12 +736,18 @@ static const struct aarch_branch_protect_type > aarch_pac_ret_subtypes[] = { > { NULL, NULL, NULL, 0 } > }; > > +static const struct aarch_branch_protect_type aarch_bti_subtypes[] = { > + { "all", aarch_handle_bti_all, NULL, 0 }, > + { NULL, NULL, NULL, 0 } > +}; > + > static const struct aarch_branch_protect_type aarch_branch_protect_types[] > = { > { "none", aarch_handle_no_branch_protection, NULL, 0 }, > { "standard", aarch_handle_standard_branch_protection, NULL, 0 }, > { "pac-ret", aarch_handle_pac_ret_protection, aarch_pac_ret_subtypes, > ARRAY_SIZE (aarch_pac_ret_subtypes) }, > - { "bti", aarch_handle_bti_protection, NULL, 0 }, > + { "bti", aarch_handle_bti_protection, aarch_bti_subtypes, > + ARRAY_SIZE (aarch_bti_subtypes) }, > { NULL, NULL, NULL, 0 } > }; > > diff --git a/gcc/config/arm/aarch-common.h b/gcc/config/arm/aarch- > common.h > index c6a67f0d05c..c90b9120102 100644 > --- a/gcc/config/arm/aarch-common.h > +++ b/gcc/config/arm/aarch-common.h > @@ -50,6 +50,24 @@ enum aarch_key_type { > AARCH_KEY_B > }; > > +/* Function types to insert bti. */ > +enum aarch_bti_function_type { > + AARCH_BTI_FUNCTION_UNSET = -1u, > + /* Don't add bti to any function. */ > + AARCH_BTI_FUNCTION_NONE = 0, > + /* Add bti to function that may be indirect call target. The judgement is > + only made based on the information that compiler could get. However, > + at linking stage, a direct call might be rewritten to indirect call with > + some kind of linker-generated thunk stub as invocation relay for some > + reasons. One instance is if a direct callee is placed far from its > + caller, direct branch instruction could not represent the distance. > + For this case that have to rely on linker decision, bti would not be > + added. */ > + AARCH_BTI_FUNCTION, > + /* Add bti to all functions. */ > + AARCH_BTI_FUNCTION_ALL > +}; > + > struct aarch_branch_protect_type > { > /* The type's name that the user passes to the branch-protection option > diff --git a/gcc/config/arm/arm.cc b/gcc/config/arm/arm.cc > index c3e731b8982..3b643438195 100644 > --- a/gcc/config/arm/arm.cc > +++ b/gcc/config/arm/arm.cc > @@ -28616,7 +28616,7 @@ arm_file_start (void) > { > int val; > bool pac = (aarch_ra_sign_scope != AARCH_FUNCTION_NONE); > - bool bti = (aarch_enable_bti == 1); > + bool bti = (aarch_enable_bti != AARCH_BTI_FUNCTION_NONE); > > arm_print_asm_arch_directives > (asm_out_file, TREE_TARGET_OPTION (target_option_default_node)); > @@ -33238,7 +33238,7 @@ void aarch_bti_arch_check (void) > bool > aarch_bti_enabled (void) > { > - return aarch_enable_bti != 0; > + return aarch_enable_bti != AARCH_BTI_FUNCTION_NONE; > } > > /* Check if INSN is a BTI J insn. */ > diff --git a/gcc/config/arm/arm.opt b/gcc/config/arm/arm.opt > index 3a49b51ece0..11b987e5891 100644 > --- a/gcc/config/arm/arm.opt > +++ b/gcc/config/arm/arm.opt > @@ -28,7 +28,7 @@ TargetVariable > enum aarch_function_type aarch_ra_sign_scope = AARCH_FUNCTION_NONE > > TargetVariable > -unsigned aarch_enable_bti = 0 > +enum aarch_bti_function_type aarch_enable_bti = > AARCH_BTI_FUNCTION_NONE > > TargetVariable > enum aarch_key_type aarch_ra_sign_key = AARCH_KEY_A > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi > index 875d1d08b16..87afd411c56 100644 > --- a/gcc/doc/invoke.texi > +++ b/gcc/doc/invoke.texi > @@ -778,7 +778,7 @@ Objective-C and Objective-C++ Dialects}. > -mpc-relative-literal-loads > -msign-return-address=@var{scope} > -mbranch-protection=@var{none}|@var{standard}|@var{pac- > ret}[+@var{leaf} > -+@var{b-key}]|@var{bti} > ++@var{b-key}]|@var{bti}[+@var{all}] > -mharden-sls=@var{opts} > -march=@var{name} -mcpu=@var{name} -mtune=@var{name} > -moverride=@var{string} -mverbose-cost-dump > @@ -858,7 +858,7 @@ Objective-C and Objective-C++ Dialects}. > -mstack-protector-guard=@var{guard} -mstack-protector-guard- > offset=@var{offset} > -mfdpic > -mbranch-protection=@var{none}|@var{standard}|@var{pac- > ret}[+@var{leaf}] > -[+@var{bti}]|@var{bti}[+@var{pac-ret}[+@var{leaf}]]} > +[+@var{bti}[+@var{all}]]|@var{bti}[+@var{all}][+@var{pac- > ret}[+@var{leaf}]]} > > @emph{AVR Options} > @gccoptlist{-mmcu=@var{mcu} -mabsdata -maccumulate-args > @@ -20560,7 +20560,7 @@ default value is @samp{none}. This option has > been deprecated by > -mbranch-protection. > > @opindex mbranch-protection > -@item -mbranch-protection=@var{none}|@var{standard}|@var{pac- > ret}[+@var{leaf}+@var{b-key}]|@var{bti} > +@item -mbranch-protection=@var{none}|@var{standard}|@var{pac- > ret}[+@var{leaf}+@var{b-key}]|@var{bti}[+@var{all}] > Select the branch protection features to use. > @samp{none} is the default and turns off all types of branch protection. > @samp{standard} turns on all types of branch protection features. If a > feature > @@ -20572,7 +20572,10 @@ functions will practically always do this) using > the a-key. The optional > argument @samp{leaf} can be used to extend the signing to include leaf > functions. The optional argument @samp{b-key} can be used to sign the > functions > with the B-key instead of the A-key. > -@samp{bti} turns on branch target identification mechanism. > +@samp{bti}[+@var{all}] turns on branch target identification mechanism. If > +the optional argument @samp{all} is specified, bti-c instruction will be > +unconditionally added to the beginning of all functions, otherwise, only > +added to function that is supposed to be indirect call target by compiler. > > @opindex mharden-sls > @item -mharden-sls=@var{opts} > @@ -22836,7 +22839,7 @@ build the Linux kernel using the same > (@code{arm-*-uclinuxfdpiceabi}) > toolchain as the one used to build the userland programs. > > @opindex mbranch-protection > -@item -mbranch-protection=@var{none}|@var{standard}|@var{pac- > ret}[+@var{leaf}][+@var{bti}]|@var{bti}[+@var{pac-ret}[+@var{leaf}]] > +@item -mbranch-protection=@var{none}|@var{standard}|@var{pac- > ret}[+@var{leaf}][+@var{bti}[+@var{all}]]|@var{bti}[+@var{all}][+@var{pac- > ret}[+@var{leaf}]] > Enable branch protection features (armv8.1-m.main only). > @samp{none} generate code without branch protection or return address > signing. > @@ -22848,7 +22851,8 @@ the return address to memory. > @samp{leaf} When return address signing is enabled, also sign leaf > functions even if they do not write the return address to memory. > +@samp{bti} Add landing-pad instructions at the permitted targets of > -indirect branch instructions. > +indirect branch instructions. If the optional argument @samp{all} is > +specified, the instruction will be unconditionally added to all functions. > > If the @samp{+pacbti} architecture extension is not enabled, then all > branch protection and return address signing operations are > diff --git a/gcc/testsuite/gcc.target/aarch64/bti-5.c > b/gcc/testsuite/gcc.target/aarch64/bti-5.c > new file mode 100644 > index 00000000000..654cd0cce7e > --- /dev/null > +++ b/gcc/testsuite/gcc.target/aarch64/bti-5.c > @@ -0,0 +1,17 @@ > +/* { dg-do run } */ > +/* { dg-options "-O1 -save-temps" } */ > +/* { dg-require-effective-target lp64 } */ > +/* { dg-additional-options "-mbranch-protection=bti+all" { target { ! > default_branch_protection } } } */ > + > +static int __attribute__((noinline)) > +func(int w) { > + return 37; > +} > + > +int __attribute__((section(".main.text"))) > +main(int argc, char **argv) > +{ > + return func(argc) == 37 ? 0 : 1; > +} > + > +/* { dg-final { scan-assembler-times "hint\t34" 2 } } */ > -- > 2.17.1
diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc index 71215ef9fee..a404447c8d0 100644 --- a/gcc/config/aarch64/aarch64.cc +++ b/gcc/config/aarch64/aarch64.cc @@ -8997,7 +8997,8 @@ void aarch_bti_arch_check (void) bool aarch_bti_enabled (void) { - return (aarch_enable_bti == 1); + gcc_checking_assert (aarch_enable_bti != AARCH_BTI_FUNCTION_UNSET); + return (aarch_enable_bti != AARCH_BTI_FUNCTION_NONE); } /* Check if INSN is a BTI J insn. */ @@ -18454,12 +18455,12 @@ aarch64_override_options (void) selected_tune = tune ? tune->ident : cpu->ident; - if (aarch_enable_bti == 2) + if (aarch_enable_bti == AARCH_BTI_FUNCTION_UNSET) { #ifdef TARGET_ENABLE_BTI - aarch_enable_bti = 1; + aarch_enable_bti = AARCH_BTI_FUNCTION; #else - aarch_enable_bti = 0; + aarch_enable_bti = AARCH_BTI_FUNCTION_NONE; #endif } @@ -22881,7 +22882,8 @@ aarch64_print_patchable_function_entry (FILE *file, basic_block bb = ENTRY_BLOCK_PTR_FOR_FN (cfun)->next_bb; if (!aarch_bti_enabled () - || cgraph_node::get (cfun->decl)->only_called_directly_p ()) + || (aarch_enable_bti != AARCH_BTI_FUNCTION_ALL + && cgraph_node::get (cfun->decl)->only_called_directly_p ())) { /* Emit the patchable_area at the beginning of the function. */ rtx_insn *insn = emit_insn_before (pa, BB_HEAD (bb)); diff --git a/gcc/config/aarch64/aarch64.opt b/gcc/config/aarch64/aarch64.opt index 025e52d40e5..5571f7e916d 100644 --- a/gcc/config/aarch64/aarch64.opt +++ b/gcc/config/aarch64/aarch64.opt @@ -37,7 +37,7 @@ TargetVariable aarch64_feature_flags aarch64_isa_flags = 0 TargetVariable -unsigned aarch_enable_bti = 2 +enum aarch_bti_function_type aarch_enable_bti = AARCH_BTI_FUNCTION_UNSET TargetVariable enum aarch_key_type aarch_ra_sign_key = AARCH_KEY_A diff --git a/gcc/config/arm/aarch-bti-insert.cc b/gcc/config/arm/aarch-bti-insert.cc index 71a77e29406..babd2490c9f 100644 --- a/gcc/config/arm/aarch-bti-insert.cc +++ b/gcc/config/arm/aarch-bti-insert.cc @@ -164,7 +164,8 @@ rest_of_insert_bti (void) functions that are already protected by Return Address Signing (PACIASP/ PACIBSP). For all other cases insert a BTI C at the beginning of the function. */ - if (!cgraph_node::get (cfun->decl)->only_called_directly_p ()) + if (aarch_enable_bti == AARCH_BTI_FUNCTION_ALL + || !cgraph_node::get (cfun->decl)->only_called_directly_p ()) { bb = ENTRY_BLOCK_PTR_FOR_FN (cfun)->next_bb; insn = BB_HEAD (bb); diff --git a/gcc/config/arm/aarch-common.cc b/gcc/config/arm/aarch-common.cc index 5b96ff4c2e8..7751d40f909 100644 --- a/gcc/config/arm/aarch-common.cc +++ b/gcc/config/arm/aarch-common.cc @@ -666,7 +666,7 @@ static enum aarch_parse_opt_result aarch_handle_no_branch_protection (char* str, char* rest) { aarch_ra_sign_scope = AARCH_FUNCTION_NONE; - aarch_enable_bti = 0; + aarch_enable_bti = AARCH_BTI_FUNCTION_NONE; if (rest) { error ("unexpected %<%s%> after %<%s%>", rest, str); @@ -680,7 +680,7 @@ aarch_handle_standard_branch_protection (char* str, char* rest) { aarch_ra_sign_scope = AARCH_FUNCTION_NON_LEAF; aarch_ra_sign_key = AARCH_KEY_A; - aarch_enable_bti = 1; + aarch_enable_bti = AARCH_BTI_FUNCTION; if (rest) { error ("unexpected %<%s%> after %<%s%>", rest, str); @@ -718,7 +718,15 @@ static enum aarch_parse_opt_result aarch_handle_bti_protection (char* str ATTRIBUTE_UNUSED, char* rest ATTRIBUTE_UNUSED) { - aarch_enable_bti = 1; + aarch_enable_bti = AARCH_BTI_FUNCTION; + return AARCH_PARSE_OK; +} + +static enum aarch_parse_opt_result +aarch_handle_bti_all (char* str ATTRIBUTE_UNUSED, + char* rest ATTRIBUTE_UNUSED) +{ + aarch_enable_bti = AARCH_BTI_FUNCTION_ALL; return AARCH_PARSE_OK; } @@ -728,12 +736,18 @@ static const struct aarch_branch_protect_type aarch_pac_ret_subtypes[] = { { NULL, NULL, NULL, 0 } }; +static const struct aarch_branch_protect_type aarch_bti_subtypes[] = { + { "all", aarch_handle_bti_all, NULL, 0 }, + { NULL, NULL, NULL, 0 } +}; + static const struct aarch_branch_protect_type aarch_branch_protect_types[] = { { "none", aarch_handle_no_branch_protection, NULL, 0 }, { "standard", aarch_handle_standard_branch_protection, NULL, 0 }, { "pac-ret", aarch_handle_pac_ret_protection, aarch_pac_ret_subtypes, ARRAY_SIZE (aarch_pac_ret_subtypes) }, - { "bti", aarch_handle_bti_protection, NULL, 0 }, + { "bti", aarch_handle_bti_protection, aarch_bti_subtypes, + ARRAY_SIZE (aarch_bti_subtypes) }, { NULL, NULL, NULL, 0 } }; diff --git a/gcc/config/arm/aarch-common.h b/gcc/config/arm/aarch-common.h index c6a67f0d05c..c90b9120102 100644 --- a/gcc/config/arm/aarch-common.h +++ b/gcc/config/arm/aarch-common.h @@ -50,6 +50,24 @@ enum aarch_key_type { AARCH_KEY_B }; +/* Function types to insert bti. */ +enum aarch_bti_function_type { + AARCH_BTI_FUNCTION_UNSET = -1u, + /* Don't add bti to any function. */ + AARCH_BTI_FUNCTION_NONE = 0, + /* Add bti to function that may be indirect call target. The judgement is + only made based on the information that compiler could get. However, + at linking stage, a direct call might be rewritten to indirect call with + some kind of linker-generated thunk stub as invocation relay for some + reasons. One instance is if a direct callee is placed far from its + caller, direct branch instruction could not represent the distance. + For this case that have to rely on linker decision, bti would not be + added. */ + AARCH_BTI_FUNCTION, + /* Add bti to all functions. */ + AARCH_BTI_FUNCTION_ALL +}; + struct aarch_branch_protect_type { /* The type's name that the user passes to the branch-protection option diff --git a/gcc/config/arm/arm.cc b/gcc/config/arm/arm.cc index c3e731b8982..3b643438195 100644 --- a/gcc/config/arm/arm.cc +++ b/gcc/config/arm/arm.cc @@ -28616,7 +28616,7 @@ arm_file_start (void) { int val; bool pac = (aarch_ra_sign_scope != AARCH_FUNCTION_NONE); - bool bti = (aarch_enable_bti == 1); + bool bti = (aarch_enable_bti != AARCH_BTI_FUNCTION_NONE); arm_print_asm_arch_directives (asm_out_file, TREE_TARGET_OPTION (target_option_default_node)); @@ -33238,7 +33238,7 @@ void aarch_bti_arch_check (void) bool aarch_bti_enabled (void) { - return aarch_enable_bti != 0; + return aarch_enable_bti != AARCH_BTI_FUNCTION_NONE; } /* Check if INSN is a BTI J insn. */ diff --git a/gcc/config/arm/arm.opt b/gcc/config/arm/arm.opt index 3a49b51ece0..11b987e5891 100644 --- a/gcc/config/arm/arm.opt +++ b/gcc/config/arm/arm.opt @@ -28,7 +28,7 @@ TargetVariable enum aarch_function_type aarch_ra_sign_scope = AARCH_FUNCTION_NONE TargetVariable -unsigned aarch_enable_bti = 0 +enum aarch_bti_function_type aarch_enable_bti = AARCH_BTI_FUNCTION_NONE TargetVariable enum aarch_key_type aarch_ra_sign_key = AARCH_KEY_A diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index 875d1d08b16..87afd411c56 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -778,7 +778,7 @@ Objective-C and Objective-C++ Dialects}. -mpc-relative-literal-loads -msign-return-address=@var{scope} -mbranch-protection=@var{none}|@var{standard}|@var{pac-ret}[+@var{leaf} -+@var{b-key}]|@var{bti} ++@var{b-key}]|@var{bti}[+@var{all}] -mharden-sls=@var{opts} -march=@var{name} -mcpu=@var{name} -mtune=@var{name} -moverride=@var{string} -mverbose-cost-dump @@ -858,7 +858,7 @@ Objective-C and Objective-C++ Dialects}. -mstack-protector-guard=@var{guard} -mstack-protector-guard-offset=@var{offset} -mfdpic -mbranch-protection=@var{none}|@var{standard}|@var{pac-ret}[+@var{leaf}] -[+@var{bti}]|@var{bti}[+@var{pac-ret}[+@var{leaf}]]} +[+@var{bti}[+@var{all}]]|@var{bti}[+@var{all}][+@var{pac-ret}[+@var{leaf}]]} @emph{AVR Options} @gccoptlist{-mmcu=@var{mcu} -mabsdata -maccumulate-args @@ -20560,7 +20560,7 @@ default value is @samp{none}. This option has been deprecated by -mbranch-protection. @opindex mbranch-protection -@item -mbranch-protection=@var{none}|@var{standard}|@var{pac-ret}[+@var{leaf}+@var{b-key}]|@var{bti} +@item -mbranch-protection=@var{none}|@var{standard}|@var{pac-ret}[+@var{leaf}+@var{b-key}]|@var{bti}[+@var{all}] Select the branch protection features to use. @samp{none} is the default and turns off all types of branch protection. @samp{standard} turns on all types of branch protection features. If a feature @@ -20572,7 +20572,10 @@ functions will practically always do this) using the a-key. The optional argument @samp{leaf} can be used to extend the signing to include leaf functions. The optional argument @samp{b-key} can be used to sign the functions with the B-key instead of the A-key. -@samp{bti} turns on branch target identification mechanism. +@samp{bti}[+@var{all}] turns on branch target identification mechanism. If +the optional argument @samp{all} is specified, bti-c instruction will be +unconditionally added to the beginning of all functions, otherwise, only +added to function that is supposed to be indirect call target by compiler. @opindex mharden-sls @item -mharden-sls=@var{opts} @@ -22836,7 +22839,7 @@ build the Linux kernel using the same (@code{arm-*-uclinuxfdpiceabi}) toolchain as the one used to build the userland programs. @opindex mbranch-protection -@item -mbranch-protection=@var{none}|@var{standard}|@var{pac-ret}[+@var{leaf}][+@var{bti}]|@var{bti}[+@var{pac-ret}[+@var{leaf}]] +@item -mbranch-protection=@var{none}|@var{standard}|@var{pac-ret}[+@var{leaf}][+@var{bti}[+@var{all}]]|@var{bti}[+@var{all}][+@var{pac-ret}[+@var{leaf}]] Enable branch protection features (armv8.1-m.main only). @samp{none} generate code without branch protection or return address signing. @@ -22848,7 +22851,8 @@ the return address to memory. @samp{leaf} When return address signing is enabled, also sign leaf functions even if they do not write the return address to memory. +@samp{bti} Add landing-pad instructions at the permitted targets of -indirect branch instructions. +indirect branch instructions. If the optional argument @samp{all} is +specified, the instruction will be unconditionally added to all functions. If the @samp{+pacbti} architecture extension is not enabled, then all branch protection and return address signing operations are diff --git a/gcc/testsuite/gcc.target/aarch64/bti-5.c b/gcc/testsuite/gcc.target/aarch64/bti-5.c new file mode 100644 index 00000000000..654cd0cce7e --- /dev/null +++ b/gcc/testsuite/gcc.target/aarch64/bti-5.c @@ -0,0 +1,17 @@ +/* { dg-do run } */ +/* { dg-options "-O1 -save-temps" } */ +/* { dg-require-effective-target lp64 } */ +/* { dg-additional-options "-mbranch-protection=bti+all" { target { ! default_branch_protection } } } */ + +static int __attribute__((noinline)) +func(int w) { + return 37; +} + +int __attribute__((section(".main.text"))) +main(int argc, char **argv) +{ + return func(argc) == 37 ? 0 : 1; +} + +/* { dg-final { scan-assembler-times "hint\t34" 2 } } */
This patch extends option -mbranch-protection=bti with an optional argument as bti[+all] to force compiler to unconditionally insert bti for all functions. Because a direct function call at the stage of compiling might be rewritten to an indirect call with some kind of linker-generated thunk stub as invocation relay for some reasons. One instance is if a direct callee is placed far from its caller, direct BL {imm} instruction could not represent the distance, so indirect BLR {reg} should be used. For this case, a bti is required at the beginning of the callee. caller() { bl callee } => caller() { adrp reg, <callee> add reg, reg, #constant blr reg } Although the issue could be fixed with a pretty new version of ld, here we provide another means for user who has to rely on the old ld or other non-ld linker. I also checked LLVM, by default, it implements bti just as the proposed -mbranch-protection=bti+all. Feng --- gcc/config/aarch64/aarch64.cc | 12 +++++++----- gcc/config/aarch64/aarch64.opt | 2 +- gcc/config/arm/aarch-bti-insert.cc | 3 ++- gcc/config/arm/aarch-common.cc | 22 ++++++++++++++++++---- gcc/config/arm/aarch-common.h | 18 ++++++++++++++++++ gcc/config/arm/arm.cc | 4 ++-- gcc/config/arm/arm.opt | 2 +- gcc/doc/invoke.texi | 16 ++++++++++------ gcc/testsuite/gcc.target/aarch64/bti-5.c | 17 +++++++++++++++++ 9 files changed, 76 insertions(+), 20 deletions(-) create mode 100644 gcc/testsuite/gcc.target/aarch64/bti-5.c