[PATCH] MIPS: Add option to pass return address location to _mcount. Was: [PATCH -v4 4/9] tracing: add static function tracer support for MIPS

[PATCH] MIPS: Add option to pass return address location to _mcount. Was: [PATCH -v4 4/9] tracing: add static function tracer support for MIPS

[David Daney]

[-- Attachment #1: Type: text/plain, Size: 3509 bytes --]

Richard Sandiford wrote:
> David Daney <ddaney@caviumnetworks.com> writes:
>> Wu Zhangjin wrote:
>>> On Wed, 2009-10-21 at 11:24 -0400, Steven Rostedt wrote:
>> [...]
>>>>> +
>>>>> +NESTED(_mcount, PT_SIZE, ra)
>>>>> +	RESTORE_SP_FOR_32BIT
>>>>> +	PTR_LA	t0, ftrace_stub
>>>>> +	PTR_L	t1, ftrace_trace_function /* please don't use t1 later, safe? */
>>>> Is t0 and t1 safe for mcount to use? Remember, mcount does not follow
>>>> the dynamics of C function ABI.
>>> So, perhaps we can use the saved registers(a0,a1...) instead.
>>>
>> a0..a7 may not always be saved.
>>
>> You can use at, v0, v1 and all the temporary registers.  Note that for 
>> the 64-bit ABIs sometimes the names t0-t4 shadow a4-a7.  So for a 64-bit 
>> kernel, you can use: $1, $2, $3, $12, $13, $14, $15, $24, $25, noting 
>> that at == $1 and contains the callers ra.  For a 32-bit kernel you can 
>> add $8, $9, $10, and $11
>>
>> This whole thing seems a little fragile.
>>
>> I think it might be a good idea to get input from Richard Sandiford, 
>> and/or Adam Nemet about this approach (so I add them to the CC).
>>
>> This e-mail thread starts here:
>>
>> http://www.linux-mips.org/archives/linux-mips/2009-10/msg00286.html
>>
>> and here:
>>
>> http://www.linux-mips.org/archives/linux-mips/2009-10/msg00290.html
> 
> I'm not sure that the "search for a save of RA" thing is really a good idea.
> The last version of that seemed to be "assume that any register stores
> will be in a block that immediately precedes the move into RA", but even
> if that's true now, it might not be in future.  And as Wu Zhangjin says,
> it doesn't cope with long calls, where the target address is loaded
> into a temporary register before the call.
> 
> FWIW, I'd certainly be happy to make GCC pass an additional parameter
> to _mcount.  The parameter could give the address of the return slot,
> or null for leaf functions.  In almost all cases[*], there would be
> no overhead, since the move would go in the delay slot of the call.
> 
> [*] Meaning when the frame is <=32k. ;)  I'm guessing you never
>     get anywhere near that, and if you did, the scan thing wouldn't
>     work anyway.
> 
> The new behaviour could be controlled by a command-line option,
> which would also give linux a cheap way of checking whether the
> feature is available.
> 

How about this patch, I think it does what you suggest.

When we pass -pg -mmcount-raloc, the address of the return address 
relative to sp is passed in $12 to _mcount.  If the return address is 
not saved, $12 will be zero.  I think this will work as registers are 
never saved with an offset of zero.  $12 is a temporary register that is 
not part of the ABI.

$12 is also used by libffi closure support, but I think its use there 
will not interfere with _mcount.

It is very lightly tested, I would bootstrap and regression test with 
some new test cases if it were deemed acceptable.

2009-10-23  David Daney  <ddaney@caviumnetworks.com>

	* doc/invoke.texi (mmcount-raloc): Document new command line option.
	* config/mips/mips.opt (config/mips/mips.opt): New option.
	* config/mips/mips-protos.h (mips_function_profiler): Declare new
	function.
	* config/mips/mips.c (struct mips_frame_info): Add ra_fp_offset member.
	(mips_for_each_saved_gpr_and_fpr): Set ra_fp_offset.
	(mips_raloc_in_delay_slot_p): New function.
	(mips_function_profiler): Moved from FUNCTION_PROFILER, and rewritten.
	* config/mips/mips.h (FUNCTION_PROFILER): Body of macro moved to
	mips_function_profiler.


[-- Attachment #2: mcount.patch --]
[-- Type: text/plain, Size: 8422 bytes --]

Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 153502)
+++ gcc/doc/invoke.texi	(working copy)
@@ -709,7 +709,7 @@ Objective-C and Objective-C++ Dialects}.
 -mbranch-cost=@var{num}  -mbranch-likely  -mno-branch-likely @gol
 -mfp-exceptions -mno-fp-exceptions @gol
 -mvr4130-align -mno-vr4130-align -msynci -mno-synci @gol
--mrelax-pic-calls -mno-relax-pic-calls}
+-mrelax-pic-calls -mno-relax-pic-calls -mmcount-raloc}
 
 @emph{MMIX Options}
 @gccoptlist{-mlibfuncs  -mno-libfuncs  -mepsilon  -mno-epsilon  -mabi=gnu @gol
@@ -14192,6 +14192,20 @@ an assembler and a linker that supports 
 directive and @code{-mexplicit-relocs} is in effect.  With
 @code{-mno-explicit-relocs}, this optimization can be performed by the
 assembler and the linker alone without help from the compiler.
+
+@item -mmcount-raloc
+@itemx -mno-mcount-raloc
+@opindex mmcount-raloc
+@opindex mno-mcount-raloc
+Emit (do not emit) code to pass the offset (from the stack pointer) of
+the return address save location to @code{_mcount}.  The default is
+@option{-mno-mcount-raloc}.
+
+@option{-mmcount-raloc} can be used in conjunction with @option{-pg}
+and a specially coded @code{_mcount} function to record function exit
+by replacing the return address on the stack with a pointer to the
+function exit profiling function.
+
 @end table
 
 @node MMIX Options
Index: gcc/config/mips/mips.opt
===================================================================
--- gcc/config/mips/mips.opt	(revision 153502)
+++ gcc/config/mips/mips.opt	(working copy)
@@ -208,6 +208,10 @@ mlong64
 Target Report RejectNegative Mask(LONG64)
 Use a 64-bit long type
 
+mmcount-raloc
+Target Report Var(TARGET_RALOC)
+Pass the offset from sp of the ra save location to _mcount in $12
+
 mmemcpy
 Target Report Mask(MEMCPY)
 Don't optimize block moves
Index: gcc/config/mips/mips-protos.h
===================================================================
--- gcc/config/mips/mips-protos.h	(revision 153502)
+++ gcc/config/mips/mips-protos.h	(working copy)
@@ -344,5 +344,6 @@ extern bool mips_eh_uses (unsigned int);
 extern bool mips_epilogue_uses (unsigned int);
 extern void mips_final_prescan_insn (rtx, rtx *, int);
 extern int mips_trampoline_code_size (void);
+extern void mips_function_profiler (FILE *);
 
 #endif /* ! GCC_MIPS_PROTOS_H */
Index: gcc/config/mips/mips.c
===================================================================
--- gcc/config/mips/mips.c	(revision 153502)
+++ gcc/config/mips/mips.c	(working copy)
@@ -319,6 +319,9 @@ struct GTY(())  mips_frame_info {
   HOST_WIDE_INT acc_sp_offset;
   HOST_WIDE_INT cop0_sp_offset;
 
+  /* Similar, but the value passed to _mcount.  */
+  HOST_WIDE_INT ra_fp_offset;
+
   /* The offset of arg_pointer_rtx from the bottom of the frame.  */
   HOST_WIDE_INT arg_pointer_offset;
 
@@ -9616,6 +9619,9 @@ mips_for_each_saved_gpr_and_fpr (HOST_WI
   for (regno = GP_REG_LAST; regno >= GP_REG_FIRST; regno--)
     if (BITSET_P (cfun->machine->frame.mask, regno - GP_REG_FIRST))
       {
+	/* Record the ra offset for use by mips_function_profiler.  */
+	if (regno == RETURN_ADDR_REGNUM)
+	  cfun->machine->frame.ra_fp_offset = offset + sp_offset;
 	mips_save_restore_reg (word_mode, regno, offset, fn);
 	offset -= UNITS_PER_WORD;
       }
@@ -16088,6 +16094,86 @@ mips_trampoline_init (rtx m_tramp, tree 
   emit_insn (gen_add3_insn (end_addr, addr, GEN_INT (TRAMPOLINE_SIZE)));
   emit_insn (gen_clear_cache (addr, end_addr));
 }
+
+/* Return true if the loading of $12 can be done in the jal/jalr delay
+   slot.  We can do this only if the jal will expand to a single
+   instruction and only a single instruction is required to load the
+   value to $12.  */
+
+static bool mips_raloc_in_delay_slot_p(void)
+{
+  return (SMALL_OPERAND_UNSIGNED(cfun->machine->frame.ra_fp_offset)
+	  && !TARGET_USE_GOT);
+}
+
+/* Implement FUNCTION_PROFILER.  */
+
+void mips_function_profiler (FILE *file)
+{
+  int emit_small_ra_offset;
+
+  emit_small_ra_offset = 0;
+
+  if (TARGET_MIPS16)
+    sorry ("mips16 function profiling");
+  if (TARGET_LONG_CALLS)
+    {
+      /*  For TARGET_LONG_CALLS use $3 for the address of _mcount.  */
+      if (Pmode == DImode)
+	fprintf (file, "\tdla\t%s,_mcount\n", reg_names[3]);
+      else
+	fprintf (file, "\tla\t%s,_mcount\n", reg_names[3]);
+    }
+  if (TARGET_RALOC)
+    {
+      /* If TARGET_RALOC load $12 with the offset of the ra save
+	 location.  */
+      if (mips_raloc_in_delay_slot_p())
+	emit_small_ra_offset = 1;
+      else
+	{
+	  if (Pmode == DImode)
+	    fprintf (file, "\tdli\t%s,%d\t\t# offset of ra\n", reg_names[12],
+		     cfun->machine->frame.ra_fp_offset);
+	  else
+	    fprintf (file, "\tli\t%s,%d\t\t# offset of ra\n", reg_names[12],
+		     cfun->machine->frame.ra_fp_offset);
+	}
+    }
+  mips_push_asm_switch (&mips_noat);
+  fprintf (file, "\tmove\t%s,%s\t\t# save current return address\n",
+	   reg_names[AT_REGNUM], reg_names[RETURN_ADDR_REGNUM]);
+  /* _mcount treats $2 as the static chain register.  */
+  if (cfun->static_chain_decl != NULL)
+    fprintf (file, "\tmove\t%s,%s\n", reg_names[2],
+	     reg_names[STATIC_CHAIN_REGNUM]);
+  if (!TARGET_NEWABI)
+    {
+      fprintf (file,
+	       "\t%s\t%s,%s,%d\t\t# _mcount pops 2 words from  stack\n",
+	       TARGET_64BIT ? "dsubu" : "subu",
+	       reg_names[STACK_POINTER_REGNUM],
+	       reg_names[STACK_POINTER_REGNUM],
+	       Pmode == DImode ? 16 : 8);
+    }
+  if (emit_small_ra_offset)
+    mips_push_asm_switch (&mips_noreorder);
+  if (TARGET_LONG_CALLS)
+    fprintf (file, "\tjalr\t%s\n", reg_names[3]);
+  else
+    fprintf (file, "\tjal\t_mcount\n");
+  if (emit_small_ra_offset)
+    {
+      fprintf (file, "\tori\t%s,%s,%d\t\t# offset of ra\n",
+	       reg_names[12], reg_names[0], cfun->machine->frame.ra_fp_offset);
+      mips_pop_asm_switch (&mips_noreorder);
+    }
+  mips_pop_asm_switch (&mips_noat);
+  /* _mcount treats $2 as the static chain register.  */
+  if (cfun->static_chain_decl != NULL)
+    fprintf (file, "\tmove\t%s,%s\n", reg_names[STATIC_CHAIN_REGNUM],
+	     reg_names[2]);
+}
 \f
 /* Initialize the GCC target structure.  */
 #undef TARGET_ASM_ALIGNED_HI_OP
Index: gcc/config/mips/mips.h
===================================================================
--- gcc/config/mips/mips.h	(revision 153502)
+++ gcc/config/mips/mips.h	(working copy)
@@ -2372,44 +2372,7 @@ typedef struct mips_args {
 /* Output assembler code to FILE to increment profiler label # LABELNO
    for profiling a function entry.  */
 
-#define FUNCTION_PROFILER(FILE, LABELNO)				\
-{									\
-  if (TARGET_MIPS16)							\
-    sorry ("mips16 function profiling");				\
-  if (TARGET_LONG_CALLS)						\
-    {									\
-      /*  For TARGET_LONG_CALLS use $3 for the address of _mcount.  */	\
-      if (Pmode == DImode)						\
-	fprintf (FILE, "\tdla\t%s,_mcount\n", reg_names[GP_REG_FIRST + 3]); \
-      else								\
-	fprintf (FILE, "\tla\t%s,_mcount\n", reg_names[GP_REG_FIRST + 3]); \
-    }									\
-  mips_push_asm_switch (&mips_noat);					\
-  fprintf (FILE, "\tmove\t%s,%s\t\t# save current return address\n",	\
-	   reg_names[AT_REGNUM], reg_names[RETURN_ADDR_REGNUM]);	\
-  /* _mcount treats $2 as the static chain register.  */		\
-  if (cfun->static_chain_decl != NULL)					\
-    fprintf (FILE, "\tmove\t%s,%s\n", reg_names[2],			\
-	     reg_names[STATIC_CHAIN_REGNUM]);				\
-  if (!TARGET_NEWABI)							\
-    {									\
-      fprintf (FILE,							\
-	       "\t%s\t%s,%s,%d\t\t# _mcount pops 2 words from  stack\n", \
-	       TARGET_64BIT ? "dsubu" : "subu",				\
-	       reg_names[STACK_POINTER_REGNUM],				\
-	       reg_names[STACK_POINTER_REGNUM],				\
-	       Pmode == DImode ? 16 : 8);				\
-    }									\
-  if (TARGET_LONG_CALLS)						\
-    fprintf (FILE, "\tjalr\t%s\n", reg_names[GP_REG_FIRST + 3]);	\
-  else									\
-    fprintf (FILE, "\tjal\t_mcount\n");					\
-  mips_pop_asm_switch (&mips_noat);					\
-  /* _mcount treats $2 as the static chain register.  */		\
-  if (cfun->static_chain_decl != NULL)					\
-    fprintf (FILE, "\tmove\t%s,%s\n", reg_names[STATIC_CHAIN_REGNUM],	\
-	     reg_names[2]);						\
-}
+#define FUNCTION_PROFILER(FILE, LABELNO) mips_function_profiler ((FILE))
 
 /* The profiler preserves all interesting registers, including $31.  */
 #define MIPS_SAVE_REG_FOR_PROFILING_P(REGNO) false

Re: [PATCH] MIPS: Add option to pass return address location to _mcount. Was: [PATCH -v4 4/9] tracing: add static function tracer support for MIPS

[Richard Sandiford]

Thanks for the patch.

David Daney <ddaney@caviumnetworks.com> writes:
> Richard Sandiford wrote:
>> David Daney <ddaney@caviumnetworks.com> writes:
>>> Wu Zhangjin wrote:
>>>> On Wed, 2009-10-21 at 11:24 -0400, Steven Rostedt wrote:
>>> [...]
>>>>>> +
>>>>>> +NESTED(_mcount, PT_SIZE, ra)
>>>>>> +	RESTORE_SP_FOR_32BIT
>>>>>> +	PTR_LA	t0, ftrace_stub
>>>>>> +	PTR_L	t1, ftrace_trace_function /* please don't use t1 later, safe? */
>>>>> Is t0 and t1 safe for mcount to use? Remember, mcount does not follow
>>>>> the dynamics of C function ABI.
>>>> So, perhaps we can use the saved registers(a0,a1...) instead.
>>>>
>>> a0..a7 may not always be saved.
>>>
>>> You can use at, v0, v1 and all the temporary registers.  Note that for 
>>> the 64-bit ABIs sometimes the names t0-t4 shadow a4-a7.  So for a 64-bit 
>>> kernel, you can use: $1, $2, $3, $12, $13, $14, $15, $24, $25, noting 
>>> that at == $1 and contains the callers ra.  For a 32-bit kernel you can 
>>> add $8, $9, $10, and $11
>>>
>>> This whole thing seems a little fragile.
>>>
>>> I think it might be a good idea to get input from Richard Sandiford, 
>>> and/or Adam Nemet about this approach (so I add them to the CC).
>>>
>>> This e-mail thread starts here:
>>>
>>> http://www.linux-mips.org/archives/linux-mips/2009-10/msg00286.html
>>>
>>> and here:
>>>
>>> http://www.linux-mips.org/archives/linux-mips/2009-10/msg00290.html
>> 
>> I'm not sure that the "search for a save of RA" thing is really a good idea.
>> The last version of that seemed to be "assume that any register stores
>> will be in a block that immediately precedes the move into RA", but even
>> if that's true now, it might not be in future.  And as Wu Zhangjin says,
>> it doesn't cope with long calls, where the target address is loaded
>> into a temporary register before the call.
>> 
>> FWIW, I'd certainly be happy to make GCC pass an additional parameter
>> to _mcount.  The parameter could give the address of the return slot,
>> or null for leaf functions.  In almost all cases[*], there would be
>> no overhead, since the move would go in the delay slot of the call.
>> 
>> [*] Meaning when the frame is <=32k. ;)  I'm guessing you never
>>     get anywhere near that, and if you did, the scan thing wouldn't
>>     work anyway.
>> 
>> The new behaviour could be controlled by a command-line option,
>> which would also give linux a cheap way of checking whether the
>> feature is available.
>> 
>
> How about this patch, I think it does what you suggest.
>
> When we pass -pg -mmcount-raloc, the address of the return address 
> relative to sp is passed in $12 to _mcount.  If the return address is 
> not saved, $12 will be zero.  I think this will work as registers are 
> never saved with an offset of zero.  $12 is a temporary register that is 
> not part of the ABI.

Hmm, well, the suggestion was to pass a pointer rather than an offset,
but both you and Wu Zhangjin seem to prefer the offset.  Is there a
reason for that?  I suggested a pointer because

  (a) they're more C-like
  (b) they're just as quick and easy to compute
  (c) MIPS doesn't have indexed addresses (well, except for a few
      special cases) so the callee is going to have to compute the
      pointer at some stage anyway

(It sounds from Wu Zhangjin's reply like he'd alread suggested the
offset thing before I sent my message.  If so, sorry for not using
that earlier message as context.)

> +  if (TARGET_RALOC)
> +    {
> +      /* If TARGET_RALOC load $12 with the offset of the ra save
> +	 location.  */
> +      if (mips_raloc_in_delay_slot_p())
> +	emit_small_ra_offset = 1;
> +      else
> +	{
> +	  if (Pmode == DImode)
> +	    fprintf (file, "\tdli\t%s,%d\t\t# offset of ra\n", reg_names[12],
> +		     cfun->machine->frame.ra_fp_offset);
> +	  else
> +	    fprintf (file, "\tli\t%s,%d\t\t# offset of ra\n", reg_names[12],
> +		     cfun->machine->frame.ra_fp_offset);
> +	}
> +    }

We shouldn't need to do the delay slot dance.  With either the pointer
((D)LA) or offset ((D)LI) approach, the macro won't use $at, so we can
insert the new code immediately before the jump, leaving the assembler
to fill the delay slot.  This is simpler and should mean that the delay
slot gets filled more often in the multi-insn-macro cases.

Looks good otherwise, but I'd be interested in other suggestions for
the option name.  I kept misreading "raloc" as a typo for "reloc".

Richard

Re: [PATCH] MIPS: Add option to pass return address location to _mcount. Was: [PATCH -v4 4/9] tracing: add static function tracer support for MIPS

[Wu Zhangjin]

Hi,

On Sat, 2009-10-24 at 10:12 +0100, Richard Sandiford wrote:
> Thanks for the patch.
[...]
> > How about this patch, I think it does what you suggest.
> >
> > When we pass -pg -mmcount-raloc, the address of the return address 
> > relative to sp is passed in $12 to _mcount.  If the return address is 
> > not saved, $12 will be zero.  I think this will work as registers are 
> > never saved with an offset of zero.  $12 is a temporary register that is 
> > not part of the ABI.
> 
> Hmm, well, the suggestion was to pass a pointer rather than an offset,
> but both you and Wu Zhangjin seem to prefer the offset.  Is there a
> reason for that?  I suggested a pointer because
> 
>   (a) they're more C-like
>   (b) they're just as quick and easy to compute
>   (c) MIPS doesn't have indexed addresses (well, except for a few
>       special cases) so the callee is going to have to compute the
>       pointer at some stage anyway
> 

Agree with you.

if not with -fno-omit-frame-pointer, we also need to calculate the frame
pointer, and then plus it with the offset. with pointer, we can get it
directly, but it may need a more instruction(lui..., addiu...) for
loading the pointer. of course, at last, the pointer will save more time
for us :-)

so, David, could you please use pointer instead? and then I will test it
asap(cloning the latest gcc currently). thanks!

> (It sounds from Wu Zhangjin's reply like he'd alread suggested the
> offset thing before I sent my message.  If so, sorry for not using
> that earlier message as context.)
> 

It doesn't matter, Seems at that time, you were not added in the CC
list, but added by David Daney later.

> > +  if (TARGET_RALOC)
> > +    {
> > +      /* If TARGET_RALOC load $12 with the offset of the ra save
> > +	 location.  */
> > +      if (mips_raloc_in_delay_slot_p())
> > +	emit_small_ra_offset = 1;
> > +      else
> > +	{
> > +	  if (Pmode == DImode)
> > +	    fprintf (file, "\tdli\t%s,%d\t\t# offset of ra\n", reg_names[12],
> > +		     cfun->machine->frame.ra_fp_offset);
> > +	  else
> > +	    fprintf (file, "\tli\t%s,%d\t\t# offset of ra\n", reg_names[12],
> > +		     cfun->machine->frame.ra_fp_offset);
> > +	}
> > +    }
> 
> We shouldn't need to do the delay slot dance.  With either the pointer
> ((D)LA) or offset ((D)LI) approach, the macro won't use $at, so we can
> insert the new code immediately before the jump, leaving the assembler
> to fill the delay slot.  This is simpler and should mean that the delay
> slot gets filled more often in the multi-insn-macro cases.
> 
> Looks good otherwise, but I'd be interested in other suggestions for
> the option name.  I kept misreading "raloc" as a typo for "reloc".
> 

The same misreading to me, what about -mmcount-ra-loc? add one "-", or
-mcount-ra-location?

BTW: Just made dynamic function tracer for MIPS support module tracing
with the help of -mlong-calls. after some more tests, I will send it as
-v5 revision later. hope the -v6 revision work with this new feature of
gcc from David Daney.

Regards,
	Wu Zhangjin

Re: [PATCH -v4 9/9] tracing: add function graph tracer support for MIPS

[Wu Zhangjin]

(Added linux-mips mailing list and the other people to the CC list.)

On Thu, 2009-10-22 at 04:37 -0700, pajko wrote:
[...]
> > 
> 
> All this stuff can be avoided having PROFILE_BEFORE_PROLOGUE enabled in GCC
> (gcc/config/mips/mips.h), like it is done one several other architectures.
> Some of them even require it to be set for a working _mcount. 
> Putting the call of _mcount before the function prologue should make no harm
> (it's working for me), and this way RA can be hooked for function graph
> tracing
> before it is saved to stack in the function prologue. Thus there will be no
> difference between leaf and non-leaf functions.

Good idea! Seems PROFILE_BEFORE_PROLOGUE is commented by default in
gcc/config/mips/mips.h of gcc 4.4:

/* #define PROFILE_BEFORE_PROLOGUE */

if we enable this macro, the handling will be the same to non-leaf and
leaf function, so, David's patch to gcc is not need again.

> This change is no big deal as GCC has to be patched anyway to get dynamic
> ftracing (and with 4.2.1 I had to patch it to get ftracing in modules
> working
> - using -mlong-calls -, but it's possible that this is fixed in the newer
> releases).
> 

do you mean if enabling PROFILE_BEFORE_PROLOGUE, there will be some
problems with module support using -mlong-calls?

I have made dynamic ftrace support module with -mlong-calls, I think, if
we move the codes before prologue, it should have no side effect.

        lui     v1,HI16bit     (&_mcount -> v1)
        daddiu  v1,v1,LOW16bit
        move    at,ra
        jalr    v1

Regards,
	Wu Zhangjin

Re: [PATCH -v4 9/9] tracing: add function graph tracer support for MIPS

[Patrik Kluba]

On Sun, Oct 25, 2009 at 11:48 AM, Wu Zhangjin <wuzhangjin@gmail.com> wrote:
>
> do you mean if enabling PROFILE_BEFORE_PROLOGUE, there will be some
> problems with module support using -mlong-calls?
>

No, there are no problems. I've tested it on friday, and function
graph tracing was working correctly.
I meant to say that 4.2.1 we use does not generate correct profile
calls from kernel modules. Maybe this issue was fixed in newer
releases, I did not check. I've applied a patch (don't remember where
have I found that, maybe it was created by you) to our toolchain
several months ago.

I was thinking about dynamic tracing, and I think a toolchain patch
can be avoided completely. We only need to make difference between
"jal _mcount" and "jalr v1"-style mcount calls when replacing them
with "nop" instructions in the code-patching function called by
ftrace_convert_nops(). This can be done in 2 ways:
1) keeping old instructions - takes extra memory, not an option
2) using 2 separate instructions to replace with. One of them could be
the normal NOP instruction, which expands to "sll r0, r0, 0". For the
other we could use "sll r0, r0, 1" but as it has already special
meaning (SSNOP) a better candidate could be something like "sll r1,
r1, 0". This way we can decide which instruction to patch in when
tracing is enabled for a function, eg. when the code patcher
encounters a "sll r0, r0, 0" it emits a function call using JAL and
when it encounters "sll r1, r1, 0" it emits a function call using
"JALR v1".

Regards,
Patrik

Re: [PATCH -v4 9/9] tracing: add function graph tracer support for MIPS

[Wu Zhangjin]

On Sun, 2009-10-25 at 14:37 +0100, Patrik Kluba wrote:
> On Sun, Oct 25, 2009 at 11:48 AM, Wu Zhangjin <wuzhangjin@gmail.com> wrote:
> >
> > do you mean if enabling PROFILE_BEFORE_PROLOGUE, there will be some
> > problems with module support using -mlong-calls?
> >
> 
> No, there are no problems. I've tested it on friday, and function
> graph tracing was working correctly.
> I meant to say that 4.2.1 we use does not generate correct profile
> calls from kernel modules. Maybe this issue was fixed in newer
> releases, I did not check. I've applied a patch (don't remember where
> have I found that, maybe it was created by you) to our toolchain
> several months ago.

I have never sent a patch to gcc before :-) but perhaps somebody have
fixed it for us. so, the left job is hoping somebody enable
PROFILE_BEFORE_PROLOGUE for MIPS in the next version of gcc if there is
no side effect, and then we can hijack the return address of non-leaf &
leaf function directly in the same way in _mcount.

> 
> I was thinking about dynamic tracing, and I think a toolchain patch
> can be avoided completely. We only need to make difference between
> "jal _mcount" and "jalr v1"-style mcount calls when replacing them
> with "nop" instructions in the code-patching function called by
> ftrace_convert_nops(). This can be done in 2 ways:
> 1) keeping old instructions - takes extra memory, not an option
> 2) using 2 separate instructions to replace with. One of them could be
> the normal NOP instruction, which expands to "sll r0, r0, 0". For the
> other we could use "sll r0, r0, 1" but as it has already special
> meaning (SSNOP) a better candidate could be something like "sll r1,
> r1, 0". This way we can decide which instruction to patch in when
> tracing is enabled for a function, eg. when the code patcher
> encounters a "sll r0, r0, 0" it emits a function call using JAL and
> when it encounters "sll r1, r1, 0" it emits a function call using
> "JALR v1".

If only thinking about dynamic tracing, no patch for gcc needed,
-mlong-calls is enough, I have done it via a "stupid" trick, will send
the patchset out asap :-)

Regards,
	Wu Zhangjin

Re: [PATCH -v4 9/9] tracing: add function graph tracer support for MIPS

[Richard Sandiford]

Wu Zhangjin <wuzhangjin@gmail.com> writes:
> (Added linux-mips mailing list and the other people to the CC list.)
>
> On Thu, 2009-10-22 at 04:37 -0700, pajko wrote:
> [...]
>> > 
>> 
>> All this stuff can be avoided having PROFILE_BEFORE_PROLOGUE enabled in GCC
>> (gcc/config/mips/mips.h), like it is done one several other architectures.
>> Some of them even require it to be set for a working _mcount. 
>> Putting the call of _mcount before the function prologue should make no harm
>> (it's working for me), and this way RA can be hooked for function graph
>> tracing
>> before it is saved to stack in the function prologue. Thus there will be no
>> difference between leaf and non-leaf functions.
>
> Good idea! Seems PROFILE_BEFORE_PROLOGUE is commented by default in
> gcc/config/mips/mips.h of gcc 4.4:
>
> /* #define PROFILE_BEFORE_PROLOGUE */
>
> if we enable this macro, the handling will be the same to non-leaf and
> leaf function, so, David's patch to gcc is not need again.

Defining PROFILE_BEFORE_PROLOGUE isn't correct for abicalls code,
because "jal _mcount" is a macro that loads _mcount from the
GOT into $25.  We don't have access to $28 at the beginning of
the function, and we mustn't clobber the incoming value of $25.
So we could only make this change for non-abicalls code.

It's then a choice between (a) having new non-abicalls-specific
behaviour or (b) going with David's patch.  The advantage of
(a) is that the linux code is slightly simpler.  The disadvantage
is that it makes the _mcount interface differ between -mabicalls
and -mno-abicalls.  And IMO the disadvantage outweights the advantage.
If this new behaviour is useful for linux, it could easily be useful
for userspace too.  And with the new PLT support, non-shared abicalls
code is supposed to be link-compatible with non-abicalls code.

I think David's patch is the way to go.

Richard

Re: [PATCH] MIPS: Add option to pass return address location to _mcount.

[David Daney]

[-- Attachment #1: Type: text/plain, Size: 2533 bytes --]

Richard Sandiford wrote:
> Thanks for the patch.
> 
> David Daney <ddaney@caviumnetworks.com> writes:
>> Richard Sandiford wrote:
[...]
>>>
>>> FWIW, I'd certainly be happy to make GCC pass an additional parameter
>>> to _mcount.  The parameter could give the address of the return slot,
>>> or null for leaf functions.  In almost all cases[*], there would be
>>> no overhead, since the move would go in the delay slot of the call.
>>>
>>> [*] Meaning when the frame is <=32k. ;)  I'm guessing you never
>>>     get anywhere near that, and if you did, the scan thing wouldn't
>>>     work anyway.
>>>
>>> The new behaviour could be controlled by a command-line option,
>>> which would also give linux a cheap way of checking whether the
>>> feature is available.
>>>
>> How about this patch, I think it does what you suggest.
>>
[...]
> 
> Hmm, well, the suggestion was to pass a pointer rather than an offset,
> but both you and Wu Zhangjin seem to prefer the offset.  Is there a
> reason for that?  I suggested a pointer because
> 
>   (a) they're more C-like
>   (b) they're just as quick and easy to compute
>   (c) MIPS doesn't have indexed addresses (well, except for a few
>       special cases) so the callee is going to have to compute the
>       pointer at some stage anyway

Passing a pointer is better.  The original patch was based on a 
misunderstanding of your original suggestion, this new patch passes the 
pointer.

[...]
> 
> Looks good otherwise, but I'd be interested in other suggestions for
> the option name.  I kept misreading "raloc" as a typo for "reloc".

Well how about raaddr?  (Return-Address Address)

Still not tested, but how about this version instead?

gcc/
2009-10-26  David Daney  <ddaney@caviumnetworks.com>

	* doc/invoke.texi (mmcount-raaddr): Document new command line
	option.
	* config/mips/mips.opt (mmcount-raaddr): New option.
	* config/mips/mips-protos.h (mips_function_profiler): Declare new
	function.
	* config/mips/mips.c (struct mips_frame_info): Add ra_fp_offset
	member.
	(mips_for_each_saved_gpr_and_fpr): Set ra_fp_offset.
	(mips_function_profiler): Moved from FUNCTION_PROFILER, and
	rewritten.
	* config/mips/mips.h (FUNCTION_PROFILER): Body of macro moved to
	mips_function_profiler.

gcc/testsuite/
2009-10-26  David Daney  <ddaney@caviumnetworks.com>

	* gcc.target/mips/mips.exp (mips_option_groups): Add
	mcount-raaddr.
	* gcc.target/mips/mmcount-raaddr-1.c: New test.
	* gcc.target/mips/mmcount-raaddr-2.c: New test.
	* gcc.target/mips/mmcount-raaddr-3.c: New test.


[-- Attachment #2: mcount.patch --]
[-- Type: text/plain, Size: 10163 bytes --]

Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 153502)
+++ gcc/doc/invoke.texi	(working copy)
@@ -709,7 +709,7 @@ Objective-C and Objective-C++ Dialects}.
 -mbranch-cost=@var{num}  -mbranch-likely  -mno-branch-likely @gol
 -mfp-exceptions -mno-fp-exceptions @gol
 -mvr4130-align -mno-vr4130-align -msynci -mno-synci @gol
--mrelax-pic-calls -mno-relax-pic-calls}
+-mrelax-pic-calls -mno-relax-pic-calls -mmcount-raaddr}
 
 @emph{MMIX Options}
 @gccoptlist{-mlibfuncs  -mno-libfuncs  -mepsilon  -mno-epsilon  -mabi=gnu @gol
@@ -14192,6 +14192,19 @@ an assembler and a linker that supports 
 directive and @code{-mexplicit-relocs} is in effect.  With
 @code{-mno-explicit-relocs}, this optimization can be performed by the
 assembler and the linker alone without help from the compiler.
+
+@item -mmcount-raaddr
+@itemx -mno-mcount-raaddr
+@opindex mmcount-raaddr
+@opindex mno-mcount-raaddr
+Emit (do not emit) code to pass the address of the return address save
+location to @code{_mcount}.  The default is @option{-mno-mcount-raaddr}.
+
+@option{-mmcount-raaddr} can be used in conjunction with @option{-pg}
+and a specially coded @code{_mcount} function to record function exit
+by replacing the saved return address with a pointer to the function
+exit profiling function.
+
 @end table
 
 @node MMIX Options
Index: gcc/testsuite/gcc.target/mips/mips.exp
===================================================================
--- gcc/testsuite/gcc.target/mips/mips.exp	(revision 153502)
+++ gcc/testsuite/gcc.target/mips/mips.exp	(working copy)
@@ -263,6 +263,7 @@ foreach option {
     sym32
     synci
     relax-pic-calls
+    mcount-raaddr
 } {
     lappend mips_option_groups $option "-m(no-|)$option"
 }
Index: gcc/testsuite/gcc.target/mips/mmcount-raaddr-1.c
===================================================================
--- gcc/testsuite/gcc.target/mips/mmcount-raaddr-1.c	(revision 0)
+++ gcc/testsuite/gcc.target/mips/mmcount-raaddr-1.c	(revision 0)
@@ -0,0 +1,7 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -pg -mmcount-raaddr -march=mips64 -mabi=64 -mno-abicalls -msym32" } */
+/* { dg-final { scan-assembler "\tmove\t\\\$12,\\\$0" } } */
+int bazl(int i)
+{
+  return i + 2;
+}
Index: gcc/testsuite/gcc.target/mips/mmcount-raaddr-2.c
===================================================================
--- gcc/testsuite/gcc.target/mips/mmcount-raaddr-2.c	(revision 0)
+++ gcc/testsuite/gcc.target/mips/mmcount-raaddr-2.c	(revision 0)
@@ -0,0 +1,8 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -pg -mmcount-raaddr -march=mips64 -mabi=64 -mno-abicalls -msym32" } */
+/* { dg-final { scan-assembler "\tdaddiu\t\\\$12,\\\$sp,8" } } */
+int foo (int);
+int bar (int i)
+{
+  return foo (i) + 2;
+}
Index: gcc/testsuite/gcc.target/mips/mmcount-raaddr-3.c
===================================================================
--- gcc/testsuite/gcc.target/mips/mmcount-raaddr-3.c	(revision 0)
+++ gcc/testsuite/gcc.target/mips/mmcount-raaddr-3.c	(revision 0)
@@ -0,0 +1,10 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -pg -mmcount-raaddr -march=mips64 -mabi=64 -mno-abicalls -msym32" } */
+/* { dg-final { scan-assembler "\tdli\t\\\$12,200008" } } */
+/* { dg-final { scan-assembler "\tdaddu\t\\\$12,\\\$12,\\\$sp" } } */
+int foo (int *);
+int bar(int i)
+{
+  int big[50000];
+  return foo (big) + 2;
+}
Index: gcc/config/mips/mips.opt
===================================================================
--- gcc/config/mips/mips.opt	(revision 153502)
+++ gcc/config/mips/mips.opt	(working copy)
@@ -208,6 +208,10 @@ mlong64
 Target Report RejectNegative Mask(LONG64)
 Use a 64-bit long type
 
+mmcount-raaddr
+Target Report Var(TARGET_MCOUNT_RAADDR)
+Pass the address of the ra save location to _mcount in $12
+
 mmemcpy
 Target Report Mask(MEMCPY)
 Don't optimize block moves
Index: gcc/config/mips/mips-protos.h
===================================================================
--- gcc/config/mips/mips-protos.h	(revision 153502)
+++ gcc/config/mips/mips-protos.h	(working copy)
@@ -344,5 +344,6 @@ extern bool mips_eh_uses (unsigned int);
 extern bool mips_epilogue_uses (unsigned int);
 extern void mips_final_prescan_insn (rtx, rtx *, int);
 extern int mips_trampoline_code_size (void);
+extern void mips_function_profiler (FILE *);
 
 #endif /* ! GCC_MIPS_PROTOS_H */
Index: gcc/config/mips/mips.c
===================================================================
--- gcc/config/mips/mips.c	(revision 153502)
+++ gcc/config/mips/mips.c	(working copy)
@@ -319,6 +319,9 @@ struct GTY(())  mips_frame_info {
   HOST_WIDE_INT acc_sp_offset;
   HOST_WIDE_INT cop0_sp_offset;
 
+  /* Similar, but the value passed to _mcount.  */
+  HOST_WIDE_INT ra_fp_offset;
+
   /* The offset of arg_pointer_rtx from the bottom of the frame.  */
   HOST_WIDE_INT arg_pointer_offset;
 
@@ -9616,6 +9619,9 @@ mips_for_each_saved_gpr_and_fpr (HOST_WI
   for (regno = GP_REG_LAST; regno >= GP_REG_FIRST; regno--)
     if (BITSET_P (cfun->machine->frame.mask, regno - GP_REG_FIRST))
       {
+	/* Record the ra offset for use by mips_function_profiler.  */
+	if (regno == RETURN_ADDR_REGNUM)
+	  cfun->machine->frame.ra_fp_offset = offset + sp_offset;
 	mips_save_restore_reg (word_mode, regno, offset, fn);
 	offset -= UNITS_PER_WORD;
       }
@@ -16088,6 +16094,74 @@ mips_trampoline_init (rtx m_tramp, tree 
   emit_insn (gen_add3_insn (end_addr, addr, GEN_INT (TRAMPOLINE_SIZE)));
   emit_insn (gen_clear_cache (addr, end_addr));
 }
+
+/* Implement FUNCTION_PROFILER.  */
+
+void mips_function_profiler (FILE *file)
+{
+  if (TARGET_MIPS16)
+    sorry ("mips16 function profiling");
+  if (TARGET_LONG_CALLS)
+    {
+      /*  For TARGET_LONG_CALLS use $3 for the address of _mcount.  */
+      if (Pmode == DImode)
+	fprintf (file, "\tdla\t%s,_mcount\n", reg_names[3]);
+      else
+	fprintf (file, "\tla\t%s,_mcount\n", reg_names[3]);
+    }
+  mips_push_asm_switch (&mips_noat);
+  fprintf (file, "\tmove\t%s,%s\t\t# save current return address\n",
+	   reg_names[AT_REGNUM], reg_names[RETURN_ADDR_REGNUM]);
+  /* _mcount treats $2 as the static chain register.  */
+  if (cfun->static_chain_decl != NULL)
+    fprintf (file, "\tmove\t%s,%s\n", reg_names[2],
+	     reg_names[STATIC_CHAIN_REGNUM]);
+  if (TARGET_MCOUNT_RAADDR)
+    {
+      /* If TARGET_MCOUNT_RAADDR load $12 with the address of the ra save
+	 location.  */
+      if (cfun->machine->frame.ra_fp_offset == 0)
+	/* ra not saved, pass zero.  */
+	fprintf (file, "\tmove\t%s,%s\t\t# address of ra\n",
+		 reg_names[12], reg_names[0]);
+      else if (SMALL_OPERAND (cfun->machine->frame.ra_fp_offset))
+	fprintf (file,
+		 "\t%s\t%s,%s," HOST_WIDE_INT_PRINT_DEC "\t\t# address of ra\n",
+		 Pmode == DImode ? "daddiu" : "addiu",
+		 reg_names[12], reg_names[STACK_POINTER_REGNUM],
+		 cfun->machine->frame.ra_fp_offset);
+      else
+	{
+	  fprintf (file,
+		   "\t%s\t%s," HOST_WIDE_INT_PRINT_DEC "\n",
+		   Pmode == DImode ? "dli" : "li",
+		   reg_names[12],
+		   cfun->machine->frame.ra_fp_offset);
+	  fprintf (file, "\t%s\t%s,%s,%s\t\t# address of ra\n",
+		   Pmode == DImode ? "daddu" : "addu",
+		   reg_names[12], reg_names[12],
+		   reg_names[STACK_POINTER_REGNUM]);
+	}
+    }
+  if (!TARGET_NEWABI)
+    {
+      fprintf (file,
+	       "\t%s\t%s,%s,%d\t\t# _mcount pops 2 words from  stack\n",
+	       TARGET_64BIT ? "dsubu" : "subu",
+	       reg_names[STACK_POINTER_REGNUM],
+	       reg_names[STACK_POINTER_REGNUM],
+	       Pmode == DImode ? 16 : 8);
+    }
+  if (TARGET_LONG_CALLS)
+    fprintf (file, "\tjalr\t%s\n", reg_names[3]);
+  else
+    fprintf (file, "\tjal\t_mcount\n");
+  mips_pop_asm_switch (&mips_noat);
+  /* _mcount treats $2 as the static chain register.  */
+  if (cfun->static_chain_decl != NULL)
+    fprintf (file, "\tmove\t%s,%s\n", reg_names[STATIC_CHAIN_REGNUM],
+	     reg_names[2]);
+}
 \f
 /* Initialize the GCC target structure.  */
 #undef TARGET_ASM_ALIGNED_HI_OP
Index: gcc/config/mips/mips.h
===================================================================
--- gcc/config/mips/mips.h	(revision 153502)
+++ gcc/config/mips/mips.h	(working copy)
@@ -2372,44 +2372,7 @@ typedef struct mips_args {
 /* Output assembler code to FILE to increment profiler label # LABELNO
    for profiling a function entry.  */
 
-#define FUNCTION_PROFILER(FILE, LABELNO)				\
-{									\
-  if (TARGET_MIPS16)							\
-    sorry ("mips16 function profiling");				\
-  if (TARGET_LONG_CALLS)						\
-    {									\
-      /*  For TARGET_LONG_CALLS use $3 for the address of _mcount.  */	\
-      if (Pmode == DImode)						\
-	fprintf (FILE, "\tdla\t%s,_mcount\n", reg_names[GP_REG_FIRST + 3]); \
-      else								\
-	fprintf (FILE, "\tla\t%s,_mcount\n", reg_names[GP_REG_FIRST + 3]); \
-    }									\
-  mips_push_asm_switch (&mips_noat);					\
-  fprintf (FILE, "\tmove\t%s,%s\t\t# save current return address\n",	\
-	   reg_names[AT_REGNUM], reg_names[RETURN_ADDR_REGNUM]);	\
-  /* _mcount treats $2 as the static chain register.  */		\
-  if (cfun->static_chain_decl != NULL)					\
-    fprintf (FILE, "\tmove\t%s,%s\n", reg_names[2],			\
-	     reg_names[STATIC_CHAIN_REGNUM]);				\
-  if (!TARGET_NEWABI)							\
-    {									\
-      fprintf (FILE,							\
-	       "\t%s\t%s,%s,%d\t\t# _mcount pops 2 words from  stack\n", \
-	       TARGET_64BIT ? "dsubu" : "subu",				\
-	       reg_names[STACK_POINTER_REGNUM],				\
-	       reg_names[STACK_POINTER_REGNUM],				\
-	       Pmode == DImode ? 16 : 8);				\
-    }									\
-  if (TARGET_LONG_CALLS)						\
-    fprintf (FILE, "\tjalr\t%s\n", reg_names[GP_REG_FIRST + 3]);	\
-  else									\
-    fprintf (FILE, "\tjal\t_mcount\n");					\
-  mips_pop_asm_switch (&mips_noat);					\
-  /* _mcount treats $2 as the static chain register.  */		\
-  if (cfun->static_chain_decl != NULL)					\
-    fprintf (FILE, "\tmove\t%s,%s\n", reg_names[STATIC_CHAIN_REGNUM],	\
-	     reg_names[2]);						\
-}
+#define FUNCTION_PROFILER(FILE, LABELNO) mips_function_profiler ((FILE))
 
 /* The profiler preserves all interesting registers, including $31.  */
 #define MIPS_SAVE_REG_FOR_PROFILING_P(REGNO) false

Re: [PATCH] MIPS: Add option to pass return address location to _mcount.

[Wu Zhangjin]

Hi,

[...]
> > 
> > Looks good otherwise, but I'd be interested in other suggestions for
> > the option name.  I kept misreading "raloc" as a typo for "reloc".
> 
> Well how about raaddr?  (Return-Address Address)
> 
> Still not tested, but how about this version instead?
> 

I will help to test it later(but maybe tomorrow to later), I think this
will can not pass the compiling, have tried it with the 4.4-200903...,
some of the macros are not defined, such as RETURN_ADDR_REGNUM(not sure
in the latest version of gcc).

Regards,
	Wu Zhangjin
 
> gcc/
> 2009-10-26  David Daney  <ddaney@caviumnetworks.com>
> 
> 	* doc/invoke.texi (mmcount-raaddr): Document new command line
> 	option.
> 	* config/mips/mips.opt (mmcount-raaddr): New option.
> 	* config/mips/mips-protos.h (mips_function_profiler): Declare new
> 	function.
> 	* config/mips/mips.c (struct mips_frame_info): Add ra_fp_offset
> 	member.
> 	(mips_for_each_saved_gpr_and_fpr): Set ra_fp_offset.
> 	(mips_function_profiler): Moved from FUNCTION_PROFILER, and
> 	rewritten.
> 	* config/mips/mips.h (FUNCTION_PROFILER): Body of macro moved to
> 	mips_function_profiler.
> 
> gcc/testsuite/
> 2009-10-26  David Daney  <ddaney@caviumnetworks.com>
> 
> 	* gcc.target/mips/mips.exp (mips_option_groups): Add
> 	mcount-raaddr.
> 	* gcc.target/mips/mmcount-raaddr-1.c: New test.
> 	* gcc.target/mips/mmcount-raaddr-2.c: New test.
> 	* gcc.target/mips/mmcount-raaddr-3.c: New test.
> 
> plain text document attachment (mcount.patch)
> Index: gcc/doc/invoke.texi
> ===================================================================
> --- gcc/doc/invoke.texi	(revision 153502)
> +++ gcc/doc/invoke.texi	(working copy)
> @@ -709,7 +709,7 @@ Objective-C and Objective-C++ Dialects}.
>  -mbranch-cost=@var{num}  -mbranch-likely  -mno-branch-likely @gol
>  -mfp-exceptions -mno-fp-exceptions @gol
>  -mvr4130-align -mno-vr4130-align -msynci -mno-synci @gol
> --mrelax-pic-calls -mno-relax-pic-calls}
> +-mrelax-pic-calls -mno-relax-pic-calls -mmcount-raaddr}
>  
>  @emph{MMIX Options}
>  @gccoptlist{-mlibfuncs  -mno-libfuncs  -mepsilon  -mno-epsilon  -mabi=gnu @gol
> @@ -14192,6 +14192,19 @@ an assembler and a linker that supports 
>  directive and @code{-mexplicit-relocs} is in effect.  With
>  @code{-mno-explicit-relocs}, this optimization can be performed by the
>  assembler and the linker alone without help from the compiler.
> +
> +@item -mmcount-raaddr
> +@itemx -mno-mcount-raaddr
> +@opindex mmcount-raaddr
> +@opindex mno-mcount-raaddr
> +Emit (do not emit) code to pass the address of the return address save
> +location to @code{_mcount}.  The default is @option{-mno-mcount-raaddr}.
> +
> +@option{-mmcount-raaddr} can be used in conjunction with @option{-pg}
> +and a specially coded @code{_mcount} function to record function exit
> +by replacing the saved return address with a pointer to the function
> +exit profiling function.
> +
>  @end table
>  
>  @node MMIX Options
> Index: gcc/testsuite/gcc.target/mips/mips.exp
> ===================================================================
> --- gcc/testsuite/gcc.target/mips/mips.exp	(revision 153502)
> +++ gcc/testsuite/gcc.target/mips/mips.exp	(working copy)
> @@ -263,6 +263,7 @@ foreach option {
>      sym32
>      synci
>      relax-pic-calls
> +    mcount-raaddr
>  } {
>      lappend mips_option_groups $option "-m(no-|)$option"
>  }
> Index: gcc/testsuite/gcc.target/mips/mmcount-raaddr-1.c
> ===================================================================
> --- gcc/testsuite/gcc.target/mips/mmcount-raaddr-1.c	(revision 0)
> +++ gcc/testsuite/gcc.target/mips/mmcount-raaddr-1.c	(revision 0)
> @@ -0,0 +1,7 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -pg -mmcount-raaddr -march=mips64 -mabi=64 -mno-abicalls -msym32" } */
> +/* { dg-final { scan-assembler "\tmove\t\\\$12,\\\$0" } } */
> +int bazl(int i)
> +{
> +  return i + 2;
> +}
> Index: gcc/testsuite/gcc.target/mips/mmcount-raaddr-2.c
> ===================================================================
> --- gcc/testsuite/gcc.target/mips/mmcount-raaddr-2.c	(revision 0)
> +++ gcc/testsuite/gcc.target/mips/mmcount-raaddr-2.c	(revision 0)
> @@ -0,0 +1,8 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -pg -mmcount-raaddr -march=mips64 -mabi=64 -mno-abicalls -msym32" } */
> +/* { dg-final { scan-assembler "\tdaddiu\t\\\$12,\\\$sp,8" } } */
> +int foo (int);
> +int bar (int i)
> +{
> +  return foo (i) + 2;
> +}
> Index: gcc/testsuite/gcc.target/mips/mmcount-raaddr-3.c
> ===================================================================
> --- gcc/testsuite/gcc.target/mips/mmcount-raaddr-3.c	(revision 0)
> +++ gcc/testsuite/gcc.target/mips/mmcount-raaddr-3.c	(revision 0)
> @@ -0,0 +1,10 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -pg -mmcount-raaddr -march=mips64 -mabi=64 -mno-abicalls -msym32" } */
> +/* { dg-final { scan-assembler "\tdli\t\\\$12,200008" } } */
> +/* { dg-final { scan-assembler "\tdaddu\t\\\$12,\\\$12,\\\$sp" } } */
> +int foo (int *);
> +int bar(int i)
> +{
> +  int big[50000];
> +  return foo (big) + 2;
> +}
> Index: gcc/config/mips/mips.opt
> ===================================================================
> --- gcc/config/mips/mips.opt	(revision 153502)
> +++ gcc/config/mips/mips.opt	(working copy)
> @@ -208,6 +208,10 @@ mlong64
>  Target Report RejectNegative Mask(LONG64)
>  Use a 64-bit long type
>  
> +mmcount-raaddr
> +Target Report Var(TARGET_MCOUNT_RAADDR)
> +Pass the address of the ra save location to _mcount in $12
> +
>  mmemcpy
>  Target Report Mask(MEMCPY)
>  Don't optimize block moves
> Index: gcc/config/mips/mips-protos.h
> ===================================================================
> --- gcc/config/mips/mips-protos.h	(revision 153502)
> +++ gcc/config/mips/mips-protos.h	(working copy)
> @@ -344,5 +344,6 @@ extern bool mips_eh_uses (unsigned int);
>  extern bool mips_epilogue_uses (unsigned int);
>  extern void mips_final_prescan_insn (rtx, rtx *, int);
>  extern int mips_trampoline_code_size (void);
> +extern void mips_function_profiler (FILE *);
>  
>  #endif /* ! GCC_MIPS_PROTOS_H */
> Index: gcc/config/mips/mips.c
> ===================================================================
> --- gcc/config/mips/mips.c	(revision 153502)
> +++ gcc/config/mips/mips.c	(working copy)
> @@ -319,6 +319,9 @@ struct GTY(())  mips_frame_info {
>    HOST_WIDE_INT acc_sp_offset;
>    HOST_WIDE_INT cop0_sp_offset;
>  
> +  /* Similar, but the value passed to _mcount.  */
> +  HOST_WIDE_INT ra_fp_offset;
> +
>    /* The offset of arg_pointer_rtx from the bottom of the frame.  */
>    HOST_WIDE_INT arg_pointer_offset;
>  
> @@ -9616,6 +9619,9 @@ mips_for_each_saved_gpr_and_fpr (HOST_WI
>    for (regno = GP_REG_LAST; regno >= GP_REG_FIRST; regno--)
>      if (BITSET_P (cfun->machine->frame.mask, regno - GP_REG_FIRST))
>        {
> +	/* Record the ra offset for use by mips_function_profiler.  */
> +	if (regno == RETURN_ADDR_REGNUM)
> +	  cfun->machine->frame.ra_fp_offset = offset + sp_offset;
>  	mips_save_restore_reg (word_mode, regno, offset, fn);
>  	offset -= UNITS_PER_WORD;
>        }
> @@ -16088,6 +16094,74 @@ mips_trampoline_init (rtx m_tramp, tree 
>    emit_insn (gen_add3_insn (end_addr, addr, GEN_INT (TRAMPOLINE_SIZE)));
>    emit_insn (gen_clear_cache (addr, end_addr));
>  }
> +
> +/* Implement FUNCTION_PROFILER.  */
> +
> +void mips_function_profiler (FILE *file)
> +{
> +  if (TARGET_MIPS16)
> +    sorry ("mips16 function profiling");
> +  if (TARGET_LONG_CALLS)
> +    {
> +      /*  For TARGET_LONG_CALLS use $3 for the address of _mcount.  */
> +      if (Pmode == DImode)
> +	fprintf (file, "\tdla\t%s,_mcount\n", reg_names[3]);
> +      else
> +	fprintf (file, "\tla\t%s,_mcount\n", reg_names[3]);
> +    }
> +  mips_push_asm_switch (&mips_noat);
> +  fprintf (file, "\tmove\t%s,%s\t\t# save current return address\n",
> +	   reg_names[AT_REGNUM], reg_names[RETURN_ADDR_REGNUM]);
> +  /* _mcount treats $2 as the static chain register.  */
> +  if (cfun->static_chain_decl != NULL)
> +    fprintf (file, "\tmove\t%s,%s\n", reg_names[2],
> +	     reg_names[STATIC_CHAIN_REGNUM]);
> +  if (TARGET_MCOUNT_RAADDR)
> +    {
> +      /* If TARGET_MCOUNT_RAADDR load $12 with the address of the ra save
> +	 location.  */
> +      if (cfun->machine->frame.ra_fp_offset == 0)
> +	/* ra not saved, pass zero.  */
> +	fprintf (file, "\tmove\t%s,%s\t\t# address of ra\n",
> +		 reg_names[12], reg_names[0]);
> +      else if (SMALL_OPERAND (cfun->machine->frame.ra_fp_offset))
> +	fprintf (file,
> +		 "\t%s\t%s,%s," HOST_WIDE_INT_PRINT_DEC "\t\t# address of ra\n",
> +		 Pmode == DImode ? "daddiu" : "addiu",
> +		 reg_names[12], reg_names[STACK_POINTER_REGNUM],
> +		 cfun->machine->frame.ra_fp_offset);
> +      else
> +	{
> +	  fprintf (file,
> +		   "\t%s\t%s," HOST_WIDE_INT_PRINT_DEC "\n",
> +		   Pmode == DImode ? "dli" : "li",
> +		   reg_names[12],
> +		   cfun->machine->frame.ra_fp_offset);
> +	  fprintf (file, "\t%s\t%s,%s,%s\t\t# address of ra\n",
> +		   Pmode == DImode ? "daddu" : "addu",
> +		   reg_names[12], reg_names[12],
> +		   reg_names[STACK_POINTER_REGNUM]);
> +	}
> +    }
> +  if (!TARGET_NEWABI)
> +    {
> +      fprintf (file,
> +	       "\t%s\t%s,%s,%d\t\t# _mcount pops 2 words from  stack\n",
> +	       TARGET_64BIT ? "dsubu" : "subu",
> +	       reg_names[STACK_POINTER_REGNUM],
> +	       reg_names[STACK_POINTER_REGNUM],
> +	       Pmode == DImode ? 16 : 8);
> +    }
> +  if (TARGET_LONG_CALLS)
> +    fprintf (file, "\tjalr\t%s\n", reg_names[3]);
> +  else
> +    fprintf (file, "\tjal\t_mcount\n");
> +  mips_pop_asm_switch (&mips_noat);
> +  /* _mcount treats $2 as the static chain register.  */
> +  if (cfun->static_chain_decl != NULL)
> +    fprintf (file, "\tmove\t%s,%s\n", reg_names[STATIC_CHAIN_REGNUM],
> +	     reg_names[2]);
> +}
>  \f
>  /* Initialize the GCC target structure.  */
>  #undef TARGET_ASM_ALIGNED_HI_OP
> Index: gcc/config/mips/mips.h
> ===================================================================
> --- gcc/config/mips/mips.h	(revision 153502)
> +++ gcc/config/mips/mips.h	(working copy)
> @@ -2372,44 +2372,7 @@ typedef struct mips_args {
>  /* Output assembler code to FILE to increment profiler label # LABELNO
>     for profiling a function entry.  */
>  
> -#define FUNCTION_PROFILER(FILE, LABELNO)				\
> -{									\
> -  if (TARGET_MIPS16)							\
> -    sorry ("mips16 function profiling");				\
> -  if (TARGET_LONG_CALLS)						\
> -    {									\
> -      /*  For TARGET_LONG_CALLS use $3 for the address of _mcount.  */	\
> -      if (Pmode == DImode)						\
> -	fprintf (FILE, "\tdla\t%s,_mcount\n", reg_names[GP_REG_FIRST + 3]); \
> -      else								\
> -	fprintf (FILE, "\tla\t%s,_mcount\n", reg_names[GP_REG_FIRST + 3]); \
> -    }									\
> -  mips_push_asm_switch (&mips_noat);					\
> -  fprintf (FILE, "\tmove\t%s,%s\t\t# save current return address\n",	\
> -	   reg_names[AT_REGNUM], reg_names[RETURN_ADDR_REGNUM]);	\
> -  /* _mcount treats $2 as the static chain register.  */		\
> -  if (cfun->static_chain_decl != NULL)					\
> -    fprintf (FILE, "\tmove\t%s,%s\n", reg_names[2],			\
> -	     reg_names[STATIC_CHAIN_REGNUM]);				\
> -  if (!TARGET_NEWABI)							\
> -    {									\
> -      fprintf (FILE,							\
> -	       "\t%s\t%s,%s,%d\t\t# _mcount pops 2 words from  stack\n", \
> -	       TARGET_64BIT ? "dsubu" : "subu",				\
> -	       reg_names[STACK_POINTER_REGNUM],				\
> -	       reg_names[STACK_POINTER_REGNUM],				\
> -	       Pmode == DImode ? 16 : 8);				\
> -    }									\
> -  if (TARGET_LONG_CALLS)						\
> -    fprintf (FILE, "\tjalr\t%s\n", reg_names[GP_REG_FIRST + 3]);	\
> -  else									\
> -    fprintf (FILE, "\tjal\t_mcount\n");					\
> -  mips_pop_asm_switch (&mips_noat);					\
> -  /* _mcount treats $2 as the static chain register.  */		\
> -  if (cfun->static_chain_decl != NULL)					\
> -    fprintf (FILE, "\tmove\t%s,%s\n", reg_names[STATIC_CHAIN_REGNUM],	\
> -	     reg_names[2]);						\
> -}
> +#define FUNCTION_PROFILER(FILE, LABELNO) mips_function_profiler ((FILE))
>  
>  /* The profiler preserves all interesting registers, including $31.  */
>  #define MIPS_SAVE_REG_FOR_PROFILING_P(REGNO) false

Re: [PATCH] MIPS: Add option to pass return address location to _mcount.

[Richard Sandiford]

David Daney <ddaney@caviumnetworks.com> writes:
>> Looks good otherwise, but I'd be interested in other suggestions for
>> the option name.  I kept misreading "raloc" as a typo for "reloc".
>
> Well how about raaddr?  (Return-Address Address)

Taking Wu Zhangjin's suggestion of an extra dash, how about
-mmcount-ra-address?

> +@option{-mmcount-raaddr} can be used in conjunction with @option{-pg}
> +and a specially coded @code{_mcount} function to record function exit

Doc conventions require "specially-coded", I think.  We should
mention the $12 register too, and the treatment of leaf functions.
How about:

--------------------------
Allow (do not allow) @code{_mcount} to modify the calling function's
return address.  When enabled, this option extends the usual @code{_mcount}
interface with a new @var{ra-address} parameter, which has type
@code{intptr_t *} and is passed in register @code{$12}.  @code{_mcount}
can then modify the return address by doing both of the following:
@itemize
@item
Returning the new address in register @code{$31}.
@item
Storing the new address in @code{*@var{ra-address}},
if @var{ra-address} is nonnull.
@end @itemize

The default is @option{-mno-mcount-ra-address}.
--------------------------

> +/* { dg-do compile } */
> +/* { dg-options "-O2 -pg -mmcount-raaddr -march=mips64 -mabi=64 -mno-abicalls -msym32" } */

This is a general feature, so I'd rather test with as few special options
as possible.  Just "-O2 -pg" if we can.  Same with the other tests.

Sorry to ask, but while you're here, would you mind fixing a couple
of pre-existing formatting problems too?  Namely:

> +      /*  For TARGET_LONG_CALLS use $3 for the address of _mcount.  */

Only one space for "For" and

> +  if (!TARGET_NEWABI)
> +    {
> +      fprintf (file,
> +	       "\t%s\t%s,%s,%d\t\t# _mcount pops 2 words from  stack\n",
> +	       TARGET_64BIT ? "dsubu" : "subu",
> +	       reg_names[STACK_POINTER_REGNUM],
> +	       reg_names[STACK_POINTER_REGNUM],
> +	       Pmode == DImode ? 16 : 8);
> +    }

No braces here.

As far as the new bits are concerned:

> +      /* If TARGET_MCOUNT_RAADDR load $12 with the address of the ra save
> +	 location.  */
> +      if (cfun->machine->frame.ra_fp_offset == 0)
> +	/* ra not saved, pass zero.  */
> +	fprintf (file, "\tmove\t%s,%s\t\t# address of ra\n",
> +		 reg_names[12], reg_names[0]);

Let's drop the "# address of ra" comment.  We don't add comments to
the other bits.

Everything in the following block:

> +      else if (SMALL_OPERAND (cfun->machine->frame.ra_fp_offset))
> +	fprintf (file,
> +		 "\t%s\t%s,%s," HOST_WIDE_INT_PRINT_DEC "\t\t# address of ra\n",
> +		 Pmode == DImode ? "daddiu" : "addiu",
> +		 reg_names[12], reg_names[STACK_POINTER_REGNUM],
> +		 cfun->machine->frame.ra_fp_offset);
> +      else
> +	{
> +	  fprintf (file,
> +		   "\t%s\t%s," HOST_WIDE_INT_PRINT_DEC "\n",
> +		   Pmode == DImode ? "dli" : "li",
> +		   reg_names[12],
> +		   cfun->machine->frame.ra_fp_offset);
> +	  fprintf (file, "\t%s\t%s,%s,%s\t\t# address of ra\n",
> +		   Pmode == DImode ? "daddu" : "addu",
> +		   reg_names[12], reg_names[12],
> +		   reg_names[STACK_POINTER_REGNUM]);
> +	}

reduces to:

      else
	fprintf (file, "\t%s\t%s," HOST_WIDE_INT_PRINT_DEC "(%s)",
		 Pmode == DImode ? "dla" : "la", reg_names[12],
		 cfun->machine->frame.ra_fp_offset,
		 reg_names[STACK_POINTER_REGNUM]);

OK with those changes, thanks, if it passing testing, and if
Ralf is happy with the linux side.

Richard

Re: [PATCH] MIPS: Add option to pass return address location to _mcount.

[David Daney]

Richard Sandiford wrote:
> David Daney <ddaney@caviumnetworks.com> writes:
[...]
>> +/* { dg-do compile } */
>> +/* { dg-options "-O2 -pg -mmcount-raaddr -march=mips64 -mabi=64 -mno-abicalls -msym32" } */
> 
> This is a general feature, so I'd rather test with as few special options
> as possible.  Just "-O2 -pg" if we can.  Same with the other tests.
> 

The tests check for the exact offset.  This varies with ABI, so I would 
like to keep the options that force the ABI.  I could probably get rid 
of -msys32 and -mno-abicalls.

David Daney

Re: [PATCH] MIPS: Add option to pass return address location to _mcount.

[Richard Sandiford]

David Daney <ddaney@caviumnetworks.com> writes:
> Richard Sandiford wrote:
>> David Daney <ddaney@caviumnetworks.com> writes:
> [...]
>>> +/* { dg-do compile } */
>>> +/* { dg-options "-O2 -pg -mmcount-raaddr -march=mips64 -mabi=64 -mno-abicalls -msym32" } */
>> 
>> This is a general feature, so I'd rather test with as few special options
>> as possible.  Just "-O2 -pg" if we can.  Same with the other tests.
>> 
>
> The tests check for the exact offset.  This varies with ABI, so I would 
> like to keep the options that force the ABI.  I could probably get rid 
> of -msys32 and -mno-abicalls.

Ah, yeah, good point.  Let's go for "-O2 -pg -mcount-blah -mabi=64" then.
(The testsuite will force an appropriate architecture.)

Thanks,
Richard

Re: [PATCH] MIPS: Add option to pass return address location to _mcount.

[Wu Zhangjin]

Hi,

On Tue, 2009-10-27 at 21:20 +0000, Richard Sandiford wrote:
[...]
> OK with those changes,

Just applied the above changes, and added one more for not getting the
ra_fp_offset when -pg is not enabled:

@@ -9619,6 +9622,9 @@ mips_for_each_saved_gpr_and_fpr (HOST_WIDE_INT
sp_offset,
   for (regno = GP_REG_LAST; regno >= GP_REG_FIRST; regno--)
     if (BITSET_P (cfun->machine->frame.mask, regno - GP_REG_FIRST))
       {
+       /* Record the ra offset for use by mips_function_profiler.  */
+       if (crtl->profile && regno == RETURN_ADDR_REGNUM)
+         cfun->machine->frame.ra_fp_offset = offset + sp_offset;
        mips_save_restore_reg (word_mode, regno, offset, fn);
        offset -= UNITS_PER_WORD;
       }

"crtl->profile &&" was added.

>  thanks, if it passing testing, and if
> Ralf is happy with the linux side.
> 

Just appended that patch with the above changes to the latest gcc 4.5 in
the git repository, and tested it, which works well.

this is the result of a non-leaf function:

ffffffff80243c08 <copy_process>:
ffffffff80243c08:       67bdff40        daddiu  sp,sp,-192
ffffffff80243c0c:       ffbe00b0        sd      s8,176(sp)
ffffffff80243c10:       03a0f02d        move    s8,sp
ffffffff80243c14:       ffbf00b8        sd      ra,184(sp)
[...]
ffffffff80243c38:       3c038021        lui     v1,0x8021
ffffffff80243c3c:       64631530        daddiu  v1,v1,5424
ffffffff80243c40:       03e0082d        move    at,ra
ffffffff80243c44:       0060f809        jalr    v1
ffffffff80243c48:       67ac00b8        daddiu  t0,sp,184 --> Here it is

and for a leaf function:

ffffffff802054d0 <au1k_wait>:
ffffffff802054d0:       67bdfff0        daddiu  sp,sp,-16
ffffffff802054d4:       ffbe0008        sd      s8,8(sp)
ffffffff802054d8:       03a0f02d        move    s8,sp
ffffffff802054dc:       3c038021        lui     v1,0x8021
ffffffff802054e0:       64631530        daddiu  v1,v1,5424
ffffffff802054e4:       03e0082d        move    at,ra
ffffffff802054e8:       0060f809        jalr    v1
ffffffff802054ec:       0000602d        move    t0,zero --> Here it is

and with this new feature, I can safely remove the old
ftrace_get_parent_addr() and add several more instructions to make
function graph tracer work. and also, 'Cause this feature used the
t0($12) register, I must change the temp registers I have used in the
patches of ftrace for MIPS(t0 --> t1, t1 --> t2, t2-> t3...). no more
touch.

The latest revision of this patch will be sent out in the next E-mail,
and also, the -v8 revision of ftrace for MIPS will be sent out with this
new feature of Gcc asap.

Thanks & Regards,
	Wu Zhangjin

Re: [PATCH] MIPS: Add option to pass return address location to _mcount.

[David Daney]

Wu Zhangjin wrote:
> Hi,
> 
> On Tue, 2009-10-27 at 21:20 +0000, Richard Sandiford wrote:
> [...]
>> OK with those changes,
> 
> Just applied the above changes, and added one more for not getting the
> ra_fp_offset when -pg is not enabled:
> 
> @@ -9619,6 +9622,9 @@ mips_for_each_saved_gpr_and_fpr (HOST_WIDE_INT
> sp_offset,
>    for (regno = GP_REG_LAST; regno >= GP_REG_FIRST; regno--)
>      if (BITSET_P (cfun->machine->frame.mask, regno - GP_REG_FIRST))
>        {
> +       /* Record the ra offset for use by mips_function_profiler.  */
> +       if (crtl->profile && regno == RETURN_ADDR_REGNUM)
> +         cfun->machine->frame.ra_fp_offset = offset + sp_offset;
>         mips_save_restore_reg (word_mode, regno, offset, fn);
>         offset -= UNITS_PER_WORD;
>        }
> 
> "crtl->profile &&" was added.
> 

I am a little confused.

I don't think your change is needed.  The FUNCTION_PROFILER code is not 
invoked unless -pg is passed.

Were you getting wrong code without your change?  The ra_fp_offset field 
will be unused if -pg is not specified, but its existence shouldn't 
affect code generation.

David Daney

Re: [PATCH] MIPS: Add option to pass return address location to _mcount.

[David Daney]

[-- Attachment #1: Type: text/plain, Size: 4857 bytes --]

Richard Sandiford wrote:
> David Daney <ddaney@caviumnetworks.com> writes:
>>> Looks good otherwise, but I'd be interested in other suggestions for
>>> the option name.  I kept misreading "raloc" as a typo for "reloc".
>> Well how about raaddr?  (Return-Address Address)
> 
> Taking Wu Zhangjin's suggestion of an extra dash, how about
> -mmcount-ra-address?
> 

Right.


>> +@option{-mmcount-raaddr} can be used in conjunction with @option{-pg}
>> +and a specially coded @code{_mcount} function to record function exit
> 
> Doc conventions require "specially-coded", I think.  We should
> mention the $12 register too, and the treatment of leaf functions.
> How about:
> 
> --------------------------
> Allow (do not allow) @code{_mcount} to modify the calling function's
> return address.  When enabled, this option extends the usual @code{_mcount}
> interface with a new @var{ra-address} parameter, which has type
> @code{intptr_t *} and is passed in register @code{$12}.  @code{_mcount}
> can then modify the return address by doing both of the following:
> @itemize
> @item
> Returning the new address in register @code{$31}.
> @item
> Storing the new address in @code{*@var{ra-address}},
> if @var{ra-address} is nonnull.
> @end @itemize
> 
> The default is @option{-mno-mcount-ra-address}.
> --------------------------

Ok.  I took a couple of liberties, but used essentially what you suggest.

> 
>> +/* { dg-do compile } */
>> +/* { dg-options "-O2 -pg -mmcount-raaddr -march=mips64 -mabi=64 -mno-abicalls -msym32" } */
> 
> This is a general feature, so I'd rather test with as few special options
> as possible.  Just "-O2 -pg" if we can.  Same with the other tests.
> 

As discussed in the other branch of the thread, I now use "-O2 -pg 
-mmcount-raaddr -mabi=64"


> Sorry to ask, but while you're here, would you mind fixing a couple
> of pre-existing formatting problems too?  Namely:
> 
>> +      /*  For TARGET_LONG_CALLS use $3 for the address of _mcount.  */
> 
> Only one space for "For" and
> 
>> +  if (!TARGET_NEWABI)
>> +    {
>> +      fprintf (file,
>> +	       "\t%s\t%s,%s,%d\t\t# _mcount pops 2 words from  stack\n",
>> +	       TARGET_64BIT ? "dsubu" : "subu",
>> +	       reg_names[STACK_POINTER_REGNUM],
>> +	       reg_names[STACK_POINTER_REGNUM],
>> +	       Pmode == DImode ? 16 : 8);
>> +    }
> 
> No braces here.

OK.

> 
> As far as the new bits are concerned:
> 
>> +      /* If TARGET_MCOUNT_RAADDR load $12 with the address of the ra save
>> +	 location.  */
>> +      if (cfun->machine->frame.ra_fp_offset == 0)
>> +	/* ra not saved, pass zero.  */
>> +	fprintf (file, "\tmove\t%s,%s\t\t# address of ra\n",
>> +		 reg_names[12], reg_names[0]);
> 
> Let's drop the "# address of ra" comment.  We don't add comments to
> the other bits.
> 

OK.

> Everything in the following block:
> 
>> +      else if (SMALL_OPERAND (cfun->machine->frame.ra_fp_offset))
>> +	fprintf (file,
>> +		 "\t%s\t%s,%s," HOST_WIDE_INT_PRINT_DEC "\t\t# address of ra\n",
>> +		 Pmode == DImode ? "daddiu" : "addiu",
>> +		 reg_names[12], reg_names[STACK_POINTER_REGNUM],
>> +		 cfun->machine->frame.ra_fp_offset);
>> +      else
>> +	{
>> +	  fprintf (file,
>> +		   "\t%s\t%s," HOST_WIDE_INT_PRINT_DEC "\n",
>> +		   Pmode == DImode ? "dli" : "li",
>> +		   reg_names[12],
>> +		   cfun->machine->frame.ra_fp_offset);
>> +	  fprintf (file, "\t%s\t%s,%s,%s\t\t# address of ra\n",
>> +		   Pmode == DImode ? "daddu" : "addu",
>> +		   reg_names[12], reg_names[12],
>> +		   reg_names[STACK_POINTER_REGNUM]);
>> +	}
> 
> reduces to:
> 
>       else
> 	fprintf (file, "\t%s\t%s," HOST_WIDE_INT_PRINT_DEC "(%s)",
> 		 Pmode == DImode ? "dla" : "la", reg_names[12],
> 		 cfun->machine->frame.ra_fp_offset,
> 		 reg_names[STACK_POINTER_REGNUM]);
> 

Right.  I had not realized the the assembler was so 'smart'.


> OK with those changes, thanks, if it passing testing, and if
> Ralf is happy with the linux side.
> 

No regressions, and discussed with Ralf.2009-10-29  David Daney 
<ddaney@caviumnetworks.com>

	* doc/invoke.texi (mmcount-ra-address): Document new command line
	option.
	* config/mips/mips.opt (mmcount-ra-address): New option.
	* config/mips/mips-protos.h (mips_function_profiler): Declare new
	function.
	* config/mips/mips.c (struct mips_frame_info): Add ra_fp_offset
	member.
	(mips_for_each_saved_gpr_and_fpr): Set ra_fp_offset.
	(mips_function_profiler): Moved from FUNCTION_PROFILER, and
	rewritten.
	* config/mips/mips.h (FUNCTION_PROFILER): Body of macro moved to
	mips_function_profiler.

2009-10-29  David Daney  <ddaney@caviumnetworks.com>

	* gcc.target/mips/mips.exp (mips_option_groups): Add
	mcount-ra-address.
	* gcc.target/mips/mmcount-ra-address-1.c: New test.
	* gcc.target/mips/mmcount-ra-address-2.c: New test.
	* gcc.target/mips/mmcount-ra-address-3.c: New test.


FWIW, this is the version I committed:



[-- Attachment #2: mcount.patch --]
[-- Type: text/plain, Size: 9800 bytes --]

Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 153716)
+++ gcc/doc/invoke.texi	(working copy)
@@ -709,7 +709,7 @@ Objective-C and Objective-C++ Dialects}.
 -mbranch-cost=@var{num}  -mbranch-likely  -mno-branch-likely @gol
 -mfp-exceptions -mno-fp-exceptions @gol
 -mvr4130-align -mno-vr4130-align -msynci -mno-synci @gol
--mrelax-pic-calls -mno-relax-pic-calls}
+-mrelax-pic-calls -mno-relax-pic-calls -mmcount-ra-address}
 
 @emph{MMIX Options}
 @gccoptlist{-mlibfuncs  -mno-libfuncs  -mepsilon  -mno-epsilon  -mabi=gnu @gol
@@ -14208,6 +14208,27 @@ an assembler and a linker that supports 
 directive and @code{-mexplicit-relocs} is in effect.  With
 @code{-mno-explicit-relocs}, this optimization can be performed by the
 assembler and the linker alone without help from the compiler.
+
+@item -mmcount-ra-address
+@itemx -mno-mcount-ra-address
+@opindex mmcount-ra-address
+@opindex mno-mcount-ra-address
+Emit (do not emit) code that allows @code{_mcount} to modify the
+colling function's return address.  When enabled, this option extends
+the usual @code{_mcount} interface with a new @var{ra-address}
+parameter, which has type @code{intptr_t *} and is passed in register
+@code{$12}.  @code{_mcount} can then modify the return address by
+doing both of the following:
+@itemize
+@item
+Returning the new address in register @code{$31}.
+@item
+Storing the new address in @code{*@var{ra-address}},
+if @var{ra-address} is nonnull.
+@end itemize
+
+The default is @option{-mno-mcount-ra-address}.
+
 @end table
 
 @node MMIX Options
Index: gcc/testsuite/gcc.target/mips/mips.exp
===================================================================
--- gcc/testsuite/gcc.target/mips/mips.exp	(revision 153716)
+++ gcc/testsuite/gcc.target/mips/mips.exp	(working copy)
@@ -263,6 +263,7 @@ foreach option {
     sym32
     synci
     relax-pic-calls
+    mcount-ra-address
 } {
     lappend mips_option_groups $option "-m(no-|)$option"
 }
Index: gcc/testsuite/gcc.target/mips/mmcount-ra-address-1.c
===================================================================
--- gcc/testsuite/gcc.target/mips/mmcount-ra-address-1.c	(revision 0)
+++ gcc/testsuite/gcc.target/mips/mmcount-ra-address-1.c	(revision 0)
@@ -0,0 +1,7 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -pg -mmcount-ra-address -mabi=64" } */
+/* { dg-final { scan-assembler "\tmove\t\\\$12,\\\$0" } } */
+int bazl(int i)
+{
+  return i + 2;
+}
Index: gcc/testsuite/gcc.target/mips/mmcount-ra-address-2.c
===================================================================
--- gcc/testsuite/gcc.target/mips/mmcount-ra-address-2.c	(revision 0)
+++ gcc/testsuite/gcc.target/mips/mmcount-ra-address-2.c	(revision 0)
@@ -0,0 +1,8 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -pg -mmcount-ra-address -mabi=64" } */
+/* { dg-final { scan-assembler "\tdla\t\\\$12,8\\(\\\$sp\\)" } } */
+int foo (int);
+int bar (int i)
+{
+  return foo (i) + 2;
+}
Index: gcc/testsuite/gcc.target/mips/mmcount-ra-address-3.c
===================================================================
--- gcc/testsuite/gcc.target/mips/mmcount-ra-address-3.c	(revision 0)
+++ gcc/testsuite/gcc.target/mips/mmcount-ra-address-3.c	(revision 0)
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -pg -mmcount-ra-address -mabi=64" } */
+/* { dg-final { scan-assembler "\tdla\t\\\$12,200008\\(\\\$sp\\)" } } */
+int foo (int *);
+int bar(int i)
+{
+  int big[50000];
+  return foo (big) + 2;
+}
Index: gcc/config/mips/mips.opt
===================================================================
--- gcc/config/mips/mips.opt	(revision 153716)
+++ gcc/config/mips/mips.opt	(working copy)
@@ -208,6 +208,10 @@ mlong64
 Target Report RejectNegative Mask(LONG64)
 Use a 64-bit long type
 
+mmcount-ra-address
+Target Report Var(TARGET_MCOUNT_RA_ADDRESS)
+Pass the address of the ra save location to _mcount in $12
+
 mmemcpy
 Target Report Mask(MEMCPY)
 Don't optimize block moves
Index: gcc/config/mips/mips-protos.h
===================================================================
--- gcc/config/mips/mips-protos.h	(revision 153716)
+++ gcc/config/mips/mips-protos.h	(working copy)
@@ -344,5 +344,6 @@ extern bool mips_eh_uses (unsigned int);
 extern bool mips_epilogue_uses (unsigned int);
 extern void mips_final_prescan_insn (rtx, rtx *, int);
 extern int mips_trampoline_code_size (void);
+extern void mips_function_profiler (FILE *);
 
 #endif /* ! GCC_MIPS_PROTOS_H */
Index: gcc/config/mips/mips.c
===================================================================
--- gcc/config/mips/mips.c	(revision 153716)
+++ gcc/config/mips/mips.c	(working copy)
@@ -319,6 +319,9 @@ struct GTY(())  mips_frame_info {
   HOST_WIDE_INT acc_sp_offset;
   HOST_WIDE_INT cop0_sp_offset;
 
+  /* Similar, but the value passed to _mcount.  */
+  HOST_WIDE_INT ra_fp_offset;
+
   /* The offset of arg_pointer_rtx from the bottom of the frame.  */
   HOST_WIDE_INT arg_pointer_offset;
 
@@ -9666,6 +9669,9 @@ mips_for_each_saved_gpr_and_fpr (HOST_WI
   for (regno = GP_REG_LAST; regno >= GP_REG_FIRST; regno--)
     if (BITSET_P (cfun->machine->frame.mask, regno - GP_REG_FIRST))
       {
+	/* Record the ra offset for use by mips_function_profiler.  */
+	if (regno == RETURN_ADDR_REGNUM)
+	  cfun->machine->frame.ra_fp_offset = offset + sp_offset;
 	mips_save_restore_reg (word_mode, regno, offset, fn);
 	offset -= UNITS_PER_WORD;
       }
@@ -16138,6 +16144,59 @@ mips_trampoline_init (rtx m_tramp, tree 
   emit_insn (gen_add3_insn (end_addr, addr, GEN_INT (TRAMPOLINE_SIZE)));
   emit_insn (gen_clear_cache (addr, end_addr));
 }
+
+/* Implement FUNCTION_PROFILER.  */
+
+void mips_function_profiler (FILE *file)
+{
+  if (TARGET_MIPS16)
+    sorry ("mips16 function profiling");
+  if (TARGET_LONG_CALLS)
+    {
+      /* For TARGET_LONG_CALLS use $3 for the address of _mcount.  */
+      if (Pmode == DImode)
+	fprintf (file, "\tdla\t%s,_mcount\n", reg_names[3]);
+      else
+	fprintf (file, "\tla\t%s,_mcount\n", reg_names[3]);
+    }
+  mips_push_asm_switch (&mips_noat);
+  fprintf (file, "\tmove\t%s,%s\t\t# save current return address\n",
+	   reg_names[AT_REGNUM], reg_names[RETURN_ADDR_REGNUM]);
+  /* _mcount treats $2 as the static chain register.  */
+  if (cfun->static_chain_decl != NULL)
+    fprintf (file, "\tmove\t%s,%s\n", reg_names[2],
+	     reg_names[STATIC_CHAIN_REGNUM]);
+  if (TARGET_MCOUNT_RA_ADDRESS)
+    {
+      /* If TARGET_MCOUNT_RA_ADDRESS load $12 with the address of the
+	 ra save location.  */
+      if (cfun->machine->frame.ra_fp_offset == 0)
+	/* ra not saved, pass zero.  */
+	fprintf (file, "\tmove\t%s,%s\n", reg_names[12], reg_names[0]);
+      else
+	fprintf (file, "\t%s\t%s," HOST_WIDE_INT_PRINT_DEC "(%s)\n",
+		 Pmode == DImode ? "dla" : "la", reg_names[12],
+		 cfun->machine->frame.ra_fp_offset,
+		 reg_names[STACK_POINTER_REGNUM]);
+    }
+  if (!TARGET_NEWABI)
+    fprintf (file,
+	     "\t%s\t%s,%s,%d\t\t# _mcount pops 2 words from  stack\n",
+	     TARGET_64BIT ? "dsubu" : "subu",
+	     reg_names[STACK_POINTER_REGNUM],
+	     reg_names[STACK_POINTER_REGNUM],
+	     Pmode == DImode ? 16 : 8);
+
+  if (TARGET_LONG_CALLS)
+    fprintf (file, "\tjalr\t%s\n", reg_names[3]);
+  else
+    fprintf (file, "\tjal\t_mcount\n");
+  mips_pop_asm_switch (&mips_noat);
+  /* _mcount treats $2 as the static chain register.  */
+  if (cfun->static_chain_decl != NULL)
+    fprintf (file, "\tmove\t%s,%s\n", reg_names[STATIC_CHAIN_REGNUM],
+	     reg_names[2]);
+}
 \f
 /* Initialize the GCC target structure.  */
 #undef TARGET_ASM_ALIGNED_HI_OP
Index: gcc/config/mips/mips.h
===================================================================
--- gcc/config/mips/mips.h	(revision 153716)
+++ gcc/config/mips/mips.h	(working copy)
@@ -2372,44 +2372,7 @@ typedef struct mips_args {
 /* Output assembler code to FILE to increment profiler label # LABELNO
    for profiling a function entry.  */
 
-#define FUNCTION_PROFILER(FILE, LABELNO)				\
-{									\
-  if (TARGET_MIPS16)							\
-    sorry ("mips16 function profiling");				\
-  if (TARGET_LONG_CALLS)						\
-    {									\
-      /*  For TARGET_LONG_CALLS use $3 for the address of _mcount.  */	\
-      if (Pmode == DImode)						\
-	fprintf (FILE, "\tdla\t%s,_mcount\n", reg_names[GP_REG_FIRST + 3]); \
-      else								\
-	fprintf (FILE, "\tla\t%s,_mcount\n", reg_names[GP_REG_FIRST + 3]); \
-    }									\
-  mips_push_asm_switch (&mips_noat);					\
-  fprintf (FILE, "\tmove\t%s,%s\t\t# save current return address\n",	\
-	   reg_names[AT_REGNUM], reg_names[RETURN_ADDR_REGNUM]);	\
-  /* _mcount treats $2 as the static chain register.  */		\
-  if (cfun->static_chain_decl != NULL)					\
-    fprintf (FILE, "\tmove\t%s,%s\n", reg_names[2],			\
-	     reg_names[STATIC_CHAIN_REGNUM]);				\
-  if (!TARGET_NEWABI)							\
-    {									\
-      fprintf (FILE,							\
-	       "\t%s\t%s,%s,%d\t\t# _mcount pops 2 words from  stack\n", \
-	       TARGET_64BIT ? "dsubu" : "subu",				\
-	       reg_names[STACK_POINTER_REGNUM],				\
-	       reg_names[STACK_POINTER_REGNUM],				\
-	       Pmode == DImode ? 16 : 8);				\
-    }									\
-  if (TARGET_LONG_CALLS)						\
-    fprintf (FILE, "\tjalr\t%s\n", reg_names[GP_REG_FIRST + 3]);	\
-  else									\
-    fprintf (FILE, "\tjal\t_mcount\n");					\
-  mips_pop_asm_switch (&mips_noat);					\
-  /* _mcount treats $2 as the static chain register.  */		\
-  if (cfun->static_chain_decl != NULL)					\
-    fprintf (FILE, "\tmove\t%s,%s\n", reg_names[STATIC_CHAIN_REGNUM],	\
-	     reg_names[2]);						\
-}
+#define FUNCTION_PROFILER(FILE, LABELNO) mips_function_profiler ((FILE))
 
 /* The profiler preserves all interesting registers, including $31.  */
 #define MIPS_SAVE_REG_FOR_PROFILING_P(REGNO) false

Re: [PATCH] MIPS: Add option to pass return address location to _mcount.

[Richard Sandiford]

David Daney <ddaney@caviumnetworks.com> writes:
> Richard Sandiford wrote:
>> David Daney <ddaney@caviumnetworks.com> writes:
>>> +@option{-mmcount-raaddr} can be used in conjunction with @option{-pg}
>>> +and a specially coded @code{_mcount} function to record function exit
>> 
>> Doc conventions require "specially-coded", I think.  We should
>> mention the $12 register too, and the treatment of leaf functions.
>> How about:
>> 
>> --------------------------
>> Allow (do not allow) @code{_mcount} to modify the calling function's
>> return address.  When enabled, this option extends the usual @code{_mcount}
>> interface with a new @var{ra-address} parameter, which has type
>> @code{intptr_t *} and is passed in register @code{$12}.  @code{_mcount}
>> can then modify the return address by doing both of the following:
>> @itemize
>> @item
>> Returning the new address in register @code{$31}.
>> @item
>> Storing the new address in @code{*@var{ra-address}},
>> if @var{ra-address} is nonnull.
>> @end @itemize
>> 
>> The default is @option{-mno-mcount-ra-address}.
>> --------------------------
>
> Ok.  I took a couple of liberties, but used essentially what you suggest.

Yeah, the reworked version looks good, thanks, but I noticed a typo:

> +Emit (do not emit) code that allows @code{_mcount} to modify the
> +colling function's return address.  When enabled, this option extends
   ^^^^^^^

Fixed as follows and applied.

Richard


gcc/
	* doc/invoke.texi: Fix typo.

Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	2009-11-03 21:25:03.000000000 +0000
+++ gcc/doc/invoke.texi	2009-11-03 21:25:03.000000000 +0000
@@ -14216,7 +14216,7 @@ assembler and the linker alone without h
 @opindex mmcount-ra-address
 @opindex mno-mcount-ra-address
 Emit (do not emit) code that allows @code{_mcount} to modify the
-colling function's return address.  When enabled, this option extends
+calling function's return address.  When enabled, this option extends
 the usual @code{_mcount} interface with a new @var{ra-address}
 parameter, which has type @code{intptr_t *} and is passed in register
 @code{$12}.  @code{_mcount} can then modify the return address by