[PATCH 2/3] msqid: Provide internal copy of struct __msqid64_ds

Wed Nov 4 13:30:08 GMT 2020

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.

> 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.

> 
>     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 [***] ?

> 
>   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].

> 
>     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.

> 
> 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.

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
-------------- 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/20201104/3343ef83/attachment-0001.sig>