Patchwork [mips] Follow up to pr54061 patch, fix another abort.

login
register
mail settings
Submitter Steve Ellcey
Date Dec. 11, 2012, 9 p.m.
Message ID <2fec14de-684b-4578-b8b6-cc95f6f1ebbe@EXCHHUB01.MIPS.com>
Download mbox | patch
Permalink /patch/205300/
State New
Headers show

Comments

Steve Ellcey - Dec. 11, 2012, 9 p.m.
While building libgcc in mips16 mode I found another instance of
dbx_reg_number aborting that the patch to pr54061 did not fix.
This code:

extern void __chk_fail (void) __attribute__ ((__noreturn__));
__strncpy_chk (s1, s2, n, s1len)
{
  char c;
  char *s = s1;
  if (__builtin_expect (s1len < n, 0))
    __chk_fail ();
  while (c != '\0');
  return s;
}

aborts when compiled with -O2 -g -fpic -mips16.  The following patch
fixes it.  OK to checkin?

Steve Ellcey
sellcey@mips.com


2012-12-11  Steve Ellcey  <sellcey@mips.com>

	* config/mips/mips.c (mips_option_override): Set
	mips_dbx_regno[CPRESTORE_SLOT_REGNUM] to IGNORED_DWARF_REGNUM.
Richard Sandiford - Dec. 11, 2012, 9:52 p.m.
"Steve Ellcey " <sellcey@mips.com> writes:
> While building libgcc in mips16 mode I found another instance of
> dbx_reg_number aborting that the patch to pr54061 did not fix.
> This code:
>
> extern void __chk_fail (void) __attribute__ ((__noreturn__));
> __strncpy_chk (s1, s2, n, s1len)
> {
>   char c;
>   char *s = s1;
>   if (__builtin_expect (s1len < n, 0))
>     __chk_fail ();
>   while (c != '\0');
>   return s;
> }
>
> aborts when compiled with -O2 -g -fpic -mips16.  The following patch
> fixes it.  OK to checkin?
>
> Steve Ellcey
> sellcey@mips.com
>
>
> 2012-12-11  Steve Ellcey  <sellcey@mips.com>
>
> 	* config/mips/mips.c (mips_option_override): Set
> 	mips_dbx_regno[CPRESTORE_SLOT_REGNUM] to IGNORED_DWARF_REGNUM.
>
>
> diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
> index 820b228..8113e5d 100644
> --- a/gcc/config/mips/mips.c
> +++ b/gcc/config/mips/mips.c
> @@ -16760,6 +16760,8 @@ mips_option_override (void)
>    for (i = ALL_COP_REG_FIRST; i <= ALL_COP_REG_LAST; i++)
>      mips_dbx_regno[i] = IGNORED_DWARF_REGNUM;
>  
> +  mips_dbx_regno[CPRESTORE_SLOT_REGNUM] = IGNORED_DWARF_REGNUM;
> +
>    /* Accumulator debug registers use big-endian ordering.  */
>    mips_dbx_regno[HI_REGNUM] = MD_DBX_FIRST + 0;
>    mips_dbx_regno[LO_REGNUM] = MD_DBX_FIRST + 1;

If even fake registers like these are going to be used, then I think
we should initialise to IGNORED_DWARF_REGNUM rather than INVALID_REGNUM in:

  for (i = 0; i < FIRST_PSEUDO_REGISTER; i++)
    {
      mips_dbx_regno[i] = INVALID_REGNUM;
      ...
    }

and remove the ALL_COP_REG loop that was in the earlier patch.

Richard
Steve Ellcey - Dec. 11, 2012, 10:02 p.m.
On Tue, 2012-12-11 at 21:52 +0000, Richard Sandiford wrote:

> > +  mips_dbx_regno[CPRESTORE_SLOT_REGNUM] = IGNORED_DWARF_REGNUM;


> If even fake registers like these are going to be used, then I think
> we should initialise to IGNORED_DWARF_REGNUM rather than INVALID_REGNUM in:
> 
>   for (i = 0; i < FIRST_PSEUDO_REGISTER; i++)
>     {
>       mips_dbx_regno[i] = INVALID_REGNUM;
>       ...
>     }
> 
> and remove the ALL_COP_REG loop that was in the earlier patch.
> 
> Richard

So far, this is the only fake register that I have seen show up while
building GCC, glibc, newlib, binutils, etc.  I am not sure if we want to
set all fake registers to IGNORED because of this one case.  If more
popped up then I could see us making that change.

Steve Ellcey
sellcey@mips.com
Richard Sandiford - Dec. 12, 2012, 8:21 a.m.
Steve Ellcey <sellcey@mips.com> writes:
> On Tue, 2012-12-11 at 21:52 +0000, Richard Sandiford wrote:
>
>> > +  mips_dbx_regno[CPRESTORE_SLOT_REGNUM] = IGNORED_DWARF_REGNUM;
>
>
>> If even fake registers like these are going to be used, then I think
>> we should initialise to IGNORED_DWARF_REGNUM rather than INVALID_REGNUM in:
>> 
>>   for (i = 0; i < FIRST_PSEUDO_REGISTER; i++)
>>     {
>>       mips_dbx_regno[i] = INVALID_REGNUM;
>>       ...
>>     }
>> 
>> and remove the ALL_COP_REG loop that was in the earlier patch.
>> 
>> Richard
>
> So far, this is the only fake register that I have seen show up while
> building GCC, glibc, newlib, binutils, etc.  I am not sure if we want to
> set all fake registers to IGNORED because of this one case.  If more
> popped up then I could see us making that change.

I'm pretty sure we'll need more eventually though.  A quick inspection
shows that we don't set mips_dbx_regno for DSP_ACC_REGS or ST_REGS.
DSP_ACC_REGS in paticular seems likely to hit, although you need to
test with an -mdsp option to get coverage.  It's even conceivable
that we could end up with info for GOT_VERSION_REGNUM.

I don't think we really gain much by trying to distinguish in mips_dbx_regno
between INVALID_REGNUM (register isn't used at all by the compiler) and
IGNORED_DWARF_REGNUM.  Especially since we don't distinguish between
registers that have been disabled through -msoft-float, -mips3 and below
(for ST_REGS other than $fcc0), -mno-dsp, etc.

Richard

Patch

diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
index 820b228..8113e5d 100644
--- a/gcc/config/mips/mips.c
+++ b/gcc/config/mips/mips.c
@@ -16760,6 +16760,8 @@  mips_option_override (void)
   for (i = ALL_COP_REG_FIRST; i <= ALL_COP_REG_LAST; i++)
     mips_dbx_regno[i] = IGNORED_DWARF_REGNUM;
 
+  mips_dbx_regno[CPRESTORE_SLOT_REGNUM] = IGNORED_DWARF_REGNUM;
+
   /* Accumulator debug registers use big-endian ordering.  */
   mips_dbx_regno[HI_REGNUM] = MD_DBX_FIRST + 0;
   mips_dbx_regno[LO_REGNUM] = MD_DBX_FIRST + 1;