[PATCH] move __gmon_start__ call out of .init section

[PATCH] move __gmon_start__ call out of .init section

[David Mosberger]

The patch below changes the ia64 initfini.c such that the call to
__gmon_start__ is done via the .init_array section (if available).
The idea here is to keep the .init/.fini empty (apart from
prologue/epilogue) so as to ensure that the unwind info is always
correct.

Note: this patch should be applied _after_ HJ's init_array fixes as
otherwise gmon startup would fail for profiled binaries.

Thanks,

	--david

2002-10-29  David Mosberger  <davidm@hpl.hp.com>

	* sysdeps/ia64/elf/initfini.c [HAVE_INITFINI_ARRAY]
	(gmon_initializer): New function.
	(.init prologue): If HAVE_INITFINI_ARRAY is true, don't call
	__gmon_start__ here.  Call it from gmon_initializer() instead.

Index: sysdeps/ia64/elf/initfini.c
===================================================================
RCS file: /cvs/glibc/libc/sysdeps/ia64/elf/initfini.c,v
retrieving revision 1.3
diff -u -r1.3 initfini.c
--- sysdeps/ia64/elf/sysdeps/ia64/elf/initfini.c	25 Aug 2002 00:34:23 -0000	1.3
+++ sysdeps/ia64/elf/sysdeps/ia64/elf/initfini.c	30 Oct 2002 04:37:13 -0000
@@ -27,81 +27,110 @@
    * crtn.s puts the corresponding function epilogues
    in the .init and .fini sections. */
 
-__asm__ ("\n\
-\n\
-#include \"defs.h\"\n\
-\n\
-/*@HEADER_ENDS*/\n\
-\n\
-/*@_init_PROLOG_BEGINS*/\n\
-	.section .init\n\
-	.align 16\n\
-	.global _init#\n\
-	.proc _init#\n\
-_init:\n\
-	alloc r34 = ar.pfs, 0, 3, 0, 0\n\
-	mov r32 = r12\n\
-	mov r33 = b0\n\
-	adds r12 = -16, r12\n\
-	addl r14 = @ltoff(@fptr(__gmon_start__#)), gp\n\
-	;;\n\
-	ld8 r15 = [r14]\n\
-	;;\n\
-	cmp.eq p6, p7 = 0, r15\n\
-	(p6) br.cond.dptk .L5\n\
-\n\
-/* we could use r35 to save gp, but we use the stack since that's what\n\
- * all the other init routines will do --davidm 00/04/05 */\n\
-	st8 [r12] = gp, -16\n\
-	br.call.sptk.many b0 = __gmon_start__# ;;\n\
-	adds r12 = 16, r12\n\
-	;;\n\
-	ld8 gp = [r12]\n\
-	;;\n\
-.L5:\n\
-	.align 16\n\
-	.endp _init#\n\
-\n\
-/*@_init_PROLOG_ENDS*/\n\
-\n\
-/*@_init_EPILOG_BEGINS*/\n\
-	.section .init\n\
-	.regstk 0,2,0,0\n\
-	mov r12 = r32\n\
-	mov ar.pfs = r34\n\
-	mov b0 = r33\n\
-	br.ret.sptk.many b0\n\
-	.endp _init#\n\
-/*@_init_EPILOG_ENDS*/\n\
-\n\
-/*@_fini_PROLOG_BEGINS*/\n\
-	.section .fini\n\
-	.align 16\n\
-	.global _fini#\n\
-	.proc _fini#\n\
-_fini:\n\
-	alloc r34 = ar.pfs, 0, 3, 0, 0\n\
-	mov r32 = r12\n\
-	mov r33 = b0\n\
-	adds r12 = -16, r12\n\
-	;;\n\
-	.align 16\n\
-	.endp _fini#\n\
-\n\
-/*@_fini_PROLOG_ENDS*/\n\
-	br.call.sptk.many b0 = i_am_not_a_leaf# ;;\n\
-	;;\n\
-\n\
-/*@_fini_EPILOG_BEGINS*/\n\
-	.section .fini\n\
-	mov r12 = r32\n\
-	mov ar.pfs = r34\n\
-	mov b0 = r33\n\
-	br.ret.sptk.many b0\n\
-	.endp _fini#\n\
-\n\
-/*@_fini_EPILOG_ENDS*/\n\
-\n\
-/*@TRAILER_BEGINS*/\n\
-	.weak	__gmon_start__#\n\
-");
+__asm__ ("\n\n"
+"#include \"defs.h\"\n"
+"\n"
+"/*@HEADER_ENDS*/\n"
+"\n"
+"/*@_init_PROLOG_BEGINS*/\n");
+
+#ifdef HAVE_INITFINI_ARRAY
+
+/* If we have working .init_array support, we want to keep the .init
+   section empty (apart from the mandatory prologue/epilogue.  This
+   ensures that the default unwind conventions (return-pointer in b0,
+   frame state in ar.pfs, etc.)  will do the Right Thing.  To ensure
+   an empty .init section, we register gmon_initializer() via the
+   .init_array.
+
+	--davidm 02/10/29 */
+
+static void
+gmon_initializer (void)
+{
+  extern void weak_function __gmon_start__ (void);
+
+  if (__gmon_start__)
+    (*__gmon_start__)();
+}
+
+__asm__ (".section .init_array, \"aw\"\n"
+	 "\tdata8 @fptr(gmon_initializer)\n");
+
+#endif
+
+__asm__ (".section .init\n"
+"	.align 16\n"
+"	.global _init#\n"
+"	.proc _init#\n"
+"_init:\n"
+"	alloc r34 = ar.pfs, 0, 3, 0, 0\n"
+"	mov r32 = r12\n"
+"	mov r33 = b0\n"
+"	adds r12 = -16, r12\n"
+#ifdef HAVE_INITFINI_ARRAY
+ "	;;\n"		/* see gmon_initializer() below */
+#else
+"	.weak	__gmon_start__#\n"
+"	addl r14 = @ltoff(@fptr(__gmon_start__#)), gp\n"
+"	;;\n"
+"	ld8 r15 = [r14]\n"
+"	;;\n"
+"	cmp.eq p6, p7 = 0, r15\n"
+"	(p6) br.cond.dptk .L5\n"
+"\n"
+"/* we could use r35 to save gp, but we use the stack since that's what\n"
+" * all the other init routines will do --davidm 00/04/05 */\n"
+"	st8 [r12] = gp, -16\n"
+"	br.call.sptk.many b0 = __gmon_start__# ;;\n"
+"	adds r12 = 16, r12\n"
+"	;;\n"
+"	ld8 gp = [r12]\n"
+"	;;\n"
+".L5:\n"
+#endif
+"	.align 16\n"
+"	.endp _init#\n"
+"\n"
+"/*@_init_PROLOG_ENDS*/\n"
+"\n"
+"/*@_init_EPILOG_BEGINS*/\n"
+"	.section .init\n"
+"	.regstk 0,2,0,0\n"
+"	mov r12 = r32\n"
+"	mov ar.pfs = r34\n"
+"	mov b0 = r33\n"
+"	br.ret.sptk.many b0\n"
+"	.endp _init#\n"
+"/*@_init_EPILOG_ENDS*/\n"
+"\n"
+"/*@_fini_PROLOG_BEGINS*/\n"
+"	.section .fini\n"
+"	.align 16\n"
+"	.global _fini#\n"
+"	.proc _fini#\n"
+"_fini:\n"
+"	alloc r34 = ar.pfs, 0, 3, 0, 0\n"
+"	mov r32 = r12\n"
+"	mov r33 = b0\n"
+"	adds r12 = -16, r12\n"
+"	;;\n"
+"	.align 16\n"
+"	.endp _fini#\n"
+"\n"
+"/*@_fini_PROLOG_ENDS*/\n"
+"	br.call.sptk.many b0 = i_am_not_a_leaf# ;;\n"
+"	;;\n"
+"\n"
+"/*@_fini_EPILOG_BEGINS*/\n"
+"	.section .fini\n"
+"	mov r12 = r32\n"
+"	mov ar.pfs = r34\n"
+"	mov b0 = r33\n"
+"	br.ret.sptk.many b0\n"
+"	.endp _fini#\n"
+"\n"
+"/*@_fini_EPILOG_ENDS*/\n"
+"\n"
+"/*@TRAILER_BEGINS*/\n"
+);

Re: [PATCH] move __gmon_start__ call out of .init section

[Roland McGrath]

> The patch below changes the ia64 initfini.c such that the call to
> __gmon_start__ is done via the .init_array section (if available).
> The idea here is to keep the .init/.fini empty (apart from
> prologue/epilogue) so as to ensure that the unwind info is always
> correct.

Can you elaborate a bit on the problem you are addressing here?

This seems wrong to me, in that the user program might put some code into
the .init section and then __gmon_start__ would come after it instead of
before.

Re: [PATCH] move __gmon_start__ call out of .init section

[David Mosberger]

>>>>> On Thu, 7 Nov 2002 13:45:47 -0800, Roland McGrath <roland@redhat.com> said:

  >> The patch below changes the ia64 initfini.c such that the call to
  >> __gmon_start__ is done via the .init_array section (if
  >> available).  The idea here is to keep the .init/.fini empty
  >> (apart from prologue/epilogue) so as to ensure that the unwind
  >> info is always correct.

  Roland> Can you elaborate a bit on the problem you are addressing
  Roland> here?

The problem is that the unwind info for the .init/.fini section will
be incorrect if it contains any function call.  It's easiest to make a
concrete example:

 - on ia64, if there is no explicit unwind info for a piece of code,
   the assumption is that the return address is stored in branch
   register b0

 - if .init contains a call, then the value in the return address
   register (b0) needs to be saved somewhere (normally on the memory
   stack); but since .init is created "dynamically" by the linker, the
   unwind info for .init will be wrong and the fact that b0 has been
   saved on the memory stack won't be recorded anywhere

Of course, one could hack the linker to correct the unwind info but
this would be fragile and ugly, so the preferred solution is to
deprecate .init/.fini alltogether and to use .init_array/.fini_array
instead (which is much easier to use anyhow).

  Roland> This seems wrong to me, in that the user program might put
  Roland> some code into the .init section and then __gmon_start__
  Roland> would come after it instead of before.

That's true, but .init is deprecated on ia64 anyhow.  If legacy code
doesn't get profiled because it was using .init, I doubt that's a big
loss.

Perhaps the call could be moved into .preinit_array?  I'm not entirely
sure whether that would be safe though.

	--david

Re: [PATCH] move __gmon_start__ call out of .init section

[Roland McGrath]

> The problem is that the unwind info for the .init/.fini section will
> be incorrect if it contains any function call.

I don't really see why it should matter for these functions (when do you
unwind through them?), but I can take your word for it that it does.

> That's true, but .init is deprecated on ia64 anyhow.  If legacy code
> doesn't get profiled because it was using .init, I doubt that's a big
> loss.

Ok.  

> Perhaps the call could be moved into .preinit_array?  I'm not entirely
> sure whether that would be safe though.

It's a weak reference and so might be zero.  There is no provision in the
spec for preinit_array elements being zero, so it would be questionable to
have it skip them (rather than a user putting a random zero into the array
getting the crash he deserves).


It sounds like this is really an ia64-specific issue and so there isn't any
question of wanting to do this for the other platforms.  So I will put your
change in.  Please fix whatever you are using to munge your diffs so that
it produces correct file names instead of the garbage all your recent
patches have contained.  After today I won't be in the mood to manually
unmunge patches that can't be applied by any single -pN setting.


Thanks,
Roland

Re: [PATCH] move __gmon_start__ call out of .init section

[David Mosberger]

>>>>> On Thu, 7 Nov 2002 14:27:35 -0800, Roland McGrath <roland@redhat.com> said:

  >> The problem is that the unwind info for the .init/.fini section
  >> will be incorrect if it contains any function call.

  Roland> I don't really see why it should matter for these functions
  Roland> (when do you unwind through them?), but I can take your word
  Roland> for it that it does.

It matters during debugging for example (that's how I originally found
the problem).  There are other reasons.  For example, a profiling tool
might want to periodically create a stack trace to create call-graph
profiles.

  Roland> It's a weak reference and so might be zero.  There is no
  Roland> provision in the spec for preinit_array elements being zero,
  Roland> so it would be questionable to have it skip them (rather
  Roland> than a user putting a random zero into the array getting the
  Roland> crash he deserves).

We wouldn't be putting __gmon_init__ directly into .preinit---we would
put a function there which will then check whether __gmon_init__ is
NULL and, if not, call it.

  Roland> It sounds like this is really an ia64-specific issue and so
  Roland> there isn't any question of wanting to do this for the other
  Roland> platforms.  So I will put your change in.

OK, thanks.

  Roland> Please fix whatever you are using to munge your diffs so
  Roland> that it produces correct file names instead of the garbage
  Roland> all your recent patches have contained.  After today I won't
  Roland> be in the mood to manually unmunge patches that can't be
  Roland> applied by any single -pN setting.

Hmmh, I'm sorry about that.  I was using a script that Uli sent me
some time ago.  I'm not a (heavy) CVS user but Uli told me that the
patches created by "cvs diff -U" are broken.  What's the recommend way
to create a diff from the glibc CVS?

	--david

Re: [PATCH] move __gmon_start__ call out of .init section

[Roland McGrath]

> We wouldn't be putting __gmon_init__ directly into .preinit---we would
> put a function there which will then check whether __gmon_init__ is
> NULL and, if not, call it.

Oh, well I don't see any potential problem with that then.  

> Hmmh, I'm sorry about that.  I was using a script that Uli sent me
> some time ago.  I'm not a (heavy) CVS user but Uli told me that the
> patches created by "cvs diff -U" are broken.  What's the recommend way
> to create a diff from the glibc CVS?

My cvs version calls itself 1.11.1p1, and it produces diffs with proper
file names in them for cvs diff by itself.  Probably yours does too, and
the script you are using that was necessary for older cvs versions does not
mix well with already-correct input.

Re: [PATCH] move __gmon_start__ call out of .init section

[Andreas Schwab]

Roland McGrath <roland@redhat.com> writes:

|> > Hmmh, I'm sorry about that.  I was using a script that Uli sent me
|> > some time ago.  I'm not a (heavy) CVS user but Uli told me that the
|> > patches created by "cvs diff -U" are broken.  What's the recommend way
|> > to create a diff from the glibc CVS?
|> 
|> My cvs version calls itself 1.11.1p1, and it produces diffs with proper
|> file names in them for cvs diff by itself.  Probably yours does too, and
|> the script you are using that was necessary for older cvs versions does not
|> mix well with already-correct input.

The problem is not the client, but the server (it's the latter that
produces the diffs).  It seems like the cvs server on sources.redhat.com
has been upgraded already, so the script should not be necessary here any
more.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Deutschherrnstr. 15-19, D-90429 Nürnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] move __gmon_start__ call out of .init section

[David Mosberger]

>>>>> On Thu, 7 Nov 2002 17:43:33 -0800, Roland McGrath <roland@redhat.com> said:

  Roland> My cvs version calls itself 1.11.1p1, and it produces diffs
  Roland> with proper file names in them for cvs diff by itself.
  Roland> Probably yours does too, and the script you are using that
  Roland> was necessary for older cvs versions does not mix well with
  Roland> already-correct input.

Ah, that makes sense (along with Andreas' comments).  Thanks for the help.

	--david