[binutils-gdb] gdb/infcall: Make infcall_suspend_state into a class

[Andrew Burgess <aburgess at sourceware dot org>]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=6bf78e29a1dea7121f050c6ee4990298eec679dd

commit 6bf78e29a1dea7121f050c6ee4990298eec679dd
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Mon Nov 26 12:48:05 2018 +0000

    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:
---
 gdb/ChangeLog |  20 +++++++++
 gdb/infrun.c  | 134 +++++++++++++++++++++++++++++++++++-----------------------
 2 files changed, 101 insertions(+), 53 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 687c5ae..dc705dc 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,25 @@
 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)
 	arguments are passed in integer registers.
 	(riscv_call_arg_complex_float): Likewise.
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 46a8985..e9336ab 100644
--- 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