Re: AArch64: Add UNSPECV_PATCHABLE_AREA [PR98776]

[Richard Sandiford <richard.sandiford@arm.com>]

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