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

Thu Nov 12 15:27:17 GMT 2020

Hi Adhemerval,

> On 11/11/2020 20:57, Lukasz Majewski wrote:
> > Hi Adhemerval,
> >   
> >> On 09/11/2020 21:02, Lukasz Majewski wrote:  
> >>> Hi Joseph, Adhemerval,
> >>>     
> >>>> On Mon, 9 Nov 2020, Lukasz Majewski wrote:
> >>>>    
> >>>>>> If in a particular case defining the struct tag as a macro is
> >>>>>> unsafe,      
> >>>>>
> >>>>> Could you explain why having construct as:
> >>>>>
> >>>>> #define timespec __timespec64 in
> >>>>> ./time/bits/types/struct_timespec.c is unsafe (after having the
> >>>>> file ./time/bits/types/struct___timespec64.c exported)?      
> >>>>
> >>>> I think "#define stat __stat64_time64" is unsafe, since people
> >>>> may do "#undef stat" to get a function rather than a macro (and
> >>>> thereby suppress the macro expansion for the struct tag as well).
> >>>>
> >>>> I'm less sure there would be any problems with "#define timespec 
> >>>> __timespec64".  The POSIX namespace rules are clearer on e.g.
> >>>> what's reserved with file scope, than on what's reserved as a
> >>>> macro name that the user mustn't undefine.
> >>>>    
> >>>
> >>> Please correct me if I'm wrong, but the scheme shall be
> >>> consistent. Which approach shall be taken then?    
> >>
> >> As Joseph has put, some header name space allows the use of use 
> >> identifiers prefix by the implementation.  However, there are
> >> restriction as well of what the implementation is allowed to do,
> >> check the 'Name Space' part at the POSIX definition [1].  
> > 
> > Thanks for sharing the link. Now I do understand a bit more.
> >   
> >>
> >> For instance, for the sys/msg.h interface we can export the
> >> 'struct msqid64_ds' type since 'msg' is a reserve prefix. And I
> >> think that doing
> >>
> >>     #if __TIMESIZE == 32 && defined __USE_TIME_BITS64
> >>     # define msqid_ds __msqid64_ds
> >>     #endif
> >>
> >> For 'sys/msg.h' should be safe as well.  
> > 
> > According to the link [1] - the "msg" prefix is reserved in the
> > 'sys/msg.h', but we do #define msqid_ds - the msq prefix is not
> > reserved...
> > 
> > Is this correct?  
> 
> Indeed, I overlooked this case. It seems we will need to go on the
> same route I laid out for 'struct timespec' for the SysV interface
> as well. It would require to change some arch-specific files I was
> expecting to have to touch, but it seems there is no better option.

You may also consider reusing following patches:
https://github.com/lmajewski/y2038_glibc/commit/e7acba0ea587afab875f62eb132608ba6a679af5
https://github.com/lmajewski/y2038_glibc/commit/4c42b58c539af778dc17476187fa8330db37d7d2
https://github.com/lmajewski/y2038_glibc/commit/0ade361931988d6918888019d233335c82acc087

> 
> >   
> >>
> >> However, doing:
> >>
> >>   #if __TIMESIZE == 32 && defined __USE_TIME_BITS64
> >>   # define timespec __timespec64
> >>   #endif
> >>
> >> It is not allowed since 'timespec' does not fall on the reserved
> >> namespace of 'time.h' header.   
> > 
> > Definately - the timespec is not reserved in the time.h name space.
> >   
> >>  As indicated by Joseph, if in a 
> >> particular case defining the struct tag as a macro is unsafe (as
> >> for timespec), we would need to redefine the struct member itself
> >> depending of the _TIME_BITS.
> >>
> >> It means that for timespec, we will need to do something as:
> >>
> >>   1. On 'time/bits/types/struct_timespec.h' define 'struct
> >> timespec' as:
> >>
> >>   struct timespec
> >>   {
> >>   #if __TIMESIZE == 32 && defined __USE_TIME_BITS64
> >>     __tim64_t tv_sec;             /* Seconds.  */
> >>   #else
> >>     __time_t tv_sec;
> >>   #endif
> >>   #if __WORDSIZE == 64 \
> >>     || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64) \
> >>     || __TIMESIZE == 32
> >>     __syscall_slong_t tv_nsec;    /* Nanoseconds.  */
> >>   #else
> >>   # if __BYTE_ORDER == __BIG_ENDIAN
> >>     int: 32;           /* Padding.  */
> >>     long int tv_nsec;  /* Nanoseconds.  */
> >>   # else
> >>     long int tv_nsec;  /* Nanoseconds.  */
> >>     int: 32;           /* Padding.  */
> >>   # endif
> >>   #endif
> >>   };
> >>  
> > 
> > I do follow this approach - such implementation is in [2].  
> 
> Yeah, I though we would be able to simplify the boilerplate required,
> but it seems we are constrained by the POSIX namespace names.

Ok.

> 
> >   
> >>   
> >>   2. And for the interfaces that uses it, on time/time.h:
> >>
> >>   [...]
> >>   #ifdef __USE_POSIX199309
> >>   [...]
> >>   #if __TIMESIZE == 64 || !defined __USE_TIME_BITS64
> >>   extern int nanosleep (const struct timespec *__requested_time,
> >>                         struct timespec *__remaining);
> >>   #else
> >>   # ifdef __REDIRECT_NTH
> >>   extern int __REDIRECT_NTH (nanosleep (const struct timespec
> >> *__requested_time, struct timespec *__remaining),
> >>                                         __nanosleep64);
> >>   # else
> >>   /* This might not be safe, but I don't see a better way to do it
> >> without __REDIRECT_NTH support.  */
> >>   #  define nanosleep __nanosleep64
> >>   # endif
> >>    
> > 
> > This is also done in the code from [2]. However, there is no need to
> > have 
> > #if __TIMESIZE == 64 || !defined __USE_TIME_BITS64
> > as we override it when __USE_TIME_BITS64 is defined
> >   
> >>
> >>   3. It would be good to add a internal tests to check if the
> >> export ABI for timespec does in fact match the internal
> >> definition.  A test that does something:
> >>
> >>      #define _TIME_SIZE 64
> >>
> >>      _Static_assert (sizeof (struct timespec)
> >>                      == sizeof (struct __timespec64),
> >>                      ...);
> >>   
> >>      _Static_assert (sizeof ((struct timespec){0}.tv_sec)
> >>                      == sizeof ((struct __timespec64){0}.tv_nsec),
> >>                      ...);
> >>      _Static_assert (offsetof (struct timespec, tv_sec)
> >>                      == offsetof (struct __timespec64, tv_sec),
> >>                      ...); 
> >>      _Static_assert (sizeof ((struct timespec){0}.tv_nsec)
> >>                      == sizeof ((struct __timespec64){0}.tv_nsec),
> >>                      ...);
> >>      _Static_assert (offsetof (struct timespec, tv_nsec)
> >>                      == offsetof (struct __timespec64, tv_nsec),
> >>                      ...); 
> >>  
> > 
> > This would be welcome, I will add such simple check when I add
> > __USE_TIME_BITS64 support (i.e. Y2038 support).
> >   
> >>      I don't think it needs test for the internal fields (unless
> >> the implementation does use it in some way, which I don't think it
> >> is the case).
> >>  
> > 
> > Ok.
> >   
> >>  
> >>   4. For timespec we should change all symbols that uses it,
> >> including compound types.  
> > 
> > Yes - this is also done in patch [2]. For example struct itimerspec
> > is adjusted as well.
> >   
> >>
> >>
> >>   5. We need to make sure glibc does not build or define _TIME_SIZE
> >> internally (we already have issues if user tries to enable the LFS
> >> support, so it should be ok as-is with default values).  
> > 
> > Do you think about _TIME_BITS=64 when the code is compiled
> > (-D_TIME_BITS=64)?
> > When -D_TIME_BITS=64 is passed during compilation of user program,
> > the __USE_TIME_BITS64 is set to 1 in exported headers, so aliasing
> > for Y2038 is enabled.  
> 
> I meant building glibc itself, not user programs using glibc provided
> headers.  It is to ensure that internally 'struct timespec' (or any
> affected struct by 64-bit time) will have 32-bit ABI layout for ABIs
> that supports it since glibc itself includes installed headers.
> 

Could you elaborate on this?

I do guess that _TIME_BITS when added to glibc (i.e. when it will be
recognized by "exported" headers) will enable 64 bit time support on
archs with __WORDSIZE=32 (e.g. ARM 32 bit).

Then, after some time, we will add #define _TIME_BITS 64 at e.g.
features.h (exported) on eligible ports, which would mean that by
default support for 64 bit time is enabled on them.

Am I correct?

> > 
> > I'm not sure about the role of _TIME_SIZE. I only recall the
> > __TIMESIZE which is set depending on __WORDSIZE (or explicitly to
> > 64 on e.g. RISC-V).  
> 
> My understanding is _TIME_SIZE will be the compiler flag used by the
> programs to define which time ABI they want to use.

Yes, this is also my understanding.

> 
> >   
> >>
> >>
> >> I think I have outlined what is required, but I might have
> >> overlooked something.  
> > 
> > So to sum up:
> > 
> > - When prefix is reserved in POSIX namespaces [1] it may be allowed
> > to export __foo64 struct and make the aliasing in the corresponding
> >   exported header.
> > 
> > - Otherwise, we need an internal glibc copy of the struct (i.e.
> > struct __timespec64) and also corresponding exported struct (i.e.
> > struct timespec) needs to be adjusted when __USE_TIME_BITS64 is
> > defined.
> > 
> > The second case seems to be pervasive and is already utilized in my
> > WIP patch [2].  
> 
> I think to simplify we should for [2] as you initially suggested.  I
> though we could mimic LFS support, but for y2038 support it seems that
> we will need to re-define the types without using different names for
> the affected types.

Ok.

> 
> >   
> >>
> >>
> >> [1]
> >> https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html
> >>  
> > 
> > 
> > Links:
> > 
> > [2] -
> > https://github.com/lmajewski/y2038_glibc/commit/b5d9a0881ccc0a06e520c495ce7409f8e077fb20#diff-8297a1191cc06a8a7af6fc6ac33af4bef2d7d72efd901d1634d910b56631e0c7R13
> >  
> 

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/20201112/e681d4a1/attachment.sig>