Hello, Matthew, Thanks for the review.
On Feb 26, 2024, Matthew Malcomson <matthew.malcom...@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
