This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 1/4] Set up the data structures for vDSO in libc.a
- From: Roland McGrath <roland at hack dot frob dot com>
- To: "H.J. Lu" <hjl dot tools at gmail dot com>
- Cc: GNU C Library <libc-alpha at sourceware dot org>
- Date: Thu, 4 Oct 2012 14:16:47 -0700 (PDT)
- Subject: Re: [PATCH 1/4] Set up the data structures for vDSO in libc.a
- References: <20121004195011.GA19684@gmail.com>
> +2012-09-28 H.J. Lu <hongjiu.lu@intel.com>
I'm not trying to give you hard time, but please observe the convention and
post log entries in non-diff format like we tell everyone else to do.
> + [BZ #14557]
> + * elf/dl-support.c: Include <assert.h>.
> + (_dl_sysinfo_map): New.
> + Incldue "get-dynamic-info.h" and "setup-vdso.h".
Typo.
> + (_dl_non_dynamic_init): Call setup_vdso.
> + * elf/dynamic-link.h: Remove elf_get_dynamic_info.
> + Include "get-dynamic-info.h".
> + * elf/get-dynamic-info.h: New file.
The usual thing to write is:
* elf/dynamic-link.h (elf_get_dynamic_info): Moved to ...
* elf/get-dynamic-info.h: ... this new file.
* elf/dynamic-link.h: Include that.
> + * elf/setup-vdso.h: Likewise.
> + * elf/rtld.c: Include "setup-vdso.h".
> + (dl_main): Call setup_vdso.
* elf/rtld.c (dl_main): Break out vDSO setup code into ...
* elf/setup-vdso.h (setup_vdso): ... this new file and function.
* elf/rtld.c: Include that.
(dl_main): Call setup_vdso.
> diff --git a/elf/dl-support.c b/elf/dl-support.c
> index 2bb468a..23c4e53 100644
> --- a/elf/dl-support.c
> +++ b/elf/dl-support.c
> @@ -33,6 +33,7 @@
> #include <unsecvars.h>
> #include <hp-timing.h>
> #include <stackinfo.h>
> +#include <assert.h>
There is no use of assert in this file, so this does not belong here.
elf_get_dynamic_info uses assert, so get-dynamic-info.h should include it.
> extern char *__progname;
> char **_dl_argv = &__progname; /* This is checked for some error messages. */
> @@ -161,6 +162,11 @@ uintptr_t _dl_sysinfo = DL_SYSINFO_DEFAULT;
> #if defined NEED_DL_SYSINFO || defined NEED_DL_SYSINFO_DSO
> /* Address of the ELF headers in the vsyscall page. */
> const ElfW(Ehdr) *_dl_sysinfo_dso;
> +
> +struct link_map *_dl_sysinfo_map;
> +
> +#include "get-dynamic-info.h"
> +#include "setup-vdso.h"
> #endif
These should be "# include" since they're inside one level of #if.
> /* During the program run we must not modify the global data of
> @@ -266,6 +272,14 @@ _dl_non_dynamic_init (void)
>
> _dl_verbose = *(getenv ("LD_WARN") ?: "") == '\0' ? 0 : 1;
>
> +#if defined HAVE_AUX_VECTOR \
> + && (defined NEED_DL_SYSINFO || defined NEED_DL_SYSINFO_DSO)
> + /* Set up the data structures for the system-supplied DSO early,
> + so they can influence _dl_init_paths. */
> + if (GLRO(dl_sysinfo_dso) != NULL)
> + setup_vdso ();
> +#endif
Why is the HAVE_AUX_VECTOR conditional there? It's not in the rtld.c
instance. Since you're duplicating the #if and the if here, I think
instead it should just be an unconditional:
_dl_sysinfo_setup ();
and have setup-vdso.h contain the common conditionalization.
> +static inline void __attribute__ ((always_inline))
> +# ifdef IS_IN_rtld
> +setup_vdso (struct link_map *main_map, struct link_map ***first_preload)
> +# else
> +setup_vdso (void)
> +# endif
Just give it a single signature. The dl-support.c case can pass NULL for
both arguments, and you can check that. The check should be optimized away.
Thanks,
Roland