From: Adhemerval Zanella <adhemerval.zanella@linaro.org>
To: Tobias Klauser <tklauser@distanz.ch>
Cc: libc-alpha@sourceware.org
Subject: Re: [PATCH] Add missing VDSO_{NAME,HASH}_* macros and use them for PREPARE_VERSION_KNOWN
Date: Fri, 24 May 2019 11:42:32 -0300 [thread overview]
Message-ID: <e4b5f12a-c828-c77b-e85e-886a0368c4fa@linaro.org> (raw)
In-Reply-To: <20190524072356.it7dqmlkmt4vf3p2@distanz.ch>
On 24/05/2019 04:23, Tobias Klauser wrote:
> Thank you for the review.
>
> On 2019-05-23 at 21:36:01 +0200, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
>> On 16/05/2019 08:59, Tobias Klauser wrote:
>>> Define all currently used Linux versions used for
>>> PREPARE_VERSION{,_KNOWN} in sysdeps/unix/sysv/linux/dl-vdso.h and use
>>> them instead of duplicating the versions and precomputed hashes across
>>> architecture specific files.
>>>
>>> 2019-05-16 Tobias Klauser <tklauser@distanz.ch>
>>>
>>> * sysdeps/unix/sysv/linux/aarch64/gettimeofday.c (INIT_ARCH): Use
>>> PREPARE_VERSION_KNOWN.
>>> * sysdeps/unix/sysv/linux/aarch64/init-first.c: Likewise.
>>> * sysdeps/unix/sysv/linux/dl-vdso.h (VDSO_NAME_LINUX_2_6_39): New
>>> define.
>>> (VDSO_HASH_LINUX_2_6_39): Likewise.
>>> (VDSO_NAME_LINUX_4_9): Likewise.
>>> (VDSO_HASH_LINUX_4_9): Likewise.
>>> * sysdeps/unix/sysv/linux/m68k/init-first.c (_libc_vdso_platform_setup):
>>> Use PREPARE_VERSION_KNOWN.
>>> * sysdeps/unix/sysv/linux/powerpc/gettimeofday.c (INIT_ARCH): Likewise.
>>> * sysdeps/unix/sysv/linux/powerpc/init-first.c
>>> (_libc_vdso_platform_setup): Likewise.
>>> * sysdeps/unix/sysv/linux/powerpc/time.c (INIT_ARCH): Likewise.
>>> * sysdeps/unix/sysv/linux/s390/init-first.c (_libc_vdso_platform_setup):
>>> Likewise.
>>> * sysdeps/unix/sysv/linux/x86_64/init-first.c (__vdso_platform_setup):
>>> Likewise.
>>
>>
>> LGTM, I assume you checked a build against affected architectures. As a side
>> note I think also should just remove PREPARE_VERSION macro and replace it with
>> a more sane interface:
>
> I (cross-)compile tested on all architectures and ran the test suite on
> x86_64.
>
>> --
>> static inline struct r_found_version
>> prepare_version_base (const char *name, ElfW(Word) hash)
>> {
>> assert (hash == _dl_elf_hash (name));
>> return (struct r_found_version) { name, hash, 1, NULL };
>> }
>> #define prepare_version(vname) \
>> prepare_version_base (VDSO_NAME_##vname, VDSO_HASH_##vname)
>> --
>
> Should I add this to a v2 of this patch? Or should I send a follow-up
> patch with the change?
I don't have a preference here. Refactor the vDSO code is something
I have in mind for some time.
>
>>> ---
>>> sysdeps/unix/sysv/linux/aarch64/gettimeofday.c | 4 ++--
>>> sysdeps/unix/sysv/linux/aarch64/init-first.c | 4 ++--
>>> sysdeps/unix/sysv/linux/dl-vdso.h | 4 ++++
>>> sysdeps/unix/sysv/linux/m68k/init-first.c | 2 +-
>>> sysdeps/unix/sysv/linux/powerpc/gettimeofday.c | 4 ++--
>>> sysdeps/unix/sysv/linux/powerpc/init-first.c | 2 +-
>>> sysdeps/unix/sysv/linux/powerpc/time.c | 4 ++--
>>> sysdeps/unix/sysv/linux/s390/init-first.c | 2 +-
>>> sysdeps/unix/sysv/linux/x86_64/init-first.c | 2 +-
>>> 9 files changed, 16 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/sysdeps/unix/sysv/linux/aarch64/gettimeofday.c b/sysdeps/unix/sysv/linux/aarch64/gettimeofday.c
>>> index 6c008ed9357f..9180b50bf7c3 100644
>>> --- a/sysdeps/unix/sysv/linux/aarch64/gettimeofday.c
>>> +++ b/sysdeps/unix/sysv/linux/aarch64/gettimeofday.c
>>> @@ -38,13 +38,13 @@ __gettimeofday_vsyscall (struct timeval *tv, struct timezone *tz)
>>> return INLINE_VSYSCALL (gettimeofday, 2, tv, tz);
>>> }
>>>
>>> -/* PREPARE_VERSION will need an __LP64__ ifdef when ILP32 support
>>> +/* PREPARE_VERSION_KNOWN will need an __LP64__ ifdef when ILP32 support
>>> goes in. See _libc_vdso_platform_setup in
>>> sysdeps/unix/sysv/linux/aarch64/init-first.c. */
>>>
>>> # undef INIT_ARCH
>>> # define INIT_ARCH() \
>>> - PREPARE_VERSION (linux_version, "LINUX_2.6.39", 123718537); \
>>> + PREPARE_VERSION_KNOWN (linux_version, LINUX_2_6_39); \
>>> void *vdso_gettimeofday = \
>>> _dl_vdso_vsym ("__kernel_gettimeofday", &linux_version);
>>
>> Also a side note, this vsdo setup is not required. The init-first.c
>> already setups the VDSO_SYMBOL(gettimeofday). So a possible followup
>> cleanup would be:
>>
>> --
>> # undef INIT_ARCH
>> # define INIT_ARCH()
>>
>> static inline void *
>> gettimeofday_vdso (void)
>> {
>> __typeof (VDSO_SYMBOL(gettimeofday)) vdsop = VDSO_SYMBOL(gettimeofday);
>> PTR_DEMANGLE (vdsop);
>> return vdsop != NULL ? vdsop : (void *) __gettimeofday_vsyscall;
>> }
>>
>> libc_ifunc_hidden (__redirect___gettimeofday, __gettimeofday,
>> gettimeofday_vdso ())
>> --
>
> Thanks, will send a follow-up patch.
>
next prev parent reply other threads:[~2019-05-24 14:42 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-05-16 11:59 [PATCH] Add missing VDSO_{NAME,HASH}_* macros and use them for PREPARE_VERSION_KNOWN Tobias Klauser
2019-05-23 19:36 ` Adhemerval Zanella
2019-05-24 7:23 ` Tobias Klauser
2019-05-24 14:42 ` Adhemerval Zanella [this message]
2019-05-24 15:41 ` Tobias Klauser
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.gnu.org/software/libc/involved.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e4b5f12a-c828-c77b-e85e-886a0368c4fa@linaro.org \
--to=adhemerval.zanella@linaro.org \
--cc=libc-alpha@sourceware.org \
--cc=tklauser@distanz.ch \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).