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

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

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