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

[Andrea Corallo <andrea.corallo@arm.com>]

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>