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

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

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