parsic/sh/sparc tracehook breakage when tracing signals

parsic/sh/sparc tracehook breakage when tracing signals

[Mike Frysinger]

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

when i ported the Blackfin code to the tracehook framework, i copied a latent 
bug from the sparc port.  trying to trace another process while handling 
signals no longer worked (and subsequently broke some of the gdb tests).

this was due to calling tracehook_signal_handler() with the last argument 
(stepping) always as 0.  if we look at the definition of this function in 
linux/tracehook.h, we see that calling the function stepping=0 is pointless:
	if (stepping)
		ptrace_notify(SIGTRAP);

after Roland pointed out some more stuff, i went back and looked at all the 
tracehook arches in the tree.  it seems like these arches are all broken in 
the same way:
	parisc (arch/parisc/kernel/signal.c)
	SuperH (64bit only) (arch/sh/kernel/signal_64.c)
	Sparc (all bits) (arch/sparc/kernel/signal{_32,32,_64}.c)

seems like you guys should just change the last argument to:
	test_thread_flag(TIF_SINGLESTEP)
-mike

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

Re: parsic/sh/sparc tracehook breakage when tracing signals

[David Miller]

From: Mike Frysinger <vapier@gentoo.org>
Date: Thu, 11 Feb 2010 19:10:47 -0500

> when i ported the Blackfin code to the tracehook framework, i copied a latent 
> bug from the sparc port.  trying to trace another process while handling 
> signals no longer worked (and subsequently broke some of the gdb tests).

What you seem to be missing is that on Sparc TIF_SINGLESTEP will never
be set, because it does not support hardware single step.

This thread flag is not even defined on that platform.

Re: parsic/sh/sparc tracehook breakage when tracing signals

[Mike Frysinger]

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

On Thursday 11 February 2010 19:39:50 David Miller wrote:
> From: Mike Frysinger <vapier@gentoo.org>
> > when i ported the Blackfin code to the tracehook framework, i copied a
> > latent bug from the sparc port.  trying to trace another process while
> > handling signals no longer worked (and subsequently broke some of the gdb
> > tests).
> 
> What you seem to be missing is that on Sparc TIF_SINGLESTEP will never
> be set, because it does not support hardware single step.
> 
> This thread flag is not even defined on that platform.

my point is that these arches never call ptrace_notify(SIGTRAP) from their 
signal handlers.  maybe the arch is unable to due to some port/hardware 
limitation and this is currently the expected behavior.

dont get me wrong ... i dont particularly care if your arch has a bug in it 
here.  my arch did have a bug and on the off chance that others did too, i 
thought i'd drop an e-mail to people.  if your arch is in the "limited" 
category, feel free to ignore this.
-mike

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

Re: parsic/sh/sparc tracehook breakage when tracing signals

[Roland McGrath]

> when i ported the Blackfin code to the tracehook framework, i copied a latent 
> bug from the sparc port.  trying to trace another process while handling 
> signals no longer worked (and subsequently broke some of the gdb tests).

What you mean is that single-step into a signal handler fails to stop at
the first instruction of the handler.  (Instead it stops after the first
instruction in the handler's prologue.)

> this was due to calling tracehook_signal_handler() with the last argument 
> (stepping) always as 0.  if we look at the definition of this function in 
> linux/tracehook.h, we see that calling the function stepping=0 is pointless:
> 	if (stepping)
> 		ptrace_notify(SIGTRAP);

Indeed, it doesn't do anything else right now.  But the reason to call it
regardless is so that every arch is consistent in telling the generic code
what is going on.  If we add a future feature to ptrace (or something else)
to track signal handler setups, that feature may very well not be one that
fires only when single-step is in use.  An arch that is set up now to call
tracehook_signal_handler() exactly as that function's kerneldoc says to do
will be prepared for such things to work without later arch changes.

> after Roland pointed out some more stuff, i went back and looked at all the 
> tracehook arches in the tree.  it seems like these arches are all broken in 
> the same way:
> 	parisc (arch/parisc/kernel/signal.c)
> 	SuperH (64bit only) (arch/sh/kernel/signal_64.c)
> 	Sparc (all bits) (arch/sparc/kernel/signal{_32,32,_64}.c)
> 
> seems like you guys should just change the last argument to:
> 	test_thread_flag(TIF_SINGLESTEP)

Whether there is a TIF_SINGLESTEP and what it means is arch-specific.
If arch_has_single_step(), then the argument should be nonzero if
user_enable_single_step() is in force at the time of handler setup.

In parisc, it should test for either TIF_SINGLESTEP or TIF_BLOCKSTEP.

In sh, going from what signal_32.c does, it should indeed do as you say.

In sparc, arch_has_single_step()=0, so there is nothing to do.



Thanks,
Roland

Re: parsic/sh/sparc tracehook breakage when tracing signals

[Kyle McMartin]

On Thu, Feb 11, 2010 at 07:10:47PM -0500, Mike Frysinger wrote:
> seems like you guys should just change the last argument to:
> 	test_thread_flag(TIF_SINGLESTEP)

Good catch, I've committed a patch to fix this, will send it upstream
today.

regards, Kyle