Yury Khrustalev <yury.khrusta...@arm.com> writes:
> From: Szabolcs Nagy <szabolcs.n...@arm.com>
>
> Tail calls of indirect_return functions from non-indirect_return
> functions are disallowed even if BTI is disabled, since the call
> site may have BTI enabled.
>
> Following x86, mismatching attribute on function pointers is not
> a type error even though this can lead to bugs.
That's no longer true (thanks!). It would be good to have a test
to verify though. E.g.:
void f(void);
void (*f_ptr)(void) __attribute__((indirect_return)) = f; // Error
void g(void) __attribute__((indirect_return));
void (*g_ptr1)(void) = g; // Error
void (*g_ptr2)(void) __attribute__((indirect_return)) = g; // OK
where the errors would be checked using dg-error.
Some very minor comments below, but it looks good otherwise:
> Needed for swapcontext within the same function when GCS is enabled.
>
> gcc/ChangeLog:
>
> * config/aarch64/aarch64.cc (aarch64_gnu_attributes): Add
> indirect_return.
> (aarch64_gen_callee_cookie): Use indirect_return attribute.
> (aarch64_callee_indirect_return): New.
> (aarch_fun_is_indirect_return): New.
> (aarch64_function_ok_for_sibcall): Disallow tail calls if caller
> is non-indirect_return but callee is indirect_return.
> (aarch64_function_arg): Add indirect_return to cookie.
> (aarch64_init_cumulative_args): Record indirect_return in
> CUMULATIVE_ARGS.
> (aarch64_comp_type_attributes): Check indirect_return attribute.
> (aarch64_output_mi_thunk): Add indirect_return to cookie.
> * config/aarch64/aarch64.h (CUMULATIVE_ARGS): Add new field
> indirect_return.
> * config/aarch64/aarch64.md (tlsdesc_small_<mode>): Update.
> * config/aarch64/aarch64-opts.h (AARCH64_NUM_ABI_ATTRIBUTES): New.
> * config/aarch64/aarch64-protos.h (aarch64_gen_callee_cookie): Update.
> * config/arm/aarch-bti-insert.cc (call_needs_bti_j): New.
> (rest_of_insert_bti): Use call_needs_bti_j.
> * config/arm/aarch-common-protos.h
> (aarch_fun_is_indirect_return): New.
> * config/arm/arm.cc
> (aarch_fun_is_indirect_return): New.
>
> Co-authored-by: Yury Khrustalev <yury.khrusta...@arm.com>
> ---
> gcc/config/aarch64/aarch64-opts.h | 2 ++
> gcc/config/aarch64/aarch64-protos.h | 2 +-
> gcc/config/aarch64/aarch64.cc | 53 ++++++++++++++++++++++++----
> gcc/config/aarch64/aarch64.h | 1 +
> gcc/config/aarch64/aarch64.md | 3 +-
> gcc/config/arm/aarch-bti-insert.cc | 23 +++++++++---
> gcc/config/arm/aarch-common-protos.h | 1 +
> gcc/config/arm/arm.cc | 9 +++++
> 8 files changed, 81 insertions(+), 13 deletions(-)
>
> [...]
> @@ -2483,11 +2484,14 @@ aarch64_reg_save_mode (unsigned int regno)
> return the CONST_INT that should be placed in an UNSPEC_CALLEE_ABI rtx.
> */
>
> rtx
> -aarch64_gen_callee_cookie (aarch64_isa_mode isa_mode, arm_pcs pcs_variant)
> +aarch64_gen_callee_cookie (aarch64_isa_mode isa_mode, arm_pcs pcs_variant,
> + bool indirect_return)
The comment above the call needs to be updated too. E.g.:
/* Return the CONST_INT that should be placed in an UNSPEC_CALLEE_ABI rtx,
given:
- the ISA mode on entry to the callee (ISA_MODE)
- the ABI of the callee (PCS_VARIANT)
- whether the callee has an indirect_return attribute (INDIRECT_RETURN). */
> {
> - return gen_int_mode ((unsigned int) isa_mode
> - | (unsigned int) pcs_variant << AARCH64_NUM_ISA_MODES,
> - DImode);
> + unsigned int im = (unsigned int) isa_mode;
> + unsigned int ir = (indirect_return ? 1 : 0) << AARCH64_NUM_ISA_MODES;
> + unsigned int pv = (unsigned int) pcs_variant
> + << (AARCH64_NUM_ABI_ATTRIBUTES + AARCH64_NUM_ISA_MODES);
> + return gen_int_mode (im | ir | pv, DImode);
> }
>
> /* COOKIE is a CONST_INT from an UNSPEC_CALLEE_ABI rtx. Return the
> [...]
> @@ -2509,6 +2514,15 @@ aarch64_callee_isa_mode (rtx cookie)
> return UINTVAL (cookie) & ((1 << AARCH64_NUM_ISA_MODES) - 1);
> }
>
> +/* COOKIE is a CONST_INT from an UNSPEC_CALLEE_ABI rtx. Return
> + whether function was marked with indirect_return attribute. */
whether the function was marked with an ...
> +
> +static bool
> +aarch64_callee_indirect_return (rtx cookie)
> +{
> + return (UINTVAL (cookie) >> AARCH64_NUM_ISA_MODES) & 1 == 1;
> +}
> +
> /* INSN is a call instruction. Return the CONST_INT stored in its
> UNSPEC_CALLEE_ABI rtx. */
>
> [...]
> @@ -2523,6 +2537,16 @@ aarch64_insn_callee_cookie (const rtx_insn *insn)
> return XVECEXP (unspec, 0, 0);
> }
>
> +/* INSN is a call instruction. Check if function associated with
> + INSN has indirect return attribute declared in its cookie. */
Maybe:
/* INSN is a call instruction. Return true if the callee has an
indirect_return attribute. */
> +
> +bool
> +aarch_fun_is_indirect_return (rtx_insn *insn)
> +{
> + rtx cookie = aarch64_insn_callee_cookie (insn);
> + return aarch64_callee_indirect_return (cookie);
> +}
> +
> /* Implement TARGET_INSN_CALLEE_ABI. */
>
> const predefined_function_abi &
> [...]
> @@ -7320,11 +7353,13 @@ aarch64_init_cumulative_args (CUMULATIVE_ARGS *pcum,
> {
> pcum->pcs_variant = (arm_pcs) fntype_abi (fntype).id ();
> pcum->isa_mode = aarch64_fntype_isa_mode (fntype);
> + pcum->indirect_return = lookup_attribute ("indirect_return",
> TYPE_ATTRIBUTES (fntype));
Long line, maybe:
pcum->indirect_return = lookup_attribute ("indirect_return",
TYPE_ATTRIBUTES (fntype));
> }
> else
> {
> pcum->pcs_variant = ARM_PCS_AAPCS64;
> pcum->isa_mode = AARCH64_DEFAULT_ISA_MODE;
> + pcum->indirect_return = false;
> }
> pcum->aapcs_reg = NULL_RTX;
> pcum->aapcs_arg_processed = false;
> [...]
> diff --git a/gcc/config/arm/aarch-bti-insert.cc
> b/gcc/config/arm/aarch-bti-insert.cc
> index 14d36971cd4..4eb2034a2de 100644
> --- a/gcc/config/arm/aarch-bti-insert.cc
> +++ b/gcc/config/arm/aarch-bti-insert.cc
> @@ -92,6 +92,22 @@ const pass_data pass_data_insert_bti =
> 0, /* todo_flags_finish. */
> };
>
> +/* Decide if BTI J is needed after a call instruction. */
> +static bool
> +call_needs_bti_j (rtx_insn *insn)
> +{
> + /* Call returns twice, one of which may be indirect. */
> + if (find_reg_note (insn, REG_SETJMP, NULL))
> + return true;
> +
> + /* Tail call does not return. */
> + if (SIBLING_CALL_P (insn))
> + return false;
> +
> + /* Check if the function is marked to return indirectly. */
> + return aarch_fun_is_indirect_return(insn);
Space before "(insn)" (sorry!)
> +}
> +
> /* Insert the BTI instruction. */
> /* This is implemented as a late RTL pass that runs before branch
> shortening and does the following. */
> [...]
> diff --git a/gcc/config/arm/arm.cc b/gcc/config/arm/arm.cc
> index 6f11b6c816d..5075140c2a1 100644
> --- a/gcc/config/arm/arm.cc
> +++ b/gcc/config/arm/arm.cc
> @@ -33492,6 +33492,15 @@ aarch_gen_bti_j (void)
> return gen_bti_nop ();
> }
>
> +/* For Arm, we always return false because indirect_return attribute
Might be better as AArch32 instead of Arm.
Thanks,
Richard
> + is only supported on AArch64 targets. */
> +
> +bool
> +aarch_fun_is_indirect_return (rtx_insn *)
> +{
> + return false;
> +}
> +
> /* Implement TARGET_SCHED_CAN_SPECULATE_INSN. Return true if INSN can be
> scheduled for speculative execution. Reject the long-running division
> and square-root instructions. */
