[PATCH] x86-64 support for singlestep into 32-bit system calls

[PATCH] x86-64 support for singlestep into 32-bit system calls

[Roland McGrath]

This patch makes x86-64's 32-bit support behave consistently with the
native i386 behavior achieved in Davide Libenzi's patch:

http://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.7/2.6.7-mm7/broken-out/really-ptrace-single-step-2.patch

I hope these both can go into 2.6.8 or 2.6.9, since they make life 
better for gdb.


Thanks,
Roland

Signed-off-by: Roland McGrath <roland@redhat.com>


Index: linux-2.6/arch/x86_64/kernel/entry.S
===================================================================
RCS file: /home/roland/redhat/bkcvs/linux-2.5/arch/x86_64/kernel/entry.S,v
retrieving revision 1.22
diff -b -p -u -r1.22 entry.S
--- linux-2.6/arch/x86_64/kernel/entry.S 12 Apr 2004 20:29:12 -0000 1.22
+++ linux-2.6/arch/x86_64/kernel/entry.S 12 Jul 2004 23:48:37 -0000
@@ -297,7 +297,7 @@ int_very_careful:
 	sti
 	SAVE_REST
 	/* Check for syscall exit trace */	
-	testl $(_TIF_SYSCALL_TRACE|_TIF_SYSCALL_AUDIT),%edx
+	testl $(_TIF_SYSCALL_TRACE|_TIF_SYSCALL_AUDIT|_TIF_SINGLESTEP),%edx
 	jz int_signal
 	pushq %rdi
 	leaq 8(%rsp),%rdi	# &ptregs -> arg1	
Index: linux-2.6/arch/x86_64/kernel/ptrace.c
===================================================================
RCS file: /home/roland/redhat/bkcvs/linux-2.5/arch/x86_64/kernel/ptrace.c,v
retrieving revision 1.16
diff -b -p -u -r1.16 ptrace.c
--- linux-2.6/arch/x86_64/kernel/ptrace.c 31 May 2004 03:07:42 -0000 1.16
+++ linux-2.6/arch/x86_64/kernel/ptrace.c 12 Jul 2004 23:49:16 -0000
@@ -88,6 +88,9 @@ void ptrace_disable(struct task_struct *
 { 
 	long tmp;
 
+#ifdef CONFIG_IA32_EMULATION
+	clear_tsk_thread_flag(child, TIF_SINGLESTEP);
+#endif
 	tmp = get_stack_long(child, EFL_OFFSET) & ~TRAP_FLAG;
 	put_stack_long(child, EFL_OFFSET, tmp);
 }
@@ -344,6 +347,9 @@ asmlinkage long sys_ptrace(long request,
 			set_tsk_thread_flag(child,TIF_SYSCALL_TRACE);
 		else
 			clear_tsk_thread_flag(child,TIF_SYSCALL_TRACE);
+#ifdef CONFIG_IA32_EMULATION
+		clear_tsk_thread_flag(child, TIF_SINGLESTEP);
+#endif
 		child->exit_code = data;
 	/* make sure the single step bit is not set. */
 		tmp = get_stack_long(child, EFL_OFFSET);
@@ -395,6 +401,9 @@ asmlinkage long sys_ptrace(long request,
 		ret = 0;
 		if (child->state == TASK_ZOMBIE)	/* already dead */
 			break;
+#ifdef CONFIG_IA32_EMULATION
+		clear_tsk_thread_flag(child, TIF_SINGLESTEP);
+#endif
 		child->exit_code = SIGKILL;
 		/* make sure the single step bit is not set. */
 		tmp = get_stack_long(child, EFL_OFFSET) & ~TRAP_FLAG;
@@ -416,6 +425,10 @@ asmlinkage long sys_ptrace(long request,
 		}
 		tmp = get_stack_long(child, EFL_OFFSET) | TRAP_FLAG;
 		put_stack_long(child, EFL_OFFSET, tmp);
+#ifdef CONFIG_IA32_EMULATION
+		if (test_tsk_thread_flag(child, TIF_IA32))
+			set_tsk_thread_flag(child, TIF_SINGLESTEP);
+#endif
 		child->exit_code = data;
 		/* give it a chance to run. */
 		wake_up_process(child);
@@ -528,7 +541,10 @@ asmlinkage void syscall_trace_leave(stru
 	if (unlikely(current->audit_context))
 		audit_syscall_exit(current, regs->rax);
 
-	if (test_thread_flag(TIF_SYSCALL_TRACE)
-	    && (current->ptrace & PT_PTRACED))
+	if ((test_thread_flag(TIF_SYSCALL_TRACE)
+#ifdef CONFIG_IA32_EMULATION
+	     || test_thread_flag(TIF_SINGLESTEP)
+#endif
+		    ) && (current->ptrace & PT_PTRACED))
 		syscall_trace(regs);
 }

Re: [PATCH] x86-64 support for singlestep into 32-bit system calls

[Andi Kleen]

On Mon, 12 Jul 2004 16:58:26 -0700
Roland McGrath <roland@redhat.com> wrote:

> This patch makes x86-64's 32-bit support behave consistently with the
> native i386 behavior achieved in Davide Libenzi's patch:
> http://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.7/2.6.7-mm7/broken-out/really-ptrace-single-step-2.patch
> 
> I hope these both can go into 2.6.8 or 2.6.9, since they make life 
> better for gdb.

I would prefer not to merge this. It looks extremly ugly and the current
behaviour is not that unreasonable and gdb has lived with it forever,
so why can't it continue it? It has to keep supporting old versions
anyways and other i386 OS it supports probably do the same.

If i386 merges it I guess it's ok for consistency, but it would be 
better to not do it at all.


> +#ifdef CONFIG_IA32_EMULATION
> +	clear_tsk_thread_flag(child, TIF_SINGLESTEP);
> +#endif


This looks wrong. Why do you do this unconditionally when 32bit 
emulation is compiled in? Either it is correct to do for 64bit 
processes, then the ifdef should go, or it is not correct then
it would need a test. For me dropping the flag both for 32bit
and 64bit looks wrong.

Same in other hunks.

-Andi

Re: [PATCH] x86-64 support for singlestep into 32-bit system calls

[Roland McGrath]

> I would prefer not to merge this. It looks extremly ugly and the current
> behaviour is not that unreasonable and gdb has lived with it forever,
> so why can't it continue it? It has to keep supporting old versions
> anyways and other i386 OS it supports probably do the same.
> 
> If i386 merges it I guess it's ok for consistency, but it would be 
> better to not do it at all.

Just because it has always been wrong before doesn't mean it's not wrong or
shouldn't be fixed.  I think I have stated the case for the i386 behavior
change in that thread, so I won't reiterate it.  The solution currently in
-mm is arguably ugly, but it adds no overhead and affects only the behavior
of the ptrace cases, and makes the user semantics go from ugly to correct.

Certainly the changes for x86_64's ia32 support should go in if the i386
native changes do and not if they don't--the purpose of the x86_64 ia32
support is to behave the same way the native i386 kernel does.  But as to
whether both together should be in or out, the people who actually use
ptrace are agreed that they should go in.

> > +#ifdef CONFIG_IA32_EMULATION
> > +	clear_tsk_thread_flag(child, TIF_SINGLESTEP);
> > +#endif
> 
> This looks wrong. Why do you do this unconditionally when 32bit 
> emulation is compiled in? Either it is correct to do for 64bit 
> processes, then the ifdef should go, or it is not correct then
> it would need a test. For me dropping the flag both for 32bit
> and 64bit looks wrong.

It is not wrong; it is sometimes superfluous.  In 64-bit processes it is
superfluous because the flag can never be set in a 64-bit process anyway.
But there is no reason to do it at all when there are no 32-bit processes
to consider, hence the #ifdef's.  I figured the superfluous bit clearing
would be cheaper than the extra test and branch to avoid it.  But I haven't
done any measurements to support that.  It would also be correct to do:

	#ifdef CONFIG_IA32_EMULATION
		if (test_tsk_thread_flag(child, TIF_IA32))
			clear_tsk_thread_flag(child, TIF_SINGLESTEP);
	#endif


Thanks,
Roland