diff mbox series

AArch64: Add UNSPECV_PATCHABLE_AREA [PR98776]

Message ID 3b2be13be3534681af5a64b8163a3c8c@amazon.com
State Accepted
Headers
Series AArch64: Add UNSPECV_PATCHABLE_AREA [PR98776] |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Li, Pan2 via Gcc-patches Dec. 1, 2022, 3:04 a.m. UTC 
  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?

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.

Comments

Richard Sandiford Dec. 5, 2022, 11:34 a.m. UTC | #1 
"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.

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 <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" } } */
Li, Pan2 via Gcc-patches Dec. 7, 2022, 12:51 a.m. UTC | #2 
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
Richard Sandiford Dec. 7, 2022, 9:12 a.m. UTC | #3 
"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" } } */
Li, Pan2 via Gcc-patches Dec. 7, 2022, 6:46 p.m. UTC | #4 
Hi Richard,


Please find attached a patch that follows your recommendations to generate the BTI_C instructions.

Please let me know if the patch can be further improved.

The patch passed bootstrap and regressions tests on arm64-linux.


Thanks,

Sebastian
Richard Sandiford Dec. 8, 2022, 7:38 a.m. UTC | #5 
"Pop, Sebastian" <spop@amazon.com> writes:
> Hi Richard,
>
>
> Please find attached a patch that follows your recommendations to generate the BTI_C instructions.
>
> Please let me know if the patch can be further improved.
>
> The patch passed bootstrap and regressions tests on arm64-linux.

LGTM.  OK for trunk, thanks, and for release branches after a grace period.

Richard

> Thanks,
>
> Sebastian
>
> ________________________________
> From: Richard Sandiford <richard.sandiford@arm.com>
> Sent: Wednesday, December 7, 2022 3:12:08 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:
>> 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" } } */
> From 656b6906847c781caa8ee38639e0e5bbd679771b 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                | 54 +++++++++++++++-----
>  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, 70 insertions(+), 15 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..3951aa9d1a1 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);
> +      return;
> +    }
> +
> +  /* 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;
> +
> +  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");
> +      if (!insn
> +	  || !INSN_P (insn)
> +	  || GET_CODE (PATTERN (insn)) != UNSPEC_VOLATILE
> +	  || XINT (PATTERN (insn), 1) != UNSPECV_BTI_C)
> +	{
> +	  /* Emit a BTI_C.  */
> +	  insn = emit_insn_before (gen_bti_c (), BB_HEAD (bb));
> +	}
> +
> +      /* Emit the patchable_area after BTI_C.  */
> +      insn = emit_insn_after (pa, insn);
> +      INSN_ADDRESSES_NEW (insn, -1);
> +      return;
>      }
>  
> -  default_print_patchable_function_entry (file, patch_area_size, record_p);
> +  /* Emit the patchable_area at the beginning of the function.  */
> +  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)
> +{
> +  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" } } */
Li, Pan2 via Gcc-patches Dec. 15, 2022, 4:51 a.m. UTC | #6 
Hi Richard,


I will commit tomorrow the attached patches to the active branches gcc-10, 11, and 12.

The patches passed bootstrap and regression test on arm64-linux.


Sebastian
diff mbox series

Patch

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" } } */
-- 
2.37.1