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

["Metzger, Markus T" <markus dot t dot metzger at intel dot com>]

> -----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