Re: [PATCH 8/8] Special-case wildcard requests in ravenscar-thread.c

[Pedro Alves <palves at redhat dot com>]

On 02/09/2019 04:41 AM, Joel Brobecker wrote:
>>> ravenscar-thread.c intercepts resume and wait target requests and
>>> replaces the requested ptid with the ptid of the underlying CPU.
>>> However, this is incorrect when a request is made with a wildcard
>>> ptid.
>>
>> Can you clarify a bit more what went wrong?
>> IIRC, the base target always has one thread/cpu only, anyway?  What
>> difference does the patch make in practice?
> 
> This happens when debugging application on a multi-CPU target,
> using QEMU, where some tasks are allocated to one CPU, and others
> are allocated to the other.
> 
> When stopping, QEMU tells us about one thread per CPU, so we start
> with one possible base_ptid per CPU.  Previously, sending a resume
> order for one thread actually resumed execution on all the CPUs.
> But during an upgrade, which behavior was changed so that sending
> a resume order for one thread only wakes the corresponding CPU up.

Interesting -- that clarifies the "why changed that made this
noticeable now" question I had in my mind.  It's the sort of thing that
I think is worth it to include in the commit log.

> 
> At the user level, we noticed the issue because we had a test were
> we insert a breakpoint one some code which is only run from, say,
> CPU #2, whereas we unfortunately resumed the execution after having
> stopped somewhere in CPU #1. As a result, we sent an order to resume
> CPU #1, which starves CPU #2 forever, because the code in CPU #1
> waits for some of the Ada tasks allocated to CPU #2 (and we never
> reach our breakpoint either).

I see.  For some reason I recalled that Ravenscar didn't support SMP.

> 
>> The reason I'm wondering is because the patch makes me wonder about
>> a step request with no sched-lock, which is the default (*).
>> In that case, you'll have:
>>
>>  - the thread to step is in inferior_ptid
>>  - ptid == minus_one_ptid (indicating everything resumes / no schedlock)
>>
>> So seems like after this patch the lower layer might get a request to step
>> an unknown inferior_ptid?  Might not happen by luck/accident.
>> I'd suspect that you'd want to do instead:
>>
>>   inferior_ptid = m_base_ptid;
>>   /* If we see a wildcard resume, we simply pass that on.  Otherwise,
>>      arrange to resume the base ptid.  */
>>   if (ptid != minus_one_ptid)
>>     ptid = m_base_ptid;
>>   beneath ()->resume (ptid, step, siggnal);
>>
>> I.e., always flip inferior_ptid.

This comment still applies, I think.

BTW, the main reason I asked is because I have a change in the multi-target
branch that makes infrun switch inferior_ptid to null_ptid before
handling events, which exposed a number of bad assumptions that could lead
to accessing the wrong target.  Ravenscar may end up affected, but I'm
not certain.

>>
>> (*) since ravenscar-thread.c doesn't know how to interact with
>>     the ravenscar runtime to specify which threads are resumable,
>>     "schedlock on" most certainly doesn't work properly.  E.g.,
>>       "task 1 stops; set scheduler-locking on; task 2; step"
>>     seemingly will step task 1 instead of 2, AFAICT.
> 
> That's correct (unless if you are in the particular situation where
> you have one task per CPU).
Thanks,
Pedro Alves