Message ID | D06BD949.1758%alan.hayward@arm.com |
---|---|
State | New |
Headers | show |
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
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?
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.
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.
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.
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.
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.
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
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.
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;