diff mbox

[v2,PR,symtab/17391] gdb internal error: assertion fails in regcache.c:178

Message ID 001a113308e8dafbc3051d8af62f@google.com
State New
Headers show

Commit Message

Doug Evans Aug. 18, 2015, 12:26 a.m. UTC 
Doug Evans writes:
  > Pedro Alves writes:

  >   > On 08/13/2015 02:21 AM, Doug Evans wrote:

  >   > > Doug Evans writes:

  >   > >   > Hi.

  >   > >   >

  >   > >   > This patch, I hope, addresses PR 17391.

  >   > >   > I couldn't recreate 17391 but there is clearly a lack of

  > robustness here:

  >   > >   > gdbarch_dwarf2_reg_to_regnum can return -1 but not all callers

  >   > >   > watch for that.  Some callers do watch for it but call error

  > themselves.

  >   > >   > There is already dwarf2_reg_to_regnum_or_error so this patch  

just
  >   > >   > changes appropriate callers of gdbarch_dwarf2_reg_to_regnum to  

use
  > it.

  >   > >   > Sometimes a register number is stored as a ULONGEST, so I  

changed
  >   > >   > dwarf2_reg_to_regnum_or_error to take one.

  >   > >   >

  >   > >   > Regression tested on amd64-linux.

  >   > >   >

  >   > >   > 2015-08-10  Doug Evans  <dje@google.com>

  >   > >   >

  >   > >   > 	PR symtab/17391

  >   > >   > 	* dwarf2-frame.c (read_addr_from_reg): Call

  >   > >   > 	dwarf2_reg_to_regnum_or_error instead of

  > gdbarch_dwarf2_reg_to_regnum.

  >   > >   > 	(get_reg_value): Ditto.

  >   > >   > 	(dwarf2_fetch_cfa_info): Ditto.

  >   > >   > 	(dwarf2_frame_prev_register): Ditto.

  >   > >   > 	* dwarf2loc.c (dwarf_expr_read_addr_from_reg): Ditto.

  >   > >   > 	(dwarf_expr_get_reg_value): Ditto.

  >   > >   > 	(read_pieced_value): Ditto.

  >   > >   > 	(write_pieced_value): Ditto.

  >   > >   > 	(dwarf2_evaluate_loc_desc_full): Ditto.

  >   > >   > 	(dwarf2_reg_to_regnum_or_error): Change to take a ULONGEST  

regnum.
  >   > >   > 	(locexpr_regname): Improve text of bad register number.

  >   > >   > 	* dwarf2loc.h (dwarf2_reg_to_regnum_or_error): Update  

prototype.
  >   > >   >

  >   > >   > 	testsuite/

  >   > >   > 	* lib/gdb.exp (_location): Add support for DW_OP_regx.

  >   > >   > 	* gdb.dwarf2/bad-regnum.c: New file.

  >   > >   > 	* gdb.dwarf2/bad-regnum.exp: New file.

  >   > >

  >   > > Hi.

  >   > >

  >   > > This is a revised version.

  >   > > Digging deeper I found several broken targets.

  >   > > I'm not prepared to fix all of them,

  >   >

  >   > OOC, was that because the fix looked complicated, or because

  >   > you wanted to avoid breaking them?  If the latter, I'd be more  

inclined
  >   > to avoiding leaving a wart in place that is probably going to take

  >   > a long while to address, even if that meant the fix goes in untested.

  >   > Is there an easy way to tell which targets still need fixing?  Or  

what
  >   > to look for?

  >

  > More like for those I left it wasn't clear to me how to fix them.

  > e.g., rs6000-tdep.c:rs6000_dwarf2_reg_to_regnum.

  > I'm certainly not making things worse by leaving them alone.

  >

  >   > > but this fixes a lot of them and adds documentation

  >   > > to actually specify what the rules are

  >   > > (as opposed to targets just cut-n-pasting

  >   > > and hoping it was right).

  >   >

  >   > Otherwise the patch looked fine to me.


I missed a few obvious cases.
The main remaining cases that I can think of are rs6000 and spu.
Plus this supplemental patch makes stabs and ecoff more robust.
Stabs was using num_arch + num_pseudo to denote "bad reg",
but some targets use the same reg_to_regnum function for both
stabs and dwarf, so that's asking for trouble (since dwarf uses -1).
This patch makes stabs handle negative regnos too.

If it's too hard to review this apart from the original patch
I can resubmit the entire thing.

2015-08-17  Doug Evans  <dje@google.com>

	* m68k-tdep.c (m68k_dwarf_reg_to_regnum): Fix error result.
	* mep-tdep.c (mep_debug_reg_to_regnum): Ditto.
	* mips-tdep.c (mips_stab_reg_to_regnum): Ditto.
	(mips_dwarf_dwarf2_ecoff_reg_to_regnum): Ditto.
	* mn10300-tdep.c (mn10300_dwarf2_reg_to_regnum): Remove warning
	for bad regs.
	* xtensa-tdep.c (xtensa_reg_to_regnum): Remove internal error for
	bad regs.  Fix error result.
	* stabsread.c (stab_reg_to_regnum): Watch for negative regno.
	(reg_value_complaint): Update complaint text.
	* mdebugread.c (reg_value_complaint): New function.
	(mdebug_reg_to_regnum): Rewrite to watch for bad reg numbers.

Comments

Pedro Alves Aug. 18, 2015, 11:48 a.m. UTC | #1 
On 08/18/2015 01:26 AM, Doug Evans wrote:

>   > More like for those I left it wasn't clear to me how to fix them.
>   > e.g., rs6000-tdep.c:rs6000_dwarf2_reg_to_regnum.
>   > I'm certainly not making things worse by leaving them alone.
>   >

OK.  Neither rs6000 spu look like cases that
returns num_regs + num_pseudo_regs for bad registers, as suggested
here, though:

+	/* If there's no corresponding GDB register, ignore it.
+	   Some targets return num_regs (+ num_pseudo_regs) for bad registers.
+	   Handle them until they are fixed.  */
  	if (regnum < 0 || regnum >= num_regs)
  	  continue;

IMO, as written, this is the sort of comment that ends up staying
stale forever even after all such targets stop returning num_regs+num_pseudo_regs
specifically (they may still return bogus register numbers higher
than that).  After your supplemental patch, I think we can just drop it
or say "Protect against a target returning a bad register" or some such?

> I missed a few obvious cases.
> The main remaining cases that I can think of are rs6000 and spu.
> Plus this supplemental patch makes stabs and ecoff more robust.
> Stabs was using num_arch + num_pseudo to denote "bad reg",
> but some targets use the same reg_to_regnum function for both
> stabs and dwarf, so that's asking for trouble (since dwarf uses -1).
> This patch makes stabs handle negative regnos too.
> 
> If it's too hard to review this apart from the original patch
> I can resubmit the entire thing.

Looks good to me.

Thanks,
Pedro Alves
Doug Evans Oct. 26, 2015, 11:08 p.m. UTC | #2 
On Tue, Aug 18, 2015 at 4:48 AM, Pedro Alves <palves@redhat.com> wrote:
> On 08/18/2015 01:26 AM, Doug Evans wrote:
>
>>   > More like for those I left it wasn't clear to me how to fix them.
>>   > e.g., rs6000-tdep.c:rs6000_dwarf2_reg_to_regnum.
>>   > I'm certainly not making things worse by leaving them alone.
>>   >
>
> OK.  Neither rs6000 spu look like cases that
> returns num_regs + num_pseudo_regs for bad registers, as suggested
> here, though:
>
> +       /* If there's no corresponding GDB register, ignore it.
> +          Some targets return num_regs (+ num_pseudo_regs) for bad registers.
> +          Handle them until they are fixed.  */
>         if (regnum < 0 || regnum >= num_regs)
>           continue;
>
> IMO, as written, this is the sort of comment that ends up staying
> stale forever even after all such targets stop returning num_regs+num_pseudo_regs
> specifically (they may still return bogus register numbers higher
> than that).  After your supplemental patch, I think we can just drop it
> or say "Protect against a target returning a bad register" or some such?
>
>> I missed a few obvious cases.
>> The main remaining cases that I can think of are rs6000 and spu.
>> Plus this supplemental patch makes stabs and ecoff more robust.
>> Stabs was using num_arch + num_pseudo to denote "bad reg",
>> but some targets use the same reg_to_regnum function for both
>> stabs and dwarf, so that's asking for trouble (since dwarf uses -1).
>> This patch makes stabs handle negative regnos too.
>>
>> If it's too hard to review this apart from the original patch
>> I can resubmit the entire thing.
>
> Looks good to me.

Committed with the suggested change. Thanks.
diff mbox

Patch

diff --git a/gdb/m68k-tdep.c b/gdb/m68k-tdep.c
index d7347b6..5a3ed95 100644
--- a/gdb/m68k-tdep.c
+++ b/gdb/m68k-tdep.c
@@ -573,7 +573,7 @@  m68k_dwarf_reg_to_regnum (struct gdbarch *gdbarch, int  
num)
      /* pc */
      return M68K_PC_REGNUM;
    else
-    return gdbarch_num_regs (gdbarch) + gdbarch_num_pseudo_regs (gdbarch);
+    return -1;
  }

  
diff --git a/gdb/mdebugread.c b/gdb/mdebugread.c
index 3a81615..90f1f51 100644
--- a/gdb/mdebugread.c
+++ b/gdb/mdebugread.c
@@ -521,6 +521,14 @@  add_pending (FDR *fh, char *sh, struct type *t)

  /* Parsing Routines proper.  */

+static void
+reg_value_complaint (int regnum, int num_regs, const char *sym)
+{
+  complaint (&symfile_complaints,
+	     _("bad register number %d (max %d) in symbol %s"),
+             regnum, num_regs - 1, sym);
+}
+
  /* Parse a single symbol.  Mostly just make up a GDB symbol for it.
     For blocks, procedures and types we open a new lexical context.
     This is basically just a big switch on the symbol's type.  Argument
@@ -533,7 +541,21 @@  add_pending (FDR *fh, char *sh, struct type *t)
  static int
  mdebug_reg_to_regnum (struct symbol *sym, struct gdbarch *gdbarch)
  {
-  return gdbarch_ecoff_reg_to_regnum (gdbarch, SYMBOL_VALUE (sym));
+  int regno = gdbarch_ecoff_reg_to_regnum (gdbarch, SYMBOL_VALUE (sym));
+
+  if (regno < 0
+      || regno >= (gdbarch_num_regs (gdbarch)
+		   + gdbarch_num_pseudo_regs (gdbarch)))
+    {
+      reg_value_complaint (regno,
+			   gdbarch_num_regs (gdbarch)
+			     + gdbarch_num_pseudo_regs (gdbarch),
+			   SYMBOL_PRINT_NAME (sym));
+
+      regno = gdbarch_sp_regnum (gdbarch); /* Known safe, though useless.   
*/
+    }
+
+  return regno;
  }

  static const struct symbol_register_ops mdebug_register_funcs = {
diff --git a/gdb/mep-tdep.c b/gdb/mep-tdep.c
index d2831db..7ab4928 100644
--- a/gdb/mep-tdep.c
+++ b/gdb/mep-tdep.c
@@ -784,7 +784,9 @@  static int
  mep_debug_reg_to_regnum (struct gdbarch *gdbarch, int debug_reg)
  {
    /* The debug info uses the raw register numbers.  */
-  return mep_raw_to_pseudo[debug_reg];
+  if (debug_reg >= 0 && debug_reg < ARRAY_SIZE (mep_raw_to_pseudo))
+    return mep_raw_to_pseudo[debug_reg];
+  return -1;
  }


diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
index e0706db..4bf0d99 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -8020,9 +8020,7 @@  mips_stab_reg_to_regnum (struct gdbarch *gdbarch, int  
num)
    else if (mips_regnum (gdbarch)->dspacc != -1 && num >= 72 && num < 78)
      regnum = num + mips_regnum (gdbarch)->dspacc - 72;
    else
-    /* This will hopefully (eventually) provoke a warning.  Should
-       we be calling complaint() here?  */
-    return gdbarch_num_regs (gdbarch) + gdbarch_num_pseudo_regs (gdbarch);
+    return -1;
    return gdbarch_num_regs (gdbarch) + regnum;
  }

@@ -8045,9 +8043,7 @@  mips_dwarf_dwarf2_ecoff_reg_to_regnum (struct gdbarch  
*gdbarch, int num)
    else if (mips_regnum (gdbarch)->dspacc != -1 && num >= 66 && num < 72)
      regnum = num + mips_regnum (gdbarch)->dspacc - 66;
    else
-    /* This will hopefully (eventually) provoke a warning.  Should we
-       be calling complaint() here?  */
-    return gdbarch_num_regs (gdbarch) + gdbarch_num_pseudo_regs (gdbarch);
+    return -1;
    return gdbarch_num_regs (gdbarch) + regnum;
  }

diff --git a/gdb/mn10300-tdep.c b/gdb/mn10300-tdep.c
index 985821c..83a3e48 100644
--- a/gdb/mn10300-tdep.c
+++ b/gdb/mn10300-tdep.c
@@ -1383,10 +1383,7 @@  mn10300_dwarf2_reg_to_regnum (struct gdbarch  
*gdbarch, int dwarf2)

    if (dwarf2 < 0
        || dwarf2 >= ARRAY_SIZE (dwarf2_to_gdb))
-    {
-      warning (_("Bogus register number in debug info: %d"), dwarf2);
-      return -1;
-    }
+    return -1;

    return dwarf2_to_gdb[dwarf2];
  }
diff --git a/gdb/stabsread.c b/gdb/stabsread.c
index 03c9eb1..f6d8343 100644
--- a/gdb/stabsread.c
+++ b/gdb/stabsread.c
@@ -169,7 +169,7 @@  static void
  reg_value_complaint (int regnum, int num_regs, const char *sym)
  {
    complaint (&symfile_complaints,
-	     _("register number %d too large (max %d) in symbol %s"),
+	     _("bad register number %d (max %d) in symbol %s"),
               regnum, num_regs - 1, sym);
  }

@@ -598,8 +598,9 @@  stab_reg_to_regnum (struct symbol *sym, struct gdbarch  
*gdbarch)
  {
    int regno = gdbarch_stab_reg_to_regnum (gdbarch, SYMBOL_VALUE (sym));

-  if (regno >= gdbarch_num_regs (gdbarch)
-		+ gdbarch_num_pseudo_regs (gdbarch))
+  if (regno < 0
+      || regno >= (gdbarch_num_regs (gdbarch)
+		   + gdbarch_num_pseudo_regs (gdbarch)))
      {
        reg_value_complaint (regno,
  			   gdbarch_num_regs (gdbarch)
diff --git a/gdb/xtensa-tdep.c b/gdb/xtensa-tdep.c
index 55e7d98..6c60070 100644
--- a/gdb/xtensa-tdep.c
+++ b/gdb/xtensa-tdep.c
@@ -354,9 +354,7 @@  xtensa_reg_to_regnum (struct gdbarch *gdbarch, int  
regnum)
      if (regnum == gdbarch_tdep (gdbarch)->regmap[i].target_number)
        return i;

-  internal_error (__FILE__, __LINE__,
-		  _("invalid dwarf/stabs register number %d"), regnum);
-  return 0;
+  return -1;
  }