Message ID | 96c5552629ae8b306cff4b5cef44ba00634276d2.1578824547.git.alistair.francis@wdc.com |
---|---|
State | Committed |
Headers |
Received: (qmail 10094 invoked by alias); 12 Jan 2020 10:40:15 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 10057 invoked by uid 89); 12 Jan 2020 10:40:15 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.6 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3 autolearn=ham version=3.3.1 spammy=HContent-Transfer-Encoding:8bit X-HELO: esa6.hgst.iphmx.com DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=wdc.com; i=@wdc.com; q=dns/txt; s=dkim.wdc.com; t=1578825614; x=1610361614; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=wId+Laet6mw4vDeuuNVX1W0Y5cphO4IAOPYWId/O9yU=; b=XeUUpUSFHukhbsd1IXXmfLwjREivqQDVAxx4lFTEhvWUAbK7+cDjoLHK bAxw7jYR7pxjs5cU1Kufd5SsRAs2ELW+M260D32CaDOr94DxlIBpt1qIa Pe5nDNwTWrGXV9eXlYr8hk7nCy9z9AeBOfWOXw6qa3nlTTMHJhfF4f+Ph E9rh61j8GlQN15jHDkfipGufn/rRexjfgYVjG6miJlQ0ssCm8gTK46Btd XvQ6083ucR5YJH2weSC9yLpvANAlILFWQtbds+Ld7DmGozRUHbSacZK4u wA302JM1IpRrfrIlLByeVtB+fEuNkzqDwVfBDb2IlZjxKdOCGiVBAGVOZ Q==; IronPort-SDR: sXbXOOD6Hu2gxRyXf5eMp3vcjO+3nacGUTf0kM2ao635W+6sbcDPAw0XMqEOlJBP6B6JyY5YJ+ 6CRTdBJam/Acc4YdAsOCcFaLvncKn//8QtBYLA9IqsDi6VXCGqRoDopyOAKYZhBGT+exyK4Q2l fW6XQB9+qj8L7D+wp2+YydpyJYsiWXYSzMersP6Sp7LCiRWsqrPhRHxm09B3k2I9ivgdsJ0o8R zrCO6pnh3QSkxyY44LirH8b5gMenj9Gq8UI+vYFde4GqXD3RFnmneqwIWnJ3W0EYWiBM9PNCkY CEE= IronPort-SDR: QKz7pn1D2CuesDMLpjNJO0qgZH+GfbN+B/3ea8GDPLBLPxszupMSn+MIfQ4k3nnlCfqsx6IMsj KNxgpgU/Zf8ce8/F9TLDR+58EzFhOWrIXvXPF3ZfFixb0KaXU4jx182pFQyvY/3Gn2L1ku8k++ KIekOEabdAN7Ix5UFaytok7EYK5ixyVTnZiEb5XBeIQrSQ2GbRFwGXOZvjoAQd/ddsf4X7CnIr j4QVyB0cCmPBdfW6ACuIG/WHArBSTMmGtzkY1z4PL7n46EAVRxl7XdZkf4lWxPXW0Um9XGzvKd Eipe1me3+2g9gapziLsIAWk3 IronPort-SDR: VaXU+HDy06GfBzvbNS9/83tyzWHR7LtWYBUWSBRa1u462lt6FZltlbxylh7458hasUspQuNVW4 owqqItvc1c5HtU3031zeBiAZWKPJKARRWXMcS7Vkzkei21b2VzX3/EHsPrUsjRjg0N906EjOC+ PUHKBcF/a9/GC5jc9Oey+JpgndON8GT2n3BGiWoXvkrR1MK/vOJlHVe3BtaBA2iH4V/uW7pagI k6deNSlLEQ3cBPV2x8hc5y+xYt607/rSkCEYCHTQEfZNRSCvLwBMh7f5StUUh16cTcI731d3VU IeM= WDCIronportException: Internal From: Alistair Francis <alistair.francis@wdc.com> To: libc-alpha@sourceware.org Cc: arnd@arndb.de, adhemerval.zanella@linaro.org, fweimer@redhat.com, joseph@codesourcery.com, palmerdabbelt@google.com, macro@wdc.com, zongbox@gmail.com, alistair.francis@wdc.com, alistair23@gmail.com Subject: [RFC v6 06/23] sysdeps: Use long types with 64-bit time_t on 32-bit archs Date: Sun, 12 Jan 2020 02:33:47 -0800 Message-Id: <96c5552629ae8b306cff4b5cef44ba00634276d2.1578824547.git.alistair.francis@wdc.com> In-Reply-To: <cover.1578824547.git.alistair.francis@wdc.com> References: <cover.1578824547.git.alistair.francis@wdc.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit |
Commit Message
Alistair Francis
Jan. 12, 2020, 10:33 a.m. UTC
If we are using a 32-bit architecture with a 64-bit time_t let's define __SEM_PAD_BEFORE_TIME as 1. This is because the kernel is expecting a type __kernel_old_time_t which is 32-bit even with a 64-bit time_t. Instead of adding another check, let's just set __SEM_PAD_BEFORE_TIME as that will use a long type instead of long long. --- sysdeps/unix/sysv/linux/bits/sem-pad.h | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
Comments
On Sun, Jan 12, 2020 at 11:40 AM Alistair Francis <alistair.francis@wdc.com> wrote: > > If we are using a 32-bit architecture with a 64-bit time_t let's > define __SEM_PAD_BEFORE_TIME as 1. This is because the kernel is > expecting a type __kernel_old_time_t which is 32-bit even with a > 64-bit time_t. Instead of adding another check, let's just set > __SEM_PAD_BEFORE_TIME as that will use a long type instead of > long long. I think you need to do this differently to avoid truncating the timestamp to 32 bit. The type in the generic kernel headers for 32-bit targets is not '__kernel_old_time_t sem_otime; __kernel_long_t pad' but it is 'unsigned long sem_otime; unsigned long sem_otime_high;'. Both need to be combined to read the 64-bit time_t value. It should be possible to do this in an architecture independent way, but note that the two halves are not always adjacent or in the correct order, and in one case (mips shmbuf) the high word is only 16 bit instead of 32. Arnd > --- > sysdeps/unix/sysv/linux/bits/sem-pad.h | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/sysdeps/unix/sysv/linux/bits/sem-pad.h b/sysdeps/unix/sysv/linux/bits/sem-pad.h > index 566ce039cc..4b419a6100 100644 > --- a/sysdeps/unix/sysv/linux/bits/sem-pad.h > +++ b/sysdeps/unix/sysv/linux/bits/sem-pad.h > @@ -30,4 +30,15 @@ > layout conversions for this structure. */ > > #define __SEM_PAD_AFTER_TIME (__TIMESIZE == 32) > -#define __SEM_PAD_BEFORE_TIME 0 > + > +/* If we are using a 32-bit architecture with a 64-bit time_t let's > + define __SEM_PAD_BEFORE_TIME as 1. This is because the kernel is > + expecting a type __kernel_old_time_t which is 32-bit even with a > + 64-bit time_t. Instead of adding another check, let's just set > + __SEM_PAD_BEFORE_TIME as that will use a long type instead of > + long long. */ > +#if __WORDSIZE == 32 && __TIMESIZE == 64 > +# define __SEM_PAD_BEFORE_TIME 1 > +#else > +# define __SEM_PAD_BEFORE_TIME 0 > +#endif > -- > 2.24.1 >
On Mon, Jan 13, 2020 at 7:41 AM Arnd Bergmann <arnd@arndb.de> wrote: > > On Sun, Jan 12, 2020 at 11:40 AM Alistair Francis > <alistair.francis@wdc.com> wrote: > > > > If we are using a 32-bit architecture with a 64-bit time_t let's > > define __SEM_PAD_BEFORE_TIME as 1. This is because the kernel is > > expecting a type __kernel_old_time_t which is 32-bit even with a > > 64-bit time_t. Instead of adding another check, let's just set > > __SEM_PAD_BEFORE_TIME as that will use a long type instead of > > long long. > > I think you need to do this differently to avoid truncating the timestamp > to 32 bit. The type in the generic kernel headers for 32-bit targets is not > '__kernel_old_time_t sem_otime; __kernel_long_t pad' but it is > 'unsigned long sem_otime; unsigned long sem_otime_high;'. Argh! That is even harder. I think I have updated both sem_otime and sem_ctime to have a high and low 32-bit version. That looks actually pretty straightforward. It does seem like we will also have to stich the two values together as bits/sem.h expects them to just be a time_t. That seems self contained in sysdeps/unix/sysv/linux/semctl.c though. > > Both need to be combined to read the 64-bit time_t value. It should > be possible to do this in an architecture independent way, but note > that the two halves are not always adjacent or in the correct order, > and in one case (mips shmbuf) the high word is only 16 bit instead > of 32. That complicates things. I can probably get away with adding a new define, but that seems like a pain. The best bet looks like allowing arches to set __SEM_PAD_TIME and then having each arch specify their own. Maybe with some sane default and then have MIPS set it's own. Do glibc develepers have a preference here? Alistair > > Arnd > > > --- > > sysdeps/unix/sysv/linux/bits/sem-pad.h | 13 ++++++++++++- > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > diff --git a/sysdeps/unix/sysv/linux/bits/sem-pad.h b/sysdeps/unix/sysv/linux/bits/sem-pad.h > > index 566ce039cc..4b419a6100 100644 > > --- a/sysdeps/unix/sysv/linux/bits/sem-pad.h > > +++ b/sysdeps/unix/sysv/linux/bits/sem-pad.h > > @@ -30,4 +30,15 @@ > > layout conversions for this structure. */ > > > > #define __SEM_PAD_AFTER_TIME (__TIMESIZE == 32) > > -#define __SEM_PAD_BEFORE_TIME 0 > > + > > +/* If we are using a 32-bit architecture with a 64-bit time_t let's > > + define __SEM_PAD_BEFORE_TIME as 1. This is because the kernel is > > + expecting a type __kernel_old_time_t which is 32-bit even with a > > + 64-bit time_t. Instead of adding another check, let's just set > > + __SEM_PAD_BEFORE_TIME as that will use a long type instead of > > + long long. */ > > +#if __WORDSIZE == 32 && __TIMESIZE == 64 > > +# define __SEM_PAD_BEFORE_TIME 1 > > +#else > > +# define __SEM_PAD_BEFORE_TIME 0 > > +#endif > > -- > > 2.24.1 > >
On Thu, Jan 16, 2020 at 5:30 PM Alistair Francis <alistair23@gmail.com> wrote: > > On Mon, Jan 13, 2020 at 7:41 AM Arnd Bergmann <arnd@arndb.de> wrote: > > > > On Sun, Jan 12, 2020 at 11:40 AM Alistair Francis > > <alistair.francis@wdc.com> wrote: > > > > > > If we are using a 32-bit architecture with a 64-bit time_t let's > > > define __SEM_PAD_BEFORE_TIME as 1. This is because the kernel is > > > expecting a type __kernel_old_time_t which is 32-bit even with a > > > 64-bit time_t. Instead of adding another check, let's just set > > > __SEM_PAD_BEFORE_TIME as that will use a long type instead of > > > long long. > > > > I think you need to do this differently to avoid truncating the timestamp > > to 32 bit. The type in the generic kernel headers for 32-bit targets is not > > '__kernel_old_time_t sem_otime; __kernel_long_t pad' but it is > > 'unsigned long sem_otime; unsigned long sem_otime_high;'. > > Argh! That is even harder. > > I think I have updated both sem_otime and sem_ctime to have a high and > low 32-bit version. That looks actually pretty straightforward. > > It does seem like we will also have to stich the two values together > as bits/sem.h expects them to just be a time_t. That seems self > contained in sysdeps/unix/sysv/linux/semctl.c though. Scratch that, sysdeps/unix/sysv/linux/bits/sem.h overwrites it so I don't see how we could stitch the values together. > > > > > Both need to be combined to read the 64-bit time_t value. It should > > be possible to do this in an architecture independent way, but note > > that the two halves are not always adjacent or in the correct order, > > and in one case (mips shmbuf) the high word is only 16 bit instead > > of 32. > > That complicates things. I can probably get away with adding a new > define, but that seems like a pain. The best bet looks like allowing > arches to set __SEM_PAD_TIME and then having each arch specify their > own. Maybe with some sane default and then have MIPS set it's own. This actually isn't bad, I just added this: #define __SEM_NO_PAD (__WORDSIZE == 32 && __TIMESIZE == 64) and forced it to 0 for MIPS. Alistair > > Do glibc develepers have a preference here? > > Alistair > > > > > Arnd > > > > > --- > > > sysdeps/unix/sysv/linux/bits/sem-pad.h | 13 ++++++++++++- > > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > > > diff --git a/sysdeps/unix/sysv/linux/bits/sem-pad.h b/sysdeps/unix/sysv/linux/bits/sem-pad.h > > > index 566ce039cc..4b419a6100 100644 > > > --- a/sysdeps/unix/sysv/linux/bits/sem-pad.h > > > +++ b/sysdeps/unix/sysv/linux/bits/sem-pad.h > > > @@ -30,4 +30,15 @@ > > > layout conversions for this structure. */ > > > > > > #define __SEM_PAD_AFTER_TIME (__TIMESIZE == 32) > > > -#define __SEM_PAD_BEFORE_TIME 0 > > > + > > > +/* If we are using a 32-bit architecture with a 64-bit time_t let's > > > + define __SEM_PAD_BEFORE_TIME as 1. This is because the kernel is > > > + expecting a type __kernel_old_time_t which is 32-bit even with a > > > + 64-bit time_t. Instead of adding another check, let's just set > > > + __SEM_PAD_BEFORE_TIME as that will use a long type instead of > > > + long long. */ > > > +#if __WORDSIZE == 32 && __TIMESIZE == 64 > > > +# define __SEM_PAD_BEFORE_TIME 1 > > > +#else > > > +# define __SEM_PAD_BEFORE_TIME 0 > > > +#endif > > > -- > > > 2.24.1 > > >
On Thu, Jan 16, 2020 at 8:31 AM Alistair Francis <alistair23@gmail.com> wrote: > On Mon, Jan 13, 2020 at 7:41 AM Arnd Bergmann <arnd@arndb.de> wrote: > > On Sun, Jan 12, 2020 at 11:40 AM Alistair Francis <alistair.francis@wdc.com> wrote: > > > > > > If we are using a 32-bit architecture with a 64-bit time_t let's > > > define __SEM_PAD_BEFORE_TIME as 1. This is because the kernel is > > > expecting a type __kernel_old_time_t which is 32-bit even with a > > > 64-bit time_t. Instead of adding another check, let's just set > > > __SEM_PAD_BEFORE_TIME as that will use a long type instead of > > > long long. > > > > I think you need to do this differently to avoid truncating the timestamp > > to 32 bit. The type in the generic kernel headers for 32-bit targets is not > > '__kernel_old_time_t sem_otime; __kernel_long_t pad' but it is > > 'unsigned long sem_otime; unsigned long sem_otime_high;'. > > Argh! That is even harder. > > I think I have updated both sem_otime and sem_ctime to have a high and > low 32-bit version. That looks actually pretty straightforward. > > It does seem like we will also have to stich the two values together > as bits/sem.h expects them to just be a time_t. That seems self > contained in sysdeps/unix/sysv/linux/semctl.c though. > > > > > Both need to be combined to read the 64-bit time_t value. It should > > be possible to do this in an architecture independent way, but note > > that the two halves are not always adjacent or in the correct order, > > and in one case (mips shmbuf) the high word is only 16 bit instead > > of 32. > > That complicates things. I can probably get away with adding a new > define, but that seems like a pain. The best bet looks like allowing > arches to set __SEM_PAD_TIME and then having each arch specify their > own. Maybe with some sane default and then have MIPS set it's own. There are three approaches that we have discussed in the past: 1. Convert the timestamps in place where possible, with special cases for architectures that have the upper halves in non-consecutive locations 2. Create a new set of portable sysvipc data structures for time64 user space, e.g. modeled after the generic 64-bit structures, and convert all fields between the kernel version and the libc version. 3. Keep the current layout but rename the existing time fields and add new 64-bit fields at the end. You can have a look at the musl implementation for the third approach, the main advantage there is that it simplifies conversion between the old and new format for supporting the time32 interfaces on the existing 32-bit architectures as wrappers on top of the tim64 interfaces. There is also a simplified form of 1) that will work on most of the architectures that glibc cares about: i386, armel, rv32le, csky, nds32, ppc32, microblaze-le, sparc, parisc, sh-le and nios2 all have the 'high' fields in the right place so you can simply treat them as 64-bit fields with no padding, while armeb, mips (all of them), s390, microblaze-be, sh-be and m68k get it wrong in one way or another and require would require some special case here. Arnd
On Thu, Jan 16, 2020 at 6:36 PM Arnd Bergmann <arnd@arndb.de> wrote: > > On Thu, Jan 16, 2020 at 8:31 AM Alistair Francis <alistair23@gmail.com> wrote: > > On Mon, Jan 13, 2020 at 7:41 AM Arnd Bergmann <arnd@arndb.de> wrote: > > > On Sun, Jan 12, 2020 at 11:40 AM Alistair Francis <alistair.francis@wdc.com> wrote: > > > > > > > > If we are using a 32-bit architecture with a 64-bit time_t let's > > > > define __SEM_PAD_BEFORE_TIME as 1. This is because the kernel is > > > > expecting a type __kernel_old_time_t which is 32-bit even with a > > > > 64-bit time_t. Instead of adding another check, let's just set > > > > __SEM_PAD_BEFORE_TIME as that will use a long type instead of > > > > long long. > > > > > > I think you need to do this differently to avoid truncating the timestamp > > > to 32 bit. The type in the generic kernel headers for 32-bit targets is not > > > '__kernel_old_time_t sem_otime; __kernel_long_t pad' but it is > > > 'unsigned long sem_otime; unsigned long sem_otime_high;'. > > > > Argh! That is even harder. > > > > I think I have updated both sem_otime and sem_ctime to have a high and > > low 32-bit version. That looks actually pretty straightforward. > > > > It does seem like we will also have to stich the two values together > > as bits/sem.h expects them to just be a time_t. That seems self > > contained in sysdeps/unix/sysv/linux/semctl.c though. > > > > > > > > Both need to be combined to read the 64-bit time_t value. It should > > > be possible to do this in an architecture independent way, but note > > > that the two halves are not always adjacent or in the correct order, > > > and in one case (mips shmbuf) the high word is only 16 bit instead > > > of 32. > > > > That complicates things. I can probably get away with adding a new > > define, but that seems like a pain. The best bet looks like allowing > > arches to set __SEM_PAD_TIME and then having each arch specify their > > own. Maybe with some sane default and then have MIPS set it's own. > > There are three approaches that we have discussed in the past: > > 1. Convert the timestamps in place where possible, with special cases > for architectures that have the upper halves in non-consecutive > locations > 2. Create a new set of portable sysvipc data structures for time64 > user space, e.g. modeled after the generic 64-bit structures, > and convert all fields between the kernel version and the libc > version. > 3. Keep the current layout but rename the existing time fields and > add new 64-bit fields at the end. Ah ok, I prefer option 1 I think. So I guess we will do something like: 1. Define __semid_ds32 in sysdeps/unix/sysv/linux/bits/sem.h based on macros in sem-pad.h - This allows us to have a generic implementation in sysdeps/unix/sysv/linux/bits that can be overwritten by other architectures 2. If 32-bit with 64-bit time_t pass __semid_ds32 into the kernel in sysdeps/unix/sysv/linux/semctl.c - Then reconstruct the user exposed version (unchanged). This is the current struct semid_ds and uses time_t - The problem is not every architecture uses time_t here, that only seems to apply if __TIMESIZE == 32 so we should be ok here. Alistair > > You can have a look at the musl implementation for the third approach, > the main advantage there is that it simplifies conversion between > the old and new format for supporting the time32 interfaces on > the existing 32-bit architectures as wrappers on top of the tim64 > interfaces. > > There is also a simplified form of 1) that will work on most of the > architectures that glibc cares about: i386, armel, rv32le, csky, nds32, > ppc32, microblaze-le, sparc, parisc, sh-le and nios2 all have the 'high' > fields in the right place so you can simply treat them as 64-bit > fields with no padding, while armeb, mips (all of them), s390, > microblaze-be, sh-be and m68k get it wrong in one way or another > and require would require some special case here. > > Arnd
On Fri, 17 Jan 2020, Alistair Francis wrote: > 1. Define __semid_ds32 in sysdeps/unix/sysv/linux/bits/sem.h based on > macros in sem-pad.h The usual rule applies that internal structures generally do not belong in installed headers. sysdeps/unix/sysv/linux/bits/sem.h should define the *public* struct semid_ds. That public definition may eventually vary based on some #if conditions (__USE_TIME_BITS64 or whatever _TIME_BITS=64 causes to be defined - not yet, only once we're actually ready to support _TIME_BITS=64 as a complete, consistent interface). But if the purpose of a definition is to pass into the kernel, rather than as a public API (under a public name) for user programs, it does not belong in an installed header - unless it's e.g. referenced in some other public structure in some way.
diff --git a/sysdeps/unix/sysv/linux/bits/sem-pad.h b/sysdeps/unix/sysv/linux/bits/sem-pad.h index 566ce039cc..4b419a6100 100644 --- a/sysdeps/unix/sysv/linux/bits/sem-pad.h +++ b/sysdeps/unix/sysv/linux/bits/sem-pad.h @@ -30,4 +30,15 @@ layout conversions for this structure. */ #define __SEM_PAD_AFTER_TIME (__TIMESIZE == 32) -#define __SEM_PAD_BEFORE_TIME 0 + +/* If we are using a 32-bit architecture with a 64-bit time_t let's + define __SEM_PAD_BEFORE_TIME as 1. This is because the kernel is + expecting a type __kernel_old_time_t which is 32-bit even with a + 64-bit time_t. Instead of adding another check, let's just set + __SEM_PAD_BEFORE_TIME as that will use a long type instead of + long long. */ +#if __WORDSIZE == 32 && __TIMESIZE == 64 +# define __SEM_PAD_BEFORE_TIME 1 +#else +# define __SEM_PAD_BEFORE_TIME 0 +#endif