From mboxrd@z Thu Jan 1 00:00:00 1970 From: xiakaixu@huawei.com (xiakaixu) Date: Thu, 21 Jan 2016 16:06:21 +0800 Subject: [PATCH v2] arm64: Store breakpoint single step state into pstate In-Reply-To: <5698ABB5.6000207@huawei.com> References: <1450860731-194418-1-git-send-email-wangnan0@huawei.com> <1450921362-198371-1-git-send-email-wangnan0@huawei.com> <20160104165535.GI1616@arm.com> <568B4F47.5080307@huawei.com> <20160112170650.GI15737@arm.com> <5698ABB5.6000207@huawei.com> Message-ID: <56A0917D.6010909@huawei.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 at 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 >> >> . >> > > -- Regards Kaixu Xia From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757805AbcAUIG5 (ORCPT ); Thu, 21 Jan 2016 03:06:57 -0500 Received: from szxga03-in.huawei.com ([119.145.14.66]:47175 "EHLO szxga03-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751329AbcAUIGy (ORCPT ); Thu, 21 Jan 2016 03:06:54 -0500 Message-ID: <56A0917D.6010909@huawei.com> Date: Thu, 21 Jan 2016 16:06:21 +0800 From: xiakaixu User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:17.0) Gecko/20130801 Thunderbird/17.0.8 MIME-Version: 1.0 To: Will Deacon CC: "Wangnan (F)" , , , , , , Fengguang Wu , Jiri Olsa Subject: Re: [PATCH v2] arm64: Store breakpoint single step state into pstate References: <1450860731-194418-1-git-send-email-wangnan0@huawei.com> <1450921362-198371-1-git-send-email-wangnan0@huawei.com> <20160104165535.GI1616@arm.com> <568B4F47.5080307@huawei.com> <20160112170650.GI15737@arm.com> <5698ABB5.6000207@huawei.com> In-Reply-To: <5698ABB5.6000207@huawei.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit X-Originating-IP: [10.111.101.23] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A090202.56A09191.00EE,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2013-05-26 15:14:31, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: c3145d60c010eff17c1b746552af5c47 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >> >> . >> > > -- Regards Kaixu Xia