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

Andrew Burgess
2018-11-27T11:13:18Z

* 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