Re: [PATCH 1/4] Set up the data structures for vDSO in libc.a

[Roland McGrath <roland at hack dot frob dot 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