[PATCH v2 0/2] thread, btrace: allow "record btrace" for running threads

[PATCH v2 0/2] thread, btrace: allow "record btrace" for running threads

[Markus Metzger]

This refers to:  https://sourceware.org/ml/gdb-patches/2016-11/msg00994.html.

The first version tried to detect the error in gdbserver, propagate it back, and
handle it in btrace.

This version checks whether registers can be accessed before making the request
to gdbserver based on feedback from Pedro.

Markus Metzger (2):
  thread: add can_access_registers_ptid
  btrace: allow recording to be started for running threads

 gdb/btrace.c                                | 44 ++++++++++++++++--
 gdb/gdbthread.h                             |  4 ++
 gdb/testsuite/gdb.btrace/enable-running.c   | 47 +++++++++++++++++++
 gdb/testsuite/gdb.btrace/enable-running.exp | 72 +++++++++++++++++++++++++++++
 gdb/thread.c                                | 20 ++++++++
 5 files changed, 183 insertions(+), 4 deletions(-)
 create mode 100644 gdb/testsuite/gdb.btrace/enable-running.c
 create mode 100644 gdb/testsuite/gdb.btrace/enable-running.exp

-- 
1.8.3.1

[PATCH v2 1/2] thread: add can_access_registers_ptid

[Markus Metzger]

Add a function can_access_registers_ptid that behaves like
validate_registers_access but returns a boolean value instead of throwing an
exception.

2017-01-20  Markus Metzger  <markus.t.metzger@intel.com>

	* gdbthread.h (can_access_registers_ptid): New.
	* thread.c (can_access_registers_ptid): New.
---
 gdb/gdbthread.h |  4 ++++
 gdb/thread.c    | 20 ++++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index 455cfd8..041b7c2 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -625,6 +625,10 @@ extern void thread_cancel_execution_command (struct thread_info *thr);
    executing).  */
 extern void validate_registers_access (void);
 
+/* Check whether it makes sense to access a register of PTID at this point.
+   Returns non-zero if registers may be accessed; zero otherwise.  */
+extern int can_access_registers_ptid (ptid_t ptid);
+
 /* Returns whether to show which thread hit the breakpoint, received a
    signal, etc. and ended up causing a user-visible stop.  This is
    true iff we ever detected multiple threads.  */
diff --git a/gdb/thread.c b/gdb/thread.c
index e45b257..d5dfe80 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -1141,6 +1141,26 @@ validate_registers_access (void)
     error (_("Selected thread is running."));
 }
 
+/* See gdbthread.h.  */
+
+int
+can_access_registers_ptid (ptid_t ptid)
+{
+  /* No thread, no registers.  */
+  if (ptid_equal (ptid, null_ptid))
+    return 0;
+
+  /* Don't try to read from a dead thread.  */
+  if (is_exited (ptid))
+    return 0;
+
+  /* ... or from a spinning thread.  FIXME: see validate_registers_access.  */
+  if (is_executing (ptid))
+    return 0;
+
+  return 1;
+}
+
 int
 pc_in_thread_step_range (CORE_ADDR pc, struct thread_info *thread)
 {
-- 
1.8.3.1

[PATCH v2 2/2] btrace: allow recording to be started for running threads

[Markus Metzger]

When recording is started for a running thread, GDB was able to start tracing
but then failed to read registers to insert the initial entry for the current
PC.  We don't really need that initial entry if we don't know where exactly we
started recording.  Skip that step to allow recording to be started while
threads are running.

If we do run into errors, we need to undo the tracing enable to not leak this
thread.  The operation did not complete so our caller won't clean up this
thread.

For the BTRACE_FORMAT_PT btrace format, we don't need that initial entry since
it will be recorded in the trace.  We can omit the call to btrace_add_pc.

CC:  Simon Marchi  <simon.marchi@ericsson.com>

2017-01-20  Markus Metzger  <markus.t.metzger@intel.com>

gdb/
	* btrace.c (btrace_enable): Do not call btrace_add_pc for
	BTRACE_FORMAT_PT or if can_access_registers_ptid returns false.
	(validate_registers_access_ptid): New.
	(btrace_add_pc): Call validate_registers_access_ptid.

testsuite/
	* gdb.btrace/enable-running.c: New.
	* gdb.btrace/enable-running.exp: New.
---
 gdb/btrace.c                                | 44 ++++++++++++++++--
 gdb/testsuite/gdb.btrace/enable-running.c   | 47 +++++++++++++++++++
 gdb/testsuite/gdb.btrace/enable-running.exp | 72 +++++++++++++++++++++++++++++
 3 files changed, 159 insertions(+), 4 deletions(-)
 create mode 100644 gdb/testsuite/gdb.btrace/enable-running.c
 create mode 100644 gdb/testsuite/gdb.btrace/enable-running.exp

diff --git a/gdb/btrace.c b/gdb/btrace.c
index d266af7..4380e6d 100644
--- a/gdb/btrace.c
+++ b/gdb/btrace.c
@@ -1422,6 +1422,18 @@ btrace_compute_ftrace (struct thread_info *tp, struct btrace_data *btrace)
   do_cleanups (old_chain);
 }
 
+/* Validate that we can read PTID's registers.  */
+
+static void
+validate_registers_access_ptid (ptid_t ptid)
+{
+  struct cleanup *cleanup = save_inferior_ptid ();
+
+  inferior_ptid = ptid;
+  validate_registers_access ();
+  do_cleanups (cleanup);
+}
+
 /* Add an entry for the current PC.  */
 
 static void
@@ -1433,6 +1445,9 @@ btrace_add_pc (struct thread_info *tp)
   struct cleanup *cleanup;
   CORE_ADDR pc;
 
+  /* Make sure we can read TP's registers.  */
+  validate_registers_access_ptid (tp->ptid);
+
   regcache = get_thread_regcache (tp->ptid);
   pc = regcache_read_pc (regcache);
 
@@ -1472,10 +1487,31 @@ btrace_enable (struct thread_info *tp, const struct btrace_config *conf)
 
   tp->btrace.target = target_enable_btrace (tp->ptid, conf);
 
-  /* Add an entry for the current PC so we start tracing from where we
-     enabled it.  */
-  if (tp->btrace.target != NULL)
-    btrace_add_pc (tp);
+  /* We're done if we failed to enable tracing.  */
+  if (tp->btrace.target == NULL)
+    return;
+
+  /* We need to undo the enable in case of errors.  */
+  TRY
+    {
+      /* Add an entry for the current PC so we start tracing from where we
+	 enabled it.
+
+	 If we can't access TP's registers, TP is most likely running.  In this
+	 case, we can't really say where tracing was enabled so it should be
+	 safe to simply skip this step.
+
+	 This is not relevant for BTRACE_FORMAT_PT since the trace will already
+	 start at the PC at which tracing was enabled.  */
+      if ((conf->format != BTRACE_FORMAT_PT)
+	  && can_access_registers_ptid (tp->ptid))
+	btrace_add_pc (tp);
+    }
+  CATCH (exception, RETURN_MASK_ALL)
+    {
+      btrace_disable (tp);
+    }
+  END_CATCH
 }
 
 /* See btrace.h.  */
diff --git a/gdb/testsuite/gdb.btrace/enable-running.c b/gdb/testsuite/gdb.btrace/enable-running.c
new file mode 100644
index 0000000..9fd9aca
--- /dev/null
+++ b/gdb/testsuite/gdb.btrace/enable-running.c
@@ -0,0 +1,47 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2017 Free Software Foundation, Inc.
+
+   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/>.  */
+
+#include <pthread.h>
+
+static int global;
+
+static void *
+test (void *arg)
+{
+  for (;;)
+    global += 1;
+
+  return arg;
+}
+
+int
+main (void)
+{
+  int i;
+
+  for (i = 0; i < 3; ++i)
+    {
+      pthread_t th;
+
+      pthread_create (&th, NULL, test, NULL);
+      pthread_detach (th);
+    }
+
+  test (NULL); /* bp.1 */
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.btrace/enable-running.exp b/gdb/testsuite/gdb.btrace/enable-running.exp
new file mode 100644
index 0000000..577c319
--- /dev/null
+++ b/gdb/testsuite/gdb.btrace/enable-running.exp
@@ -0,0 +1,72 @@
+# This testcase is part of GDB, the GNU debugger.
+#
+# Copyright 2017 Free Software Foundation, Inc.
+#
+# 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/>.
+
+if { [skip_btrace_tests] } { return -1 }
+
+standard_testfile
+if {[gdb_compile_pthreads "$srcdir/$subdir/$srcfile" "$binfile" executable {debug}] != "" } {
+    return -1
+}
+clean_restart $testfile
+
+# We need to enable non-stop mode for the remote case.
+gdb_test_no_output "set non-stop on"
+
+if ![runto_main] {
+    return -1
+}
+
+set bp_1 [gdb_get_line_number "bp.1" $srcfile]
+
+gdb_breakpoint $bp_1
+gdb_continue_to_breakpoint "cont to $bp_1" ".*$bp_1\r\n.*"
+gdb_test "cont&" "Continuing\."
+
+# All threads are running.  Let's start recording.
+gdb_test_no_output "record btrace"
+
+proc check_tracing_enabled { thread } {
+    global gdb_prompt
+
+    with_test_prefix "thread $thread" {
+        gdb_test "thread $thread" "(running).*" "is running"
+        # Stop the thread before reading the trace.
+        gdb_test_multiple "interrupt" "interrupt" {
+            -re "interrupt\r\n$gdb_prompt " {
+                pass "interrupt"
+            }
+        }
+        # Wait until the thread actually stopped.
+        gdb_test_multiple "" "stopped" {
+            -re "Thread $thread.*stopped\." {
+                pass "stopped"
+            }
+        }
+        # We will consume the thread's current location as part of the
+        # "info record" output.
+        gdb_test "info record" [multi_line \
+            "Active record target: record-btrace" \
+            "Recording format: .*" \
+            "Recorded \[0-9\]+ instructions \[^\\\r\\\n\]*" \
+        ]
+    }
+}
+
+# Check that recording was started on each thread.
+foreach thread {1 2 3 4} {
+    check_tracing_enabled $thread
+}
-- 
1.8.3.1

Re: [PATCH v2 2/2] btrace: allow recording to be started for running threads

[Pedro Alves]

On 01/20/2017 02:38 PM, Markus Metzger wrote:
> When recording is started for a running thread, GDB was able to start tracing
> but then failed to read registers to insert the initial entry for the current
> PC.  We don't really need that initial entry if we don't know where exactly we
> started recording.  Skip that step to allow recording to be started while
> threads are running.
> 
> If we do run into errors, we need to undo the tracing enable to not leak this
> thread.  The operation did not complete so our caller won't clean up this
> thread.
> 
> For the BTRACE_FORMAT_PT btrace format, we don't need that initial entry since
> it will be recorded in the trace.  We can omit the call to btrace_add_pc.
> 
> CC:  Simon Marchi  <simon.marchi@ericsson.com>
> 
> 2017-01-20  Markus Metzger  <markus.t.metzger@intel.com>
> 
> gdb/
> 	* btrace.c (btrace_enable): Do not call btrace_add_pc for
> 	BTRACE_FORMAT_PT or if can_access_registers_ptid returns false.
> 	(validate_registers_access_ptid): New.
> 	(btrace_add_pc): Call validate_registers_access_ptid.
> 
> testsuite/
> 	* gdb.btrace/enable-running.c: New.
> 	* gdb.btrace/enable-running.exp: New.
> ---
>  gdb/btrace.c                                | 44 ++++++++++++++++--
>  gdb/testsuite/gdb.btrace/enable-running.c   | 47 +++++++++++++++++++
>  gdb/testsuite/gdb.btrace/enable-running.exp | 72 +++++++++++++++++++++++++++++
>  3 files changed, 159 insertions(+), 4 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.btrace/enable-running.c
>  create mode 100644 gdb/testsuite/gdb.btrace/enable-running.exp
> 
> diff --git a/gdb/btrace.c b/gdb/btrace.c
> index d266af7..4380e6d 100644
> --- a/gdb/btrace.c
> +++ b/gdb/btrace.c
> @@ -1422,6 +1422,18 @@ btrace_compute_ftrace (struct thread_info *tp, struct btrace_data *btrace)
>    do_cleanups (old_chain);
>  }
>  
> +/* Validate that we can read PTID's registers.  */
> +
> +static void
> +validate_registers_access_ptid (ptid_t ptid)
> +{
> +  struct cleanup *cleanup = save_inferior_ptid ();
> +
> +  inferior_ptid = ptid;
> +  validate_registers_access ();
> +  do_cleanups (cleanup);
> +}

Do we need this, we we have the new function added
by patch #1 ?

> +
>  /* Add an entry for the current PC.  */
>  
>  static void
> @@ -1433,6 +1445,9 @@ btrace_add_pc (struct thread_info *tp)
>    struct cleanup *cleanup;
>    CORE_ADDR pc;
>  
> +  /* Make sure we can read TP's registers.  */
> +  validate_registers_access_ptid (tp->ptid);
> +
>    regcache = get_thread_regcache (tp->ptid);
>    pc = regcache_read_pc (regcache);
>  
> @@ -1472,10 +1487,31 @@ btrace_enable (struct thread_info *tp, const struct btrace_config *conf)
>  
>    tp->btrace.target = target_enable_btrace (tp->ptid, conf);
>  
> -  /* Add an entry for the current PC so we start tracing from where we
> -     enabled it.  */
> -  if (tp->btrace.target != NULL)
> -    btrace_add_pc (tp);
> +  /* We're done if we failed to enable tracing.  */
> +  if (tp->btrace.target == NULL)
> +    return;
> +
> +  /* We need to undo the enable in case of errors.  */
> +  TRY
> +    {
> +      /* Add an entry for the current PC so we start tracing from where we
> +	 enabled it.
> +
> +	 If we can't access TP's registers, TP is most likely running.  In this
> +	 case, we can't really say where tracing was enabled so it should be
> +	 safe to simply skip this step.
> +
> +	 This is not relevant for BTRACE_FORMAT_PT since the trace will already
> +	 start at the PC at which tracing was enabled.  */
> +      if ((conf->format != BTRACE_FORMAT_PT)

Unnecessary parens.

> +	  && can_access_registers_ptid (tp->ptid))

If you have this check, why do you need to the TRY/CATCH ?

Or even, given the validate_registers_access_ptid check
above, why is this check necessary?

> +	btrace_add_pc (tp);
> +    }
> +  CATCH (exception, RETURN_MASK_ALL)

Adding a RETURN_MASK_ALL always raises alarm bells,
because this swallows a user-typed ctrl-c, which
is probably wrong.

> +    {
> +      btrace_disable (tp);
> +    }
> +  END_CATCH
>  }
>  
>  /* See btrace.h.  */
> diff --git a/gdb/testsuite/gdb.btrace/enable-running.c b/gdb/testsuite/gdb.btrace/enable-running.c
> new file mode 100644
> index 0000000..9fd9aca
> --- /dev/null
> +++ b/gdb/testsuite/gdb.btrace/enable-running.c
> @@ -0,0 +1,47 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2017 Free Software Foundation, Inc.
> +
> +   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/>.  */
> +
> +#include <pthread.h>
> +
> +static int global;
> +
> +static void *
> +test (void *arg)
> +{
> +  for (;;)

https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Don.27t_write_tests_that_run_forever

> +    global += 1;
> +
> +  return arg;
> +}
> +
> +int
> +main (void)
> +{
> +  int i;
> +
> +  for (i = 0; i < 3; ++i)
> +    {
> +      pthread_t th;
> +
> +      pthread_create (&th, NULL, test, NULL);
> +      pthread_detach (th);
> +    }
> +
> +  test (NULL); /* bp.1 */
> +
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.btrace/enable-running.exp b/gdb/testsuite/gdb.btrace/enable-running.exp
> new file mode 100644
> index 0000000..577c319
> --- /dev/null
> +++ b/gdb/testsuite/gdb.btrace/enable-running.exp
> @@ -0,0 +1,72 @@
> +# This testcase is part of GDB, the GNU debugger.
> +#
> +# Copyright 2017 Free Software Foundation, Inc.
> +#
> +# 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/>.
> +
> +if { [skip_btrace_tests] } { return -1 }
> +
> +standard_testfile
> +if {[gdb_compile_pthreads "$srcdir/$subdir/$srcfile" "$binfile" executable {debug}] != "" } {
> +    return -1
> +}
> +clean_restart $testfile
> +
> +# We need to enable non-stop mode for the remote case.
> +gdb_test_no_output "set non-stop on"

This is too late with --target_board=native-extended-gdbserver.
Use instead:

save_vars { GDBFLAGS } {
    append GDBFLAGS " -ex \"set non-stop on\""
    clean_restart $testfile
}



> +
> +if ![runto_main] {
> +    return -1
> +}
> +
> +set bp_1 [gdb_get_line_number "bp.1" $srcfile]
> +
> +gdb_breakpoint $bp_1
> +gdb_continue_to_breakpoint "cont to $bp_1" ".*$bp_1\r\n.*"
> +gdb_test "cont&" "Continuing\."
> +
> +# All threads are running.  Let's start recording.
> +gdb_test_no_output "record btrace"
> +
> +proc check_tracing_enabled { thread } {
> +    global gdb_prompt
> +
> +    with_test_prefix "thread $thread" {
> +        gdb_test "thread $thread" "(running).*" "is running"
> +        # Stop the thread before reading the trace.
> +        gdb_test_multiple "interrupt" "interrupt" {
> +            -re "interrupt\r\n$gdb_prompt " {
> +                pass "interrupt"
> +            }
> +        }
> +        # Wait until the thread actually stopped.
> +        gdb_test_multiple "" "stopped" {
> +            -re "Thread $thread.*stopped\." {
> +                pass "stopped"
> +            }
> +        }
> +        # We will consume the thread's current location as part of the
> +        # "info record" output.
> +        gdb_test "info record" [multi_line \
> +            "Active record target: record-btrace" \
> +            "Recording format: .*" \
> +            "Recorded \[0-9\]+ instructions \[^\\\r\\\n\]*" \
> +        ]
> +    }
> +}
> +
> +# Check that recording was started on each thread.
> +foreach thread {1 2 3 4} {
> +    check_tracing_enabled $thread

Looks like this is adding duplicate test messages?  See:

https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Make_sure_test_messages_are_unique

Thanks,
Pedro Alves

Re: [PATCH v2 1/2] thread: add can_access_registers_ptid

[Pedro Alves]

On 01/20/2017 02:38 PM, Markus Metzger wrote:

> +/* See gdbthread.h.  */
> +
> +int
> +can_access_registers_ptid (ptid_t ptid)
> +{
> +  /* No thread, no registers.  */
> +  if (ptid_equal (ptid, null_ptid))
> +    return 0;
> +
> +  /* Don't try to read from a dead thread.  */
> +  if (is_exited (ptid))
> +    return 0;
> +
> +  /* ... or from a spinning thread.  FIXME: see validate_registers_access.  */
> +  if (is_executing (ptid))
> +    return 0;
> +
> +  return 1;
> +}

Use "bool" and true/false.  OK with that change.

Thanks,
Pedro Alves

Re: [PATCH v2 2/2] btrace: allow recording to be started for running threads

[Pedro Alves]

On 01/25/2017 02:32 PM, Pedro Alves wrote:

>> +# Check that recording was started on each thread.
>> +foreach thread {1 2 3 4} {
>> +    check_tracing_enabled $thread
> 
> Looks like this is adding duplicate test messages?  See:
> 
> https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Make_sure_test_messages_are_unique

Bah, somehow missed the with_test_prefix inside the proc.  Ignore
this remark.

Thanks,
Pedro Alves

RE: [PATCH v2 2/2] btrace: allow recording to be started for running threads

[Metzger, Markus T]

Hello Pedro,

Thanks for your review.


> > +/* Validate that we can read PTID's registers.  */
> > +
> > +static void
> > +validate_registers_access_ptid (ptid_t ptid)
> > +{
> > +  struct cleanup *cleanup = save_inferior_ptid ();
> > +
> > +  inferior_ptid = ptid;
> > +  validate_registers_access ();
> > +  do_cleanups (cleanup);
> > +}
> 
> Do we need this, we we have the new function added
> by patch #1 ?

This is a safeguard.  I don't expect that we will ever throw, here.


> > +	  && can_access_registers_ptid (tp->ptid))
> 
> If you have this check, why do you need to the TRY/CATCH ?
> 
> Or even, given the validate_registers_access_ptid check
> above, why is this check necessary?
> 
> > +	btrace_add_pc (tp);
> > +    }
> > +  CATCH (exception, RETURN_MASK_ALL)
> 
> Adding a RETURN_MASK_ALL always raises alarm bells,
> because this swallows a user-typed ctrl-c, which
> is probably wrong.
> 
> > +    {
> > +      btrace_disable (tp);
> > +    }
> > +  END_CATCH

The TRY/CATCH is to clean things up in case of errors or ctrl-c.  The caller
will clean things up for previously enabled threads but expects each
btrace_enable to either succeed or fail and throw.

I see that I forgot to rethrow, though.  This is not intended to swallow
the error - only to disable tracing again in case of errors.

The can_access_registers_ptid check is to avoid the error and thus allow
"record btrace" for running (or exited) threads where the btrace_add_pc
call can be omitted.


> > +# We need to enable non-stop mode for the remote case.
> > +gdb_test_no_output "set non-stop on"
> 
> This is too late with --target_board=native-extended-gdbserver.
> Use instead:
> 
> save_vars { GDBFLAGS } {
>     append GDBFLAGS " -ex \"set non-stop on\""
>     clean_restart $testfile
> }

This test seems to run fine with --target_board=native-extended-gdbserver.

Making the change nevertheless.  There's another instance of this in
gdb.btrace/non-stop.exp.  Will fix this in a separate patch.

A different test, gdb.btrace/enable.exp is failing with that target_board.

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 v2 2/2] btrace: allow recording to be started for running threads

[Pedro Alves]

On 01/25/2017 03:52 PM, Metzger, Markus T wrote:
> Hello Pedro,
> 
> Thanks for your review.
> 
> 
>>> +/* Validate that we can read PTID's registers.  */
>>> +
>>> +static void
>>> +validate_registers_access_ptid (ptid_t ptid)
>>> +{
>>> +  struct cleanup *cleanup = save_inferior_ptid ();
>>> +
>>> +  inferior_ptid = ptid;
>>> +  validate_registers_access ();
>>> +  do_cleanups (cleanup);
>>> +}
>>
>> Do we need this, we we have the new function added
>> by patch #1 ?
> 
> This is a safeguard.  I don't expect that we will ever throw, here.

Aren't the checks done inside the validate_registers_access function
exactly the same as done in can_access_registers_ptid?

And if that doesn't throw, and the thread is running, you have
register accesses right after:

  /* Make sure we can read TP's registers.  */
  validate_registers_access_ptid (tp->ptid);

  regcache = get_thread_regcache (tp->ptid);
  pc = regcache_read_pc (regcache);


I think I must be missing something.

> 
> 
>>> +	  && can_access_registers_ptid (tp->ptid))
>>
>> If you have this check, why do you need to the TRY/CATCH ?
>>
>> Or even, given the validate_registers_access_ptid check
>> above, why is this check necessary?
>>
>>> +	btrace_add_pc (tp);
>>> +    }
>>> +  CATCH (exception, RETURN_MASK_ALL)
>>
>> Adding a RETURN_MASK_ALL always raises alarm bells,
>> because this swallows a user-typed ctrl-c, which
>> is probably wrong.
>>
>>> +    {
>>> +      btrace_disable (tp);
>>> +    }
>>> +  END_CATCH
> 
> The TRY/CATCH is to clean things up in case of errors or ctrl-c.  The caller
> will clean things up for previously enabled threads but expects each
> btrace_enable to either succeed or fail and throw.
> 
> I see that I forgot to rethrow, though.  This is not intended to swallow
> the error - only to disable tracing again in case of errors.

OK, with the rethrow it'd make a lot more sense.

> 
> The can_access_registers_ptid check is to avoid the error and thus allow
> "record btrace" for running (or exited) threads where the btrace_add_pc
> call can be omitted.
> 
> 
>>> +# We need to enable non-stop mode for the remote case.
>>> +gdb_test_no_output "set non-stop on"
>>
>> This is too late with --target_board=native-extended-gdbserver.
>> Use instead:
>>
>> save_vars { GDBFLAGS } {
>>     append GDBFLAGS " -ex \"set non-stop on\""
>>     clean_restart $testfile
>> }
> 
> This test seems to run fine with --target_board=native-extended-gdbserver.

With that board, gdb connects to gdbserver as soon as it starts.

In the posted version of the patch, "set non-stop on" was being issued
after starting gdb.  The result being, "set non-stop on" was being issued
after the initial connection.

But "set non-stop on" won't really work correctly after initial connection
It'd be possible to make work, but, currently it doesn't.
The issue is that GDB only tells the server to switch to the non-stop
variant of the protocol on the initial connection setup.  After the initial
connection, the server continue in all-stop mode, while gdb is in non-stop
mode.  This results in errors like:

 Unexpected vCont reply in non-stop mode: T05swbreak:;06:f0d8ffffff7f0000;07:10d8ffffff7f0000;10:08f9ddf7ff7f0000;thread:p4c0d.4c0d;core:2;

when you try to run something.

So I'm puzzled.  If the test was still passing, but gdb.log showed errors
like the above, it suggests that the test may need to have its regexps
tightened a bit.

Thanks,
Pedro Alves

RE: [PATCH v2 2/2] btrace: allow recording to be started for running threads

[Metzger, Markus T]

Hello Pedro,

> >>> +/* Validate that we can read PTID's registers.  */
> >>> +
> >>> +static void
> >>> +validate_registers_access_ptid (ptid_t ptid)
> >>> +{
> >>> +  struct cleanup *cleanup = save_inferior_ptid ();
> >>> +
> >>> +  inferior_ptid = ptid;
> >>> +  validate_registers_access ();
> >>> +  do_cleanups (cleanup);
> >>> +}
> >>
> >> Do we need this, we we have the new function added
> >> by patch #1 ?
> >
> > This is a safeguard.  I don't expect that we will ever throw, here.
> 
> Aren't the checks done inside the validate_registers_access function
> exactly the same as done in can_access_registers_ptid?
> 
> And if that doesn't throw, and the thread is running, you have
> register accesses right after:
> 
>   /* Make sure we can read TP's registers.  */
>   validate_registers_access_ptid (tp->ptid);
> 
>   regcache = get_thread_regcache (tp->ptid);
>   pc = regcache_read_pc (regcache);
> 
> 
> I think I must be missing something.

btrace_add_pc is called in two different flows:
  - when enabling tracing
  - when decoding trace

The first checks if registers can be accessed and silently omits the
call if they can't.
The second calls btrace_add_pc without checking that registers can
be accessed.  I expected it to fail already before calling btrace_add_pc
and I added this just as a safeguard.

When I actually try to read the trace while the thread is running it fails
in perf_event_read_bts for BTS - again while trying to read the PC.
For PT, it fails here.

It isn't really safe to read the trace buffer while the thread is running
and generating more trace data.  I would therefore add the check to
btrace_fetch and remove it here.

We thus allow tracing to be started and stopped while the thread is
running but we require the thread to be stopped before reading
the trace.  OK?


> >>> +# We need to enable non-stop mode for the remote case.
> >>> +gdb_test_no_output "set non-stop on"
> >>
> >> This is too late with --target_board=native-extended-gdbserver.
> >> Use instead:
> >>
> >> save_vars { GDBFLAGS } {
> >>     append GDBFLAGS " -ex \"set non-stop on\""
> >>     clean_restart $testfile
> >> }
> >
> > This test seems to run fine with --target_board=native-extended-gdbserver.
> 
> With that board, gdb connects to gdbserver as soon as it starts.
> 
> In the posted version of the patch, "set non-stop on" was being issued
> after starting gdb.  The result being, "set non-stop on" was being issued
> after the initial connection.
> 
> But "set non-stop on" won't really work correctly after initial connection
> It'd be possible to make work, but, currently it doesn't.
> The issue is that GDB only tells the server to switch to the non-stop
> variant of the protocol on the initial connection setup.  After the initial
> connection, the server continue in all-stop mode, while gdb is in non-stop
> mode.  This results in errors like:
> 
>  Unexpected vCont reply in non-stop mode:
> T05swbreak:;06:f0d8ffffff7f0000;07:10d8ffffff7f0000;10:08f9ddf7ff7f0000;thread:
> p4c0d.4c0d;core:2;
> 
> when you try to run something.
> 
> So I'm puzzled.  If the test was still passing, but gdb.log showed errors
> like the above, it suggests that the test may need to have its regexps
> tightened a bit.

The test silently stopped when runto_main failed.  With another patch that is
currently in review, this will cause an UNTESTED.

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