Re: [PATCH] Fix "is a record target open" checks.

[Pedro Alves <palves@redhat.com>]

On 01/14/2014 03:30 PM, Tom Tromey wrote:

> Pedro> +/* See record-full.h.  */
> Pedro> +
> Pedro> +int
> Pedro> +record_full_is_used (void)
> Pedro> +{
> Pedro> +  struct target_ops *t;
> Pedro> +
> Pedro> +  t = find_record_target ();
> Pedro> +  return (t != NULL
> Pedro> +	  && (t->to_shortname == record_full_shortname
> Pedro> +	      || t->to_shortname == record_full_core_shortname));
> 
> Could this check against record_full_*ops directly rather than checking
> the shortname?  

Oh yes it could.  I don't really know why I didn't think
of that...  Adjusted.

> I'm curious about the rationale for this choice 

ENOCAFFEINE I guess.  :-)

> Pedro> +void
> Pedro> +record_preopen (void)
> Pedro> +{
> 
> Need a "/* See record.h.  */"

Fixed.

> 
> Pedro> +  /* Check if a record target is already running.  */
> Pedro> +  if (find_record_target ())
> 
> != NULL
> 

Fixed.

> Pedro> +/* Find the record_stratum target in the target stack.  */
> Pedro> +extern struct target_ops *find_record_target (void);
> 
> I think this should mention that it can return NULL.

Fixed.

Below's what I pushed after retesting.

Thanks.

----------
Subject: [PATCH] Fix "is a record target open" checks.

RECORD_IS_USED and record_full_open look at current_target.to_stratum
to determine whether a record target is in use.  This is wrong because
arch_stratum is greater than record_stratum, so if an arch_stratum
target is pushed, RECORD_IS_USED and record_full_open will miss it.

To fix this, we can use the existing find_record_target instead, which
looks up for a record stratum target across the target stack.  Since
that means exporting find_record_target in record.h, RECORD_IS_USED
ends up redundant, so the patch eliminates it.

That exercise then reveals other issues:

- adjust_pc_after_break is gating record_full_... calls based on
RECORD_IS_USED.  But, record_full_ calls shouldn't be made when
recording with the record-btrace target.  So this adds a new
record_full_is_used predicate to be used in that spot.

- record_full_open says "Process record target already running", even
if the recording target is record-btrace ("process record" is the
original complete name of the record-full target).  record_btrace_open
only says "The process is already being recorded." and does not
suggest "record stop", like record-full does.  The patch factors out
and merges that error to a new record_preopen function that all record
targets call in their open routine.

Tested on x86_64 Fedora 17.

gdb/
2014-01-14  Pedro Alves  <palves@redhat.com>
	    Tom Tromey  <tromey@redhat.com>

	* infrun.c (use_displaced_stepping): Use find_record_target
	instead of RECORD_IS_USED.
	(adjust_pc_after_break): Use record_full_is_used instead of
	RECORD_IS_USED.
	* record-btrace.c (record_btrace_open): Call record_preopen
	instead of checking RECORD_IS_USED.
	* record-full.c (record_full_shortname)
	(record_full_core_shortname): New globals.
	(record_full_is_used): New function.
	(find_full_open): Call record_preopen instead of checking
	RECORD_IS_USED.
	(init_record_full_ops): Set the target's shortname to
	record_full_shortname.
	(init_record_full_core_ops): Set the target's shortname to
	record_full_core_shortname.
	* record-full.h (record_full_is_used): Declare.
	* record.c (find_record_target): Make extern.
	(record_preopen): New function.
	* record.h (RECORD_IS_USED): Delete macro.
	(find_record_target, record_preopen): Declare functions.
---
 gdb/ChangeLog       | 24 ++++++++++++++++++++++++
 gdb/infrun.c        |  4 ++--
 gdb/record-btrace.c |  3 +--
 gdb/record-full.c   | 18 ++++++++++++++----
 gdb/record-full.h   |  4 ++++
 gdb/record.c        | 15 +++++++++++++--
 gdb/record.h        | 10 ++++++++--
 7 files changed, 66 insertions(+), 12 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 40e703f..3daf77f 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,27 @@
+2014-01-14  Pedro Alves  <palves@redhat.com>
+	    Tom Tromey  <tromey@redhat.com>
+
+	* infrun.c (use_displaced_stepping): Use find_record_target
+	instead of RECORD_IS_USED.
+	(adjust_pc_after_break): Use record_full_is_used instead of
+	RECORD_IS_USED.
+	* record-btrace.c (record_btrace_open): Call record_preopen
+	instead of checking RECORD_IS_USED.
+	* record-full.c (record_full_shortname)
+	(record_full_core_shortname): New globals.
+	(record_full_is_used): New function.
+	(find_full_open): Call record_preopen instead of checking
+	RECORD_IS_USED.
+	(init_record_full_ops): Set the target's shortname to
+	record_full_shortname.
+	(init_record_full_core_ops): Set the target's shortname to
+	record_full_core_shortname.
+	* record-full.h (record_full_is_used): Declare.
+	* record.c (find_record_target): Make extern.
+	(record_preopen): New function.
+	* record.h (RECORD_IS_USED): Delete macro.
+	(find_record_target, record_preopen): Declare functions.
+
 2014-01-14  Yao Qi  <yao@codesourcery.com>
 
 	* gdbarch.sh (core_xfer_shared_libraries): Change its argument
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 73038a3..311bf9c 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1240,7 +1240,7 @@ use_displaced_stepping (struct gdbarch *gdbarch)
   return (((can_use_displaced_stepping == AUTO_BOOLEAN_AUTO && non_stop)
 	   || can_use_displaced_stepping == AUTO_BOOLEAN_TRUE)
 	  && gdbarch_displaced_step_copy_insn_p (gdbarch)
-	  && !RECORD_IS_USED);
+	  && find_record_target () == NULL);
 }
 
 /* Clean out any stray displaced stepping state.  */
@@ -3048,7 +3048,7 @@ adjust_pc_after_break (struct execution_control_state *ecs)
     {
       struct cleanup *old_cleanups = make_cleanup (null_cleanup, NULL);
 
-      if (RECORD_IS_USED)
+      if (record_full_is_used ())
 	record_full_gdb_operation_disable_set ();
 
       /* When using hardware single-step, a SIGTRAP is reported for both
diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index 5fd26e2..c3330e9 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -142,8 +142,7 @@ record_btrace_open (char *args, int from_tty)
 
   DEBUG ("open");
 
-  if (RECORD_IS_USED)
-    error (_("The process is already being recorded."));
+  record_preopen ();
 
   if (!target_has_execution)
     error (_("The program is not being run."));
diff --git a/gdb/record-full.c b/gdb/record-full.c
index a9fc018..a93418c 100644
--- a/gdb/record-full.c
+++ b/gdb/record-full.c
@@ -208,6 +208,19 @@ static ULONGEST record_full_insn_count;
 static struct target_ops record_full_ops;
 static struct target_ops record_full_core_ops;
 
+/* See record-full.h.  */
+
+int
+record_full_is_used (void)
+{
+  struct target_ops *t;
+
+  t = find_record_target ();
+  return (t == &record_full_ops
+	  || t == &record_full_core_ops);
+}
+
+
 /* Command lists for "set/show record full".  */
 static struct cmd_list_element *set_record_full_cmdlist;
 static struct cmd_list_element *show_record_full_cmdlist;
@@ -907,10 +920,7 @@ record_full_open (char *name, int from_tty)
   if (record_debug)
     fprintf_unfiltered (gdb_stdlog, "Process record: record_full_open\n");
 
-  /* Check if record target is already running.  */
-  if (current_target.to_stratum == record_stratum)
-    error (_("Process record target already running.  Use \"record stop\" to "
-             "stop record target first."));
+  record_preopen ();
 
   /* Reset the tmp beneath pointers.  */
   tmp_to_resume_ops = NULL;
diff --git a/gdb/record-full.h b/gdb/record-full.h
index 517d786..ef3b387 100644
--- a/gdb/record-full.h
+++ b/gdb/record-full.h
@@ -25,6 +25,10 @@ extern int record_full_memory_query;
 extern int record_full_arch_list_add_reg (struct regcache *regcache, int num);
 extern int record_full_arch_list_add_mem (CORE_ADDR addr, int len);
 extern int record_full_arch_list_add_end (void);
+
+/* Returns true if the process record target is open.  */
+extern int record_full_is_used (void);
+
 extern struct cleanup *record_full_gdb_operation_disable_set (void);
 
 #endif /* RECORD_FULL_H */
diff --git a/gdb/record.c b/gdb/record.c
index e0df6b1..f2cfcc8 100644
--- a/gdb/record.c
+++ b/gdb/record.c
@@ -57,9 +57,9 @@ struct cmd_list_element *info_record_cmdlist = NULL;
   if (record_debug)							\
     fprintf_unfiltered (gdb_stdlog, "record: " msg "\n", ##args)
 
-/* Find the record target in the target stack.  */
+/* See record.h.  */
 
-static struct target_ops *
+struct target_ops *
 find_record_target (void)
 {
   struct target_ops *t;
@@ -88,6 +88,17 @@ require_record_target (void)
 
 /* See record.h.  */
 
+void
+record_preopen (void)
+{
+  /* Check if a record target is already running.  */
+  if (find_record_target () != NULL)
+    error (_("The process is already being recorded.  Use \"record stop\" to "
+	     "stop recording first."));
+}
+
+/* See record.h.  */
+
 int
 record_read_memory (struct gdbarch *gdbarch,
 		    CORE_ADDR memaddr, gdb_byte *myaddr,
diff --git a/gdb/record.h b/gdb/record.h
index ab5ea4b..962e382 100644
--- a/gdb/record.h
+++ b/gdb/record.h
@@ -22,8 +22,6 @@
 
 struct cmd_list_element;
 
-#define RECORD_IS_USED	(current_target.to_stratum == record_stratum)
-
 extern unsigned int record_debug;
 
 /* Allow record targets to add their own sub-commands.  */
@@ -63,4 +61,12 @@ extern void record_mourn_inferior (struct target_ops *);
 /* The default "to_kill" target method for record targets.  */
 extern void record_kill (struct target_ops *);
 
+/* Find the record_stratum target in the current target stack.
+   Returns NULL if none is found.  */
+extern struct target_ops *find_record_target (void);
+
+/* This is to be called by record_stratum targets' open routine before
+   it does anything.  */
+extern void record_preopen (void);
+
 #endif /* _RECORD_H_ */
-- 
1.7.11.7