From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29457 invoked by alias); 6 Oct 2003 19:12:40 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 29450 invoked from network); 6 Oct 2003 19:12:39 -0000 Received: from unknown (HELO mx1.redhat.com) (66.187.233.31) by sources.redhat.com with SMTP; 6 Oct 2003 19:12:39 -0000 Received: from int-mx1.corp.redhat.com (int-mx1.corp.redhat.com [172.16.52.254]) by mx1.redhat.com (8.11.6/8.11.6) with ESMTP id h96JCd103137 for ; Mon, 6 Oct 2003 15:12:39 -0400 Received: from pobox.corp.redhat.com (pobox.corp.redhat.com [172.16.52.156]) by int-mx1.corp.redhat.com (8.11.6/8.11.6) with ESMTP id h96JCdc09912; Mon, 6 Oct 2003 15:12:39 -0400 Received: from localhost.localdomain (vpn50-46.rdu.redhat.com [172.16.50.46]) by pobox.corp.redhat.com (8.12.8/8.12.8) with ESMTP id h96JCbbe003893; Mon, 6 Oct 2003 15:12:37 -0400 Received: (from kev@localhost) by localhost.localdomain (8.11.6/8.11.6) id h96JCVi12231; Mon, 6 Oct 2003 12:12:31 -0700 Date: Mon, 06 Oct 2003 19:12:00 -0000 From: Kevin Buettner Message-Id: <1031006191231.ZM12230@localhost.localdomain> In-Reply-To: Andrew Cagney "Re: [rfa?] Implement ppc32 SYSV {extract,store} return value" (Oct 4, 1:43pm) References: <3F68D829.6010001@redhat.com> <1030922215845.ZM29725@localhost.localdomain> <3F7F06D8.9000702@redhat.com> To: Andrew Cagney , Kevin Buettner Subject: Re: [rfa?] Implement ppc32 SYSV {extract,store} return value Cc: gdb-patches@sources.redhat.com, Jason R Thorpe MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-SW-Source: 2003-10/txt/msg00112.txt.bz2 On Oct 4, 1:43pm, Andrew Cagney wrote: > > Anyway, your patch looks okay to me. Feel free to check it in. > > Here is a revised version. It's now implemented using a wrapped > ..._return_value. > > Still ok? Please fix the following: + if (TYPE_LENGTH (type) <= 8) + { + if (outval) + { + /* This matches SVr4 PPC, it does not match gcc. */ + /* The value is padded out to 8 bytes and then loaded, as + two "words" into r3/r3. */ The comment should say r3/r4, not r3/r3. Likewise here: + if (inval) + { + /* This matches SVr4 PPC, it does not match gcc. */ + /* The value is padded out to 8 bytes and then loaded, as + two "words" into r3/r3. */ In a comment prior to do_ppc_sysv_return_value(), please specify precisely which ABIs this function covers. The function name implies that it's only SysV, but it looks to me like it's for SysV, Altivec, and e500. I don't think it's worth making the function name more verbose, but I do think it's important to list the other ABIs that someone looking at this function needs to consider. Otherwise, a lot of it doesn't make sense. I'm puzzled by the following clause: + if (TYPE_LENGTH (type) == 8 + && TYPE_CODE (type) == TYPE_CODE_ARRAY + && TYPE_VECTOR (type) + && tdep->ppc_ev0_regnum >= 0) + { + if (outval) + { + /* e500 places the return value in "ev2". */ + regcache_cooked_read (regcache, tdep->ppc_ev0_regnum + 2, outval); + } + if (inval) + { + /* e500 places the return value in "ev2". */ + regcache_cooked_write (regcache, tdep->ppc_ev0_regnum + 2, inval); + } + return RETURN_VALUE_REGISTER_CONVENTION; + } I realize that this was lifted from code that existed elsewhere before, but after comparing this to the e500 ABI doc that I have, this doesn't look right. The ABI says: Functions shall return values of 64-bit DSP types (__ev64_opaque__) in r3. IIRC, ev3 corresponds to the 64-bit r3, right? So, shouldn't the above be returning the value in ev3 (rather than ev2)? Or am I missing something? This suggests that for each clause, it'd be useful to have a comment listing which ABIs are (or are not) covered by that clause along with additional descriptive text describing how the code corresponds to the ABI for the non-obvious cases. E.g, for the above, I'd get rid of the two ``e500 places the return value in "ev2".'' comments and instead place something like this just above the ``if (outval)'' statement: /* The e500 ABI places return values for the 64-bit DSP types (__ev64_opaque__) in r3. However, in GDB-speak, ev3 corresponds to the entire r3 value for e500, whereas r3 only corresponds to the lower 32-bits. So place the 64-bit DSP type's value in ev3. */ Hmm... I just realized that I don't have the Altivec ABI doc. Do you have a pointer? Thanks, Kevin P.S. For ppc_sysv_abi_use_struct_convention(), I was considering asking you to organize it as follows: int ppc_sysv_abi_use_struct_convention (int gcc_p, struct type *type) { int is_struct_return; is_struct_return = (some concise logical expression); /* Verify that the ``is_struct_return'' calculation matches the value return implementation. */ assert (is_struct_return == (do_ppc_sysv_return_value (type, NULL, NULL, NULL, 0) == RETURN_VALUE_STRUCT_CONVENTION)); return is_struct_return; } I decided against asking you to do this for the PPC SysV ABI because, after again looking at the ABI, I concluded that more work is required to verify the correctness of ``some concise logical expression'' wrt the ABI than it was to do this verification by looking at do_ppc_sysv_return_value(). I don't think I'd care to conclude that this will always (or even usually) be the case after only considering a few examples.