Subject: ptrace patch

From: Philip Blundell <[email-masked]>
Date: Mon, 12 Jun 2000 20:44:18 +0100

Any comments on this patch?  It's been in CVS at netwinder.org for some time.  
I think it's necessary for correct single-stepping.

p.

--- linux/arch/arm/kernel/ptrace.c	Sun Apr 30 11:15:42 2000
+++ linux/arch/arm/kernel/ptrace.c	Thu Mar  9 08:03:29 2000
@@ -15,6 +15,7 @@
 #include <asm/uaccess.h>
 #include <asm/pgtable.h>
 #include <asm/system.h>
+#include <asm/breakpoint.h>
 
 /*
  * does not yet catch signals sent when the child dies.
@@ -92,7 +93,6 @@ static unsigned long ptrace_getrn (struc
 	else
 		val = get_stack_long (child, reg);
 
-printk ("r%02d=%08lX ", reg, val);
 	return val;
 }
 
@@ -105,12 +105,10 @@ static unsigned long ptrace_getaluop2 (s
 	int shift;
 	int type;
 
-printk ("op2=");
 	if (insn & 1 << 25) {
 		val = insn & 255;
 		shift = (insn >> 8) & 15;
 		type = 3;
-printk ("(imm)");
 	} else {
 		val = get_stack_long (child, insn & 15);
 
@@ -120,9 +118,7 @@ printk ("(imm)");
 			shift = (insn >> 7) & 31;
 
 		type = (insn >> 5) & 3;
-printk ("(r%02ld)", insn & 15);
 	}
-printk ("sh%dx%d", type, shift);
 	switch (type) {
 	case 0:	val <<= shift;	break;
 	case 1:	val >>= shift;	break;
@@ -133,7 +129,6 @@ printk ("sh%dx%d", type, shift);
  		val = (val >> shift) | (val << (32 - shift));
 		break;
 	}
-printk ("=%08lX ", val);
 	return val;
 }
 
@@ -150,7 +145,6 @@ static unsigned long ptrace_getldrop2 (s
 	shift = (insn >> 7) & 31;
 	type = (insn >> 5) & 3;
 
-printk ("op2=r%02ldsh%dx%d", insn & 15, shift, type);
 	switch (type) {
 	case 0:	val <<= shift;	break;
 	case 1:	val >>= shift;	break;
@@ -161,7 +155,6 @@ printk ("op2=r%02ldsh%dx%d", insn & 15, 
  		val = (val >> shift) | (val << (32 - shift));
 		break;
 	}
-printk ("=%08lX ", val);
 	return val;
 }
 
@@ -170,13 +163,11 @@ get_branch_address(struct task_struct *c
 {
 	unsigned long alt = 0;
 
-printk(KERN_DEBUG "ptrace_set_bpt: insn=%08lX pc=%08lX ", insn, pc);
 	switch (insn & 0x0e100000) {
 	case 0x00000000:
 	case 0x00100000:
 	case 0x02000000:
 	case 0x02100000: /* data processing */
-		printk ("data ");
 		switch (insn & 0x01e0f000) {
 		case 0x0000f000:
 			alt = ptrace_getrn(child, insn) & ptrace_getaluop2(child, insn);
@@ -220,9 +211,8 @@ printk(KERN_DEBUG "ptrace_set_bpt: insn=
 		}
 		break;
 
-	case 0x04100000: /* ldr */
+	case 0x06100000: /* ldr */
 		if ((insn & 0xf000) == 0xf000) {
-printk ("ldr ");
 			alt = ptrace_getrn(child, insn);
 			if (insn & 1 << 24) {
 				if (insn & 1 << 23)
@@ -237,9 +227,8 @@ printk ("ldr ");
 		}
 		break;
 
-	case 0x06100000: /* ldr imm */
+	case 0x04100000: /* ldr imm */
 		if ((insn & 0xf000) == 0xf000) {
-printk ("ldrimm ");
 			alt = ptrace_getrn(child, insn);
 			if (insn & 1 << 24) {
 				if (insn & 1 << 23)
@@ -258,7 +247,6 @@ printk ("ldrimm ");
 		if (insn & (1 << 15)) {
 			unsigned long base;
 			int nr_regs;
-printk ("ldm ");
 
 			if (insn & (1 << 23)) {
 				nr_regs = insn & 65535;
@@ -291,7 +279,6 @@ printk ("ldm ");
 	case 0x0a000000:
 	case 0x0a100000: { /* bl or b */
 		signed long displ;
-printk ("b/bl ");
 		/* It's a branch/branch link: instead of trying to
 		 * figure out whether the branch will be taken or not,
 		 * we'll put a breakpoint at either location.  This is
@@ -306,7 +293,6 @@ printk ("b/bl ");
 	    }
 	    break;
 	}
-printk ("=%08lX\n", alt);
 
 	return alt;
 }
@@ -332,6 +318,8 @@ add_breakpoint(struct task_struct *child
 	return res;
 }
 
+#define predicate(x)	(x & 0xf0000000)
+
 int ptrace_set_bpt (struct task_struct *child)
 {
 	struct debug_info *dbg = &child->thread.debug;
@@ -346,22 +334,20 @@ int ptrace_set_bpt (struct task_struct *
 
 		dbg->nsaved = 0;
 
-		res = add_breakpoint(child, dbg, pc + 4);
+		alt = get_branch_address(child, pc, insn);
+		if (alt)
+			res = add_breakpoint(child, dbg, alt);
 
-		if (res == 0) {
-			alt = get_branch_address(child, pc, insn);
-			if (alt)
-				res = add_breakpoint(child, dbg, alt);
-		}
+		if (!res && (!alt || (predicate(insn) != 0xe0000000)))
+			res = add_breakpoint(child, dbg, pc + 4);
 	}
 
 	return res;
 }
 
-/* Ensure no single-step breakpoint is pending.  Returns non-zero
- * value if child was being single-stepped.
+/* Ensure no single-step breakpoint is pending.
  */
-int ptrace_cancel_bpt (struct task_struct *child)
+void ptrace_do_cancel_bpt (struct task_struct *child)
 {
 	struct debug_info *dbg = &child->thread.debug;
 	unsigned long tmp;
@@ -380,8 +366,6 @@ int ptrace_cancel_bpt (struct task_struc
 			printk(KERN_ERR "ptrace_cancel_bpt: weirdness\n");
 		write_long(child, dbg->bp[i].address, dbg->bp[i].insn);
 	}
-
-	return nsaved != 0;
 }
 
 asmlinkage int sys_ptrace(long request, long pid, long addr, long data)
@@ -492,7 +476,7 @@ asmlinkage int sys_ptrace(long request, 
 			child->exit_code = data;
 			wake_up_process (child);
 			/* make sure single-step breakpoint is gone. */
-			ptrace_cancel_bpt (child);
+			ptrace_do_cancel_bpt (child);
 			ret = 0;
 			goto out;
 
@@ -506,7 +490,7 @@ asmlinkage int sys_ptrace(long request, 
 			wake_up_process (child);
 			child->exit_code = SIGKILL;
 			/* make sure single-step breakpoint is gone. */
-			ptrace_cancel_bpt (child);
+			ptrace_do_cancel_bpt (child);
 			ret = 0;
 			goto out;
 
@@ -575,7 +559,7 @@ asmlinkage int sys_ptrace(long request, 
 			child->p_pptr = child->p_opptr;
 			SET_LINKS(child);
 			/* make sure single-step breakpoint is gone. */
-			ptrace_cancel_bpt (child);
+			ptrace_do_cancel_bpt (child);
 			ret = 0;
 			goto out;
 
--- linux/arch/arm/kernel/signal.c	Sun Jun  4 17:07:26 2000
+++ linux/arch/arm/kernel/signal.c	Sun Jun  4 17:40:15 2000
@@ -22,6 +22,7 @@
 #include <asm/ucontext.h>
 #include <asm/uaccess.h>
 #include <asm/pgalloc.h>
+#include <asm/breakpoint.h>
 
 #define _BLOCKABLE (~(sigmask(SIGKILL) | sigmask(SIGSTOP)))
 
@@ -31,8 +32,6 @@
 asmlinkage int sys_wait4(pid_t pid, unsigned long * stat_addr,
 			 int options, unsigned long *ru);
 asmlinkage int do_signal(sigset_t *oldset, struct pt_regs * regs, int syscall);
-extern int ptrace_cancel_bpt (struct task_struct *);
-extern int ptrace_set_bpt (struct task_struct *);
 
 int copy_siginfo_to_user(siginfo_t *to, siginfo_t *from)
 {
--- linux/include/asm-arm/breakpoint.h	Thu Jan  1 01:00:00 1970
+++ linux/include/asm-arm/breakpoint.h	Fri Aug  6 19:09:02 1999
@@ -0,0 +1,24 @@
+#ifndef __ASM_BREAKPOINT_H
+#define __ASM_BREAKPOINT_H
+
+extern void ptrace_do_cancel_bpt (struct task_struct *);
+extern int ptrace_set_bpt (struct task_struct *);
+
+static inline int 
+ptrace_query_bpt (struct task_struct *child)
+{
+	return (child->thread.debug.nsaved != 0);
+}
+
+static inline int
+ptrace_cancel_bpt (struct task_struct *child)
+{
+	int r;
+	if (r = ptrace_query_bpt (child), r == 0)
+		return 0;
+
+	ptrace_do_cancel_bpt (child);
+	return r;
+}
+
+#endif
--- linux/arch/arm/kernel/traps.c	Sat May 27 19:53:37 2000
+++ linux/arch/arm/kernel/traps.c	Mon Jun 12 20:41:56 2000
@@ -24,6 +24,7 @@
 #include <asm/io.h>
 #include <asm/atomic.h>
 #include <asm/pgtable.h>
+#include <asm/breakpoint.h>
 
 extern void c_backtrace (unsigned long fp, int pmode);
 extern int ptrace_cancel_bpt (struct task_struct *);

---

From: Russell King - ARM Linux Admin <[email-masked]>
Date: Mon, 12 Jun 2000 21:42:03 +0100 (BST)

Do you really have to send messages that are twice as long as they
have to be?  If this is how GPG works, then it ought to be consigned
to the dustbin.

Please fix your mailer/GPG configuration.

> I think it's necessary for correct single-stepping.

Seems to correct a problem with the ldr/ldr immediate stuff, but other
than that its just performance enhancements.
   _____
  |_____| ------------------------------------------------- ---+---+-
  |   |        Russell King       [email-masked]      --- ---
  | | | |            http://www.arm.linux.org.uk/            /  /  |
  | +-+-+                                                     --- -+-
  /   |               THE developer of ARM Linux              |+| /|\
 /  | | |                                                     ---  |
    +-+-+ -------------------------------------------------  /\\\  |

unsubscribe: body of `unsubscribe linux-arm-kernel' to [email-masked]

---

From: Philip Blundell <[email-masked]>
Date: Mon, 12 Jun 2000 21:53:55 +0100

>have to be?  If this is how GPG works, then it ought to be consigned

No, just my misconfiguration.  Sorry.

>than that its just performance enhancements.

This bit is important too:

-		if (res == 0) {

p.