On 2020-01-16, Richard Sandiford wrote:
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
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.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. */
