Re: [patch 2/2] Fix watchpoints for multi-inferior #2

Pedro Alves
2012-01-24T13:20:00Z

On 01/20/2012 09:31 PM, Jan Kratochvil wrote:
> On Mon, 02 Jan 2012 20:14:23 +0100, Pedro Alves wrote:
>> Do we clear inf_data or inf_data's contents anywhere on inferior
>> exit or startup, so to not leave debug registers stale across runs?
>> (The cleanup only runs when the inferior is deleted.)
>
> Yes, it is already cleared in FSF GDB.

Good, thanks.

> Plus I think this issue is unrelated to this multi-inferiorization patch.

It would be if the multi-inferiorization would make debug registers stale,
hence my question. Please try to keep an open spirit.

>
>>> --- /dev/null
>>> +++ b/gdb/testsuite/gdb.multi/watchpoint-multi.exp
> [...]
>>> +if [is_remote target] {
>>> + # It is KFAIL.
>>> + continue
>>
>> Did you mean to turn this into a real kfail? What are the
>> gdbserver problems, btw?
>
> It is no longer KFAIL, included gdbserver fixes.
>
> The first one is for dead-loop of:
> Got an event from pending child 10373 (057f)
> Got a pending child 10373
> Got an event from pending child 10373 (057f)
> Got a pending child 10373
> because linux_wait_for_event creates creates status_pending_p and then asks
> linux_wait_for_event_1 for the next event which apparently returns the newly
> created status_pending_p so linux_wait_for_event stores it back and so on.
>
> The second fix is that despite default `set schedule-multiple off' gdbserver
> sometimes resumed all the inferiors on GDB "continue".
>
> Both cases are visible with the testcase (the first one in ~50% of runs).

Ah. Could you please split the gdbserver bits into a separate patch?
I'd like to take a good look at them, but if the watchpoint
bits proper are already in, it'd be easier. The non-gdbserver bits
look okay to me.

> /* Count the LWP's that have had events. */
>
> static int
> @@ -2107,7 +2090,14 @@ retry:
> if (thread == NULL)
> {
> struct thread_resume resume_info;
> - resume_info.thread = minus_one_ptid;
> +
> + /* Resume only a single process if requested so. */
> + if (!ptid_equal (cont_thread, minus_one_ptid)
> + && ptid_get_lwp (cont_thread) == -1)
> + resume_info.thread = cont_thread;

Just above we see:

thread = (struct thread_info *) find_inferior_id (&all_threads,
cont_thread);

/* No stepping, no signal - unless one is pending already, of course. */
if (thread == NULL)

So, cont_thread does not exist, which was the whole point of reaching
here. Therefore there's no use trying to resuming it (at first sight).

BTW, I have just recently stumbled on this:

http://sourceware.org/ml/gdb-patches/2012-01/msg00502.html

But as said, I'll need to take a better look at the gdbserver bits.

> + else
> + resume_info.thread = minus_one_ptid;
> +
> resume_info.kind = resume_continue;
> resume_info.sig = 0;
> linux_resume (&resume_info, 1);

--
Pedro Alves