[PATCH][AArch64] PR92424: Fix -fpatchable-function-entry=N,M with BTI

[Fāng-ruì Sòng via gcc-patches]

On 2020-01-16, Richard Sandiford wrote:
>Szabolcs Nagy <Szabolcs.Nagy@arm.com> writes:
>> this affects the linux kernel and technically a wrong code bug
>> so this fix tries to be backportable (fixing all issues with
>> -fpatchable-function-entry=N,M will likely require new option).
>
>Even for the backportable version, I think it would be better
>not to duplicate so much varasm stuff.
>
>Perhaps instead we could:
>
>(a) set a cfun->machine flag in aarch64_declare_function_name
>    to say that we've assembled the label
>
>(b) override print_patchable_function_entry so that it prints a BTI if
>    this flag is set and the usual other conditions are true
>
>As discussed off-list, it'd be good to avoid a second BTI after the
>nops if possible.  print_patchable_function_entry should be able
>to find the first instruction using get_insns and next_real_insn,
>then remove it if it turns out to be a BTI.
>
>Thanks,
>Richard

It'd be great to have some tests, e.g.

1. -fpatchable-function-entry=0 -mbranch-protection=bti
2. -fpatchable-function-entry=2 -mbranch-protection=bti

I have updated clang to emit   `.Lfunc_begin0: bti c; nop; nop` for case 2.
The __patchable_function_entries entry points to .Lfunc_begin0 (bti c).

(The change is not included in the llvm 10.0 branch.)

I think we can address all except the SHF_WRITE issue in a backward
compatible manner. For some items listed in
https://gcc.gnu.org/ml/gcc/2020-01/msg00067.html , the fixes require GNU as
(https://sourceware.org/bugzilla/show_bug.cgi?id=25380 https://sourceware.org/bugzilla/show_bug.cgi?id=25381)
and ld features (https://sourceware.org/ml/binutils/2019-11/msg00266.html).

>>
>> gcc/ChangeLog:
>>
>> 2020-01-16  Szabolcs Nagy  <szabolcs.nagy@arm.com>
>>
>> 	PR target/92424
>> 	* config/aarch64/aarch64.c (aarch64_declare_function_name): Emit BTI c
>> 	if the function label is followed by a patch area.
>>
>> From ac2a46bab60ecc80a453328b4749a951908c02c5 Mon Sep 17 00:00:00 2001
>> From: Szabolcs Nagy <szabolcs.nagy@arm.com>
>> Date: Wed, 15 Jan 2020 12:23:40 +0000
>> Subject: [PATCH] [AArch64] PR92424: Fix -fpatchable-function-entry=N,M with
>>  BTI
>>
>> This is a minimal workaround that emits another BTI after the function
>> label if that is followed by a patch area.
>>
>> So before this commit -fpatchable-function-entry=3,1 with bti generates
>>
>>     .section __patchable_function_entries
>>     .8byte .LPFE
>>     .text
>>   .LPFE:
>>     nop
>>   foo:
>>     nop
>>     nop
>>     bti c // or paciasp
>>     ...
>>
>> and after this commit
>>
>>     .section __patchable_function_entries
>>     .8byte .LPFE
>>     .text
>>   .LPFE:
>>     nop
>>   foo:
>>     bti c
>>     nop
>>     nop
>>     bti c // or paciasp
>>     ...
>>
>> There is a new bti insn in the middle of the patchable area unless M=0
>> (patch area is after the new bti) or M=N (patch area is before the
>> label, no new bti).
>>
>> Note that .cfi_startproc and the asynchronous unwind table entry label
>> comes after the patch area, but whatever effect that has on the newly
>> inserted bti c, it already affected the insns in the patch area.
>>
>> Tested on aarch64-none-linux-gnu.
>>
>> gcc/ChangeLog:
>>
>> 	PR target/92424
>> 	* config/aarch64/aarch64.c (aarch64_declare_function_name): Emit BTI c
>> 	if the function label is followed by a patch area.
>> ---
>>  gcc/config/aarch64/aarch64.c | 33 +++++++++++++++++++++++++++++++++
>>  1 file changed, 33 insertions(+)
>>
>> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
>> index 66e20becaf2..0394c274330 100644
>> --- a/gcc/config/aarch64/aarch64.c
>> +++ b/gcc/config/aarch64/aarch64.c
>> @@ -18079,6 +18079,39 @@ aarch64_declare_function_name (FILE *stream, const char* name,
>>    /* Don't forget the type directive for ELF.  */
>>    ASM_OUTPUT_TYPE_DIRECTIVE (stream, name, "function");
>>    ASM_OUTPUT_LABEL (stream, name);
>> +
>> +  if (!aarch64_bti_enabled ()
>> +      || cgraph_node::get (fndecl)->only_called_directly_p ())
>> +    return;
>> +
>> +  /* Copy logic from varasm.c assemble_start_function
>> +     to handle -fpatchable-function-entry=N,M with BTI.  */
>> +  unsigned HOST_WIDE_INT patch_area_size = function_entry_patch_area_size;
>> +  unsigned HOST_WIDE_INT patch_area_entry = function_entry_patch_area_start;
>> +
>> +  tree patchable_function_entry_attr
>> +    = lookup_attribute ("patchable_function_entry", DECL_ATTRIBUTES (fndecl));
>> +  if (patchable_function_entry_attr)
>> +    {
>> +      tree pp_val = TREE_VALUE (patchable_function_entry_attr);
>> +      tree patchable_function_entry_value1 = TREE_VALUE (pp_val);
>> +
>> +      patch_area_size = tree_to_uhwi (patchable_function_entry_value1);
>> +      patch_area_entry = 0;
>> +      if (TREE_CHAIN (pp_val) != NULL_TREE)
>> +	{
>> +	  tree patchable_function_entry_value2
>> +	    = TREE_VALUE (TREE_CHAIN (pp_val));
>> +	  patch_area_entry = tree_to_uhwi (patchable_function_entry_value2);
>> +	}
>> +    }
>> +
>> +  if (patch_area_entry > patch_area_size)
>> +    patch_area_entry = 0;
>> +
>> +  /* Emit a BTI c if a patch area comes after the function label.  */
>> +  if (patch_area_size > patch_area_entry)
>> +    asm_fprintf (stream, "\thint\t34 // bti c\n");
>>  }
>>
>>  /* Implement ASM_OUTPUT_DEF_FROM_DECLS.  Output .variant_pcs for aliases.  */