[PATCH v2 0/7] gdb, thread-iter: handle null_ptid

[PATCH v2 0/7] gdb, thread-iter: handle null_ptid

[Markus Metzger]

This is an extension of 'gdb, thread-iter: handle null_ptid' implementing
what was discussed here:
https://sourceware.org/pipermail/gdb-patches/2021-November/183833.html.

Markus Metzger (7):
  gdb, thread-iter: handle null_ptid
  gdb, btrace: rename record_btrace_enable_warn()
  gdb, ptid: add is_lwp() and is_lwp_or_pid()
  gdb, btrace: check inferior_ptid in
    record_btrace_target::xfer_partial()
  gdb, btrace: switch threads in remote_btrace_maybe_reopen()
  gdb, gdbserver: update thread identifier in enable_btrace target
    method
  gdb, remote, btrace: move switch_to_thread call right before xfer call

 gdb/btrace.c           |  2 +-
 gdb/ravenscar-thread.c | 10 +++++++---
 gdb/record-btrace.c    |  7 ++++---
 gdb/remote.c           | 23 +++++++++++++----------
 gdb/target-delegates.c | 12 ++++++------
 gdb/target.c           |  4 ++--
 gdb/target.h           |  6 +++---
 gdb/thread-iter.c      |  2 +-
 gdb/x86-linux-nat.c    |  3 ++-
 gdb/x86-linux-nat.h    |  2 +-
 gdbserver/gdbthread.h  |  3 +++
 gdbserver/linux-low.cc |  4 ++--
 gdbserver/linux-low.h  |  2 +-
 gdbserver/server.cc    |  4 ++--
 gdbserver/target.cc    |  3 ++-
 gdbserver/target.h     |  8 ++++----
 gdbsupport/ptid.h      | 18 ++++++++++++++++++
 17 files changed, 72 insertions(+), 41 deletions(-)

-- 
2.31.1

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

[PATCH v2 1/7] gdb, thread-iter: handle null_ptid

[Markus Metzger]

Fix a regression introduced by

    0618ae41497 gdb: optimize all_matching_threads_iterator

and exposed by

    FAIL: gdb.btrace/enable-new-thread.exp: ... (GDB internal error)

When we learn about a new thread in a new inferior, we add both and notify
GDB about them, but we do not set inferior_ptid.

On the notification, record-btrace tries to enable recording of the new
thread and, while reading the configuration, checks whether inferior_ptid
is replaying.

This causes the new all_matching_threads_iterator to think we want to
iterate over a single thread, while in reality we do not really want to
iterate over any thread.

Handle that case.
---
 gdb/thread-iter.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/thread-iter.c b/gdb/thread-iter.c
index e56ccd857b0..a2018fd7c10 100644
--- a/gdb/thread-iter.c
+++ b/gdb/thread-iter.c
@@ -117,7 +117,7 @@ all_matching_threads_iterator::all_matching_threads_iterator
 	  if (m_inf != nullptr)
 	    m_thr = &m_inf->thread_list.front ();
 	}
-      else
+      else if (filter_ptid != null_ptid)
 	{
 	  /* Iterate on a single thread.  */
 	  m_mode = mode::SINGLE_THREAD;
-- 
2.31.1

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

Re: [PATCH v2 1/7] gdb, thread-iter: handle null_ptid

[Simon Marchi]

On 2021-12-02 02:15, Markus Metzger wrote:
> Fix a regression introduced by
> 
>     0618ae41497 gdb: optimize all_matching_threads_iterator
> 
> and exposed by
> 
>     FAIL: gdb.btrace/enable-new-thread.exp: ... (GDB internal error)
> 
> When we learn about a new thread in a new inferior, we add both and notify
> GDB about them, but we do not set inferior_ptid.
> 
> On the notification, record-btrace tries to enable recording of the new
> thread and, while reading the configuration, checks whether inferior_ptid
> is replaying.
> 
> This causes the new all_matching_threads_iterator to think we want to
> iterate over a single thread, while in reality we do not really want to
> iterate over any thread.
> 
> Handle that case.
> ---
>  gdb/thread-iter.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdb/thread-iter.c b/gdb/thread-iter.c
> index e56ccd857b0..a2018fd7c10 100644
> --- a/gdb/thread-iter.c
> +++ b/gdb/thread-iter.c
> @@ -117,7 +117,7 @@ all_matching_threads_iterator::all_matching_threads_iterator
>  	  if (m_inf != nullptr)
>  	    m_thr = &m_inf->thread_list.front ();
>  	}
> -      else
> +      else if (filter_ptid != null_ptid)
>  	{
>  	  /* Iterate on a single thread.  */
>  	  m_mode = mode::SINGLE_THREAD;

Hi Markus,

I'm still of the opinion that we shouldn't do this change, that it only
covers up potential bugs.  I think we'll need a third opinion, probably
Pedro if he can fit that in his schedule.  But really the opinion from
anybody else would be appreciated.  The context is in the discussion on
v1:

https://sourceware.org/pipermail/gdb-patches/2021-November/183579.html

Simon

[PATCH v2 2/7] gdb, btrace: rename record_btrace_enable_warn()

[Markus Metzger]

We use record_btrace_enable_warn() as the new-thread observer callback.
It is not used in other contexts.

Rename it to record_btrace_on_new_thread() to make its role more clear.
---
 gdb/record-btrace.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index 3fcfd6a4761..a25670e7e24 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -279,10 +279,10 @@ require_btrace (void)
   return &tp->btrace;
 }
 
-/* Enable branch tracing for one thread.  Warn on errors.  */
+/* The new thread observer.  */
 
 static void
-record_btrace_enable_warn (struct thread_info *tp)
+record_btrace_on_new_thread (struct thread_info *tp)
 {
   /* Ignore this thread if its inferior is not recorded by us.  */
   target_ops *rec = tp->inf->target_at (record_stratum);
@@ -306,7 +306,7 @@ record_btrace_auto_enable (void)
 {
   DEBUG ("attach thread observer");
 
-  gdb::observers::new_thread.attach (record_btrace_enable_warn,
+  gdb::observers::new_thread.attach (record_btrace_on_new_thread,
 				     record_btrace_thread_observer_token,
 				     "record-btrace");
 }
-- 
2.31.1

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

Re: [PATCH v2 2/7] gdb, btrace: rename record_btrace_enable_warn()

[Simon Marchi]

On 2021-12-02 02:15, Markus Metzger wrote:
> We use record_btrace_enable_warn() as the new-thread observer callback.
> It is not used in other contexts.
> 
> Rename it to record_btrace_on_new_thread() to make its role more clear.
> ---
>  gdb/record-btrace.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
> index 3fcfd6a4761..a25670e7e24 100644
> --- a/gdb/record-btrace.c
> +++ b/gdb/record-btrace.c
> @@ -279,10 +279,10 @@ require_btrace (void)
>    return &tp->btrace;
>  }
>  
> -/* Enable branch tracing for one thread.  Warn on errors.  */
> +/* The new thread observer.  */
>  
>  static void
> -record_btrace_enable_warn (struct thread_info *tp)
> +record_btrace_on_new_thread (struct thread_info *tp)
>  {
>    /* Ignore this thread if its inferior is not recorded by us.  */
>    target_ops *rec = tp->inf->target_at (record_stratum);
> @@ -306,7 +306,7 @@ record_btrace_auto_enable (void)
>  {
>    DEBUG ("attach thread observer");
>  
> -  gdb::observers::new_thread.attach (record_btrace_enable_warn,
> +  gdb::observers::new_thread.attach (record_btrace_on_new_thread,
>  				     record_btrace_thread_observer_token,
>  				     "record-btrace");
>  }

LGTM.

Simon

[PATCH v2 3/7] gdb, ptid: add is_lwp() and is_lwp_or_pid()

[Markus Metzger]

Add member functions to check whether a ptid is a single light-weight
process and whether a ptid is a light-weight or full process.
---
 gdbsupport/ptid.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/gdbsupport/ptid.h b/gdbsupport/ptid.h
index 7cdf468589d..08a818ceb59 100644
--- a/gdbsupport/ptid.h
+++ b/gdbsupport/ptid.h
@@ -89,6 +89,24 @@ class ptid_t
 	    && m_tid == 0);
   }
 
+  /* Return true if the ptid represents a light-weight process.  */
+
+  constexpr bool is_lwp () const
+  {
+    return (*this != make_null ()
+	    && *this != make_minus_one ()
+	    && !is_pid ());
+  }
+
+  /* Return true if the ptid represents a light-weight process or a whole
+     process.  */
+
+  constexpr bool is_lwp_or_pid () const
+  {
+    return (*this != make_null ()
+	    && *this != make_minus_one ());
+  }
+
   /* Compare two ptids to see if they are equal.  */
 
   constexpr bool operator== (const ptid_t &other) const
-- 
2.31.1

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

Re: [PATCH v2 3/7] gdb, ptid: add is_lwp() and is_lwp_or_pid()

[Simon Marchi]

On 2021-12-02 02:15, Markus Metzger wrote:
> Add member functions to check whether a ptid is a single light-weight
> process and whether a ptid is a light-weight or full process.
> ---
>  gdbsupport/ptid.h | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/gdbsupport/ptid.h b/gdbsupport/ptid.h
> index 7cdf468589d..08a818ceb59 100644
> --- a/gdbsupport/ptid.h
> +++ b/gdbsupport/ptid.h
> @@ -89,6 +89,24 @@ class ptid_t
>  	    && m_tid == 0);
>    }
>  
> +  /* Return true if the ptid represents a light-weight process.  */
> +
> +  constexpr bool is_lwp () const
> +  {
> +    return (*this != make_null ()
> +	    && *this != make_minus_one ()
> +	    && !is_pid ());

What about the tid field?  Just reading the name `is_lwp`, my intuition
would be that it checks for a null tid field.  So calling is_lwp on (1,
2, 3) or (1, 0, 3) would return false.

At this point it might be easier to write it as:

	return m_pid > 0 && m_lwp > 0 && m_tid == 0;



> +  }
> +
> +  /* Return true if the ptid represents a light-weight process or a whole
> +     process.  */
> +
> +  constexpr bool is_lwp_or_pid () const
> +  {
> +    return (*this != make_null ()
> +	    && *this != make_minus_one ());
> +  }
> +

Same concern, what about tid?

Simon

RE: [PATCH v2 3/7] gdb, ptid: add is_lwp() and is_lwp_or_pid()

[Metzger, Markus T]

Hello Simon,

Thanks for your review.

>> +  /* Return true if the ptid represents a light-weight process.  */
>> +
>> +  constexpr bool is_lwp () const
>> +  {
>> +    return (*this != make_null ()
>> +	    && *this != make_minus_one ()
>> +	    && !is_pid ());
>
>What about the tid field?  Just reading the name `is_lwp`, my intuition
>would be that it checks for a null tid field.  So calling is_lwp on (1,
>2, 3) or (1, 0, 3) would return false.
>
>At this point it might be easier to write it as:
>
>	return m_pid > 0 && m_lwp > 0 && m_tid == 0;

My interpretation was that the tid comes from some user-space
threading library that may or may not use light-weight processes
as their threads.

The tid field would hence not be relevant.  Is that interpretation wrong?

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

Re: [PATCH v2 3/7] gdb, ptid: add is_lwp() and is_lwp_or_pid()

[Simon Marchi]

> My interpretation was that the tid comes from some user-space
> threading library that may or may not use light-weight processes
> as their threads.
> 
> The tid field would hence not be relevant.  Is that interpretation wrong?

That's right.  A simple model could have many tids mapped to a single
lwp (1:n), so I could imagine that at the process stratum target level,
you would have ptids such as (pid, lwp, 0), and have another target
above that add the tid information, so use ptids of the fork (pid, lwp,
tid).

Other models might not map specific tids to specific lwps (a m:n
model), so the thread target may choose to use (pid, 0, tid) - I
suppose, I never actually dealt with that.

I'm wondering about the symmetry with is_pid.  is_pid returns true if
the ptid represents a whole process, not a specific lwp or tid within
it.  If a target uses the 1:n thread model and you have a (pid, lwp,
tid) ptid, shouldn't the semantic be similar, where is_lwp returns true
only if the ptid represents a "whole" lwp and not a specific tid within
it?

If you don't care whether the tid field is set, then it sounds like you
only care about whether the lwp field is set?  If so, can you just use
ptid_t::lwp_p?

Simon

RE: [PATCH v2 3/7] gdb, ptid: add is_lwp() and is_lwp_or_pid()

[Metzger, Markus T]

Hello Simon,

>> My interpretation was that the tid comes from some user-space
>> threading library that may or may not use light-weight processes
>> as their threads.
>>
>> The tid field would hence not be relevant.  Is that interpretation wrong?
>
>That's right.  A simple model could have many tids mapped to a single
>lwp (1:n), so I could imagine that at the process stratum target level,
>you would have ptids such as (pid, lwp, 0), and have another target
>above that add the tid information, so use ptids of the fork (pid, lwp,
>tid).
>
>Other models might not map specific tids to specific lwps (a m:n
>model), so the thread target may choose to use (pid, 0, tid) - I
>suppose, I never actually dealt with that.
>
>I'm wondering about the symmetry with is_pid.  is_pid returns true if
>the ptid represents a whole process, not a specific lwp or tid within
>it.  If a target uses the 1:n thread model and you have a (pid, lwp,
>tid) ptid, shouldn't the semantic be similar, where is_lwp returns true
>only if the ptid represents a "whole" lwp and not a specific tid within
>it?

That assumes, or maybe requires, that such a target models lwps as
(pid, lwp, 0) and puts user-space threads on top.

If that's the case, you're right and we should require tid == 0.

I somehow expected that for pthreads, for example, we'd put the OS
id into lwp and the pthread id into tid and pick the appropriate id
depending on whom we're talking to.

A quick look at the code, however, shows that this is not the case.
We put the pthread id into the lwp_info or thread_info->priv.


>If you don't care whether the tid field is set, then it sounds like you
>only care about whether the lwp field is set?  If so, can you just use
>ptid_t::lwp_p?

This checks against 0 so it would return true for minus_one_ptid.

I could, of course, just check for minus_one_ptid.  I wanted to add
a predicate that makes it clear what the caller wants to check.

The only use is in patch 4, which looks like it is getting dropped,
so let's drop this one, too.

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

[PATCH v2 4/7] gdb, btrace: check inferior_ptid in record_btrace_target::xfer_partial()

[Markus Metzger]

In record_btrace_target::xfer_partial(), we handle accessing read-only
memory during replay.  This request only makes sense if inferior_ptid is
either a single light-weight process or a full process.  Check that.
---
 gdb/record-btrace.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index a25670e7e24..13665096f14 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -1434,6 +1434,7 @@ record_btrace_target::xfer_partial (enum target_object object,
   /* Filter out requests that don't make sense during replay.  */
   if (replay_memory_access == replay_memory_access_read_only
       && !record_btrace_generating_corefile
+      && inferior_ptid.is_lwp_or_pid ()
       && record_is_replaying (inferior_ptid))
     {
       switch (object)
-- 
2.31.1

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

Re: [PATCH v2 4/7] gdb, btrace: check inferior_ptid in record_btrace_target::xfer_partial()

[Simon Marchi]

On 2021-12-02 02:15, Markus Metzger wrote:
> In record_btrace_target::xfer_partial(), we handle accessing read-only
> memory during replay.  This request only makes sense if inferior_ptid is
> either a single light-weight process or a full process.  Check that.
> ---
>  gdb/record-btrace.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
> index a25670e7e24..13665096f14 100644
> --- a/gdb/record-btrace.c
> +++ b/gdb/record-btrace.c
> @@ -1434,6 +1434,7 @@ record_btrace_target::xfer_partial (enum target_object object,
>    /* Filter out requests that don't make sense during replay.  */
>    if (replay_memory_access == replay_memory_access_read_only
>        && !record_btrace_generating_corefile
> +      && inferior_ptid.is_lwp_or_pid ()
>        && record_is_replaying (inferior_ptid))
>      {
>        switch (object)

Given your implementation of is_lwp_or_pid, that means checking that
inferior_ptid is not null_ptid nor minus_one_ptid.

I don't think minus_one_ptid is ever a valid value for inferior_ptid, if
inferior_ptid is minus_one_ptid it means there is clearly a bug
somewhere.  So I think it doesn't make sense to check for that.

As for inferior_ptid being null here... I doubt that there is a
legitimate reason for this to happen... but I suppose that it can
technically happen.  <S-Del>Tthat you could call
switch_to_inferior_no_thread (which sets inferior_ptid to null_ptid) and
then call target_ops::xfer_partial.  Maybe there's some target_object
that read a target-wide property and therefore don't need inferior_ptid
to point to a pid or an lwp?

I would lean towards saying the same thing as for patch 1/7, that this
change only ends up hiding potential future bugs.  But if you can give
me a legitimate scenario for this to happen, then I can be convinced
otherwise.

Simon

RE: [PATCH v2 4/7] gdb, btrace: check inferior_ptid in record_btrace_target::xfer_partial()

[Metzger, Markus T]

Hello Simon,

Thanks for your review.


>> @@ -1434,6 +1434,7 @@ record_btrace_target::xfer_partial (enum
>target_object object,
>>    /* Filter out requests that don't make sense during replay.  */
>>    if (replay_memory_access == replay_memory_access_read_only
>>        && !record_btrace_generating_corefile
>> +      && inferior_ptid.is_lwp_or_pid ()
>>        && record_is_replaying (inferior_ptid))
>>      {
>>        switch (object)
>
>Given your implementation of is_lwp_or_pid, that means checking that
>inferior_ptid is not null_ptid nor minus_one_ptid.
>
>I don't think minus_one_ptid is ever a valid value for inferior_ptid, if
>inferior_ptid is minus_one_ptid it means there is clearly a bug
>somewhere.  So I think it doesn't make sense to check for that.
>
>As for inferior_ptid being null here... I doubt that there is a
>legitimate reason for this to happen... but I suppose that it can
>technically happen.  <S-Del>Tthat you could call
>switch_to_inferior_no_thread (which sets inferior_ptid to null_ptid) and
>then call target_ops::xfer_partial.  Maybe there's some target_object
>that read a target-wide property and therefore don't need inferior_ptid
>to point to a pid or an lwp?
>
>I would lean towards saying the same thing as for patch 1/7, that this
>change only ends up hiding potential future bugs.  But if you can give
>me a legitimate scenario for this to happen, then I can be convinced
>otherwise.

My motivation was simply robustness.  This function is using inferior_ptid
in a call to record_is_replaying() so I'd say it is responsible for checking
that the inferior_ptid value makes sense.

We did have a case where we got null_ptid, here, but that was indeed
due to a bug that gets fixed in the subsequent patch.

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

Re: [PATCH v2 4/7] gdb, btrace: check inferior_ptid in record_btrace_target::xfer_partial()

[Simon Marchi]

On 2022-01-24 11:58, Metzger, Markus T wrote:
> Hello Simon,
> 
> Thanks for your review.
> 
> 
>>> @@ -1434,6 +1434,7 @@ record_btrace_target::xfer_partial (enum
>> target_object object,
>>>    /* Filter out requests that don't make sense during replay.  */
>>>    if (replay_memory_access == replay_memory_access_read_only
>>>        && !record_btrace_generating_corefile
>>> +      && inferior_ptid.is_lwp_or_pid ()
>>>        && record_is_replaying (inferior_ptid))
>>>      {
>>>        switch (object)
>>
>> Given your implementation of is_lwp_or_pid, that means checking that
>> inferior_ptid is not null_ptid nor minus_one_ptid.
>>
>> I don't think minus_one_ptid is ever a valid value for inferior_ptid, if
>> inferior_ptid is minus_one_ptid it means there is clearly a bug
>> somewhere.  So I think it doesn't make sense to check for that.
>>
>> As for inferior_ptid being null here... I doubt that there is a
>> legitimate reason for this to happen... but I suppose that it can
>> technically happen.  <S-Del>Tthat you could call
>> switch_to_inferior_no_thread (which sets inferior_ptid to null_ptid) and
>> then call target_ops::xfer_partial.  Maybe there's some target_object
>> that read a target-wide property and therefore don't need inferior_ptid
>> to point to a pid or an lwp?
>>
>> I would lean towards saying the same thing as for patch 1/7, that this
>> change only ends up hiding potential future bugs.  But if you can give
>> me a legitimate scenario for this to happen, then I can be convinced
>> otherwise.
> 
> My motivation was simply robustness.  This function is using inferior_ptid
> in a call to record_is_replaying() so I'd say it is responsible for checking
> that the inferior_ptid value makes sense.

I would suggest making it a gdb_assert then.

I'm not a fan of handling cases that shouldn't actually happen, because
they increase cognitive load when reading and trying to understand the
code.  If I see "gdb_assert(x > 0)", I known right away what the
pre-condition of the function is.  If I see "if (x == 0) return", I try
to figure out in which situation x can possibly be 0, because it sounds
like this can actually happen, and possibly end up more confused than
where I started.

(plus, it can hide bugs, as mentioned already)

Simon

RE: [PATCH v2 4/7] gdb, btrace: check inferior_ptid in record_btrace_target::xfer_partial()

[Metzger, Markus T]

Hello Simon,

>>>> @@ -1434,6 +1434,7 @@ record_btrace_target::xfer_partial (enum
>>> target_object object,
>>>>    /* Filter out requests that don't make sense during replay.  */
>>>>    if (replay_memory_access == replay_memory_access_read_only
>>>>        && !record_btrace_generating_corefile
>>>> +      && inferior_ptid.is_lwp_or_pid ()
>>>>        && record_is_replaying (inferior_ptid))
>>>>      {
>>>>        switch (object)
>>>
>>> Given your implementation of is_lwp_or_pid, that means checking that
>>> inferior_ptid is not null_ptid nor minus_one_ptid.
>>>
>>> I don't think minus_one_ptid is ever a valid value for inferior_ptid, if
>>> inferior_ptid is minus_one_ptid it means there is clearly a bug
>>> somewhere.  So I think it doesn't make sense to check for that.
>>>
>>> As for inferior_ptid being null here... I doubt that there is a
>>> legitimate reason for this to happen... but I suppose that it can
>>> technically happen.  <S-Del>Tthat you could call
>>> switch_to_inferior_no_thread (which sets inferior_ptid to null_ptid) and
>>> then call target_ops::xfer_partial.  Maybe there's some target_object
>>> that read a target-wide property and therefore don't need inferior_ptid
>>> to point to a pid or an lwp?
>>>
>>> I would lean towards saying the same thing as for patch 1/7, that this
>>> change only ends up hiding potential future bugs.  But if you can give
>>> me a legitimate scenario for this to happen, then I can be convinced
>>> otherwise.
>>
>> My motivation was simply robustness.  This function is using inferior_ptid
>> in a call to record_is_replaying() so I'd say it is responsible for checking
>> that the inferior_ptid value makes sense.
>
>I would suggest making it a gdb_assert then.
>
>I'm not a fan of handling cases that shouldn't actually happen, because
>they increase cognitive load when reading and trying to understand the
>code.  If I see "gdb_assert(x > 0)", I known right away what the
>pre-condition of the function is.  If I see "if (x == 0) return", I try
>to figure out in which situation x can possibly be 0, because it sounds
>like this can actually happen, and possibly end up more confused than
>where I started.

This function is handling a subset of requests and forwards what it doesn't
handle.  I don't think that an assert is appropriate.  We will eventually assert
if we just drop this patch and don't do this extra check.

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

[PATCH v2 5/7] gdb, btrace: switch threads in remote_btrace_maybe_reopen()

[Markus Metzger]

In remote_btrace_maybe_reopen() we iterate over threads and use
set_general_thread() to set the thread from which to transfer the btrace
configuration.

This sets the remote general thread but does not affect inferior_ptid.  On
the xfer request later on, remote_target::xfer_partial() again sets the
remote general thread to inferior_ptid, overwriting what
remote_btrace_maybe_reopen() had done.

In one case, this led to inferior_ptid being null_ptid when we tried to
enable tracing on a newly created thread inside a newly created process
during attach.

This, in turn, led to find_inferior_pid() asserting when we iterated over
threads in record_btrace_is_replaying(), which was called from
record_btrace_target::xfer_partial() when reading the btrace configuration
of the new thread to check whether it was already being recorded.

The bug was exposed by

    0618ae41497 gdb: optimize all_matching_threads_iterator

and found by

    FAIL: gdb.btrace/enable-new-thread.exp: ... (GDB internal error)

Use switch_to_thread() in remote_btrace_maybe_reopen().
---
 gdb/remote.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index f53e31e126e..bcc71a1d021 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -14117,7 +14117,7 @@ remote_target::remote_btrace_maybe_reopen ()
 
   for (thread_info *tp : all_non_exited_threads (this))
     {
-      set_general_thread (tp->ptid);
+      switch_to_thread (tp);
 
       memset (&rs->btrace_config, 0x00, sizeof (struct btrace_config));
       btrace_read_config (&rs->btrace_config);
-- 
2.31.1

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

Re: [PATCH v2 5/7] gdb, btrace: switch threads in remote_btrace_maybe_reopen()

[Simon Marchi]

On 2021-12-02 02:15, Markus Metzger wrote:
> In remote_btrace_maybe_reopen() we iterate over threads and use
> set_general_thread() to set the thread from which to transfer the btrace
> configuration.
> 
> This sets the remote general thread but does not affect inferior_ptid.  On
> the xfer request later on, remote_target::xfer_partial() again sets the
> remote general thread to inferior_ptid, overwriting what
> remote_btrace_maybe_reopen() had done.
> 
> In one case, this led to inferior_ptid being null_ptid when we tried to
> enable tracing on a newly created thread inside a newly created process
> during attach.
> 
> This, in turn, led to find_inferior_pid() asserting when we iterated over
> threads in record_btrace_is_replaying(), which was called from
> record_btrace_target::xfer_partial() when reading the btrace configuration
> of the new thread to check whether it was already being recorded.
> 
> The bug was exposed by
> 
>     0618ae41497 gdb: optimize all_matching_threads_iterator
> 
> and found by
> 
>     FAIL: gdb.btrace/enable-new-thread.exp: ... (GDB internal error)
> 
> Use switch_to_thread() in remote_btrace_maybe_reopen().
> ---
>  gdb/remote.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdb/remote.c b/gdb/remote.c
> index f53e31e126e..bcc71a1d021 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -14117,7 +14117,7 @@ remote_target::remote_btrace_maybe_reopen ()
>  
>    for (thread_info *tp : all_non_exited_threads (this))
>      {
> -      set_general_thread (tp->ptid);
> +      switch_to_thread (tp);
>  
>        memset (&rs->btrace_config, 0x00, sizeof (struct btrace_config));
>        btrace_read_config (&rs->btrace_config);

As far as I can remember from our past discussion, this LGTM, thanks.

Simon

[PATCH v2 6/7] gdb, gdbserver: update thread identifier in enable_btrace target method

[Markus Metzger]

The enable_btrace target method takes a ptid_t to identify the thread on
which tracing shall be enabled.

Change this to thread_info * to avoid translating back and forth between
the two.  This will be used in a subsequent patch.
---
 gdb/btrace.c           |  2 +-
 gdb/ravenscar-thread.c | 10 +++++++---
 gdb/remote.c           |  8 +++++---
 gdb/target-delegates.c | 12 ++++++------
 gdb/target.c           |  4 ++--
 gdb/target.h           |  6 +++---
 gdb/x86-linux-nat.c    |  3 ++-
 gdb/x86-linux-nat.h    |  2 +-
 gdbserver/gdbthread.h  |  3 +++
 gdbserver/linux-low.cc |  4 ++--
 gdbserver/linux-low.h  |  2 +-
 gdbserver/server.cc    |  4 ++--
 gdbserver/target.cc    |  3 ++-
 gdbserver/target.h     |  8 ++++----
 14 files changed, 41 insertions(+), 30 deletions(-)

diff --git a/gdb/btrace.c b/gdb/btrace.c
index f12b1554d65..57fb04b0f5f 100644
--- a/gdb/btrace.c
+++ b/gdb/btrace.c
@@ -1618,7 +1618,7 @@ btrace_enable (struct thread_info *tp, const struct btrace_config *conf)
   DEBUG ("enable thread %s (%s)", print_thread_id (tp),
 	 target_pid_to_str (tp->ptid).c_str ());
 
-  tp->btrace.target = target_enable_btrace (tp->ptid, conf);
+  tp->btrace.target = target_enable_btrace (tp, conf);
 
   if (tp->btrace.target == NULL)
     error (_("Failed to enable recording on thread %s (%s)."),
diff --git a/gdb/ravenscar-thread.c b/gdb/ravenscar-thread.c
index 0611fa8b4f1..6ab4c3ab406 100644
--- a/gdb/ravenscar-thread.c
+++ b/gdb/ravenscar-thread.c
@@ -120,12 +120,16 @@ struct ravenscar_thread_target final : public target_ops
 
   ptid_t get_ada_task_ptid (long lwp, ULONGEST thread) override;
 
-  struct btrace_target_info *enable_btrace (ptid_t ptid,
+  struct btrace_target_info *enable_btrace (thread_info *tp,
 					    const struct btrace_config *conf)
     override
   {
-    ptid = get_base_thread_from_ravenscar_task (ptid);
-    return beneath ()->enable_btrace (ptid, conf);
+    process_stratum_target *proc_target
+      = as_process_stratum_target (this->beneath ());
+    ptid_t underlying = get_base_thread_from_ravenscar_task (tp->ptid);
+    tp = find_thread_ptid (proc_target, underlying);
+
+    return beneath ()->enable_btrace (tp, conf);
   }
 
   void mourn_inferior () override;
diff --git a/gdb/remote.c b/gdb/remote.c
index bcc71a1d021..f86f885dbb6 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -669,8 +669,8 @@ class remote_target : public process_stratum_target
   bool use_agent (bool use) override;
   bool can_use_agent () override;
 
-  struct btrace_target_info *enable_btrace (ptid_t ptid,
-					    const struct btrace_config *conf) override;
+  struct btrace_target_info *
+    enable_btrace (thread_info *tp, const struct btrace_config *conf) override;
 
   void disable_btrace (struct btrace_target_info *tinfo) override;
 
@@ -14159,7 +14159,8 @@ remote_target::remote_btrace_maybe_reopen ()
 /* Enable branch tracing.  */
 
 struct btrace_target_info *
-remote_target::enable_btrace (ptid_t ptid, const struct btrace_config *conf)
+remote_target::enable_btrace (thread_info *tp,
+			      const struct btrace_config *conf)
 {
   struct btrace_target_info *tinfo = NULL;
   struct packet_config *packet = NULL;
@@ -14183,6 +14184,7 @@ remote_target::enable_btrace (ptid_t ptid, const struct btrace_config *conf)
 
   btrace_sync_conf (conf);
 
+  ptid_t ptid = tp->ptid;
   set_general_thread (ptid);
 
   buf += xsnprintf (buf, endbuf - buf, "%s", packet->name);
diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c
index fb9c78a5f79..4e9c2648da6 100644
--- a/gdb/target-delegates.c
+++ b/gdb/target-delegates.c
@@ -146,7 +146,7 @@ struct dummy_target : public target_ops
   traceframe_info_up traceframe_info () override;
   bool use_agent (bool arg0) override;
   bool can_use_agent () override;
-  struct btrace_target_info *enable_btrace (ptid_t arg0, const struct btrace_config *arg1) override;
+  struct btrace_target_info *enable_btrace (thread_info *arg0, const struct btrace_config *arg1) override;
   void disable_btrace (struct btrace_target_info *arg0) override;
   void teardown_btrace (struct btrace_target_info *arg0) override;
   enum btrace_error read_btrace (struct btrace_data *arg0, struct btrace_target_info *arg1, enum btrace_read_type arg2) override;
@@ -321,7 +321,7 @@ struct debug_target : public target_ops
   traceframe_info_up traceframe_info () override;
   bool use_agent (bool arg0) override;
   bool can_use_agent () override;
-  struct btrace_target_info *enable_btrace (ptid_t arg0, const struct btrace_config *arg1) override;
+  struct btrace_target_info *enable_btrace (thread_info *arg0, const struct btrace_config *arg1) override;
   void disable_btrace (struct btrace_target_info *arg0) override;
   void teardown_btrace (struct btrace_target_info *arg0) override;
   enum btrace_error read_btrace (struct btrace_data *arg0, struct btrace_target_info *arg1, enum btrace_read_type arg2) override;
@@ -3784,25 +3784,25 @@ debug_target::can_use_agent ()
 }
 
 struct btrace_target_info *
-target_ops::enable_btrace (ptid_t arg0, const struct btrace_config *arg1)
+target_ops::enable_btrace (thread_info *arg0, const struct btrace_config *arg1)
 {
   return this->beneath ()->enable_btrace (arg0, arg1);
 }
 
 struct btrace_target_info *
-dummy_target::enable_btrace (ptid_t arg0, const struct btrace_config *arg1)
+dummy_target::enable_btrace (thread_info *arg0, const struct btrace_config *arg1)
 {
   tcomplain ();
 }
 
 struct btrace_target_info *
-debug_target::enable_btrace (ptid_t arg0, const struct btrace_config *arg1)
+debug_target::enable_btrace (thread_info *arg0, const struct btrace_config *arg1)
 {
   struct btrace_target_info * result;
   fprintf_unfiltered (gdb_stdlog, "-> %s->enable_btrace (...)\n", this->beneath ()->shortname ());
   result = this->beneath ()->enable_btrace (arg0, arg1);
   fprintf_unfiltered (gdb_stdlog, "<- %s->enable_btrace (", this->beneath ()->shortname ());
-  target_debug_print_ptid_t (arg0);
+  target_debug_print_thread_info_p (arg0);
   fputs_unfiltered (", ", gdb_stdlog);
   target_debug_print_const_struct_btrace_config_p (arg1);
   fputs_unfiltered (") = ", gdb_stdlog);
diff --git a/gdb/target.c b/gdb/target.c
index 06a21c46a19..666edcc41e0 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -4049,9 +4049,9 @@ target_ranged_break_num_registers (void)
 /* See target.h.  */
 
 struct btrace_target_info *
-target_enable_btrace (ptid_t ptid, const struct btrace_config *conf)
+target_enable_btrace (thread_info *tp, const struct btrace_config *conf)
 {
-  return current_inferior ()->top_target ()->enable_btrace (ptid, conf);
+  return current_inferior ()->top_target ()->enable_btrace (tp, conf);
 }
 
 /* See target.h.  */
diff --git a/gdb/target.h b/gdb/target.h
index e709b7d7cfd..6466ba7028e 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1153,10 +1153,10 @@ struct target_ops
     virtual bool can_use_agent ()
       TARGET_DEFAULT_RETURN (false);
 
-    /* Enable branch tracing for PTID using CONF configuration.
+    /* Enable branch tracing for TP using CONF configuration.
        Return a branch trace target information struct for reading and for
        disabling branch trace.  */
-    virtual struct btrace_target_info *enable_btrace (ptid_t ptid,
+    virtual struct btrace_target_info *enable_btrace (thread_info *tp,
 						      const struct btrace_config *conf)
       TARGET_DEFAULT_NORETURN (tcomplain ());
 
@@ -2511,7 +2511,7 @@ extern void update_target_permissions (void);
 
 /* See to_enable_btrace in struct target_ops.  */
 extern struct btrace_target_info *
-  target_enable_btrace (ptid_t ptid, const struct btrace_config *);
+  target_enable_btrace (thread_info *tp, const struct btrace_config *);
 
 /* See to_disable_btrace in struct target_ops.  */
 extern void target_disable_btrace (struct btrace_target_info *btinfo);
diff --git a/gdb/x86-linux-nat.c b/gdb/x86-linux-nat.c
index adea1ad0092..bb422263112 100644
--- a/gdb/x86-linux-nat.c
+++ b/gdb/x86-linux-nat.c
@@ -210,10 +210,11 @@ x86_linux_nat_target::read_description ()
 /* Enable branch tracing.  */
 
 struct btrace_target_info *
-x86_linux_nat_target::enable_btrace (ptid_t ptid,
+x86_linux_nat_target::enable_btrace (thread_info *tp,
 				     const struct btrace_config *conf)
 {
   struct btrace_target_info *tinfo = nullptr;
+  ptid_t ptid = tp->ptid;
   try
     {
       tinfo = linux_enable_btrace (ptid, conf);
diff --git a/gdb/x86-linux-nat.h b/gdb/x86-linux-nat.h
index a8123f76491..28926eb1b21 100644
--- a/gdb/x86-linux-nat.h
+++ b/gdb/x86-linux-nat.h
@@ -35,7 +35,7 @@ struct x86_linux_nat_target : public x86_nat_target<linux_nat_target>
   /* Add the description reader.  */
   const struct target_desc *read_description () override;
 
-  struct btrace_target_info *enable_btrace (ptid_t ptid,
+  struct btrace_target_info *enable_btrace (thread_info *tp,
 					    const struct btrace_config *conf) override;
   void disable_btrace (struct btrace_target_info *tinfo) override;
   void teardown_btrace (struct btrace_target_info *tinfo) override;
diff --git a/gdbserver/gdbthread.h b/gdbserver/gdbthread.h
index 7c293b1f89d..26a3bc86a25 100644
--- a/gdbserver/gdbthread.h
+++ b/gdbserver/gdbthread.h
@@ -21,6 +21,9 @@
 
 #include "gdbsupport/common-gdbthread.h"
 #include "inferiors.h"
+#include "target/waitstatus.h"
+#include "target.h"
+#include "regcache.h"
 
 #include <list>
 
diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index d214aff7051..171525ec485 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -6932,10 +6932,10 @@ linux_process_target::qxfer_libraries_svr4 (const char *annex,
 #ifdef HAVE_LINUX_BTRACE
 
 btrace_target_info *
-linux_process_target::enable_btrace (ptid_t ptid,
+linux_process_target::enable_btrace (thread_info *tp,
 				     const btrace_config *conf)
 {
-  return linux_enable_btrace (ptid, conf);
+  return linux_enable_btrace (tp->id, conf);
 }
 
 /* See to_disable_btrace target method.  */
diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
index 05067ffc6e6..704e507ed0d 100644
--- a/gdbserver/linux-low.h
+++ b/gdbserver/linux-low.h
@@ -276,7 +276,7 @@ class linux_process_target : public process_stratum_target
   bool supports_agent () override;
 
 #ifdef HAVE_LINUX_BTRACE
-  btrace_target_info *enable_btrace (ptid_t ptid,
+  btrace_target_info *enable_btrace (thread_info *tp,
 				     const btrace_config *conf) override;
 
   int disable_btrace (btrace_target_info *tinfo) override;
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index b1d4c92b3d9..2200a0f8a0c 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -409,7 +409,7 @@ handle_btrace_enable_bts (struct thread_info *thread)
     error (_("Btrace already enabled."));
 
   current_btrace_conf.format = BTRACE_FORMAT_BTS;
-  thread->btrace = target_enable_btrace (thread->id, &current_btrace_conf);
+  thread->btrace = target_enable_btrace (thread, &current_btrace_conf);
 }
 
 /* Handle btrace enabling in Intel Processor Trace format.  */
@@ -421,7 +421,7 @@ handle_btrace_enable_pt (struct thread_info *thread)
     error (_("Btrace already enabled."));
 
   current_btrace_conf.format = BTRACE_FORMAT_PT;
-  thread->btrace = target_enable_btrace (thread->id, &current_btrace_conf);
+  thread->btrace = target_enable_btrace (thread, &current_btrace_conf);
 }
 
 /* Handle btrace disabling.  */
diff --git a/gdbserver/target.cc b/gdbserver/target.cc
index bfa860546a0..779bb1aa9a6 100644
--- a/gdbserver/target.cc
+++ b/gdbserver/target.cc
@@ -736,7 +736,8 @@ process_stratum_target::supports_agent ()
 }
 
 btrace_target_info *
-process_stratum_target::enable_btrace (ptid_t ptid, const btrace_config *conf)
+process_stratum_target::enable_btrace (thread_info *tp,
+				       const btrace_config *conf)
 {
   error (_("Target does not support branch tracing."));
 }
diff --git a/gdbserver/target.h b/gdbserver/target.h
index 6c863c84666..ae14023c26b 100644
--- a/gdbserver/target.h
+++ b/gdbserver/target.h
@@ -403,9 +403,9 @@ class process_stratum_target
   /* Return true if target supports debugging agent.  */
   virtual bool supports_agent ();
 
-  /* Enable branch tracing for PTID based on CONF and allocate a branch trace
+  /* Enable branch tracing for TP based on CONF and allocate a branch trace
      target information struct for reading and for disabling branch trace.  */
-  virtual btrace_target_info *enable_btrace (ptid_t ptid,
+  virtual btrace_target_info *enable_btrace (thread_info *tp,
 					     const btrace_config *conf);
 
   /* Disable branch tracing.
@@ -627,9 +627,9 @@ int kill_inferior (process_info *proc);
   the_target->supports_agent ()
 
 static inline struct btrace_target_info *
-target_enable_btrace (ptid_t ptid, const struct btrace_config *conf)
+target_enable_btrace (thread_info *tp, const struct btrace_config *conf)
 {
-  return the_target->enable_btrace (ptid, conf);
+  return the_target->enable_btrace (tp, conf);
 }
 
 static inline int
-- 
2.31.1

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

Re: [PATCH v2 6/7] gdb, gdbserver: update thread identifier in enable_btrace target method

[Simon Marchi]

> diff --git a/gdbserver/gdbthread.h b/gdbserver/gdbthread.h
> index 7c293b1f89d..26a3bc86a25 100644
> --- a/gdbserver/gdbthread.h
> +++ b/gdbserver/gdbthread.h
> @@ -21,6 +21,9 @@
>  
>  #include "gdbsupport/common-gdbthread.h"
>  #include "inferiors.h"
> +#include "target/waitstatus.h"
> +#include "target.h"
> +#include "regcache.h"

Are these spurious changes?

Otherwise, LGTM.

Simon

RE: [PATCH v2 6/7] gdb, gdbserver: update thread identifier in enable_btrace target method

[Metzger, Markus T]

Hello Simon,

>>  #include "gdbsupport/common-gdbthread.h"
>>  #include "inferiors.h"
>> +#include "target/waitstatus.h"
>> +#include "target.h"
>> +#include "regcache.h"
>
>Are these spurious changes?

They are indeed spurious.  I remember having had build issues but when I remove that hunk now, everything builds just fine.  Dropping it.

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

[PATCH v2 7/7] gdb, remote, btrace: move switch_to_thread call right before xfer call

[Markus Metzger]

In remote_target::remote_btrace_maybe_reopen, we switch to the currently
iterated thread in order to set inferior_ptid for a subsequent xfer.

Move the switch_to_thread call directly before the target_read_stralloc
call to clarify why we need to switch threads.
---
 gdb/remote.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index f86f885dbb6..d0ca86f6e23 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -14084,12 +14084,15 @@ remote_target::btrace_sync_conf (const btrace_config *conf)
     }
 }
 
-/* Read the current thread's btrace configuration from the target and
-   store it into CONF.  */
+/* Read TP's btrace configuration from the target and store it into CONF.  */
 
 static void
-btrace_read_config (struct btrace_config *conf)
+btrace_read_config (thread_info *tp, struct btrace_config *conf)
 {
+  /* target_read_stralloc relies on INFERIOR_PTID.  */
+  scoped_restore_current_thread restore_thread;
+  switch_to_thread (tp);
+
   gdb::optional<gdb::char_vector> xml
     = target_read_stralloc (current_inferior ()->top_target (),
 			    TARGET_OBJECT_BTRACE_CONF, "");
@@ -14117,10 +14120,8 @@ remote_target::remote_btrace_maybe_reopen ()
 
   for (thread_info *tp : all_non_exited_threads (this))
     {
-      switch_to_thread (tp);
-
       memset (&rs->btrace_config, 0x00, sizeof (struct btrace_config));
-      btrace_read_config (&rs->btrace_config);
+      btrace_read_config (tp, &rs->btrace_config);
 
       if (rs->btrace_config.format == BTRACE_FORMAT_NONE)
 	continue;
@@ -14208,7 +14209,7 @@ remote_target::enable_btrace (thread_info *tp,
      tracing itself is not impacted.  */
   try
     {
-      btrace_read_config (&tinfo->conf);
+      btrace_read_config (tp, &tinfo->conf);
     }
   catch (const gdb_exception_error &err)
     {
-- 
2.31.1

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

Re: [PATCH v2 7/7] gdb, remote, btrace: move switch_to_thread call right before xfer call

[Simon Marchi]

On 2021-12-02 02:15, Markus Metzger wrote:
> In remote_target::remote_btrace_maybe_reopen, we switch to the currently
> iterated thread in order to set inferior_ptid for a subsequent xfer.
> 
> Move the switch_to_thread call directly before the target_read_stralloc
> call to clarify why we need to switch threads.
> ---
>  gdb/remote.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/gdb/remote.c b/gdb/remote.c
> index f86f885dbb6..d0ca86f6e23 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -14084,12 +14084,15 @@ remote_target::btrace_sync_conf (const btrace_config *conf)
>      }
>  }
>  
> -/* Read the current thread's btrace configuration from the target and
> -   store it into CONF.  */
> +/* Read TP's btrace configuration from the target and store it into CONF.  */
>  
>  static void
> -btrace_read_config (struct btrace_config *conf)
> +btrace_read_config (thread_info *tp, struct btrace_config *conf)
>  {
> +  /* target_read_stralloc relies on INFERIOR_PTID.  */
> +  scoped_restore_current_thread restore_thread;
> +  switch_to_thread (tp);
> +
>    gdb::optional<gdb::char_vector> xml
>      = target_read_stralloc (current_inferior ()->top_target (),
>  			    TARGET_OBJECT_BTRACE_CONF, "");
> @@ -14117,10 +14120,8 @@ remote_target::remote_btrace_maybe_reopen ()
>  
>    for (thread_info *tp : all_non_exited_threads (this))
>      {
> -      switch_to_thread (tp);
> -
>        memset (&rs->btrace_config, 0x00, sizeof (struct btrace_config));
> -      btrace_read_config (&rs->btrace_config);
> +      btrace_read_config (tp, &rs->btrace_config);
>  
>        if (rs->btrace_config.format == BTRACE_FORMAT_NONE)
>  	continue;
> @@ -14208,7 +14209,7 @@ remote_target::enable_btrace (thread_info *tp,
>       tracing itself is not impacted.  */
>    try
>      {
> -      btrace_read_config (&tinfo->conf);
> +      btrace_read_config (tp, &tinfo->conf);
>      }
>    catch (const gdb_exception_error &err)
>      {

Just one question, have you check whether the
scoped_restore_current_thread in remote_btrace_maybe_reopen can be
removed?

In any case, LGTM.

Simon

RE: [PATCH v2 7/7] gdb, remote, btrace: move switch_to_thread call right before xfer call

[Metzger, Markus T]

Hello Simon,

>Just one question, have you check whether the
>scoped_restore_current_thread in remote_btrace_maybe_reopen can be
>removed?

It can indeed be dropped now that we added a scoped-restore to btrace_read_config().  Dropped it.

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

RE: [PATCH v2 0/7] gdb, thread-iter: handle null_ptid

[Metzger, Markus T]

Kindly pinging,

Markus.

>-----Original Message-----
>From: Metzger, Markus T <markus.t.metzger@intel.com>
>Sent: Thursday, December 2, 2021 8:15 AM
>To: gdb-patches@sourceware.org
>Cc: simon.marchi@polymtl.ca
>Subject: [PATCH v2 0/7] gdb, thread-iter: handle null_ptid
>
>This is an extension of 'gdb, thread-iter: handle null_ptid' implementing
>what was discussed here:
>https://sourceware.org/pipermail/gdb-patches/2021-November/183833.html.
>
>Markus Metzger (7):
>  gdb, thread-iter: handle null_ptid
>  gdb, btrace: rename record_btrace_enable_warn()
>  gdb, ptid: add is_lwp() and is_lwp_or_pid()
>  gdb, btrace: check inferior_ptid in
>    record_btrace_target::xfer_partial()
>  gdb, btrace: switch threads in remote_btrace_maybe_reopen()
>  gdb, gdbserver: update thread identifier in enable_btrace target
>    method
>  gdb, remote, btrace: move switch_to_thread call right before xfer call
>
> gdb/btrace.c           |  2 +-
> gdb/ravenscar-thread.c | 10 +++++++---
> gdb/record-btrace.c    |  7 ++++---
> gdb/remote.c           | 23 +++++++++++++----------
> gdb/target-delegates.c | 12 ++++++------
> gdb/target.c           |  4 ++--
> gdb/target.h           |  6 +++---
> gdb/thread-iter.c      |  2 +-
> gdb/x86-linux-nat.c    |  3 ++-
> gdb/x86-linux-nat.h    |  2 +-
> gdbserver/gdbthread.h  |  3 +++
> gdbserver/linux-low.cc |  4 ++--
> gdbserver/linux-low.h  |  2 +-
> gdbserver/server.cc    |  4 ++--
> gdbserver/target.cc    |  3 ++-
> gdbserver/target.h     |  8 ++++----
> gdbsupport/ptid.h      | 18 ++++++++++++++++++
> 17 files changed, 72 insertions(+), 41 deletions(-)
>
>--
>2.31.1

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

RE: [PATCH v2 0/7] gdb, thread-iter: handle null_ptid

[Metzger, Markus T]

Thanks for your review, Simon,

I dropped the patches that were a bit more controversial and went beyond
fixing the bug this series intended to fix, i.e.

>  gdb, thread-iter: handle null_ptid
>  gdb, ptid: add is_lwp() and is_lwp_or_pid()
>  gdb, btrace: check inferior_ptid in
>    record_btrace_target::xfer_partial()

The remaining patches

>  gdb, btrace: rename record_btrace_enable_warn()
>  gdb, btrace: switch threads in remote_btrace_maybe_reopen()
>  gdb, gdbserver: update thread identifier in enable_btrace target
>    method
>  gdb, remote, btrace: move switch_to_thread call right before xfer call

were approved.  I'm pushing those with the modifications suggested
in the review.

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

Re: [PATCH v2 0/7] gdb, thread-iter: handle null_ptid

[Simon Marchi]

On 2022-01-27 07:49, Metzger, Markus T wrote:
> Thanks for your review, Simon,
> 
> I dropped the patches that were a bit more controversial and went beyond
> fixing the bug this series intended to fix, i.e.
> 
>>  gdb, thread-iter: handle null_ptid
>>  gdb, ptid: add is_lwp() and is_lwp_or_pid()
>>  gdb, btrace: check inferior_ptid in
>>    record_btrace_target::xfer_partial()
> 
> The remaining patches
> 
>>  gdb, btrace: rename record_btrace_enable_warn()
>>  gdb, btrace: switch threads in remote_btrace_maybe_reopen()
>>  gdb, gdbserver: update thread identifier in enable_btrace target
>>    method
>>  gdb, remote, btrace: move switch_to_thread call right before xfer call
> 
> were approved.  I'm pushing those with the modifications suggested
> in the review.

Thanks for fixing this.

Simon