[PATCH v3 7/7] y2038: linux: Provide ___gettimeofday64 implementation

[Lukasz Majewski]

Hi Adhemerval,

> On 08/02/2020 19:15, Lukasz Majewski wrote:
> > Hi Adhemerval,
> >   
> >>>>  #else /* USE_IFUNC_GETTIMEOFDAY  */
> >>>> -# include <time/gettimeofday.c>
> >>>> +/* Conversion of gettimeofday function to support 64 bit time on
> >>>> archs
> >>>> +   with __WORDSIZE == 32 and __TIMESIZE == 32/64  */
> >>>> +#include <errno.h>
> >>>> +
> >>>> +int
> >>>> +___gettimeofday64 (struct __timeval64 *restrict tv, void
> >>>> *restrict tz) +{
> >>>> +  if (__glibc_unlikely (tz != 0))
> >>>> +    memset (tz, 0, sizeof (struct timezone));
> >>>> +
> >>>> +  struct __timespec64 ts64;
> >>>> +  int ret = __clock_gettime64 (CLOCK_REALTIME, &ts64);
> >>>> +
> >>>> +  if (ret == 0 && tv)
> >>>> +    *tv = timespec64_to_timeval64 (ts64);      
> >>>
> >>> No implicit checks.  Also, we already set 'tv' with nonull
> >>> attribute, so I am not sure if it is worth to add an extra check
> >>> for 'tv' validity (specially because users tend to expect low
> >>> latency for the symbol).
> >>>
> >>> In any case, if the idea is to add such check as QoI I think it
> >>> would be better to do a early bail before actually issue
> >>> __clock_gettime64.    
> >>
> >> No, this was just my mistake. There was a discussion with Paul and
> >> Joseph earlier. We shall _only_ check for NULL when it is required
> >> by syscalls/command documentation. This is the case for e.g.
> >> setitimer's *old_value pointer.  
> > 
> > I've double check this and in the documentation/manual [1] for
> > gettimeofday there is a sentence:
> > 
> > ----8<--------
> > If either tv or tz is NULL, the corresponding structure is not set
> > or returned. (However, compilation warnings will result if tv is
> > NULL.) ---->8--------  
> > 
> > That was the rationale to add the check
> > if (ret == 0 && tv)
> > 
> > 
> > I also think that the code as is now is correct - it returns the
> > result of getting the time from Linux, but it is not updating the
> > tv structure.  
> 
> The man-pages is not really the glibc manual, but rather documents de
> facto glibc/kernel behaviour.  The glibc 'gettimeofday' entry in
> manual (manual/time.texi) is also not explicit about this, and the
> generic implementation (time/gettimeofday.c) also does not add this
> test.

Ok.

> 
> But again, I am not against of this change as QoI and it is what
> kernel vDSO symbol does anyway (lib/vdso/gettimeofday.c:270).

I do guess that you refer to:
https://elixir.bootlin.com/linux/latest/source/lib/vdso/gettimeofday.c#L144

We could rely on the kernel if there weren't conversions (64 <->
32 bit time ) needed. 

> However, I think we should be done in a separated patch and for 'all'
> implementation to get a concise behaviour.

Shall I:

1. Extend this patch to add this extra check to all eligible
gettimeofday places.

2. Do not check if tv is NULL at all

3. Do not check if tv is NULL in this particular patch and prepare next
patch which would add check for tv != NULL to all gettimeofday
implementations in glibc ?

> 
> 
> > 
> > 
> > Links:
> > 
> > [1] - https://linux.die.net/man/2/gettimeofday
> > 
> > 
> > Best regards,
> > 
> > Lukasz Majewski
> >   




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: <http://sourceware.org/pipermail/libc-alpha/attachments/20200210/061a2911/attachment.sig>