From patchwork Fri Jan 15 08:20:05 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Kaixu Xia X-Patchwork-Id: 59847 Delivered-To: patch@linaro.org Received: by 10.112.130.2 with SMTP id oa2csp388180lbb; Fri, 15 Jan 2016 00:20:43 -0800 (PST) X-Received: by 10.98.12.21 with SMTP id u21mr12965616pfi.36.1452846043480; Fri, 15 Jan 2016 00:20:43 -0800 (PST) Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id sk6si14959234pab.138.2016.01.15.00.20.43; Fri, 15 Jan 2016 00:20:43 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756021AbcAOIUl (ORCPT + 29 others); Fri, 15 Jan 2016 03:20:41 -0500 Received: from szxga02-in.huawei.com ([119.145.14.65]:44881 "EHLO szxga02-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751325AbcAOIUk (ORCPT ); Fri, 15 Jan 2016 03:20:40 -0500 Received: from 172.24.1.50 (EHLO szxeml428-hub.china.huawei.com) ([172.24.1.50]) by szxrg02-dlp.huawei.com (MOS 4.3.7-GA FastPath queued) with ESMTP id CZX29371; Fri, 15 Jan 2016 16:20:20 +0800 (CST) Received: from [127.0.0.1] (10.111.101.23) by szxeml428-hub.china.huawei.com (10.82.67.183) with Microsoft SMTP Server id 14.3.235.1; Fri, 15 Jan 2016 16:20:12 +0800 Message-ID: <5698ABB5.6000207@huawei.com> Date: Fri, 15 Jan 2016 16:20:05 +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> In-Reply-To: <20160112170650.GI15737@arm.com> X-Originating-IP: [10.111.101.23] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A020201.5698ABCC.00BD, ss=1, re=0.000, recu=0.000, reip=0.000, cl=1, cld=1, fgs=0, ip=0.0.0.0, so=2013-06-18 04:22:30, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: 14b06495c08f740f693950e60fdf6ef8 Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 于 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(-) [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 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);