diff mbox

Don't use INTDEF/INTUSE with _dl_argv (bug 14132)

Message ID Pine.LNX.4.64.1410210044550.30508@digraph.polyomino.org.uk
State Committed
Headers

Commit Message

Joseph Myers Oct. 21, 2014, 12:45 a.m. UTC 
  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.

Comments

Andreas Schwab Oct. 21, 2014, 8:22 a.m. UTC | #1 
Shouldn't sysdeps/ia64/dl-machine.h be using __GI__dl_argv as well?

Andreas.
Joseph Myers Oct. 21, 2014, 12:39 p.m. UTC | #2 
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).
Andreas Schwab Oct. 21, 2014, 12:43 p.m. UTC | #3 
"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.
Joseph Myers Oct. 21, 2014, 12:56 p.m. UTC | #4 
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.
Andreas Schwab Oct. 21, 2014, 3:16 p.m. UTC | #5 
"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.
Joseph Myers Oct. 21, 2014, 10:06 p.m. UTC | #6 
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.
Joseph Myers Oct. 27, 2014, 12:28 p.m. UTC | #7 
Ping.  This patch 
<https://sourceware.org/ml/libc-alpha/2014-10/msg00443.html> is pending 
review.
Joseph Myers Nov. 3, 2014, 6:15 p.m. UTC | #8 
Ping^2.  This patch 
<https://sourceware.org/ml/libc-alpha/2014-10/msg00443.html> is still 
pending review.
Siddhesh Poyarekar Nov. 4, 2014, 5:16 p.m. UTC | #9 
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
Joseph Myers Nov. 4, 2014, 5:41 p.m. UTC | #10 
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 mbox

Patch

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"							\