* [PATCH] gdb, thread-iter: handle null_ptid @ 2021-11-19 7:23 Markus Metzger 2021-11-19 13:56 ` Simon Marchi 0 siblings, 1 reply; 9+ messages in thread From: Markus Metzger @ 2021-11-19 7:23 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gdb, thread-iter: handle null_ptid 2021-11-19 7:23 [PATCH] gdb, thread-iter: handle null_ptid Markus Metzger @ 2021-11-19 13:56 ` Simon Marchi 2021-11-22 5:59 ` Metzger, Markus T 0 siblings, 1 reply; 9+ messages in thread From: Simon Marchi @ 2021-11-19 13:56 UTC (permalink / raw) To: Markus Metzger, gdb-patches 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] gdb, thread-iter: handle null_ptid 2021-11-19 13:56 ` Simon Marchi @ 2021-11-22 5:59 ` Metzger, Markus T 2021-11-22 16:07 ` Simon Marchi 0 siblings, 1 reply; 9+ messages in thread From: Metzger, Markus T @ 2021-11-22 5:59 UTC (permalink / raw) To: Simon Marchi; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gdb, thread-iter: handle null_ptid 2021-11-22 5:59 ` Metzger, Markus T @ 2021-11-22 16:07 ` Simon Marchi 2021-11-23 14:09 ` Metzger, Markus T 0 siblings, 1 reply; 9+ messages in thread From: Simon Marchi @ 2021-11-22 16:07 UTC (permalink / raw) To: Metzger, Markus T; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] gdb, thread-iter: handle null_ptid 2021-11-22 16:07 ` Simon Marchi @ 2021-11-23 14:09 ` Metzger, Markus T 2021-11-23 17:22 ` Simon Marchi 0 siblings, 1 reply; 9+ messages in thread From: Metzger, Markus T @ 2021-11-23 14:09 UTC (permalink / raw) To: Simon Marchi; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gdb, thread-iter: handle null_ptid 2021-11-23 14:09 ` Metzger, Markus T @ 2021-11-23 17:22 ` Simon Marchi 2021-11-24 7:12 ` Metzger, Markus T 0 siblings, 1 reply; 9+ messages in thread From: Simon Marchi @ 2021-11-23 17:22 UTC (permalink / raw) To: Metzger, Markus T; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] gdb, thread-iter: handle null_ptid 2021-11-23 17:22 ` Simon Marchi @ 2021-11-24 7:12 ` Metzger, Markus T 2021-11-24 20:54 ` Simon Marchi 0 siblings, 1 reply; 9+ messages in thread From: Metzger, Markus T @ 2021-11-24 7:12 UTC (permalink / raw) To: Simon Marchi; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gdb, thread-iter: handle null_ptid 2021-11-24 7:12 ` Metzger, Markus T @ 2021-11-24 20:54 ` Simon Marchi 2021-11-25 14:57 ` Metzger, Markus T 0 siblings, 1 reply; 9+ messages in thread From: Simon Marchi @ 2021-11-24 20:54 UTC (permalink / raw) To: Metzger, Markus T; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] gdb, thread-iter: handle null_ptid 2021-11-24 20:54 ` Simon Marchi @ 2021-11-25 14:57 ` Metzger, Markus T 0 siblings, 0 replies; 9+ messages in thread From: Metzger, Markus T @ 2021-11-25 14:57 UTC (permalink / raw) To: Simon Marchi; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-11-25 14:57 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-11-19 7:23 [PATCH] gdb, thread-iter: handle null_ptid Markus Metzger 2021-11-19 13:56 ` Simon Marchi 2021-11-22 5:59 ` Metzger, Markus T 2021-11-22 16:07 ` Simon Marchi 2021-11-23 14:09 ` Metzger, Markus T 2021-11-23 17:22 ` Simon Marchi 2021-11-24 7:12 ` Metzger, Markus T 2021-11-24 20:54 ` Simon Marchi 2021-11-25 14:57 ` Metzger, Markus T
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).