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

  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.
  

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
  

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.