[PATCHv2 0/3] Handle Errors While Preparing For Inferior Call

[Andrew Burgess <andrew dot burgess at embecosm dot com>]

* 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(-)

-- 
2.14.5