public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] arm: fix c23 0-named-args caller-side stdarg
@ 2023-11-19  7:32 Alexandre Oliva
  2023-12-06  2:27 ` Alexandre Oliva
  0 siblings, 1 reply; 9+ messages in thread
From: Alexandre Oliva @ 2023-11-19  7:32 UTC (permalink / raw)
  To: gcc-patches
  Cc: Joseph Myers, Nick Clifton, Richard Earnshaw,
	Ramana Radhakrishnan, Kyrylo Tkachov


On arm-eabi targets, c23 stdarg execution tests that pass arguments to
(...) functions (without any named argument), the caller passes
everything on the stack, but the callee expects arguments in
registers.  My reading of the AAPCS32 suggests that the caller is
correct, so I've arranged for the caller to pass the first arguments
in registers to TYPE_NO_NAMED_STDARG_P-typed functions.

The implementation issue in calls.cc is that n_named_args is initially
set to zero in expand_call, so the test argpos < n_named_args yields
false for all arguments, and aapcs_layout_arg takes !named as meaning
stack.

But there's a catch there: on targets in which neither
strict_argument_naming nor !pretend_outgoing_varargs_named hold,
n_named_args is bumped up to num_actuals, which covers stdarg
arguments in pre-c23 cases, but not for TYPE_NO_NAMED_ARGS_STDARG_P.

I'm hesitant to modify the generic ABI-affecting code, so I'm going
for a more surgical fix for ARM AAPCS only.  I suspect we might want
yet another targetm predicate to enable the n_named_args overriding
block to disregard TYPE_NO_NAMED_ARGS_STDARG_P, and allow all actuals
to be passed as if named.

Regstrapped on x86_64-linux-gnu, also tested on arm-eabi with default
cpu on trunk, and with tms570 on gcc-13.  Ok to install?


for  gcc/ChangeLog

	* config/arm/arm.h (CUMULATIVE_ARGS): Add aapcs_pretend_named.
	* config/arm/arm.cc (arm_init_cumulative_args): Set it for
	aapcs no-named-args stdarg functions.
	(aapcs_layout_arg): Ignore named if aapcs_pretend_named.
---
 gcc/config/arm/arm.cc |    4 +++-
 gcc/config/arm/arm.h  |    1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/gcc/config/arm/arm.cc b/gcc/config/arm/arm.cc
index 25a1ad736ad96..c31bf193365ef 100644
--- a/gcc/config/arm/arm.cc
+++ b/gcc/config/arm/arm.cc
@@ -7011,7 +7011,7 @@ aapcs_layout_arg (CUMULATIVE_ARGS *pcum, machine_mode mode,
 
   /* Special case: if named is false then we are handling an incoming
      anonymous argument which is on the stack.  */
-  if (!named)
+  if (!named && !pcum->aapcs_pretend_named)
     return;
 
   /* Is this a potential co-processor register candidate?  */
@@ -7132,6 +7132,8 @@ arm_init_cumulative_args (CUMULATIVE_ARGS *pcum, tree fntype,
       pcum->aapcs_arg_processed = false;
       pcum->aapcs_cprc_slot = -1;
       pcum->can_split = true;
+      pcum->aapcs_pretend_named = (fntype
+				   && TYPE_NO_NAMED_ARGS_STDARG_P (fntype));
 
       if (pcum->pcs_variant != ARM_PCS_AAPCS)
 	{
diff --git a/gcc/config/arm/arm.h b/gcc/config/arm/arm.h
index a9c2752c0ea5e..65d2d567686d3 100644
--- a/gcc/config/arm/arm.h
+++ b/gcc/config/arm/arm.h
@@ -1702,6 +1702,7 @@ typedef struct
   unsigned aapcs_vfp_reg_alloc;
   int aapcs_vfp_rcount;
   MACHMODE aapcs_vfp_rmode;
+  bool aapcs_pretend_named; /* Set for TYPE_NO_NAMED_ARGS_STDARG_P.  */
 } CUMULATIVE_ARGS;
 #endif
 

-- 
Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
   Free Software Activist                   GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity
Excluding neuro-others for not behaving ""normal"" is *not* inclusive

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] arm: fix c23 0-named-args caller-side stdarg
  2023-11-19  7:32 [PATCH] arm: fix c23 0-named-args caller-side stdarg Alexandre Oliva
@ 2023-12-06  2:27 ` Alexandre Oliva
  2024-01-23  8:26   ` Alexandre Oliva
  0 siblings, 1 reply; 9+ messages in thread
From: Alexandre Oliva @ 2023-12-06  2:27 UTC (permalink / raw)
  To: gcc-patches
  Cc: Joseph Myers, Nick Clifton, Richard Earnshaw,
	Ramana Radhakrishnan, Kyrylo Tkachov

On Nov 19, 2023, Alexandre Oliva <oliva@adacore.com> wrote:

> On arm-eabi targets, c23 stdarg execution tests that pass arguments to
> (...) functions (without any named argument), the caller passes
> everything on the stack, but the callee expects arguments in
> registers.

Ping?  This slightly modified patch only adds comments to
aapcs_layout_arg compared with the original one.

The commit message doesn't name explicitly the fixed testsuite
failures.  Here they are:

FAIL: gcc.dg/c23-stdarg-4.c execution test
FAIL: gcc.dg/torture/c23-stdarg-split-1a.c   -O0  execution test
FAIL: gcc.dg/torture/c23-stdarg-split-1a.c   -O1  execution test
FAIL: gcc.dg/torture/c23-stdarg-split-1a.c   -O2  execution test
FAIL: gcc.dg/torture/c23-stdarg-split-1a.c   -O2 -flto -fno-use-linker-plugin -flto-partition=none  execution test
FAIL: gcc.dg/torture/c23-stdarg-split-1a.c   -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects  execution test
FAIL: gcc.dg/torture/c23-stdarg-split-1a.c   -O3 -g  execution test
FAIL: gcc.dg/torture/c23-stdarg-split-1a.c   -Os  execution test

Tested on arm-eabi.  Ok to install?


arm: fix c23 0-named-args caller-side stdarg

On arm-eabi targets, c23 stdarg execution tests that pass arguments to
(...) functions (without any named argument), the caller passes
everything on the stack, but the callee expects arguments in
registers.  My reading of the AAPCS32 suggests that the caller is
correct, so I've arranged for the caller to pass the first arguments
in registers to TYPE_NO_NAMED_STDARG_P-typed functions.

The implementation issue in calls.cc is that n_named_args is initially
set to zero in expand_call, so the test argpos < n_named_args yields
false for all arguments, and aapcs_layout_arg takes !named as meaning
stack.

But there's a catch there: on targets in which neither
strict_argument_naming nor !pretend_outgoing_varargs_named hold,
n_named_args is bumped up to num_actuals, which covers stdarg
arguments in pre-c23 cases, but not for TYPE_NO_NAMED_ARGS_STDARG_P.

I'm hesitant to modify the generic ABI-affecting code, so I'm going
for a more surgical fix for ARM AAPCS only.  I suspect we might want
yet another targetm predicate to enable the n_named_args overriding
block to disregard TYPE_NO_NAMED_ARGS_STDARG_P, and allow all actuals
to be passed as if named.


for  gcc/ChangeLog

	* config/arm/arm.h (CUMULATIVE_ARGS): Add aapcs_pretend_named.
	* config/arm/arm.cc (arm_init_cumulative_args): Set it for
	aapcs no-named-args stdarg functions.
	(aapcs_layout_arg): Ignore named if aapcs_pretend_named.
---
 gcc/config/arm/arm.cc |    9 +++++++--
 gcc/config/arm/arm.h  |    1 +
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/gcc/config/arm/arm.cc b/gcc/config/arm/arm.cc
index 6e3e2e8fb1bfb..4a350bd8c8f47 100644
--- a/gcc/config/arm/arm.cc
+++ b/gcc/config/arm/arm.cc
@@ -7019,8 +7019,11 @@ 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
+     aapcs_pretend_named, in which case we're dealing with a
+     TYPE_NO_NAMED_ARGS_STDARG_P call and, even if args are !named, we
+     ought to use available registers first.  */
+  if (!named && !pcum->aapcs_pretend_named)
     return;
 
   /* Is this a potential co-processor register candidate?  */
@@ -7141,6 +7144,8 @@ arm_init_cumulative_args (CUMULATIVE_ARGS *pcum, tree fntype,
       pcum->aapcs_arg_processed = false;
       pcum->aapcs_cprc_slot = -1;
       pcum->can_split = true;
+      pcum->aapcs_pretend_named = (fntype
+				   && TYPE_NO_NAMED_ARGS_STDARG_P (fntype));
 
       if (pcum->pcs_variant != ARM_PCS_AAPCS)
 	{
diff --git a/gcc/config/arm/arm.h b/gcc/config/arm/arm.h
index a9c2752c0ea5e..65d2d567686d3 100644
--- a/gcc/config/arm/arm.h
+++ b/gcc/config/arm/arm.h
@@ -1702,6 +1702,7 @@ typedef struct
   unsigned aapcs_vfp_reg_alloc;
   int aapcs_vfp_rcount;
   MACHMODE aapcs_vfp_rmode;
+  bool aapcs_pretend_named; /* Set for TYPE_NO_NAMED_ARGS_STDARG_P.  */
 } CUMULATIVE_ARGS;
 #endif
 


-- 
Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
   Free Software Activist                   GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity
Excluding neuro-others for not behaving ""normal"" is *not* inclusive

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] arm: fix c23 0-named-args caller-side stdarg
  2023-12-06  2:27 ` Alexandre Oliva
@ 2024-01-23  8:26   ` Alexandre Oliva
  2024-02-26 16:41     ` Matthew Malcomson
  0 siblings, 1 reply; 9+ messages in thread
From: Alexandre Oliva @ 2024-01-23  8:26 UTC (permalink / raw)
  To: gcc-patches
  Cc: Joseph Myers, Nick Clifton, Richard Earnshaw,
	Ramana Radhakrishnan, Kyrylo Tkachov

On Dec  5, 2023, Alexandre Oliva <oliva@adacore.com> wrote:

> arm: fix c23 0-named-args caller-side stdarg

Ping?
https://gcc.gnu.org/pipermail/gcc-patches/2023-December/639472.html

> The commit message doesn't name explicitly the fixed testsuite
> failures.  Here they are:

> FAIL: gcc.dg/c23-stdarg-4.c execution test
> FAIL: gcc.dg/torture/c23-stdarg-split-1a.c   -O0  execution test
> FAIL: gcc.dg/torture/c23-stdarg-split-1a.c   -O1  execution test
> FAIL: gcc.dg/torture/c23-stdarg-split-1a.c   -O2  execution test
> FAIL: gcc.dg/torture/c23-stdarg-split-1a.c   -O2 -flto -fno-use-linker-plugin -flto-partition=none  execution test
> FAIL: gcc.dg/torture/c23-stdarg-split-1a.c   -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects  execution test
> FAIL: gcc.dg/torture/c23-stdarg-split-1a.c   -O3 -g  execution test
> FAIL: gcc.dg/torture/c23-stdarg-split-1a.c   -Os  execution test

> Tested on arm-eabi.  Ok to install?


-- 
Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
   Free Software Activist                   GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity
Excluding neuro-others for not behaving ""normal"" is *not* inclusive

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] arm: fix c23 0-named-args caller-side stdarg
  2024-01-23  8:26   ` Alexandre Oliva
@ 2024-02-26 16:41     ` Matthew Malcomson
  2024-03-01  4:38       ` Alexandre Oliva
  0 siblings, 1 reply; 9+ messages in thread
From: Matthew Malcomson @ 2024-02-26 16:41 UTC (permalink / raw)
  To: Alexandre Oliva, gcc-patches
  Cc: Joseph Myers, Nick Clifton, Richard Earnshaw,
	Ramana Radhakrishnan, Kyrylo Tkachov

Hi Alexandre,

I don't have the ability to OK the patch, but I'm attempting to do a 
review in
order to reduce the workload for any maintainer.  (Apologies for the slow
response).

I think you're right that the AAPCS32 requires all arguments to be passed in
registers for this testcase.
(Nit on the commit-message: It says that your reading of the AAPCS32 
suggests
that the *caller* is correct -- I believe based on the change you 
suggested you
meant *callee* is correct in expecting arguments in registers.)

The approach you suggest looks OK to me -- I do notice that it doesn't 
fix the
legacy ABI's of `atpcs` and `apcs` and guess it would be nicer to have them
working at the same time though would defer to maintainers on how 
important that
is.
(For the benefit of others reading) I don't believe there is any ABI concern
with this since it's fixing something that is currently not working at 
all and
only applies to c23 (so a change shouldn't have too much of an impact).

You mention you chose to make the change in the arm backend rather than 
general
code due to hesitancy to change the generic ABI-affecting code. That makes
sense to me, certainly at this late stage in the development cycle.

That said, I do expect the more robust solution would be to change a part of
the generic ABI code and would like to check that this makes sense to 
you and
anyone else upstream in the discussion (justification below).
So would suggest to the maintainers that maybe this goes in with a note to
remember to look at a possible generic code change later on.

Does that sound reasonable to you?

------------------------------
Justification for why a change to the generic ABI code would be better 
in the
end:

IIUC (from the documentation of the hooks) `pretend_outgoing_varargs_named`
really should mean that `n_named_args = num_actuals`.  That seems to be 
what the
documentation for `strict_argument_naming` indicates should happen.

``` (Documentation for TARGET_STRICT_ARGUMENT_NAMING):
If it returns 'false', but 'TARGET_PRETEND_OUTGOING_VARARGS_NAMED' returns
'true', then all arguments are treated as named.
```

Because of this I guess that if `pretend_outgoing_varargs_named` only 
means to
pretend the above *except* in the c23 0-named-args case that seems like 
it would
be a bit awkward to account for in backends.

 From a quick check on c23-stdarg-4.c it does look like the below change 
ends up
with the same codegen as your patch (except in the case of those legacy 
ABI's,
where the below does make the caller and callee ABI match AFAICT):

```
   diff --git a/gcc/calls.cc b/gcc/calls.cc
   index 01f44734743..0b302f633ed 100644
   --- a/gcc/calls.cc
   +++ b/gcc/calls.cc
   @@ -2970,14 +2970,15 @@ expand_call (tree exp, rtx target, int ignore)
         we do not have any reliable way to pass unnamed args in
         registers, so we must force them into memory.  */

   -  if (type_arg_types != 0
   +  if ((type_arg_types != 0 || TYPE_NO_NAMED_ARGS_STDARG_P (funtype))
          && targetm.calls.strict_argument_naming (args_so_far))
        ;
      else if (type_arg_types != 0
              && ! targetm.calls.pretend_outgoing_varargs_named 
(args_so_far))
        /* Don't include the last named arg.  */
        --n_named_args;
   -  else if (TYPE_NO_NAMED_ARGS_STDARG_P (funtype))
   +  else if (TYPE_NO_NAMED_ARGS_STDARG_P (funtype)
   +        && ! targetm.calls.pretend_outgoing_varargs_named (args_so_far))
        n_named_args = 0;
      else
        /* Treat all args as named.  */
```

Do you agree that this makes sense (i.e. is there something I'm 
completely missing)?

FWIW I attempted to try and find other targets which have
`strict_argument_naming` returning `false`, `pretend_outgoing_varargs_named`
returning true, and some use of the `.named` member of function 
arguments.  I
got the below list.  I recognise it doesn't mean there's a bug in these
backends, but thought it might help the discussion.
(lm32 mcore msp430 gcn cris fr30 frv h8300 arm v850 rx pru)



On 1/23/24 08:26, Alexandre Oliva wrote:
> On Dec  5, 2023, Alexandre Oliva <oliva@adacore.com> wrote:
>
>> arm: fix c23 0-named-args caller-side stdarg
> Ping?
> https://gcc.gnu.org/pipermail/gcc-patches/2023-December/639472.html
>
>> The commit message doesn't name explicitly the fixed testsuite
>> failures.  Here they are:
>> FAIL: gcc.dg/c23-stdarg-4.c execution test
>> FAIL: gcc.dg/torture/c23-stdarg-split-1a.c   -O0  execution test
>> FAIL: gcc.dg/torture/c23-stdarg-split-1a.c   -O1  execution test
>> FAIL: gcc.dg/torture/c23-stdarg-split-1a.c   -O2  execution test
>> FAIL: gcc.dg/torture/c23-stdarg-split-1a.c   -O2 -flto -fno-use-linker-plugin -flto-partition=none  execution test
>> FAIL: gcc.dg/torture/c23-stdarg-split-1a.c   -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects  execution test
>> FAIL: gcc.dg/torture/c23-stdarg-split-1a.c   -O3 -g  execution test
>> FAIL: gcc.dg/torture/c23-stdarg-split-1a.c   -Os  execution test
>> Tested on arm-eabi.  Ok to install?
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] arm: fix c23 0-named-args caller-side stdarg
  2024-02-26 16:41     ` Matthew Malcomson
@ 2024-03-01  4:38       ` Alexandre Oliva
  2024-03-01 14:56         ` Richard Earnshaw (lists)
  0 siblings, 1 reply; 9+ messages in thread
From: Alexandre Oliva @ 2024-03-01  4:38 UTC (permalink / raw)
  To: Matthew Malcomson
  Cc: gcc-patches, Joseph Myers, Nick Clifton, Richard Earnshaw,
	Ramana Radhakrishnan, Kyrylo Tkachov, Jakub Jelinek

Hello, Matthew,

Thanks for the review.

On Feb 26, 2024, Matthew Malcomson <matthew.malcomson@arm.com> wrote:

> I think you're right that the AAPCS32 requires all arguments to be passed in
> registers for this testcase.
> (Nit on the commit-message: It says that your reading of the AAPCS32
> suggests
> that the *caller* is correct -- I believe based on the change you
> suggested you
> meant *callee* is correct in expecting arguments in registers.)

Ugh, yeah, sorry about the typo.

> The approach you suggest looks OK to me -- I do notice that it doesn't
> fix the
> legacy ABI's of `atpcs` and `apcs` and guess it would be nicer to have them
> working at the same time though would defer to maintainers on how
> important that
> is.
> (For the benefit of others reading) I don't believe there is any ABI concern
> with this since it's fixing something that is currently not working at
> all and
> only applies to c23 (so a change shouldn't have too much of an impact).

> You mention you chose to make the change in the arm backend rather
> than general
> code due to hesitancy to change the generic ABI-affecting code. That makes
> sense to me, certainly at this late stage in the development cycle.

*nod* I wrote the patch in the following context: I hit the problem on
the very first toolchain I started transitioning to gcc-13.  I couldn't
really fathom the notion that this breakage could have survived an
entire release cycle if it affected many targets, and sort of held on to
an assumption that the abi used by our arm-eabi toolchain had to be an
uncommon one.

All of this hypothesizing falls apart by the now apparent knowledge that
the test is faling elsewhere as well, even on other ARM ABIs, it just
hadn't been addressed yet.  I'm glad we're getting there :-)

> From a quick check on c23-stdarg-4.c it does look like the below
> change ends up
> with the same codegen as your patch (except in the case of those
> legacy ABI's,
> where the below does make the caller and callee ABI match AFAICT):

> ```
>   diff --git a/gcc/calls.cc b/gcc/calls.cc
>   index 01f44734743..0b302f633ed 100644
>   --- a/gcc/calls.cc
>   +++ b/gcc/calls.cc
>   @@ -2970,14 +2970,15 @@ expand_call (tree exp, rtx target, int ignore)
>         we do not have any reliable way to pass unnamed args in
>         registers, so we must force them into memory.  */

>   -  if (type_arg_types != 0
>   +  if ((type_arg_types != 0 || TYPE_NO_NAMED_ARGS_STDARG_P (funtype))
>          && targetm.calls.strict_argument_naming (args_so_far))
>        ;
>      else if (type_arg_types != 0
>              && ! targetm.calls.pretend_outgoing_varargs_named
> (args_so_far))
>        /* Don't include the last named arg.  */
>        --n_named_args;
>   -  else if (TYPE_NO_NAMED_ARGS_STDARG_P (funtype))
>   +  else if (TYPE_NO_NAMED_ARGS_STDARG_P (funtype)
>   +        && ! targetm.calls.pretend_outgoing_varargs_named (args_so_far))
>        n_named_args = 0;
>      else
>        /* Treat all args as named.  */
> ```

> Do you agree that this makes sense (i.e. is there something I'm
> completely missing)?

Yeah, your argument is quite convincing, and the target knobs are indeed
in line with the change you suggest, whereas the current code seems to
deviate from them.

With my ABI designer hat on, however, I see that there's room for ABIs
to make decisions about 0-args stdargs that go differently from stdargs
with leading named args, from prototyped functions, and even from
prototypeless functions, and we might end up needing more knobs to deal
with such custom decisions.  We can cross that bridge if/when we get to
it, though.

> (lm32 mcore msp430 gcn cris fr30 frv h8300 arm v850 rx pru)

Interesting that ppc64le is not on your list.  There's PR107453 about
that, and another thread is discussing a fix for it that is somewhat
different from what you propose (presumably because the way the problem
manifests on ppc64le is different), but it also tweaks expand_call.

I'll copy you when following up there.

-- 
Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
   Free Software Activist                   GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity
Excluding neuro-others for not behaving ""normal"" is *not* inclusive

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] arm: fix c23 0-named-args caller-side stdarg
  2024-03-01  4:38       ` Alexandre Oliva
@ 2024-03-01 14:56         ` Richard Earnshaw (lists)
  2024-03-06 20:28           ` Alexandre Oliva
  0 siblings, 1 reply; 9+ messages in thread
From: Richard Earnshaw (lists) @ 2024-03-01 14:56 UTC (permalink / raw)
  To: Alexandre Oliva, Matthew Malcomson
  Cc: gcc-patches, Joseph Myers, Nick Clifton, Ramana Radhakrishnan,
	Kyrylo Tkachov, Jakub Jelinek

On 01/03/2024 04:38, Alexandre Oliva wrote:
> Hello, Matthew,
> 
> Thanks for the review.

For closure, Jakub has just pushed a patch to the generic code, so I don't think we need this now.

R.

> 
> On Feb 26, 2024, Matthew Malcomson <matthew.malcomson@arm.com> wrote:
> 
>> I think you're right that the AAPCS32 requires all arguments to be passed in
>> registers for this testcase.
>> (Nit on the commit-message: It says that your reading of the AAPCS32
>> suggests
>> that the *caller* is correct -- I believe based on the change you
>> suggested you
>> meant *callee* is correct in expecting arguments in registers.)
> 
> Ugh, yeah, sorry about the typo.
> 
>> The approach you suggest looks OK to me -- I do notice that it doesn't
>> fix the
>> legacy ABI's of `atpcs` and `apcs` and guess it would be nicer to have them
>> working at the same time though would defer to maintainers on how
>> important that
>> is.
>> (For the benefit of others reading) I don't believe there is any ABI concern
>> with this since it's fixing something that is currently not working at
>> all and
>> only applies to c23 (so a change shouldn't have too much of an impact).
> 
>> You mention you chose to make the change in the arm backend rather
>> than general
>> code due to hesitancy to change the generic ABI-affecting code. That makes
>> sense to me, certainly at this late stage in the development cycle.
> 
> *nod* I wrote the patch in the following context: I hit the problem on
> the very first toolchain I started transitioning to gcc-13.  I couldn't
> really fathom the notion that this breakage could have survived an
> entire release cycle if it affected many targets, and sort of held on to
> an assumption that the abi used by our arm-eabi toolchain had to be an
> uncommon one.
> 
> All of this hypothesizing falls apart by the now apparent knowledge that
> the test is faling elsewhere as well, even on other ARM ABIs, it just
> hadn't been addressed yet.  I'm glad we're getting there :-)
> 
>> From a quick check on c23-stdarg-4.c it does look like the below
>> change ends up
>> with the same codegen as your patch (except in the case of those
>> legacy ABI's,
>> where the below does make the caller and callee ABI match AFAICT):
> 
>> ```
>>   diff --git a/gcc/calls.cc b/gcc/calls.cc
>>   index 01f44734743..0b302f633ed 100644
>>   --- a/gcc/calls.cc
>>   +++ b/gcc/calls.cc
>>   @@ -2970,14 +2970,15 @@ expand_call (tree exp, rtx target, int ignore)
>>         we do not have any reliable way to pass unnamed args in
>>         registers, so we must force them into memory.  */
> 
>>   -  if (type_arg_types != 0
>>   +  if ((type_arg_types != 0 || TYPE_NO_NAMED_ARGS_STDARG_P (funtype))
>>          && targetm.calls.strict_argument_naming (args_so_far))
>>        ;
>>      else if (type_arg_types != 0
>>              && ! targetm.calls.pretend_outgoing_varargs_named
>> (args_so_far))
>>        /* Don't include the last named arg.  */
>>        --n_named_args;
>>   -  else if (TYPE_NO_NAMED_ARGS_STDARG_P (funtype))
>>   +  else if (TYPE_NO_NAMED_ARGS_STDARG_P (funtype)
>>   +        && ! targetm.calls.pretend_outgoing_varargs_named (args_so_far))
>>        n_named_args = 0;
>>      else
>>        /* Treat all args as named.  */
>> ```
> 
>> Do you agree that this makes sense (i.e. is there something I'm
>> completely missing)?
> 
> Yeah, your argument is quite convincing, and the target knobs are indeed
> in line with the change you suggest, whereas the current code seems to
> deviate from them.
> 
> With my ABI designer hat on, however, I see that there's room for ABIs
> to make decisions about 0-args stdargs that go differently from stdargs
> with leading named args, from prototyped functions, and even from
> prototypeless functions, and we might end up needing more knobs to deal
> with such custom decisions.  We can cross that bridge if/when we get to
> it, though.
> 
>> (lm32 mcore msp430 gcn cris fr30 frv h8300 arm v850 rx pru)
> 
> Interesting that ppc64le is not on your list.  There's PR107453 about
> that, and another thread is discussing a fix for it that is somewhat
> different from what you propose (presumably because the way the problem
> manifests on ppc64le is different), but it also tweaks expand_call.
> 
> I'll copy you when following up there.
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] arm: fix c23 0-named-args caller-side stdarg
  2024-03-01 14:56         ` Richard Earnshaw (lists)
@ 2024-03-06 20:28           ` Alexandre Oliva
  2024-03-06 20:32             ` Jakub Jelinek
  2024-03-07 10:01             ` Richard Earnshaw (lists)
  0 siblings, 2 replies; 9+ messages in thread
From: Alexandre Oliva @ 2024-03-06 20:28 UTC (permalink / raw)
  To: Richard Earnshaw (lists)
  Cc: Matthew Malcomson, gcc-patches, Joseph Myers, Nick Clifton,
	Ramana Radhakrishnan, Kyrylo Tkachov, Jakub Jelinek

On Mar  1, 2024, "Richard Earnshaw (lists)" <Richard.Earnshaw@arm.com> wrote:

> On 01/03/2024 04:38, Alexandre Oliva wrote:
>> Thanks for the review.

> For closure, Jakub has just pushed a patch to the generic code, so I
> don't think we need this now.

ACK.  I see the c2x-stdarg-4.c test is now passing in our arm-eabi
gcc-13 tree.  Thank you all.

Alas, the same nightly build showed a new riscv fail in c23-stdarg-6.c,
that also got backported to gcc-13.  Presumably it's failing in the
trunk as well, both riscv32-elf and riscv64-elf.

I haven't looked into whether it's a regression brought about by the
patch or just a new failure mode that the new test exposed.  Either way,
I'm not sure whether to link this new failure to any of the associated
PRs or to file a new one, but, FTR, I'm going to look into it.

-- 
Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
   Free Software Activist                   GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity
Excluding neuro-others for not behaving ""normal"" is *not* inclusive

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] arm: fix c23 0-named-args caller-side stdarg
  2024-03-06 20:28           ` Alexandre Oliva
@ 2024-03-06 20:32             ` Jakub Jelinek
  2024-03-07 10:01             ` Richard Earnshaw (lists)
  1 sibling, 0 replies; 9+ messages in thread
From: Jakub Jelinek @ 2024-03-06 20:32 UTC (permalink / raw)
  To: Alexandre Oliva
  Cc: Richard Earnshaw (lists),
	Matthew Malcomson, gcc-patches, Joseph Myers, Nick Clifton,
	Ramana Radhakrishnan, Kyrylo Tkachov

On Wed, Mar 06, 2024 at 05:28:21PM -0300, Alexandre Oliva wrote:
> On Mar  1, 2024, "Richard Earnshaw (lists)" <Richard.Earnshaw@arm.com> wrote:
> 
> > On 01/03/2024 04:38, Alexandre Oliva wrote:
> >> Thanks for the review.
> 
> > For closure, Jakub has just pushed a patch to the generic code, so I
> > don't think we need this now.
> 
> ACK.  I see the c2x-stdarg-4.c test is now passing in our arm-eabi
> gcc-13 tree.  Thank you all.
> 
> Alas, the same nightly build showed a new riscv fail in c23-stdarg-6.c,
> that also got backported to gcc-13.  Presumably it's failing in the
> trunk as well, both riscv32-elf and riscv64-elf.

That is PR114175 I guess.  The test just makes it clear what has been broken
already in GCC 13 there.

	Jakub


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] arm: fix c23 0-named-args caller-side stdarg
  2024-03-06 20:28           ` Alexandre Oliva
  2024-03-06 20:32             ` Jakub Jelinek
@ 2024-03-07 10:01             ` Richard Earnshaw (lists)
  1 sibling, 0 replies; 9+ messages in thread
From: Richard Earnshaw (lists) @ 2024-03-07 10:01 UTC (permalink / raw)
  To: Alexandre Oliva
  Cc: Matthew Malcomson, gcc-patches, Joseph Myers, nickc,
	Ramana Radhakrishnan, Kyrylo Tkachov, Jakub Jelinek

On 06/03/2024 20:28, Alexandre Oliva wrote:
> On Mar  1, 2024, "Richard Earnshaw (lists)" <Richard.Earnshaw@arm.com> wrote:
> 
>> On 01/03/2024 04:38, Alexandre Oliva wrote:
>>> Thanks for the review.
> 
>> For closure, Jakub has just pushed a patch to the generic code, so I
>> don't think we need this now.
> 
> ACK.  I see the c2x-stdarg-4.c test is now passing in our arm-eabi
> gcc-13 tree.  Thank you all.
> 
> Alas, the same nightly build showed a new riscv fail in c23-stdarg-6.c,
> that also got backported to gcc-13.  Presumably it's failing in the
> trunk as well, both riscv32-elf and riscv64-elf.
> 
> I haven't looked into whether it's a regression brought about by the
> patch or just a new failure mode that the new test exposed.  Either way,
> I'm not sure whether to link this new failure to any of the associated
> PRs or to file a new one, but, FTR, I'm going to look into it.
> 

I'd suggest a new pr.  It's easier to track than re-opening an existing on.

R.

> -- 
> Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/ <https://FSFLA.org/blogs/lxo/>
>    Free Software Activist                   GNU Toolchain Engineer
> More tolerance and less prejudice are key for inclusion and diversity
> Excluding neuro-others for not behaving ""normal"" is *not* inclusive


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2024-03-07 10:01 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-11-19  7:32 [PATCH] arm: fix c23 0-named-args caller-side stdarg Alexandre Oliva
2023-12-06  2:27 ` Alexandre Oliva
2024-01-23  8:26   ` Alexandre Oliva
2024-02-26 16:41     ` Matthew Malcomson
2024-03-01  4:38       ` Alexandre Oliva
2024-03-01 14:56         ` Richard Earnshaw (lists)
2024-03-06 20:28           ` Alexandre Oliva
2024-03-06 20:32             ` Jakub Jelinek
2024-03-07 10:01             ` Richard Earnshaw (lists)

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).