[PATCH 2/3] msqid: Provide internal copy of struct __msqid64_ds
Lukasz Majewski
lukma@denx.de
Fri Nov 6 17:15:03 GMT 2020
Hi Adhemerval,
> On 04/11/2020 10:30, Lukasz Majewski wrote:
> > Hi Adhemerval,
> >
> >> On 27/10/2020 19:10, Lukasz Majewski wrote:
> >>> After the commit SHA1: 3283f711132eaadc4f04bd8c1d84c910c29ba
> >>> "sysv: linux: Add 64-bit time_t variant for msgctl"
> >>>
> >>> The glibc internal, 64 bit supporting version of struct
> >>> __msqid64_ds has been exported.
> >>>
> >>> This approach has issue when one would like to add Y2038 support
> >>> to glibc. The problem is that both struct msqid_ds and
> >>> __msqid64_ds rely on exported implementation of the msg.h header
> >>> (./sysvipc/sys/msg.h) which unconditionally includes
> >>> ./sysdeps/unix/sysv/linux/bits/msq.h which in turn includes
> >>> ./sysdeps/unix/sysv/linux/bits/types/struct_msqid_ds.h
> >>>
> >>> As a result the externally exported struct msqid_ds.h is used
> >>> internally by glibc. Problem starts when one wants to extend this
> >>> struct with 64 bit time support:
> >>>
> >>> | In file included from ../sysdeps/unix/sysv/linux/bits/msq.h:28,
> >>> | from ../sysvipc/sys/msg.h:30,
> >>> | from
> >>> ../sysdeps/unix/sysv/linux/include/sys/msg.h:2, |
> >>> from ../include/sys/msg.h:1, | from
> >>> ../sysdeps/unix/sysv/linux/msgctl.c:19: |
> >>> ../sysdeps/unix/sysv/linux/bits/types/struct_msqid_ds.h:31:6:
> >>> error: "__USE_TIME_BITS64" is not defined, evaluates to 0
> >>> [-Werror=undef] | # if __USE_TIME_BITS64 |
> >>> ^~~~~~~~~~~~~~~~~
> >>>
> >>>
> >>> To fix this issue - the internal,for glibc, copy of struct
> >>> __msqid64_ds has been re-introduced. This will allow clear
> >>> separation between exported and internal glibc types.
> >>>
> >>> Moreover, the __TIMESIZE based alias shall be internal to glibc.
> >>>
> >>
> >> I don think the definition duplication is the correct approach,
> >
> > Up till now my Y2038 supporting code uses duplication of exported
> > and internal structs [1].
> >
> > For example in-glibc we do use struct __timespec64, which matches
> > exported (and properly adjusted when #ifdef __USE_TIME_BITS64)
> > struct timespec [2].
> >
> > The same situation is with time_t and struct timeval.
>
> The drawback of using this strategy is exactly what you pointed out:
> the exported ABI is different than the one used internally which
> require duplicate definition (which adds extra effort to keep both
> in sync), one must take care to avoid use __USE_TIME_BITS64 internally
> which might either break the build or generate wrong code, and it
> requires to change over multiple files for full y2038 support
> (specially in the case for arch-specific structs).
Ok.
>
> My understanding of y2038 of moving to platform neutral definition
> on specific file is just to simplify it: no need to keep multiple
> architecture specific definitions and the same definition is used
> internally and externally.
So if I understood you correctly - lets consider the struct timespec
(exported definition) and struct __timespec64 (internal one).
1. We shall export struct __timespec64:
./time/bits/types/struct___timespec64.h
2. The above header shall be re-included by glibc -
./include/bits/types/struct_timespec.h shall have just #include <....
struct___timespec64.h>
3. The exported header (from point 1) shall have:
#if __TIMESIZE == 32 && defined __USE_TIME_BITS64
# define timespec __timespec64
#endif
So the 'struct timespec' in user program would then be aliased to
'struct __timespec64' (the same, which was used in glib internally).
>
> >
> >> it is
> >> not required for other ABIs which vary depending of the compiler
> >> flags (such as LFS). Also, __TIMESIZE is already defined on an
> >> installed header (timesize.h) and it defines a macro which within
> >> the glibc namespace.
> >
> > Up till now the aliasing (like in [3]) was done in internal glibc
> > headers.
> >
> >>
> >> The redirection is different than LFS since we don't have the
> >> symbol64 for all ABIs, so we will need a different way to redirect
> >> the implementation.
> >
> > Ok.
> >
> >>
> >> What I think it should be done it to move the __TIMESIZE
> >> definition to exported header that actually includes and use the
> >> redirect macros we have for LFS support. Something as:
> >>
> >> 1. On sysdeps/unix/sysv/linux/bits/msq.h
> >>
> >> #if __TIMESIZE == 64 || !defined __USE_TIME_BITS64
> >> # include <bits/types/struct_msqid_ds.h>
> >> #else
> >> # include <bits/types/struct_msqid_ds64.h>
> >> #endif
> >>
> >> This make currently 64-bit time ABIs or 32-bit ABIs using
> >> 32-bit flags to include the architecture defined struct_msqid_ds.h
> >>
> >
> > As I've stated before - I've been avoiding using __TIMESIZE in
> > exported headers. Instead, I was only using __USE_TIME_BITS64 there.
> >
> > IIRC that stem from the very early discussion with Joseph about the
> > generic shape of Y2038 support.
> >
> > I can adjust my WIP patch supporting Y2038 [1], but I would need a
> > consensus to avoid rewritting the code in the future.
>
> The __TIMESIZE is used now internally to define whether the ABI has
> originally support for 32-bit time_t and it is an installed header.
> It is also used on *installed* headers to define Linux specific ABI,
> for instance on bits/socket-constants.h introduced on riscv32 support.
>
Ok. I've overlooked the __TIMESIZE ifdefs in exported
sysdeps/unix/sysv/linux/powerpc/bits/types/struct_* files.
If you think that __TIMESIZE check shall be added along the
__USE_TIME_BITS64 to redirections then I'm fine with it:
In my code I do have:
extern int msgctl (int __msqid, int __cmd, struct msqid_ds *__buf)
#ifdef __USE_TIME_BITS64
# ifdef __REDIRECT_NTH
extern int __REDIRECT_NTH (msgctl (int __msqid,
int __cmd,
struct msqid64_ds *__buf),
__msgctl64) __THROW;
# else
# define msgctl __msgctl64
# endif
#endif
And if you think it is a better approach - we can replace it to:
#if __TIMESIZE == 64 || !defined __USE_TIME_BITS64
extern int msgctl (int __msqid, int __cmd, struct msqid_ds
*__buf)__THROW;
#else
# ifdef __REDIRECT_NTH
extern int __REDIRECT_NTH (msgctl (int __msqid,
int __cmd,
struct msqid64_ds *__buf),
__msgctl64) __THROW;
# else
# define msgctl __msgctl64
# endif
#endif
> So I think it is ok to use to decide whether the ABI has 32-bit
> support to decide whether __USE_TIME_BITS64 should redirect the
> symbol to the y2038 ones.
>
> >
> >>
> >> I used this approach instead of moving the logic to bits/msq.h
> >> to avoid the need to add this boilerplate and on each architecture
> >> define bits/msq.h.
> >
> > Isn't the solution from point 1. requiring to have struct msqid64_ds
> > instead of struct msqid_ds in [***] ?
>
> Indeed on [***] it should be 'struct __msqid64_ds'.
Ok.
>
> >
> >>
> >> 2. On the installed sysvipc/sys/msg.h header do a similar thing:
> >>
> >> #if __TIMESIZE == 32 && defined __USE_TIME_BITS64
> >> # define msqid_ds __msqid64_ds
> >> #endif
> >>
> >> #if __TIMESIZE == 64 || !defined __USE_TIME_BITS64
> >> extern int msgctl (int __msqid, int __cmd, struct msqid_ds
> >> *__buf) __THROW; #else
> > ^^^^^^^^^^^^^^ [*]
> >
> >> # ifdef __REDIRECT_NTH
> > ^^^^^^^^^^^^^^ [**]
> >
> >> extern int __REDIRECT_NTH (msgctl (int __msqid,
> >> int __cmd,
> >> struct msqid64_ds *__buf),
> > ^^^^^^^^^^^^^^^^^^^ - [***]
> >
> >> __msgctl64) __THROW;
> >> # else
> >> # define msgctl __msgctl64
> >> # endif
> >> #endif
> >
> > Till now I'm only adding redirection [**] to exported files. The
> > part from [*] is used in internal glibc headers. I've used the [*]
> > pattern in all already converted (and pulled to -master) syscalls.
> >
> >>
> >> Different than LFS, I don't think we need to support the
> >> symbol64 usage (enabled by _LARGEFILE64_SOURCE); just the build to
> >> use the 64-bit time_t ABI.
> >
> > As Joseph stated in the other mail - user code shall use "normal"
> > syscalls and structures - namely e.g. clock_settime() and struct
> > timespec.
> >
> > The aliasing shall be done in exported glibc headers when one
> > compiles the program with -D_TIME_BITS=64 -D_FILE_OFFSET_BITS=64
> >
> > This is how it is done in [1].
>
> That's exactly the idea of __REDIRECT_NTH. For the code:
>
> struct msqid_ds buf;
> [...]
> msgctl (..., &buf);
>
> With _TIME_BITS=32 it will emit a call to msgctl, while for
> _TIME_BITS=64 it will still emit a call to __msgctl64 (due the asm
> alias).
Yes, correct.
>
> >
> >>
> >> Unfortunately I don't know a easy way to redirect the 'struct
> >> msqid' instantiation to the 64-bit time_t type without a '#define'.
> >>
> >> 3. Install the struct_msqid_ds64.h header.
> >>
> >
> > IIRC it is already exported.
>
> I meant to add it on 'sysdep_headers' rule to install with 'make
> install'.
Ok, this is interesting. I've grep'ed for "sysdep_headers" and some
headers have been found. However, I'm wondering how other ones are
exported as the list seems to be reduced when compared to the total
number of installed headers.
>
> >
> >>
> >> You might need to adjust something on the build itself, but I think
> >> this is the general idea of exporting the 64-bit time interfaces.
> >
> > Having [*] in exported headers duplicates the code which is already
> > pulled.
> >
> >> What I am not sure if how we gonna export the 64-bit ABI: whether
> >> to use current name with double underscore of use a similar name
> >> convention used for LFS (without the underscore).
> >>
> >
> > I do guess that you refer to for example exported version of
> > in-glibc struct __timespec64 ?
> >
> > And wonder if it shall have exported name as struct __timespec,
> > struct timespec64 or struct __timespec64 ?
> >
> > Up till now my WIP patch, which adds Y2038 support [1], gets away
> > with just adjusting the exported struct timespec, so there is no
> > need to export struct __timespec64.
>
> I think the exported names for the y2038 struct should be different
> than the default 32-time one to force mangling mismatch when mixing
> C++ object built with different _TIME_BITS values. The names should
> also have the double underscore as indicated by Joseph (and it avoid
> the issue we have for LFS, BZ#14106).
IMHO the struct __timespec64 shall be a good choice for exported name
(the same would be done for __time64 and __timeval64).
>
> >
> >
> > Links:
> > [1] -
> > https://github.com/lmajewski/y2038_glibc/commit/400ea2f0c98ec36666b395b8e7fe822a3dd40565
> >
> > [2] -
> > https://github.com/lmajewski/y2038_glibc/commit/400ea2f0c98ec36666b395b8e7fe822a3dd40565#diff-8297a1191cc06a8a7af6fc6ac33af4bef2d7d72efd901d1634d910b56631e0c7R13
> >
> > [3] -
> > https://github.com/lmajewski/y2038_glibc/commit/75c4044b9a49faaeec245cc3a79a390dde7c804e#diff-6403fd00660a397fd05b433121f58de44f09b18aed3a3843669c14041ad79d7bR320
> >
> >>>
> >>> Build tests:
> >>> ./src/scripts/build-many-glibcs.py glibcs
> >>> ---
> >>> include/bits/types/struct_msqid64_ds.h | 36
> >>> +++++++++++++++++++ .../sysv/linux/bits/types/struct_msqid64_ds.h
> >>> | 4 --- 2 files changed, 36 insertions(+), 4 deletions(-)
> >>> create mode 100644 include/bits/types/struct_msqid64_ds.h
> >>>
> >>> diff --git a/include/bits/types/struct_msqid64_ds.h
> >>> b/include/bits/types/struct_msqid64_ds.h new file mode 100644
> >>> index 0000000000..8c39a2a872
> >>> --- /dev/null
> >>> +++ b/include/bits/types/struct_msqid64_ds.h
> >>> @@ -0,0 +1,36 @@
> >>> +/* Internal for glibc implementation of the SysV message struct
> >>> msqid64_ds.
> >>> + Copyright (C) 2020 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
> >>> + <https://www.gnu.org/licenses/>. */
> >>> +
> >>> +#include <time.h>
> >>> +
> >>> +#if __TIMESIZE == 64
> >>> +# define __msqid64_ds msqid_ds
> >>> +#else
> >>> +struct __msqid64_ds
> >>> +{
> >>> + struct ipc_perm msg_perm; /* structure describing
> >>> operation permission */
> >>> + __time64_t msg_stime; /* time of last msgsnd
> >>> command */
> >>> + __time64_t msg_rtime; /* time of last msgsnd
> >>> command */
> >>> + __time64_t msg_ctime; /* time of last change */
> >>> + __syscall_ulong_t __msg_cbytes; /* current number of bytes on
> >>> queue */
> >>> + msgqnum_t msg_qnum; /* number of messages
> >>> currently on queue */
> >>> + msglen_t msg_qbytes; /* max number of bytes
> >>> allowed on queue */
> >>> + __pid_t msg_lspid; /* pid of last msgsnd() */
> >>> + __pid_t msg_lrpid; /* pid of last msgrcv() */
> >>> +};
> >>> +#endif
> >>> diff --git
> >>> a/sysdeps/unix/sysv/linux/bits/types/struct_msqid64_ds.h
> >>> b/sysdeps/unix/sysv/linux/bits/types/struct_msqid64_ds.h index
> >>> 3536c8ea62..07cc1036ab 100644 ---
> >>> a/sysdeps/unix/sysv/linux/bits/types/struct_msqid64_ds.h +++
> >>> b/sysdeps/unix/sysv/linux/bits/types/struct_msqid64_ds.h @@ -20,9
> >>> +20,6 @@ # error "Never use <bits/msq.h> directly; include
> >>> <sys/msg.h> instead." #endif
> >>> -#if __TIMESIZE == 64
> >>> -# define __msqid64_ds msqid_ds
> >>> -#else
> >>> struct __msqid64_ds
> >>> {
> >>> struct ipc_perm msg_perm; /* structure describing
> >>> operation permission */ @@ -35,4 +32,3 @@ struct __msqid64_ds
> >>> __pid_t msg_lspid; /* pid of last msgsnd() */
> >>> __pid_t msg_lrpid; /* pid of last msgrcv() */
> >>> };
> >>> -#endif
> >>>
> >
> >
> >
> >
> > Best regards,
> >
> > Lukasz Majewski
> >
> > --
> >
> > DENX Software Engineering GmbH, Managing Director: Wolfgang
> > Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell,
> > Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
> > lukma@denx.de
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://sourceware.org/pipermail/libc-alpha/attachments/20201106/f632ff51/attachment.sig>
More information about the Libc-alpha
mailing list