This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v2] Add x86 32 bit vDSO time function support
- From: Roland McGrath <roland at hack dot frob dot com>
- To: Adhemerval Zanella <azanella at linux dot vnet dot ibm dot com>
- Cc: "GNU C. Library" <libc-alpha at sourceware dot org>, Stefani Seibold <stefani at seibold dot net>
- Date: Wed, 9 Jul 2014 13:02:43 -0700 (PDT)
- Subject: Re: [PATCH v2] Add x86 32 bit vDSO time function support
- Authentication-results: sourceware.org; auth=none
- References: <53ADE03D dot 7070300 at linux dot vnet dot ibm dot com> <20140627215816 dot 050DA2C39AB at topped-with-meat dot com> <53B03755 dot 6030600 at linux dot vnet dot ibm dot 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