From: Richard Sandiford <richard.sandiford@arm.com>
To: "Pop\, Sebastian" <spop@amazon.com>
Cc: "gcc-patches\@gcc.gnu.org" <gcc-patches@gcc.gnu.org>,
"sebpop\@gmail.com" <sebpop@gmail.com>,
Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Subject: Re: AArch64: Add UNSPECV_PATCHABLE_AREA [PR98776]
Date: Wed, 07 Dec 2022 09:12:08 +0000 [thread overview]
Message-ID: <mptedtba87r.fsf@arm.com> (raw)
In-Reply-To: <91a2310b642145aa9039066951a5f571@amazon.com> (Sebastian Pop's message of "Wed, 7 Dec 2022 00:51:32 +0000")
"Pop, Sebastian" <spop@amazon.com> writes:
> 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.
Thanks. I still think...
> The attached patch passed bootstrap and regression test on arm64-linux.
>
> Ok to commit to gcc trunk?
>
>
> Thank you,
> Sebastian
>
> ________________________________
> From: Richard Sandiford <richard.sandiford@arm.com>
> 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" <spop@amazon.com> 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.
...this approach would be slightly cleaner though. The .hint asm string
we're emitting here is exactly the same as the one emiitted by the
original bti_c instruction. The only reason for deleting the
instruction and emitting text was because we were emitting the
patchable entry directly as text, and the BTI text had to come
before the patchable entry text.
Now that we're emitting the patchable entry via a normal instruction
(a good thing!) we can keep the preceding bti_c as a normal instruction
too. That is, I think we should use emit_insn_after to emit the entry
after the bti_c insn (if it exists) instead of before BB_HEAD.
Thanks,
Richard
>> 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 <spop@amazon.com>
>> 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" } } */
> From a26a38ef91d3cb8bd776c66be735a2ec1f46d8fb Mon Sep 17 00:00:00 2001
> From: Sebastian Pop <spop@amazon.com>
> 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.target/aarch64/pr92424-2.c: Adjust pattern.
> * gcc.target/aarch64/pr92424-3.c: Adjust pattern.
> ---
> gcc/config/aarch64/aarch64-protos.h | 2 +
> gcc/config/aarch64/aarch64.cc | 56 +++++++++++++++-----
> gcc/config/aarch64/aarch64.md | 14 +++++
> gcc/testsuite/gcc.target/aarch64/pr92424-2.c | 2 +-
> gcc/testsuite/gcc.target/aarch64/pr92424-3.c | 2 +-
> gcc/testsuite/gcc.target/aarch64/pr98776.c | 11 ++++
> 6 files changed, 71 insertions(+), 16 deletions(-)
> 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..4d25f492cfa 100644
> --- a/gcc/config/aarch64/aarch64.cc
> +++ b/gcc/config/aarch64/aarch64.cc
> @@ -22661,30 +22661,58 @@ aarch64_declare_function_name (FILE *stream, const char* name,
> cfun->machine->label_is_assembled = true;
> }
>
> -/* Implement PRINT_PATCHABLE_FUNCTION_ENTRY. Check if the patch area is after
> - the function label and emit a BTI if necessary. */
> +/* Implement PRINT_PATCHABLE_FUNCTION_ENTRY. */
>
> void
> aarch64_print_patchable_function_entry (FILE *file,
> unsigned HOST_WIDE_INT patch_area_size,
> bool record_p)
> {
> - if (cfun->machine->label_is_assembled
> - && aarch64_bti_enabled ()
> + if (!cfun->machine->label_is_assembled)
> + {
> + /* Emit the patching area before the entry label, if any. */
> + default_print_patchable_function_entry (file, patch_area_size,
> + record_p);
> + }
> + else
> + {
> + if (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);
> + }
> +
> + /* Emit a pseudo PATCHABLE_AREA instruction. The pseudo gets expanded
> + with aarch64_output_patchable_area. */
> + 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);
> + }
> +}
> +
> +/* Output patchable area. */
> +
> +void
> +aarch64_output_patchable_area (unsigned int patch_area_size, bool record_p)
> +{
> + /* Emit a BTI if necessary. */
> + if (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");
> + asm_fprintf (asm_out_file, "\thint\t34 // bti c\n");
> }
>
> - default_print_patchable_function_entry (file, patch_area_size, 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/pr92424-2.c b/gcc/testsuite/gcc.target/aarch64/pr92424-2.c
> index 12465213aef..0a79901108c 100644
> --- a/gcc/testsuite/gcc.target/aarch64/pr92424-2.c
> +++ b/gcc/testsuite/gcc.target/aarch64/pr92424-2.c
> @@ -9,4 +9,4 @@ __attribute__ ((target("branch-protection=bti"),
> f10_bti ()
> {
> }
> -/* { dg-final { scan-assembler "f10_bti:\n\thint\t34 // bti c\n.*\.LPFE0:\n\tnop\n.*\tret\n" } } */
> +/* { dg-final { scan-assembler "hint\t34 // bti c\n.*\.LPFE0:\n\tnop\n.*\tret\n" } } */
> diff --git a/gcc/testsuite/gcc.target/aarch64/pr92424-3.c b/gcc/testsuite/gcc.target/aarch64/pr92424-3.c
> index 2c6a73789f0..854bb7f9fec 100644
> --- a/gcc/testsuite/gcc.target/aarch64/pr92424-3.c
> +++ b/gcc/testsuite/gcc.target/aarch64/pr92424-3.c
> @@ -9,4 +9,4 @@ __attribute__ ((target("branch-protection=bti+pac-ret+leaf"),
> f10_pac ()
> {
> }
> -/* { dg-final { scan-assembler "f10_pac:\n\thint\t34 // bti c\n.*\.LPFE0:\n\tnop\n.*\thint\t25 // paciasp\n.*\thint\t29 // autiasp\n.*\tret\n" } } */
> +/* { dg-final { scan-assembler "hint\t34 // bti c\n.*\.LPFE0:\n\tnop\n.*\thint\t25 // paciasp\n.*\thint\t29 // autiasp\n.*\tret\n" } } */
> 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" } } */
next prev parent reply other threads:[~2022-12-07 9:12 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-01 3:04 Pop, Sebastian
2022-12-05 11:34 ` Richard Sandiford
2022-12-07 0:51 ` Pop, Sebastian
2022-12-07 9:12 ` Richard Sandiford [this message]
2022-12-07 18:46 ` Pop, Sebastian
2022-12-08 7:38 ` Richard Sandiford
2022-12-15 4:51 ` Pop, Sebastian
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=mptedtba87r.fsf@arm.com \
--to=richard.sandiford@arm.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=kyrylo.tkachov@arm.com \
--cc=sebpop@gmail.com \
--cc=spop@amazon.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).