Re: [PATCH v2] Add x86 32 bit vDSO time function support

[Roland McGrath <roland@hack.frob.com>]

> 	* sysdeps/unix/sysv/linux/x86_64/Makefile [sysdep_routing]: Move
> 	dl-vdso rule to ...
> 	* sysdeps/unix/sysv/linux/x86/Makefile [sysdep_routines]: ... here.

Typo: s/routing/routines/.  Also, [] is for identifying conditional
sections (if* in makefiles, #if* in C).  Use () for identifying the entity
being changed.  Also, there is no rule here.  It's just appending it to the
list (or not).  I would have written:

	* sysdeps/unix/sysv/linux/x86/Makefile [$(subdir) = elf]
	(sysdep_routines): Add dl-vdso here, ...
	* sysdeps/unix/sysv/linux/x86_64/Makefile [$(subdir) = elf]
	(sysdep_routines): ... not here.

> 	* sysdeps/unix/sysv/linux/x86/gettimeofday.c: ... here. Also added

Two spaces between sentences.

> 	* sysdeps/unix/sysv/linux/x86/time.c: ... here. Also refactored to

And here.

> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/i386/gettimeofday.c
> @@ -0,0 +1,34 @@
> +/* Copyright (C) 2014 Free Software Foundation, Inc.

Still needs a top-line descriptive comment.

> +# define GETTIMEOFAY_FALLBACK (void*) &__gettimeofday_syscall

Put parens around the rhs so it's a single syntactic unit.

> +++ b/sysdeps/unix/sysv/linux/i386/time.c
> @@ -0,0 +1,34 @@
> +/* time implementation call for Linux/i386.

/* time -- Get number of seconds since Epoch.  Linux/i386 version.

> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/x86/clock_gettime.c
> @@ -0,0 +1,38 @@
> +/* clock_gettime implementation call for Linux/x86.

/* Get the current value of a clock.  Linux/x86 version.

(Here I copied the description from the stub file rt/clock_gettime.c.)

> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/x86/gettimeofday.c
> @@ -0,0 +1,57 @@
> +/* Copyright (C) 2014 Free Software Foundation, Inc.

Still needs a top-line descriptive comment.

> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/x86/time.c
> @@ -0,0 +1,52 @@
> +/* Copyright (C) 2014 Free Software Foundation, Inc.

Still needs a top-line descriptive comment.

> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/x86/timespec_get.c
> @@ -0,0 +1,28 @@
> +/* timespec_get implementation call for Linux/x86.

This is not a sensical English sentence fragment.
I think you meant "timespec_get call implementation".
But that's not actually descriptive.

> +#ifdef SHARED
> +# define INTERNAL_GETTIME(id, tp) \
> +  ({ long int (*f) (clockid_t, struct timespec *) = __vdso_clock_gettime; \
> +  PTR_DEMANGLE (f);							  \
> +  f (id, tp); })

Why isn't this just an inline function?  If it had to be a macro, it should
have line breaks around ({ and }) to be more readable.  Either way, it
should use (*f) (...) to call via the function pointer.


You didn't mention what testing you did.  For this sort of change, it is
important to test (and report about it) for more than one kernel version,
including at least one and one without the vDSO support that this code
should use but not rely on.

I tend to think this is getting a bit close to freeze time for a
substantial semantic change like this one.  But I'll defer that paranoia to
others, and if the folks here who are distro package maintainers are not
worried about it then I won't object.


Thanks,
Roland