Re: [PATCH] parisc: fix tracing of signals

Re: [PATCH] parisc: fix tracing of signals

[Linus Torvalds]

On Fri, 12 Feb 2010, Kyle McMartin wrote:
> 
> Mike Frysinger pointed out that calling tracehook_signal_handler with
> stepping=0 missed testing the thread flags, resulting in not calling
> ptrace_notify. Fix this by testing if we're single stepping or branch
> stepping and setting the flag accordingly.
> 
> Tested, seems to work.

Hmm. All other architectures either pass in zero, or test TIF_SINGLESTEP. 

I guess TIF_BLOCKSTEP is a parisc addition, so now parisc matches x86 and 
power etc, but it still makes me wonder about all those other 
architectures that pass in zero.

For the curious, that seems to be at least sparc and 64-bit (but not 
32-bit) sh.

David? Paul?

		Linus

---
> Reported-by: Mike Frysinger <vapier@gentoo.org>
> Signed-off-by: Kyle McMartin <kyle@mcmartin.ca>
> ---
>  arch/parisc/kernel/signal.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/parisc/kernel/signal.c b/arch/parisc/kernel/signal.c
> index e8467e4..07b3dac 100644
> --- a/arch/parisc/kernel/signal.c
> +++ b/arch/parisc/kernel/signal.c
> @@ -469,7 +469,9 @@ handle_signal(unsigned long sig, siginfo_t *info, struct k_sigaction *ka,
>  	recalc_sigpending();
>  	spin_unlock_irq(&current->sighand->siglock);
>  
> -	tracehook_signal_handler(sig, info, ka, regs, 0);
> +	tracehook_signal_handler(sig, info, ka, regs, 
> +		test_thread_flag(TIF_SINGLESTEP) ||
> +		test_thread_flag(TIF_BLOCKSTEP));
>  
>  	return 1;
>  }
> -- 
> 1.6.6
> 

Re: [PATCH] parisc: fix tracing of signals

[Kyle McMartin]

On Fri, Feb 12, 2010 at 08:21:33AM -0800, Linus Torvalds wrote:
> Hmm. All other architectures either pass in zero, or test TIF_SINGLESTEP. 
> 
> I guess TIF_BLOCKSTEP is a parisc addition, so now parisc matches x86 and 
> power etc, but it still makes me wonder about all those other 
> architectures that pass in zero.
>

Yeah, BLOCKSTEP is the branch tracing bit (traps on a taken branch.)
 
> For the curious, that seems to be at least sparc and 64-bit (but not 
> 32-bit) sh.
> 

Mike sent an email to them reporting this as well, David points out that
sparc has no hw singlestepping, so 0 is appropriate there. SH does seem
to have a legitimate bug.

regards, Kyle

Re: [PATCH] parisc: fix tracing of signals

[Kyle McMartin]

On Fri, Feb 12, 2010 at 08:21:33AM -0800, Linus Torvalds wrote:
> Hmm. All other architectures either pass in zero, or test TIF_SINGLESTEP. 
> 
> I guess TIF_BLOCKSTEP is a parisc addition, so now parisc matches x86 and 
> power etc, but it still makes me wonder about all those other 
> architectures that pass in zero.
>

Yeah, BLOCKSTEP is the branch tracing bit (traps on a taken branch.)
 
> For the curious, that seems to be at least sparc and 64-bit (but not 
> 32-bit) sh.
> 

Mike sent an email to them reporting this as well, David points out that
sparc has no hw singlestepping, so 0 is appropriate there. SH does seem
to have a legitimate bug.

regards, Kyle

Re: [PATCH] parisc: fix tracing of signals

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 929 bytes --]

On Friday 12 February 2010 11:21:33 Linus Torvalds wrote:
> On Fri, 12 Feb 2010, Kyle McMartin wrote:
> > Mike Frysinger pointed out that calling tracehook_signal_handler with
> > stepping=0 missed testing the thread flags, resulting in not calling
> > ptrace_notify. Fix this by testing if we're single stepping or branch
> > stepping and setting the flag accordingly.
> >
> > Tested, seems to work.
> 
> Hmm. All other architectures either pass in zero, or test TIF_SINGLESTEP.
> 
> I guess TIF_BLOCKSTEP is a parisc addition, so now parisc matches x86 and
> power etc, but it still makes me wonder about all those other
> architectures that pass in zero.
> 
> For the curious, that seems to be at least sparc and 64-bit (but not
> 32-bit) sh.
> 
> David? Paul?

David said that the sparc code is currently correct as hardware single 
stepping is not supported:
http://lkml.org/lkml/2010/2/11/385
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] parisc: fix tracing of signals

[Paul Mundt]

On Fri, Feb 12, 2010 at 08:21:33AM -0800, Linus Torvalds wrote:
> On Fri, 12 Feb 2010, Kyle McMartin wrote:
> > Mike Frysinger pointed out that calling tracehook_signal_handler with
> > stepping=0 missed testing the thread flags, resulting in not calling
> > ptrace_notify. Fix this by testing if we're single stepping or branch
> > stepping and setting the flag accordingly.
> > 
> > Tested, seems to work.
> 
> Hmm. All other architectures either pass in zero, or test TIF_SINGLESTEP. 
> 
> I guess TIF_BLOCKSTEP is a parisc addition, so now parisc matches x86 and 
> power etc, but it still makes me wonder about all those other 
> architectures that pass in zero.
> 
> For the curious, that seems to be at least sparc and 64-bit (but not 
> 32-bit) sh.
> 
> David? Paul?
> 
It's a legitimate bug on sh64. We support hardware single stepping there
but never tied in the thread flags when the code was merged with 32-bit,
so this behaviour has existed for some time. I'll fix it up and send out
patches as soon as I get a chance to test it.