RE: [PATCH] arm/aarch64: Add bti for all functions [PR106671]

[Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>]

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