| Message ID | 5698ABB5.6000207@huawei.com |
|---|---|
| State | New |
| Headers | show |
ping... 于 2016/1/15 16:20, xiakaixu 写道: > 于 2016/1/13 1:06, Will Deacon 写道: >> On Tue, Jan 05, 2016 at 01:06:15PM +0800, Wangnan (F) wrote: >>> On 2016/1/5 0:55, Will Deacon wrote: >>>> The problem seems to be that we take the debug exception before the >>>> breakpointed instruction has been executed and call perf_bp_event at >>>> that moment, so when we single-step the faulting instruction we actually >>>> step into the SIGIO handler and end up getting stuck. >>>> >>>> Your fix doesn't really address this afaict, in that you don't (can't?) >>>> handle: >>>> >>>> * A longjmp out of a signal handler >>>> * A watchpoint and a breakpoint that fire on the same instruction >>>> * User-controlled single-step from a signal handler that enables a >>>> breakpoint explicitly >>>> * Nested signals >>> >>> Please have a look at [1], which I improve test__bp_signal() to >>> check bullet 2 and 4 you mentioned above. Seems my fix is correct. >>> >>> [1] http://lkml.kernel.org/g/1451969880-14877-1-git-send-email-wangnan0@huawei.com >> >> I'm still really uneasy about this change. Pairing up the signal delivery >> with the sigreturn to keep track of the debug state is extremely fragile >> and I'm not keen on adding this logic there. I also think we need to >> track the address that the breakpoint is originally taken on so that we >> can only perform the extra sigreturn work if we're returning to the same >> instruction. Furthermore, I wouldn't want to do this for signals other >> than those generated directly by a breakpoint. >> >> An alternative would be to postpone the signal delivery until after the >> stepping has been taken care of, but that's a change in ABI and I worry >> we'll break somebody relying on the current behaviour. >> >> What exactly does x86 do? I couldn't figure it out from the code. > > Hi Will, > > I changed the signal SIGIO to SIGUSR2 according to the patch that Wang Nan > sent out about improving test__bp_signal() to check bullet 2 and 4 you mentioned. > I tested it with arm64 qemu and gdb. The single instruction execution on qemu > shows that the result is the same as the processing described in Wang Nan's patch[2]. > > I also tested the patch on x86 qemu and found that the result is the same as > arm64 qemu. > > [1] > tools/perf/tests/bp_signal.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/tools/perf/tests/bp_signal.c b/tools/perf/tests/bp_signal.c > index 1d1bb48..3046cba 100644 > --- a/tools/perf/tests/bp_signal.c > +++ b/tools/perf/tests/bp_signal.c > @@ -175,7 +175,7 @@ int test__bp_signal(int subtest __maybe_unused) > sa.sa_sigaction = (void *) sig_handler; > sa.sa_flags = SA_SIGINFO; > > - if (sigaction(SIGIO, &sa, NULL) < 0) { > + if (sigaction(SIGUSR2, &sa, NULL) < 0) { > pr_debug("failed setting up signal handler\n"); > return TEST_FAIL; > } > @@ -237,9 +237,9 @@ int test__bp_signal(int subtest __maybe_unused) > * > */ > > - fd1 = bp_event(__test_function, SIGIO); > + fd1 = bp_event(__test_function, SIGUSR2); > fd2 = bp_event(sig_handler, SIGUSR1); > - fd3 = wp_event((void *)&the_var, SIGIO); > + fd3 = wp_event((void *)&the_var, SIGUSR2); > > ioctl(fd1, PERF_EVENT_IOC_ENABLE, 0); > ioctl(fd2, PERF_EVENT_IOC_ENABLE, 0); > > [2] > * Following processing should happen: > * Exec: Action: Result: > * incq (%rdi) - fd1 event breakpoint hit -> count1 == 1 > * - SIGIO is delivered > * sig_handler - fd2 event breakpoint hit -> count2 == 1 > * - SIGUSR1 is delivered > * sig_handler_2 -> overflows_2 == 1 (nested signal) > * sys_rt_sigreturn - return from sig_handler_2 > * overflows++ -> overflows = 1 > * sys_rt_sigreturn - return from sig_handler > * incq (%rdi) - fd3 event watchpoint hit -> count3 == 1 (wp and bp in one insn) > * - SIGIO is delivered > * sig_handler - fd2 event breakpoint hit -> count2 == 2 > * - SIGUSR1 is delivered > * sig_handler_2 -> overflows_2 == 2 (nested signal) > * sys_rt_sigreturn - return from sig_handler_2 > * overflows++ -> overflows = 2 > * sys_rt_sigreturn - return from sig_handler > * the_var++ - fd3 event watchpoint hit -> count3 == 2 (standalone watchpoint) > * - SIGIO is delivered > * sig_handler - fd2 event breakpoint hit -> count2 == 3 > * - SIGUSR1 is delivered > * sig_handler_2 -> overflows_2 == 3 (nested signal) > * sys_rt_sigreturn - return from sig_handler_2 > * overflows++ -> overflows == 3 > * sys_rt_sigreturn - return from sig_handler >> >> Will >> >> . >> > >
diff --git a/tools/perf/tests/bp_signal.c b/tools/perf/tests/bp_signal.c index 1d1bb48..3046cba 100644 --- a/tools/perf/tests/bp_signal.c +++ b/tools/perf/tests/bp_signal.c @@ -175,7 +175,7 @@ int test__bp_signal(int subtest __maybe_unused) sa.sa_sigaction = (void *) sig_handler; sa.sa_flags = SA_SIGINFO; - if (sigaction(SIGIO, &sa, NULL) < 0) { + if (sigaction(SIGUSR2, &sa, NULL) < 0) { pr_debug("failed setting up signal handler\n"); return TEST_FAIL; } @@ -237,9 +237,9 @@ int test__bp_signal(int subtest __maybe_unused) * */ - fd1 = bp_event(__test_function, SIGIO); + fd1 = bp_event(__test_function, SIGUSR2); fd2 = bp_event(sig_handler, SIGUSR1); - fd3 = wp_event((void *)&the_var, SIGIO); + fd3 = wp_event((void *)&the_var, SIGUSR2); ioctl(fd1, PERF_EVENT_IOC_ENABLE, 0); ioctl(fd2, PERF_EVENT_IOC_ENABLE, 0);