[patch] Skip kill-after-signal.exp if hw single-step is not supported

[patch] Skip kill-after-signal.exp if hw single-step is not supported

[Yao Qi]

[-- Attachment #1: Type: text/plain, Size: 392 bytes --]

This test `gdb_test "stepi" "\r\nhandler .*"' in kill-after-signal.exp
performs a single-step, and deliver a signal (SIGUSR1) to inferior.
Software single step can not do that, because the next instruction
(start of handler) is unable to be determined.

This patch is to allow running this case only on x86 and x86_64, on
which hardware single step is supported.

OK?

-- 
Yao (齐尧)

[-- Attachment #2: skip_kill_after_signal.patch --]
[-- Type: text/x-patch, Size: 1013 bytes --]

2011-07-19  Yao Qi  <yao@codesourcery.com>

	gdb/testsuite/
	* gdb.base/kill-after-signal.exp: Skip if target doesn't support hardware
	single step.

diff --git a/gdb/testsuite/gdb.base/kill-after-signal.exp b/gdb/testsuite/gdb.base/kill-after-signal.exp
index eecad2e..df0afe7 100644
--- a/gdb/testsuite/gdb.base/kill-after-signal.exp
+++ b/gdb/testsuite/gdb.base/kill-after-signal.exp
@@ -14,6 +14,17 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 set testfile "kill-after-signal"
+
+if { ! [istarget "i?86-*-linux*"]
+     && ! [istarget "x86_64-*-linux*"] } {
+    # skip it if target doesn't support hardware single-step.  In following tests,
+    # in "stepi", a signal is delivered along with single-step.  In software single
+    # step, gdb is unable to determine the next instruction addresses, because start
+    # of signal handler is one of them.
+    untested ${testfile}.exp
+    return
+}
+
 if [prepare_for_testing ${testfile}.exp ${testfile}] {
     return -1
 }

Re: [patch] Skip kill-after-signal.exp if hw single-step is not supported

[Jan Kratochvil]

On Tue, 19 Jul 2011 16:43:33 +0200, Yao Qi wrote:
> --- a/gdb/testsuite/gdb.base/kill-after-signal.exp
> +++ b/gdb/testsuite/gdb.base/kill-after-signal.exp
> @@ -14,6 +14,17 @@
>  # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>  
>  set testfile "kill-after-signal"
> +
> +if { ! [istarget "i?86-*-linux*"]
> +     && ! [istarget "x86_64-*-linux*"] } {

I do not yet have experience with software single stepping arches so this is
not an approval from me, just please add also:
"powerpc*-*-linux*"
"s390*-*-linux*"
"ia64*-*-linux*"


Thanks,
Jan

Re: [patch] Skip kill-after-signal.exp if hw single-step is not supported

[Pedro Alves]

On Tuesday 19 July 2011 15:43:33, Yao Qi wrote:
> This test `gdb_test "stepi" "\r\nhandler .*"' in kill-after-signal.exp
> performs a single-step, and deliver a signal (SIGUSR1) to inferior.
> Software single step can not do that, because the next instruction
> (start of handler) is unable to be determined.
> 
> This patch is to allow running this case only on x86 and x86_64, on
> which hardware single step is supported.
> 
> OK?
> 
> -- 
> Yao (齐尧)
> skip_kill_after_signal.patch
>   2011-07-19  Yao Qi  <yao@codesourcery.com>
> 
>         gdb/testsuite/
>         * gdb.base/kill-after-signal.exp: Skip if target doesn't support hardware
>         single step.
> 
> diff --git a/gdb/testsuite/gdb.base/kill-after-signal.exp b/gdb/testsuite/gdb.base/kill-after-signal.exp
> index eecad2e..df0afe7 100644
> --- a/gdb/testsuite/gdb.base/kill-after-signal.exp
> +++ b/gdb/testsuite/gdb.base/kill-after-signal.exp
> @@ -14,6 +14,17 @@
>  # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>  
>  set testfile "kill-after-signal"
> +
> +if { ! [istarget "i?86-*-linux*"]
> +     && ! [istarget "x86_64-*-linux*"] } {

Other archs !x86, and other kernels/stubs/servers !Linux that
can do hardware stepping.  If we're going to have a list, invert
the logic of the check, defaulting to running the test, and leaving
out archs were we know software stepping is used.  Some targets,
like x86/OpenBSD and MacOS, although can hardware step, can't step
into a handler.  I think there are a couple more tests that rely on
stepping into a handler.  Maybe a predicate in gdb.exp for that
would be good.

> +    # skip it if target doesn't support hardware single-step.  In following tests,
> +    # in "stepi", a signal is delivered along with single-step.  In software single
> +    # step, gdb is unable to determine the next instruction addresses, because start
> +    # of signal handler is one of them.
> +    untested ${testfile}.exp
> +    return
> +}
> +
>  if [prepare_for_testing ${testfile}.exp ${testfile}] {
>      return -1
>  }

-- 
Pedro Alves

Re: [patch] Skip kill-after-signal.exp if hw single-step is not supported

[Mark Kettenis]

> From: Pedro Alves <pedro@codesourcery.com>
> Date: Wed, 20 Jul 2011 16:05:43 +0100
> 
> Other archs !x86, and other kernels/stubs/servers !Linux that
> can do hardware stepping.  If we're going to have a list, invert
> the logic of the check, defaulting to running the test, and leaving
> out archs were we know software stepping is used.  Some targets,
> like x86/OpenBSD and MacOS, although can hardware step, can't step
> into a handler.

Just FYI, it's putting breakpoints into the signal trampoline that is
the issue, at least on OpenBSD.  Single-stepping should work fine on
OpenBSD/i386 and OpenBSD/amd64 and all other architectures where it's
done in hardware.

Re: [patch] Skip kill-after-signal.exp if hw single-step is not supported

[Yao Qi]

[-- Attachment #1: Type: text/plain, Size: 1089 bytes --]

On 07/24/2011 06:47 AM, Mark Kettenis wrote:
>> From: Pedro Alves <pedro@codesourcery.com>
>> Date: Wed, 20 Jul 2011 16:05:43 +0100
>>
>> Other archs !x86, and other kernels/stubs/servers !Linux that
>> can do hardware stepping.  If we're going to have a list, invert
>> the logic of the check, defaulting to running the test, and leaving
>> out archs were we know software stepping is used.  Some targets,
>> like x86/OpenBSD and MacOS, although can hardware step, can't step
>> into a handler.
> 
> Just FYI, it's putting breakpoints into the signal trampoline that is
> the issue, at least on OpenBSD.  Single-stepping should work fine on
> OpenBSD/i386 and OpenBSD/amd64 and all other architectures where it's
> done in hardware.

This is the 2nd version of the patch.  A new predicate,
single_step_to_signal_handler_p, is added in lib/gdb.exp.  In default,
it returns true, and return false on target "arm*-*-* and "mips*-*-*".

I don't find any other tests using "stepi" to step into signal handler,
so I only changed kill-after-signal.exp.

OK for mainline?

-- 
Yao (齐尧)

[-- Attachment #2: single_step_to_signal_handler_p.patch --]
[-- Type: text/x-patch, Size: 1627 bytes --]


gdb/testsuite/

	* lib/gdb.exp (single_step_to_signal_handler_p): New.
	* gdb.base/kill-after-signal.exp: Skip if target supports single step
	to signal handler.

diff --git a/gdb/testsuite/gdb.base/kill-after-signal.exp b/gdb/testsuite/gdb.base/kill-after-signal.exp
index eecad2e..f7086fc 100644
--- a/gdb/testsuite/gdb.base/kill-after-signal.exp
+++ b/gdb/testsuite/gdb.base/kill-after-signal.exp
@@ -14,6 +14,12 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 set testfile "kill-after-signal"
+
+if { ![single_step_to_signal_handler_p] } {
+    untested ${testfile}.exp
+    return
+}
+
 if [prepare_for_testing ${testfile}.exp ${testfile}] {
     return -1
 }
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index ef5ad5c..b967a97 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -1527,6 +1527,21 @@ proc support_complex_tests {} {
     return $support_complex_tests_saved
 }
 
+# Return 1 if target hardware or OS supports single stepping to single handler,
+# otherwise, return 0.
+
+proc single_step_to_signal_handler_p {} {
+
+    # Targets don't have hardware single step.  On these targets, when a signal
+    # is delivered during software single step, gdb is unable to determine the
+    # next instruction addresses, because start of signal handler is one of them.
+    if { [istarget "arm*-*-*"] || [istarget "mips*-*-*"] } {
+	return 0
+    }
+
+    return 1
+}
+
 # Return 1 if target is ILP32.
 # This cannot be decided simply from looking at the target string,
 # as it might depend on externally passed compiler options like -m64.

Re: [patch] Skip kill-after-signal.exp if hw single-step is not supported

[Matthew Gretton-Dann]

On 09/08/11 15:32, Yao Qi wrote:
> On 07/24/2011 06:47 AM, Mark Kettenis wrote:
>>> From: Pedro Alves<pedro@codesourcery.com>
>>> Date: Wed, 20 Jul 2011 16:05:43 +0100
>>>
>>> Other archs !x86, and other kernels/stubs/servers !Linux that
>>> can do hardware stepping.  If we're going to have a list, invert
>>> the logic of the check, defaulting to running the test, and leaving
>>> out archs were we know software stepping is used.  Some targets,
>>> like x86/OpenBSD and MacOS, although can hardware step, can't step
>>> into a handler.
>>
>> Just FYI, it's putting breakpoints into the signal trampoline that is
>> the issue, at least on OpenBSD.  Single-stepping should work fine on
>> OpenBSD/i386 and OpenBSD/amd64 and all other architectures where it's
>> done in hardware.
>
> This is the 2nd version of the patch.  A new predicate,
> single_step_to_signal_handler_p, is added in lib/gdb.exp.  In default,
> it returns true, and return false on target "arm*-*-* and "mips*-*-*".
>
> I don't find any other tests using "stepi" to step into signal handler,
> so I only changed kill-after-signal.exp.
>
> OK for mainline?
>
>
>
> single_step_to_signal_handler_p.patch
>
>
>
> gdb/testsuite/
>
> 	* lib/gdb.exp (single_step_to_signal_handler_p): New.
> 	* gdb.base/kill-after-signal.exp: Skip if target supports single step
> 	to signal handler.
>
> diff --git a/gdb/testsuite/gdb.base/kill-after-signal.exp b/gdb/testsuite/gdb.base/kill-after-signal.exp
> index eecad2e..f7086fc 100644
> --- a/gdb/testsuite/gdb.base/kill-after-signal.exp
> +++ b/gdb/testsuite/gdb.base/kill-after-signal.exp
> @@ -14,6 +14,12 @@
>   # along with this program.  If not, see<http://www.gnu.org/licenses/>.
>
>   set testfile "kill-after-signal"
> +
> +if { ![single_step_to_signal_handler_p] } {
> +    untested ${testfile}.exp
> +    return
> +}
> +
>   if [prepare_for_testing ${testfile}.exp ${testfile}] {
>       return -1
>   }
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index ef5ad5c..b967a97 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -1527,6 +1527,21 @@ proc support_complex_tests {} {
>       return $support_complex_tests_saved
>   }
>
> +# Return 1 if target hardware or OS supports single stepping to single handler,
> +# otherwise, return 0.
> +
> +proc single_step_to_signal_handler_p {} {
> +
> +    # Targets don't have hardware single step.  On these targets, when a signal
> +    # is delivered during software single step, gdb is unable to determine the
> +    # next instruction addresses, because start of signal handler is one of them.
> +    if { [istarget "arm*-*-*"] || [istarget "mips*-*-*"] } {
> +	return 0
> +    }
> +
> +    return 1
> +}
> +

Would this be better if it followed the pattern of the 
skip_hw_breakpoint_tests and skip_hw_watchpoint_tests family of 
functions in gdb.exp?

So I would rename the function to skip_hw_single_step_tests[1], and test 
for the existence of gdb,no_hardware_watchpoints in the board info.

[1] Although for the use you are putting it to this is possibly a 
confusing name.

Thanks,

Matt


-- 
Matthew Gretton-Dann
Principal Engineer, PD Software - Tools, ARM Ltd

Re: [patch] Skip kill-after-signal.exp if hw single-step is not supported

[Yao Qi]

On 08/09/2011 11:24 PM, Matthew Gretton-Dann wrote:
> Would this be better if it followed the pattern of the
> skip_hw_breakpoint_tests and skip_hw_watchpoint_tests family of
> functions in gdb.exp?
> 
> So I would rename the function to skip_hw_single_step_tests[1], and test
> for the existence of gdb,no_hardware_watchpoints in the board info.
> 

Personally, I don't like the procs' name skip_* in lib/gdb.exp.  IMO,
procs in lib/gdb.exp of this kind is to check a certain property in
current env, and return the result.  Leave the test case itself to
determine whether to skip or run.

"single_step_to_signal_handler_p" is not equivalent to "hardware single
step",  because some targets have hardware single step, but can't step
into signal handler.

The property interested here is "whether we can single step into a
signal handler", instead of "whether target has hardware single step".
I am not good at naming functions, so ideas on a better/clear name is
welcome.

-- 
Yao (齐尧)

ping : [patch] Skip kill-after-signal.exp if hw single-step is not supported

[Yao Qi]

On 08/09/2011 10:32 PM, Yao Qi wrote:
> gdb/testsuite/
> 
> 	* lib/gdb.exp (single_step_to_signal_handler_p): New.
> 	* gdb.base/kill-after-signal.exp: Skip if target supports single step
> 	to signal handler.

Ping.  http://sourceware.org/ml/gdb-patches/2011-08/msg00181.html

-- 
Yao (齐尧)

Re: ping : [patch] Skip kill-after-signal.exp if hw single-step is not supported

[Yao Qi]

On 08/18/2011 12:52 PM, Yao Qi wrote:
> On 08/09/2011 10:32 PM, Yao Qi wrote:
>> gdb/testsuite/
>>
>> 	* lib/gdb.exp (single_step_to_signal_handler_p): New.
>> 	* gdb.base/kill-after-signal.exp: Skip if target supports single step
>> 	to signal handler.
>
> Ping.  http://sourceware.org/ml/gdb-patches/2011-08/msg00181.html
>

Ping.

-- 
Yao (齐尧)

ping 3: [patch] Skip kill-after-signal.exp if hw single-step is not supported

[Yao Qi]

On 08/09/2011 10:32 PM, Yao Qi wrote:
> gdb/testsuite/
>
> 	* lib/gdb.exp (single_step_to_signal_handler_p): New.
> 	* gdb.base/kill-after-signal.exp: Skip if target supports single step
> 	to signal handler.

Some other stuff reminds me that there is still a patch pending for review.

Ping again.  http://sourceware.org/ml/gdb-patches/2011-08/msg00181.html

-- 
Yao (齐尧)

Re: [patch] Skip kill-after-signal.exp if hw single-step is not supported

[Joel Brobecker]

> gdb/testsuite/
> 
> 	* lib/gdb.exp (single_step_to_signal_handler_p): New.
> 	* gdb.base/kill-after-signal.exp: Skip if target supports single step
> 	to signal handler.

No one commented on this patch.

I only have minor comments, so pre-approved with those changes.

> +# Return 1 if target hardware or OS supports single stepping to single handler,
> +# otherwise, return 0.
> +
> +proc single_step_to_signal_handler_p {} {
> +
> +    # Targets don't have hardware single step.  On these targets, when a signal
> +    # is delivered during software single step, gdb is unable to determine the
> +    # next instruction addresses, because start of signal handler is one of them.
> +    if { [istarget "arm*-*-*"] || [istarget "mips*-*-*"] } {
> +	return 0
> +    }

Can you name the function "can_single_step_to_signal_handler" instead?
I think it will be a little clearer what this function is about.

Also, your comments, both in the function description as well as
inside the function itself, are a little two wide. We have a soft-limit
of 70 chars, which can be exceeded if it helps readability.

-- 
Joel

[committed]: [patch] Skip kill-after-signal.exp if hw single-step is not supported

[Yao Qi]

On 09/18/2011 08:10 AM, Joel Brobecker wrote:
>> gdb/testsuite/
>>
>> 	* lib/gdb.exp (single_step_to_signal_handler_p): New.
>> 	* gdb.base/kill-after-signal.exp: Skip if target supports single step
>> 	to signal handler.
> 
> No one commented on this patch.
> 
> I only have minor comments, so pre-approved with those changes.
> 

Joel, thanks for the review.

> 
> Can you name the function "can_single_step_to_signal_handler" instead?
> I think it will be a little clearer what this function is about.
> 

OK.

> Also, your comments, both in the function description as well as
> inside the function itself, are a little two wide. We have a soft-limit
> of 70 chars, which can be exceeded if it helps readability.
> 

OK, I re-format comments a little bit to comply to 70-char limit.
Committed.

http://sourceware.org/ml/gdb-cvs/2011-09/msg00114.html

-- 
Yao (齐尧)

Re: [patch] Skip kill-after-signal.exp if hw single-step is not supported

[Mark Kettenis]

> Date: Tue, 19 Jul 2011 22:43:33 +0800
> From: Yao Qi <yao@codesourcery.com>
> 
> This test `gdb_test "stepi" "\r\nhandler .*"' in kill-after-signal.exp
> performs a single-step, and deliver a signal (SIGUSR1) to inferior.
> Software single step can not do that, because the next instruction
> (start of handler) is unable to be determined.
> 
> This patch is to allow running this case only on x86 and x86_64, on
> which hardware single step is supported.
> 
> OK?

Certainly not.  For one thing you only allow to run this on Linux.
But there are other architectures that that have hardware single step.
And there are Operating Systems that implement software single step in
the kernel where this "problem" should not occur.  And even when GDB
does the software single stepping, it should be possible to do this
correctly with appropriate OS support.

Proper approach is probably to XFAIL this for the particular targets
that you're dealing with.

> 2011-07-19  Yao Qi  <yao@codesourcery.com>
> 
> 	gdb/testsuite/
> 	* gdb.base/kill-after-signal.exp: Skip if target doesn't support hardware
> 	single step.
> 
> diff --git a/gdb/testsuite/gdb.base/kill-after-signal.exp b/gdb/testsuite/gdb.base/kill-after-signal.exp
> index eecad2e..df0afe7 100644
> --- a/gdb/testsuite/gdb.base/kill-after-signal.exp
> +++ b/gdb/testsuite/gdb.base/kill-after-signal.exp
> @@ -14,6 +14,17 @@
>  # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>  
>  set testfile "kill-after-signal"
> +
> +if { ! [istarget "i?86-*-linux*"]
> +     && ! [istarget "x86_64-*-linux*"] } {
> +    # skip it if target doesn't support hardware single-step.  In following tests,
> +    # in "stepi", a signal is delivered along with single-step.  In software single
> +    # step, gdb is unable to determine the next instruction addresses, because start
> +    # of signal handler is one of them.
> +    untested ${testfile}.exp
> +    return
> +}
> +
>  if [prepare_for_testing ${testfile}.exp ${testfile}] {
>      return -1
>  }