gdb/regcache: When saving, ignore registers that can't be read

  If during a call to reg_buffer::save GDB encounters an error trying to
read a register then this should not cause GDB to crash, nor should it
force the save to quit.  Instead, GDB should just treat the register
as unavailable and push on.

The specific example I encountered was a RISC-V remote target that
claimed in its target description to have floating point register
support.  However, this was not true, when GDB tried to read a
floating point register the remote sent back an error.

Mostly this was fine, the program I was testing were integer only,
however, when trying to make an inferior call, GDB would try to
preserve the current values of the floating point registers, this
result in a read of a register that threw an error, and GDB would
crash like this:

    (gdb) call some_inferior_function ()
    ../../src/gdb/regcache.c:310: internal-error: void regcache::restore(readonly_detached_regcache*): Assertion `src != NULL' failed.
    A problem internal to GDB has been detected,
    further debugging may prove unreliable.
    Quit this debugging session? (y or n)

I acknowledge that the target description sent back in this case is
wrong, and the target should be fixed.  However, I think that GDB
should, at a minimum, not crash and burn in this case, and better, I
think GDB can probably just push on, ignoring the registers that can't
be read.

The solution I propose in this patch is to catch errors in
reg_buffer::save while calling cooked_read, and register that throws
an error should be considered unavailable.  GDB will not try to
restore these registers after the inferior call.

What I haven't done in this commit is provide any user feedback that
GDB would like to backup a particular register, but can't.  Right now
I figure that if the user cares about this they would probably try 'p
$reg_name' themselves, at which point it becomes obvious that the
register can't be read.  That said, I'm open to adding a warning that
the regiter failed to save if that is thought important.

I've tested this using on X86-64/Linux native, and for
native-gdbserver with no regressions.  Against my miss-behaving target
I can now make inferior calls without any problems.

gdb/ChangeLog:

	* regcache.c (reg_buffer::save): When saving the current register
	state, ignore registers that can't be read.
---
 gdb/ChangeLog  |  5 +++++
 gdb/regcache.c | 12 +++++++++++-
 2 files changed, 16 insertions(+), 1 deletion(-)
  

On 2018-11-21 1:17 p.m., Andrew Burgess wrote:
> If during a call to reg_buffer::save GDB encounters an error trying to
> read a register then this should not cause GDB to crash, nor should it
> force the save to quit.  Instead, GDB should just treat the register
> as unavailable and push on.
> 
> The specific example I encountered was a RISC-V remote target that
> claimed in its target description to have floating point register
> support.  However, this was not true, when GDB tried to read a
> floating point register the remote sent back an error.
> 
> Mostly this was fine, the program I was testing were integer only,
> however, when trying to make an inferior call, GDB would try to
> preserve the current values of the floating point registers, this
> result in a read of a register that threw an error, and GDB would
> crash like this:
> 
>     (gdb) call some_inferior_function ()
>     ../../src/gdb/regcache.c:310: internal-error: void regcache::restore(readonly_detached_regcache*): Assertion `src != NULL' failed.
>     A problem internal to GDB has been detected,
>     further debugging may prove unreliable.
>     Quit this debugging session? (y or n)
> 
> I acknowledge that the target description sent back in this case is
> wrong, and the target should be fixed.  However, I think that GDB
> should, at a minimum, not crash and burn in this case, and better, I
> think GDB can probably just push on, ignoring the registers that can't
> be read.
> 
> The solution I propose in this patch is to catch errors in
> reg_buffer::save while calling cooked_read, and register that throws
> an error should be considered unavailable.  GDB will not try to
> restore these registers after the inferior call.
> 
> What I haven't done in this commit is provide any user feedback that
> GDB would like to backup a particular register, but can't.  Right now
> I figure that if the user cares about this they would probably try 'p
> $reg_name' themselves, at which point it becomes obvious that the
> register can't be read.  That said, I'm open to adding a warning that
> the regiter failed to save if that is thought important.
> 
> I've tested this using on X86-64/Linux native, and for
> native-gdbserver with no regressions.  Against my miss-behaving target
> I can now make inferior calls without any problems.
> 
> gdb/ChangeLog:
> 
> 	* regcache.c (reg_buffer::save): When saving the current register
> 	state, ignore registers that can't be read.
> ---
>  gdb/ChangeLog  |  5 +++++
>  gdb/regcache.c | 12 +++++++++++-
>  2 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/regcache.c b/gdb/regcache.c
> index 946035ae67a..b89be24ccb6 100644
> --- a/gdb/regcache.c
> +++ b/gdb/regcache.c
> @@ -277,7 +277,17 @@ reg_buffer::save (register_read_ftype cooked_read)
>        if (gdbarch_register_reggroup_p (gdbarch, regnum, save_reggroup))
>  	{
>  	  gdb_byte *dst_buf = register_buffer (regnum);
> -	  enum register_status status = cooked_read (regnum, dst_buf);
> +	  enum register_status status;
> +
> +	  TRY
> +	    {
> +	      status = cooked_read (regnum, dst_buf);
> +	    }
> +	  CATCH (ex, RETURN_MASK_ERROR)
> +	    {
> +	      status = REG_UNAVAILABLE;
> +	    }
> +	  END_CATCH
>  
>  	  gdb_assert (status != REG_UNKNOWN);
>  
> 


Hi Andrew,

I think your fix makes sense.

About the assertion you hit, I think it shows a weakness in infcall_suspend_state_up.
The deleter is not able to handle the state of infcall_suspend_state where the
registers field is NULL.

So either:

1. We decide that an infcall_suspend_state with a NULL registers field is an invalid
   state and we make sure to never have one in this state.
2. We change the deleter (consequently restore_infcall_suspend_state) to have it
   handle the possibility of registers == NULL.

This means that even without your fix, GDB should ideally be able to handle the failure
more gracefully than it does now.  The infcall should just be aborted and an error message
shown.

Does that make sense?

Simon
  

* Simon Marchi <simark@simark.ca> [2018-11-25 21:48:35 -0500]:

> On 2018-11-21 1:17 p.m., Andrew Burgess wrote:
> > If during a call to reg_buffer::save GDB encounters an error trying to
> > read a register then this should not cause GDB to crash, nor should it
> > force the save to quit.  Instead, GDB should just treat the register
> > as unavailable and push on.
> > 
> > The specific example I encountered was a RISC-V remote target that
> > claimed in its target description to have floating point register
> > support.  However, this was not true, when GDB tried to read a
> > floating point register the remote sent back an error.
> > 
> > Mostly this was fine, the program I was testing were integer only,
> > however, when trying to make an inferior call, GDB would try to
> > preserve the current values of the floating point registers, this
> > result in a read of a register that threw an error, and GDB would
> > crash like this:
> > 
> >     (gdb) call some_inferior_function ()
> >     ../../src/gdb/regcache.c:310: internal-error: void regcache::restore(readonly_detached_regcache*): Assertion `src != NULL' failed.
> >     A problem internal to GDB has been detected,
> >     further debugging may prove unreliable.
> >     Quit this debugging session? (y or n)
> > 
> > I acknowledge that the target description sent back in this case is
> > wrong, and the target should be fixed.  However, I think that GDB
> > should, at a minimum, not crash and burn in this case, and better, I
> > think GDB can probably just push on, ignoring the registers that can't
> > be read.
> > 
> > The solution I propose in this patch is to catch errors in
> > reg_buffer::save while calling cooked_read, and register that throws
> > an error should be considered unavailable.  GDB will not try to
> > restore these registers after the inferior call.
> > 
> > What I haven't done in this commit is provide any user feedback that
> > GDB would like to backup a particular register, but can't.  Right now
> > I figure that if the user cares about this they would probably try 'p
> > $reg_name' themselves, at which point it becomes obvious that the
> > register can't be read.  That said, I'm open to adding a warning that
> > the regiter failed to save if that is thought important.
> > 
> > I've tested this using on X86-64/Linux native, and for
> > native-gdbserver with no regressions.  Against my miss-behaving target
> > I can now make inferior calls without any problems.
> > 
> > gdb/ChangeLog:
> > 
> > 	* regcache.c (reg_buffer::save): When saving the current register
> > 	state, ignore registers that can't be read.
> > ---
> >  gdb/ChangeLog  |  5 +++++
> >  gdb/regcache.c | 12 +++++++++++-
> >  2 files changed, 16 insertions(+), 1 deletion(-)
> > 
> > diff --git a/gdb/regcache.c b/gdb/regcache.c
> > index 946035ae67a..b89be24ccb6 100644
> > --- a/gdb/regcache.c
> > +++ b/gdb/regcache.c
> > @@ -277,7 +277,17 @@ reg_buffer::save (register_read_ftype cooked_read)
> >        if (gdbarch_register_reggroup_p (gdbarch, regnum, save_reggroup))
> >  	{
> >  	  gdb_byte *dst_buf = register_buffer (regnum);
> > -	  enum register_status status = cooked_read (regnum, dst_buf);
> > +	  enum register_status status;
> > +
> > +	  TRY
> > +	    {
> > +	      status = cooked_read (regnum, dst_buf);
> > +	    }
> > +	  CATCH (ex, RETURN_MASK_ERROR)
> > +	    {
> > +	      status = REG_UNAVAILABLE;
> > +	    }
> > +	  END_CATCH
> >  
> >  	  gdb_assert (status != REG_UNKNOWN);
> >  
> > 
> 
> 
> Hi Andrew,
> 
> I think your fix makes sense.
> 
> About the assertion you hit, I think it shows a weakness in infcall_suspend_state_up.
> The deleter is not able to handle the state of infcall_suspend_state where the
> registers field is NULL.
> 
> So either:
> 
> 1. We decide that an infcall_suspend_state with a NULL registers field is an invalid
>    state and we make sure to never have one in this state.
> 2. We change the deleter (consequently restore_infcall_suspend_state) to have it
>    handle the possibility of registers == NULL.
> 
> This means that even without your fix, GDB should ideally be able to handle the failure
> more gracefully than it does now.  The infcall should just be aborted and an error message
> shown.
> 
> Does that make sense?

Yes it does.

In this new series, patch #1 makes the prepare for inferior function
call process more resistant to errors during the preparation phase.
After this patch the case I addressed above would fail with an error
(better than an assertion).

In patch #2 I then handle the specific case I am encountering better,
so that for the case that a register can't be read, GDB still performs
the inferior function call.

And patch #3 is a random fix I hit while testing the above patches.

How does this look?

Thanks,
Andrew

---

Andrew Burgess (3):
  gdb/infcall: Make infcall_suspend_state more class like
  gdb/regcache: When saving, ignore registers that can't be read
  gdb: Update test pattern to deal with native-extended-gdbserver

 gdb/ChangeLog                      |  25 +++++++
 gdb/infrun.c                       | 132 ++++++++++++++++++++++---------------
 gdb/regcache.c                     |  12 +++-
 gdb/testsuite/ChangeLog            |   4 ++
 gdb/testsuite/gdb.base/annota1.exp |  23 ++++++-
 5 files changed, 141 insertions(+), 55 deletions(-)
  

This revision doesn't change much over v2, except that I have dropped
the previous patch #2 in response to Pedro's feedback.

I still think that patch #1 is worth while, it _is_ possible to throw
errors while reading registers, and ideally this wouldn't cause an
assertion (which with patch #1 it no longer will).

What is now patch #2, but was previously #3 is just a small cleanup of
test results.

---

Andrew Burgess (2):
  gdb/infcall: Make infcall_suspend_state more class like
  gdb: Update test pattern to deal with native-extended-gdbserver

 gdb/ChangeLog                      |  20 ++++++
 gdb/infrun.c                       | 132 ++++++++++++++++++++++---------------
 gdb/testsuite/ChangeLog            |   4 ++
 gdb/testsuite/gdb.base/annota1.exp |  23 ++++++-
 4 files changed, 125 insertions(+), 54 deletions(-)
  

On 12/12/2018 03:16 PM, Andrew Burgess wrote:
> This revision doesn't change much over v2, except that I have dropped
> the previous patch #2 in response to Pedro's feedback.
> 
> I still think that patch #1 is worth while, it _is_ possible to throw
> errors while reading registers, and ideally this wouldn't cause an
> assertion (which with patch #1 it no longer will).
> 
> What is now patch #2, but was previously #3 is just a small cleanup of
> test results.

Sorry for the delay, I did not intend to block this.  I've reviewed
the patches now.

Thanks,
Pedro Alves
  

* Pedro Alves <palves@redhat.com> [2018-12-12 16:14:45 +0000]:

> On 12/12/2018 03:16 PM, Andrew Burgess wrote:
> > This revision doesn't change much over v2, except that I have dropped
> > the previous patch #2 in response to Pedro's feedback.
> > 
> > I still think that patch #1 is worth while, it _is_ possible to throw
> > errors while reading registers, and ideally this wouldn't cause an
> > assertion (which with patch #1 it no longer will).
> > 
> > What is now patch #2, but was previously #3 is just a small cleanup of
> > test results.
> 
> Sorry for the delay, I did not intend to block this.  I've reviewed
> the patches now.

Absolutely not a problem.  Thanks for the review.

Pushed with the fixes you identified.

Andrew