Szabolcs Nagy <szabolcs.n...@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
>
> gcc/ChangeLog:
>
> 2020-01-16 Szabolcs Nagy <szabolcs.n...@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.n...@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. */
