Re: [PATCH] arm: Fixed C23 call compatibility with arm-none-eabi

[Andrew Pinski <pinskia@gmail.com>]

On Mon, Feb 19, 2024 at 1:14 AM Torbjörn SVENSSON
<torbjorn.svensson@foss.st.com> wrote:
>
> Ok for trunk and releases/gcc-13?
> Regtested on top of 945cb8490cb for arm-none-eabi, without any regression.
>
> Backporting to releases/gcc-13 will change -std=c23 to -std=c2x.
>
> --
>
> In commit 4fe34cdcc80ac225b80670eabc38ac5e31ce8a5a, -std=c23 support was
> introduced to support functions without any named arguments.  For
> arm-none-eabi, this is not as simple as placing all arguments on the
> stack.  Align the caller to use r0, r1, r2 and r3 for arguments even for
> functions without any named arguments, as specified in the AAPCS.
>
> Verify that the generic test case have the arguments are in the right
> order and add ARM specific test cases.
>
> gcc/ChangeLog:
>
>         * calls.h: Added the type of the function to function_arg_info.
>         * calls.cc: Save the type of the function.
>         * config/arm/arm.cc: Check in the AAPCS layout function if
>         function has no named args.
>
> gcc/testsuite/ChangeLog:
>
>         * gcc.dg/torture/c23-stdarg-split-1a.c: Detect out of order
>         arguments.
>         * gcc.dg/torture/c23-stdarg-split-1b.c: Likewise.

It is almost always better to add a new testcase for the expanded idea
of the test rather than modifying the original.

Thanks,
Andrew Pinski

>         * gcc.target/arm/aapcs/align_vaarg3.c: New test.
>         * gcc.target/arm/aapcs/align_vaarg4.c: New test.
>
> Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
> Co-authored-by: Yvan ROUX <yvan.roux@foss.st.com>
> ---
>  gcc/calls.cc                                  |  2 +-
>  gcc/calls.h                                   | 20 ++++++++--
>  gcc/config/arm/arm.cc                         | 13 ++++---
>  .../gcc.dg/torture/c23-stdarg-split-1a.c      |  4 +-
>  .../gcc.dg/torture/c23-stdarg-split-1b.c      | 15 +++++---
>  .../gcc.target/arm/aapcs/align_vaarg3.c       | 37 +++++++++++++++++++
>  .../gcc.target/arm/aapcs/align_vaarg4.c       | 31 ++++++++++++++++
>  7 files changed, 102 insertions(+), 20 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.target/arm/aapcs/align_vaarg3.c
>  create mode 100644 gcc/testsuite/gcc.target/arm/aapcs/align_vaarg4.c
>
> diff --git a/gcc/calls.cc b/gcc/calls.cc
> index 01f44734743..a1cc283b952 100644
> --- a/gcc/calls.cc
> +++ b/gcc/calls.cc
> @@ -1376,7 +1376,7 @@ initialize_argument_information (int num_actuals ATTRIBUTE_UNUSED,
>          with those made by function.cc.  */
>
>        /* See if this argument should be passed by invisible reference.  */
> -      function_arg_info arg (type, argpos < n_named_args);
> +      function_arg_info arg (type, fntype, argpos < n_named_args);
>        if (pass_by_reference (args_so_far_pnt, arg))
>         {
>           const bool callee_copies
> diff --git a/gcc/calls.h b/gcc/calls.h
> index 464a4e34e33..88836559ebe 100644
> --- a/gcc/calls.h
> +++ b/gcc/calls.h
> @@ -35,24 +35,33 @@ class function_arg_info
>  {
>  public:
>    function_arg_info ()
> -    : type (NULL_TREE), mode (VOIDmode), named (false),
> +    : type (NULL_TREE), fntype (NULL_TREE), mode (VOIDmode), named (false),
>        pass_by_reference (false)
>    {}
>
>    /* Initialize an argument of mode MODE, either before or after promotion.  */
>    function_arg_info (machine_mode mode, bool named)
> -    : type (NULL_TREE), mode (mode), named (named), pass_by_reference (false)
> +    : type (NULL_TREE), fntype (NULL_TREE), mode (mode), named (named),
> +    pass_by_reference (false)
>    {}
>
>    /* Initialize an unpromoted argument of type TYPE.  */
>    function_arg_info (tree type, bool named)
> -    : type (type), mode (TYPE_MODE (type)), named (named),
> +    : type (type), fntype (NULL_TREE), mode (TYPE_MODE (type)), named (named),
>        pass_by_reference (false)
>    {}
>
> +  /* Initialize an unpromoted argument of type TYPE with a known function type
> +     FNTYPE.  */
> +  function_arg_info (tree type, tree fntype, bool named)
> +    : type (type), fntype (fntype), mode (TYPE_MODE (type)), named (named),
> +    pass_by_reference (false)
> +  {}
> +
>    /* Initialize an argument with explicit properties.  */
>    function_arg_info (tree type, machine_mode mode, bool named)
> -    : type (type), mode (mode), named (named), pass_by_reference (false)
> +    : type (type), fntype (NULL_TREE), mode (mode), named (named),
> +    pass_by_reference (false)
>    {}
>
>    /* Return true if the gimple-level type is an aggregate.  */
> @@ -96,6 +105,9 @@ public:
>       libgcc support functions).  */
>    tree type;
>
> +  /* The type of the function that has this argument, or null if not known.  */
> +  tree fntype;
> +
>    /* The mode of the argument.  Depending on context, this might be
>       the mode of the argument type or the mode after promotion.  */
>    machine_mode mode;
> diff --git a/gcc/config/arm/arm.cc b/gcc/config/arm/arm.cc
> index 1cd69268ee9..98e149e5b7e 100644
> --- a/gcc/config/arm/arm.cc
> +++ b/gcc/config/arm/arm.cc
> @@ -7006,7 +7006,7 @@ aapcs_libcall_value (machine_mode mode)
>     numbers referred to here are those in the AAPCS.  */
>  static void
>  aapcs_layout_arg (CUMULATIVE_ARGS *pcum, machine_mode mode,
> -                 const_tree type, bool named)
> +                 const_tree type, bool named, const_tree fntype)
>  {
>    int nregs, nregs2;
>    int ncrn;
> @@ -7018,8 +7018,9 @@ aapcs_layout_arg (CUMULATIVE_ARGS *pcum, machine_mode mode,
>    pcum->aapcs_arg_processed = true;
>
>    /* Special case: if named is false then we are handling an incoming
> -     anonymous argument which is on the stack.  */
> -  if (!named)
> +     anonymous argument which is on the stack unless the function has no named
> +     arguments (functions without named arguments was introduced in C23).  */
> +  if ((fntype == NULL_TREE || !TYPE_NO_NAMED_ARGS_STDARG_P (fntype)) && !named)
>      return;
>
>    /* Is this a potential co-processor register candidate?  */
> @@ -7267,7 +7268,7 @@ arm_function_arg (cumulative_args_t pcum_v, const function_arg_info &arg)
>
>    if (pcum->pcs_variant <= ARM_PCS_AAPCS_LOCAL)
>      {
> -      aapcs_layout_arg (pcum, arg.mode, arg.type, arg.named);
> +      aapcs_layout_arg (pcum, arg.mode, arg.type, arg.named, arg.fntype);
>        return pcum->aapcs_reg;
>      }
>
> @@ -7342,7 +7343,7 @@ arm_arg_partial_bytes (cumulative_args_t pcum_v, const function_arg_info &arg)
>
>    if (pcum->pcs_variant <= ARM_PCS_AAPCS_LOCAL)
>      {
> -      aapcs_layout_arg (pcum, arg.mode, arg.type, arg.named);
> +      aapcs_layout_arg (pcum, arg.mode, arg.type, arg.named, arg.fntype);
>        return pcum->aapcs_partial;
>      }
>
> @@ -7367,7 +7368,7 @@ arm_function_arg_advance (cumulative_args_t pcum_v,
>
>    if (pcum->pcs_variant <= ARM_PCS_AAPCS_LOCAL)
>      {
> -      aapcs_layout_arg (pcum, arg.mode, arg.type, arg.named);
> +      aapcs_layout_arg (pcum, arg.mode, arg.type, arg.named, arg.fntype);
>
>        if (pcum->aapcs_cprc_slot >= 0)
>         {
> diff --git a/gcc/testsuite/gcc.dg/torture/c23-stdarg-split-1a.c b/gcc/testsuite/gcc.dg/torture/c23-stdarg-split-1a.c
> index 2dff79235b2..717fc4bfd07 100644
> --- a/gcc/testsuite/gcc.dg/torture/c23-stdarg-split-1a.c
> +++ b/gcc/testsuite/gcc.dg/torture/c23-stdarg-split-1a.c
> @@ -5,7 +5,6 @@
>  /* { dg-options "-std=c23 -pedantic-errors" } */
>  /* { dg-additional-sources "c23-stdarg-split-1b.c" } */
>
> -extern void abort (void);
>  extern void exit (int);
>
>  double f (...);
> @@ -22,8 +21,7 @@ void h7 (volatile struct s x, ...);
>  int
>  main ()
>  {
> -  if (f (1, 2.0, 3, 4.0) != 10.0)
> -    abort ();
> +  f (1, 2.0, 3, 4.0);
>    g (0.0, 1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0);
>    g (0.0f, 1.0f, 2.0f, 3.0f, 4.0f, 5.0f, 6.0f, 7.0f, 8.0f, 9.0f);
>    h1 (0, 0.0, 1, 2.0, 3, 4.0, 5, 6.0, 7, 8.0, 9);
> diff --git a/gcc/testsuite/gcc.dg/torture/c23-stdarg-split-1b.c b/gcc/testsuite/gcc.dg/torture/c23-stdarg-split-1b.c
> index 064da121ec2..3b70fe1595a 100644
> --- a/gcc/testsuite/gcc.dg/torture/c23-stdarg-split-1b.c
> +++ b/gcc/testsuite/gcc.dg/torture/c23-stdarg-split-1b.c
> @@ -8,17 +8,20 @@
>
>  extern void abort (void);
>
> -double
> +void
>  f (...)
>  {
>    va_list ap;
>    va_start (ap);
> -  double ret = va_arg (ap, int);
> -  ret += va_arg (ap, double);
> -  ret += va_arg (ap, int);
> -  ret += va_arg (ap, double);
> +  if (va_arg (ap, int) != 1)
> +    abort();
> +  if (va_arg (ap, double) != 2)
> +    abort();
> +  if (va_arg (ap, int) != 3)
> +    abort();
> +  if (va_arg (ap, double) != 4)
> +    abort();
>    va_end (ap);
> -  return ret;
>  }
>
>  void
> diff --git a/gcc/testsuite/gcc.target/arm/aapcs/align_vaarg3.c b/gcc/testsuite/gcc.target/arm/aapcs/align_vaarg3.c
> new file mode 100644
> index 00000000000..23cc78d5230
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/arm/aapcs/align_vaarg3.c
> @@ -0,0 +1,37 @@
> +/* Test AAPCS layout (alignment of varargs) for callee.  */
> +
> +/* { dg-do run { target arm_eabi } } */
> +/* { dg-require-effective-target arm32 } */
> +/* { dg-options "-std=c23 -O2 -fno-inline" } */
> +
> +#include <stdarg.h>
> +
> +extern void abort (void);
> +
> +typedef __attribute__((aligned (8))) int alignedint;
> +
> +void
> +foo (...)
> +{
> +  int i = 0;
> +  va_list va;
> +  va_start (va);
> +  /* Arguments should be passed in the same registers as if they were ints.  */
> +  for (i = 5; i >= 0; i--)
> +    if (va_arg (va, int) != i)
> +      abort ();
> +  va_end (va);
> +}
> +
> +int
> +main (int argc, char **argv)
> +{
> +  alignedint a = 5;
> +  alignedint b = 4;
> +  alignedint c = 3;
> +  alignedint d = 2;
> +  alignedint e = 1;
> +  alignedint f = 0;
> +  foo (a, b, c, d, e, f);
> +  return 0;
> +}
> diff --git a/gcc/testsuite/gcc.target/arm/aapcs/align_vaarg4.c b/gcc/testsuite/gcc.target/arm/aapcs/align_vaarg4.c
> new file mode 100644
> index 00000000000..c4333f33677
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/arm/aapcs/align_vaarg4.c
> @@ -0,0 +1,31 @@
> +/* Test AAPCS layout (alignment of varargs) for callee.  */
> +
> +/* { dg-do run { target arm_eabi } } */
> +/* { dg-require-effective-target arm32 } */
> +/* { dg-options "-std=c23 -O2 -fno-inline" } */
> +
> +#include <stdarg.h>
> +
> +extern void abort (void);
> +
> +typedef __attribute__((aligned (8))) int alignedint;
> +
> +void
> +foo (...)
> +{
> +  int i = 0;
> +  va_list va;
> +  va_start (va);
> +  /* alignedint should be pulled out of regs/stack just like an int.  */
> +  for (i = 5; i >= 0; i--)
> +    if (va_arg (va, alignedint) != i)
> +      abort ();
> +  va_end (va);
> +}
> +
> +int
> +main (int argc, char **argv)
> +{
> +  foo (5, 4, 3, 2, 1, 0);
> +  return 0;
> +}
> --
> 2.25.1
>