[PATCH 1/3] gdb, btrace: forward fetch_registers for unknown threads

[PATCH 1/3] gdb, btrace: forward fetch_registers for unknown threads

[Markus Metzger]

In the record-btrace target, while replaying, we can only provide the PC
register.  The btrace state is stored in the thread_info.  So, when trying
to determine whether we are currently replaying, GDB calls
find_thread_ptid() to obtain the thread_info.  It also asserts that we do
have a thread_info.

For new threads, libthread-db may fetch registers before the thread is
known to GDB.  In this case, find_thread_ptid() returns nullptr and the
assertion fails.

Forward the fetch_registers request to the target beneath in that case.

gdb/ChangeLog:

2020-03-19  Markus Metzger  <markus.t.metzger@intel.com>

	* record-btrace.c (record_btrace_target::fetch_registers): Forward
	request if we do not have a thread_info.

gdb/testsuite/ChangeLog:

2020-03-19  Markus Metzger  <markus.t.metzger@intel.com>

	* gdb.btrace/enable-new-thread.c: New test.
	* gdb.btrace/enable-new-thread.exp: New file.
---
 gdb/record-btrace.c                           | 11 +++-
 gdb/testsuite/gdb.btrace/enable-new-thread.c  | 36 ++++++++++++
 .../gdb.btrace/enable-new-thread.exp          | 55 +++++++++++++++++++
 3 files changed, 99 insertions(+), 3 deletions(-)
 create mode 100644 gdb/testsuite/gdb.btrace/enable-new-thread.c
 create mode 100644 gdb/testsuite/gdb.btrace/enable-new-thread.exp

diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index 2ca9a61457a..2523b05ba92 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -1532,11 +1532,16 @@ record_btrace_target::remove_breakpoint (struct gdbarch *gdbarch,
 void
 record_btrace_target::fetch_registers (struct regcache *regcache, int regno)
 {
+  btrace_insn_iterator *replay = nullptr;
+
+  /* Thread-db may ask for a thread's registers before GDB knows about the
+     thread.  We forward the request to the target beneath in this
+     case.  */
   thread_info *tp = find_thread_ptid (regcache->target (), regcache->ptid ());
-  gdb_assert (tp != NULL);
+  if (tp != nullptr)
+    replay =  tp->btrace.replay;
 
-  btrace_insn_iterator *replay = tp->btrace.replay;
-  if (replay != NULL && !record_btrace_generating_corefile)
+  if (replay != nullptr && !record_btrace_generating_corefile)
     {
       const struct btrace_insn *insn;
       struct gdbarch *gdbarch;
diff --git a/gdb/testsuite/gdb.btrace/enable-new-thread.c b/gdb/testsuite/gdb.btrace/enable-new-thread.c
new file mode 100644
index 00000000000..d4dc240e314
--- /dev/null
+++ b/gdb/testsuite/gdb.btrace/enable-new-thread.c
@@ -0,0 +1,36 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2020 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>
+#include <unistd.h>
+
+static void *
+test (void *arg)
+{
+  return arg; /* bp.1 */
+}
+
+int
+main (void)
+{
+  pthread_t th;
+
+  pthread_create (&th, NULL, test, NULL);
+  pthread_join (th, NULL);
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.btrace/enable-new-thread.exp b/gdb/testsuite/gdb.btrace/enable-new-thread.exp
new file mode 100644
index 00000000000..8671cd70aef
--- /dev/null
+++ b/gdb/testsuite/gdb.btrace/enable-new-thread.exp
@@ -0,0 +1,55 @@
+# This testcase is part of GDB, the GNU debugger.
+#
+# Copyright 2020 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] } {
+    unsupported "target does not support record-btrace"
+    return -1
+}
+
+standard_testfile
+if [prepare_for_testing "failed to prepare" $testfile $srcfile {debug pthreads}] {
+    return -1
+}
+
+if ![runto_main] {
+    untested "failed to run to main"
+    return -1
+}
+
+# Record the main thread.  Recording will automatically be enabled for the
+# other thread.
+gdb_test "record btrace"
+
+gdb_breakpoint [gdb_get_line_number "bp.1" $srcfile]
+gdb_continue_to_breakpoint "cont to bp.1" ".*/\\* bp\.1 \\*/.*"
+
+proc check_thread_recorded { num } {
+    global decimal
+
+    with_test_prefix "thread $num" {
+	gdb_test "thread $num" "Switching to thread $num.*"
+
+	gdb_test "info record" [multi_line \
+	    "Active record target: record-btrace" \
+	    ".*" \
+	    "Recorded $decimal instructions in $decimal functions \[^\\\r\\\n\]*" \
+       ]
+    }
+}
+
+check_thread_recorded 1
+check_thread_recorded 2
-- 
2.25.1

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, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH 2/3] gdb, btrace: diagnose double and failed enable

[Markus Metzger]

GDB silently ignores attempts to enable branch tracing on a thread that is
already recorded.  This shouldn't happen as recording is enabled exactly
once:

  - when the btrace record target is opened for existing threads
  - when a new thread is added while the btrace record target is pushed

GDB also silently ignores if recording is disabled on threads that were not
recorded.  This shouldn't happen, either, since when stopping recording,
we only disable recording on threads that were recorded.

GDB further silently ignores if recording was not enabled by the
corresponding target method.  Also this shouldn't happen since the target
is supposed to already throw an error if recording cannot be enabled.
This new error in btrace_enable catches cases where the target silently
failed to enable recording.

Throw an error in those cases.

This allows us to detect an actual issue more easily.  It will be
addressed in the next patch.

gdb/ChangeLog:

2020-03-19  Markus Metzger  <markus.t.metzger@intel.com>

	* btrace.c (btrace_enable): Throw an error on double enables and
	when enabling recording fails.
	(btrace_disable): Throw an error if the thread is not recorded.
---
 gdb/btrace.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/gdb/btrace.c b/gdb/btrace.c
index bbf87496497..86a025f3b70 100644
--- a/gdb/btrace.c
+++ b/gdb/btrace.c
@@ -1592,7 +1592,8 @@ void
 btrace_enable (struct thread_info *tp, const struct btrace_config *conf)
 {
   if (tp->btrace.target != NULL)
-    return;
+    error (_("Recording already enabled on thread %s (%s)."),
+	   print_thread_id (tp), target_pid_to_str (tp->ptid).c_str ());
 
 #if !defined (HAVE_LIBIPT)
   if (conf->format == BTRACE_FORMAT_PT)
@@ -1604,9 +1605,9 @@ btrace_enable (struct thread_info *tp, const struct btrace_config *conf)
 
   tp->btrace.target = target_enable_btrace (tp->ptid, conf);
 
-  /* We're done if we failed to enable tracing.  */
   if (tp->btrace.target == NULL)
-    return;
+    error (_("Failed to enable recording on thread %s (%s)."),
+	   print_thread_id (tp), target_pid_to_str (tp->ptid).c_str ());
 
   /* We need to undo the enable in case of errors.  */
   try
@@ -1651,7 +1652,8 @@ btrace_disable (struct thread_info *tp)
   struct btrace_thread_info *btp = &tp->btrace;
 
   if (btp->target == NULL)
-    return;
+    error (_("Recording not enabled on thread %s (%s)."),
+	   print_thread_id (tp), target_pid_to_str (tp->ptid).c_str ());
 
   DEBUG ("disable thread %s (%s)", print_thread_id (tp),
 	 target_pid_to_str (tp->ptid).c_str ());
-- 
2.25.1

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, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH 3/3] gdb, btrace: make record-btrace per-inferior

[Markus Metzger]

When there is more than one inferior, the "record btrace" command should
only apply to the current inferior.

gdb/ChangeLog:

2020-03-19  Markus Metzger  <markus.t.metzger@intel.com>

	* record-btrace.c (record_btrace_enable_warn): Ignore thread if
	its inferior is not recorded by us.
	(record_btrace_target_open): Replace call to all_non_exited_threads ()
	with call to current_inferior ()->non_exited_threads ().
	(record_btrace_target::stop_recording): Likewise.
	(record_btrace_target::close): Likewise.
	(record_btrace_target::wait): Likewise.
	(record_btrace_target::record_stop_replaying): Likewise.

gdb/testsuite/ChangeLog:

2020-03-19  Markus Metzger  <markus.t.metzger@intel.com>

	* gdb.btrace/multi-inferior.c: New test.
	* gdb.btrace/multi-inferior.exp: New file.
---
 gdb/record-btrace.c                         | 15 +++--
 gdb/testsuite/gdb.btrace/multi-inferior.c   | 22 +++++++
 gdb/testsuite/gdb.btrace/multi-inferior.exp | 65 +++++++++++++++++++++
 3 files changed, 97 insertions(+), 5 deletions(-)
 create mode 100644 gdb/testsuite/gdb.btrace/multi-inferior.c
 create mode 100644 gdb/testsuite/gdb.btrace/multi-inferior.exp

diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index 2523b05ba92..978d9ef2970 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -284,6 +284,11 @@ require_btrace (void)
 static void
 record_btrace_enable_warn (struct thread_info *tp)
 {
+  /* Ignore this thread if its inferior is not recorded by us.  */
+  target_ops *rec = tp->inf->target_at (record_stratum);
+  if (rec != &record_btrace_ops)
+    return;
+
   try
     {
       btrace_enable (tp, &record_btrace_conf);
@@ -387,7 +392,7 @@ record_btrace_target_open (const char *args, int from_tty)
   if (!target_has_execution)
     error (_("The program is not being run."));
 
-  for (thread_info *tp : all_non_exited_threads ())
+  for (thread_info *tp : current_inferior ()->non_exited_threads ())
     if (args == NULL || *args == 0 || number_is_in_list (args, tp->global_num))
       {
 	btrace_enable (tp, &record_btrace_conf);
@@ -409,7 +414,7 @@ record_btrace_target::stop_recording ()
 
   record_btrace_auto_disable ();
 
-  for (thread_info *tp : all_non_exited_threads ())
+  for (thread_info *tp : current_inferior ()->non_exited_threads ())
     if (tp->btrace.target != NULL)
       btrace_disable (tp);
 }
@@ -443,7 +448,7 @@ record_btrace_target::close ()
 
   /* We should have already stopped recording.
      Tear down btrace in case we have not.  */
-  for (thread_info *tp : all_non_exited_threads ())
+  for (thread_info *tp : current_inferior ()->non_exited_threads ())
     btrace_teardown (tp);
 }
 
@@ -2630,7 +2635,7 @@ record_btrace_target::wait (ptid_t ptid, struct target_waitstatus *status,
   /* Stop all other threads. */
   if (!target_is_non_stop_p ())
     {
-      for (thread_info *tp : all_non_exited_threads ())
+      for (thread_info *tp : current_inferior ()->non_exited_threads ())
 	record_btrace_cancel_resume (tp);
     }
 
@@ -2867,7 +2872,7 @@ record_btrace_target::goto_record (ULONGEST insn)
 void
 record_btrace_target::record_stop_replaying ()
 {
-  for (thread_info *tp : all_non_exited_threads ())
+  for (thread_info *tp : current_inferior ()->non_exited_threads ())
     record_btrace_stop_replaying (tp);
 }
 
diff --git a/gdb/testsuite/gdb.btrace/multi-inferior.c b/gdb/testsuite/gdb.btrace/multi-inferior.c
new file mode 100644
index 00000000000..9d7b2f1a4c2
--- /dev/null
+++ b/gdb/testsuite/gdb.btrace/multi-inferior.c
@@ -0,0 +1,22 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2020 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/>.  */
+
+int
+main (void)
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.btrace/multi-inferior.exp b/gdb/testsuite/gdb.btrace/multi-inferior.exp
new file mode 100644
index 00000000000..a85edf1d1bf
--- /dev/null
+++ b/gdb/testsuite/gdb.btrace/multi-inferior.exp
@@ -0,0 +1,65 @@
+# This testcase is part of GDB, the GNU debugger.
+#
+# Copyright 2020 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] } {
+    unsupported "target does not support record-btrace"
+    return -1
+}
+
+standard_testfile
+if [prepare_for_testing "failed to prepare" $testfile {} {debug}] {
+    return -1
+}
+
+with_test_prefix "inferior 1" {
+    if ![runto_main] {
+	untested "failed to run to main"
+	return -1
+    }
+}
+
+with_test_prefix "inferior 2" {
+    gdb_test "add-inferior -exec ${binfile}" "Added inferior 2.*"
+    gdb_test "inferior 2" "Switching to inferior 2.*"
+
+    if ![runto_main] {
+	untested "inferior 2: failed to run to main"
+	return -1
+    }
+
+    gdb_test_no_output "record btrace" "record btrace"
+}
+
+with_test_prefix "inferior 1" {
+    gdb_test "inferior 1" "Switching to inferior 1.*"
+
+    gdb_test "info record" "No record target is currently active\\."
+    gdb_test_no_output "record btrace" "record btrace"
+}
+
+with_test_prefix "inferior 3" {
+    gdb_test "add-inferior -exec ${binfile}" "Added inferior 3.*"
+    gdb_test "inferior 3" "Switching to inferior 3.*"
+
+    if ![runto_main] {
+	untested "inferior 3: failed to run to main"
+	return -1
+    }
+
+    gdb_test "info record" "No record target is currently active\\."
+    gdb_test_no_output "record btrace" "record btrace"
+}
-- 
2.25.1

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, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH 1/3] gdb, btrace: forward fetch_registers for unknown threads

[Pedro Alves]

On 4/16/20 8:00 AM, Markus Metzger via Gdb-patches wrote:

> --- /dev/null
> +++ b/gdb/testsuite/gdb.btrace/enable-new-thread.exp
> @@ -0,0 +1,55 @@
> +# This testcase is part of GDB, the GNU debugger.
> +#
> +# Copyright 2020 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/>.
> +

Please add a short intro comment describing what the testcase is about.
Like, "Test that recording is automatically enabled for spawned threads",
or whatever makes sense here.

Thanks,
Pedro Alves

Re: [PATCH 3/3] gdb, btrace: make record-btrace per-inferior

[Pedro Alves]

On 4/16/20 8:00 AM, Markus Metzger via Gdb-patches wrote:

> --- /dev/null
> +++ b/gdb/testsuite/gdb.btrace/multi-inferior.exp
> @@ -0,0 +1,65 @@
> +# This testcase is part of GDB, the GNU debugger.
> +#
> +# Copyright 2020 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/>.
> +

Missing intro comment.

Other than the missing comments, all 3 patches look good to me.

Thanks,
Pedro Alves

RE: [PATCH 1/3] gdb, btrace: forward fetch_registers for unknown threads

[Metzger, Markus T]

Hello Pedro,

Thanks for your review.

> > +# You should have received a copy of the GNU General Public License
> > +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > +
> 
> Please add a short intro comment describing what the testcase is about.
> Like, "Test that recording is automatically enabled for spawned threads",
> or whatever makes sense here.

+# Test that new threads of recorded inferiors also get recorded.
+

OK?

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, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

RE: [PATCH 3/3] gdb, btrace: make record-btrace per-inferior

[Metzger, Markus T]

Hello Pedro,

Thanks for your review.

> > +# You should have received a copy of the GNU General Public License
> > +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > +
> 
> Missing intro comment.

+# Test that recording is per-inferior.
+#
+# When recording an inferior, threads from other inferiors, both existing
+# and newly created, are not automatically recorded.
+#
+# Each inferior can be recorded separately.
+

OK?


> Other than the missing comments, all 3 patches look good to me.

Thanks.  I provided the changes inline.  I don't think there's a need for
a v2 patch series, is there?

Regards,
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, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH 1/3] gdb, btrace: forward fetch_registers for unknown threads

[Pedro Alves]

On 4/20/20 9:39 AM, Metzger, Markus T via Gdb-patches wrote:
> Hello Pedro,
> 
> Thanks for your review.
> 
>>> +# You should have received a copy of the GNU General Public License
>>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>> +
>>
>> Please add a short intro comment describing what the testcase is about.
>> Like, "Test that recording is automatically enabled for spawned threads",
>> or whatever makes sense here.
> 
> +# Test that new threads of recorded inferiors also get recorded.
> +
> 
> OK?

OK.

Thanks,
Pedro Alves

Re: [PATCH 3/3] gdb, btrace: make record-btrace per-inferior

[Pedro Alves]

On 4/20/20 9:39 AM, Metzger, Markus T via Gdb-patches wrote:
> Hello Pedro,
>>
>> Missing intro comment.
> 
> +# Test that recording is per-inferior.
> +#
> +# When recording an inferior, threads from other inferiors, both existing
> +# and newly created, are not automatically recorded.
> +#
> +# Each inferior can be recorded separately.
> +
> 
> OK?

Perfect.

> 
> 
>> Other than the missing comments, all 3 patches look good to me.
> 
> Thanks.  I provided the changes inline.  I don't think there's a need for
> a v2 patch series, is there?

There's no need for a v2 patch series.

Thanks,
Pedro Alves