On Fri, May 19, 2023 at 1:03 AM Alejandro Colomar <alx.manpages@xxxxxxxxx> wrote: > > Hi Zijun, Elliott, > > On 5/19/23 01:30, enh wrote: > > should probably remove the "The compilation warning looks to be going > > away in glibc 2.17 see glibc commit > > 4b7634a5e03b0da6f8875de9d3f74c1cf6f2a6e8" above, since this patch > > fixes that, but leave the FIXME because it looks like there are more > > FIXMEs below? > > > > > > On Thu, May 18, 2023 at 4:27 PM Zijun Zhao <zijunzhao@xxxxxxxxxx> wrote: > >> > >> Hi there, > >> We are annotating settimeofday(), gettimeofday() and we will make tv > >> Nonnull if compilation warnings will > >> > >> result. But after checking > >> https://elixir.bootlin.com/linux/latest/source/kernel/time/time.c#L199 > >> nd https://elixir.bootlin.com/linux/latest/source/kernel/time/time.c#L224, > >> > >> we think tv is okay to be nullable. So we make the fix to make it more clear. > >> > >> Best, > >> > >> Zijun Zhao > > You'll also need to add _Nullable to the function prototypes in the > SYNOPSIS. heh, funnily enough, zijunzhao's adding exactly those annotations to the bionic headers, which is how we stumbled across this. > BTW, I see that glibc still requires nonnull in gettimeofday(3). It's > only settimeofday(3) that is nullable. yeah, but the _kernel_ allows null in both (see kernel/time/time.c), and this is section 2 of the man page. it's unclear to me whether that's worth calling out up here, or should be down in the Linux-specific section? (this page seems to be written as if it cares about Hurd? "On a non-Linux kernel, with glibc ...".) > See: > > > $ grepc gettimeofday /usr/include/*/sys/time.h > /usr/include/x86_64-linux-gnu/sys/time.h:67: > extern int gettimeofday (struct timeval *__restrict __tv, > void *__restrict __tz) __THROW __nonnull ((1)); > > > /usr/include/x86_64-linux-gnu/sys/time.h:75: > # define gettimeofday __gettimeofday64 > > > $ grepc settimeofday /usr/include/*/sys/time.h > /usr/include/x86_64-linux-gnu/sys/time.h:86: > extern int settimeofday (const struct timeval *__tv, > const struct timezone *__tz) > __THROW; > > > /usr/include/x86_64-linux-gnu/sys/time.h:106: > # define settimeofday __settimeofday64 > > > And while NULL may be non-UB, the manual page is not very clear on why > someone would want to call these functions with NULL. Could you > please also explain why would someone want to call these functions > with NULL? (Let's discuss it in the list, and then we see what > wording we use for the page.) so there are definitely several orders of magnitude more users of a null timezone --- most callers don't want that (not least because glibc seems to not fill it out anyway? musl certainly doesn't, implementing gettimeofday() in terms of clock_gettime() instead). the users for a null time (and non-null timezone; i haven't seen anyone -- not even a test -- pass null for both!) are interesting. Android's init uses settimeofday() with only a timezone, but -- although the timezone to use is settable in device-specific init scripts -- for devices in our tree, that's actually always 0. there's other proprietary (non-Google) code that uses gettimeofday() with only a timezone, and i'm honestly not sure whether it's deliberate that it's asking the _kernel_ for the timezone or not (i'm guessing they want the device timezone rather than the app timezone, but i do not believe there are devices where the _kernel_ timezone matches the device timezone?). certainly both of these use cases seem worth following up on, and i will do. but looking at musl (which ignores tz in both cases) it seems like the most impactful change would be to fix "The functions gettimeofday() and settimeofday() can get and set the time as well as a timezone" :-) it also seems like an improvement to really call out the libc/kernel differences more clearly though. > Thanks! > Alex > > -- > <http://www.alejandro-colomar.es/> > GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5 >
