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

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

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