[PATCH] gdb, thread-iter: handle null_ptid

[PATCH] 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] gdb, thread-iter: handle null_ptid

[Simon Marchi]

On 2021-11-19 02:23, 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;
> 

Can you please give the backtrace when we hit that internal error?  I
don't quite understand what is happening.

Simon

RE: [PATCH] gdb, thread-iter: handle null_ptid

[Metzger, Markus T]

Hello Simon,

>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;
>>
>
>Can you please give the backtrace when we hit that internal error?  I
>don't quite understand what is happening.

Here is the backtrace:

#0  internal_error (file=0x1434ac8 "gdb/inferior.c", line=300, fmt=0x14348f3 "%s: Assertion `%s' failed.") at gdbsupport/errors.cc:54
#1  0x00000000007e6f58 in find_inferior_pid (targ=0x299a290, pid=0) at gdb/inferior.c:300
#2  0x00000000007e700e in find_inferior_ptid (targ=0x299a290, ptid=...) at gdb/inferior.c:314
#3  0x0000000000b6c718 in find_thread_ptid (targ=0x299a290, ptid=...) at gdb/thread.c:490
#4  0x0000000000b6b230 in all_matching_threads_iterator::all_matching_threads_iterator (this=0x7fffffffbef8, filter_target=0x299a290, filter_ptid=...) at gdb/thread-iter.c:125
#5  0x0000000000546422 in filtered_iterator<all_matching_threads_iterator, non_exited_thread_filter>::filtered_iterator<process_stratum_target* const&, ptid_t const&> (this=0x7fffffffbef0) at gdb/../gdbsupport/filtered-iterator.h:42
#6  0x0000000000544eb5 in all_non_exited_threads_range::begin (this=0x7fffffffbed0) at gdb/thread-iter.h:243
#7  0x00000000009d0e7f in record_btrace_target::record_is_replaying (this=0x2441680 <record_btrace_ops>, ptid=...) at gdb/record-btrace.c:1411
#8  0x00000000009d0f92 in record_btrace_target::xfer_partial (this=0x2441680 <record_btrace_ops>, object=TARGET_OBJECT_BTRACE_CONF, annex=0x148cade "", readbuf=0x2eef9e0 "\320\360B\367\377\177", writebuf=0x0, offset=0, len=4096, xfered_len=0x7fffffffc168) at gdb/record-btrace.c:1437
#9  0x0000000000b50c63 in target_xfer_partial (ops=0x2441680 <record_btrace_ops>, object=TARGET_OBJECT_BTRACE_CONF, annex=0x148cade "", readbuf=0x2eef9e0 "\320\360B\367\377\177", writebuf=0x0, offset=0, len=4096, xfered_len=0x7fffffffc168) at gdb/target.c:1740
#10 0x0000000000b513d4 in target_read_partial (ops=0x2441680 <record_btrace_ops>, object=TARGET_OBJECT_BTRACE_CONF, annex=0x148cade "", buf=0x2eef9e0 "\320\360B\367\377\177", offset=0, len=4096, xfered_len=0x7fffffffc168) at gdb/target.c:1974
#11 0x0000000000b65940 in target_read_alloc_1<char> (ops=0x2441680 <record_btrace_ops>, object=TARGET_OBJECT_BTRACE_CONF, annex=0x148cade "") at gdb/target.c:2309
#12 0x0000000000b51d27 in target_read_stralloc (ops=0x2441680 <record_btrace_ops>, object=TARGET_OBJECT_BTRACE_CONF, annex=0x148cade "") at gdb/target.c:2348
#13 0x0000000000a0f881 in btrace_read_config (conf=0x2a6f5e8) at gdb/remote.c:14048
#14 0x0000000000a0fdbc in remote_target::enable_btrace (this=0x299a290, ptid=..., conf=0x261ae90 <record_btrace_conf>) at gdb/remote.c:14162
#15 0x0000000000b6067c in target_ops::enable_btrace (this=0x2441680 <record_btrace_ops>, arg0=..., arg1=0x261ae90 <record_btrace_conf>) at gdb/target-delegates.c:3789
#16 0x0000000000b63a88 in target_enable_btrace (ptid=..., conf=0x261ae90 <record_btrace_conf>) at gdb/target.c:4042
#17 0x000000000055f39c in btrace_enable (tp=0x2a6f320, conf=0x261ae90 <record_btrace_conf>) at gdb/btrace.c:1621
#18 0x00000000009ce714 in record_btrace_enable_warn (tp=0x2a6f320) at gdb/record-btrace.c:294
#19 0x00000000008ac4f8 in std::__invoke_impl<void, void (*&)(thread_info*), thread_info*> (__f=@0x2cfc3f8: 0x9ce6d3 <record_btrace_enable_warn(thread_info*)>) at /usr/include/c++/11/bits/invoke.h:61
#20 0x00000000008ab125 in std::__invoke_r<void, void (*&)(thread_info*), thread_info*> (__fn=@0x2cfc3f8: 0x9ce6d3 <record_btrace_enable_warn(thread_info*)>) at /usr/include/c++/11/bits/invoke.h:111
#21 0x00000000008aa8b0 in std::_Function_handler<void (thread_info*), void (*)(thread_info*)>::_M_invoke(std::_Any_data const&, thread_info*&&) (__functor=..., __args#0=@0x7fffffffc540: 0x2a6f320) at /usr/include/c++/11/bits/std_function.h:291
#22 0x0000000000b7304b in std::function<void (thread_info*)>::operator()(thread_info*) const (this=0x2cfc3f8, __args#0=0x2a6f320) at /usr/include/c++/11/bits/std_function.h:560
#23 0x0000000000b71fa2 in gdb::observers::observable<thread_info*>::notify (this=0x2619e60 <gdb::observers::new_thread>, args#0=0x2a6f320) at gdb/../gdbsupport/observable.h:150
#24 0x0000000000b6bb77 in add_thread_silent (targ=0x299a290, ptid=...) at gdb/thread.c:263
#25 0x0000000000b6bba7 in add_thread_with_info (targ=0x299a290, ptid=..., priv=0x0) at gdb/thread.c:272
#26 0x0000000000b6bc7b in add_thread (targ=0x299a290, ptid=...) at gdb/thread.c:286
#27 0x00000000009f556c in remote_target::remote_add_thread (this=0x299a290, ptid=..., running=false, executing=false) at gdb/remote.c:2543
#28 0x00000000009f5837 in remote_target::remote_notice_new_inferior (this=0x299a290, currthread=..., executing=false) at gdb/remote.c:2632
#29 0x0000000000a01664 in remote_target::process_stop_reply (this=0x299a290, stop_reply=0x2c38720, status=0x7fffffffd398) at gdb/remote.c:8055
#30 0x0000000000a01df6 in remote_target::wait_as (this=0x299a290, ptid=..., status=0x7fffffffd398, options=...) at gdb/remote.c:8238
#31 0x0000000000a021f2 in remote_target::wait (this=0x299a290, ptid=..., status=0x7fffffffd398, options=...) at gdb/remote.c:8321
#32 0x00000000009d34c4 in record_btrace_target::wait (this=0x2441680 <record_btrace_ops>, ptid=..., status=0x7fffffffd398, options=...) at gdb/record-btrace.c:2545
#33 0x0000000000b52565 in target_wait (ptid=..., status=0x7fffffffd398, options=...) at gdb/target.c:2608
#34 0x00000000007f3a33 in do_target_wait_1 (inf=0x26b5750, ptid=..., status=0x7fffffffd398, options=...) at gdb/infrun.c:3635
#35 0x00000000007f3bd9 in operator() (__closure=0x7fffffffd1d0, inf=0x26b5750) at gdb/infrun.c:3696
#36 0x00000000007f3ee9 in do_target_wait (ecs=0x7fffffffd370, options=...) at gdb/infrun.c:3715
#37 0x00000000007f4c14 in fetch_inferior_event () at gdb/infrun.c:4060
#38 0x00000000007d7be5 in inferior_event_handler (event_type=INF_REG_EVENT) at gdb/inf-loop.c:41
#39 0x0000000000a10754 in remote_async_serial_handler (scb=0x2a49560, context=0x299a2b8) at gdb/remote.c:14417
#40 0x0000000000aad80d in run_async_handler_and_reschedule (scb=0x2a49560) at gdb/ser-base.c:138
#41 0x0000000000aad8f0 in fd_event (error=0, context=0x2a49560) at gdb/ser-base.c:189
#42 0x000000000131287a in handle_file_event (file_ptr=0x29e4220, ready_mask=1) at gdbsupport/event-loop.cc:575
#43 0x0000000001312e19 in gdb_wait_for_event (block=1) at gdbsupport/event-loop.cc:701
#44 0x0000000001311db5 in gdb_do_one_event () at gdbsupport/event-loop.cc:237
#45 0x0000000000b7b47c in wait_sync_command_done () at gdb/top.c:524
#46 0x0000000000b7b505 in maybe_wait_sync_command_done (was_sync=0) at gdb/top.c:541
#47 0x0000000000b7baf9 in execute_command (p=0x7fffffffe28c "", from_tty=1) at gdb/top.c:672
#48 0x0000000000879e7d in catch_command_errors (command=0xb7b51e <execute_command(char const*, int)>, arg=0x7fffffffe288 "cont", from_tty=1, do_bp_actions=true) at gdb/main.c:523
#49 0x000000000087a03c in execute_cmdargs (cmdarg_vec=0x7fffffffda80, file_type=CMDARG_FILE, cmd_type=CMDARG_COMMAND, ret=0x7fffffffda5c) at gdb/main.c:618
#50 0x000000000087b3ff in captured_main_1 (context=0x7fffffffdcc0) at gdb/main.c:1317
#51 0x000000000087b5fd in captured_main (data=0x7fffffffdcc0) at gdb/main.c:1338
#52 0x000000000087b668 in gdb_main (args=0x7fffffffdcc0) at gdb/main.c:1363
#53 0x000000000041695d in main (argc=10, argv=0x7fffffffddd8) at gdb/gdb.c:32

Regards,
Markus.

>-----Original Message-----
>From: Simon Marchi <simon.marchi@polymtl.ca>
>Sent: Friday, November 19, 2021 2:57 PM
>To: Metzger, Markus T <markus.t.metzger@intel.com>; gdb-
>patches@sourceware.org
>Subject: Re: [PATCH] gdb, thread-iter: handle null_ptid
>
>
>
>On 2021-11-19 02:23, 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;
>>
>
>Can you please give the backtrace when we hit that internal error?  I
>don't quite understand what is happening.
>
>Simon
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] gdb, thread-iter: handle null_ptid

[Simon Marchi]

On 2021-11-22 12:59 a.m., Metzger, Markus T wrote:
> Here is the backtrace:
>
> #0  internal_error (file=0x1434ac8 "gdb/inferior.c", line=300, fmt=0x14348f3 "%s: Assertion `%s' failed.") at gdbsupport/errors.cc:54
> #1  0x00000000007e6f58 in find_inferior_pid (targ=0x299a290, pid=0) at gdb/inferior.c:300
> #2  0x00000000007e700e in find_inferior_ptid (targ=0x299a290, ptid=...) at gdb/inferior.c:314
> #3  0x0000000000b6c718 in find_thread_ptid (targ=0x299a290, ptid=...) at gdb/thread.c:490
> #4  0x0000000000b6b230 in all_matching_threads_iterator::all_matching_threads_iterator (this=0x7fffffffbef8, filter_target=0x299a290, filter_ptid=...) at gdb/thread-iter.c:125
> #5  0x0000000000546422 in filtered_iterator<all_matching_threads_iterator, non_exited_thread_filter>::filtered_iterator<process_stratum_target* const&, ptid_t const&> (this=0x7fffffffbef0) at gdb/../gdbsupport/filtered-iterator.h:42
> #6  0x0000000000544eb5 in all_non_exited_threads_range::begin (this=0x7fffffffbed0) at gdb/thread-iter.h:243
> #7  0x00000000009d0e7f in record_btrace_target::record_is_replaying (this=0x2441680 <record_btrace_ops>, ptid=...) at gdb/record-btrace.c:1411
> #8  0x00000000009d0f92 in record_btrace_target::xfer_partial (this=0x2441680 <record_btrace_ops>, object=TARGET_OBJECT_BTRACE_CONF, annex=0x148cade "", readbuf=0x2eef9e0 "\320\360B\367\377\177", writebuf=0x0, offset=0, len=4096, xfered_len=0x7fffffffc168) at gdb/record-btrace.c:1437
> #9  0x0000000000b50c63 in target_xfer_partial (ops=0x2441680 <record_btrace_ops>, object=TARGET_OBJECT_BTRACE_CONF, annex=0x148cade "", readbuf=0x2eef9e0 "\320\360B\367\377\177", writebuf=0x0, offset=0, len=4096, xfered_len=0x7fffffffc168) at gdb/target.c:1740
> #10 0x0000000000b513d4 in target_read_partial (ops=0x2441680 <record_btrace_ops>, object=TARGET_OBJECT_BTRACE_CONF, annex=0x148cade "", buf=0x2eef9e0 "\320\360B\367\377\177", offset=0, len=4096, xfered_len=0x7fffffffc168) at gdb/target.c:1974
> #11 0x0000000000b65940 in target_read_alloc_1<char> (ops=0x2441680 <record_btrace_ops>, object=TARGET_OBJECT_BTRACE_CONF, annex=0x148cade "") at gdb/target.c:2309
> #12 0x0000000000b51d27 in target_read_stralloc (ops=0x2441680 <record_btrace_ops>, object=TARGET_OBJECT_BTRACE_CONF, annex=0x148cade "") at gdb/target.c:2348
> #13 0x0000000000a0f881 in btrace_read_config (conf=0x2a6f5e8) at gdb/remote.c:14048
> #14 0x0000000000a0fdbc in remote_target::enable_btrace (this=0x299a290, ptid=..., conf=0x261ae90 <record_btrace_conf>) at gdb/remote.c:14162

I think your patch is just hiding the real problem.  Between frames 13
and 14, we are going from a function that takes the ptid as a parameter
to a function that takes the ptid implicitly through the inferior_ptid
global.  Something needs to temporarily set inferior_ptid.

Is there an easy way to reproduce this situation?  Is is an existing
test that fails when running against a gdbserver target board?

Simon

RE: [PATCH] gdb, thread-iter: handle null_ptid

[Metzger, Markus T]

Hello Simon,

>Is there an easy way to reproduce this situation?  Is is an existing
>test that fails when running against a gdbserver target board?

This was found by gdb.btrace/enable-new-thread.exp.


>> #10 0x0000000000b513d4 in target_read_partial (ops=0x2441680
><record_btrace_ops>, object=TARGET_OBJECT_BTRACE_CONF, annex=0x148cade
>"", buf=0x2eef9e0 "\320\360B\367\377\177", offset=0, len=4096,
>xfered_len=0x7fffffffc168) at gdb/target.c:1974
>> #11 0x0000000000b65940 in target_read_alloc_1<char> (ops=0x2441680
><record_btrace_ops>, object=TARGET_OBJECT_BTRACE_CONF, annex=0x148cade
>"") at gdb/target.c:2309
>> #12 0x0000000000b51d27 in target_read_stralloc (ops=0x2441680
><record_btrace_ops>, object=TARGET_OBJECT_BTRACE_CONF, annex=0x148cade
>"") at gdb/target.c:2348
>> #13 0x0000000000a0f881 in btrace_read_config (conf=0x2a6f5e8) at
>gdb/remote.c:14048
>> #14 0x0000000000a0fdbc in remote_target::enable_btrace (this=0x299a290,
>ptid=..., conf=0x261ae90 <record_btrace_conf>) at gdb/remote.c:14162
>
>I think your patch is just hiding the real problem.  Between frames 13
>and 14, we are going from a function that takes the ptid as a parameter
>to a function that takes the ptid implicitly through the inferior_ptid
>global.  Something needs to temporarily set inferior_ptid.

all_matching_threads_iterator() does not handle null_ptid, which leads to the
assert in find_inferior_pid().  I would say that this is a bug.  Would you agree?

Looks like there is another bug in record_btrace_enable_warn(), which should
temporarily switch to the new thread.

How about this [1]?  I'd keep it as a separate patch in addition to this one.

Regards,
Markus.

[1]
[PATCH] gdb, btrace: switch to new thread when enabling tracing

When new_thread overservers are notified, the new thread is not stopped,
yet, and inferior_ptid is stale.

Temporarily switch to the new thread in record-btrace's new_thread
observer.  Also change the function name to make it clear it is only used
as new-thread callback.
---
 gdb/record-btrace.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index 3fcfd6a4761..198306a657d 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -279,16 +279,21 @@ 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);
   if (rec != &record_btrace_ops)
     return;
 
+  /* TP hasn't stopped, yet, so INFERIOR_PTID is stale.  Temporarily set
+     INFERIOR_PTID to TP while we enable tracing for TP.  */
+  scoped_restore_current_thread restore_thread;
+  switch_to_thread (tp);
+
   try
     {
       btrace_enable (tp, &record_btrace_conf);
@@ -306,7 +311,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] gdb, thread-iter: handle null_ptid

[Simon Marchi]

On 2021-11-23 9:09 a.m., Metzger, Markus T wrote:
> Hello Simon,
>
>> Is there an easy way to reproduce this situation?  Is is an existing
>> test that fails when running against a gdbserver target board?
>
> This was found by gdb.btrace/enable-new-thread.exp.

Thanks, I can reproduce it.

>
>
>>> #10 0x0000000000b513d4 in target_read_partial (ops=0x2441680
>> <record_btrace_ops>, object=TARGET_OBJECT_BTRACE_CONF, annex=0x148cade
>> "", buf=0x2eef9e0 "\320\360B\367\377\177", offset=0, len=4096,
>> xfered_len=0x7fffffffc168) at gdb/target.c:1974
>>> #11 0x0000000000b65940 in target_read_alloc_1<char> (ops=0x2441680
>> <record_btrace_ops>, object=TARGET_OBJECT_BTRACE_CONF, annex=0x148cade
>> "") at gdb/target.c:2309
>>> #12 0x0000000000b51d27 in target_read_stralloc (ops=0x2441680
>> <record_btrace_ops>, object=TARGET_OBJECT_BTRACE_CONF, annex=0x148cade
>> "") at gdb/target.c:2348
>>> #13 0x0000000000a0f881 in btrace_read_config (conf=0x2a6f5e8) at
>> gdb/remote.c:14048
>>> #14 0x0000000000a0fdbc in remote_target::enable_btrace (this=0x299a290,
>> ptid=..., conf=0x261ae90 <record_btrace_conf>) at gdb/remote.c:14162
>>
>> I think your patch is just hiding the real problem.  Between frames 13
>> and 14, we are going from a function that takes the ptid as a parameter
>> to a function that takes the ptid implicitly through the inferior_ptid
>> global.  Something needs to temporarily set inferior_ptid.
>
> all_matching_threads_iterator() does not handle null_ptid, which leads to the
> assert in find_inferior_pid().  I would say that this is a bug.  Would you agree?

No, unless proven otherwise, I don't think all_matching_threads_iterator
has to handle null_ptid.  I would make it assert that it doesn't receive
null_ptid in fact.  By making it handle null_ptid, making it yield an
empty range, I think it will more likely just hide bugs such as the one
you've found.  I would rather have it catch the bugs, so we can fix
them.

> Looks like there is another bug in record_btrace_enable_warn(), which should
> temporarily switch to the new thread.

I'm not sure.  record_btrace_enable_warn only calls btrace_enable, which
takes the thread as a parameter.  So we can consider that
record_btrace_enable_warn doesn't rely on inferior_ptid, and neither
does btrace_enable.  record_btrace_enable_warn doesn't have to set
inferior_ptid before calling btrace_enable, as it passes that
information through a parameter.

> How about this [1]?  I'd keep it as a separate patch in addition to this one.

The only fix I see for now is to have remote_target::enable_btrace
use switch_to_thread before calling btrace_read_config.  Or, as I did in
the patch below, add a thread_info parameter to btrace_read_config and
make it use switch_to_thread before calling target_read_stralloc.  This
fixes the gdb.btrace/enable-new-thread.exp failure for me.

From 8b03d464d057e5395b780a68eaa9c781d54571ce Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@efficios.com>
Date: Tue, 23 Nov 2021 11:23:47 -0500
Subject: [PATCH] fix

Change-Id: Ib9c232ef3c848e94e015420fb7f2ee81a0490a2f
---
 gdb/remote.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 724386e09164..150001a112b7 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -14037,12 +14037,17 @@ 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 THREAD'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 *thread, btrace_config *conf)
 {
+  /* target_read_stralloc relies on inferior_ptid and the current inferior's
+     target stack being the right one.  */
+  scoped_restore_current_thread restore_thread;
+  switch_to_thread (thread);
+
   gdb::optional<gdb::char_vector> xml
     = target_read_stralloc (current_inferior ()->top_target (),
 			    TARGET_OBJECT_BTRACE_CONF, "");
@@ -14073,7 +14078,7 @@ remote_target::remote_btrace_maybe_reopen ()
       set_general_thread (tp->ptid);

       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;
@@ -14159,7 +14164,9 @@ remote_target::enable_btrace (ptid_t ptid, const struct btrace_config *conf)
      tracing itself is not impacted.  */
   try
     {
-      btrace_read_config (&tinfo->conf);
+      thread_info *thread = find_thread_ptid (this, ptid);
+
+      btrace_read_config (thread, &tinfo->conf);
     }
   catch (const gdb_exception_error &err)
     {
-- 
2.26.2

RE: [PATCH] gdb, thread-iter: handle null_ptid

[Metzger, Markus T]

Hello Simon,

>> all_matching_threads_iterator() does not handle null_ptid, which leads to the
>> assert in find_inferior_pid().  I would say that this is a bug.  Would you agree?
>
>No, unless proven otherwise, I don't think all_matching_threads_iterator
>has to handle null_ptid.  I would make it assert that it doesn't receive
>null_ptid in fact.  By making it handle null_ptid, making it yield an
>empty range, I think it will more likely just hide bugs such as the one
>you've found.

So you're saying that the old implementation of all_matching_threads_iterator()
was buggy by not asserting ptid != null_ptid and that

    0618ae41497 gdb: optimize all_matching_threads_iterator

fixed that bug?

If we just looked at the functionality that all_matching_threads_iterator() provides,
I'd say it is perfectly reasonable to end up with an empty list.  You'd get the same
if you used a stale ptid, or a process ptid w/o any threads.

IMHO the assert should be at a higher layer.  This would have hidden the other
bug, I agree, but it makes things more complicated and more difficult to expand,
if the basic functions assert assumptions on how they are being used.


>> Looks like there is another bug in record_btrace_enable_warn(), which should
>> temporarily switch to the new thread.
>
>I'm not sure.  record_btrace_enable_warn only calls btrace_enable, which
>takes the thread as a parameter.  So we can consider that
>record_btrace_enable_warn doesn't rely on inferior_ptid, and neither
>does btrace_enable.  record_btrace_enable_warn doesn't have to set
>inferior_ptid before calling btrace_enable, as it passes that
>information through a parameter.

My reasoning was that record_btrace_enable_warn() is the new-thread
notification handler and this is where we may see threads without a matching
inferior_ptid update.

Your direction is more consistent with previous discussions, however.  We had
a similar discussion on the exact same topic in July:
https://sourceware.org/pipermail/gdb-patches/2021-July/180969.html.

It was another instance where

    0618ae41497 gdb: optimize all_matching_threads_iterator

resulted in the same assertion.  At that time, Pedro's guidance was to put those

>+  scoped_restore_current_thread restore_thread;
>+  switch_to_thread (thread);

at the boundary where we switch from TP argument to implicitly using inferior_ptid.
If we did that consistently within GDB, the entire code would be full of them;-)
 

>-/* Read the current thread's btrace configuration from the target and
>-   store it into CONF.  */
>+/* Read THREAD'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 *thread, btrace_config *conf)
> {
>+  /* target_read_stralloc relies on inferior_ptid and the current inferior's
>+     target stack being the right one.  */
>+  scoped_restore_current_thread restore_thread;
>+  switch_to_thread (thread);
>+
>   gdb::optional<gdb::char_vector> xml
>     = target_read_stralloc (current_inferior ()->top_target (),
> 			    TARGET_OBJECT_BTRACE_CONF, "");
>@@ -14073,7 +14078,7 @@ remote_target::remote_btrace_maybe_reopen ()
>       set_general_thread (tp->ptid);
>
>       memset (&rs->btrace_config, 0x00, sizeof (struct btrace_config));
>-      btrace_read_config (&rs->btrace_config);
>+      btrace_read_config (tp, &rs->btrace_config);

There's no need to pass the btrace config if we pass TP.  We should also leave
the memset to btrace_read_config() in that case.

There's not enough context in the diff.  Here's some more:

  scoped_restore_current_thread restore_thread;

  for (thread_info *tp : all_non_exited_threads (this))
    {
      set_general_thread (tp->ptid);

      memset (&rs->btrace_config, 0x00, sizeof (struct btrace_config));
      btrace_read_config (&rs->btrace_config);


I think the issue is that we call set_general_thread() instead of
switch_to_thread().  The former is called by xfer_partial() based
on inferior_ptid.  So it looks like this code was reading the same
configuration for all threads.


>       if (rs->btrace_config.format == BTRACE_FORMAT_NONE)
> 	continue;
>@@ -14159,7 +14164,9 @@ remote_target::enable_btrace (ptid_t ptid, const
>struct btrace_config *conf)
>      tracing itself is not impacted.  */
>   try
>     {
>-      btrace_read_config (&tinfo->conf);
>+      thread_info *thread = find_thread_ptid (this, ptid);

This is awkward given that we actually start with a thread_info * in btrace_enable().

There's too much back-and-forth between thread_info * and ptid_t.

>+
>+      btrace_read_config (thread, &tinfo->conf);
>     }
>   catch (const gdb_exception_error &err)
>     {
>--
>2.26.2
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] gdb, thread-iter: handle null_ptid

[Simon Marchi]

On 2021-11-24 2:12 a.m., Metzger, Markus T wrote:
> Hello Simon,
>
>>> all_matching_threads_iterator() does not handle null_ptid, which leads to the
>>> assert in find_inferior_pid().  I would say that this is a bug.  Would you agree?
>>
>> No, unless proven otherwise, I don't think all_matching_threads_iterator
>> has to handle null_ptid.  I would make it assert that it doesn't receive
>> null_ptid in fact.  By making it handle null_ptid, making it yield an
>> empty range, I think it will more likely just hide bugs such as the one
>> you've found.
>
> So you're saying that the old implementation of all_matching_threads_iterator()
> was buggy by not asserting ptid != null_ptid and that
>
>     0618ae41497 gdb: optimize all_matching_threads_iterator
>
> fixed that bug?

We could see it like that, yes, although it was not intentional.  It
*could* be useful to be able to pass null_ptid to
all_matching_threads_iterator as a filter that matches nothing, and
therefore that yields an empty thread range.  But I haven't encountered
a situation where that would be useful.  In the case you have shown,
passing null_ptid to all_matching_threads_iterator was the result of a
bug, it was not intentional.  It sounds more useful to me to assert that
filter_ptid is not null_ptid, as a way to catch such bugs, than to
silently allow it.

> If we just looked at the functionality that all_matching_threads_iterator() provides,
> I'd say it is perfectly reasonable to end up with an empty list.  You'd get the same
> if you used a stale ptid, or a process ptid w/o any threads.
>
> IMHO the assert should be at a higher layer.  This would have hidden the other
> bug, I agree, but it makes things more complicated and more difficult to expand,
> if the basic functions assert assumptions on how they are being used.

So in this case, does this mean you would add an assert in
record_btrace_target::record_is_replaying, which was the last frame
before all_matching_threads_iterator?

>>> Looks like there is another bug in record_btrace_enable_warn(), which should
>>> temporarily switch to the new thread.
>>
>> I'm not sure.  record_btrace_enable_warn only calls btrace_enable, which
>> takes the thread as a parameter.  So we can consider that
>> record_btrace_enable_warn doesn't rely on inferior_ptid, and neither
>> does btrace_enable.  record_btrace_enable_warn doesn't have to set
>> inferior_ptid before calling btrace_enable, as it passes that
>> information through a parameter.
>
> My reasoning was that record_btrace_enable_warn() is the new-thread
> notification handler and this is where we may see threads without a matching
> inferior_ptid update.

If it only calls functions that don't rely on inferior_ptid, I don't
think this function should care about inferior_ptid.  As you said below,
the scheme we follow is to put the switch/restore at the frontier.


> Your direction is more consistent with previous discussions, however.  We had
> a similar discussion on the exact same topic in July:
> https://sourceware.org/pipermail/gdb-patches/2021-July/180969.html.
>
> It was another instance where
>
>     0618ae41497 gdb: optimize all_matching_threads_iterator
>
> resulted in the same assertion.  At that time, Pedro's guidance was to put those
>
>> +  scoped_restore_current_thread restore_thread;
>> +  switch_to_thread (thread);
>
> at the boundary where we switch from TP argument to implicitly using inferior_ptid.
> If we did that consistently within GDB, the entire code would be full of them;-)

Yeah.  I totally agree that this is not ideal.  I would be really happy
if we did not have this global state.  But as long as we have it, I
think this gives a consistent and easy to follow rule to determine where
the switch must happen.  By keeping those calls at the frontier of "does
not care about inferior_ptid" and "cares about inferior_ptid", we can
slowly converge towards less things that care about inferior_ptid.  For
example, change the callee to take a thread_info by parameter, and move
the switch/restore down in the callee.  Repeat until you reach the leaf
functions (more easily said than done, but that's the idea).

>> -/* Read the current thread's btrace configuration from the target and
>> -   store it into CONF.  */
>> +/* Read THREAD'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 *thread, btrace_config *conf)
>> {
>> +  /* target_read_stralloc relies on inferior_ptid and the current inferior's
>> +     target stack being the right one.  */
>> +  scoped_restore_current_thread restore_thread;
>> +  switch_to_thread (thread);
>> +
>>   gdb::optional<gdb::char_vector> xml
>>     = target_read_stralloc (current_inferior ()->top_target (),
>> 			    TARGET_OBJECT_BTRACE_CONF, "");
>> @@ -14073,7 +14078,7 @@ remote_target::remote_btrace_maybe_reopen ()
>>       set_general_thread (tp->ptid);
>>
>>       memset (&rs->btrace_config, 0x00, sizeof (struct btrace_config));
>> -      btrace_read_config (&rs->btrace_config);
>> +      btrace_read_config (tp, &rs->btrace_config);
>
> There's no need to pass the btrace config if we pass TP.  We should also leave
> the memset to btrace_read_config() in that case.

Hmm but we write the config into rs->btrace_config, isn't that
important?

>
> There's not enough context in the diff.  Here's some more:
>
>   scoped_restore_current_thread restore_thread;
>
>   for (thread_info *tp : all_non_exited_threads (this))
>     {
>       set_general_thread (tp->ptid);
>
>       memset (&rs->btrace_config, 0x00, sizeof (struct btrace_config));
>       btrace_read_config (&rs->btrace_config);
>
>
> I think the issue is that we call set_general_thread() instead of
> switch_to_thread().  The former is called by xfer_partial() based
> on inferior_ptid.  So it looks like this code was reading the same
> configuration for all threads.

I see.  So we can remove the set_general_thread call here.

>>       if (rs->btrace_config.format == BTRACE_FORMAT_NONE)
>> 	continue;
>> @@ -14159,7 +14164,9 @@ remote_target::enable_btrace (ptid_t ptid, const
>> struct btrace_config *conf)
>>      tracing itself is not impacted.  */
>>   try
>>     {
>> -      btrace_read_config (&tinfo->conf);
>> +      thread_info *thread = find_thread_ptid (this, ptid);
>
> This is awkward given that we actually start with a thread_info * in btrace_enable().
>
> There's too much back-and-forth between thread_info * and ptid_t.

If we can pass down the thread_info directly, that would be fine with
me.

Simon

RE: [PATCH] gdb, thread-iter: handle null_ptid

[Metzger, Markus T]

Hello Simon,

>> IMHO the assert should be at a higher layer.  This would have hidden the other
>> bug, I agree, but it makes things more complicated and more difficult to
>expand,
>> if the basic functions assert assumptions on how they are being used.
>
>So in this case, does this mean you would add an assert in
>record_btrace_target::record_is_replaying, which was the last frame
>before all_matching_threads_iterator?

That wouldn't be the right place, either.  Also, I noticed that ...

bool
record_btrace_target::record_is_replaying (ptid_t ptid)
{
  process_stratum_target *proc_target = current_inferior ()->process_target ();

... this is making assumptions about the global state, as well.

  for (thread_info *tp : all_non_exited_threads (proc_target, ptid))
    if (btrace_is_replaying (tp))
      return true;

  return false;
}

I think the right thing to do here is to check null_ptid and find the inferior
of PTID - or pass the process target as argument since we now need it
for all_non_exited_threads().  This pattern is all over the place, though.

Regarding the assert() it should be where we actually rely on INFERIOR_PTID
to specify a thread or a process, e.g. record_btrace_target::xfer_partial().  We'd
probably also want to assert that INFERIOR_PTID is a single thread or a process
depending on what we want to transfer.

I added that as an additional check since we forward most of the requests to
the target beneath and I did not want to assert here based on assumptions what
the target beneath might require.


>> Your direction is more consistent with previous discussions, however.  We had
>> a similar discussion on the exact same topic in July:
>> https://sourceware.org/pipermail/gdb-patches/2021-July/180969.html.
>>
>> It was another instance where
>>
>>     0618ae41497 gdb: optimize all_matching_threads_iterator
>>
>> resulted in the same assertion.  At that time, Pedro's guidance was to put those
>>
>>> +  scoped_restore_current_thread restore_thread;
>>> +  switch_to_thread (thread);
>>
>> at the boundary where we switch from TP argument to implicitly using
>inferior_ptid.
>> If we did that consistently within GDB, the entire code would be full of them;-)
>
>Yeah.  I totally agree that this is not ideal.  I would be really happy
>if we did not have this global state.  But as long as we have it, I
>think this gives a consistent and easy to follow rule to determine where
>the switch must happen.  By keeping those calls at the frontier of "does
>not care about inferior_ptid" and "cares about inferior_ptid", we can
>slowly converge towards less things that care about inferior_ptid.  For
>example, change the callee to take a thread_info by parameter, and move
>the switch/restore down in the callee.  Repeat until you reach the leaf
>functions (more easily said than done, but that's the idea).

Sounds good.  Sound like a lot of work, too.


>>> -/* Read the current thread's btrace configuration from the target and
>>> -   store it into CONF.  */
>>> +/* Read THREAD'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 *thread, btrace_config *conf)
>>> {
>>> +  /* target_read_stralloc relies on inferior_ptid and the current inferior's
>>> +     target stack being the right one.  */
>>> +  scoped_restore_current_thread restore_thread;
>>> +  switch_to_thread (thread);
>>> +
>>>   gdb::optional<gdb::char_vector> xml
>>>     = target_read_stralloc (current_inferior ()->top_target (),
>>> 			    TARGET_OBJECT_BTRACE_CONF, "");
>>> @@ -14073,7 +14078,7 @@ remote_target::remote_btrace_maybe_reopen ()
>>>       set_general_thread (tp->ptid);
>>>
>>>       memset (&rs->btrace_config, 0x00, sizeof (struct btrace_config));
>>> -      btrace_read_config (&rs->btrace_config);
>>> +      btrace_read_config (tp, &rs->btrace_config);
>>
>> There's no need to pass the btrace config if we pass TP.  We should also leave
>> the memset to btrace_read_config() in that case.
>
>Hmm but we write the config into rs->btrace_config, isn't that
>important?

Ouch.  I confused it with TP's btrace_thread_config, which btrace_read_config()
could have gotten itself from the new TP argument.

We copy the RS' config into TP's a few lines below:

      tp->btrace.target = XCNEW (struct btrace_target_info);
      tp->btrace.target->ptid = tp->ptid;
      tp->btrace.target->conf = rs->btrace_config;

The compiler would have caught it since those are different types.


>> There's not enough context in the diff.  Here's some more:
>>
>>   scoped_restore_current_thread restore_thread;
>>
>>   for (thread_info *tp : all_non_exited_threads (this))
>>     {
>>       set_general_thread (tp->ptid);
>>
>>       memset (&rs->btrace_config, 0x00, sizeof (struct btrace_config));
>>       btrace_read_config (&rs->btrace_config);
>>
>>
>> I think the issue is that we call set_general_thread() instead of
>> switch_to_thread().  The former is called by xfer_partial() based
>> on inferior_ptid.  So it looks like this code was reading the same
>> configuration for all threads.
>
>I see.  So we can remove the set_general_thread call here.

Yes.  And replace it with switch_to_thread().


>>>       if (rs->btrace_config.format == BTRACE_FORMAT_NONE)
>>> 	continue;
>>> @@ -14159,7 +14164,9 @@ remote_target::enable_btrace (ptid_t ptid, const
>>> struct btrace_config *conf)
>>>      tracing itself is not impacted.  */
>>>   try
>>>     {
>>> -      btrace_read_config (&tinfo->conf);
>>> +      thread_info *thread = find_thread_ptid (this, ptid);
>>
>> This is awkward given that we actually start with a thread_info * in
>btrace_enable().
>>
>> There's too much back-and-forth between thread_info * and ptid_t.
>
>If we can pass down the thread_info directly, that would be fine with
>me.

I implemented what we discussed in a small series.  I'll test it, then send
it out in a few days.

I kept this patch at the bottom of the series (see our discussion above) but
I did not add asserts into all the right places, which, I think, would be right
where we actually rely on ptid identifying a single light-weight process or
a single full process.

If you disagree, we can drop it.  Let's discuss when I have sent it out.

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