[PATCH] btrace: continue recording after connection loss.

[PATCH] btrace: continue recording after connection loss.

[Tim Wiederhake]

If the connection between gdb and gdbserver drops unexpectedly, gdb
gets confused about whether a record target is active or not.

Current behavior (after connection loss and reconnect):
  (gdb) record btrace
  [record-btrace] open
  [record-btrace] open
  [btrace] enable thread 1 (Thread 23143)
  Sending packet: $Qbtrace-conf:bts:size=0x10000#e3...Packet received: OK
  Sending packet: $Qbtrace:bts#45...Packet received: E.Btrace already enabled.
  Could not enable branch tracing for Thread 23143: Btrace already enabled.
  (gdb) info record
  No record target is currently active.
  (gdb)

With this patch, gdb checks for running records on connect, pushes the record
target if necessary and allows to resume the recording.

2016-06-09  Tim Wiederhake  <tim.wiederhake@intel.com>

gdb/
	* record-btrace.c (record_btrace_open): Split into this and ...
	(record_btrace_push_target): ... this.
	* record-btrace.h: New file.
	* remote.c (remote_start_remote): Check recording status on connection
	and resync with running recording.
	(remote_btrace_read_config): Does not throw any more.
	(remote_btrace_maybe_reopen): New. Check for resumable recordings and
	resume if possible.
	(remote_enable_btrace): Account for remote_btrace_read_config not
	throwing anymore.

gdb/testsuite/

	* gdb.btrace/reconnect.c: New file.
	* gdb.btrace/reconnect.exp: New file.
---
 gdb/record-btrace.c                    |  29 ++++++---
 gdb/record-btrace.h                    |  28 +++++++++
 gdb/remote.c                           | 109 ++++++++++++++++++++++++---------
 gdb/testsuite/gdb.btrace/reconnect.c   |  25 ++++++++
 gdb/testsuite/gdb.btrace/reconnect.exp |  63 +++++++++++++++++++
 5 files changed, 214 insertions(+), 40 deletions(-)
 create mode 100644 gdb/record-btrace.h
 create mode 100644 gdb/testsuite/gdb.btrace/reconnect.c
 create mode 100644 gdb/testsuite/gdb.btrace/reconnect.exp

diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index 77b5180..4331709 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -21,6 +21,7 @@
 
 #include "defs.h"
 #include "record.h"
+#include "record-btrace.h"
 #include "gdbthread.h"
 #include "target.h"
 #include "gdbcmd.h"
@@ -199,6 +200,23 @@ record_btrace_handle_async_inferior_event (gdb_client_data data)
   inferior_event_handler (INF_REG_EVENT, NULL);
 }
 
+/* See record-btrace.h.  */
+
+void
+record_btrace_push_target (void)
+{
+  record_btrace_auto_enable ();
+
+  push_target (&record_btrace_ops);
+
+  record_btrace_async_inferior_event_handler
+    = create_async_event_handler (record_btrace_handle_async_inferior_event,
+				  NULL);
+  record_btrace_generating_corefile = 0;
+
+  observer_notify_record_changed (current_inferior (),  1);
+}
+
 /* The to_open method of target record-btrace.  */
 
 static void
@@ -225,16 +243,7 @@ record_btrace_open (const char *args, int from_tty)
 	make_cleanup (record_btrace_disable_callback, tp);
       }
 
-  record_btrace_auto_enable ();
-
-  push_target (&record_btrace_ops);
-
-  record_btrace_async_inferior_event_handler
-    = create_async_event_handler (record_btrace_handle_async_inferior_event,
-				  NULL);
-  record_btrace_generating_corefile = 0;
-
-  observer_notify_record_changed (current_inferior (),  1);
+  record_btrace_push_target ();
 
   discard_cleanups (disable_chain);
 }
diff --git a/gdb/record-btrace.h b/gdb/record-btrace.h
new file mode 100644
index 0000000..d2062b8
--- /dev/null
+++ b/gdb/record-btrace.h
@@ -0,0 +1,28 @@
+/* Branch trace support for GDB, the GNU debugger.
+
+   Copyright (C) 2016 Free Software Foundation, Inc.
+
+   Contributed by Intel Corp. <tim.wiederhake@intel.com>
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifndef RECORD_BTRACE_H
+#define RECORD_BTRACE_H
+
+/* Push the record_btrace target.  */
+extern void record_btrace_push_target (void);
+
+#endif /* RECORD_BTRACE_H */
diff --git a/gdb/remote.c b/gdb/remote.c
index 1f0d67c..0bb69c8 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -70,6 +70,7 @@
 #include "ax-gdb.h"
 #include "agent.h"
 #include "btrace.h"
+#include "record-btrace.h"
 
 /* Temp hacks for tracepoint encoding migration.  */
 static char *target_buf;
@@ -231,6 +232,8 @@ static int remote_can_run_breakpoint_commands (struct target_ops *self);
 
 static void remote_btrace_reset (void);
 
+static void remote_btrace_maybe_reopen (void);
+
 static int stop_reply_queue_length (void);
 
 static void readahead_cache_invalidate (void);
@@ -4298,6 +4301,10 @@ remote_start_remote (int from_tty, struct target_ops *target, int extended_p)
       merge_uploaded_tracepoints (&uploaded_tps);
     }
 
+  /* Possibly the target has been engaged in a btrace record started
+     previously; find out where things are at.  */
+  remote_btrace_maybe_reopen ();
+
   /* The thread and inferior lists are now synchronized with the
      target, our symbols have been relocated, and we're merged the
      target's tracepoints with ours.  We're done with basic start
@@ -12667,6 +12674,77 @@ remote_btrace_reset (void)
   memset (&rs->btrace_config, 0, sizeof (rs->btrace_config));
 }
 
+/* Read the current thread's btrace configuration from the target and
+   store it into CONF.  Returns -1 on failure.  */
+
+static int
+remote_btrace_read_config (struct btrace_config *conf)
+{
+  int ret = 0;
+  char *xml = target_read_stralloc (&current_target,
+				    TARGET_OBJECT_BTRACE_CONF, "");
+
+  if (xml == NULL)
+    return -1;
+
+  TRY
+    {
+      parse_xml_btrace_conf (conf, xml);
+    }
+  CATCH (err, RETURN_MASK_ERROR)
+    {
+      if (err.message != NULL)
+	warning ("%s", err.message);
+      ret = -1;
+    }
+  END_CATCH
+
+  xfree (xml);
+  return ret;
+}
+
+/* Maybe reopen target btrace.  */
+
+static void
+remote_btrace_maybe_reopen (void)
+{
+  struct remote_state *rs = get_remote_state ();
+  struct thread_info *tp;
+  int btrace_opened = 0;
+  int warned = 0;
+
+  ALL_NON_EXITED_THREADS (tp)
+    {
+      set_general_thread (tp->ptid);
+
+      if (remote_btrace_read_config (&rs->btrace_config) != -1)
+	{
+	  #if !defined (HAVE_LIBIPT)
+	    if (rs->btrace_config.format == BTRACE_FORMAT_PT)
+	      {
+		if (!warned)
+		  {
+		    warning (_("GDB does not support Intel Processor Trace."));
+		    warned = 1;
+		  }
+
+		continue;
+	      }
+
+	  #endif /* !defined (HAVE_LIBIPT) */
+
+	  btrace_opened = 1;
+
+	  tp->btrace.target = XCNEW (struct btrace_target_info);
+	  tp->btrace.target->ptid = tp->ptid;
+	  tp->btrace.target->conf = rs->btrace_config;
+	}
+    }
+
+  if (btrace_opened)
+    record_btrace_push_target ();
+}
+
 /* Check whether the target supports branch tracing.  */
 
 static int
@@ -12756,26 +12834,6 @@ btrace_sync_conf (const struct btrace_config *conf)
     }
 }
 
-/* Read the current thread's btrace configuration from the target and
-   store it into CONF.  */
-
-static void
-btrace_read_config (struct btrace_config *conf)
-{
-  char *xml;
-
-  xml = target_read_stralloc (&current_target,
-			      TARGET_OBJECT_BTRACE_CONF, "");
-  if (xml != NULL)
-    {
-      struct cleanup *cleanup;
-
-      cleanup = make_cleanup (xfree, xml);
-      parse_xml_btrace_conf (conf, xml);
-      do_cleanups (cleanup);
-    }
-}
-
 /* Enable branch tracing.  */
 
 static struct btrace_target_info *
@@ -12825,16 +12883,7 @@ remote_enable_btrace (struct target_ops *self, ptid_t ptid,
 
   /* If we fail to read the configuration, we lose some information, but the
      tracing itself is not impacted.  */
-  TRY
-    {
-      btrace_read_config (&tinfo->conf);
-    }
-  CATCH (err, RETURN_MASK_ERROR)
-    {
-      if (err.message != NULL)
-	warning ("%s", err.message);
-    }
-  END_CATCH
+  remote_btrace_read_config (&tinfo->conf);
 
   return tinfo;
 }
diff --git a/gdb/testsuite/gdb.btrace/reconnect.c b/gdb/testsuite/gdb.btrace/reconnect.c
new file mode 100644
index 0000000..f2a9d35
--- /dev/null
+++ b/gdb/testsuite/gdb.btrace/reconnect.c
@@ -0,0 +1,25 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2016 Free Software Foundation, Inc.
+
+   Contributed by Intel Corp. <tim.wiederhake@intel.com>
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+
+int
+main (void)
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.btrace/reconnect.exp b/gdb/testsuite/gdb.btrace/reconnect.exp
new file mode 100644
index 0000000..7e321ae
--- /dev/null
+++ b/gdb/testsuite/gdb.btrace/reconnect.exp
@@ -0,0 +1,63 @@
+# This testcase is part of GDB, the GNU debugger.
+#
+# Copyright 2016 Free Software Foundation, Inc.
+#
+# Contributed by Intel Corp. <tim.wiederhake@intel.com>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+load_lib gdbserver-support.exp
+
+if { [skip_btrace_tests] } { return -1 }
+if { [skip_gdbserver_tests] } { return -1 }
+
+standard_testfile
+if [prepare_for_testing $testfile.exp $testfile $srcfile] {
+    return -1
+}
+
+# Start gdbserver.
+set gdbserver_reconnect_p 1
+set res [gdbserver_start "" $binfile]
+set gdbserver_protocol [lindex $res 0]
+set gdbserver_gdbport [lindex $res 1]
+
+# Begin btrace.
+gdb_start
+gdb_target_cmd $gdbserver_protocol $gdbserver_gdbport
+gdb_test_no_output "record btrace" "record btrace enable"
+
+set bp_location [gdb_get_line_number "return" $testfile.c]
+gdb_breakpoint $bp_location
+gdb_continue_to_breakpoint "cont to $bp_location" ".*$testfile.c:$bp_location.*"
+
+# Simulate connection loss by killing gdb.
+set gdb_pid [exp_pid -i [board_info host fileid]]
+exec kill "-KILL" ${gdb_pid}
+
+# Cleanup and restart gdb.
+gdb_exit
+gdb_start
+gdb_target_cmd $gdbserver_protocol $gdbserver_gdbport
+
+# Enabling btrace should not be possible here since gdbserver is
+# already recording.  Check for the correct error message.
+gdb_test "record btrace" "The process is already being recorded\\.  Use \"record stop\" to stop recording first\\." "record btrace re-enable"
+
+# Test if we can access the recorded data from before the connection
+# loss.
+gdb_test "info record" [multi_line \
+  "Active record target: .*" \
+  "Recorded $decimal instructions in .$decimal functions \\($decimal gaps\\) for thread 1 \\(Thread .*\\)."
+]
-- 
2.7.4

RE: [PATCH] btrace: continue recording after connection loss.

[Metzger, Markus T]

> -----Original Message-----
> From: Wiederhake, Tim
> Sent: Thursday, June 9, 2016 11:32 AM
> To: gdb-patches@sourceware.org
> Cc: Metzger, Markus T <markus.t.metzger@intel.com>
> Subject: [PATCH] btrace: continue recording after connection loss.

Thanks for the patch, Tim,

> If the connection between gdb and gdbserver drops unexpectedly, gdb
> gets confused about whether a record target is active or not.
>
> Current behavior (after connection loss and reconnect):
>   (gdb) record btrace
>   [record-btrace] open
>   [record-btrace] open
>   [btrace] enable thread 1 (Thread 23143)
>   Sending packet: $Qbtrace-conf:bts:size=0x10000#e3...Packet received: OK
>   Sending packet: $Qbtrace:bts#45...Packet received: E.Btrace already enabled.
>   Could not enable branch tracing for Thread 23143: Btrace already enabled.
>   (gdb) info record
>   No record target is currently active.
>   (gdb)
> 
> With this patch, gdb checks for running records on connect, pushes the record
> target if necessary and allows to resume the recording.

For the record, this goes back to an issue pointed out by Pedro:
https://sourceware.org/ml/gdb-patches/2015-11/msg00420.html.


 
> 2016-06-09  Tim Wiederhake  <tim.wiederhake@intel.com>
> 
> gdb/
> 	* record-btrace.c (record_btrace_open): Split into this and ...
> 	(record_btrace_push_target): ... this.
> 	* record-btrace.h: New file.
> 	* remote.c (remote_start_remote): Check recording status on
> connection
> 	and resync with running recording.
> 	(remote_btrace_read_config): Does not throw any more.
> 	(remote_btrace_maybe_reopen): New. Check for resumable recordings
> and
> 	resume if possible.
> 	(remote_enable_btrace): Account for remote_btrace_read_config not
> 	throwing anymore.
> 
> gdb/testsuite/
> 
> 	* gdb.btrace/reconnect.c: New file.
> 	* gdb.btrace/reconnect.exp: New file.


> diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
> index 77b5180..4331709 100644
> --- a/gdb/record-btrace.c
> +++ b/gdb/record-btrace.c
> @@ -21,6 +21,7 @@
> 
>  #include "defs.h"
>  #include "record.h"
> +#include "record-btrace.h"

New include not listed in changelog.


> diff --git a/gdb/remote.c b/gdb/remote.c
> index 1f0d67c..0bb69c8 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -70,6 +70,7 @@
>  #include "ax-gdb.h"
>  #include "agent.h"
>  #include "btrace.h"
> +#include "record-btrace.h"

New include not listed in changelog.


> +/* Maybe reopen target btrace.  */
> +
> +static void
> +remote_btrace_maybe_reopen (void)
> +{
> +  struct remote_state *rs = get_remote_state ();
> +  struct thread_info *tp;
> +  int btrace_opened = 0;
> +  int warned = 0;
> +
> +  ALL_NON_EXITED_THREADS (tp)
> +    {
> +      set_general_thread (tp->ptid);

This changes the selected thread as a side-effect.  May need to call
make_cleanup_restore_current_thread before the loop and do the
cleanups afterwards to switch back to the originally selected thread.


The patch looks good to me otherwise.

thanks,
markus.

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH] btrace: continue recording after connection loss.

[Pedro Alves]

On 06/23/2016 02:40 PM, Metzger, Markus T wrote:
>> From: Wiederhake, Tim

>> With this patch, gdb checks for running records on connect, pushes the record
>> target if necessary and allows to resume the recording.
> 
> For the record, this goes back to an issue pointed out by Pedro:
> https://sourceware.org/ml/gdb-patches/2015-11/msg00420.html.
> 

Ah, thanks for fixing it!

/me runs a pair of extra eyes over the patch.

>
> +/* Read the current thread's btrace configuration from the target and
> +   store it into CONF.  Returns -1 on failure.  */
> +
> +static int
> +remote_btrace_read_config (struct btrace_config *conf)
> +{
> +  int ret = 0;
> +  char *xml = target_read_stralloc (&current_target,
> +				    TARGET_OBJECT_BTRACE_CONF, "");
> +
> +  if (xml == NULL)
> +    return -1;
> +
> +  TRY
> +    {
> +      parse_xml_btrace_conf (conf, xml);
> +    }
> +  CATCH (err, RETURN_MASK_ERROR)

Since this doesn't catch QUITs (Ctrl-C) ...

> +    {
> +      if (err.message != NULL)
> +	warning ("%s", err.message);

And also this warning can throw too (pagination -> Ctrl-C).

> +      ret = -1;
> +    }
> +  END_CATCH
> +
> +  xfree (xml);

... this xfree isn't guaranteed to be reached.  You need to
use a cleanup to xfree xml instead.

> +  return ret;
> +}





> +      if (remote_btrace_read_config (&rs->btrace_config) != -1)
> +	{
> +	  #if !defined (HAVE_LIBIPT)

We don't use this #if indentation style in gdb.  Please put the #if
at  column 0, and then indent the C if as if no #if was there.

> +	    if (rs->btrace_config.format == BTRACE_FORMAT_PT)






> +# Simulate connection loss by killing gdb.
> +set gdb_pid [exp_pid -i [board_info host fileid]]
> +exec kill "-KILL" ${gdb_pid}

This kills a random process in the build machine, when
remote-host testing.  You need do use "remote_exec host"
instead of bare "exec".

Since "disconnect" is supposed to leave target as it was,
and now that we can recover the btrace state, I wonder about
making "disconnect" not stop the trace.  (And with that, you
could test with the "disconnect" command, instead of
force-killing gdb.)

Thanks,
Pedro Alves