Message ID | Pine.LNX.4.64.1410210044550.30508@digraph.polyomino.org.uk |
---|---|
State | New |
Headers | show |
Shouldn't sysdeps/ia64/dl-machine.h be using __GI__dl_argv as well? Andreas.
On Tue, 21 Oct 2014, Andreas Schwab wrote:
> Shouldn't sysdeps/ia64/dl-machine.h be using __GI__dl_argv as well?
I don't know. Since it's not using _dl_argv_internal or INTUSE(_dl_argv)
at present I see no reason my patch should break it (the aim of the patch
isn't to make _dl_argv uses more efficient than at present, simply to
remove use of an obsolete mechanism without making anything worse).
"Joseph S. Myers" <joseph@codesourcery.com> writes: > On Tue, 21 Oct 2014, Andreas Schwab wrote: > >> Shouldn't sysdeps/ia64/dl-machine.h be using __GI__dl_argv as well? > > I don't know. Since it's not using _dl_argv_internal or INTUSE(_dl_argv) > at present I see no reason my patch should break it It's not about breaking it, it's about making it consistent. Andreas.
On Tue, 21 Oct 2014, Andreas Schwab wrote: > "Joseph S. Myers" <joseph@codesourcery.com> writes: > > > On Tue, 21 Oct 2014, Andreas Schwab wrote: > > > >> Shouldn't sysdeps/ia64/dl-machine.h be using __GI__dl_argv as well? > > > > I don't know. Since it's not using _dl_argv_internal or INTUSE(_dl_argv) > > at present I see no reason my patch should break it > > It's not about breaking it, it's about making it consistent. That's beyond the scope of what this patch (or patch series) is intended to do. Followups with other cleanups are of course welcome.
"Joseph S. Myers" <joseph@codesourcery.com> writes: > On Tue, 21 Oct 2014, Andreas Schwab wrote: > >> "Joseph S. Myers" <joseph@codesourcery.com> writes: >> >> > On Tue, 21 Oct 2014, Andreas Schwab wrote: >> > >> >> Shouldn't sysdeps/ia64/dl-machine.h be using __GI__dl_argv as well? >> > >> > I don't know. Since it's not using _dl_argv_internal or INTUSE(_dl_argv) >> > at present I see no reason my patch should break it >> >> It's not about breaking it, it's about making it consistent. > > That's beyond the scope of what this patch (or patch series) is intended > to do. It's perfectly fitting, just like the other bare _dl_argv reference. Andreas.
On Tue, 21 Oct 2014, Andreas Schwab wrote: > "Joseph S. Myers" <joseph@codesourcery.com> writes: > > > On Tue, 21 Oct 2014, Andreas Schwab wrote: > > > >> "Joseph S. Myers" <joseph@codesourcery.com> writes: > >> > >> > On Tue, 21 Oct 2014, Andreas Schwab wrote: > >> > > >> >> Shouldn't sysdeps/ia64/dl-machine.h be using __GI__dl_argv as well? > >> > > >> > I don't know. Since it's not using _dl_argv_internal or INTUSE(_dl_argv) > >> > at present I see no reason my patch should break it > >> > >> It's not about breaking it, it's about making it consistent. > > > > That's beyond the scope of what this patch (or patch series) is intended > > to do. > > It's perfectly fitting, just like the other bare _dl_argv reference. This patch is intended to be minimal - that is, containing *only* the changes that are necessarily part of eliminating this particular use of INTDEF/INTUSE. Such a change to sysdeps/ia64/dl-machine.h is clearly not necessary for this patch.
Ping. This patch <https://sourceware.org/ml/libc-alpha/2014-10/msg00443.html> is pending review.
Ping^2. This patch <https://sourceware.org/ml/libc-alpha/2014-10/msg00443.html> is still pending review.
On Tue, Oct 21, 2014 at 12:45:50AM +0000, Joseph S. Myers wrote: > Continuing the removal of the obsolete INTDEF / INTUSE mechanism, this > patch replaces its use for _dl_argv with rtld_hidden_data_def and > rtld_hidden_proto. Some places in .S files that previously used > _dl_argv_internal or INTUSE(_dl_argv) now use __GI__dl_argv directly > (there are plenty of existing examples of such direct use of __GI_*). > > A single place in rtld.c previously used _dl_argv without INTUSE, > apparently accidentally, while the rtld_hidden_proto mechanism avoids > such accidential omissions. As a consequence, this patch *does* > change the contents of stripped ld.so. However, the installed > stripped shared libraries are identical to those you get if instead of > this patch you change that single _dl_argv use to use INTUSE, without > any other changes. > > Tested for x86_64 (testsuite as well as comparison of installed > stripped shared libraries as described above). > > 2014-10-21 Joseph Myers <joseph@codesourcery.com> > > [BZ #14132] > * sysdeps/generic/ldsodefs.h (_dl_argv): Use rtld_hidden_proto. > [IS_IN_rtld] (_dl_argv_internal): Do not declare. > (rtld_progname): Make macro definition unconditional. > * elf/rtld.c (_dl_argv): Use rtld_hidden_data_def instead of > INTDEF. > (dlmopen_doit): Do not use INTUSE with _dl_argv. > (dl_main): Likewise. > * elf/dl-sysdep.c (_dl_sysdep_start): Likewise. > * sysdeps/alpha/dl-machine.h (RTLD_START): Use __GI__dl_argv > instead of _dl_argv_internal. > * sysdeps/powerpc/powerpc32/dl-start.S (_dl_start_user): Use > __GI__dl_argv instead of INTUSE(_dl_argv). > * sysdeps/powerpc/powerpc64/dl-machine.h (RTLD_START): Use > __GI__dl_argv instead of _dl_argv_internal. > Looks good to me. Siddhesh
On Tue, 4 Nov 2014, Siddhesh Poyarekar wrote:
> Looks good to me.
Thanks, committed. Andreas, feel free to follow up with the ia64 change
you wanted.
diff --git a/elf/dl-sysdep.c b/elf/dl-sysdep.c index d1a2bd2..ae24d5d 100644 --- a/elf/dl-sysdep.c +++ b/elf/dl-sysdep.c @@ -108,7 +108,7 @@ _dl_sysdep_start (void **start_argptr, #endif __libc_stack_end = DL_STACK_END (start_argptr); - DL_FIND_ARG_COMPONENTS (start_argptr, _dl_argc, INTUSE(_dl_argv), _environ, + DL_FIND_ARG_COMPONENTS (start_argptr, _dl_argc, _dl_argv, _environ, GLRO(dl_auxv)); user_entry = (ElfW(Addr)) ENTRY_POINT; diff --git a/elf/rtld.c b/elf/rtld.c index d5e007f..537fc43 100644 --- a/elf/rtld.c +++ b/elf/rtld.c @@ -83,7 +83,7 @@ int _dl_argc attribute_relro attribute_hidden; char **_dl_argv attribute_relro = NULL; unsigned int _dl_skip_args attribute_relro attribute_hidden; #endif -INTDEF(_dl_argv) +rtld_hidden_data_def (_dl_argv) #ifndef THREAD_SET_STACK_GUARD /* Only exported for architectures that don't store the stack guard canary @@ -492,7 +492,7 @@ dlmopen_doit (void *a) args->map = _dl_open (args->fname, (RTLD_LAZY | __RTLD_DLOPEN | __RTLD_AUDIT | __RTLD_SECURE), - dl_main, LM_ID_NEWLM, _dl_argc, INTUSE(_dl_argv), + dl_main, LM_ID_NEWLM, _dl_argc, _dl_argv, __environ); } @@ -804,55 +804,55 @@ dl_main (const ElfW(Phdr) *phdr, GL(dl_rtld_map).l_name = rtld_progname; while (_dl_argc > 1) - if (! strcmp (INTUSE(_dl_argv)[1], "--list")) + if (! strcmp (_dl_argv[1], "--list")) { mode = list; GLRO(dl_lazy) = -1; /* This means do no dependency analysis. */ ++_dl_skip_args; --_dl_argc; - ++INTUSE(_dl_argv); + ++_dl_argv; } - else if (! strcmp (INTUSE(_dl_argv)[1], "--verify")) + else if (! strcmp (_dl_argv[1], "--verify")) { mode = verify; ++_dl_skip_args; --_dl_argc; - ++INTUSE(_dl_argv); + ++_dl_argv; } - else if (! strcmp (INTUSE(_dl_argv)[1], "--inhibit-cache")) + else if (! strcmp (_dl_argv[1], "--inhibit-cache")) { GLRO(dl_inhibit_cache) = 1; ++_dl_skip_args; --_dl_argc; - ++INTUSE(_dl_argv); + ++_dl_argv; } - else if (! strcmp (INTUSE(_dl_argv)[1], "--library-path") + else if (! strcmp (_dl_argv[1], "--library-path") && _dl_argc > 2) { - library_path = INTUSE(_dl_argv)[2]; + library_path = _dl_argv[2]; _dl_skip_args += 2; _dl_argc -= 2; - INTUSE(_dl_argv) += 2; + _dl_argv += 2; } - else if (! strcmp (INTUSE(_dl_argv)[1], "--inhibit-rpath") + else if (! strcmp (_dl_argv[1], "--inhibit-rpath") && _dl_argc > 2) { - GLRO(dl_inhibit_rpath) = INTUSE(_dl_argv)[2]; + GLRO(dl_inhibit_rpath) = _dl_argv[2]; _dl_skip_args += 2; _dl_argc -= 2; - INTUSE(_dl_argv) += 2; + _dl_argv += 2; } - else if (! strcmp (INTUSE(_dl_argv)[1], "--audit") && _dl_argc > 2) + else if (! strcmp (_dl_argv[1], "--audit") && _dl_argc > 2) { - process_dl_audit (INTUSE(_dl_argv)[2]); + process_dl_audit (_dl_argv[2]); _dl_skip_args += 2; _dl_argc -= 2; - INTUSE(_dl_argv) += 2; + _dl_argv += 2; } else break; @@ -886,7 +886,7 @@ of this helper program; chances are you did not intend to run this program.\n\ ++_dl_skip_args; --_dl_argc; - ++INTUSE(_dl_argv); + ++_dl_argv; /* The initialization of _dl_stack_flags done below assumes the executable's PT_GNU_STACK may have been honored by the kernel, and @@ -1800,7 +1800,7 @@ ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n", ElfW(Addr) loadbase; lookup_t result; - result = _dl_lookup_symbol_x (INTUSE(_dl_argv)[i], main_map, + result = _dl_lookup_symbol_x (_dl_argv[i], main_map, &ref, main_map->l_scope, NULL, ELF_RTYPE_CLASS_PLT, DL_LOOKUP_ADD_DEPENDENCY, NULL); @@ -1808,7 +1808,7 @@ ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n", loadbase = LOOKUP_VALUE_ADDRESS (result); _dl_printf ("%s found at 0x%0*Zd in object at 0x%0*Zd\n", - INTUSE(_dl_argv)[i], + _dl_argv[i], (int) sizeof ref->st_value * 2, (size_t) ref->st_value, (int) sizeof loadbase * 2, (size_t) loadbase); diff --git a/sysdeps/alpha/dl-machine.h b/sysdeps/alpha/dl-machine.h index e6537dd..64385ad 100644 --- a/sysdeps/alpha/dl-machine.h +++ b/sysdeps/alpha/dl-machine.h @@ -184,16 +184,16 @@ $fixup_stack: \n\ /* Adjust the stack pointer to skip _dl_skip_args words.\n\ This involves copying everything down, since the \n\ stack pointer must always be 16-byte aligned. */ \n\ - ldah $7, _dl_argv_internal($gp) !gprelhigh \n\ + ldah $7, __GI__dl_argv($gp) !gprelhigh \n\ ldq $2, 0($sp) \n\ - ldq $5, _dl_argv_internal($7) !gprellow \n\ + ldq $5, __GI__dl_argv($7) !gprellow \n\ subq $31, $1, $6 \n\ subq $2, $1, $2 \n\ s8addq $6, $5, $5 \n\ mov $sp, $4 \n\ s8addq $1, $sp, $3 \n\ stq $2, 0($sp) \n\ - stq $5, _dl_argv_internal($7) !gprellow \n\ + stq $5, __GI__dl_argv($7) !gprellow \n\ /* Copy down argv. */ \n\ 0: ldq $5, 8($3) \n\ addq $4, 8, $4 \n\ diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h index e01df84..d1c8e2c 100644 --- a/sysdeps/generic/ldsodefs.h +++ b/sysdeps/generic/ldsodefs.h @@ -641,6 +641,7 @@ extern char **_dl_argv attribute_relro #endif ; +rtld_hidden_proto (_dl_argv) #ifdef IS_IN_rtld extern unsigned int _dl_skip_args attribute_hidden # ifndef DL_ARGV_NOT_RELRO @@ -652,15 +653,8 @@ extern unsigned int _dl_skip_args_internal attribute_hidden attribute_relro # endif ; -extern char **_dl_argv_internal attribute_hidden -# ifndef DL_ARGV_NOT_RELRO - attribute_relro -# endif - ; -# define rtld_progname (INTUSE(_dl_argv)[0]) -#else -# define rtld_progname _dl_argv[0] #endif +#define rtld_progname _dl_argv[0] /* Flag set at startup and cleared when the last initializer has run. */ extern int _dl_starting_up; diff --git a/sysdeps/powerpc/powerpc32/dl-start.S b/sysdeps/powerpc/powerpc32/dl-start.S index b2078d4..be8ce44 100644 --- a/sysdeps/powerpc/powerpc32/dl-start.S +++ b/sysdeps/powerpc/powerpc32/dl-start.S @@ -54,7 +54,7 @@ _dl_start_user: /* &_dl_argc in 29, &_dl_argv in 27, and _dl_loaded in 28. */ lwz r28,_rtld_local@got(r31) lwz r29,_dl_argc@got(r31) - lwz r27,INTUSE(_dl_argv)@got(r31) + lwz r27,__GI__dl_argv@got(r31) /* Call _dl_init (_dl_loaded, _dl_argc, _dl_argv, _dl_argv+_dl_argc+1). */ lwz r3,0(r28) diff --git a/sysdeps/powerpc/powerpc64/dl-machine.h b/sysdeps/powerpc/powerpc64/dl-machine.h index 735a549..3007e80 100644 --- a/sysdeps/powerpc/powerpc64/dl-machine.h +++ b/sysdeps/powerpc/powerpc64/dl-machine.h @@ -169,7 +169,7 @@ DL_STARTING_UP_DEF \ ".LC__dl_argc:\n" \ " .tc _dl_argc[TC],_dl_argc\n" \ ".LC__dl_argv:\n" \ -" .tc _dl_argv_internal[TC],_dl_argv_internal\n" \ +" .tc __GI__dl_argv[TC],__GI__dl_argv\n" \ ".LC__dl_fini:\n" \ " .tc _dl_fini[TC],_dl_fini\n" \ " .popsection\n" \