From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7502 invoked by alias); 10 Oct 2003 07:29:35 -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 7495 invoked from network); 10 Oct 2003 07:29:33 -0000 Received: from unknown (HELO mx1.redhat.com) (66.187.233.31) by sources.redhat.com with SMTP; 10 Oct 2003 07:29:33 -0000 Received: from int-mx2.corp.redhat.com (nat-pool-rdu-dmz.redhat.com [172.16.52.200] (may be forged)) by mx1.redhat.com (8.11.6/8.11.6) with ESMTP id h9A7TXM03013 for ; Fri, 10 Oct 2003 03:29:33 -0400 Received: from potter.sfbay.redhat.com (potter.sfbay.redhat.com [172.16.27.15]) by int-mx2.corp.redhat.com (8.11.6/8.11.6) with ESMTP id h9A7TWD31788 for ; Fri, 10 Oct 2003 03:29:32 -0400 Received: from cygbert.vinschen.de (vpn50-15.rdu.redhat.com [172.16.50.15]) by potter.sfbay.redhat.com (8.11.6/8.11.6) with ESMTP id h9A7TUi30196 for ; Fri, 10 Oct 2003 00:29:30 -0700 Received: by cygbert.vinschen.de (Postfix, from userid 500) id 1FA9D58047; Fri, 10 Oct 2003 09:29:29 +0200 (CEST) Date: Fri, 10 Oct 2003 07:29:00 -0000 From: Corinna Vinschen To: gdb-patches@sources.redhat.com Subject: Re: [RFA] sh-tdep.c (sh_use_struct_convention): Restructure and fix Message-ID: <20031010072929.GF14344@cygbert.vinschen.de> Reply-To: gdb-patches@sources.redhat.com Mail-Followup-To: gdb-patches@sources.redhat.com References: <20031004113939.GK11435@cygbert.vinschen.de> <16261.53197.951881.194874@localhost.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <16261.53197.951881.194874@localhost.redhat.com> User-Agent: Mutt/1.4.1i X-SW-Source: 2003-10/txt/msg00354.txt.bz2 On Thu, Oct 09, 2003 at 05:14:53PM -0400, Elena Zannoni wrote: > Corinna Vinschen writes: > > Hi, > > > > the below patch straightens out sh_use_struct_convention() so that it > > allows a far better readability than before, especially by allowing > > a bunch of comments spread out through the code. > > > > I just added a detailed comment. Does that match what you implemented? > I'd prefer the use of 'aggregate' instead of 'struct' in your comments. Yes, thanks, I saw the comment. It's enlightening. However, the first sentence seems to be a copy/paste hangover: /* Should call_function allocate stack space for a struct return? And even though I have to admit, that I'm not 100% sure (perhaps I miss a case) I think the implementation should match at least 99% of the description. The difference between the old and the new code is given by allowing 4 byte structs (erm, aggregates) with more than one element, but a size of 4 byte for the first element. This sounds somewhat weird, but that's exactly the case if the 4 byte agregate is a bitfield or contains a bitfield. So the change in this patch solves exactly these bitfields as return type problem. > > Additionally it fixes one bug: A struct of lenght 4 bytes, which > > consists of only a bitfield, is returned in register r0, not on the > > stack using the struct convention. So far, GDB got that wrong. > > Is there a test case that was failing? If not, it should be added. Yes, testcases which uncover that problem exist in call-ar-st and call-rt-st. Actually the whole change was a result of these testcases. I saw the bitfield problem but I found the former one-expression evaluation very unreadable. So, first I straightened out the expression, then I added the bitfield case. > > + if (len != 1 && len != 2 && len != 4 && len != 8) > > + return 1; > > + /* Structs with more than 1 fields use struct convention, if... */ > > + if (nelem != 1) > > + { > > + /* ... they are 1 or 2 bytes in size (e.g. struct of two chars)... */ > > + if (len != 4 && len != 8) > > Can you just say len == 1 or len == 2 so that it matches your comment? Sure. No problem to change this. > Wait, this contradicts what the comments I just added say: > > For example, a 2-byte aligned structure with size 2 bytes has the > same size and alignment as a short int, and will be returned in R0. > > Which is correct? Both. An aggregate of size 1 or 2 byte with more than 1 element is not correctly alligned so it will not be returned in R0. > > + /* ... or, if the struct is 4 or 8 bytes and the first field is > > + not of size 4 bytes. Note that this also covers structs with > > + bitfields. */ > > + if (TYPE_LENGTH (TYPE_FIELD_TYPE (type, 0)) != 4) > > I am not sure I understand this one, that's why asked a pointer to a > test case. It seems to contradict the following, i.e. it should still > be in registers, or maybe I don't understand the language: > > When an aggregate type is returned in R0 and R1, R0 contains the first > four bytes of the aggregate, and R1 contains the remainder. If the size > of the aggregate type is not a multiple of 4 bytes, the aggregate is > tail-padded up to a multiple of 4 bytes. The value of the padding is > undefined. Is my above description better? The code is identical to the former implementation except for the 4 byte bitfield case. That one is now covered here. I have not attached a new patch, but I've noted to change "struct" to "aggregate" in the comments and the if (len != 4 && len != 8) to if (len == 1 || len == 2) Corinna -- Corinna Vinschen Cygwin Developer Red Hat, Inc.