diff mbox

Unify ctf_fetch_registers and tfile_fetch_registers

Message ID 1397997905-394-1-git-send-email-yao@codesourcery.com
State Committed
Headers show

Commit Message

Yao Qi April 20, 2014, 12:45 p.m. UTC
Functions ctf_fetch_registers and tfile_fetch_registers have some
duplicated code about guessing the PC in regcache.  Sometimes, we
may change one function and forget to update the other one, like this
https://www.sourceware.org/ml/gdb-patches/2014-01/msg00292.html

This patch is to move the duplicated code into a new function
tracefile_fetch_registers, and let both ctf_fetch_registers and
tfile_fetch_registers call it.

Patch is regression tested on x86_64-linux.  Is it OK?

gdb:

2014-04-20  Yao Qi  <yao@codesourcery.com>

	* tracefile-tfile.c (tfile_fetch_registers): Move the bottom to ...
	* tracefile.c (tracefile_fetch_registers): ... it.  New function.
	* tracefile.h (tracefile_fetch_registers): Declare.
	* ctf.c (ctf_fetch_registers): Remove the bottom.  Call
	tracefile_fetch_registers.
---
 gdb/ctf.c             | 48 ++++------------------------------------
 gdb/tracefile-tfile.c | 58 +++++-------------------------------------------
 gdb/tracefile.c       | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/tracefile.h       |  2 ++
 4 files changed, 72 insertions(+), 97 deletions(-)

Comments

Joel Brobecker April 21, 2014, 4:09 p.m. UTC | #1
> 2014-04-20  Yao Qi  <yao@codesourcery.com>
> 
> 	* tracefile-tfile.c (tfile_fetch_registers): Move the bottom to ...
> 	* tracefile.c (tracefile_fetch_registers): ... it.  New function.
> 	* tracefile.h (tracefile_fetch_registers): Declare.
> 	* ctf.c (ctf_fetch_registers): Remove the bottom.  Call
> 	tracefile_fetch_registers.

Thanks, and OK.
Yao Qi April 22, 2014, 1:50 a.m. UTC | #2
On 04/22/2014 12:09 AM, Joel Brobecker wrote:
>> 2014-04-20  Yao Qi  <yao@codesourcery.com>
>>
>> 	* tracefile-tfile.c (tfile_fetch_registers): Move the bottom to ...
>> 	* tracefile.c (tracefile_fetch_registers): ... it.  New function.
>> 	* tracefile.h (tracefile_fetch_registers): Declare.
>> 	* ctf.c (ctf_fetch_registers): Remove the bottom.  Call
>> 	tracefile_fetch_registers.
> 
> Thanks, and OK.
> 

Thanks for the review.  Patch is pushed in.
diff mbox

Patch

diff --git a/gdb/ctf.c b/gdb/ctf.c
index 25d63c6..bac7c28 100644
--- a/gdb/ctf.c
+++ b/gdb/ctf.c
@@ -1229,8 +1229,6 @@  ctf_fetch_registers (struct target_ops *ops,
 		     struct regcache *regcache, int regno)
 {
   struct gdbarch *gdbarch = get_regcache_arch (regcache);
-  int offset, regn, regsize, pc_regno;
-  gdb_byte *regs = NULL;
   struct bt_ctf_event *event = NULL;
   struct bt_iter_pos *pos;
 
@@ -1270,13 +1268,14 @@  ctf_fetch_registers (struct target_ops *ops,
 
   if (event != NULL)
     {
+      int offset, regsize, regn;
       const struct bt_definition *scope
 	= bt_ctf_get_top_level_scope (event,
 				      BT_EVENT_FIELDS);
       const struct bt_definition *array
 	= bt_ctf_get_field (event, scope, "contents");
+      gdb_byte *regs = (gdb_byte *) bt_ctf_get_char_array (array);
 
-      regs = (gdb_byte *) bt_ctf_get_char_array (array);
       /* Assume the block is laid out in GDB register number order,
 	 each register with the size that it has in GDB.  */
       offset = 0;
@@ -1300,48 +1299,9 @@  ctf_fetch_registers (struct target_ops *ops,
 	    }
 	  offset += regsize;
 	}
-      return;
-    }
-
-  regs = alloca (trace_regblock_size);
-
-  /* We get here if no register data has been found.  Mark registers
-     as unavailable.  */
-  for (regn = 0; regn < gdbarch_num_regs (gdbarch); regn++)
-    regcache_raw_supply (regcache, regn, NULL);
-
-  /* We can often usefully guess that the PC is going to be the same
-     as the address of the tracepoint.  */
-  pc_regno = gdbarch_pc_regnum (gdbarch);
-  if (pc_regno >= 0 && (regno == -1 || regno == pc_regno))
-    {
-      struct tracepoint *tp = get_tracepoint (get_tracepoint_number ());
-
-      if (tp != NULL && tp->base.loc)
-	{
-	  /* But don't try to guess if tracepoint is multi-location...  */
-	  if (tp->base.loc->next != NULL)
-	    {
-	      warning (_("Tracepoint %d has multiple "
-			 "locations, cannot infer $pc"),
-		       tp->base.number);
-	      return;
-	    }
-	  /* ... or does while-stepping.  */
-	  if (tp->step_count > 0)
-	    {
-	      warning (_("Tracepoint %d does while-stepping, "
-			 "cannot infer $pc"),
-		       tp->base.number);
-	      return;
-	    }
-
-	  store_unsigned_integer (regs, register_size (gdbarch, pc_regno),
-				  gdbarch_byte_order (gdbarch),
-				  tp->base.loc->address);
-	  regcache_raw_supply (regcache, pc_regno, regs);
-	}
     }
+  else
+    tracefile_fetch_registers (regcache, regno);
 }
 
 /* This is the implementation of target_ops method to_xfer_partial.
diff --git a/gdb/tracefile-tfile.c b/gdb/tracefile-tfile.c
index a36596f..efa69b2 100644
--- a/gdb/tracefile-tfile.c
+++ b/gdb/tracefile-tfile.c
@@ -795,18 +795,17 @@  tfile_fetch_registers (struct target_ops *ops,
 		       struct regcache *regcache, int regno)
 {
   struct gdbarch *gdbarch = get_regcache_arch (regcache);
-  int offset, regn, regsize, pc_regno;
-  gdb_byte *regs;
+  int offset, regn, regsize;
 
   /* An uninitialized reg size says we're not going to be
      successful at getting register blocks.  */
   if (!trace_regblock_size)
     return;
 
-  regs = alloca (trace_regblock_size);
-
   if (traceframe_find_block_type ('R', 0) >= 0)
     {
+      gdb_byte *regs = alloca (trace_regblock_size);
+
       tfile_read (regs, trace_regblock_size);
 
       /* Assume the block is laid out in GDB register number order,
@@ -832,56 +831,9 @@  tfile_fetch_registers (struct target_ops *ops,
 	    }
 	  offset += regsize;
 	}
-      return;
-    }
-
-  /* We get here if no register data has been found.  Mark registers
-     as unavailable.  */
-  for (regn = 0; regn < gdbarch_num_regs (gdbarch); regn++)
-    regcache_raw_supply (regcache, regn, NULL);
-
-  /* We can often usefully guess that the PC is going to be the same
-     as the address of the tracepoint.  */
-  pc_regno = gdbarch_pc_regnum (gdbarch);
-
-  /* XXX This guessing code below only works if the PC register isn't
-     a pseudo-register.  The value of a pseudo-register isn't stored
-     in the (non-readonly) regcache -- instead it's recomputed
-     (probably from some other cached raw register) whenever the
-     register is read.  This guesswork should probably move to some
-     higher layer.  */
-  if (pc_regno < 0 || pc_regno >= gdbarch_num_regs (gdbarch))
-    return;
-
-  if (regno == -1 || regno == pc_regno)
-    {
-      struct tracepoint *tp = get_tracepoint (get_tracepoint_number ());
-
-      if (tp && tp->base.loc)
-	{
-	  /* But don't try to guess if tracepoint is multi-location...  */
-	  if (tp->base.loc->next)
-	    {
-	      warning (_("Tracepoint %d has multiple "
-			 "locations, cannot infer $pc"),
-		       tp->base.number);
-	      return;
-	    }
-	  /* ... or does while-stepping.  */
-	  if (tp->step_count > 0)
-	    {
-	      warning (_("Tracepoint %d does while-stepping, "
-			 "cannot infer $pc"),
-		       tp->base.number);
-	      return;
-	    }
-
-	  store_unsigned_integer (regs, register_size (gdbarch, pc_regno),
-				  gdbarch_byte_order (gdbarch),
-				  tp->base.loc->address);
-	  regcache_raw_supply (regcache, pc_regno, regs);
-	}
     }
+  else
+    tracefile_fetch_registers (regcache, regno);
 }
 
 static enum target_xfer_status
diff --git a/gdb/tracefile.c b/gdb/tracefile.c
index a7c3cf7..0b89cfa 100644
--- a/gdb/tracefile.c
+++ b/gdb/tracefile.c
@@ -21,6 +21,7 @@ 
 #include "tracefile.h"
 #include "ctf.h"
 #include "exec.h"
+#include "regcache.h"
 
 /* Helper macros.  */
 
@@ -377,6 +378,66 @@  trace_save_ctf (const char *dirname, int target_does_save)
   do_cleanups (back_to);
 }
 
+/* Fetch register data from tracefile, shared for both tfile and
+   ctf.  */
+
+void
+tracefile_fetch_registers (struct regcache *regcache, int regno)
+{
+  struct gdbarch *gdbarch = get_regcache_arch (regcache);
+  int regn, pc_regno;
+
+  /* We get here if no register data has been found.  Mark registers
+     as unavailable.  */
+  for (regn = 0; regn < gdbarch_num_regs (gdbarch); regn++)
+    regcache_raw_supply (regcache, regn, NULL);
+
+  /* We can often usefully guess that the PC is going to be the same
+     as the address of the tracepoint.  */
+  pc_regno = gdbarch_pc_regnum (gdbarch);
+
+  /* XXX This guessing code below only works if the PC register isn't
+     a pseudo-register.  The value of a pseudo-register isn't stored
+     in the (non-readonly) regcache -- instead it's recomputed
+     (probably from some other cached raw register) whenever the
+     register is read.  This guesswork should probably move to some
+     higher layer.  */
+  if (pc_regno < 0 || pc_regno >= gdbarch_num_regs (gdbarch))
+    return;
+
+  if (regno == -1 || regno == pc_regno)
+    {
+      struct tracepoint *tp = get_tracepoint (get_tracepoint_number ());
+      gdb_byte *regs;
+
+      if (tp && tp->base.loc)
+	{
+	  /* But don't try to guess if tracepoint is multi-location...  */
+	  if (tp->base.loc->next)
+	    {
+	      warning (_("Tracepoint %d has multiple "
+			 "locations, cannot infer $pc"),
+		       tp->base.number);
+	      return;
+	    }
+	  /* ... or does while-stepping.  */
+	  if (tp->step_count > 0)
+	    {
+	      warning (_("Tracepoint %d does while-stepping, "
+			 "cannot infer $pc"),
+		       tp->base.number);
+	      return;
+	    }
+
+	  regs = alloca (register_size (gdbarch, pc_regno));
+	  store_unsigned_integer (regs, register_size (gdbarch, pc_regno),
+				  gdbarch_byte_order (gdbarch),
+				  tp->base.loc->address);
+	  regcache_raw_supply (regcache, pc_regno, regs);
+	}
+    }
+}
+
 /* This is the implementation of target_ops method to_has_all_memory.  */
 
 static int
diff --git a/gdb/tracefile.h b/gdb/tracefile.h
index db454e3..5485370 100644
--- a/gdb/tracefile.h
+++ b/gdb/tracefile.h
@@ -113,4 +113,6 @@  extern struct trace_file_writer *tfile_trace_file_writer_new (void);
 
 extern void init_tracefile_ops (struct target_ops *ops);
 
+extern void tracefile_fetch_registers (struct regcache *regcache, int regno);
+
 #endif /* TRACEFILE_H */