Skip to contentSkip to conversation

Change to allow signal handlers to set SE and BE bits.

PATCH Corey Minyard 4 participants 11 messages Aug 29, 2003 – Sep 10, 2003

lore
Share & tools
lore
#0 lore raw

I have a debugger that runs in an application that requires access to the SE and BE bits. The following patch adds that capability to 2.4.21-ben1. I have tested this, and gdb still seems to correctly step out of signal handlers, and it seems to work for 4xx. Does this look ok?

Thanks

-Corey

ppc/kernel/signal.c renamed from ppc/kernel/signal.c.old +27 -2
@@ -284,12 +284,12 @@
 
 /*
  * Restore the current user register values from the user stack,
- * (except for MSR).
+ * (except for MSR, which is partially restored for 6xx).
  */
 static int
 restore_user_regs(struct pt_regs *regs, struct mcontext *sr, int mctxsize)
 {
-#ifdef CONFIG_ALTIVEC
+#if defined(CONFIG_ALTIVEC) || defined(CONFIG_6xx)
 	unsigned long msr;
 #endif
 
@@ -299,6 +299,31 @@
 	/* copy up to but not including MSR */
 	if (__copy_from_user(regs, &sr->mc_gregs, PT_MSR * sizeof(elf_greg_t)))
 		return 1;
+	/* Special handling for the msr. */
+	if (__copy_from_user(&msr, &sr->mc_gregs[PT_MSR], sizeof(elf_greg_t)))
+		return 1;
+#if defined(CONFIG_4xx)
+	/*
+	 * If the signal handlers sets the MSR_SE bit in the MSR, then
+	 * we use that to tell us to turn on single-stepping.  Also set
+	 * it if single stepping is enabled.
+	 */
+	if ((msr & MSR_SE) || (current->thread.single_stepping)) {
+		regs->msr |= MSR_DE;
+		task->thread.dbcr0 |=  (DBCR0_IDM | DBCR0_IC);
+	}
+#else
+	/*
+	 * Take certain bits from the MSR.
+	 * Allow a signal handler to set these bits in the MSR, it is
+	 * neccesary for some debuggers to work.
+	 */
+#define MSR_USERALLOWED		(MSR_BE | MSR_SE)
+	regs->msr &= ~MSR_USERALLOWED;
+	regs->msr |= msr & MSR_USERALLOWED;
+	if (current->thread.single_stepping)
+		regs->msr |= MSR_SE;
+#endif
 	/* copy from orig_r3 (the word after the MSR) up to the end */
 	if (__copy_from_user(&regs->orig_gpr3, &sr->mc_gregs[PT_ORIG_R3],
 			     GP_REGS_SIZE - PT_ORIG_R3 * sizeof(elf_greg_t)))
ppc/kernel/traps.c renamed from ppc/kernel/traps.c.old +3 -1
@@ -396,7 +396,8 @@
 void
 SingleStepException(struct pt_regs *regs)
 {
-	regs->msr &= ~MSR_SE;  /* Turn off 'trace' bit */
+	regs->msr &= ~(MSR_SE | MSR_BE);  /* Turn off 'trace' bits */
+	current->thread.single_stepping = 0;
 	if (debugger_sstep(regs))
 		return;
 	_exception(SIGTRAP, regs, TRAP_TRACE, 0);
@@ -491,6 +492,7 @@
 
 		mtspr(SPRN_DBSR, DBSR_IC);
 		current->thread.dbcr0 &=  ~DBCR0_IC;
+		current->thread.single_stepping = 0;
 
 		if (!user_mode(regs) && debugger_sstep(regs))
 			return;
ppc/kernel/ptrace.c renamed from ppc/kernel/ptrace.c.old +2 -1
@@ -140,7 +140,7 @@
 	if (regs != NULL)
 		regs->msr |= MSR_SE;
 #endif
-
+	task->thread.single_stepping = 1;
 }
 
 static inline void
@@ -154,6 +154,7 @@
 	if (regs != NULL)
 		regs->msr &= ~MSR_SE;
 #endif
+	task->thread.single_stepping = 0;
 }
 
 /*
asm-ppc/processor.h renamed from asm-ppc/processor.h.old +3 -0
@@ -722,6 +722,8 @@
 	/* Saved 4xx debug registers */
 	unsigned long dbcr0;
 #endif
+	int           single_stepping;  /* Set by ptrace if the thread is
+					   single-stepping. */
 };
 
 #define INIT_SP		(sizeof(init_stack) + (unsigned long) &init_stack)
@@ -731,6 +733,7 @@
 	.fs = KERNEL_DS, \
 	.pgdir = swapper_pg_dir, \
 	.fpexc_mode = MSR_FE0 | MSR_FE1, \
+	.single_stepping = 0, \
 }
 
 /*
#1 lore raw
Quoted 4 lines · Corey Minyard

I have a debugger that runs in an application that requires access to the SE and BE bits. The following patch adds that capability to 2.4.21-ben1. I have tested this, and gdb still seems to correctly step out of signal handlers, and it seems to work for 4xx. Does this look ok?

Please change MSR_SE->MSR_DWE in the 4xx-specific sections. Also, a comment should be added to each generic section use of MSR_SE to make it clear that this is the BookE/4xx MSR_DWE.

-Matt

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

#2 lore raw

Actually, using the SE bit may not be the best way to handle this to cover all the PPC variants.

Would it be better to have a special bit field someplace that is used to communicate between the signal handler and the kernel? Some possibilities are:

* The top 16 bits of the trap field
* The currently unused mq field (except on APUS?)
* A new field in the signal frame

I'm thinking that reserving the top 16 bits of the trap field may be the best. It would always come in as zero (so existing software won't be broken) and it will be available for all processors and will not be used for anything else by the processor.

Any thoughts?

-Corey

Quoted 10 lines · Matt Porter
Quoted 6 lines · Corey Minyard

I have a debugger that runs in an application that requires access to the SE and BE bits. The following patch adds that capability to 2.4.21-ben1. I have tested this, and gdb still seems to correctly step out of signal handlers, and it seems to work for 4xx. Does this look ok?

Quoted 8 lines

Please change MSR_SE->MSR_DWE in the 4xx-specific sections. Also, a comment should be added to each generic section use of MSR_SE to make it clear that this is the BookE/4xx MSR_DWE.

-Matt

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

#3 lore raw

Here's an example patch (that I have tested) that shows the use of the top 16 bits of the trap field as communication between the signal handler and the kernel.

-Corey

Quoted 17 lines · Corey Minyard

Actually, using the SE bit may not be the best way to handle this to cover all the PPC variants.

Would it be better to have a special bit field someplace that is used to communicate between the signal handler and the kernel? Some possibilities are:

* The top 16 bits of the trap field
* The currently unused mq field (except on APUS?)
* A new field in the signal frame

I'm thinking that reserving the top 16 bits of the trap field may be the best. It would always come in as zero (so existing software won't be broken) and it will be available for all processors and will not be used for anything else by the processor.

Quoted 17 lines

Any thoughts?

-Corey

Quoted 11 lines · Matt Porter
Quoted 7 lines · Corey Minyard

I have a debugger that runs in an application that requires access to the SE and BE bits. The following patch adds that capability to 2.4.21-ben1. I have tested this, and gdb still seems to correctly step out of signal handlers, and it seems to work for 4xx. Does this look ok?

ppc/kernel/signal.c renamed from ppc/kernel/signal.c.old +23 -0
@@ -304,6 +304,29 @@
 			     GP_REGS_SIZE - PT_ORIG_R3 * sizeof(elf_greg_t)))
 		return 1;
 
+
+	/* Check any special handling requests from the signal
+	   handler */
+	if (regs->trap >> 16) {
+		/* If the signal handler has asked for
+		   single-stepping, set it up. */
+		if (regs->trap & PPC_TRAP_ENABLE_SINGLE_STEP) {
+#if defined(CONFIG_4xx)
+			regs->msr |= MSR_DE;
+			current->thread.dbcr0 |= (DBCR0_IDM | DBCR0_IC);
+#else
+			regs->msr |= MSR_SE;
+#endif
+		}
+		/* If the signal handler has asked for branch
+		   tracing, set it up. */
+		if (regs->trap & PPC_TRAP_ENABLE_BRANCH_TRACE) {
+#if !defined(CONFIG_4xx)
+			regs->msr |= MSR_BE;
+#endif
+		}
+	}
+
 	/* force the process to reload the FP registers from
 	   current->thread when it next does FP instructions */
 	regs->msr &= ~MSR_FP;
ppc/kernel/traps.c renamed from ppc/kernel/traps.c.old +1 -1
@@ -396,7 +396,7 @@
 void
 SingleStepException(struct pt_regs *regs)
 {
-	regs->msr &= ~MSR_SE;  /* Turn off 'trace' bit */
+	regs->msr &= ~(MSR_SE | MSR_BE);  /* Turn off 'trace' bits */
 	if (debugger_sstep(regs))
 		return;
 	_exception(SIGTRAP, regs, TRAP_TRACE, 0);
asm-ppc/ptrace.h renamed from asm-ppc/ptrace.h.old +17 -0
@@ -29,11 +29,28 @@
 	unsigned long ccr;
 	unsigned long mq;		/* 601 only (not used at present) */
 					/* Used on APUS to hold IPL value. */
+
+	/* Note that the high-order 16-bits of the trap field are used
+	   to communicate information back from the signal handler, as
+	   described in the PPC_TRAP_xxx macros below.  You should
+	   leave this alone if you do not need these functions. */
 	unsigned long trap;		/* Reason for being here */
 	unsigned long dar;		/* Fault registers */
 	unsigned long dsisr;		/* used for ESR on 4xx/Book-E */
 	unsigned long result; 		/* Result of a system call */
 };
+
+/* If you set this bit in the "trap" field when returning from a
+   signal handler, single stepping will be enabled on the first
+   instruction back from the signal handler, if the processor supports
+   this. */
+#define PPC_TRAP_ENABLE_SINGLE_STEP	(1 << 16)
+
+/* If you set this bit in the "trap" field when returning from a
+   signal handler, branch tracing will be enabled on the first
+   instruction back from the signal handler, if the processor supports
+   this. */
+#define PPC_TRAP_ENABLE_BRANCH_TRACE	(1 << 17)
 #endif
 
 #ifdef __KERNEL__
#4 lore raw

Paul came up with a much better method for this. I have added a syscall that does a "debug" return from the signal handler. It's much cleaner.

I ahve a patch for this, and I've done a number of things besides just this. I seemed bad to me to add yet another kludge to the beginning of DoSyscall for handling yet another signal return value. So I turned all the signal return syscalls into normal syscalls. This should speed up normal syscall handling by removing four instructions from the syscall entry. Is this ok?

I added some new syscalls for each sigreturn option, but there were already some more that would obviously not work for this. Should I convert the others over to work correctly, or should I leave these like they are?

I made the interface to the debug sigreturn extensible, it takes an array of two-member structures, where the first member is a debug type and the second is a debug value. This should allow a lot of flexibility for adding new things (probably a lot more than is required).

I also brought the syscall table in asm/unistd.h up to date.

I've got some code for setting dabr from userland and causing traps. I could work it into this if someone is interested. It involves some complicated code at exception entry.

-Corey

Quoted 4 lines · Corey Minyard

Here's an example patch (that I have tested) that shows the use of the top 16 bits of the trap field as communication between the signal handler and the kernel.

Quoted 25 lines

-Corey

Quoted 20 lines · Corey Minyard

Actually, using the SE bit may not be the best way to handle this to cover all the PPC variants.

Would it be better to have a special bit field someplace that is used to communicate between the signal handler and the kernel? Some possibilities are:

* The top 16 bits of the trap field
* The currently unused mq field (except on APUS?)
* A new field in the signal frame

I'm thinking that reserving the top 16 bits of the trap field may be the best. It would always come in as zero (so existing software won't be broken) and it will be available for all processors and will not be used for anything else by the processor.

Any thoughts?

-Corey

ppc/kernel/signal.c renamed from ppc/kernel/signal.c.old +61 -8
@@ -383,7 +383,7 @@
 
 	/* Save user registers on the stack */
 	frame = &rt_sf->uc.uc_mcontext;
-	if (save_user_regs(regs, frame, 0x6666, 0))
+	if (save_user_regs(regs, frame, __NR_kern_rt_sigreturn, 0))
 		goto badframe;
 
 	if (put_user(regs->gpr[1], (unsigned long *)newsp))
@@ -461,15 +461,13 @@
 	return 0;
 }
 
-int sys_rt_sigreturn(struct pt_regs *regs)
+int rt_sigreturn(struct ucontext *uc, struct pt_regs *regs)
 {
-	struct rt_sigframe *rt_sf;
 	stack_t st;
 
-	rt_sf = (struct rt_sigframe *)(regs->gpr[1] + __SIGNAL_FRAMESIZE + 16);
-	if (verify_area(VERIFY_READ, rt_sf, sizeof(struct rt_sigframe)))
+	if (verify_area(VERIFY_READ, uc, sizeof(struct ucontext)))
 		goto bad;
-	if (do_setcontext(&rt_sf->uc, regs, 0))
+	if (do_setcontext(uc, regs, 0))
 		goto bad;
 
 	/*
@@ -479,7 +477,7 @@
 	 * always done it up until now so it is probably better not to
 	 * change it.  -- paulus
 	 */
-	if (__copy_from_user(&st, &rt_sf->uc.uc_stack, sizeof(st)))
+	if (__copy_from_user(&st, &uc->uc_stack, sizeof(st)))
 		goto bad;
 	do_sigaltstack(&st, NULL, regs->gpr[1]);
 
@@ -489,6 +487,61 @@
 	do_exit(SIGSEGV);
 }
 
+int sys_rt_sigreturn(struct pt_regs *regs)
+{
+	struct rt_sigframe *rt_sf;
+
+	rt_sf = (struct rt_sigframe *)(regs->gpr[1] + __SIGNAL_FRAMESIZE + 16);
+	return rt_sigreturn(&rt_sf->uc, regs);
+}
+
+int sys_dbg_sigreturn(struct ucontext *uc, int ndbg, struct sig_dbg_op *dbg,
+		      struct pt_regs *regs)
+{
+  	struct sig_dbg_op op;
+	int i;
+
+	for (i=0; i<ndbg; i++) {
+		if (__copy_from_user(&op, dbg, sizeof(op)))
+			return -EFAULT;
+		switch (op.dbg_type) {
+		case SIG_DBG_SINGLE_STEPPING:
+#if defined(CONFIG_4xx)
+			if (op.dbg_value) {
+				regs->msr |= MSR_DE;
+				current->thread.dbcr0 |= (DBCR0_IDM | DBCR0_IC);
+			} else {
+				regs->msr &= ~MSR_DE;
+				current->thread.dbcr0 &= ~(DBCR0_IDM | DBCR0_IC);
+				regs->msr &= ~MSR_SE;
+			}
+#else
+			if (op.dbg_value)
+				regs->msr |= MSR_SE;
+			else
+				regs->msr &= ~MSR_SE;
+#endif
+			break;
+
+		case SIG_DBG_BRANCH_TRACING:
+#if defined(CONFIG_4xx)
+			return -EINVAL;
+#else
+			if (op.dbg_value)
+				regs->msr |= MSR_BE;
+			else
+				regs->msr &= ~MSR_BE;
+#endif
+			break;
+
+		default:
+			return -EINVAL;
+		}
+	}
+
+	return rt_sigreturn(uc, regs);
+}
+
 /*
  * OK, we're invoking a handler
  */
@@ -525,7 +578,7 @@
 	    || __put_user(sig, &sc->signal))
 		goto badframe;
 
-	if (save_user_regs(regs, &frame->mctx, 0x7777, 0))
+	if (save_user_regs(regs, &frame->mctx, __NR_kern_sigreturn, 0))
 		goto badframe;
 
 	if (put_user(regs->gpr[1], (unsigned long *)newsp))
ppc/kernel/traps.c renamed from ppc/kernel/traps.c.old +1 -1
@@ -396,7 +396,7 @@
 void
 SingleStepException(struct pt_regs *regs)
 {
-	regs->msr &= ~MSR_SE;  /* Turn off 'trace' bit */
+	regs->msr &= ~(MSR_SE | MSR_BE);  /* Turn off 'trace' bits */
 	if (debugger_sstep(regs))
 		return;
 	_exception(SIGTRAP, regs, TRAP_TRACE, 0);
ppc/kernel/entry.S renamed from ppc/kernel/entry.S.old +1 -17
@@ -80,10 +80,6 @@
 	lwz	r8,GPR8(r1)
 1:
 #endif /* SHOW_SYSCALLS */
-	cmpi	0,r0,0x7777	/* Special case for 'sys_sigreturn' */
-	beq-	10f
-	cmpi    0,r0,0x6666     /* Special case for 'sys_rt_sigreturn' */
-	beq-    16f
 	lwz	r10,TASK_PTRACE(r2)
 	andi.	r10,r10,PT_TRACESYS
 	bne-	50f
@@ -100,7 +96,7 @@
 	blrl			/* Call handler */
 	.globl	ret_from_syscall_1
 ret_from_syscall_1:
-20:	stw	r3,RESULT(r1)	/* Save result */
+	stw	r3,RESULT(r1)	/* Save result */
 #ifdef SHOW_SYSCALLS
 #ifdef SHOW_SYSCALLS_TASK
 	cmp	0,r2,r31
@@ -127,18 +123,6 @@
 	b	ret_from_except
 66:	li	r3,ENOSYS
 	b	22b
-/* sys_sigreturn */
-10:	addi	r3,r1,STACK_FRAME_OVERHEAD
-	bl	sys_sigreturn
-	cmpi    0,r3,0          /* Check for restarted system call */
-	bge     ret_from_except
-	b       20b
-/* sys_rt_sigreturn */
-16:	addi    r3,r1,STACK_FRAME_OVERHEAD
-	bl      sys_rt_sigreturn
-	cmpi	0,r3,0		/* Check for restarted system call */
-	bge	ret_from_except
-	b	20b
 /* Traced system call support */
 50:	bl	syscall_trace
 	lwz	r0,GPR0(r1)	/* Restore original registers */
ppc/kernel/misc.S renamed from ppc/kernel/misc.S.old +34 -0
@@ -1034,6 +1034,37 @@
 _GLOBAL(__main)
 	blr
 
+_sys_sigreturn:
+	addi    r3,r1,STACK_FRAME_OVERHEAD
+	mflr	r30
+	bl      sys_sigreturn
+	cmpi	0,r3,0		/* Check for restarted system call */
+	bge	ret_from_except
+	mtlr	r30
+	blr
+
+_sys_rt_sigreturn:
+	addi    r3,r1,STACK_FRAME_OVERHEAD
+	mflr	r30
+	bl      sys_rt_sigreturn
+	cmpi	0,r3,0		/* Check for restarted system call */
+	bge	ret_from_except
+	mtlr	r30
+	blr
+
+/*
+ * This handles the special case of the debug sigreturn call, which needs
+ * the registers in order to do its job.
+ */
+_sys_dbg_sigreturn:
+	addi    r6,r1,STACK_FRAME_OVERHEAD
+	mflr	r30
+	bl      sys_dbg_sigreturn
+	cmpi	0,r3,0		/* Check for restarted system call */
+	bge	ret_from_except
+	mtlr	r30
+	blr
+
 #define SYSCALL(name) \
 _GLOBAL(name) \
 	li	r0,__NR_##name; \
@@ -1314,6 +1345,9 @@
 	.long sys_ni_syscall	/*	reserved for sys_clock_getres */
 	.long sys_ni_syscall	/*	reserved for sys_clock_nanosleep */
 	.long sys_swapcontext
+	.long _sys_sigreturn	/* 250 */
+	.long _sys_rt_sigreturn
+	.long _sys_dbg_sigreturn
 
 	.rept NR_syscalls-(.-sys_call_table)/4
 		.long sys_ni_syscall
asm-ppc/signal.h renamed from asm-ppc/signal.h.old +19 -0
@@ -152,4 +152,23 @@
 #include <asm/sigcontext.h>
 #endif /* __KERNEL__ */
 
+/*
+ * These are parameters to dbg_sigreturn syscall.  They enable or
+ * disable certain debugging things that can be done from signal
+ * handlers.  The dbg_sigreturn syscall *must* be called from a
+ * SA_SIGINFO signal so the ucontext can be passed to it.  It takes an
+ * array of struct sig_dbg_op, which has the debug operations to
+ * perform before returning from the signal.
+ */
+struct sig_dbg_op {
+	int dbg_type;
+	unsigned long dbg_value;
+};
+
+/* Enable or disable single-stepping.  The value sets the state. */
+#define SIG_DBG_SINGLE_STEPPING		1
+
+/* Enable or disable branch tracing.  The value sets the state. */
+#define SIG_DBG_BRANCH_TRACING		2
+
 #endif
asm-ppc/unistd.h renamed from asm-ppc/unistd.h.old +21 -0
@@ -237,7 +237,28 @@
 #define __NR_io_getevents	229
 #define __NR_io_submit		230
 #define __NR_io_cancel		231
+#define __NR_set_tid_address	232
+#define __NR_fadvise64		233
+#define __NR_exit_group		234
+#define __NR_lookup_dcookie	235
+#define __NR_epoll_create	236
+#define __NR_epoll_ctl		237
+#define __NR_epoll_wait		238
+#define __NR_remap_file_pages	239
+#define __NR_timer_create	240
+#define __NR_timer_settime	241
+#define __NR_timer_gettime	242
+#define __NR_timer_getoverrun	243
+#define __NR_timer_delete	244
+#define __NR_clock_settime	245
+#define __NR_clock_gettime	246
+#define __NR_clock_getres	247
+#define __NR_clock_nanosleep	248
 #endif
+#define __NR_swapcontext	249
+#define __NR_kern_sigreturn	250
+#define __NR_kern_rt_sigreturn	251
+#define __NR_dbg_sigreturn	252
 
 #define __NR(n)	#n
 
#5 lore raw
Quoted 9 lines · Corey Minyard

Paul came up with a much better method for this. I have added a syscall that does a "debug" return from the signal handler. It's much cleaner.

I ahve a patch for this, and I've done a number of things besides just this. I seemed bad to me to add yet another kludge to the beginning of DoSyscall for handling yet another signal return value. So I turned all the signal return syscalls into normal syscalls. This should speed up normal syscall handling by removing four instructions from the syscall entry. Is this ok?

The problem with changing the signal return is that you break at least gdb, and maybe more (g++ stack unwinding ?)

Ben.

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

#6 lore raw

What if you went back to using the original syscall numbers? Those seem to have code in gdb to support them.

-Corey

Quoted 15 lines · Benjamin Herrenschmidt
Quoted 11 lines · Corey Minyard

Paul came up with a much better method for this. I have added a syscall that does a "debug" return from the signal handler. It's much cleaner.

I ahve a patch for this, and I've done a number of things besides just this. I seemed bad to me to add yet another kludge to the beginning of DoSyscall for handling yet another signal return value. So I turned all the signal return syscalls into normal syscalls. This should speed up normal syscall handling by removing four instructions from the syscall entry. Is this ok?

Quoted 8 lines

The problem with changing the signal return is that you break at least gdb, and maybe more (g++ stack unwinding ?)

Ben.

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

#7 lore raw
In reply to Corey Minyard
Quoted 4 lines · Corey Minyard

I added some new syscalls for each sigreturn option, but there were already some more that would obviously not work for this. Should I convert the others over to work correctly, or should I leave these like they are?

There already were numbers assigned for the sigreturn and rt_sigreturn system calls which weren't being used in 2.4. In 2.5/2.6 I have changed the kernel to use them. I thought the stack unwinding code in glibc (at least) had already been updated to reflect that.

Which tree is your patch against? Note that there are PPC signal changes in 2.4.23-pre3. I hope your patch is against the new version not the old version. :)

Paul.

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

#8 lore raw

After your and Ben's comments, I looked at this some more. Yes, I should have used the old versions of sigreturn and rt_sigreturn. I have looked at gdb and gcc unwinding. gdb 5.3 only handles the "7777" (non-rt) signal frames properly. It does not handle rt frames. gdb 6.0 pre has handling code for the "6666" (rt), "7777" (non-rt), sigreturn, and rt_sigreturn versions of the stack frames. However, the rt versions are broken, it doesn't account for the different frame format in rt signal handlers.

gcc unwinding looks ok, it handles the old and new versions of the stack frame system calls.

I'm being a little persistent on this because I think changing the way signal handling works would be better if changed. System calls would be a little faster with the change (although signal returns would be slightly slower, but I assume the occur less often). Plus, strace is unable to trace signal returns with the way it works now. I consider signal returns a pretty important thing for strace to show, and other arches do this.

I've attached yet another patch. By default, this patch uses the "0x6666" and "0x7777" versions of the signal return syscalls. The DoSyscall code will translate from the 0x6666 and 0x7777 to the sys_rt_sigreturn and sys_sigreturn. You can do "echo '0' > /proc/sys/ppc/sigtype" to change it to use the syscalls for sys_sigreturn and sys_rt_sigreturn. I've tested gdb 5.3, gdb 6.0, and strace with it in both modes. (when using sys_sigreturn, strace actually prints the right thing).

I'm going to test signal unwinding now after I brush up on my C++ skills :-). I expect they will work, but I'll send an email.

This is against 2.4.22-ben2, and it has the new code. The 2.5 code is quite different, but my changes make 2.4 more like 2.5. A 2.5 patch would be pretty easy to do.

I also have a patch against 2.4.20 (I have to do that version for our product), but with stacked signal frames things get ugly.

-Corey

Quoted 15 lines · Paul Mackerras
Quoted 6 lines · Corey Minyard

I added some new syscalls for each sigreturn option, but there were already some more that would obviously not work for this. Should I convert the others over to work correctly, or should I leave these like they are?

There already were numbers assigned for the sigreturn and rt_sigreturn system calls which weren't being used in 2.4. In 2.5/2.6 I have changed the kernel to use them. I thought the stack unwinding code in glibc (at least) had already been updated to reflect that.

Quoted 8 lines

Which tree is your patch against? Note that there are PPC signal changes in 2.4.23-pre3. I hope your patch is against the new version not the old version. :)

Paul.

ppc/kernel/signal.c renamed from ppc/kernel/signal.c.old +101 -10
@@ -26,6 +26,8 @@
 #include <linux/unistd.h>
 #include <linux/stddef.h>
 #include <linux/elf.h>
+#include <linux/sysctl.h>
+#include <linux/init.h>
 #include <asm/ucontext.h>
 #include <asm/uaccess.h>
 #include <asm/pgtable.h>
@@ -40,6 +42,8 @@
 
 #define GP_REGS_SIZE	MIN(sizeof(elf_gregset_t), sizeof(struct pt_regs))
 
+int ppc_use_old_sigret = 1;
+
 extern void syscall_direct_return(struct pt_regs *regs);
 
 int do_signal(sigset_t *oldset, struct pt_regs *regs);
@@ -383,8 +387,15 @@
 
 	/* Save user registers on the stack */
 	frame = &rt_sf->uc.uc_mcontext;
-	if (save_user_regs(regs, frame, 0x6666, 0))
-		goto badframe;
+	if (ppc_use_old_sigret) {
+		/* We use 0x6666 for a rt signal frame, the entry code will
+		   map it to __NR_rt_sigreturn. */
+		if (save_user_regs(regs, frame, 0x6666, 0))
+			goto badframe;
+	} else {
+		if (save_user_regs(regs, frame, __NR_rt_sigreturn, 0))
+			goto badframe;
+	}
 
 	if (put_user(regs->gpr[1], (unsigned long *)newsp))
 		goto badframe;
@@ -461,15 +472,13 @@
 	return 0;
 }
 
-int sys_rt_sigreturn(struct pt_regs *regs)
+int rt_sigreturn(struct ucontext *uc, struct pt_regs *regs)
 {
-	struct rt_sigframe *rt_sf;
 	stack_t st;
 
-	rt_sf = (struct rt_sigframe *)(regs->gpr[1] + __SIGNAL_FRAMESIZE + 16);
-	if (verify_area(VERIFY_READ, rt_sf, sizeof(struct rt_sigframe)))
+	if (verify_area(VERIFY_READ, uc, sizeof(struct ucontext)))
 		goto bad;
-	if (do_setcontext(&rt_sf->uc, regs, 0))
+	if (do_setcontext(uc, regs, 0))
 		goto bad;
 
 	/*
@@ -479,7 +488,7 @@
 	 * always done it up until now so it is probably better not to
 	 * change it.  -- paulus
 	 */
-	if (__copy_from_user(&st, &rt_sf->uc.uc_stack, sizeof(st)))
+	if (__copy_from_user(&st, &uc->uc_stack, sizeof(st)))
 		goto bad;
 	do_sigaltstack(&st, NULL, regs->gpr[1]);
 
@@ -489,6 +498,61 @@
 	do_exit(SIGSEGV);
 }
 
+int sys_rt_sigreturn(struct pt_regs *regs)
+{
+	struct rt_sigframe *rt_sf;
+
+	rt_sf = (struct rt_sigframe *)(regs->gpr[1] + __SIGNAL_FRAMESIZE + 16);
+	return rt_sigreturn(&rt_sf->uc, regs);
+}
+
+int sys_dbg_sigreturn(struct ucontext *uc, int ndbg, struct sig_dbg_op *dbg,
+		      struct pt_regs *regs)
+{
+  	struct sig_dbg_op op;
+	int i;
+
+	for (i=0; i<ndbg; i++) {
+		if (__copy_from_user(&op, dbg, sizeof(op)))
+			return -EFAULT;
+		switch (op.dbg_type) {
+		case SIG_DBG_SINGLE_STEPPING:
+#if defined(CONFIG_4xx)
+			if (op.dbg_value) {
+				regs->msr |= MSR_DE;
+				current->thread.dbcr0 |= (DBCR0_IDM | DBCR0_IC);
+			} else {
+				regs->msr &= ~MSR_DE;
+				current->thread.dbcr0 &= ~(DBCR0_IDM | DBCR0_IC);
+				regs->msr &= ~MSR_SE;
+			}
+#else
+			if (op.dbg_value)
+				regs->msr |= MSR_SE;
+			else
+				regs->msr &= ~MSR_SE;
+#endif
+			break;
+
+		case SIG_DBG_BRANCH_TRACING:
+#if defined(CONFIG_4xx)
+			return -EINVAL;
+#else
+			if (op.dbg_value)
+				regs->msr |= MSR_BE;
+			else
+				regs->msr &= ~MSR_BE;
+#endif
+			break;
+
+		default:
+			return -EINVAL;
+		}
+	}
+
+	return rt_sigreturn(uc, regs);
+}
+
 /*
  * OK, we're invoking a handler
  */
@@ -525,8 +589,15 @@
 	    || __put_user(sig, &sc->signal))
 		goto badframe;
 
-	if (save_user_regs(regs, &frame->mctx, 0x7777, 0))
-		goto badframe;
+	if (ppc_use_old_sigret) {
+		/* We use 0x7777 for a normal signal frame, the entry code will
+		   map it to __NR_sigreturn. */
+		if (save_user_regs(regs, &frame->mctx, 0x7777, 0))
+			goto badframe;
+	} else {
+		if (save_user_regs(regs, &frame->mctx, __NR_sigreturn, 0))
+			goto badframe;
+	}
 
 	if (put_user(regs->gpr[1], (unsigned long *)newsp))
 		goto badframe;
@@ -747,3 +818,23 @@
 	return 1;
 }
 
+#define PPC_CPU_DEFHANDLER_SIGTYPE 1
+static struct ctl_table ppc_table[] = {
+	{PPC_CPU_DEFHANDLER_SIGTYPE, "sigtype", &ppc_use_old_sigret,
+		sizeof(int), 0644, NULL, &proc_dointvec_minmax},
+	{0}
+};
+
+static struct ctl_table ppc_root_table[] = {
+	{CTL_CPU, "ppc", NULL, 0, 0555, ppc_table},
+	{0}
+};
+
+static int __init
+ppc_register_sysctl(void)
+{
+	register_sysctl_table(ppc_root_table, 1);
+	return 0;
+}
+
+__initcall(ppc_register_sysctl);
ppc/kernel/traps.c renamed from ppc/kernel/traps.c.old +1 -1
@@ -396,7 +396,7 @@
 void
 SingleStepException(struct pt_regs *regs)
 {
-	regs->msr &= ~MSR_SE;  /* Turn off 'trace' bit */
+	regs->msr &= ~(MSR_SE | MSR_BE);  /* Turn off 'trace' bits */
 	if (debugger_sstep(regs))
 		return;
 	_exception(SIGTRAP, regs, TRAP_TRACE, 0);
ppc/kernel/entry.S renamed from ppc/kernel/entry.S.old +25 -21

97 lines not on this page · raw message

ppc/kernel/misc.S renamed from ppc/kernel/misc.S.old +38 -2
@@ -1034,6 +1034,37 @@
 _GLOBAL(__main)
 	blr
 
+_sys_sigreturn:
+	addi    r3,r1,STACK_FRAME_OVERHEAD
+	mflr	r30
+	bl      sys_sigreturn
+	cmpi	0,r3,0		/* Check for restarted system call */
+	bge	ret_from_except
+	mtlr	r30
+	blr
+
+_sys_rt_sigreturn:
+	addi    r3,r1,STACK_FRAME_OVERHEAD
+	mflr	r30
+	bl      sys_rt_sigreturn
+	cmpi	0,r3,0		/* Check for restarted system call */
+	bge	ret_from_except
+	mtlr	r30
+	blr
+
+/*
+ * This handles the special case of the debug sigreturn call, which needs
+ * the registers in order to do its job.
+ */
+_sys_dbg_sigreturn:
+	addi    r6,r1,STACK_FRAME_OVERHEAD
+	mflr	r30
+	bl      sys_dbg_sigreturn
+	cmpi	0,r3,0		/* Check for restarted system call */
+	bge	ret_from_except
+	mtlr	r30
+	blr
+
 #define SYSCALL(name) \
 _GLOBAL(name) \
 	li	r0,__NR_##name; \
@@ -1183,7 +1214,7 @@
 	.long sys_sysinfo
 	.long sys_ipc
 	.long sys_fsync
-	.long sys_sigreturn
+	.long _sys_sigreturn
 	.long sys_clone		/* 120 */
 	.long sys_setdomainname
 	.long sys_newuname
@@ -1236,7 +1267,7 @@
 	.long sys_setresgid
 	.long sys_getresgid	/* 170 */
 	.long sys_prctl
-	.long sys_rt_sigreturn
+	.long _sys_rt_sigreturn
 	.long sys_rt_sigaction
 	.long sys_rt_sigprocmask
 	.long sys_rt_sigpending	/* 175 */
@@ -1314,6 +1345,11 @@
 	.long sys_ni_syscall	/*	reserved for sys_clock_getres */
 	.long sys_ni_syscall	/*	reserved for sys_clock_nanosleep */
 	.long sys_swapcontext
+	.long sys_ni_syscall	/* 250  reserved for sys_tgkill */
+	.long sys_ni_syscall	/*      reserved for sys_utimes */
+	.long sys_ni_syscall	/*      reserved for sys_statfs64 */
+	.long sys_ni_syscall	/*      reserved for sys_fstatfs64 */
+	.long _sys_dbg_sigreturn
 
 	.rept NR_syscalls-(.-sys_call_table)/4
 		.long sys_ni_syscall
asm-ppc/signal.h renamed from asm-ppc/signal.h.old +19 -0
@@ -152,4 +152,23 @@
 #include <asm/sigcontext.h>
 #endif /* __KERNEL__ */
 
+/*
+ * These are parameters to dbg_sigreturn syscall.  They enable or
+ * disable certain debugging things that can be done from signal
+ * handlers.  The dbg_sigreturn syscall *must* be called from a
+ * SA_SIGINFO signal so the ucontext can be passed to it.  It takes an
+ * array of struct sig_dbg_op, which has the debug operations to
+ * perform before returning from the signal.
+ */
+struct sig_dbg_op {
+	int dbg_type;
+	unsigned long dbg_value;
+};
+
+/* Enable or disable single-stepping.  The value sets the state. */
+#define SIG_DBG_SINGLE_STEPPING		1
+
+/* Enable or disable branch tracing.  The value sets the state. */
+#define SIG_DBG_BRANCH_TRACING		2
+
 #endif
asm-ppc/unistd.h renamed from asm-ppc/unistd.h.old +26 -0
@@ -237,7 +237,33 @@
 #define __NR_io_getevents	229
 #define __NR_io_submit		230
 #define __NR_io_cancel		231
+#define __NR_set_tid_address	232
+#define __NR_fadvise64		233
+#define __NR_exit_group		234
+#define __NR_lookup_dcookie	235
+#define __NR_epoll_create	236
+#define __NR_epoll_ctl		237
+#define __NR_epoll_wait		238
+#define __NR_remap_file_pages	239
+#define __NR_timer_create	240
+#define __NR_timer_settime	241
+#define __NR_timer_gettime	242
+#define __NR_timer_getoverrun	243
+#define __NR_timer_delete	244
+#define __NR_clock_settime	245
+#define __NR_clock_gettime	246
+#define __NR_clock_getres	247
+#define __NR_clock_nanosleep	248
 #endif
+#define __NR_swapcontext	249
+#if 0
+#define __NR_tgkill		250
+#define __NR__utimes		251
+#define __NR_statfs64		252
+#define __NR_fstatfs64		253
+#endif
+#define __NR_dbg_sigreturn	254
+
 
 #define __NR(n)	#n
 
#9 lore raw
In reply to Corey Minyard
Quoted 4 lines · Corey Minyard

I have a debugger that runs in an application that requires access to the SE and BE bits. The following patch adds that capability to 2.4.21-ben1. I have tested this, and gdb still seems to correctly step out of signal handlers, and it seems to work for 4xx. Does this look ok?

Can you explain a bit more about why the process needs to be able to single-step itself rather than having another process single-step it using ptrace?

It looks like you're using the old trick of sending yourself a signal and then modifying the register values stored on the stack. That seems like a bit of a kludge to me. If there is some reason why you would always naturally be in a signal handler anyway at the point where you want to single-step, then maybe it is less of a kludge. Does your in-process debugger run entirely inside signal handlers? (And if it does, how do you cope with the fact that there are lots of C library functions you can't use inside signal handlers, including printf?)

Also... would this have any impact on how you expect setcontext() to behave? Passing the 3rd argument of a "real-time" signal handler to setcontext is supposed to have the same effect as returning from the handler.

Paul.

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

#10 lore raw
Quoted 11 lines · Paul Mackerras
Quoted 6 lines · Corey Minyard

I have a debugger that runs in an application that requires access to the SE and BE bits. The following patch adds that capability to 2.4.21-ben1. I have tested this, and gdb still seems to correctly step out of signal handlers, and it seems to work for 4xx. Does this look ok?

Quoted 4 lines

Can you explain a bit more about why the process needs to be able to single-step itself rather than having another process single-step it using ptrace?

Sure. This is part of an in-application soft real-time debugger/patcher. The idea is that the debugger/patcher is actually linked to the application. ptrace is really to slow and has too much overhead to use on a running soft real-time application. So instead, the debugger uses signal to do tracepoints. A small bytecode script runs (actually the same bytecode machine that GDB understands) that collects information and then the application continues. The need to single-step actually comes from the fact that you have inserted a trace instruction in the code, and thus the instruction that was there must be run somehow. Relative branch instructions are simulated by software, other instructions are placed into a thread-local memory and single-stepped.

The patching part also needs single-stepping. The patcher does function replacement, meaning that it puts a jump to a new function from the old function (to allow bugs to be fixed and new features to be added). It needs to be able to tell if a function is executing in the section of memory being patched, and single-step it through that section.

Someday, in my spare time, I plan to add a function trace capability to this, to allow you to watch the flow of execution through a piece of code (thus the need for the BE bit).

Quoted 11 lines

It looks like you're using the old trick of sending yourself a signal and then modifying the register values stored on the stack. That seems like a bit of a kludge to me. If there is some reason why you would always naturally be in a signal handler anyway at the point where you want to single-step, then maybe it is less of a kludge. Does your in-process debugger run entirely inside signal handlers? (And if it does, how do you cope with the fact that there are lots of C library functions you can't use inside signal handlers, including printf?)

Well, not exactly that trick. The signal handling code for the traps is the place where this is done. You are already in a signal handler for the trap, so the natural way to single-step is this way. The signal handlers have to be carefully written to not do any C library functions. The only thing they can do is dump data into a shared memory buffer.

Quoted 6 lines

Also... would this have any impact on how you expect setcontext() to behave? Passing the 3rd argument of a "real-time" signal handler to setcontext is supposed to have the same effect as returning from the handler.

Hmm, I haven't thought of that. In fact, I haven't ever used that function. Is this a more efficient way of returning from a signal without going though the kernel, or does it still go through the kernel? If it doesn't go through the kernel, you obviously can't set the MSR bits, but that's not a big concern since this is a specialized function. If it does go through the kernel, then it should work the same.

BTW, i386 allows the TF bit to be set in signal handlers, and I expect that some applications use it to do debugging.

-Corey

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/