This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v2] Always pass signals to the right thread.
- From: Yao Qi <yao at codesourcery dot com>
- To: Pedro Alves <palves at redhat dot com>, <gdb-patches at sourceware dot org>
- Date: Thu, 26 Jun 2014 13:45:17 +0800
- Subject: Re: [PATCH v2] Always pass signals to the right thread.
- Authentication-results: sourceware.org; auth=none
- References: <1403183341-12581-1-git-send-email-palves at redhat dot com> <53A790DB dot 40908 at codesourcery dot com> <53A9648F dot 1030006 at redhat dot com>
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 (éå)