Re: Add SystemV IPC drivers to PSIM

[Doug Evans <dje@google.com>]

On Fri, Nov 7, 2008 at 12:57 PM, Joel Sherrill
<joel.sherrill@oarcorp.com> 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 <hp@axis.com>).
>> 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).