Re: patch to make init_array work (2nd version; resend)

Re: patch to make init_array work (2nd version; resend)

[Roland McGrath]

When this approach came up before, Ulrich also objected to the implementation,
see http://sources.redhat.com/ml/libc-alpha/2002-03/msg00063.html.
In future repostings of patches previously discussed, it would be helpful
if you could cite the prior posting and discussion in the mailing list
archives, for the benefit of people like me who were not involved in the
discussion the first time around.

For dynamic executables, the dynamic linker or libc could in fact examine
the DT_INIT_ARRAY slots directly, rather than having the crt1.o code supply
the values.  I think this is what Ulrich wanted to see from what he said in
the message cited above.

The same is true of DT_INIT/DT_FINI, but for whatever reason the convention
inherited from SVR4 is to have the startup code call its own _init and
_fini entry points rather than the dynamic linker doing it.  

That suggested approach is not available for static linking, however.  We
need to keep the same crt1.o code for both static and dynamic linking (or
else all go raving mad).  So it is not entirely clear to me what exact
criteria the ideal solution should meet.

I am inclined to think the plan we discussed yesterday is the best one.
But if Ulrich prefers to have ld.so or libc look up DT_INIT_ARRAY et al,
then I would like to hear his thoughts on the plan for static linking.



Thanks,
Roland

Re: patch to make init_array work (2nd version; resend)

[David Mosberger]

>>>>> On Fri, 8 Nov 2002 11:34:31 -0800, Roland McGrath <roland@frob.com> said:

  Roland> When this approach came up before, Ulrich also objected to
  Roland> the implementation, see
  Roland> http://sources.redhat.com/ml/libc-alpha/2002-03/msg00063.html.
  Roland> In future repostings of patches previously discussed, it
  Roland> would be helpful if you could cite the prior posting and
  Roland> discussion in the mailing list archives, for the benefit of
  Roland> people like me who were not involved in the discussion the
  Roland> first time around.

Yes, though I did point to the thread earlier on
(see http://sources.redhat.com/ml/libc-hacker/2002-10/msg00085.html).

  Roland> That suggested approach is not available for static linking,
  Roland> however.

Right.

  Roland> I am inclined to think the plan we discussed yesterday is
  Roland> the best one.

I like it, too.

	--david

patch to make init_array work (3nd version)

[David Mosberger]

Below is a revised patch to get init_array working.  This is basically
what Richard suggested.  Roland, you suggested that the new routines
in csu/init.c could be "static", but I don't see how this would work
given that the routines need to be accessed from start.S.

A problem with this new approach is that the preinit_array functions
need to be executed by csu/init.c only for statically linked programs.
I don't know of a good way to detect this inside init.c, so I changed
the init callback to take an extra argument (which will be ignored on
platforms that don't support init_array).

To be honest, HJ's original solution seems nicer to me (e.g., doesn't
require changes to platform-specific code), though I see the point
about additional relocs.

Note: the patch below makes init_array working on ia64 only.  Other
platforms would have to make the analogous (trivial) change to
start.S.

In terms of testing, "make check" passed, so the basics seem to be
right.

	--david

ChangeLog

2002-11-27  David Mosberger  <davidm@hpl.hp.com>

	* sysdeps/generic/libc-start.c (__libc_start_main): Declare
	INIT callback as taking an integer argument indicating whether
	program is a statically-linked executable.  Update call-site
	accordingly.

	* elf/Makefile (tests): Re-enable init_array tests.

	* csu/init.c (__preinit_array_start): Declare.
	(__preinit_array_end): Ditto.
	(__init_array_start): Ditto.
	(__init_array_end): Ditto.
	(__fini_array_start): Ditto.
	(__fini_array_end): Ditto.
	(_init): Declare as an external function.
	(_fini): Ditto.
	(__libc_do_init_calls): New function.
	(__libc_do_fini_calls): Ditto.

Index: csu/init.c
===================================================================
RCS file: /cvs/glibc/libc/csu/init.c,v
retrieving revision 1.4
diff -u -r1.4 init.c
--- csu/init.c	6 Jul 2001 04:54:45 -0000	1.4
+++ csu/init.c	27 Nov 2002 22:28:11 -0000
@@ -25,3 +25,54 @@
 const int _IO_stdin_used = _G_IO_IO_FILE_VERSION;
 
 #endif
+
+#ifdef HAVE_INITFINI_ARRAY
+extern void (*__preinit_array_start []) (void);
+extern void (*__preinit_array_end []) (void);
+extern void (*__init_array_start []) (void);
+extern void (*__init_array_end []) (void);
+extern void (*__fini_array_start []) (void);
+extern void (*__fini_array_end []) (void);
+#endif
+
+extern void _init (void);
+extern void _fini (void);
+
+void
+__libc_do_init_calls (int static_executable)
+{
+  size_t size, i;
+
+#ifdef HAVE_INITFINI_ARRAY
+  /* For dynamically linked executables the preinit array must be
+     executed by the loader (before initializing any shared
+     object.  */
+  if (static_executable)
+    {
+      size = __preinit_array_end - __preinit_array_start;
+      for (i = 0; i < size; i++)
+	(*__preinit_array_start [i]) ();
+    }
+#endif
+
+  _init ();
+
+#ifdef HAVE_INITFINI_ARRAY
+  size =  __init_array_end - __init_array_start;
+  for (i = 0; i < size; i++)
+    (*__init_array_start [i]) ();
+#endif
+}
+
+void
+__libc_do_fini_calls (void)
+{
+  size_t i;
+
+#ifdef HAVE_INITFINI_ARRAY
+  for (i = __fini_array_end - __fini_array_start; i-- > 0; )
+    (*__fini_array_start [i]) ();
+#endif
+
+  _fini ();
+}
Index: elf/Makefile
===================================================================
RCS file: /cvs/glibc/libc/elf/Makefile,v
retrieving revision 1.245
diff -u -r1.245 Makefile
--- elf/Makefile	23 Nov 2002 21:34:16 -0000	1.245
+++ elf/Makefile	27 Nov 2002 22:28:11 -0000
@@ -118,7 +118,7 @@
 
 tests = tst-tls1 tst-tls2 tst-tls9
 ifeq (yes,$(have-initfini-array))
-#tests += tst-array1 tst-array2 tst-array3
+tests += tst-array1 tst-array2 tst-array3
 endif
 ifeq (yes,$(build-static))
 tests-static = tst-tls1-static tst-tls2-static
Index: sysdeps/generic/libc-start.c
===================================================================
RCS file: /cvs/glibc/libc/sysdeps/generic/libc-start.c,v
retrieving revision 1.33
diff -u -r1.33 libc-start.c
--- sysdeps/generic/libc-start.c	25 Oct 2002 19:41:24 -0000	1.33
+++ sysdeps/generic/libc-start.c	27 Nov 2002 22:28:11 -0000
@@ -40,7 +40,7 @@
 extern int BP_SYM (__libc_start_main) (int (*main) (int, char **, char **),
 				       int argc,
 				       char *__unbounded *__unbounded ubp_av,
-				       void (*init) (void),
+				       void (*init) (int),
 				       void (*fini) (void),
 				       void (*rtld_fini) (void),
 				       void *__unbounded stack_end)
@@ -51,7 +51,7 @@
    BPs in the arglist of startup_info.main and startup_info.init. */
 BP_SYM (__libc_start_main) (int (*main) (int, char **, char **),
 		   int argc, char *__unbounded *__unbounded ubp_av,
-		   void (*init) (void), void (*fini) (void),
+		   void (*init) (int), void (*fini) (void),
 		   void (*rtld_fini) (void), void *__unbounded stack_end)
 {
   char *__unbounded *__unbounded ubp_ev = &ubp_av[argc + 1];
@@ -121,7 +121,11 @@
     _dl_debug_printf ("\ninitialize program: %s\n\n", argv[0]);
 #endif
   if (init)
-    (*init) ();
+# ifdef SHARED
+    (*init) (0);
+# else
+    (*init) (1);
+# endif
 
 #ifdef SHARED
   if (__builtin_expect (GL(dl_debug_mask) & DL_DEBUG_IMPCALLS, 0))
Index: sysdeps/ia64/elf/start.S
===================================================================
RCS file: /cvs/glibc/libc/sysdeps/ia64/elf/start.S,v
retrieving revision 1.10
diff -u -r1.10 start.S
--- sysdeps/ia64/elf/start.S	6 Jul 2001 04:55:54 -0000	1.10
+++ sysdeps/ia64/elf/start.S	27 Nov 2002 22:28:11 -0000
@@ -1,4 +1,4 @@
-/* Copyright (C) 1999, 2000, 2001 Free Software Foundation, Inc.
+/* Copyright (C) 1999, 2000, 2001, 2002 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
    Written by Jes Sorensen, <Jes.Sorensen@cern.ch>, April 1999.
 
@@ -64,13 +64,13 @@
 	{
 	  addl r11 = @ltoff(__libc_ia64_register_backing_store_base), gp
 	  addl out0 = @ltoff(@fptr(main)), gp
-	  addl out3 = @ltoff(@fptr(_init)), gp
+	  addl out3 = @ltoff(@fptr(__libc_do_init_calls)), gp
 	  ;;
 	}
 	{ .mmi
 	  ld8 r3 = [r11]	/* pointer to __libc_ia64_register_backing_store_base */
 	  ld8 out0 = [out0]	/* pointer to `main' function descriptor */
-	  addl out4 = @ltoff(@fptr(_fini)), gp
+	  addl out4 = @ltoff(@fptr(__libc_do_fini_calls)), gp
 	  ;;
 	}
 	{ .mmi

Re: patch to make init_array work (3nd version)

[H. J. Lu]

On Wed, Nov 27, 2002 at 02:35:33PM -0800, David Mosberger wrote:
> Below is a revised patch to get init_array working.  This is basically
> what Richard suggested.  Roland, you suggested that the new routines
> in csu/init.c could be "static", but I don't see how this would work
> given that the routines need to be accessed from start.S.
> 
> A problem with this new approach is that the preinit_array functions
> need to be executed by csu/init.c only for statically linked programs.
> I don't know of a good way to detect this inside init.c, so I changed
> the init callback to take an extra argument (which will be ignored on
> platforms that don't support init_array).
> 
> To be honest, HJ's original solution seems nicer to me (e.g., doesn't
> require changes to platform-specific code), though I see the point
> about additional relocs.

I am not sure how this approach will work. If I get it right,
__libc_do_init_calls and __libc_do_fini_calls are defined in
executables. But they are only available when executables are
linked against with the new glibc. For the existing executables,
there are no __libc_do_init_calls nor __libc_do_fini_calls. That
is why my original solution has

#ifdef HAVE_INITFINI_ARRAY
# ifdef SHARED
extern void __libc_init_array (void) __attribute__ ((weak));
extern void __libc_fini_array (void) __attribute__ ((weak));
# else
extern void __libc_preinit_array (void); 
extern void __libc_init_array (void); 
extern void __libc_fini_array (void); 
# endif
#endif

...

#ifdef HAVE_INITFINI_ARRAY
# ifdef SHARED
  if (__libc_fini_array)
# endif
    __cxa_atexit ((void (*) (void *)) __libc_fini_array, NULL, NULL);

# ifndef SHARED
  __libc_preinit_array ();
# endif
#endif

which will work with all exutables.

> 
> Note: the patch below makes init_array working on ia64 only.  Other
> platforms would have to make the analogous (trivial) change to
> start.S.
> 
> In terms of testing, "make check" passed, so the basics seem to be
> right.
> 
> 	--david
> 
> ChangeLog
> 
> 2002-11-27  David Mosberger  <davidm@hpl.hp.com>
> 
> 	* sysdeps/generic/libc-start.c (__libc_start_main): Declare
> 	INIT callback as taking an integer argument indicating whether
> 	program is a statically-linked executable.  Update call-site
> 	accordingly.
> 
> 	* elf/Makefile (tests): Re-enable init_array tests.
> 
> 	* csu/init.c (__preinit_array_start): Declare.
> 	(__preinit_array_end): Ditto.
> 	(__init_array_start): Ditto.
> 	(__init_array_end): Ditto.
> 	(__fini_array_start): Ditto.
> 	(__fini_array_end): Ditto.
> 	(_init): Declare as an external function.
> 	(_fini): Ditto.
> 	(__libc_do_init_calls): New function.
> 	(__libc_do_fini_calls): Ditto.
> 

You missed the ChangeLog entry for sysdeps/ia64/elf/start.S.


H.J.

Re: patch to make init_array work (3nd version)

[David Mosberger]

>>>>> On Wed, 27 Nov 2002 15:09:58 -0800, "H. J. Lu" <hjl@lucon.org> said:

  HJ> I am not sure how this approach will work. If I get it right,
  HJ> __libc_do_init_calls and __libc_do_fini_calls are defined in
  HJ> executables. But they are only available when executables are
  HJ> linked against with the new glibc. For the existing executables,
  HJ> there are no __libc_do_init_calls nor __libc_do_fini_calls. That
  HJ> is why my original solution has

Old executables directly pass _init() and _fini() to
__libc_start_main, so they'll work normally.  Of course, the
preinit_array, init_array, and fini_array in such executables won't be
executed either, but since this support is new, those binaries
presumably don't have such sections.  Also, ld.so should take care of
executing init_array and fini_array for the shared objects that the
executable depends on, so I think everything should be fine.

	--david

Re: patch to make init_array work (3nd version)

[Roland McGrath]

I had not been aware of the difference in behavior for static vs dynamic
executables when making my previous suggestion.  Your change to the calling
convention of the function passed to __libc_start_main would require symbol
versioning of __libc_start_main to avoid creating new executables with the
false appearance of compatibility with existing shared libraries.  It's
also an ugly kludge that there is no need for.

I reimplemented it myself.  I put the functions called from start.S into
libc.a and libc_nonshared.a with different versions for the two cases, so
the correct behavior (calling the executable's preinit array or not) is
selected at link time instead of using a kludge with run-time overhead.  
I also performed the query replace on all the start.S files.  

No patch to the generic code in libc is complete without updating all the
existing sysdeps code, and it is not hard to do a query replace.  For
future contributions that change generic code in ways that necessitate
updating sysdeps code, please include those changes if you expect your
patches to go in as they are or in a timely fashion.


Thanks,
Roland

Re: patch to make init_array work (3nd version)

[David Mosberger]

>>>>> On Mon, 9 Dec 2002 01:13:13 -0800, Roland McGrath <roland@redhat.com> said:

  Roland> I reimplemented it myself.  I put the functions called from
  Roland> start.S into libc.a and libc_nonshared.a with different
  Roland> versions for the two cases, so the correct behavior (calling
  Roland> the executable's preinit array or not) is selected at link
  Roland> time instead of using a kludge with run-time overhead.

Cool, that is a much nicer solution.

I'm glad that init_array will finally be working.

Thanks,

	--david