From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 5B0513858CDB for ; Sat, 8 Oct 2022 04:20:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 5B0513858CDB Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-673--__8cE1YP-GmhnYqY3RLRA-1; Sat, 08 Oct 2022 00:20:34 -0400 X-MC-Unique: -__8cE1YP-GmhnYqY3RLRA-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 1C53A811E67; Sat, 8 Oct 2022 04:20:34 +0000 (UTC) Received: from f35-zws-1 (unknown [10.2.16.76]) by smtp.corp.redhat.com (Postfix) with ESMTPS id A0BEBAE7C4; Sat, 8 Oct 2022 04:20:33 +0000 (UTC) Date: Fri, 7 Oct 2022 21:20:32 -0700 From: Kevin Buettner To: Carl Love via Gdb-patches Cc: Carl Love , Ulrich Weigand Subject: Re: [PATCH 1/2] PowerPC, function ppc64_sysv_abi_return_value add missing return value convention Message-ID: <20221007212032.1944919c@f35-zws-1> In-Reply-To: <56392545859f7e949ee1b8fa2198f3928f714bad.camel@us.ibm.com> References: <28ce795ca489f69829207b2a7a535cf7f77f6dd8.camel@us.ibm.com> <8decd662134d57e8caf43960a1cdc47723e2bfe3.camel@us.ibm.com> <79d82ed277308ed5ce312bff398e770ab234390a.camel@us.ibm.com> <63f21a897f452d81a73fb386cb99110a359ef0b7.camel@de.ibm.com> <97275f61ef101a12cde8e5a45008ed8e479424eb.camel@us.ibm.com> <5a34aaeab59f0099b915d1780c701284a6cf691e.camel@us.ibm.com> <8aa882863b2f4cef38c22386387c5705bf63c3d5.camel@de.ibm.com> <56392545859f7e949ee1b8fa2198f3928f714bad.camel@us.ibm.com> Organization: Red Hat MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.5 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.6 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 08 Oct 2022 04:20:37 -0000 Hi Carl, See my comments below... On Thu, 06 Oct 2022 09:37:10 -0700 Carl Love via Gdb-patches wrote: > GDB maintainers: > > The following PowerPC specific patch fixes an issue of GDB reporting a > bogus return value for functions that return a non-trivial value. The > bogus return values result in five testcase failures for test > gdb.cp/non-trivial-retval.exp. The issue is the function > ppc64_sysv_abi_return_value does not return the correct value when the > valtype->code() is TYPE_CODE_STRUCT and the language_pass_by_reference > is not trivially_copyable. This patch adds the needed code to return > RETURN_VALUE_STRUCT_CONVENTION in these cases. > > The testcase gdb.cp/non-trivial-retval.exp still fails as gdb now > correctly reports "Cannot determine contents" instead of the expected > values, which is correct in this case. The PowerPC ABI uses passes the > return buffer address in r3. The value of r3 is valid on entry to the > function but the PowerPC ABI does not guarantee it will not be changed > in the function. Hence the contents of r3 is not reliable on exit from > the function. This issue will be addressed by the next patch in this > patch series. > > The patch has been tested on PowerPC and on Intel X86-64 with no > regression failures. > > Please let me know if this patch is acceptable for the GDB mainline. > Thanks. > > Carl Love > > ----------------------------------------- > PowerPC, function ppc64_sysv_abi_return_value add missing return value convention > > This patch address five testcase failures in gdb.cp/non-trivial-retval.exp. > The following commit resulted in the five testcases failures on PowerPC. The > value returned by the function is being reported incorrectly. > > commit b1718fcdd1d2a5c514f8ee504ba07fb3f42b8608 > Author: Andrew Burgess > Date: Mon Dec 13 16:56:16 2021 +0000 > > gdb: on x86-64 non-trivial C++ objects are returned in memory > > Fixes PR gdb/28681. It was observed that after using the `finish` > command an incorrect value was displayed in some cases. Specifically, > this behaviour was observed on an x86-64 target. > > The function: > > enum return_value_convention > ppc64_sysv_abi_return_value (struct gdbarch *gdbarch, struct value *function, > struct type *valtype, struct regcache *regcache, > gdb_byte *readbuf, const gdb_byte *writebuf) > > should return RETURN_VALUE_STRUCT_CONVENTION if the valtype->code() is > TYPE_CODE_STRUCT and if the language_pass_by_reference is not > trivially_copyable. > > This patch adds the need code to return the value s/need/needed/ > RETURN_VALUE_STRUCT_CONVENTION in the case of this case. s/the case of // > > With this patch, the five test cases still fail but with the message "Value > returned has type: A. Cannot determine contents". The PowerPC ABI stores the > address of the buffer containing the function return value in register r3 on > entry to the function. However, the PowerPC ABI does not guarentee that r3 > will not be modified in the function. So when the function returns, the return > buffer address cannot be reliably obtained from register r3. Thus the message > "Cannot determine contents" is appropriate in this case. > --- > gdb/ppc-sysv-tdep.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/gdb/ppc-sysv-tdep.c b/gdb/ppc-sysv-tdep.c > index f57c261d9dc..14effb93210 100644 > --- a/gdb/ppc-sysv-tdep.c > +++ b/gdb/ppc-sysv-tdep.c > @@ -2099,6 +2099,10 @@ ppc64_sysv_abi_return_value (struct gdbarch *gdbarch, struct value *function, > return RETURN_VALUE_REGISTER_CONVENTION; > } > > + if (!language_pass_by_reference (valtype).trivially_copyable > + && valtype->code () == TYPE_CODE_STRUCT) > + return RETURN_VALUE_STRUCT_CONVENTION; > + > /* In the ELFv2 ABI, aggregate types of up to 16 bytes are > returned in registers r3:r4. */ > if (tdep->elf_abi == POWERPC_ELF_V2 > -- > 2.31.1 This change looks good to me, but note the tweaks to the commit log remarks above. Kevin P.S. I'm still looking at part 2.