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

[Pedro Alves <palves@redhat.com>]

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