RE: [PATCH] gdb: set current thread in btrace_compute_ftrace_1

["Metzger, Markus T" <markus.t.metzger@intel.com>]

Hello Simon,

>> It is indeed confusing who is responsible for switching / restoring the
>> current thread.  After talking to Pedro about this problem a few times,
>> I think a guideline to make progressive changes could be: if a function
>> A (such as btrace_compute_ftrace_bts) takes a thread pointer as
>> parameter (or inferior, or frame), it's because it advertises that it
>> will be working on that thread and does not require that thread to be
>> the current one.  If it did require the current thread to be the same
>> thread it receives as argument, then why have the parameter at all?  So

Because I don't like global state and didn't want to add to the problem.


>> if function A (who doesn't care about the global context) needs to call
>> a function B (like gdb_insn_length in our example) that relies on the
>> current global thread, then it needs to set the current thread to
>> prepare the call B.  It also needs to restore the current thread that
>> was there on entry, because it could happen to be called from a function
>> that cares about the current thread.  And that caller wouldn't expect
>> a function that doesn't care about the global context to modify the
>> global context.

OK, that makes sense.  Those switch-to would go away once we got rid of
global state so we'd want to mark them somehow.

It would really be nice to put them right in front of the call that required it.
But that would mean a lot of switching back and forth.


>From b153656b2b8890ab67ed697da64c8fc6fbfde03f Mon Sep 17 00:00:00 2001
>From: Simon Marchi <simon.marchi@polymtl.ca>
>Date: Wed, 14 Jul 2021 16:31:09 -0400
>Subject: [PATCH v2] gdb: set current thread in btrace_compute_ftrace_1
>
>As documented in bug 28086, test gdb.btrace/enable-new-thread.exp
>started failing with commit 0618ae414979 ("gdb: optimize
>all_matching_threads_iterator"):
>
>    (gdb) record btrace^M
>    (gdb) PASS: gdb.btrace/enable-new-thread.exp: record btrace
>    break 24^M
>    Breakpoint 2 at 0x555555555175: file /home/smarchi/src/binutils-
>gdb/gdb/testsuite/gdb.btrace/enable-new-thread.c, line 24.^M
>    (gdb) continue^M
>    Continuing.^M
>    /home/smarchi/src/binutils-gdb/gdb/inferior.c:303: internal-error: inferior*
>find_inferior_pid(process_stratum_target*, int): Assertion `pid != 0' failed.^M
>    A problem internal to GDB has been detected,^M
>    further debugging may prove unreliable.^M
>    Quit this debugging session? (y or n) FAIL: gdb.btrace/enable-new-thread.exp:
>continue to breakpoint: cont to bp.1 (GDB internal error)
>
>Note that I only see the failure if GDB is compiled without libipt
>support.  I presume that this makes it use BTS instead of PT, so
>exercises different code paths.

Only BTS calls gdb_insn_length ().  But, as you point out below, PT passes a
read-memory callback to the decoder.  I wonder why we are not seeing a
similar problem there?

static int
btrace_pt_readmem_callback (gdb_byte *buffer, size_t size,
                            const struct pt_asid *asid, uint64_t pc,
                            void *context)
{
  int result, errcode;

  result = (int) size;
  try
    {
      errcode = target_read_code ((CORE_ADDR) pc, buffer, size);
      if (errcode != 0)
        result = -pte_nomap;
    }
  catch (const gdb_exception_error &error)
    {
      result = -pte_nomap;
    }

  return result;
}

We turn the exception into a decoder error code.  In the corresponding
test, gdb.btrace/enable-new-thread.exp, we only check that we are recording;
we are not looking at the actual trace.


>Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28086
>Change-Id: I407fbfe41aab990068bd102491aa3709b0a034b3
>---
> gdb/btrace.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
>diff --git a/gdb/btrace.c b/gdb/btrace.c
>index 5e689c11d4bd..6fec8103d4ce 100644
>--- a/gdb/btrace.c
>+++ b/gdb/btrace.c
>@@ -1052,6 +1052,12 @@ btrace_compute_ftrace_bts (struct thread_info *tp,
> 			   const struct btrace_data_bts *btrace,
> 			   std::vector<unsigned int> &gaps)
> {
>+ /* btrace_compute_ftrace_bts may end up doing target calls that require the
>+    current thread to be TP, for example reading memory through
>gdb_insn_length.
>+    Make sure TP is the current thread.  */
>+  scoped_restore_current_thread restore_thread;
>+  switch_to_thread (tp);
>+
>   struct btrace_thread_info *btinfo;
>   struct gdbarch *gdbarch;
>   unsigned int blk;
>@@ -1427,6 +1433,12 @@ btrace_compute_ftrace_pt (struct thread_info *tp,
> 			  const struct btrace_data_pt *btrace,
> 			  std::vector<unsigned int> &gaps)
> {
>+ /* btrace_compute_ftrace_bts may end up doing target calls that require the

The function name is no longer valid in this copy.  Maybe just say 'We may end up ...'?


>+    current thread to be TP, for example reading memory through
>+    btrace_pt_readmem_callback.  Make sure TP is the current thread.  */
>+  scoped_restore_current_thread restore_thread;
>+  switch_to_thread (tp);
>+
>   struct btrace_thread_info *btinfo;
>   struct pt_insn_decoder *decoder;
>   struct pt_config config;
>--
>2.32.0

LGTM.

Thanks,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928