From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id 55A2C3858D39 for ; Fri, 1 Mar 2024 14:56:44 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 55A2C3858D39 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=arm.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 55A2C3858D39 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=217.140.110.172 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1709305007; cv=none; b=otY/maT9b0+DeP3X1sMGS1x2apT8TxXx7qT/6manYRFGUD3wp7QUAOTQCZNrvFAhi2/fzPkQNKrhI5BOjQ0k4ge3loTdMXQf/z244VjHjZUDI13011VscjSSGPeblcxCShYwfUTebVAPsbeTcw1PiOb4/p4XgiDre9eAPsvsezI= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1709305007; c=relaxed/simple; bh=qz1J/Nk+KG3qfMEjaU1O7fTutlDcyKxV5zqCPvJ7Alg=; h=Message-ID:Date:MIME-Version:Subject:To:From; b=Y1eOBJjBfwe5TmGOpKQ2In+VKZUIkAR7jEf41BiVA1GqEQRpJnGn4gdFhJggoaGrj58Ea0qaGuT0ib1w4zlFJ9f7lg6nAoLU3uBlGu1+kFnPyOkJrWFl/KSbFTDpfcJsQlqVCcvjt+uwrvSowmJ2vLJxWVfHDaZuwr0O1ayghsk= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id F2E5F1FB; Fri, 1 Mar 2024 06:57:21 -0800 (PST) Received: from [10.2.78.54] (e120077-lin.cambridge.arm.com [10.2.78.54]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 85C8D3F73F; Fri, 1 Mar 2024 06:56:42 -0800 (PST) Message-ID: Date: Fri, 1 Mar 2024 14:56:41 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] arm: fix c23 0-named-args caller-side stdarg Content-Language: en-GB To: Alexandre Oliva , Matthew Malcomson Cc: gcc-patches@gcc.gnu.org, Joseph Myers , Nick Clifton , Ramana Radhakrishnan , Kyrylo Tkachov , Jakub Jelinek References: From: "Richard Earnshaw (lists)" In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-3496.1 required=5.0 tests=BAYES_00,BODY_8BITS,GIT_PATCH_0,KAM_DMARC_NONE,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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 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. >