Re: [PATCH] aarch64: emit .variant_pcs for aarch64_vector_pcs symbol references

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

Szabolcs Nagy <Szabolcs.Nagy@arm.com> writes:
> A dynamic linker with lazy binding support may need to handle variant
> PCS function symbols specially, so an ELF symbol table marking was
> introduced for such symbols.
>
> Global symbol references and definitions that follow the vector PCS are
> marked in the generated assembly and then the STO_AARCH64_VARIANT_PCS
> st_other flag is set on the symbol in the object file.
>
> For this to work, the assembler, the static linker and the dynamic
> linker has to be updated on a system.  Old assembler does not support
> the new .variant_pcs directive, so a toolchain with old binutils won't
> be able to compile code that references vector PCS symbols.
>
> For details about the ELF ABI and rationale see
> https://gcc.gnu.org/ml/gcc/2019-05/msg00185.html
>
> gcc/ChangeLog:
>
> 2019-05-22  Szabolcs Nagy  <szabolcs.nagy@arm.com>
>
> 	* config/aarch64/aarch64-protos.h (aarch64_asm_output_alias): Declare.
> 	(aarch64_asm_output_external): Declare.
> 	* config/aarch64/aarch64.c (aarch64_asm_output_variant_pcs): New.
> 	(aarch64_declare_function_name): Call aarch64_asm_output_variant_pcs.
> 	(aarch64_asm_output_alias): New.
> 	(aarch64_asm_output_external): New.
> 	* config/aarch64/aarch64.h (ASM_OUTPUT_DEF_FROM_DECLS): Define.
> 	(ASM_OUTPUT_EXTERNAL): Define.
>
> gcc/testsuite/ChangeLog:
>
> 2019-05-22  Szabolcs Nagy  <szabolcs.nagy@arm.com>
>
> 	* gcc.target/aarch64/pcs_attribute-2.c: New test.
> 	* gcc.target/aarch64/pcs_attribute.c: Require .variant_pcs support.
> 	* gcc.target/aarch64/torture/simd-abi-1.c: Likewise.
> 	* gcc.target/aarch64/torture/simd-abi-3.c: Likewise.
> 	* gcc.target/aarch64/torture/simd-abi-4.c: Likewise.
> 	* gcc.target/aarch64/torture/simd-abi-5.c: Likewise.
> 	* gcc.target/aarch64/torture/simd-abi-6.c: Likewise.
> 	* gcc.target/aarch64/torture/simd-abi-7.c: Likewise.
> 	* lib/target-supports.exp (check_effective_target_aarch64_variant_pcs):
> 	New.
>
> diff --git a/gcc/config/aarch64/aarch64-protos.h b/gcc/config/aarch64/aarch64-protos.h
> index b6c0d0a8eb6..68f0b542202 100644
> --- a/gcc/config/aarch64/aarch64-protos.h
> +++ b/gcc/config/aarch64/aarch64-protos.h
> @@ -427,6 +427,8 @@ bool aarch64_is_long_call_p (rtx);
>  bool aarch64_is_noplt_call_p (rtx);
>  bool aarch64_label_mentioned_p (rtx);
>  void aarch64_declare_function_name (FILE *, const char*, tree);
> +void aarch64_asm_output_alias (FILE *, const tree, const tree);
> +void aarch64_asm_output_external (FILE *, const tree, const char*);
>  bool aarch64_legitimate_pic_operand_p (rtx);
>  bool aarch64_mask_and_shift_for_ubfiz_p (scalar_int_mode, rtx, rtx);
>  bool aarch64_masks_and_shift_for_bfi_p (scalar_int_mode, unsigned HOST_WIDE_INT,
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index 83453d03095..189d958d817 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -15276,6 +15276,22 @@ aarch64_asm_preferred_eh_data_format (int code ATTRIBUTE_UNUSED, int global)
>     return (global ? DW_EH_PE_indirect : 0) | DW_EH_PE_pcrel | type;
>  }
>  
> +/* Output .variant_pcs for aarch64_vector_pcs function symbols.  */
> +
> +static void
> +aarch64_asm_output_variant_pcs (FILE *stream, const tree decl, const char* name)
> +{
> +  if (TREE_CODE (decl) == FUNCTION_DECL
> +      && TREE_PUBLIC (decl)
> +      && lookup_attribute ("aarch64_vector_pcs",
> +			   TYPE_ATTRIBUTES (TREE_TYPE (decl))))
> +    {
> +      fprintf (stream, "\t.variant_pcs\t");
> +      assemble_name (stream, name);
> +      fprintf (stream, "\n");
> +    }
> +}

What do you think about leaving out the TREE_PUBLIC check?  The ABI
doesn't require !TREE_PUBLIC functions to be marked, but it might be
safer to do the same thing for all VPCS functions anyway (especially
since, like you say below, we still mark some things that don't
necessarily need to be marked).

IMO it'd be better to use aarch64_simd_decl_p instead of the attribute test.

> +/* Implement ASM_OUTPUT_EXTERNAL.  Output .variant_pcs for undefined
> +   function symbols references.  */

"symbol references"

> diff --git a/gcc/testsuite/gcc.target/aarch64/pcs_attribute-2.c b/gcc/testsuite/gcc.target/aarch64/pcs_attribute-2.c
> new file mode 100644
> index 00000000000..c808bd45d0f
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/pcs_attribute-2.c
> @@ -0,0 +1,101 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target aarch64_variant_pcs } */
> +
> +/* Test that .variant_pcs is emitted for vector PCS symbol references.  */
> +
> +#define ATTR __attribute__ ((aarch64_vector_pcs))
> +
> +void f_undef_basepcs (void);
> +
> +void f_def_basepcs (void)
> +{
> +}
> +
> +ATTR void f_undef_vpcs (void);
> +
> +ATTR void f_def_vpcs (void)
> +{
> +}
> +
> +__attribute__ ((alias ("f_def_vpcs")))
> +ATTR void f_alias_vpcs (void);
> +
> +__attribute__ ((weak, alias ("f_def_vpcs")))
> +ATTR void f_weak_alias_vpcs (void);
> +
> +__attribute__ ((weak))
> +ATTR void f_weak_undef_vpcs (void);
> +
> +__attribute__ ((visibility ("protected")))
> +ATTR void f_protected_vpcs (void)
> +{
> +}
> +
> +__attribute__ ((visibility ("hidden")))
> +ATTR void f_hidden_vpcs (void)
> +{
> +}
> +
> +ATTR static void f_local_vpcs (void)
> +{
> +}
> +
> +ATTR void bar_undef_vpcs (void) __asm__ ("f_undef_renamed_vpcs");
> +
> +ATTR void bar_def_vpcs (void) __asm__ ("f_def_renamed_vpcs");
> +ATTR void bar_def_vpcs (void)
> +{
> +}
> +
> +static void (*f_ifunc_resolver ()) (void)
> +{
> +  return (void (*)(void))f_local_vpcs;
> +}
> +
> +__attribute__ ((ifunc ("f_ifunc_resolver")))
> +ATTR void f_ifunc_vpcs (void);
> +
> +__attribute__ ((visibility ("hidden")))
> +__attribute__ ((ifunc ("f_ifunc_resolver")))
> +ATTR void f_hidden_ifunc_vpcs (void);
> +
> +__attribute__ ((ifunc ("f_ifunc_resolver")))
> +ATTR static void f_local_ifunc_vpcs (void);
> +
> +void (*refs_basepcs[]) (void) = {
> +	f_undef_basepcs,
> +	f_def_basepcs,
> +};
> +
> +void (*ATTR refs_vpcs[]) (void) = {
> +	f_undef_vpcs,
> +	f_def_vpcs,
> +	f_alias_vpcs,
> +	f_weak_alias_vpcs,
> +	f_weak_undef_vpcs,
> +	f_protected_vpcs,
> +	f_hidden_vpcs,
> +	f_local_vpcs,
> +	bar_undef_vpcs,
> +	bar_def_vpcs,
> +	f_ifunc_vpcs,
> +	f_hidden_ifunc_vpcs,
> +	f_local_ifunc_vpcs,
> +};
> +
> +/* Note: hidden and local symbols don't need .variant_pcs,
> +   here we accept it on hidden symbols, but not on local ones.  */
> +
> +/* { dg-final { scan-assembler-not {\.variant_pcs\tf_undef_basepcs} } } */
> +/* { dg-final { scan-assembler-not {\.variant_pcs\tf_def_basepcs} } } */
> +/* { dg-final { scan-assembler-not {\.variant_pcs\tf_local_vpcs} } } */
> +/* { dg-final { scan-assembler-not {\.variant_pcs\tf_local_ifunc_vpcs} } } */
> +/* { dg-final { scan-assembler-times {\.variant_pcs\tf_undef_vpcs} 1 } } */
> +/* { dg-final { scan-assembler-times {\.variant_pcs\tf_def_vpcs} 1 } } */
> +/* { dg-final { scan-assembler-times {\.variant_pcs\tf_alias_vpcs} 1 } } */
> +/* { dg-final { scan-assembler-times {\.variant_pcs\tf_weak_alias_vpcs} 1 } } */
> +/* { dg-final { scan-assembler-times {\.variant_pcs\tf_weak_undef_vpcs} 1 } } */
> +/* { dg-final { scan-assembler-times {\.variant_pcs\tf_protected_vpcs} 1 } } */
> +/* { dg-final { scan-assembler-times {\.variant_pcs\tf_undef_renamed_vpcs} 1 } } */
> +/* { dg-final { scan-assembler-times {\.variant_pcs\tf_def_renamed_vpcs} 1 } } */
> +/* { dg-final { scan-assembler-times {\.variant_pcs\tf_ifunc_vpcs} 1 } } */

It might be worth including weakref too.  (Just for completeness.
It's not likely to be different in an interesting way from the cases above.)

> diff --git a/gcc/testsuite/gcc.target/aarch64/pcs_attribute.c b/gcc/testsuite/gcc.target/aarch64/pcs_attribute.c
> index 9a99b91a6ed..8fe390f15ac 100644
> --- a/gcc/testsuite/gcc.target/aarch64/pcs_attribute.c
> +++ b/gcc/testsuite/gcc.target/aarch64/pcs_attribute.c
> @@ -1,4 +1,5 @@
>  /* { dg-do compile } */
> +/* { dg-require-effective-target aarch64_variant_pcs } */
>   
>  /* Test that the assignment of f (with the attribute) to function pointer g
>     (with no attribute) results in an error.  */

Do we need this for compile tests?  It would be better to use it only
for assemble, link and run tests if possible.

> diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp
> index 3bd6e815715..50c05ffd1eb 100644
> --- a/gcc/testsuite/lib/target-supports.exp
> +++ b/gcc/testsuite/lib/target-supports.exp
> @@ -8591,6 +8591,15 @@ proc check_effective_target_aarch64_large { } {
>      }
>  }
>  
> +proc check_effective_target_aarch64_variant_pcs { } {
> +    if { [istarget aarch64*-*-*] } {
> +	return [check_no_compiler_messages aarch64_variant_pcs object {
> +	    __asm__ (".variant_pcs foo");
> +	}]
> +    } else {
> +	return 0
> +    }
> +}
>  
>  # Return 1 if this is a reduced AVR Tiny core.  Such cores have different
>  # register set, instruction set, addressing capabilities and ABI.

Needs a comment. :-)

Thanks,
Richard