[v2] Add x86 32 bit vDSO time function support

  Hi Roland,

Thanks again for reviewing it.

On 09-07-2014 17:02, Roland McGrath wrote:
>> 	* 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.

Thanks, I used your wording.

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

I have addressed all the aforementioned issues.

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

My first approach was to just existent code, that's why I haven't changed it to
an inline function.  And if you double check there is already line breaks around it.
I have changed the function call to use (*f).

>
>
> 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 have indicated on my first message (x86 and x86_64 with a 3.16-rc2 kernel) and I also
checked x86 and x86_64 on a 3.5.0 kernel.

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

Indeed, although this is more like a cleanup.  Anyway, I will defer this work for
another release if other raise concerns about it.

What about now, is everything ok?

--

2014-07-10  Adhemerval Zanella  <azanella@linux.vnet.ibm.com>
	    Stefani Seibold  <stefani@seibold.net>

	* sysdeps/unix/sysv/linux/x86_64/Makefile [$(subdir) = elf]
	(sysdep_routines): Add dl-vdso here, ...
	* sysdeps/unix/sysv/linux/x86/Makefile [$(subdir) = elf]
	(sysdep_routines): ... not here.
	* sysdeps/unix/sysv/linux/i386/gettimeofday.c: New file: set syscall
	fallback when vDSO is not presented.
	* sysdeps/unix/sysv/linux/x86_64/init-first.c (__vdso_clock_gettime):
	Define with libc_hidden_proto/libc_hidden_data_def definitions.
	(_libc_vdso_platform_setup): Rename to __vdso_platform_setup.
	* sysdeps/unix/sysv/linux/i386/init-first.c: New file: likewise.
	* sysdeps/unix/sysv/linux/i386/time.c: New file: likewise.
	* sysdeps/unix/sysv/linux/x86_64/bits/libc-vdso.h: Moved to ...
	* sysdeps/unix/sysv/linux/x86/libc-vdso.h: ... here.
	* sysdeps/unix/sysv/linux/x86_64/clock_gettime.c: Move to ...
	* sysdeps/unix/sysv/linux/x86/clock_gettime.c: ... here.
	* sysdeps/unix/sysv/linux/x86_64/gettimeofday.c: Move to ...
	* sysdeps/unix/sysv/linux/x86/gettimeofday.c: ... here.  Also added
	fallback configurable symbol when vDSO is not available.
	* sysdeps/unix/sysv/linux/x86_64/time.c: Move to ...
	* sysdeps/unix/sysv/linux/x86/time.c: ... here.  Also refactored to
	be able to redefine fallback symbol when vDSO is not available.
	* sysdeps/unix/sysv/linux/x86_64/timespec_get.c: Move to ...
	* sysdeps/unix/sysv/linux/x86/timespec_get.c: ... here.

---

[v2] Add x86 32 bit vDSO time function support

Commit Message

Comments

Patch