[PATCH v2] Always pass signals to the right thread.

[Yao Qi]

On 06/24/2014 07:44 PM, Pedro Alves wrote:
> But, what about "signal 0" ?  That results in:
> 
> (gdb) signal 0
> Some of the threads other than the current that are also about to be resumed had previously stopped for signals.
> If you proceed, those signals will be delivered to the respective threads.
>   Continuing thread 3 with signal SIGUSR2.
>   Continuing thread 1 with no signal.
> Continue anyway? (y or n)
> 
> And again, that makes me wonder why "no signal" is mentioned for only
> one thread and not any other that will be resumed with no signal
> either (because it hadn't stopped with one).
> 
> We could then list what will happen to _all_ threads that will be
> resumed, with or without signal, but I _really_ don't think it's
> a good idea -- that set will usually be _much_ larger than the
> signalled threads, which will normally be just one.

Agreed.

> 
> So all in all, in the end, I still prefer my original output.

That is fine by me.

>> > 
>>> >> @@ -5190,6 +5161,10 @@ switch_back_to_stepped_thread (struct execution_control_state *ecs)
>>> >>  	 what keep_going does as well, if we call it.  */
>>> >>        ecs->event_thread->control.trap_expected = 0;
>>> >>  
>>> >> +      /* Likewise, clear the signal if it should not be passed.  */
>>> >> +      if (!signal_program[ecs->event_thread->suspend.stop_signal])
>>> >> +	ecs->event_thread->suspend.stop_signal = GDB_SIGNAL_0;
>>> >> +
>> > 
>> > I can understand this patch except this part.  Why do we need to set
>> > stop_signal here?
> We can get here with the stop signal set to GDB_SIGNAL_TRAP due
> to a breakpoint of a finished step.  If we don't clear it now,
> the next time we'll resume the target, and this thread isn't
> current, we'd pass the trap to the thread.  See below for further
> explanation.
> 

Right, without this part, I can get this:

"Program terminated with signal SIGTRAP, Trace/breakpoint trap."

>> > 
>>> >> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
>>> >> index c9677ca..8f2600d 100644
>>> >> --- a/gdb/linux-nat.c
>>> >> +++ b/gdb/linux-nat.c
>>> >> @@ -1694,8 +1694,7 @@ linux_nat_resume_callback (struct lwp_info *lp, void *except)
>>> >>        thread = find_thread_ptid (lp->ptid);
>>> >>        if (thread != NULL)
>>> >>  	{
>>> >> -	  if (signal_pass_state (thread->suspend.stop_signal))
>>> >> -	    signo = thread->suspend.stop_signal;
>>> >> +	  signo = thread->suspend.stop_signal;
>>> >>  	  thread->suspend.stop_signal = GDB_SIGNAL_0;
>>> >>  	}
> 
>> > 
>> > You didn't say much about these two changes.  Is it because we've
>> > already set thread->suspend.stop_signal to GDB_SIGNAL_0 if
>> > signal_pass_state (tp->suspend.stop_signal) is false,
> ...
> 
>> > 
>> > +  /* If this signal should not be seen by program, give it zero.
>> > +     Used for debugging signals.  */
>> > +  if (!signal_pass_state (tp->suspend.stop_signal))
>> > +    tp->suspend.stop_signal = GDB_SIGNAL_0;
>> > +
>> > 
>> > that means (tp->suspend.stop_signal == GDB_SIGNAL_0 ||
>> > signal_pass_state (tp->suspend.stop_signal)) is constantly true.
>> > so in the callees of target_resume, signal_pass_state
>> > (thread->suspend.stop_signal) is always true if
>> > thread->suspend.stop_signal isn't GDB_SIGNAL_0?
> Nope, "signal SIGFOO" should still pass SIGFOO even if
> "handle SIGFOO nopass".  So in that case, 
> signal_pass_state (thread->suspend.stop_signal) is false,
> but we should still pass stop_signal.  We assume the thread

Yeah, that is why I am confused on this....

> that was current at "signal SIGFOO" time will be resumed immediately,
> because GDB may need to e.g., step-over a breakpoint for another
> thread first.  So we store the intent to pass SIGFOO once the
> step over is finished in stop_signal.
> 
> I've added this comment:
> 
>  --- c/gdb/gdbthread.h
>  +++ w/gdb/gdbthread.h
>  @@ -135,7 +135,13 @@ struct thread_control_state
>  
>   struct thread_suspend_state
>   {
>  -  /* Last signal that the inferior received (why it stopped).  */
>  +  /* Last signal that the inferior received (why it stopped).  When
>  +     the thread is resumed, this signal is delivered.  Note: the
>  +     target should not check whether the signal is in pass state,
>  +     because the signal may have been explicitly passed with the
>  +     "signal" command, which overrides "handle nopass".  If the signal
>  +     should be suppressed, the core will take care of clearing this
>  +     before the target is resumed.  */
>     enum gdb_signal stop_signal;
>   };
> 
> Does that make things clearer ?
> 

Yes, it is clearer.  Now, "stop_signal" is the signal to be delivered
to the inferior, either from the program or from "signal" command.


>> > We want to know whether thread 1 is selected, so using "info threads 1"
>> > is simpler in the regexp pattern, like
>> > 
>> >  gdb_test "info threads 1" "\\\* 1${ws}Thread .*" "thread 1 selected"
> The reason I don't like "info threads 1" this is that if the test fails,
> when you go look at the logs, you don't see which thread was actually
> current, which tends to make debugging things a little easier.   But we
> can still simplify the patterns.  I was aiming at having no wildcards in
> the middle of the output, but that's really not necessary.
> I've simplified the patterns now.

OK.

> 
> Here's the updated patch.

Patch looks good to me.

-- 
Yao (齐尧)