Patchwork [RFC,2/2] powerpc/booke: revert PTRACE_SINGLEBLOCK to BookE behavior

login
register
mail settings
Submitter James Yang
Date July 5, 2013, 10:11 p.m.
Message ID <1373062265-4267-3-git-send-email-James.Yang@freescale.com>
Download mbox | patch
Permalink /patch/257206/
State RFC
Headers show

Comments

James Yang - July 5, 2013, 10:11 p.m.
A BookE branch taken debug exception followed by a single step does not
accurately simulate Server's branch execute debug exception.  BookE's
branch taken debug exception stops before the branch is to be executed
and only happens if the branch will actually be taken.  Server's branch
execute trace exception stops on the instruction after the branch
executes, regardless of whether or not the branch redirected the program
counter.

The existing PTRACE_SINGLEBLOCK support for BookE hardcodes a single
step after the branch taken exception is taken in order to simulate
Server's behavior, but this misses fall-through branch instructions
(i.e., branches that are NOT taken).  Also, the si_code became masked as
TRAP_TRACE instead of TRAP_BRANCH.

This patch provides native support for the BookE branch taken debug
exception's behavior:  PTRACE_SINGLEBLOCK stops with a SIGTRAP before a
branch-that-would-be-taken would execute.  Userspace software will be
able to examine the process state upon catching the SIGTRAP, and it
will need to issue a PTRACE_SINGLESTEP or PTRACE_CONT to resume program
execution past the branch.

Signed-off-by: James Yang <James.Yang@freescale.com>
---
 arch/powerpc/kernel/traps.c |   40 +++++++++++++++++++++++++++-------------
 1 files changed, 27 insertions(+), 13 deletions(-)
Benjamin Herrenschmidt - July 6, 2013, 12:21 a.m.
On Fri, 2013-07-05 at 17:11 -0500, James Yang wrote:
> A BookE branch taken debug exception followed by a single step does not
> accurately simulate Server's branch execute debug exception.  BookE's
> branch taken debug exception stops before the branch is to be executed
> and only happens if the branch will actually be taken.  Server's branch
> execute trace exception stops on the instruction after the branch
> executes, regardless of whether or not the branch redirected the program
> counter.
> 
> The existing PTRACE_SINGLEBLOCK support for BookE hardcodes a single
> step after the branch taken exception is taken in order to simulate
> Server's behavior, but this misses fall-through branch instructions
> (i.e., branches that are NOT taken).  Also, the si_code became masked as
> TRAP_TRACE instead of TRAP_BRANCH.

But that changes the user visible behaviour, won't that break gdb
expectations ?

Another way to "fix" it is to instead use lib/sstep.c to emulate the
single step maybe ?

On the other hand, I tend to think that trapping before the branch is
actually more useful especially if you don't have the CFAR register.

Cheers,
Ben.
James Yang - July 6, 2013, 5:01 a.m.
On Sat, 6 Jul 2013, Benjamin Herrenschmidt wrote:

> On Fri, 2013-07-05 at 17:11 -0500, James Yang wrote:
> > A BookE branch taken debug exception followed by a single step does not
> > accurately simulate Server's branch execute debug exception.  BookE's
> > branch taken debug exception stops before the branch is to be executed
> > and only happens if the branch will actually be taken.  Server's branch
> > execute trace exception stops on the instruction after the branch
> > executes, regardless of whether or not the branch redirected the program
> > counter.
> > 
> > The existing PTRACE_SINGLEBLOCK support for BookE hardcodes a single
> > step after the branch taken exception is taken in order to simulate
> > Server's behavior, but this misses fall-through branch instructions
> > (i.e., branches that are NOT taken).  Also, the si_code became masked as
> > TRAP_TRACE instead of TRAP_BRANCH.
> 
> But that changes the user visible behaviour, won't that break gdb
> expectations ?

I am having a difficult time finding any use of PTRACE_SINGLEBLOCK in 
any arch in the various versions of gdb source trees I downloaded.  
Would you please provide a pointer or at least a hint as to where you 
think it would be?  I don't know gdb internals at all, but grepping 
the sources for PTRACE_SINGLEBLOCK returns nothing.


> Another way to "fix" it is to instead use lib/sstep.c to emulate the
> single step maybe ?

I don't think there's any issue any more with the hard coded single 
step with the fixes that Scott and Bharat recently provided.  
Actually, I don't even know if this feature was practically useful on 
BookE before those fixes due to the hangs and non-deterministic 
behavior.

Hypothetically, sstep.c could let server to emulate BookE's behavior..


> On the other hand, I tend to think that trapping before the branch is
> actually more useful especially if you don't have the CFAR register.

And there's no exception for fall-through branches.

So, yeah, this really is the question:  are there actually any tools 
that rely on PTRACE_SINGLEBLOCK behaving in the different ways it 
currently does on the two Power subarchiectures?  For BookE targes, 
how do they handle not being able to catch fall-through branches?  
For server targets, how do they capture the old LR value for blrl 
after the branch?  (I'm guessing not even sstep emulation can provide 
this information, though it might not be practically useful.)

Patch

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index c3ceaa2..5837d7f 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -1518,12 +1518,21 @@  void __kprobes DebugException(struct pt_regs *regs, unsigned long debug_status)
 {
 	current->thread.dbsr = debug_status;
 
-	/* Hack alert: On BookE, Branch Taken stops on the branch itself, while
-	 * on server, it stops on the target of the branch. In order to simulate
-	 * the server behaviour, we thus restart right away with a single step
-	 * instead of stopping here when hitting a BT
+	/* BookE's Branch Taken Debug Exception stops on the branch iff the
+	 * branch is resolved to be taken.  No exception occurs if the branch
+	 * is not taken (no exception if the branch does not redirect the PC).
+	 * This is unlike Classic/Server's behavior where the exception occurs
+	 * after the branch executes, regardless of whether or not the branch
+	 * redirected the PC.
+	 *
+	 * The past behavior of this function was to simulate Classic/Server's
+	 * behavior by performing a single-step upon a branch taken exception.
+	 * However, the simulation is not accurate because fall-through non-
+	 * taken branches would not result in a SIGTRAP.  Also, that SIGTRAP's
+	 * si_code would be reported as a TRAP_TRACE instead of a TRAP_BRANCH.
 	 */
-	if (debug_status & DBSR_BT) {
+
+	if (debug_status & DBSR_BT) {	/* Branch Taken */
 		regs->msr &= ~MSR_DE;
 
 		/* Disable BT */
@@ -1531,20 +1540,25 @@  void __kprobes DebugException(struct pt_regs *regs, unsigned long debug_status)
 		/* Clear the BT event */
 		mtspr(SPRN_DBSR, DBSR_BT);
 
-		/* Do the single step trick only when coming from userspace */
-		if (user_mode(regs)) {
-			current->thread.dbcr0 &= ~DBCR0_BT;
-			current->thread.dbcr0 |= DBCR0_IDM | DBCR0_IC;
-			regs->msr |= MSR_DE;
-			return;
-		}
-
 		if (notify_die(DIE_SSTEP, "block_step", regs, 5,
 			       5, SIGTRAP) == NOTIFY_STOP) {
 			return;
 		}
+
 		if (debugger_sstep(regs))
 			return;
+
+		if (user_mode(regs)) {
+			current->thread.dbcr0 &= ~DBCR0_BT;
+			if (DBCR_ACTIVE_EVENTS(current->thread.dbcr0,
+					       current->thread.dbcr1))
+				regs->msr |= MSR_DE;
+			else
+				/* Make sure the IDM bit is off */
+				current->thread.dbcr0 &= ~DBCR0_IDM;
+		}
+
+		_exception(SIGTRAP, regs, TRAP_BRANCH, regs->nip);
 	} else if (debug_status & DBSR_IC) { 	/* Instruction complete */
 		regs->msr &= ~MSR_DE;