[RFC][PATCH] ARM: ptrace: remove single-step emulation code

[RFC][PATCH] ARM: ptrace: remove single-step emulation code

[Will Deacon]

PTRACE_SINGLESTEP is a ptrace request designed to offer single-stepping
support to userspace when the underlying architecture has hardware
support for this operation.

On ARM, we set arch_has_single_step() to 1 and attempt to emulate hardware
single-stepping by disassembling the current instruction to determine the
next pc and placing a software breakpoint on that location.

Unfortunately this has the following problems:

1.) Only a subset of ARMv7 instructions are supported
2.) Thumb-2 is unsupported
3.) The code is not SMP safe

We could try to fix this code, but it turns out that nobody uses it anyway.
GDB, for example, uses PTRACE_POKETEXT and PTRACE_PEEKTEXT to manage
breakpoints itself and does not require any kernel assistance.

This patch removes the single-step emulation code from ptrace meaning that
the PTRACE_SINGLESTEP request will return -EIO on ARM.

Signed-off-by: Will Deacon <will.deacon@arm.com>
---
I'm posting this as an RFC to see if anybody has a good reason to keep this
code around. There's also a chance I've missed an opportunity to remove some
related code, but I think I found everything. Tested on a Versatile Express,
single-stepping in GDB worked fine.

 arch/arm/include/asm/processor.h |   12 --
 arch/arm/include/asm/ptrace.h    |    2 -
 arch/arm/include/asm/traps.h     |    1 +
 arch/arm/kernel/ptrace.c         |  383 +-------------------------------------
 arch/arm/kernel/ptrace.h         |   37 ----
 arch/arm/kernel/signal.c         |    9 -
 arch/arm/kernel/traps.c          |    1 -
 7 files changed, 2 insertions(+), 443 deletions(-)
 delete mode 100644 arch/arm/kernel/ptrace.h

diff --git a/arch/arm/include/asm/processor.h b/arch/arm/include/asm/processor.h
index 67357ba..b439b41 100644
--- a/arch/arm/include/asm/processor.h
+++ b/arch/arm/include/asm/processor.h
@@ -29,19 +29,7 @@
 #define STACK_TOP_MAX	TASK_SIZE
 #endif
 
-union debug_insn {
-	u32	arm;
-	u16	thumb;
-};
-
-struct debug_entry {
-	u32			address;
-	union debug_insn	insn;
-};
-
 struct debug_info {
-	int			nsaved;
-	struct debug_entry	bp[2];
 #ifdef CONFIG_HAVE_HW_BREAKPOINT
 	struct perf_event	*hbp[ARM_MAX_HBP_SLOTS];
 #endif
diff --git a/arch/arm/include/asm/ptrace.h b/arch/arm/include/asm/ptrace.h
index 783d50f..a8ff22b 100644
--- a/arch/arm/include/asm/ptrace.h
+++ b/arch/arm/include/asm/ptrace.h
@@ -130,8 +130,6 @@ struct pt_regs {
 
 #ifdef __KERNEL__
 
-#define arch_has_single_step()	(1)
-
 #define user_mode(regs)	\
 	(((regs)->ARM_cpsr & 0xf) == 0)
 
diff --git a/arch/arm/include/asm/traps.h b/arch/arm/include/asm/traps.h
index 1b960d5..f90756d 100644
--- a/arch/arm/include/asm/traps.h
+++ b/arch/arm/include/asm/traps.h
@@ -45,6 +45,7 @@ static inline int in_exception_text(unsigned long ptr)
 
 extern void __init early_trap_init(void);
 extern void dump_backtrace_entry(unsigned long where, unsigned long from, unsigned long frame);
+extern void ptrace_break(struct task_struct *tsk, struct pt_regs *regs);
 
 extern void *vectors_page;
 
diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index 19c6816..eace844 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -26,8 +26,6 @@
 #include <asm/system.h>
 #include <asm/traps.h>
 
-#include "ptrace.h"
-
 #define REG_PC	15
 #define REG_PSR	16
 /*
@@ -184,389 +182,12 @@ put_user_reg(struct task_struct *task, int offset, long data)
 	return ret;
 }
 
-static inline int
-read_u32(struct task_struct *task, unsigned long addr, u32 *res)
-{
-	int ret;
-
-	ret = access_process_vm(task, addr, res, sizeof(*res), 0);
-
-	return ret == sizeof(*res) ? 0 : -EIO;
-}
-
-static inline int
-read_instr(struct task_struct *task, unsigned long addr, u32 *res)
-{
-	int ret;
-
-	if (addr & 1) {
-		u16 val;
-		ret = access_process_vm(task, addr & ~1, &val, sizeof(val), 0);
-		ret = ret == sizeof(val) ? 0 : -EIO;
-		*res = val;
-	} else {
-		u32 val;
-		ret = access_process_vm(task, addr & ~3, &val, sizeof(val), 0);
-		ret = ret == sizeof(val) ? 0 : -EIO;
-		*res = val;
-	}
-	return ret;
-}
-
-/*
- * Get value of register `rn' (in the instruction)
- */
-static unsigned long
-ptrace_getrn(struct task_struct *child, unsigned long insn)
-{
-	unsigned int reg = (insn >> 16) & 15;
-	unsigned long val;
-
-	val = get_user_reg(child, reg);
-	if (reg == 15)
-		val += 8;
-
-	return val;
-}
-
-/*
- * Get value of operand 2 (in an ALU instruction)
- */
-static unsigned long
-ptrace_getaluop2(struct task_struct *child, unsigned long insn)
-{
-	unsigned long val;
-	int shift;
-	int type;
-
-	if (insn & 1 << 25) {
-		val = insn & 255;
-		shift = (insn >> 8) & 15;
-		type = 3;
-	} else {
-		val = get_user_reg (child, insn & 15);
-
-		if (insn & (1 << 4))
-			shift = (int)get_user_reg (child, (insn >> 8) & 15);
-		else
-			shift = (insn >> 7) & 31;
-
-		type = (insn >> 5) & 3;
-	}
-
-	switch (type) {
-	case 0:	val <<= shift;	break;
-	case 1:	val >>= shift;	break;
-	case 2:
-		val = (((signed long)val) >> shift);
-		break;
-	case 3:
- 		val = (val >> shift) | (val << (32 - shift));
-		break;
-	}
-	return val;
-}
-
-/*
- * Get value of operand 2 (in a LDR instruction)
- */
-static unsigned long
-ptrace_getldrop2(struct task_struct *child, unsigned long insn)
-{
-	unsigned long val;
-	int shift;
-	int type;
-
-	val = get_user_reg(child, insn & 15);
-	shift = (insn >> 7) & 31;
-	type = (insn >> 5) & 3;
-
-	switch (type) {
-	case 0:	val <<= shift;	break;
-	case 1:	val >>= shift;	break;
-	case 2:
-		val = (((signed long)val) >> shift);
-		break;
-	case 3:
- 		val = (val >> shift) | (val << (32 - shift));
-		break;
-	}
-	return val;
-}
-
-#define OP_MASK	0x01e00000
-#define OP_AND	0x00000000
-#define OP_EOR	0x00200000
-#define OP_SUB	0x00400000
-#define OP_RSB	0x00600000
-#define OP_ADD	0x00800000
-#define OP_ADC	0x00a00000
-#define OP_SBC	0x00c00000
-#define OP_RSC	0x00e00000
-#define OP_ORR	0x01800000
-#define OP_MOV	0x01a00000
-#define OP_BIC	0x01c00000
-#define OP_MVN	0x01e00000
-
-static unsigned long
-get_branch_address(struct task_struct *child, unsigned long pc, unsigned long insn)
-{
-	u32 alt = 0;
-
-	switch (insn & 0x0e000000) {
-	case 0x00000000:
-	case 0x02000000: {
-		/*
-		 * data processing
-		 */
-		long aluop1, aluop2, ccbit;
-
-	        if ((insn & 0x0fffffd0) == 0x012fff10) {
-		        /*
-			 * bx or blx
-			 */
-			alt = get_user_reg(child, insn & 15);
-			break;
-		}
-
-
-		if ((insn & 0xf000) != 0xf000)
-			break;
-
-		aluop1 = ptrace_getrn(child, insn);
-		aluop2 = ptrace_getaluop2(child, insn);
-		ccbit  = get_user_reg(child, REG_PSR) & PSR_C_BIT ? 1 : 0;
-
-		switch (insn & OP_MASK) {
-		case OP_AND: alt = aluop1 & aluop2;		break;
-		case OP_EOR: alt = aluop1 ^ aluop2;		break;
-		case OP_SUB: alt = aluop1 - aluop2;		break;
-		case OP_RSB: alt = aluop2 - aluop1;		break;
-		case OP_ADD: alt = aluop1 + aluop2;		break;
-		case OP_ADC: alt = aluop1 + aluop2 + ccbit;	break;
-		case OP_SBC: alt = aluop1 - aluop2 + ccbit;	break;
-		case OP_RSC: alt = aluop2 - aluop1 + ccbit;	break;
-		case OP_ORR: alt = aluop1 | aluop2;		break;
-		case OP_MOV: alt = aluop2;			break;
-		case OP_BIC: alt = aluop1 & ~aluop2;		break;
-		case OP_MVN: alt = ~aluop2;			break;
-		}
-		break;
-	}
-
-	case 0x04000000:
-	case 0x06000000:
-		/*
-		 * ldr
-		 */
-		if ((insn & 0x0010f000) == 0x0010f000) {
-			unsigned long base;
-
-			base = ptrace_getrn(child, insn);
-			if (insn & 1 << 24) {
-				long aluop2;
-
-				if (insn & 0x02000000)
-					aluop2 = ptrace_getldrop2(child, insn);
-				else
-					aluop2 = insn & 0xfff;
-
-				if (insn & 1 << 23)
-					base += aluop2;
-				else
-					base -= aluop2;
-			}
-			read_u32(child, base, &alt);
-		}
-		break;
-
-	case 0x08000000:
-		/*
-		 * ldm
-		 */
-		if ((insn & 0x00108000) == 0x00108000) {
-			unsigned long base;
-			unsigned int nr_regs;
-
-			if (insn & (1 << 23)) {
-				nr_regs = hweight16(insn & 65535) << 2;
-
-				if (!(insn & (1 << 24)))
-					nr_regs -= 4;
-			} else {
-				if (insn & (1 << 24))
-					nr_regs = -4;
-				else
-					nr_regs = 0;
-			}
-
-			base = ptrace_getrn(child, insn);
-
-			read_u32(child, base + nr_regs, &alt);
-			break;
-		}
-		break;
-
-	case 0x0a000000: {
-		/*
-		 * bl or b
-		 */
-		signed long displ;
-		/* 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@both locations.  This is
-		 * simpler, more reliable, and probably not a whole lot
-		 * slower than the alternative approach of emulating the
-		 * branch.
-		 */
-		displ = (insn & 0x00ffffff) << 8;
-		displ = (displ >> 6) + 8;
-		if (displ != 0 && displ != 4)
-			alt = pc + displ;
-	    }
-	    break;
-	}
-
-	return alt;
-}
-
-static int
-swap_insn(struct task_struct *task, unsigned long addr,
-	  void *old_insn, void *new_insn, int size)
-{
-	int ret;
-
-	ret = access_process_vm(task, addr, old_insn, size, 0);
-	if (ret == size)
-		ret = access_process_vm(task, addr, new_insn, size, 1);
-	return ret;
-}
-
-static void
-add_breakpoint(struct task_struct *task, struct debug_info *dbg, unsigned long addr)
-{
-	int nr = dbg->nsaved;
-
-	if (nr < 2) {
-		u32 new_insn = BREAKINST_ARM;
-		int res;
-
-		res = swap_insn(task, addr, &dbg->bp[nr].insn, &new_insn, 4);
-
-		if (res == 4) {
-			dbg->bp[nr].address = addr;
-			dbg->nsaved += 1;
-		}
-	} else
-		printk(KERN_ERR "ptrace: too many breakpoints\n");
-}
-
-/*
- * Clear one breakpoint in the user program.  We copy what the hardware
- * does and use bit 0 of the address to indicate whether this is a Thumb
- * breakpoint or an ARM breakpoint.
- */
-static void clear_breakpoint(struct task_struct *task, struct debug_entry *bp)
-{
-	unsigned long addr = bp->address;
-	union debug_insn old_insn;
-	int ret;
-
-	if (addr & 1) {
-		ret = swap_insn(task, addr & ~1, &old_insn.thumb,
-				&bp->insn.thumb, 2);
-
-		if (ret != 2 || old_insn.thumb != BREAKINST_THUMB)
-			printk(KERN_ERR "%s:%d: corrupted Thumb breakpoint at "
-				"0x%08lx (0x%04x)\n", task->comm,
-				task_pid_nr(task), addr, old_insn.thumb);
-	} else {
-		ret = swap_insn(task, addr & ~3, &old_insn.arm,
-				&bp->insn.arm, 4);
-
-		if (ret != 4 || old_insn.arm != BREAKINST_ARM)
-			printk(KERN_ERR "%s:%d: corrupted ARM breakpoint at "
-				"0x%08lx (0x%08x)\n", task->comm,
-				task_pid_nr(task), addr, old_insn.arm);
-	}
-}
-
-void ptrace_set_bpt(struct task_struct *child)
-{
-	struct pt_regs *regs;
-	unsigned long pc;
-	u32 insn;
-	int res;
-
-	regs = task_pt_regs(child);
-	pc = instruction_pointer(regs);
-
-	if (thumb_mode(regs)) {
-		printk(KERN_WARNING "ptrace: can't handle thumb mode\n");
-		return;
-	}
-
-	res = read_instr(child, pc, &insn);
-	if (!res) {
-		struct debug_info *dbg = &child->thread.debug;
-		unsigned long alt;
-
-		dbg->nsaved = 0;
-
-		alt = get_branch_address(child, pc, insn);
-		if (alt)
-			add_breakpoint(child, dbg, alt);
-
-		/*
-		 * Note that we ignore the result of setting the above
-		 * breakpoint since it may fail.  When it does, this is
-		 * not so much an error, but a forewarning that we may
-		 * be receiving a prefetch abort shortly.
-		 *
-		 * If we don't set this breakpoint here, then we can
-		 * lose control of the thread during single stepping.
-		 */
-		if (!alt || predicate(insn) != PREDICATE_ALWAYS)
-			add_breakpoint(child, dbg, pc + 4);
-	}
-}
-
-/*
- * Ensure no single-step breakpoint is pending.  Returns non-zero
- * value if child was being single-stepped.
- */
-void ptrace_cancel_bpt(struct task_struct *child)
-{
-	int i, nsaved = child->thread.debug.nsaved;
-
-	child->thread.debug.nsaved = 0;
-
-	if (nsaved > 2) {
-		printk("ptrace_cancel_bpt: bogus nsaved: %d!\n", nsaved);
-		nsaved = 2;
-	}
-
-	for (i = 0; i < nsaved; i++)
-		clear_breakpoint(child, &child->thread.debug.bp[i]);
-}
-
-void user_disable_single_step(struct task_struct *task)
-{
-	task->ptrace &= ~PT_SINGLESTEP;
-	ptrace_cancel_bpt(task);
-}
-
-void user_enable_single_step(struct task_struct *task)
-{
-	task->ptrace |= PT_SINGLESTEP;
-}
-
 /*
  * Called by kernel/ptrace.c when detaching..
  */
 void ptrace_disable(struct task_struct *child)
 {
-	user_disable_single_step(child);
+	/* Nothing to do. */
 }
 
 /*
@@ -576,8 +197,6 @@ void ptrace_break(struct task_struct *tsk, struct pt_regs *regs)
 {
 	siginfo_t info;
 
-	ptrace_cancel_bpt(tsk);
-
 	info.si_signo = SIGTRAP;
 	info.si_errno = 0;
 	info.si_code  = TRAP_BRKPT;
diff --git a/arch/arm/kernel/ptrace.h b/arch/arm/kernel/ptrace.h
deleted file mode 100644
index 3926605..0000000
--- a/arch/arm/kernel/ptrace.h
+++ /dev/null
@@ -1,37 +0,0 @@
-/*
- *  linux/arch/arm/kernel/ptrace.h
- *
- *  Copyright (C) 2000-2003 Russell King
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- */
-#include <linux/ptrace.h>
-
-extern void ptrace_cancel_bpt(struct task_struct *);
-extern void ptrace_set_bpt(struct task_struct *);
-extern void ptrace_break(struct task_struct *, struct pt_regs *);
-
-/*
- * Send SIGTRAP if we're single-stepping
- */
-static inline void single_step_trap(struct task_struct *task)
-{
-	if (task->ptrace & PT_SINGLESTEP) {
-		ptrace_cancel_bpt(task);
-		send_sig(SIGTRAP, task, 1);
-	}
-}
-
-static inline void single_step_clear(struct task_struct *task)
-{
-	if (task->ptrace & PT_SINGLESTEP)
-		ptrace_cancel_bpt(task);
-}
-
-static inline void single_step_set(struct task_struct *task)
-{
-	if (task->ptrace & PT_SINGLESTEP)
-		ptrace_set_bpt(task);
-}
diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
index 907d5a6..7709668 100644
--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -20,7 +20,6 @@
 #include <asm/unistd.h>
 #include <asm/vfp.h>
 
-#include "ptrace.h"
 #include "signal.h"
 
 #define _BLOCKABLE (~(sigmask(SIGKILL) | sigmask(SIGSTOP)))
@@ -348,8 +347,6 @@ asmlinkage int sys_sigreturn(struct pt_regs *regs)
 	if (restore_sigframe(regs, frame))
 		goto badframe;
 
-	single_step_trap(current);
-
 	return regs->ARM_r0;
 
 badframe:
@@ -383,8 +380,6 @@ asmlinkage int sys_rt_sigreturn(struct pt_regs *regs)
 	if (do_sigaltstack(&frame->sig.uc.uc_stack, NULL, regs->ARM_sp) == -EFAULT)
 		goto badframe;
 
-	single_step_trap(current);
-
 	return regs->ARM_r0;
 
 badframe:
@@ -704,8 +699,6 @@ static void do_signal(struct pt_regs *regs, int syscall)
 	if (try_to_freeze())
 		goto no_signal;
 
-	single_step_clear(current);
-
 	signr = get_signal_to_deliver(&info, &ka, regs, NULL);
 	if (signr > 0) {
 		sigset_t *oldset;
@@ -724,7 +717,6 @@ static void do_signal(struct pt_regs *regs, int syscall)
 			if (test_thread_flag(TIF_RESTORE_SIGMASK))
 				clear_thread_flag(TIF_RESTORE_SIGMASK);
 		}
-		single_step_set(current);
 		return;
 	}
 
@@ -770,7 +762,6 @@ static void do_signal(struct pt_regs *regs, int syscall)
 			sigprocmask(SIG_SETMASK, &current->saved_sigmask, NULL);
 		}
 	}
-	single_step_set(current);
 }
 
 asmlinkage void
diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
index ee57640..1f3f0e8 100644
--- a/arch/arm/kernel/traps.c
+++ b/arch/arm/kernel/traps.c
@@ -32,7 +32,6 @@
 #include <asm/unwind.h>
 #include <asm/tls.h>
 
-#include "ptrace.h"
 #include "signal.h"
 
 static const char *handler[]= { "prefetch abort", "data abort", "address exception", "interrupt" };
-- 
1.7.0.4

[RFC][PATCH] ARM: ptrace: remove single-step emulation code

[Russell King - ARM Linux]

On Wed, Jan 19, 2011 at 03:07:15PM +0000, Will Deacon wrote:
> I'm posting this as an RFC to see if anybody has a good reason to keep this
> code around. There's also a chance I've missed an opportunity to remove some
> related code, but I think I found everything. Tested on a Versatile Express,
> single-stepping in GDB worked fine.

Have you checked whether strace and ltrace use single stepping?

[RFC][PATCH] ARM: ptrace: remove single-step emulation code

[Will Deacon]

Hi Russell,

> On Wed, Jan 19, 2011 at 03:07:15PM +0000, Will Deacon wrote:
> > I'm posting this as an RFC to see if anybody has a good reason to keep this
> > code around. There's also a chance I've missed an opportunity to remove some
> > related code, but I think I found everything. Tested on a Versatile Express,
> > single-stepping in GDB worked fine.
> 
> Have you checked whether strace and ltrace use single stepping?

strace works fine with this patch applied and, looking at the
sources, it doesn't use the SINGLESTEP request. As for ltrace,
it *does* use SINGLESTEP but it can use PTRACE_SYSCALL instead
(indeed, it does this for sparc, ia64 and mips). ltrace doesn't
have code for checking the ptrace return value so I'd say it's
their bug.

Will

[RFC][PATCH] ARM: ptrace: remove single-step emulation code

[Arnaud Patard (Rtp)]

"Will Deacon" <will.deacon@arm.com> writes:

> Hi Russell,

Hi,

>
>> On Wed, Jan 19, 2011 at 03:07:15PM +0000, Will Deacon wrote:
>> > I'm posting this as an RFC to see if anybody has a good reason to keep this
>> > code around. There's also a chance I've missed an opportunity to remove some
>> > related code, but I think I found everything. Tested on a Versatile Express,
>> > single-stepping in GDB worked fine.
>> 
>> Have you checked whether strace and ltrace use single stepping?
>
> strace works fine with this patch applied and, looking at the
> sources, it doesn't use the SINGLESTEP request. As for ltrace,
> it *does* use SINGLESTEP but it can use PTRACE_SYSCALL instead
> (indeed, it does this for sparc, ia64 and mips). ltrace doesn't
> have code for checking the ptrace return value so I'd say it's
> their bug.

afair, the current way to prevent SINGLESTEP usage in ltrace is to
modify some #ifdef. So, while I agree that not checking ptrace return
value is not nice, it has nothing to do with SINGLESTEP removal as this
call will not get compiled in. What matters is rather to know if things
are still working once the #ifdef are changed and if they're not,
finding if it's a bug in ltrace or kernel.

Arnaud

[RFC][PATCH] ARM: ptrace: remove single-step emulation code

[Will Deacon]

Arnaud,

> > strace works fine with this patch applied and, looking at the
> > sources, it doesn't use the SINGLESTEP request. As for ltrace,
> > it *does* use SINGLESTEP but it can use PTRACE_SYSCALL instead
> > (indeed, it does this for sparc, ia64 and mips). ltrace doesn't
> > have code for checking the ptrace return value so I'd say it's
> > their bug.
> 
> afair, the current way to prevent SINGLESTEP usage in ltrace is to
> modify some #ifdef. So, while I agree that not checking ptrace return
> value is not nice, it has nothing to do with SINGLESTEP removal as this
> call will not get compiled in. What matters is rather to know if things
> are still working once the #ifdef are changed and if they're not,
> finding if it's a bug in ltrace or kernel.

Not true. A PTRACE_SINGLESTEP request will simply return -EIO. Userspace
programs should check the return value anyway because it could fail and
then handle the failure gracefully (in the case of ltrace, by trying a
PTRACE_SYSCALL request). This also means that ltrace is currently broken
on Thumb, where PTRACE_SINGLESTEP will return an error code.

Will

[RFC][PATCH] ARM: ptrace: remove single-step emulation code

[Timo Juhani Lindfors]

Will Deacon <will.deacon@arm.com> writes:
> We could try to fix this code, but it turns out that nobody uses it anyway.

Very few people use it since it does not work :-)

I tried to use PTRACE_SINGLESTEP on ARM last year but pretty soon
noticed that it doesn't work with user helpers. I sent a patch

http://lists.infradead.org/pipermail/linux-arm-kernel/2010-November/030832.html

and added ARM support to my really simple instruction level execution
tracing tool

http://iki.fi/lindi/git/itrace.git/

originally written mainly for my own debugging needs. The major
limitation of valgrind is that it can't attach to an already running
process and I guess it's too resource intensive to run my processes
constantly under valgrind on an embedded system.

I agree that decoding ARM instruction in kernel space is really
funky. Perhaps my best be would be to copy the old kernel code to my
own userland tool and use PTRACE_POKETEXT to set breakpoints? The only
drawbacks I see are:

1) I need more syscalls per instruction: PTRACE_GETREGS +
PTRACE_SINGLESTEP vs. PTRACE_GETREGS + PTRACE_PEEKTEXT +
PTRACE_POKETEXT * (number of potential branch targets) +
PTRACE_CONTINUE but I guess I can live with this.

2) itrace does not know where user helpers are. Parsing
/proc/config.gz at runtime for CONFIG_VECTORS_BASE is probably not a
good idea. If this location does not change often it is not a problem
to hardcode it in itrace.

> GDB, for example, uses PTRACE_POKETEXT and PTRACE_PEEKTEXT to manage
> breakpoints itself and does not require any kernel assistance.

I was going to say that GDB does not work either with user helpers but
it seems that in

commit 52d6c8167d4e91d89bc5c26cf0bacc2200272f96
Author: Julian Brown <julian@codesourcery.com>
Date:   Thu Jul 30 23:05:00 2009 +0000

the function arm_catch_kernel_helper_return was added to GDB. They
hard code 0xffff0000 but I guess that is ok?

[RFC][PATCH] ARM: ptrace: remove single-step emulation code

[Will Deacon]

Hi Timo,

> I agree that decoding ARM instruction in kernel space is really
> funky. Perhaps my best be would be to copy the old kernel code to my
> own userland tool and use PTRACE_POKETEXT to set breakpoints? The only
> drawbacks I see are:

You could copy the old code, but it may need updating to support ARMv7.
It also doesn't handle Thumb instructions as it stands.
 
> 1) I need more syscalls per instruction: PTRACE_GETREGS +
> PTRACE_SINGLESTEP vs. PTRACE_GETREGS + PTRACE_PEEKTEXT +
> PTRACE_POKETEXT * (number of potential branch targets) +
> PTRACE_CONTINUE but I guess I can live with this.

Ok.

> 2) itrace does not know where user helpers are. Parsing
> /proc/config.gz at runtime for CONFIG_VECTORS_BASE is probably not a
> good idea. If this location does not change often it is not a problem
> to hardcode it in itrace.
> 
> > GDB, for example, uses PTRACE_POKETEXT and PTRACE_PEEKTEXT to manage
> > breakpoints itself and does not require any kernel assistance.
> 
> I was going to say that GDB does not work either with user helpers but
> it seems that in
> 
> commit 52d6c8167d4e91d89bc5c26cf0bacc2200272f96
> Author: Julian Brown <julian@codesourcery.com>
> Date:   Thu Jul 30 23:05:00 2009 +0000
> 
> the function arm_catch_kernel_helper_return was added to GDB. They
> hard code 0xffff0000 but I guess that is ok?

I think it's always mapped at the high address if you have an MMU, so
you might need to detect the uclinux case.

Will