Thanks Richard for your review and for pointing out the issue with BTI. The current patch removes the existing BTI instruction, and then adds the BTI hint when expanding the patchable_area pseudo. The attached patch passed bootstrap and regression test on arm64-linux. Ok to commit to gcc trunk? Thank you, Sebastian ________________________________ From: Richard Sandiford Sent: Monday, December 5, 2022 5:34:40 AM To: Pop, Sebastian Cc: gcc-patches@gcc.gnu.org; sebpop@gmail.com; Kyrylo Tkachov Subject: RE: [EXTERNAL]AArch64: Add UNSPECV_PATCHABLE_AREA [PR98776] CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe. "Pop, Sebastian" writes: > Hi, > > Currently patchable area is at the wrong place on AArch64. It is placed > immediately after function label, before .cfi_startproc. This patch > adds UNSPECV_PATCHABLE_AREA for pseudo patchable area instruction and > modifies aarch64_print_patchable_function_entry to avoid placing > patchable area before .cfi_startproc. > > The patch passed bootstrap and regression test on aarch64-linux. > Ok to commit to trunk and backport to active release branches? Looks good, but doesn't the problem described in the PR then still apply to the BTI emitted by: if (cfun->machine->label_is_assembled && aarch64_bti_enabled () && !cgraph_node::get (cfun->decl)->only_called_directly_p ()) { /* Remove the BTI that follows the patch area and insert a new BTI before the patch area right after the function label. */ rtx_insn *insn = next_real_nondebug_insn (get_insns ()); if (insn && INSN_P (insn) && GET_CODE (PATTERN (insn)) == UNSPEC_VOLATILE && XINT (PATTERN (insn), 1) == UNSPECV_BTI_C) delete_insn (insn); asm_fprintf (file, "\thint\t34 // bti c\n"); } ? It seems like the BTI will be before the cfi_startproc and the patchable entry afterwards. I guess we should keep the BTI instruction as-is (rather than printing a .hint) and emit the new UNSPECV_PATCHABLE_AREA after the BTI rather than before it. Thanks, Richard > Thanks, > Sebastian > > gcc/ > PR target/93492 > * config/aarch64/aarch64-protos.h (aarch64_output_patchable_area): > Declared. > * config/aarch64/aarch64.cc (aarch64_print_patchable_function_entry): > Emit an UNSPECV_PATCHABLE_AREA pseudo instruction. > (aarch64_output_patchable_area): New. > * config/aarch64/aarch64.md (UNSPECV_PATCHABLE_AREA): New. > (patchable_area): Define. > > gcc/testsuite/ > PR target/93492 > * gcc.target/aarch64/pr98776.c: New. > > > From b9cf87bcdf65f515b38f1851eb95c18aaa180253 Mon Sep 17 00:00:00 2001 > From: Sebastian Pop > Date: Wed, 30 Nov 2022 19:45:24 +0000 > Subject: [PATCH] AArch64: Add UNSPECV_PATCHABLE_AREA [PR98776] > > Currently patchable area is at the wrong place on AArch64. It is placed > immediately after function label, before .cfi_startproc. This patch > adds UNSPECV_PATCHABLE_AREA for pseudo patchable area instruction and > modifies aarch64_print_patchable_function_entry to avoid placing > patchable area before .cfi_startproc. > > gcc/ > PR target/93492 > * config/aarch64/aarch64-protos.h (aarch64_output_patchable_area): > Declared. > * config/aarch64/aarch64.cc (aarch64_print_patchable_function_entry): > Emit an UNSPECV_PATCHABLE_AREA pseudo instruction. > (aarch64_output_patchable_area): New. > * config/aarch64/aarch64.md (UNSPECV_PATCHABLE_AREA): New. > (patchable_area): Define. > > gcc/testsuite/ > PR target/93492 > * gcc.target/aarch64/pr98776.c: New. > --- > gcc/config/aarch64/aarch64-protos.h | 2 ++ > gcc/config/aarch64/aarch64.cc | 24 +++++++++++++++++++++- > gcc/config/aarch64/aarch64.md | 14 +++++++++++++ > gcc/testsuite/gcc.target/aarch64/pr98776.c | 11 ++++++++++ > 4 files changed, 50 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/gcc.target/aarch64/pr98776.c > > diff --git a/gcc/config/aarch64/aarch64-protos.h b/gcc/config/aarch64/aarch64-protos.h > index 4be93c93c26..2fba24d947d 100644 > --- a/gcc/config/aarch64/aarch64-protos.h > +++ b/gcc/config/aarch64/aarch64-protos.h > @@ -1074,4 +1074,6 @@ const char *aarch64_indirect_call_asm (rtx); > extern bool aarch64_harden_sls_retbr_p (void); > extern bool aarch64_harden_sls_blr_p (void); > > +extern void aarch64_output_patchable_area (unsigned int, bool); > + > #endif /* GCC_AARCH64_PROTOS_H */ > diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc > index e97f3b32f7c..e84b33b958c 100644 > --- a/gcc/config/aarch64/aarch64.cc > +++ b/gcc/config/aarch64/aarch64.cc > @@ -22684,7 +22684,29 @@ aarch64_print_patchable_function_entry (FILE *file, > asm_fprintf (file, "\thint\t34 // bti c\n"); > } > > - default_print_patchable_function_entry (file, patch_area_size, record_p); > + if (cfun->machine->label_is_assembled) > + { > + rtx pa = gen_patchable_area (GEN_INT (patch_area_size), > + GEN_INT (record_p)); > + basic_block bb = ENTRY_BLOCK_PTR_FOR_FN (cfun)->next_bb; > + rtx_insn *insn = emit_insn_before (pa, BB_HEAD (bb)); > + INSN_ADDRESSES_NEW (insn, -1); > + } > + else > + { > + default_print_patchable_function_entry (file, patch_area_size, > + record_p); > + } > +} > + > +/* Output patchable area. */ > + > +void > +aarch64_output_patchable_area (unsigned int patch_area_size, bool record_p) > +{ > + default_print_patchable_function_entry (asm_out_file, > + patch_area_size, > + record_p); > } > > /* Implement ASM_OUTPUT_DEF_FROM_DECLS. Output .variant_pcs for aliases. */ > diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md > index 76b6898ca04..6501503eb25 100644 > --- a/gcc/config/aarch64/aarch64.md > +++ b/gcc/config/aarch64/aarch64.md > @@ -303,6 +303,7 @@ > UNSPEC_TAG_SPACE ; Translate address to MTE tag address space. > UNSPEC_LD1RO > UNSPEC_SALT_ADDR > + UNSPECV_PATCHABLE_AREA > ]) > > (define_c_enum "unspecv" [ > @@ -7821,6 +7822,19 @@ > [(set_attr "type" "ls64")] > ) > > +(define_insn "patchable_area" > + [(unspec_volatile [(match_operand 0 "const_int_operand") > + (match_operand 1 "const_int_operand")] > + UNSPECV_PATCHABLE_AREA)] > + "" > +{ > + aarch64_output_patchable_area (INTVAL (operands[0]), > + INTVAL (operands[1]) != 0); > + return ""; > +} > + [(set (attr "length") (symbol_ref "INTVAL (operands[0])"))] > +) > + > ;; AdvSIMD Stuff > (include "aarch64-simd.md") > > diff --git a/gcc/testsuite/gcc.target/aarch64/pr98776.c b/gcc/testsuite/gcc.target/aarch64/pr98776.c > new file mode 100644 > index 00000000000..b075b8f75ef > --- /dev/null > +++ b/gcc/testsuite/gcc.target/aarch64/pr98776.c > @@ -0,0 +1,11 @@ > +/* { dg-do "compile" } */ > +/* { dg-options "-O1 -fpatchable-function-entry=1 -fasynchronous-unwind-tables" } */ > + > +/* Test the placement of the .LPFE0 label. */ > + > +void > +foo (void) > +{ > +} > + > +/* { dg-final { scan-assembler "\t\.cfi_startproc\n.*\.LPFE0:\n\tnop\n\tret\n" } } */