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

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

[Andrew Burgess]

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);
 
-- 
2.14.5

Re: [PATCH] gdb/regcache: When saving, ignore registers that can't be read

[Simon Marchi]

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

[PATCHv2 1/3] gdb/infcall: Make infcall_suspend_state more class like

[Andrew Burgess]

I ran into a situation where attempting to make an inferior function
call would trigger an assertion, 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)

The problem that triggers the assertion is that in the function
save_infcall_suspend_state, we basically did this:

    1. Create empty infcall_suspend_state object.
    2. Fill fields of infcall_suspend_state object.

The problem is causes is that if filling any of the fields triggered
an exception then the infcall_suspend_state object would be deleted
while in a partially filled in state.

In the specific case I encountered, I had a remote RISC-V target that
claimed in its target description to support floating point registers.
However, this was not true, and when GDB tried to read a floating
point register the remote sent back an error.  This error would cause
an exception to be thrown while creating the
readonly_detached_regcache, which in turn caused GDB to try and delete
an infcall_suspend_state which didn't have any register state, and
this triggered the assertion.

To prevent this problem we have two possibilities, either, rewrite the
restore code the handle partially initialised infcall_suspend_state
objects, or, prevent partially initialised infcall_suspend_state
objects from existing.  The second of these seems like a better
solution.

So, in this patch, I move the filling in of the different
infcall_suspend_state fields within a new constructor for
infcall_suspend_state.  Now, if generating one of those fields fails
the destructor for infcall_suspend_state will not be executed and GDB
will not try to restore the partially saved state.

With this patch in place GDB now behaves like this:

    (gdb) call some_inferior_function ()
    Could not fetch register "ft0"; remote failure reply 'E99'
    (gdb)

The inferior function call is aborted due to the error.

This has been tested against x86-64/Linux native, native-gdbserver,
and native-extended-gdbserver with no regressions.  I've manually
tested this against my baddly behaving target and confirmed the
inferior function call is aborted as described above.

gdb/ChangeLog:

	* infrun.c (infcall_suspend_state::infcall_suspend_state): New.
	(infcall_suspend_state::get_registers): New.
	(infcall_suspend_state::restore): New.
	(infcall_suspend_state::thread_suspend): Rename to...
	(infcall_suspend_state::m_thread_suspend): ...this.
	(infcall_suspend_state::registers): Rename to...
	(infcall_suspend_state::m_registers): ...this.
	(infcall_suspend_state::siginfo_gdbarch): Rename to...
	(infcall_suspend_state::m_siginfo_gdbarch): ...this.
	(infcall_suspend_state::siginfo_data): Rename to...
	(infcall_suspend_state::m_siginfo_data): ...this.
	(save_infcall_suspend_state): Rewrite to use infcall_suspend_state
	constructor.
	(restore_infcall_suspend_state): Rewrite to use
	infcall_suspend_state::restore method.
	(get_infcall_suspend_state_regcache): Use
	infcall_suspend_state::get_registers method.
---
 gdb/ChangeLog |  20 +++++++++
 gdb/infrun.c  | 132 +++++++++++++++++++++++++++++++++++-----------------------
 2 files changed, 100 insertions(+), 52 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 46a8985f860..a9d7fa17aaf 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -8742,18 +8742,85 @@ siginfo_make_value (struct gdbarch *gdbarch, struct internalvar *var,
 
 struct infcall_suspend_state
 {
-  struct thread_suspend_state thread_suspend;
+public:
+  /* Capture state from GDBARCH, TP, and REGCACHE that must be restored
+     once the inferior function call has finished.  */
+  infcall_suspend_state (struct gdbarch *gdbarch,
+                         const struct thread_info *tp,
+                         struct regcache *regcache)
+    : m_thread_suspend (tp->suspend),
+      m_registers (new readonly_detached_regcache (*regcache))
+  {
+    gdb::unique_xmalloc_ptr<gdb_byte> siginfo_data;
 
-  /* Other fields:  */
-  std::unique_ptr<readonly_detached_regcache> registers;
+    if (gdbarch_get_siginfo_type_p (gdbarch))
+      {
+        struct type *type = gdbarch_get_siginfo_type (gdbarch);
+        size_t len = TYPE_LENGTH (type);
+
+        siginfo_data.reset ((gdb_byte *) xmalloc (len));
+
+        if (target_read (current_top_target (), TARGET_OBJECT_SIGNAL_INFO, NULL,
+                         siginfo_data.get (), 0, len) != len)
+          {
+            /* Errors ignored.  */
+            siginfo_data.reset (nullptr);
+          }
+      }
+
+    if (siginfo_data)
+      {
+        m_siginfo_gdbarch = gdbarch;
+        m_siginfo_data = std::move (siginfo_data);
+      }
+  }
+
+  /* Return a pointer to the stored register state.  */
+
+  readonly_detached_regcache * get_registers () const
+  {
+    return m_registers.get ();
+  }
+
+  /* Restores the stored state into GDBARCH, TP, and REGCACHE.  */
+
+  void restore (struct gdbarch *gdbarch,
+                struct thread_info *tp,
+                struct regcache *regcache) const
+  {
+    tp->suspend = m_thread_suspend;
+
+    if (m_siginfo_gdbarch == gdbarch)
+      {
+        struct type *type = gdbarch_get_siginfo_type (gdbarch);
+
+        /* Errors ignored.  */
+        target_write (current_top_target (), TARGET_OBJECT_SIGNAL_INFO, NULL,
+                      m_siginfo_data.get (), 0, TYPE_LENGTH (type));
+      }
+
+    /* The inferior can be gone if the user types "print exit(0)"
+       (and perhaps other times).  */
+    if (target_has_execution)
+      /* NB: The register write goes through to the target.  */
+      regcache->restore (get_registers ());
+  }
+
+private:
+  /* How the current thread stopped before the inferior function call was
+     executed.  */
+  struct thread_suspend_state m_thread_suspend;
+
+  /* The registers before the inferior function call was executed.  */
+  std::unique_ptr<readonly_detached_regcache> m_registers;
 
   /* Format of SIGINFO_DATA or NULL if it is not present.  */
-  struct gdbarch *siginfo_gdbarch = nullptr;
+  struct gdbarch *m_siginfo_gdbarch = nullptr;
 
   /* The inferior format depends on SIGINFO_GDBARCH and it has a length of
      TYPE_LENGTH (gdbarch_get_siginfo_type ()).  For different gdbarch the
      content would be invalid.  */
-  gdb::unique_xmalloc_ptr<gdb_byte> siginfo_data;
+  gdb::unique_xmalloc_ptr<gdb_byte> m_siginfo_data;
 };
 
 infcall_suspend_state_up
@@ -8762,39 +8829,16 @@ save_infcall_suspend_state ()
   struct thread_info *tp = inferior_thread ();
   struct regcache *regcache = get_current_regcache ();
   struct gdbarch *gdbarch = regcache->arch ();
-  gdb::unique_xmalloc_ptr<gdb_byte> siginfo_data;
-
-  if (gdbarch_get_siginfo_type_p (gdbarch))
-    {
-      struct type *type = gdbarch_get_siginfo_type (gdbarch);
-      size_t len = TYPE_LENGTH (type);
-
-      siginfo_data.reset ((gdb_byte *) xmalloc (len));
-
-      if (target_read (current_top_target (), TARGET_OBJECT_SIGNAL_INFO, NULL,
-		       siginfo_data.get (), 0, len) != len)
-	{
-	  /* Errors ignored.  */
-	  siginfo_data.reset (nullptr);
-	}
-    }
-
-  infcall_suspend_state_up inf_state (new struct infcall_suspend_state);
 
-  if (siginfo_data)
-    {
-      inf_state->siginfo_gdbarch = gdbarch;
-      inf_state->siginfo_data = std::move (siginfo_data);
-    }
-
-  inf_state->thread_suspend = tp->suspend;
+  infcall_suspend_state_up inf_state
+    (new struct infcall_suspend_state (gdbarch, tp, regcache));
 
-  /* run_inferior_call will not use the signal due to its `proceed' call with
-     GDB_SIGNAL_0 anyway.  */
+  /* Having saved the current state, adjust the thread state, discarding
+     any stop signal information, this is not useful when starting an
+     inferior function call and run_inferior_call will not use the signal
+     due to its `proceed' call with GDB_SIGNAL_0.  */
   tp->suspend.stop_signal = GDB_SIGNAL_0;
 
-  inf_state->registers.reset (new readonly_detached_regcache (*regcache));
-
   return inf_state;
 }
 
@@ -8807,23 +8851,7 @@ restore_infcall_suspend_state (struct infcall_suspend_state *inf_state)
   struct regcache *regcache = get_current_regcache ();
   struct gdbarch *gdbarch = regcache->arch ();
 
-  tp->suspend = inf_state->thread_suspend;
-
-  if (inf_state->siginfo_gdbarch == gdbarch)
-    {
-      struct type *type = gdbarch_get_siginfo_type (gdbarch);
-
-      /* Errors ignored.  */
-      target_write (current_top_target (), TARGET_OBJECT_SIGNAL_INFO, NULL,
-		    inf_state->siginfo_data.get (), 0, TYPE_LENGTH (type));
-    }
-
-  /* The inferior can be gone if the user types "print exit(0)"
-     (and perhaps other times).  */
-  if (target_has_execution)
-    /* NB: The register write goes through to the target.  */
-    regcache->restore (inf_state->registers.get ());
-
+  inf_state->restore (gdbarch, tp, regcache);
   discard_infcall_suspend_state (inf_state);
 }
 
@@ -8836,7 +8864,7 @@ discard_infcall_suspend_state (struct infcall_suspend_state *inf_state)
 readonly_detached_regcache *
 get_infcall_suspend_state_regcache (struct infcall_suspend_state *inf_state)
 {
-  return inf_state->registers.get ();
+  return inf_state->get_registers ();
 }
 
 /* infcall_control_state contains state regarding gdb's control of the
-- 
2.14.5

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

[Andrew Burgess]

* 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

[PATCHv2 2/3] gdb/regcache: When saving, ignore registers that can't be read

[Andrew Burgess]

The previous commit addressed an assertion that could trigger if a
target threw an error while saving state ahead of an inferior function
call.

The specific case that highlighted this issue was a RISC-V target that
claimed to support floating point registers, but when GDB tried to
read a floating point register the remote sent back an error.

With the previous commit we no longer see an assertion for this
target, now GDB abandons the inferior function call.

Although this is slightly better, it feels like for this specific case
GDB could do even better.  If during a call to reg_buffer::save GDB
encounters an error trying to read a register then GDB should simply
mark the register as unavailable and carry on.  The consequence of
marking the register unavailable is that GDB will not then try to
restore the register once the inferior function call is complete.

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 register 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 6e0e8c3e7e0..c9503295f59 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);
 
-- 
2.14.5

[PATCHv2 3/3] gdb: Update test pattern to deal with native-extended-gdbserver

[Andrew Burgess]

When running the test gdb.base/annota1.exp with:

  make check-gdb RUNTESTFLAGS="--target_board=native-extended-gdbserver gdb.base/annota1.exp"

I would see a failure due to some unexpecte lines in GDB's output.
The extra lines (when compared with a native run) were about file
transfer from the remote back to GDB.

This commit extends the regexp for this test to allow for these extra
lines, and also splits the rather long regexp up into a list of parts.

With this change in place I see no failures for gdb.base/annota1.exp
when using the native-extended-gdbserver target board, nor with a
native run on X86-64/Linux.

gdb/testsuite/ChangeLog:

	* gdb.base/annota1.exp: Update a test regexp.
---
 gdb/testsuite/ChangeLog            |  4 ++++
 gdb/testsuite/gdb.base/annota1.exp | 23 +++++++++++++++++++++--
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/gdb/testsuite/gdb.base/annota1.exp b/gdb/testsuite/gdb.base/annota1.exp
index 4b34aa84f29..b5a0e87c3ad 100644
--- a/gdb/testsuite/gdb.base/annota1.exp
+++ b/gdb/testsuite/gdb.base/annota1.exp
@@ -127,8 +127,27 @@ gdb_test_multiple "info break" "breakpoint info" {
 #exp_internal 1
 set binexp [string_to_regexp $binfile]
 gdb_test_multiple "run" "run until main breakpoint" {
-    -re "\r\n\032\032post-prompt\r\nStarting program: $binexp \(\r\nwarning: Skipping \[^\r\n\]+ .gdb_index section in \[^\r\n\]+\r\nDo \"set use-deprecated-index-sections on\" before the file is read\r\nto use the section anyway\\.\)?\(\(\r\n\r\n\032\032frames-invalid\)|\(\r\n\r\n\032\032breakpoints-invalid\)\)*\r\n\r\n\032\032starting\(\(\r\n\r\n\032\032frames-invalid\)|\(\r\n\r\n\032\032breakpoints-invalid\)\)*\r\n\r\n\032\032breakpoint 1\r\n\r\nBreakpoint 1, \r\n\032\032frame-begin 0 $hex\r\n\r\n\032\032frame-function-name\r\nmain\r\n\032\032frame-args\r\n \\(\\)\r\n\032\032frame-source-begin\r\n at \r\n\032\032frame-source-file\r\n.*annota1.c\r\n\032\032frame-source-file-end\r\n:\r\n\032\032frame-source-line\r\n$main_line\r\n\032\032frame-source-end\r\n\r\n\r\n\032\032source.*$srcfile:$main_line:.*:beg:$hex\r\n\r\n\032\032frame-end\r\n\r\n\032\032stopped.*$gdb_prompt$" {
-	pass "run until main breakpoint" 
+    -re [join { "\r\n\032\032post-prompt\r\nStarting program: $binexp " \
+		    "\(\(\r\nReading \[^\r\n\]+\)|\(\r\nwarning: File transfers from remote targets can be slow\[^\r\n\]+\)\)*" \
+		    "\(\r\nwarning: Skipping \[^\r\n\]+ .gdb_index section in \[^\r\n\]+\r\nDo \"set use-deprecated-index-sections on\" before the file is read\r\nto use the section anyway\\.\)?" \
+		    "\(\(\r\n\r\n\032\032frames-invalid\)|\(\r\n\r\n\032\032breakpoints-invalid\)\)*\r\n\r\n" \
+		    "\032\032starting\(\(\r\nReading \[^\r\n\]+\)|\(\r\nwarning: File transfers from remote targets can be slow\[^\r\n\]+\)\)*" \
+		    "\(\(\r\n\r\n\032\032frames-invalid\)|\(\r\n\r\n\032\032breakpoints-invalid\)\)*\r\n\r\n" \
+		    "\032\032breakpoint 1\r\n\r\n" \
+		    "Breakpoint 1, \r\n" \
+		    "\032\032frame-begin 0 $hex\r\n\r\n" \
+		    "\032\032frame-function-name\r\n" \
+		    "main\r\n" \
+		    "\032\032frame-args\r\n \\(\\)\r\n" \
+		    "\032\032frame-source-begin\r\n at \r\n" \
+		    "\032\032frame-source-file\r\n.*annota1.c\r\n" \
+		    "\032\032frame-source-file-end\r\n:\r\n" \
+		    "\032\032frame-source-line\r\n$main_line\r\n" \
+		    "\032\032frame-source-end\r\n\r\n\r\n" \
+		    "\032\032source.*$srcfile:$main_line:.*:beg:$hex\r\n\r\n" \
+		    "\032\032frame-end\r\n\r\n" \
+		    "\032\032stopped.*$gdb_prompt$" } ] {
+	pass "run until main breakpoint"
     }
 }
 #exp_internal 0
-- 
2.14.5

Re: [PATCHv2 2/3] gdb/regcache: When saving, ignore registers that can't be read

[Pedro Alves]

On 11/27/2018 11:13 AM, Andrew Burgess wrote:
> The previous commit addressed an assertion that could trigger if a
> target threw an error while saving state ahead of an inferior function
> call.
> 
> The specific case that highlighted this issue was a RISC-V target that
> claimed to support floating point registers, but when GDB tried to
> read a floating point register the remote sent back an error.
> 
> With the previous commit we no longer see an assertion for this
> target, now GDB abandons the inferior function call.
> 
> Although this is slightly better, it feels like for this specific case
> GDB could do even better.  If during a call to reg_buffer::save GDB
> encounters an error trying to read a register then GDB should simply
> mark the register as unavailable and carry on.  The consequence of
> marking the register unavailable is that GDB will not then try to
> restore the register once the inferior function call is complete.

I'm skeptical about this.  It sounds risky to me.  An infcall is
potentially state-destructive, and silencing errors just seems like asking
for trouble.  Particularly, while you're observing one specific error,
you're swallowing all kinds of errors.

> 
> 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, 

How is the user to know to do that without any kind of indication?

> at which point it becomes obvious that the
> register can't be read.  That said, I'm open to adding a warning that
> the register 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.
> 

I'm really not sure this is a good trade off.  

How could such a stub with this kind of problem end up in production?
It sounds like it can't have seen much wild use without someone running
into this.  Making GDB handle this scenario "gracefully" can only be useful
if this is really a kind of problem that can go undetected for a long
while and you plan on continuing to let users use the "bad" stub.
But what's the real scenario that would lead to that happening?

Thanks,
Pedro Alves

Re: [PATCHv2 2/3] gdb/regcache: When saving, ignore registers that can't be read

[Andrew Burgess]

* Pedro Alves <palves@redhat.com> [2018-11-27 12:41:51 +0000]:

> On 11/27/2018 11:13 AM, Andrew Burgess wrote:
> > The previous commit addressed an assertion that could trigger if a
> > target threw an error while saving state ahead of an inferior function
> > call.
> > 
> > The specific case that highlighted this issue was a RISC-V target that
> > claimed to support floating point registers, but when GDB tried to
> > read a floating point register the remote sent back an error.
> > 
> > With the previous commit we no longer see an assertion for this
> > target, now GDB abandons the inferior function call.
> > 
> > Although this is slightly better, it feels like for this specific case
> > GDB could do even better.  If during a call to reg_buffer::save GDB
> > encounters an error trying to read a register then GDB should simply
> > mark the register as unavailable and carry on.  The consequence of
> > marking the register unavailable is that GDB will not then try to
> > restore the register once the inferior function call is complete.
> 
> I'm skeptical about this.  It sounds risky to me.  An infcall is
> potentially state-destructive, and silencing errors just seems like asking
> for trouble.  Particularly, while you're observing one specific error,
> you're swallowing all kinds of errors.
> 
> > 
> > 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, 
> 
> How is the user to know to do that without any kind of indication?
> 
> > at which point it becomes obvious that the
> > register can't be read.  That said, I'm open to adding a warning that
> > the register 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.
> > 
> 
> I'm really not sure this is a good trade off.  
> 
> How could such a stub with this kind of problem end up in production?
> It sounds like it can't have seen much wild use without someone running
> into this.  Making GDB handle this scenario "gracefully" can only be useful
> if this is really a kind of problem that can go undetected for a long
> while and you plan on continuing to let users use the "bad" stub.
> But what's the real scenario that would lead to that happening?

Pedro,

Thanks for taking the time to review this patch.

Just wanted to confirm that (subject to review) the above feedback
doesn't prevent patch #1 or #3 being merged, correct?

Patch #1 specifically makes the inferior call error rather than
assert, which feels like it doesn't raise the same concerns you
discuss above.

Thanks,
Andrew

Re: [PATCHv2 2/3] gdb/regcache: When saving, ignore registers that can't be read

[Pedro Alves]

On 11/27/2018 03:30 PM, Andrew Burgess wrote:

> Thanks for taking the time to review this patch.
> 
> Just wanted to confirm that (subject to review) the above feedback
> doesn't prevent patch #1 or #3 being merged, correct?

Correct.

> Patch #1 specifically makes the inferior call error rather than
> assert, which feels like it doesn't raise the same concerns you
> discuss above.
Absolutely.

Thanks,
Pedro Alves

[PATCHv3 0/2] Handle Errors While Preparing For Inferior Call

[Andrew Burgess]

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

-- 
2.14.5

[PATCHv3 1/2] gdb/infcall: Make infcall_suspend_state more class like

[Andrew Burgess]

I ran into a situation where attempting to make an inferior function
call would trigger an assertion, 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)

The problem that triggers the assertion is that in the function
save_infcall_suspend_state, we basically did this:

    1. Create empty infcall_suspend_state object.
    2. Fill fields of infcall_suspend_state object.

The problem is causes is that if filling any of the fields triggered
an exception then the infcall_suspend_state object would be deleted
while in a partially filled in state.

In the specific case I encountered, I had a remote RISC-V target that
claimed in its target description to support floating point registers.
However, this was not true, and when GDB tried to read a floating
point register the remote sent back an error.  This error would cause
an exception to be thrown while creating the
readonly_detached_regcache, which in turn caused GDB to try and delete
an infcall_suspend_state which didn't have any register state, and
this triggered the assertion.

To prevent this problem we have two possibilities, either, rewrite the
restore code the handle partially initialised infcall_suspend_state
objects, or, prevent partially initialised infcall_suspend_state
objects from existing.  The second of these seems like a better
solution.

So, in this patch, I move the filling in of the different
infcall_suspend_state fields within a new constructor for
infcall_suspend_state.  Now, if generating one of those fields fails
the destructor for infcall_suspend_state will not be executed and GDB
will not try to restore the partially saved state.

With this patch in place GDB now behaves like this:

    (gdb) call some_inferior_function ()
    Could not fetch register "ft0"; remote failure reply 'E99'
    (gdb)

The inferior function call is aborted due to the error.

This has been tested against x86-64/Linux native, native-gdbserver,
and native-extended-gdbserver with no regressions.  I've manually
tested this against my baddly behaving target and confirmed the
inferior function call is aborted as described above.

gdb/ChangeLog:

	* infrun.c (infcall_suspend_state::infcall_suspend_state): New.
	(infcall_suspend_state::get_registers): New.
	(infcall_suspend_state::restore): New.
	(infcall_suspend_state::thread_suspend): Rename to...
	(infcall_suspend_state::m_thread_suspend): ...this.
	(infcall_suspend_state::registers): Rename to...
	(infcall_suspend_state::m_registers): ...this.
	(infcall_suspend_state::siginfo_gdbarch): Rename to...
	(infcall_suspend_state::m_siginfo_gdbarch): ...this.
	(infcall_suspend_state::siginfo_data): Rename to...
	(infcall_suspend_state::m_siginfo_data): ...this.
	(save_infcall_suspend_state): Rewrite to use infcall_suspend_state
	constructor.
	(restore_infcall_suspend_state): Rewrite to use
	infcall_suspend_state::restore method.
	(get_infcall_suspend_state_regcache): Use
	infcall_suspend_state::get_registers method.
---
 gdb/ChangeLog |  20 +++++++++
 gdb/infrun.c  | 132 +++++++++++++++++++++++++++++++++++-----------------------
 2 files changed, 100 insertions(+), 52 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 46a8985f860..a9d7fa17aaf 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -8742,18 +8742,85 @@ siginfo_make_value (struct gdbarch *gdbarch, struct internalvar *var,
 
 struct infcall_suspend_state
 {
-  struct thread_suspend_state thread_suspend;
+public:
+  /* Capture state from GDBARCH, TP, and REGCACHE that must be restored
+     once the inferior function call has finished.  */
+  infcall_suspend_state (struct gdbarch *gdbarch,
+                         const struct thread_info *tp,
+                         struct regcache *regcache)
+    : m_thread_suspend (tp->suspend),
+      m_registers (new readonly_detached_regcache (*regcache))
+  {
+    gdb::unique_xmalloc_ptr<gdb_byte> siginfo_data;
 
-  /* Other fields:  */
-  std::unique_ptr<readonly_detached_regcache> registers;
+    if (gdbarch_get_siginfo_type_p (gdbarch))
+      {
+        struct type *type = gdbarch_get_siginfo_type (gdbarch);
+        size_t len = TYPE_LENGTH (type);
+
+        siginfo_data.reset ((gdb_byte *) xmalloc (len));
+
+        if (target_read (current_top_target (), TARGET_OBJECT_SIGNAL_INFO, NULL,
+                         siginfo_data.get (), 0, len) != len)
+          {
+            /* Errors ignored.  */
+            siginfo_data.reset (nullptr);
+          }
+      }
+
+    if (siginfo_data)
+      {
+        m_siginfo_gdbarch = gdbarch;
+        m_siginfo_data = std::move (siginfo_data);
+      }
+  }
+
+  /* Return a pointer to the stored register state.  */
+
+  readonly_detached_regcache * get_registers () const
+  {
+    return m_registers.get ();
+  }
+
+  /* Restores the stored state into GDBARCH, TP, and REGCACHE.  */
+
+  void restore (struct gdbarch *gdbarch,
+                struct thread_info *tp,
+                struct regcache *regcache) const
+  {
+    tp->suspend = m_thread_suspend;
+
+    if (m_siginfo_gdbarch == gdbarch)
+      {
+        struct type *type = gdbarch_get_siginfo_type (gdbarch);
+
+        /* Errors ignored.  */
+        target_write (current_top_target (), TARGET_OBJECT_SIGNAL_INFO, NULL,
+                      m_siginfo_data.get (), 0, TYPE_LENGTH (type));
+      }
+
+    /* The inferior can be gone if the user types "print exit(0)"
+       (and perhaps other times).  */
+    if (target_has_execution)
+      /* NB: The register write goes through to the target.  */
+      regcache->restore (get_registers ());
+  }
+
+private:
+  /* How the current thread stopped before the inferior function call was
+     executed.  */
+  struct thread_suspend_state m_thread_suspend;
+
+  /* The registers before the inferior function call was executed.  */
+  std::unique_ptr<readonly_detached_regcache> m_registers;
 
   /* Format of SIGINFO_DATA or NULL if it is not present.  */
-  struct gdbarch *siginfo_gdbarch = nullptr;
+  struct gdbarch *m_siginfo_gdbarch = nullptr;
 
   /* The inferior format depends on SIGINFO_GDBARCH and it has a length of
      TYPE_LENGTH (gdbarch_get_siginfo_type ()).  For different gdbarch the
      content would be invalid.  */
-  gdb::unique_xmalloc_ptr<gdb_byte> siginfo_data;
+  gdb::unique_xmalloc_ptr<gdb_byte> m_siginfo_data;
 };
 
 infcall_suspend_state_up
@@ -8762,39 +8829,16 @@ save_infcall_suspend_state ()
   struct thread_info *tp = inferior_thread ();
   struct regcache *regcache = get_current_regcache ();
   struct gdbarch *gdbarch = regcache->arch ();
-  gdb::unique_xmalloc_ptr<gdb_byte> siginfo_data;
-
-  if (gdbarch_get_siginfo_type_p (gdbarch))
-    {
-      struct type *type = gdbarch_get_siginfo_type (gdbarch);
-      size_t len = TYPE_LENGTH (type);
-
-      siginfo_data.reset ((gdb_byte *) xmalloc (len));
-
-      if (target_read (current_top_target (), TARGET_OBJECT_SIGNAL_INFO, NULL,
-		       siginfo_data.get (), 0, len) != len)
-	{
-	  /* Errors ignored.  */
-	  siginfo_data.reset (nullptr);
-	}
-    }
-
-  infcall_suspend_state_up inf_state (new struct infcall_suspend_state);
 
-  if (siginfo_data)
-    {
-      inf_state->siginfo_gdbarch = gdbarch;
-      inf_state->siginfo_data = std::move (siginfo_data);
-    }
-
-  inf_state->thread_suspend = tp->suspend;
+  infcall_suspend_state_up inf_state
+    (new struct infcall_suspend_state (gdbarch, tp, regcache));
 
-  /* run_inferior_call will not use the signal due to its `proceed' call with
-     GDB_SIGNAL_0 anyway.  */
+  /* Having saved the current state, adjust the thread state, discarding
+     any stop signal information, this is not useful when starting an
+     inferior function call and run_inferior_call will not use the signal
+     due to its `proceed' call with GDB_SIGNAL_0.  */
   tp->suspend.stop_signal = GDB_SIGNAL_0;
 
-  inf_state->registers.reset (new readonly_detached_regcache (*regcache));
-
   return inf_state;
 }
 
@@ -8807,23 +8851,7 @@ restore_infcall_suspend_state (struct infcall_suspend_state *inf_state)
   struct regcache *regcache = get_current_regcache ();
   struct gdbarch *gdbarch = regcache->arch ();
 
-  tp->suspend = inf_state->thread_suspend;
-
-  if (inf_state->siginfo_gdbarch == gdbarch)
-    {
-      struct type *type = gdbarch_get_siginfo_type (gdbarch);
-
-      /* Errors ignored.  */
-      target_write (current_top_target (), TARGET_OBJECT_SIGNAL_INFO, NULL,
-		    inf_state->siginfo_data.get (), 0, TYPE_LENGTH (type));
-    }
-
-  /* The inferior can be gone if the user types "print exit(0)"
-     (and perhaps other times).  */
-  if (target_has_execution)
-    /* NB: The register write goes through to the target.  */
-    regcache->restore (inf_state->registers.get ());
-
+  inf_state->restore (gdbarch, tp, regcache);
   discard_infcall_suspend_state (inf_state);
 }
 
@@ -8836,7 +8864,7 @@ discard_infcall_suspend_state (struct infcall_suspend_state *inf_state)
 readonly_detached_regcache *
 get_infcall_suspend_state_regcache (struct infcall_suspend_state *inf_state)
 {
-  return inf_state->registers.get ();
+  return inf_state->get_registers ();
 }
 
 /* infcall_control_state contains state regarding gdb's control of the
-- 
2.14.5

[PATCHv3 2/2] gdb: Update test pattern to deal with native-extended-gdbserver

[Andrew Burgess]

When running the test gdb.base/annota1.exp with:

  make check-gdb RUNTESTFLAGS="--target_board=native-extended-gdbserver gdb.base/annota1.exp"

I would see a failure due to some unexpecte lines in GDB's output.
The extra lines (when compared with a native run) were about file
transfer from the remote back to GDB.

This commit extends the regexp for this test to allow for these extra
lines, and also splits the rather long regexp up into a list of parts.

With this change in place I see no failures for gdb.base/annota1.exp
when using the native-extended-gdbserver target board, nor with a
native run on X86-64/Linux.

gdb/testsuite/ChangeLog:

	* gdb.base/annota1.exp: Update a test regexp.
---
 gdb/testsuite/ChangeLog            |  4 ++++
 gdb/testsuite/gdb.base/annota1.exp | 23 +++++++++++++++++++++--
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/gdb/testsuite/gdb.base/annota1.exp b/gdb/testsuite/gdb.base/annota1.exp
index 4b34aa84f29..b5a0e87c3ad 100644
--- a/gdb/testsuite/gdb.base/annota1.exp
+++ b/gdb/testsuite/gdb.base/annota1.exp
@@ -127,8 +127,27 @@ gdb_test_multiple "info break" "breakpoint info" {
 #exp_internal 1
 set binexp [string_to_regexp $binfile]
 gdb_test_multiple "run" "run until main breakpoint" {
-    -re "\r\n\032\032post-prompt\r\nStarting program: $binexp \(\r\nwarning: Skipping \[^\r\n\]+ .gdb_index section in \[^\r\n\]+\r\nDo \"set use-deprecated-index-sections on\" before the file is read\r\nto use the section anyway\\.\)?\(\(\r\n\r\n\032\032frames-invalid\)|\(\r\n\r\n\032\032breakpoints-invalid\)\)*\r\n\r\n\032\032starting\(\(\r\n\r\n\032\032frames-invalid\)|\(\r\n\r\n\032\032breakpoints-invalid\)\)*\r\n\r\n\032\032breakpoint 1\r\n\r\nBreakpoint 1, \r\n\032\032frame-begin 0 $hex\r\n\r\n\032\032frame-function-name\r\nmain\r\n\032\032frame-args\r\n \\(\\)\r\n\032\032frame-source-begin\r\n at \r\n\032\032frame-source-file\r\n.*annota1.c\r\n\032\032frame-source-file-end\r\n:\r\n\032\032frame-source-line\r\n$main_line\r\n\032\032frame-source-end\r\n\r\n\r\n\032\032source.*$srcfile:$main_line:.*:beg:$hex\r\n\r\n\032\032frame-end\r\n\r\n\032\032stopped.*$gdb_prompt$" {
-	pass "run until main breakpoint" 
+    -re [join { "\r\n\032\032post-prompt\r\nStarting program: $binexp " \
+		    "\(\(\r\nReading \[^\r\n\]+\)|\(\r\nwarning: File transfers from remote targets can be slow\[^\r\n\]+\)\)*" \
+		    "\(\r\nwarning: Skipping \[^\r\n\]+ .gdb_index section in \[^\r\n\]+\r\nDo \"set use-deprecated-index-sections on\" before the file is read\r\nto use the section anyway\\.\)?" \
+		    "\(\(\r\n\r\n\032\032frames-invalid\)|\(\r\n\r\n\032\032breakpoints-invalid\)\)*\r\n\r\n" \
+		    "\032\032starting\(\(\r\nReading \[^\r\n\]+\)|\(\r\nwarning: File transfers from remote targets can be slow\[^\r\n\]+\)\)*" \
+		    "\(\(\r\n\r\n\032\032frames-invalid\)|\(\r\n\r\n\032\032breakpoints-invalid\)\)*\r\n\r\n" \
+		    "\032\032breakpoint 1\r\n\r\n" \
+		    "Breakpoint 1, \r\n" \
+		    "\032\032frame-begin 0 $hex\r\n\r\n" \
+		    "\032\032frame-function-name\r\n" \
+		    "main\r\n" \
+		    "\032\032frame-args\r\n \\(\\)\r\n" \
+		    "\032\032frame-source-begin\r\n at \r\n" \
+		    "\032\032frame-source-file\r\n.*annota1.c\r\n" \
+		    "\032\032frame-source-file-end\r\n:\r\n" \
+		    "\032\032frame-source-line\r\n$main_line\r\n" \
+		    "\032\032frame-source-end\r\n\r\n\r\n" \
+		    "\032\032source.*$srcfile:$main_line:.*:beg:$hex\r\n\r\n" \
+		    "\032\032frame-end\r\n\r\n" \
+		    "\032\032stopped.*$gdb_prompt$" } ] {
+	pass "run until main breakpoint"
     }
 }
 #exp_internal 0
-- 
2.14.5

Re: [PATCHv3 2/2] gdb: Update test pattern to deal with native-extended-gdbserver

[Pedro Alves]

On 12/12/2018 03:16 PM, Andrew Burgess wrote:
> When running the test gdb.base/annota1.exp with:
> 
>   make check-gdb RUNTESTFLAGS="--target_board=native-extended-gdbserver gdb.base/annota1.exp"
> 
> I would see a failure due to some unexpecte lines in GDB's output.

Typo: "unexpected"

LGTM.

Thanks,
Pedro Alves

Re: [PATCHv3 1/2] gdb/infcall: Make infcall_suspend_state more class like

[Pedro Alves]

Hi Andrew,

LGTM with the nits below addressed.

On 12/12/2018 03:16 PM, Andrew Burgess wrote:
> I ran into a situation where attempting to make an inferior function
> call would trigger an assertion, 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)
> 
> The problem that triggers the assertion is that in the function
> save_infcall_suspend_state, we basically did this:
> 
>     1. Create empty infcall_suspend_state object.
>     2. Fill fields of infcall_suspend_state object.
> 
> The problem is causes is that if filling any of the fields triggered
> an exception then the infcall_suspend_state object would be deleted
> while in a partially filled in state.
> 
> In the specific case I encountered, I had a remote RISC-V target that
> claimed in its target description to support floating point registers.
> However, this was not true, and when GDB tried to read a floating
> point register the remote sent back an error.  This error would cause
> an exception to be thrown while creating the
> readonly_detached_regcache, which in turn caused GDB to try and delete
> an infcall_suspend_state which didn't have any register state, and
> this triggered the assertion.
> 
> To prevent this problem we have two possibilities, either, rewrite the
> restore code the handle partially initialised infcall_suspend_state
> objects, or, prevent partially initialised infcall_suspend_state
> objects from existing.  The second of these seems like a better
> solution.
> 
> So, in this patch, I move the filling in of the different
> infcall_suspend_state fields within a new constructor for
> infcall_suspend_state.  Now, if generating one of those fields fails
> the destructor for infcall_suspend_state will not be executed and GDB
> will not try to restore the partially saved state.
> 
> With this patch in place GDB now behaves like this:
> 
>     (gdb) call some_inferior_function ()
>     Could not fetch register "ft0"; remote failure reply 'E99'
>     (gdb)
> 
> The inferior function call is aborted due to the error.
> 
> This has been tested against x86-64/Linux native, native-gdbserver,
> and native-extended-gdbserver with no regressions.  I've manually
> tested this against my baddly behaving target and confirmed the
> inferior function call is aborted as described above.
> 
> gdb/ChangeLog:
> 
> 	* infrun.c (infcall_suspend_state::infcall_suspend_state): New.
> 	(infcall_suspend_state::get_registers): New.
> 	(infcall_suspend_state::restore): New.
> 	(infcall_suspend_state::thread_suspend): Rename to...
> 	(infcall_suspend_state::m_thread_suspend): ...this.
> 	(infcall_suspend_state::registers): Rename to...
> 	(infcall_suspend_state::m_registers): ...this.
> 	(infcall_suspend_state::siginfo_gdbarch): Rename to...
> 	(infcall_suspend_state::m_siginfo_gdbarch): ...this.
> 	(infcall_suspend_state::siginfo_data): Rename to...
> 	(infcall_suspend_state::m_siginfo_data): ...this.
> 	(save_infcall_suspend_state): Rewrite to use infcall_suspend_state
> 	constructor.
> 	(restore_infcall_suspend_state): Rewrite to use
> 	infcall_suspend_state::restore method.
> 	(get_infcall_suspend_state_regcache): Use
> 	infcall_suspend_state::get_registers method.
> ---
>  gdb/ChangeLog |  20 +++++++++
>  gdb/infrun.c  | 132 +++++++++++++++++++++++++++++++++++-----------------------
>  2 files changed, 100 insertions(+), 52 deletions(-)
> 
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 46a8985f860..a9d7fa17aaf 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -8742,18 +8742,85 @@ siginfo_make_value (struct gdbarch *gdbarch, struct internalvar *var,
>  
>  struct infcall_suspend_state

Can you s/struct/class/ while at it?  Just for aesthetic reasons,
since all fields/methods are covered with public/private.
"struct" hints at a type that is mostly public data.

>  {
> -  struct thread_suspend_state thread_suspend;
> +public:
> +  /* Capture state from GDBARCH, TP, and REGCACHE that must be restored
> +     once the inferior function call has finished.  */
> +  infcall_suspend_state (struct gdbarch *gdbarch,
> +                         const struct thread_info *tp,
> +                         struct regcache *regcache)
> +    : m_thread_suspend (tp->suspend),
> +      m_registers (new readonly_detached_regcache (*regcache))
> +  {
> +    gdb::unique_xmalloc_ptr<gdb_byte> siginfo_data;
>  
> -  /* Other fields:  */
> -  std::unique_ptr<readonly_detached_regcache> registers;
> +    if (gdbarch_get_siginfo_type_p (gdbarch))
> +      {
> +        struct type *type = gdbarch_get_siginfo_type (gdbarch);
> +        size_t len = TYPE_LENGTH (type);
> +
> +        siginfo_data.reset ((gdb_byte *) xmalloc (len));
> +
> +        if (target_read (current_top_target (), TARGET_OBJECT_SIGNAL_INFO, NULL,
> +                         siginfo_data.get (), 0, len) != len)
> +          {
> +            /* Errors ignored.  */
> +            siginfo_data.reset (nullptr);
> +          }
> +      }
> +
> +    if (siginfo_data)
> +      {
> +        m_siginfo_gdbarch = gdbarch;
> +        m_siginfo_data = std::move (siginfo_data);
> +      }
> +  }
> +
> +  /* Return a pointer to the stored register state.  */
> +
> +  readonly_detached_regcache * get_registers () const

No space between '*' and get_registers.

A "get_" prefix is not customary in gdb (except in global functions),
IMHO it doesn't add much other than horizontal space.  :-)

  readonly_detached_regcache *registers () const

> +  {
> +    return m_registers.get ();
> +  }
> +
> +  /* Restores the stored state into GDBARCH, TP, and REGCACHE.  */
> +
> +  void restore (struct gdbarch *gdbarch,
> +                struct thread_info *tp,
> +                struct regcache *regcache) const
> +  {
> +    tp->suspend = m_thread_suspend;
> +
> +    if (m_siginfo_gdbarch == gdbarch)
> +      {
> +        struct type *type = gdbarch_get_siginfo_type (gdbarch);
> +
> +        /* Errors ignored.  */
> +        target_write (current_top_target (), TARGET_OBJECT_SIGNAL_INFO, NULL,
> +                      m_siginfo_data.get (), 0, TYPE_LENGTH (type));
> +      }
> +
> +    /* The inferior can be gone if the user types "print exit(0)"
> +       (and perhaps other times).  */
> +    if (target_has_execution)
> +      /* NB: The register write goes through to the target.  */
> +      regcache->restore (get_registers ());
> +  }
> +
> +private:
> +  /* How the current thread stopped before the inferior function call was
> +     executed.  */
> +  struct thread_suspend_state m_thread_suspend;
> +
> +  /* The registers before the inferior function call was executed.  */
> +  std::unique_ptr<readonly_detached_regcache> m_registers;
>  
>    /* Format of SIGINFO_DATA or NULL if it is not present.  */
> -  struct gdbarch *siginfo_gdbarch = nullptr;
> +  struct gdbarch *m_siginfo_gdbarch = nullptr;
>  
>    /* The inferior format depends on SIGINFO_GDBARCH and it has a length of
>       TYPE_LENGTH (gdbarch_get_siginfo_type ()).  For different gdbarch the
>       content would be invalid.  */
> -  gdb::unique_xmalloc_ptr<gdb_byte> siginfo_data;
> +  gdb::unique_xmalloc_ptr<gdb_byte> m_siginfo_data;
>  };
>  
>  infcall_suspend_state_up
> @@ -8762,39 +8829,16 @@ save_infcall_suspend_state ()
>    struct thread_info *tp = inferior_thread ();
>    struct regcache *regcache = get_current_regcache ();
>    struct gdbarch *gdbarch = regcache->arch ();
> -  gdb::unique_xmalloc_ptr<gdb_byte> siginfo_data;
> -
> -  if (gdbarch_get_siginfo_type_p (gdbarch))
> -    {
> -      struct type *type = gdbarch_get_siginfo_type (gdbarch);
> -      size_t len = TYPE_LENGTH (type);
> -
> -      siginfo_data.reset ((gdb_byte *) xmalloc (len));
> -
> -      if (target_read (current_top_target (), TARGET_OBJECT_SIGNAL_INFO, NULL,
> -		       siginfo_data.get (), 0, len) != len)
> -	{
> -	  /* Errors ignored.  */
> -	  siginfo_data.reset (nullptr);
> -	}
> -    }
> -
> -  infcall_suspend_state_up inf_state (new struct infcall_suspend_state);
>  
> -  if (siginfo_data)
> -    {
> -      inf_state->siginfo_gdbarch = gdbarch;
> -      inf_state->siginfo_data = std::move (siginfo_data);
> -    }
> -
> -  inf_state->thread_suspend = tp->suspend;
> +  infcall_suspend_state_up inf_state
> +    (new struct infcall_suspend_state (gdbarch, tp, regcache));
>  
> -  /* run_inferior_call will not use the signal due to its `proceed' call with
> -     GDB_SIGNAL_0 anyway.  */
> +  /* Having saved the current state, adjust the thread state, discarding
> +     any stop signal information, this is not useful when starting an
> +     inferior function call and run_inferior_call will not use the signal
> +     due to its `proceed' call with GDB_SIGNAL_0.  */

Long combined sentences like that are confusing to read, at least to me.
If think that if we spell out what the "this" in "this is not useful" is
referring to, and add a comma before 'and' since it's connecting two
independent clauses, it becomes clearer.  Something like:

/* Having saved the current state, adjust the thread state, discarding
   any stop signal information.  The stop signal is not useful when starting
   an inferior function call, and run_inferior_call will not use the signal
   due to its `proceed' call with GDB_SIGNAL_0.  */

>    tp->suspend.stop_signal = GDB_SIGNAL_0;
>  
> -  inf_state->registers.reset (new readonly_detached_regcache (*regcache));
> -
>    return inf_state;
>  }
>  
> @@ -8807,23 +8851,7 @@ restore_infcall_suspend_state (struct infcall_suspend_state *inf_state)
>    struct regcache *regcache = get_current_regcache ();
>    struct gdbarch *gdbarch = regcache->arch ();
>  
> -  tp->suspend = inf_state->thread_suspend;
> -
> -  if (inf_state->siginfo_gdbarch == gdbarch)
> -    {
> -      struct type *type = gdbarch_get_siginfo_type (gdbarch);
> -
> -      /* Errors ignored.  */
> -      target_write (current_top_target (), TARGET_OBJECT_SIGNAL_INFO, NULL,
> -		    inf_state->siginfo_data.get (), 0, TYPE_LENGTH (type));
> -    }
> -
> -  /* The inferior can be gone if the user types "print exit(0)"
> -     (and perhaps other times).  */
> -  if (target_has_execution)
> -    /* NB: The register write goes through to the target.  */
> -    regcache->restore (inf_state->registers.get ());
> -
> +  inf_state->restore (gdbarch, tp, regcache);
>    discard_infcall_suspend_state (inf_state);
>  }
>  
> @@ -8836,7 +8864,7 @@ discard_infcall_suspend_state (struct infcall_suspend_state *inf_state)
>  readonly_detached_regcache *
>  get_infcall_suspend_state_regcache (struct infcall_suspend_state *inf_state)
>  {
> -  return inf_state->registers.get ();
> +  return inf_state->get_registers ();
>  }
>  
>  /* infcall_control_state contains state regarding gdb's control of the
> 

Thanks,
Pedro Alves

Re: [PATCHv3 0/2] Handle Errors While Preparing For Inferior Call

[Pedro Alves]

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

Re: [PATCHv3 0/2] Handle Errors While Preparing For Inferior Call

[Andrew Burgess]

* 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