gdb/infcall: Make infcall_suspend_state into a class

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::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::registers method.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 687c5ae961e3c883fb8e7269071e9a627fec8c73..dc705dc968d15b370839984d24a0921cebd0765d 100644 (file)
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,23 @@
+2018-12-12  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+       * infrun.c (infcall_suspend_state::infcall_suspend_state): New.
+       (infcall_suspend_state::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::registers method.
+
 2018-12-12  Andrew Burgess  <andrew.burgess@embecosm.com>
 
        * riscv-tdep.c (riscv_call_arg_scalar_float): Unnamed (variadic)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 46a8985f860d0a7d47ca5197450112c70ed4a40d..e9336ab13031b60ead1b97324938c3d80cf04187 100644 (file)
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -8740,20 +8740,87 @@ siginfo_make_value (struct gdbarch *gdbarch, struct internalvar *var,
    ends (either successfully, or after it hits a breakpoint or signal)
    if the program is to properly continue where it left off.  */
 
-struct infcall_suspend_state
+class 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 *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 (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.  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->registers ();
 }
 
 /* infcall_control_state contains state regarding gdb's control of the