From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id 426163898C77 for ; Mon, 5 Dec 2022 11:34:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 426163898C77 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id A1AE023A; Mon, 5 Dec 2022 03:34:48 -0800 (PST) Received: from localhost (e121540-lin.manchester.arm.com [10.32.99.50]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 412DA3F73D; Mon, 5 Dec 2022 03:34:41 -0800 (PST) From: Richard Sandiford To: "Pop\, Sebastian" Mail-Followup-To: "Pop\, Sebastian" ,"gcc-patches\@gcc.gnu.org" , "sebpop\@gmail.com" , Kyrylo Tkachov , richard.sandiford@arm.com Cc: "gcc-patches\@gcc.gnu.org" , "sebpop\@gmail.com" , Kyrylo Tkachov Subject: Re: AArch64: Add UNSPECV_PATCHABLE_AREA [PR98776] References: <3b2be13be3534681af5a64b8163a3c8c@amazon.com> Date: Mon, 05 Dec 2022 11:34:40 +0000 In-Reply-To: <3b2be13be3534681af5a64b8163a3c8c@amazon.com> (Sebastian Pop's message of "Thu, 1 Dec 2022 03:04:52 +0000") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-39.2 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_DMARC_NONE,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,KAM_SHORT,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: "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" } } */