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

[Richard Sandiford <rdsandiford@googlemail.com>]

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