From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17975 invoked by alias); 9 Oct 2003 05:02:05 -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 17950 invoked from network); 9 Oct 2003 05:02:04 -0000 Received: from unknown (HELO mx1.redhat.com) (66.187.233.31) by sources.redhat.com with SMTP; 9 Oct 2003 05:02:04 -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 h99524M25673 for ; Thu, 9 Oct 2003 01:02:04 -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 h99524c25872 for ; Thu, 9 Oct 2003 01:02:04 -0400 Received: from localhost.localdomain (vpn50-39.rdu.redhat.com [172.16.50.39]) by pobox.corp.redhat.com (8.12.8/8.12.8) with ESMTP id h99522dT026142; Thu, 9 Oct 2003 01:02:03 -0400 Received: (from kev@localhost) by localhost.localdomain (8.11.6/8.11.6) id h9951vs11015; Wed, 8 Oct 2003 22:01:57 -0700 Date: Thu, 09 Oct 2003 05:02:00 -0000 From: Kevin Buettner Message-Id: <1031009050157.ZM11014@localhost.localdomain> In-Reply-To: Andrew Cagney "Re: [rfa:ppc64] Fix 64-bit PPC ELF function calls" (Oct 6, 6:19pm) References: <3F6E368C.30009@redhat.com> <3F6F388D.5020706@redhat.com> <1031003212231.ZM26624@localhost.localdomain> <3F81EA72.4030601@redhat.com> To: Andrew Cagney , Kevin Buettner Subject: Re: [rfa:ppc64] Fix 64-bit PPC ELF function calls Cc: gdb-patches@sources.redhat.com MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-SW-Source: 2003-10/txt/msg00290.txt.bz2 On Oct 6, 6:19pm, Andrew Cagney wrote: > > I notice some 80+ character lines in ppc64_sysv_abi_push_dummy_call(). > > Could you adjust these so that they're 80 characters or less? > > I'll run the file through gdb_indent.sh, as a separate commit. I see that you've already done it. Don't forget to do it again after you commit your ppc64_sysv_abi_push_dummy_call() changes. > > Also, a minor nit: in the comment... > > > > /* Find a value for the TOC register. Every symbol should have both > > ".FN" and "FN" in the minimal symbol table. "FN" points at the > > F's descriptor, while ".FN" points at the entry point (which > > matches FUNC_ADDR). Need to reverse from FUNC_ADDR back to the > > FN's descriptor address. */ > > > > ...at the beginning of the third line down, shouldn't that be: > > > > FN's descriptor, [...] > > > > If not, what does `F' refer to? > > It's a tipo, thanks. The term "FN" is used in the ABI. Here's another typo: + /* Compute the actual stack space requirements. 48 is lifted + direct from the ABI, it is ment to hold holds things like the s/ment/meant/ And another: + /* WARNING: cagney/2003-09-21: Strictly speaking, this + isn't necessary, unfortunatly, GCC appears to get + struct parameter passing wrong putting odd sized + structs in memory instead of a register. Work + around this by always writing the value to memory. + Fortunatly, doing this simplifies the cost. */ s/Fortunatly/Fortunately/ > ok? Not yet. Please add gdb_assert (tdep->wordsize == 8) somewhere early in ppc64_sysv_abi_push_dummy_call(). I don't mind seeing tdep->wordsize used in the calls to align_up(), align_down(), etc. because this makes it easier to reuse this code (in a copy/paste sense) in the future, but it is a 64-bit ABI and asserting that the wordsize is 8 bytes at some point makes it easier to verify that this function is correct without having to do a non-local analysis to figure out the possible values of tdep->wordsize. .... + /* The address of the top of the parameter save region (typically + points at the local variable region). On entry, the SP is + pointing at this. */ + const CORE_ADDR param_top = sp; I find this name confusing. If I understand the rest of the code correctly, this actually ends up being the bottom of the parameter save area - since the stack grows from high to low addresses. (Of course, if you draw a picture, it might be topmost in the picture...) Anyway, in this instance, I think it's advisable to avoid the names "top" and "bottom" altogether. Perhaps ``param_end'' is a more appropriate name? .... + /* Address, on the stack, of the next general (integer, struct, + float, ...) parameter. */ + CORE_ADDR gparam = 0; + /* Address, on the stack, of the next vector. */ + CORE_ADDR vparam = 0; These are addresses when write_pass == 1, but merely a running total of the number of words needed when write_pass == 0. Calling them addresses is misleading. + /* Go through the argument list twice. + + Pass 1: Figure out how much stack space is required for arguments + and pushed values. + + Pass 2: Replay the same computation but this time also write the + values out to the target. */ Perhaps you could explain the dual roles of gparam and vparam in the above comment? .... + if (!write_pass) + { + vparam = align_down (param_top - vparam, 16); + gparam = align_down (vparam - gparam, 16); + sp = align_down (gparam - 48, 16); + } The ABI says: The parameter save area, which is located at a fixed offset of 48 bytes from the stack pointer, is reserved in each stack frame for use as an argument list. A minimum of 8 doublewords is always reserved. I don't see where you've accounted for this 8 doubleword minimum. Perhaps revise this to something along the following lines? if (!write_pass) { vparam = align_down (param_top - vparam, 16); if (gparam < 8 * tdep->wordsize) gparam = 8 * tdep->wordsize; gparam = align_down (vparam - gparam, 16); sp = align_down (gparam - 48, 16); } I'm not sure this is right either since that alters the vector save area. I don't happen to have a copy of the Altivec ABI though. .... + if (greg <= 10) + { + /* It goes into the register, left aligned (as + far as I can tell). */ I can't find anything in the ABI to contradict this, but I found it surprising that floating point arguments would be left aligned. If I'm not mistaken (for big endian), this'll mean that the the float is stored in the most significant half of the register. Kevin