Add ipc.h for aarch64

Message ID D06BD949.1758%alan.hayward@arm.com
State Superseded
Headers

Commit Message

Alan Hayward Oct. 21, 2014, 8:35 a.m. UTC
  Copied sysdeps/unix/sysv/linux/mips/bits/ipc.h to (new file)
sysdeps/unix/sysv/linux/aarch64/bits/ipc.h
Did not change any of the contents.

This results in the "mode" file in "ipc_perm" changing from 16bits to
32bits - which matches up with the kernel definition.
Note that all other 64bit BE targets have this set in their own ipc.h

Without this patch, on Aarch64 Big Endian the mode field would end up in
the padding field, resulting in the failure of most LTP semaphore tests.


Regards,
Alan.


Changelog:

2014-10-21 Alan Hayward <alan.hayward@arm.com>

        * sysdeps/unix/sysv/linux/aarch64/bits/ipc.h: New file.





+    unsigned long int __glibc_reserved1;
+    unsigned long int __glibc_reserved2;
+};
  

Comments

Marcus Shawcroft Oct. 21, 2014, 9:52 a.m. UTC | #1
On 21 October 2014 09:35, Alan Hayward <alan.hayward@arm.com> wrote:
> Copied sysdeps/unix/sysv/linux/mips/bits/ipc.h to (new file)

We are currently using sysdeps/unix/sysv/linux/bits/ipc.h, I'd prefer
we took a copy of that file and changed:

unsigned short int mode;            /* Read/write permission.  */
unsigned short int __pad1;
->
unsigned int mode;            /* Read/write permission.  */

Rather than take the other changes present in the mips file.

Cheers
/Marcus
  
Christoph Hellwig Oct. 21, 2014, 10:31 a.m. UTC | #2
On Tue, Oct 21, 2014 at 10:52:44AM +0100, Marcus Shawcroft wrote:
> On 21 October 2014 09:35, Alan Hayward <alan.hayward@arm.com> wrote:
> > Copied sysdeps/unix/sysv/linux/mips/bits/ipc.h to (new file)
> 
> We are currently using sysdeps/unix/sysv/linux/bits/ipc.h, I'd prefer
> we took a copy of that file and changed:
> 
> unsigned short int mode;            /* Read/write permission.  */
> unsigned short int __pad1;
> ->
> unsigned int mode;            /* Read/write permission.  */
> 
> Rather than take the other changes present in the mips file.

Given that Marcus mentioned that all 64-bit BE Linux targets want
a 32-bit mode field, how about making a change to
sysdeps/unix/sysv/linux/bits/ipc.h so that it can be used as-is for all
these targets?
  
Alan Hayward Oct. 21, 2014, 11:01 a.m. UTC | #3
On 21/10/2014 11:31, "Christoph Hellwig" <hch@lst.de> wrote:

>On Tue, Oct 21, 2014 at 10:52:44AM +0100, Marcus Shawcroft wrote:
>> On 21 October 2014 09:35, Alan Hayward <alan.hayward@arm.com> wrote:
>> > Copied sysdeps/unix/sysv/linux/mips/bits/ipc.h to (new file)
>> 
>> We are currently using sysdeps/unix/sysv/linux/bits/ipc.h, I'd prefer
>> we took a copy of that file and changed:
>> 
>> unsigned short int mode;            /* Read/write permission.  */
>> unsigned short int __pad1;
>> ->
>> unsigned int mode;            /* Read/write permission.  */
>> 
>> Rather than take the other changes present in the mips file.
>
>Given that Marcus mentioned that all 64-bit BE Linux targets want
>a 32-bit mode field, how about making a change to
>sysdeps/unix/sysv/linux/bits/ipc.h so that it can be used as-is for all
>these targets?

I did initially think about this, but each architecture seemed to have
subtly different version of the file, even though I think ultimately they
all end up with the same shaped structure. Also, I was shying away from
making a change where I was actively changing many different targets.

I can take a look into creating a unified version.


Alan.
  
Alan Hayward Oct. 21, 2014, 3:30 p.m. UTC | #4
On 21/10/2014 12:01, "Alan Hayward" <alan.hayward@arm.com> wrote:

>
>On 21/10/2014 11:31, "Christoph Hellwig" <hch@lst.de> wrote:
>
>>On Tue, Oct 21, 2014 at 10:52:44AM +0100, Marcus Shawcroft wrote:
>>> On 21 October 2014 09:35, Alan Hayward <alan.hayward@arm.com> wrote:
>>> > Copied sysdeps/unix/sysv/linux/mips/bits/ipc.h to (new file)
>>> 
>>> We are currently using sysdeps/unix/sysv/linux/bits/ipc.h, I'd prefer
>>> we took a copy of that file and changed:
>>> 
>>> unsigned short int mode;            /* Read/write permission.  */
>>> unsigned short int __pad1;
>>> ->
>>> unsigned int mode;            /* Read/write permission.  */
>>> 
>>> Rather than take the other changes present in the mips file.
>>
>>Given that Marcus mentioned that all 64-bit BE Linux targets want
>>a 32-bit mode field, how about making a change to
>>sysdeps/unix/sysv/linux/bits/ipc.h so that it can be used as-is for all
>>these targets?
>
>I did initially think about this, but each architecture seemed to have
>subtly different version of the file, even though I think ultimately they
>all end up with the same shaped structure. Also, I was shying away from
>making a change where I was actively changing many different targets.
>
>I can take a look into creating a unified version.
>

Ok, looking into this a little further:

Power, ia64
*Has own file with __mode_t mode.

Hppa, sparc, s390
*Has own file. If 32bits, Short int mode else __mode_t mode

Mips, alpha
*Has own file with int mode.
*Also, additional fields use unsigned int instead of _uid_t/_gid_t etc


i386/x86_64/x86, arm, m68k, anything else:
*Use default file. Unsigned short int mode



Which breaks down to: 32bit targets use short int, 64bit targets use int.
Expect for the odd case out of x86_64, which always uses short int.

I think therefore we should probably:
1) Add a #if __WORDSIZE == 32 check into the generic ipc.h
2) Remove all specific ipc.h files
3) Add a specific x86_64 version for the odd case out

However, I would like to delay that change to a future patch, as it has
the potential to break in ways I might not immediately catch.

For this patch I would just make the changes Marcus suggested.

Does that make sense?


Alan.
  
Carlos O'Donell Oct. 24, 2014, 2:33 p.m. UTC | #5
On 10/21/2014 11:30 AM, Alan Hayward wrote:
> For this patch I would just make the changes Marcus suggested.
> 
> Does that make sense?

Yes.

Though the cleanup and single ipc.h is where I'd like to see us go.

c.
  
Alan Hayward Nov. 10, 2014, 1:08 p.m. UTC | #6
On 24/10/2014 15:33, "Carlos O'Donell" <carlos@redhat.com> wrote:

>On 10/21/2014 11:30 AM, Alan Hayward wrote:
>> For this patch I would just make the changes Marcus suggested.
>> 
>> Does that make sense?
>
>Yes.
>
>Though the cleanup and single ipc.h is where I'd like to see us go.
>
>c.

I’ve spent some more time investigating this, and I now no longer think we
can easily reduce the number of ipc.h files.

There were a couple of things I didn’t appreciate previously:

* There are 8 architectures which currently use the common file.
* We can’t just start switching between, say __uid_t and unsigned int,
even if in the general case for a particular architecture these types are
exactly the same size.
* A change in a file might not break any test cases, but it’s an ABI
change and could subtly break existing external applications that might
depend on exact structure orders and pad names.
* This will require regressions testing on all architectures, which is
going to very tricky.


I could only reasonably create a new common file if the structure was
identical for many architectures. But we do have subtle differences
between each of the 64bit targets.
i.e. - mips/alpha uses unsigned ints instead of typedefs; hppa has
different explicit named padding; power pc has extra defines at the end of
the file; the __WORDSIZE == 32 checks mean different architectures match
on 32bit than they do on 64bit
This results in no one common format between them. In the end the most
common version of the file is the one which currently common version. Even
if I added #if __WORDSIZE == 32 checks into the common file then I could
break existing 64bit architectures.

For the benefit of reducing a few files, this is going to carry a lot of
hidden risk.



As a reference, here they all are:


	hppa-*-linux-gnu	Not currently functional without patches.
struct ipc_perm
  {
    __key_t __key;			/* Key.  */
    __uid_t uid;			/* Owner's user ID.  */
    __gid_t gid;			/* Owner's group ID.  */
    __uid_t cuid;			/* Creator's user ID.  */
    __gid_t cgid;			/* Creator's group ID.  */
#if __WORDSIZE == 32
    unsigned short int __pad1;
    unsigned short int mode;		/* Read/write permission.  */
    unsigned short int __pad2;
#else
    __mode_t mode;			/* Read/write permission.  */
    unsigned short int __pad2;
#endif
    unsigned short int __seq;		/* Sequence number.  */
    unsigned int __pad3;
    __extension__ unsigned long long int __glibc_reserved1;
    __extension__ unsigned long long int __glibc_reserved2;
  };


	ia64-*-linux-gnu
struct ipc_perm
  {
    __key_t __key;			/* Key.  */
    __uid_t uid;			/* Owner's user ID.  */
    __gid_t gid;			/* Owner's group ID.  */
    __uid_t cuid;			/* Creator's user ID.  */
    __gid_t cgid;			/* Creator's group ID.  */
    __mode_t mode;			/* Read/write permission.  */
    unsigned short int __seq;		/* Sequence number.  */
    unsigned short int __pad1;
    unsigned long int __glibc_reserved1;
    unsigned long int __glibc_reserved2;
  };


	mips-*-linux-gnu
	mips64-*-linux-gnu
	alpha*-*-linux-gnu
struct ipc_perm
  {
    __key_t __key;			/* Key.  */
    unsigned int uid;			/* Owner's user ID.  */
    unsigned int gid;			/* Owner's group ID.  */
    unsigned int cuid;			/* Creator's user ID.  */
    unsigned int cgid;			/* Creator's group ID.	*/
    unsigned int mode;			/* Read/write permission.  */
    unsigned short int __seq;		/* Sequence number.  */
    unsigned short int __pad1;
    unsigned long int __glibc_reserved1;
    unsigned long int __glibc_reserved2;
};


	sparc*-*-linux-gnu
	sparc64*-*-linux-gnu
struct ipc_perm
  {
    __key_t __key;			/* Key.  */
    __uid_t uid;			/* Owner's user ID.  */
    __gid_t gid;			/* Owner's group ID.  */
    __uid_t cuid;			/* Creator's user ID.  */
    __gid_t cgid;			/* Creator's group ID.  */
#if __WORDSIZE == 32
    unsigned short int __pad1;
    unsigned short int mode;		/* Read/write permission.  */
    unsigned short int __pad2;
#else
    __mode_t mode;			/* Read/write permission.  */
    unsigned short int __pad1;
#endif
    unsigned short int __seq;		/* Sequence number.  */
    __extension__ unsigned long long int __glibc_reserved1;
    __extension__ unsigned long long int __glibc_reserved2;
  };


powerpc-*-linux-gnu
powerpc64*-*-linux-gnu
struct ipc_perm
{
__key_t __key;	 /* Key. */
__uid_t uid;	 /* Owner's user ID. */
__gid_t gid;	 /* Owner's group ID. */
__uid_t cuid;	 /* Creator's user ID. */
__gid_t cgid;	 /* Creator's group ID. */
__mode_t mode;	 /* Read/write permission. */
__uint32_t __seq;	 /* Sequence number. */
__uint32_t __pad1;
__uint64_t __glibc_reserved1;
__uint64_t __glibc_reserved2;
};
...also includes extra ipc and semop defines



	s390-*-linux-gnu
	s390x-*-linux-gnu
struct ipc_perm
  {
    __key_t __key;			/* Key.	 */
    __uid_t uid;			/* Owner's user ID.  */
    __gid_t gid;			/* Owner's group ID.  */
    __uid_t cuid;			/* Creator's user ID.  */
    __gid_t cgid;			/* Creator's group ID.	*/
#if __WORDSIZE == 64
    __mode_t mode;			/* Read/write permission.  */
#else
    unsigned short int mode;		/* Read/write permission.  */
    unsigned short int __pad1;
#endif
    unsigned short int __seq;		/* Sequence number.  */
    unsigned short int __pad2;
    unsigned long int __glibc_reserved1;
    unsigned long int __glibc_reserved2;
  };


	aarch64*-*-linux-gnu
struct ipc_perm
  {
    __key_t __key;                     /* Key.  */
    __uid_t uid;                       /* Owner's user ID.  */
    __gid_t gid;                       /* Owner's group ID.  */
    __uid_t cuid;                      /* Creator's user ID.  */
    __gid_t cgid;                      /* Creator's group ID.  */
    unsigned int mode;                 /* Read/write permission.  */
    unsigned short int __seq;          /* Sequence number.  */
    unsigned short int __pad1;
    __syscall_ulong_t __glibc_reserved1;
    __syscall_ulong_t __glibc_reserved2;
  };


Common version

	arm-*-linux-gnueabi
	i[4567]86-*-linux-gnu
	x86_64-*-linux-gnu	Can build either x86_64 or x32
	m68k-*-linux-gnu
	microblaze*-*-linux-gnu
	sh[34]-*-linux-gnu
	tilegx-*-linux-gnu
	tilepro-*-linux-gnu
struct ipc_perm
  {
    __key_t __key;			/* Key.  */
    __uid_t uid;			/* Owner's user ID.  */
    __gid_t gid;			/* Owner's group ID.  */
    __uid_t cuid;			/* Creator's user ID.  */
    __gid_t cgid;			/* Creator's group ID.  */
    unsigned short int mode;		/* Read/write permission.  */
    unsigned short int __pad1;
    unsigned short int __seq;		/* Sequence number.  */
    unsigned short int __pad2;
    __syscall_ulong_t __glibc_reserved1;
    __syscall_ulong_t __glibc_reserved2;
  };



Alan.
  
Carlos O'Donell Nov. 10, 2014, 3:13 p.m. UTC | #7
On 11/10/2014 08:08 AM, Alan Hayward wrote:
> 
> 
> On 24/10/2014 15:33, "Carlos O'Donell" <carlos@redhat.com> wrote:
> 
>> On 10/21/2014 11:30 AM, Alan Hayward wrote:
>>> For this patch I would just make the changes Marcus suggested.
>>>
>>> Does that make sense?
>>
>> Yes.
>>
>> Though the cleanup and single ipc.h is where I'd like to see us go.
>>
> For the benefit of reducing a few files, this is going to carry a lot of
> hidden risk.

That's a fine result to reach, but at least you attempted it 
and I appreciate that. Thanks for going over the arch differences.

Cheers,
Carlos.
  
Marcus Shawcroft Nov. 11, 2014, 3:45 p.m. UTC | #8
On 10 November 2014 13:08, Alan Hayward <alan.hayward@arm.com> wrote:

> I’ve spent some more time investigating this, and I now no longer think we
> can easily reduce the number of ipc.h files.

Alan, Thanks for looking into this.  Your proposed patch is fine.  The
inline patch doesn't apply cleanly, looks like the ARM email system
rewrote it.  Can you send me the patch as an attachment and I'll apply
it.
Thanks
/Marcus
  
Alan Hayward Nov. 11, 2014, 4:11 p.m. UTC | #9
On 11/11/2014 15:45, "Marcus Shawcroft" <marcus.shawcroft@gmail.com> wrote:

>On 10 November 2014 13:08, Alan Hayward <alan.hayward@arm.com> wrote:
>
>> I’ve spent some more time investigating this, and I now no longer think
>>we
>> can easily reduce the number of ipc.h files.
>
>Alan, Thanks for looking into this.  Your proposed patch is fine.  The
>inline patch doesn't apply cleanly, looks like the ARM email system
>rewrote it.  Can you send me the patch as an attachment and I'll apply
>it.
>Thanks
>/Marcus


Attached.
Thanks for applying.

Cheers,


Alan.
  

Patch

diff --git a/sysdeps/unix/sysv/linux/aarch64/bits/ipc.h
b/sysdeps/unix/sysv/linux/aarch64/bits/ipc.h
new file mode 100644
index 0000000..649e74a
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/aarch64/bits/ipc.h
@@ -0,0 +1,54 @@ 
+/* Copyright (C) 1995-2014 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library.  If not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#ifndef _SYS_IPC_H
+# error "Never use <bits/ipc.h> directly; include <sys/ipc.h> instead."
+#endif
+
+#include <bits/types.h>
+
+/* Mode bits for `msgget', `semget', and `shmget'.  */
+#define IPC_CREAT      01000           /* Create key if key does not
exist. */
+#define IPC_EXCL       02000           /* Fail if key exists.  */
+#define IPC_NOWAIT     04000           /* Return error on wait.  */
+
+/* Control commands for `msgctl', `semctl', and `shmctl'.  */
+#define IPC_RMID       0               /* Remove identifier.  */
+#define IPC_SET                1               /* Set `ipc_perm' options.
 */
+#define IPC_STAT       2               /* Get `ipc_perm' options.  */
+#ifdef __USE_GNU
+# define IPC_INFO      3               /* See ipcs.  */
+#endif
+
+/* Special key values. */
+#define IPC_PRIVATE    ((__key_t) 0)   /* Private key.  */
+
+
+/* Data structure used to pass permission information to IPC operations.
*/
+struct ipc_perm
+  {
+    __key_t __key;                     /* Key.  */
+    unsigned int uid;                  /* Owner's user ID.  */
+    unsigned int gid;                  /* Owner's group ID.  */
+    unsigned int cuid;                 /* Creator's user ID.  */
+    unsigned int cgid;                 /* Creator's group ID.  */
+    unsigned int mode;                 /* Read/write permission.  */
+    unsigned short int __seq;          /* Sequence number.  */
+    unsigned short int __pad1;