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

[Doug Evans <dje at google dot com>]

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.

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;
 }