From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17528 invoked by alias); 8 Nov 2008 00:07:18 -0000 Received: (qmail 17084 invoked by uid 22791); 8 Nov 2008 00:07:17 -0000 X-Spam-Check-By: sourceware.org Received: from smtp-out.google.com (HELO smtp-out.google.com) (216.239.45.13) by sourceware.org (qpsmtpd/0.31) with ESMTP; Sat, 08 Nov 2008 00:06:25 +0000 Received: from spaceape7.eur.corp.google.com (spaceape7.eur.corp.google.com [172.28.16.141]) by smtp-out.google.com with ESMTP id mA806Mrm020682 for ; Fri, 7 Nov 2008 16:06:22 -0800 Received: from rv-out-0708.google.com (rvbk29.prod.google.com [10.140.87.29]) by spaceape7.eur.corp.google.com with ESMTP id mA806Jw7024632 for ; Fri, 7 Nov 2008 16:06:20 -0800 Received: by rv-out-0708.google.com with SMTP id k29so1842763rvb.30 for ; Fri, 07 Nov 2008 16:06:19 -0800 (PST) MIME-Version: 1.0 Received: by 10.140.142.11 with SMTP id p11mr2092772rvd.276.1226102779324; Fri, 07 Nov 2008 16:06:19 -0800 (PST) In-Reply-To: <4914ABC8.7030908@oarcorp.com> References: <4911CFB1.40006@oarcorp.com> <49148DF5.2060301@oarcorp.com> <20081107190039.GA25908@caradoc.them.org> <4914ABC8.7030908@oarcorp.com> Date: Sat, 08 Nov 2008 00:07:00 -0000 Message-ID: Subject: Re: Add SystemV IPC drivers to PSIM From: Doug Evans To: Joel Sherrill Cc: "gdb-patches@sourceware.org" Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes Mailing-List: contact gdb-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-owner@sourceware.org X-SW-Source: 2008-11/txt/msg00070.txt.bz2 On Fri, Nov 7, 2008 at 12:57 PM, Joel Sherrill wrote: > Daniel Jacobowitz wrote: >> >> On Fri, Nov 07, 2008 at 12:50:29PM -0600, Joel Sherrill wrote: >> >>> >>> Most of it is boilerplate for adding a device. Looking >>> at the ChangeLog, Andrew hasn't done anything >>> except commit someone else's patch in a few years. >>> Is anyone other than Andrew capable of reviewing it? >>> >> >> You might try asking H-P (Hans-Peter Nilsson ). >> Beyond that, I'm not sure what to say; no one is really taking care of >> these simulators recently. >> >> > > Thanks. I have emailed him. > > Hopefully we can turn up someone. If not, what's next? > We are using these patches in the RTEMS GDB RPMs and > reporting test results to gcc-testresults regularly with these > simulators. Hi. Once upon a time I hacked in the sim tree. What kind of review is needed? I looked over the patch. It's pretty self-contained so the risk is pretty low, and I trust you. :-) For reference: http://sourceware.org/ml/gdb/2008-09/msg00047.html The coding style seems to follow the psim style with maybe one caveat. There's very little pretty-alignment like this in psim: +typedef struct _hw_sem_device { + unsigned_word physical_address; + key_t key; + int id; + int initial; + int count; +} hw_sem_device; I'd prefer to stick with: +typedef struct _hw_sem_device { + unsigned_word physical_address; + key_t key; + int id; + int initial; + int count; +} hw_sem_device; The indentation is off here and in another place: +static void +hw_sem_init_data(device *me) +{ + hw_sem_device *sem = (hw_sem_device*)device_data(me); + const device_unit *d; + int status; +#if !HAS_UNION_SEMUN + union semun { + int val; + struct semid_ds *buf; + unsigned short int *array; +#if defined(__linux__) + struct seminfo *__buf; +#endif + } ; +#endif + union semun help; I can't tell from reading the code what will break if sizeof(int) != 4. How about a comment? [lots may break, but why check for it here is what I'm getting at] Plus could "typing problem" be more specific? + if (sizeof(int) != 4) + error("hw_sem_init_data() typing problem\n"); I don't have an answer for this, but I don't see any other hw*.c verifying the address either: + /* do we need to worry about out of range addresses? */ I think there's a cut-n-paste error in the sem docs. Should the second 0xc0000000 be 0xfff00000? I don't completely understand the device configuration syntax, but the code checks for "value" yet you're using "reg" in the docs. Is that a problem? [perhaps just more of the same cut-n-paste error of course] Maybe something like -o '/sim@0xfff00000/value 0' or some such instead of -o '/sem@0xfff00000/reg 0xfff00000 0x80000' ? + Configure a UNIX semaphore using key 0x12345678 mapped into psim + address space at 0xfff00000: + + | -o '/sem@0xfff00000/reg 0xfff00000 0x80000' \ + | -o '/sem@0xfff00000/key 0x12345678' \ + + sim/ppc/run -o '/#address-cells 1' \ + -o '/sem@0xc0000000/reg 0xc0000000 0x80000' \ + -o '/sem@0xc0000000/key 0x12345678' ../psim-hello/hello Other than these nits, it's ok with me (fwiw of course).