[PATCH] PowerPC: bp-permanent.exp, kill-after-signal fix

[PATCH] PowerPC: bp-permanent.exp, kill-after-signal fix

[Carl Love]

GDB maintainers:

The gdb.base/bp-permanent.exp and the gdb.base/kill-after-signal tests
fail on Power 10  The tests pass without the patch on Power 9 and
Intel.  As stated in the commit log below, the tests have been run on
Power 10 Linux version 5.14.0-70.9.1.el9_0.ppc64le and on a Power 9
5.4.0-96-generic kernels.  I have examined the code for the
__kernel_start_sigtramp_rt64 function on both systems and the file is
identical.  As far as I can tell, the failure is hardware specific.

The following patch fixes the issue on Power 10 without introducing any
regression failures on Power 9 or Intel.

Please let me know if the patch is acceptable for mainline gdb. 
Thanks.

                    Carl Love

----------------------------------------------------------------------
PowerPC: bp-permanent.exp, kill-after-signal fix

The break point after the stepi on Intel is the entry point of the user
signal handler function test_signal_handler.  The code at the break point
looks like:

     0x<hex address> <test_signal_handler>: endbr64

On Power 10, the address where gdb stops after the stepi is in the kernel.
The code at the breakpoint looks like:

  0x<hex address>  <__kernel_start_sigtramp_rt64>:	bctrl

Power 10 requires one more stepi to reach the user signal handler.

The tests run fine on Power 9.  The tests have been run on multiple Power 10
systems.  The tests were done with newer and older Linux kernels and gcc
compiler versions from the Power 9 system.  The tests fail identically on
the two Power 10 systems but pass on the Power 9 system.

The two tests were run on the following PowerPC configurations:

Power 9, Ubuntu 20.04 LE, linux 5.4.0-96-generic,
gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0

  gdb.base/bp-permanent.exp 186 passes, no failures
  gdb.base/kill-after-signal.exp 4 passes, no failures

Power 10, RHEL 9, Linux 5.14.0-70.9.1.el9_0.ppc64le,
gcc (GCC) 11.2.1 20220127 (Red Hat 11.2.1-9)
  gdb.base/bp-permanent.exp 182 passes, 4 failures
  gdb.base/kill-after-signal.exp 3 passes, 1 failure

Power 10, SLE 15 SP3 , Linux 5.3.18-150300.59.63-default,
gcc (SUSE Linux) 7.5.0
  gdb.base/bp-permanent.exp 182 passes, 4 failures
  gdb.base/kill-after-signal.exp 3 passes, 1 failure

The following patch fixes the tests on Power 10. The patch does not
introduce regessions on Power 9 or Intel systems.
---
 gdb/testsuite/gdb.base/bp-permanent.exp      |  8 ++++++++
 gdb/testsuite/gdb.base/kill-after-signal.exp | 15 ++++++++++++++-
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.base/bp-permanent.exp b/gdb/testsuite/gdb.base/bp-permanent.exp
index 8be46e96238..f3f47e675ff 100644
--- a/gdb/testsuite/gdb.base/bp-permanent.exp
+++ b/gdb/testsuite/gdb.base/bp-permanent.exp
@@ -258,8 +258,16 @@ proc test {always_inserted sw_watchpoint} {
 		set test "single-step to handler"
 		gdb_test_multiple "stepi" $test {
 		    -re "Program received signal SIGTRAP.*$gdb_prompt $" {
+			# Intel needs one stepi to get to the signal handler.
 			fail $test
 		    }
+		    -re ".*signal handler called.*" {
+			# PowerPC needs one more stepi to reach the user
+			# signal handler.
+			gdb_test "p \$pc" ".*__kernel_start_sigtramp_rt64.*" \
+			    "In kernel"
+			gdb_test "stepi" "$decimal.*\{" $test
+		    }
 		    -re "handler .*$gdb_prompt $" {
 			pass $test
 		    }
diff --git a/gdb/testsuite/gdb.base/kill-after-signal.exp b/gdb/testsuite/gdb.base/kill-after-signal.exp
index 09f5cbc39a6..fc1f82bc70a 100644
--- a/gdb/testsuite/gdb.base/kill-after-signal.exp
+++ b/gdb/testsuite/gdb.base/kill-after-signal.exp
@@ -36,7 +36,20 @@ if ![runto_main] {
 }
 
 gdb_test "continue" "Program received signal SIGUSR1, .*"
-gdb_test "stepi" "\r\nhandler .*"
+
+set test "handler"
+gdb_test_multiple "stepi" $test {
+    -re "\r\nhandler .*" {
+	# Intel requires one stepi to get to the signal handler.
+	pass $test
+    }
+    -re ".*signal handler called.*" {
+	# PowerPC needs one more stepi to reach the user signal handler.
+	gdb_test "p \$pc" ".*__kernel_start_sigtramp_rt64.*" "In kernel"
+	gdb_test "stepi" "$decimal.*\{" $test
+    }
+}
+
 gdb_test_multiple "kill" "kill" {
     -re "Kill the program being debugged\\? \\(y or n\\) $" {
        gdb_test "y" "\\\[Inferior $decimal \\(.*\\) killed\\\]" "kill"
-- 
2.31.1

Re: [PATCH] PowerPC: bp-permanent.exp, kill-after-signal fix

[Ulrich Weigand]

Carl Love <cel@us.ibm.com> wrote:

>The gdb.base/bp-permanent.exp and the gdb.base/kill-after-signal tests
>fail on Power 10  The tests pass without the patch on Power 9 and
>Intel.  As stated in the commit log below, the tests have been run on
>Power 10 Linux version 5.14.0-70.9.1.el9_0.ppc64le and on a Power 9
>5.4.0-96-generic kernels.  I have examined the code for the
>__kernel_start_sigtramp_rt64 function on both systems and the file is
>identical.  As far as I can tell, the failure is hardware specific.

I believe this is related to a kernel change (on Power), not the
hardware level as such.  The sigtramp trampoline was introduced
only recently.

>The following patch fixes the issue on Power 10 without introducing
any
>regression failures on Power 9 or Intel.
>
>Please let me know if the patch is acceptable for mainline gdb. 
>Thanks.

The patch looks good to me.

Thanks,
Ulrich

Re: [PATCH] PowerPC: bp-permanent.exp, kill-after-signal fix

[Pedro Alves]

On 2022-04-29 02:06, Carl Love via Gdb-patches wrote:
> GDB maintainers:
> 
> The gdb.base/bp-permanent.exp and the gdb.base/kill-after-signal tests
> fail on Power 10  The tests pass without the patch on Power 9 and
> Intel.  As stated in the commit log below, the tests have been run on
> Power 10 Linux version 5.14.0-70.9.1.el9_0.ppc64le and on a Power 9
> 5.4.0-96-generic kernels.  I have examined the code for the
> __kernel_start_sigtramp_rt64 function on both systems and the file is
> identical.  As far as I can tell, the failure is hardware specific.
> 
> The following patch fixes the issue on Power 10 without introducing any
> regression failures on Power 9 or Intel.
> 
> Please let me know if the patch is acceptable for mainline gdb. 
> Thanks.
> 
>                     Carl Love
> 
> ----------------------------------------------------------------------
> PowerPC: bp-permanent.exp, kill-after-signal fix
> 
> The break point after the stepi on Intel is the entry point of the user
> signal handler function test_signal_handler.  The code at the break point
> looks like:
> 
>      0x<hex address> <test_signal_handler>: endbr64
> 
> On Power 10, the address where gdb stops after the stepi is in the kernel.
> The code at the breakpoint looks like:
> 
>   0x<hex address>  <__kernel_start_sigtramp_rt64>:	bctrl
> 
> Power 10 requires one more stepi to reach the user signal handler.
> 
> The tests run fine on Power 9.  The tests have been run on multiple Power 10
> systems.  The tests were done with newer and older Linux kernels and gcc
> compiler versions from the Power 9 system.  The tests fail identically on
> the two Power 10 systems but pass on the Power 9 system.
> 
> The two tests were run on the following PowerPC configurations:
> 
> Power 9, Ubuntu 20.04 LE, linux 5.4.0-96-generic,
> gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0
> 
>   gdb.base/bp-permanent.exp 186 passes, no failures
>   gdb.base/kill-after-signal.exp 4 passes, no failures
> 
> Power 10, RHEL 9, Linux 5.14.0-70.9.1.el9_0.ppc64le,
> gcc (GCC) 11.2.1 20220127 (Red Hat 11.2.1-9)
>   gdb.base/bp-permanent.exp 182 passes, 4 failures
>   gdb.base/kill-after-signal.exp 3 passes, 1 failure
> 
> Power 10, SLE 15 SP3 , Linux 5.3.18-150300.59.63-default,
> gcc (SUSE Linux) 7.5.0
>   gdb.base/bp-permanent.exp 182 passes, 4 failures
>   gdb.base/kill-after-signal.exp 3 passes, 1 failure
> 
> The following patch fixes the tests on Power 10. The patch does not
> introduce regessions on Power 9 or Intel systems.
> ---
>  gdb/testsuite/gdb.base/bp-permanent.exp      |  8 ++++++++
>  gdb/testsuite/gdb.base/kill-after-signal.exp | 15 ++++++++++++++-
>  2 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/testsuite/gdb.base/bp-permanent.exp b/gdb/testsuite/gdb.base/bp-permanent.exp
> index 8be46e96238..f3f47e675ff 100644
> --- a/gdb/testsuite/gdb.base/bp-permanent.exp
> +++ b/gdb/testsuite/gdb.base/bp-permanent.exp
> @@ -258,8 +258,16 @@ proc test {always_inserted sw_watchpoint} {
>  		set test "single-step to handler"
>  		gdb_test_multiple "stepi" $test {
>  		    -re "Program received signal SIGTRAP.*$gdb_prompt $" {
> +			# Intel needs one stepi to get to the signal handler.

A bit odd to single out "Intel" here, when what this is is really "architectures other than PowerPC".
But given Ulrich explained this isn't really about the hardware, but instead the kernel version,
I think these comments should be updated to match reality better.

>  			fail $test
>  		    }
> +		    -re ".*signal handler called.*" {

This is missing expecting the prompt, with either $gdb_prompt, or "-re -wrap".

> +			# PowerPC needs one more stepi to reach the user
> +			# signal handler.
> +			gdb_test "p \$pc" ".*__kernel_start_sigtramp_rt64.*" \
> +			    "In kernel"

lowercase "In".

> +			gdb_test "stepi" "$decimal.*\{" $test

If this got us to the user handler, why isn't it expecting the same exact
regexp as the -re below?

> +		    }
>  		    -re "handler .*$gdb_prompt $" {
>  			pass $test
>  		    }
> diff --git a/gdb/testsuite/gdb.base/kill-after-signal.exp b/gdb/testsuite/gdb.base/kill-after-signal.exp
> index 09f5cbc39a6..fc1f82bc70a 100644
> --- a/gdb/testsuite/gdb.base/kill-after-signal.exp
> +++ b/gdb/testsuite/gdb.base/kill-after-signal.exp
> @@ -36,7 +36,20 @@ if ![runto_main] {
>  }
>  
>  gdb_test "continue" "Program received signal SIGUSR1, .*"
> -gdb_test "stepi" "\r\nhandler .*"
> +
> +set test "handler"
> +gdb_test_multiple "stepi" $test {
> +    -re "\r\nhandler .*" {
> +	# Intel requires one stepi to get to the signal handler.
> +	pass $test
> +    }
> +    -re ".*signal handler called.*" {
> +	# PowerPC needs one more stepi to reach the user signal handler.
> +	gdb_test "p \$pc" ".*__kernel_start_sigtramp_rt64.*" "In kernel"
> +	gdb_test "stepi" "$decimal.*\{" $test
> +    }
> +}

Same comments apply here.

> +
>  gdb_test_multiple "kill" "kill" {
>      -re "Kill the program being debugged\\? \\(y or n\\) $" {
>         gdb_test "y" "\\\[Inferior $decimal \\(.*\\) killed\\\]" "kill"

Re: [PATCH] PowerPC: bp-permanent.exp, kill-after-signal fix

[will schmidt]

On Mon, 2022-05-02 at 15:32 +0100, Pedro Alves wrote:
> On 2022-04-29 02:06, Carl Love via Gdb-patches wrote:
> > GDB maintainers:
> > 
> > The gdb.base/bp-permanent.exp and the gdb.base/kill-after-signal tests
> > fail on Power 10  The tests pass without the patch on Power 9 and
> > Intel.  As stated in the commit log below, the tests have been run on
> > Power 10 Linux version 5.14.0-70.9.1.el9_0.ppc64le and on a Power 9
> > 5.4.0-96-generic kernels.  I have examined the code for the
> > __kernel_start_sigtramp_rt64 function on both systems and the file is
> > identical.  As far as I can tell, the failure is hardware specific.
> > 
> > The following patch fixes the issue on Power 10 without introducing any
> > regression failures on Power 9 or Intel.
> > 
> > Please let me know if the patch is acceptable for mainline gdb. 
> > Thanks.
> > 
> >                     Carl Love
> > 
> > ----------------------------------------------------------------------
> > PowerPC: bp-permanent.exp, kill-after-signal fix
> > 
> > The break point after the stepi on Intel is the entry point of the user
> > signal handler function test_signal_handler.  The code at the break point
> > looks like:
> > 
> >      0x<hex address> <test_signal_handler>: endbr64
> > 
> > On Power 10, the address where gdb stops after the stepi is in the kernel.
> > The code at the breakpoint looks like:
> > 
> >   0x<hex address>  <__kernel_start_sigtramp_rt64>:	bctrl
> > 
> > Power 10 requires one more stepi to reach the user signal handler.
> > 
> > The tests run fine on Power 9.  The tests have been run on multiple Power 10
> > systems.  The tests were done with newer and older Linux kernels and gcc
> > compiler versions from the Power 9 system.  The tests fail identically on
> > the two Power 10 systems but pass on the Power 9 system.
> > 
> > The two tests were run on the following PowerPC configurations:
> > 
> > Power 9, Ubuntu 20.04 LE, linux 5.4.0-96-generic,
> > gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0
> > 
> >   gdb.base/bp-permanent.exp 186 passes, no failures
> >   gdb.base/kill-after-signal.exp 4 passes, no failures
> > 
> > Power 10, RHEL 9, Linux 5.14.0-70.9.1.el9_0.ppc64le,
> > gcc (GCC) 11.2.1 20220127 (Red Hat 11.2.1-9)
> >   gdb.base/bp-permanent.exp 182 passes, 4 failures
> >   gdb.base/kill-after-signal.exp 3 passes, 1 failure
> > 
> > Power 10, SLE 15 SP3 , Linux 5.3.18-150300.59.63-default,
> > gcc (SUSE Linux) 7.5.0
> >   gdb.base/bp-permanent.exp 182 passes, 4 failures
> >   gdb.base/kill-after-signal.exp 3 passes, 1 failure
> > 
> > The following patch fixes the tests on Power 10. The patch does not
> > introduce regessions on Power 9 or Intel systems.
> > ---
> >  gdb/testsuite/gdb.base/bp-permanent.exp      |  8 ++++++++
> >  gdb/testsuite/gdb.base/kill-after-signal.exp | 15 ++++++++++++++-
> >  2 files changed, 22 insertions(+), 1 deletion(-)
> > 
> > diff --git a/gdb/testsuite/gdb.base/bp-permanent.exp b/gdb/testsuite/gdb.base/bp-permanent.exp
> > index 8be46e96238..f3f47e675ff 100644
> > --- a/gdb/testsuite/gdb.base/bp-permanent.exp
> > +++ b/gdb/testsuite/gdb.base/bp-permanent.exp
> > @@ -258,8 +258,16 @@ proc test {always_inserted sw_watchpoint} {
> >  		set test "single-step to handler"
> >  		gdb_test_multiple "stepi" $test {
> >  		    -re "Program received signal SIGTRAP.*$gdb_prompt $" {
> > +			# Intel needs one stepi to get to the signal handler.
> 
> A bit odd to single out "Intel" here, when what this is is really "architectures other than PowerPC".
> But given Ulrich explained this isn't really about the hardware, but instead the kernel version,
> I think these comments should be updated to match reality better.

Interesting, I did not see the comment from Uli in my inbox here, but I
did find it in the mailing list archives.

Uli stated:
> I believe this is related to a kernel change (on Power), not the
> hardware level as such.  The sigtramp trampoline was introduced
> only recently.

I thought this (kernel source for handling signals) was investigated as
part of the process to figure out the underlying cause of the issue.  
I strongly agree the detail needs to be clarified.  :-)

Thanks
-Will





> 
> >  			fail $test
> >  		    }
> > +		    -re ".*signal handler called.*" {
> 
> This is missing expecting the prompt, with either $gdb_prompt, or "-re -wrap".
> 
> > +			# PowerPC needs one more stepi to reach the user
> > +			# signal handler.
> > +			gdb_test "p \$pc" ".*__kernel_start_sigtramp_rt64.*" \
> > +			    "In kernel"
> 
> lowercase "In".
> 
> > +			gdb_test "stepi" "$decimal.*\{" $test
> 
> If this got us to the user handler, why isn't it expecting the same exact
> regexp as the -re below?
> 
> > +		    }
> >  		    -re "handler .*$gdb_prompt $" {
> >  			pass $test
> >  		    }
> > diff --git a/gdb/testsuite/gdb.base/kill-after-signal.exp b/gdb/testsuite/gdb.base/kill-after-signal.exp
> > index 09f5cbc39a6..fc1f82bc70a 100644
> > --- a/gdb/testsuite/gdb.base/kill-after-signal.exp
> > +++ b/gdb/testsuite/gdb.base/kill-after-signal.exp
> > @@ -36,7 +36,20 @@ if ![runto_main] {
> >  }
> >  
> >  gdb_test "continue" "Program received signal SIGUSR1, .*"
> > -gdb_test "stepi" "\r\nhandler .*"
> > +
> > +set test "handler"
> > +gdb_test_multiple "stepi" $test {
> > +    -re "\r\nhandler .*" {
> > +	# Intel requires one stepi to get to the signal handler.
> > +	pass $test
> > +    }
> > +    -re ".*signal handler called.*" {
> > +	# PowerPC needs one more stepi to reach the user signal handler.
> > +	gdb_test "p \$pc" ".*__kernel_start_sigtramp_rt64.*" "In kernel"
> > +	gdb_test "stepi" "$decimal.*\{" $test
> > +    }
> > +}
> 
> Same comments apply here.
> 
> > +
> >  gdb_test_multiple "kill" "kill" {
> >      -re "Kill the program being debugged\\? \\(y or n\\) $" {
> >         gdb_test "y" "\\\[Inferior $decimal \\(.*\\) killed\\\]" "kill"

Re: [PATCH] PowerPC: bp-permanent.exp, kill-after-signal fix

[will schmidt]

On Thu, 2022-04-28 at 18:06 -0700, Carl Love wrote:
> GDB maintainers:
> 
> The gdb.base/bp-permanent.exp and the gdb.base/kill-after-signal tests
> fail on Power 10  The tests pass without the patch on Power 9 and
> Intel.  As stated in the commit log below, the tests have been run on
> Power 10 Linux version 5.14.0-70.9.1.el9_0.ppc64le and on a Power 9
> 5.4.0-96-generic kernels.  I have examined the code for the
> __kernel_start_sigtramp_rt64 function on both systems and the file is
> identical.  As far as I can tell, the failure is hardware specific.
> 
> The following patch fixes the issue on Power 10 without introducing any
> regression failures on Power 9 or Intel.
> 
> Please let me know if the patch is acceptable for mainline gdb. 
> Thanks.
> 
>                     Carl Love
> 
> ----------------------------------------------------------------------
> PowerPC: bp-permanent.exp, kill-after-signal fix
> 
> The break point after the stepi on Intel is the entry point of the user
> signal handler function test_signal_handler.  The code at the break point
> looks like:
> 
>      0x<hex address> <test_signal_handler>: endbr64
> 
> On Power 10, the address where gdb stops after the stepi is in the kernel.
> The code at the breakpoint looks like:
> 
>   0x<hex address>  <__kernel_start_sigtramp_rt64>:	bctrl
> 
> Power 10 requires one more stepi to reach the user signal handler.

> 
> The tests run fine on Power 9.  The tests have been run on multiple Power 10
> systems.  The tests were done with newer and older Linux kernels and gcc
> compiler versions from the Power 9 system.  The tests fail identically on
> the two Power 10 systems but pass on the Power 9 system.
> 
> The two tests were run on the following PowerPC configurations:
> 
> Power 9, Ubuntu 20.04 LE, linux 5.4.0-96-generic,
> gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0
> 
>   gdb.base/bp-permanent.exp 186 passes, no failures
>   gdb.base/kill-after-signal.exp 4 passes, no failures
> 
> Power 10, RHEL 9, Linux 5.14.0-70.9.1.el9_0.ppc64le,
> gcc (GCC) 11.2.1 20220127 (Red Hat 11.2.1-9)
>   gdb.base/bp-permanent.exp 182 passes, 4 failures
>   gdb.base/kill-after-signal.exp 3 passes, 1 failure
> 
> Power 10, SLE 15 SP3 , Linux 5.3.18-150300.59.63-default,
> gcc (SUSE Linux) 7.5.0
>   gdb.base/bp-permanent.exp 182 passes, 4 failures
>   gdb.base/kill-after-signal.exp 3 passes, 1 failure
> 
> The following patch fixes the tests on Power 10. The patch does not
> introduce regessions on Power 9 or Intel systems.
> ---
>  gdb/testsuite/gdb.base/bp-permanent.exp      |  8 ++++++++
>  gdb/testsuite/gdb.base/kill-after-signal.exp | 15 ++++++++++++++-
>  2 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/testsuite/gdb.base/bp-permanent.exp b/gdb/testsuite/gdb.base/bp-permanent.exp
> index 8be46e96238..f3f47e675ff 100644
> --- a/gdb/testsuite/gdb.base/bp-permanent.exp
> +++ b/gdb/testsuite/gdb.base/bp-permanent.exp
> @@ -258,8 +258,16 @@ proc test {always_inserted sw_watchpoint} {
>  		set test "single-step to handler"
>  		gdb_test_multiple "stepi" $test {
>  		    -re "Program received signal SIGTRAP.*$gdb_prompt $" {
> +			# Intel needs one stepi to get to the signal handler.
>  			fail $test
>  		    }
> +		    -re ".*signal handler called.*" {
> +			# PowerPC needs one more stepi to reach the user
> +			# signal handler.

So..  Is this directly related to the amount of kernel code handling
the signals?  i.e. Would this need to be updated if another instruction
is added to the kernel code?

I've tentatively expected to see a comment
here with some form of "If we are still in the kernel handler, stepi
until we reach user ..." 


Thanks,



> +			gdb_test "p \$pc" ".*__kernel_start_sigtramp_rt64.*" \
> +			    "In kernel"
> +			gdb_test "stepi" "$decimal.*\{" $test
> +		    }
>  		    -re "handler .*$gdb_prompt $" {
>  			pass $test
>  		    }
> diff --git a/gdb/testsuite/gdb.base/kill-after-signal.exp b/gdb/testsuite/gdb.base/kill-after-signal.exp
> index 09f5cbc39a6..fc1f82bc70a 100644
> --- a/gdb/testsuite/gdb.base/kill-after-signal.exp
> +++ b/gdb/testsuite/gdb.base/kill-after-signal.exp
> @@ -36,7 +36,20 @@ if ![runto_main] {
>  }
> 
>  gdb_test "continue" "Program received signal SIGUSR1, .*"
> -gdb_test "stepi" "\r\nhandler .*"
> +
> +set test "handler"
> +gdb_test_multiple "stepi" $test {
> +    -re "\r\nhandler .*" {
> +	# Intel requires one stepi to get to the signal handler.
> +	pass $test
> +    }
> +    -re ".*signal handler called.*" {
> +	# PowerPC needs one more stepi to reach the user signal handler.
> +	gdb_test "p \$pc" ".*__kernel_start_sigtramp_rt64.*" "In kernel"
> +	gdb_test "stepi" "$decimal.*\{" $test
> +    }
> +}
> +
>  gdb_test_multiple "kill" "kill" {
>      -re "Kill the program being debugged\\? \\(y or n\\) $" {
>         gdb_test "y" "\\\[Inferior $decimal \\(.*\\) killed\\\]" "kill"

Re: [PATCH] PowerPC: bp-permanent.exp, kill-after-signal fix

[Ulrich Weigand]

will schmidt <will_schmidt@vnet.ibm.com> wrote:

>So..  Is this directly related to the amount of kernel code handling
>the signals?  i.e. Would this need to be updated if another
instruction
>is added to the kernel code?

To clarify, this not *kernel* code, it's user space code, but code
that is placed by the kernel into user space (in the vdso).

Many platforms today use the vdso in the signal *return* path, but
Power seems special in that it is using a vdso instruction also to
*enter* a signal handler in the first place.  (So instead of the
kernel directly dispatching to the installed signal handler, it
first dispatches to vdso code which in turn calls into the signal
handler.)

Reading the kernel logs, this seems to have been a performance
optimization to prevent the call/return stack from getting out
of balance.

That vdso code path when entering a signal handler is just a
single "brctl"; I don't see any particular reason why this would
ever need to become any longer.

Bye,
Ulrich

Re: [PATCH] PowerPC: bp-permanent.exp, kill-after-signal fix

[Carl Love]

On Mon, 2022-05-02 at 09:55 -0500, will schmidt wrote:
> On Mon, 2022-05-02 at 15:32 +0100, Pedro Alves wrote:
> > On 2022-04-29 02:06, Carl Love via Gdb-patches wrote:
> > > GDB maintainers:
> > > 
> > > The gdb.base/bp-permanent.exp and the gdb.base/kill-after-signal
> > > tests
> > > fail on Power 10  The tests pass without the patch on Power 9 and
> > > Intel.  As stated in the commit log below, the tests have been
> > > run on
> > > Power 10 Linux version 5.14.0-70.9.1.el9_0.ppc64le and on a Power
> > > 9
> > > 5.4.0-96-generic kernels.  I have examined the code for the
> > > __kernel_start_sigtramp_rt64 function on both systems and the
> > > file is
> > > identical.  As far as I can tell, the failure is hardware
> > > specific.
> > > 
> > > The following patch fixes the issue on Power 10 without
> > > introducing any
> > > regression failures on Power 9 or Intel.
> > > 
> > > Please let me know if the patch is acceptable for mainline gdb. 
> > > Thanks.
> > > 
> > >                     Carl Love
> > > 
> > > ---------------------------------------------------------------
> > > -------
> > > PowerPC: bp-permanent.exp, kill-after-signal fix
> > > 
> > > The break point after the stepi on Intel is the entry point of
> > > the user
> > > signal handler function test_signal_handler.  The code at the
> > > break point
> > > looks like:
> > > 
> > >      0x<hex address> <test_signal_handler>: endbr64
> > > 
> > > On Power 10, the address where gdb stops after the stepi is in
> > > the kernel.
> > > The code at the breakpoint looks like:
> > > 
> > >   0x<hex address>  <__kernel_start_sigtramp_rt64>:	bctrl
> > > 
> > > Power 10 requires one more stepi to reach the user signal
> > > handler.
> > > 
> > > The tests run fine on Power 9.  The tests have been run on
> > > multiple Power 10
> > > systems.  The tests were done with newer and older Linux kernels
> > > and gcc
> > > compiler versions from the Power 9 system.  The tests fail
> > > identically on
> > > the two Power 10 systems but pass on the Power 9 system.
> > > 
> > > The two tests were run on the following PowerPC configurations:
> > > 
> > > Power 9, Ubuntu 20.04 LE, linux 5.4.0-96-generic,
> > > gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0
> > > 
> > >   gdb.base/bp-permanent.exp 186 passes, no failures
> > >   gdb.base/kill-after-signal.exp 4 passes, no failures
> > > 
> > > Power 10, RHEL 9, Linux 5.14.0-70.9.1.el9_0.ppc64le,
> > > gcc (GCC) 11.2.1 20220127 (Red Hat 11.2.1-9)
> > >   gdb.base/bp-permanent.exp 182 passes, 4 failures
> > >   gdb.base/kill-after-signal.exp 3 passes, 1 failure
> > > 
> > > Power 10, SLE 15 SP3 , Linux 5.3.18-150300.59.63-default,
> > > gcc (SUSE Linux) 7.5.0
> > >   gdb.base/bp-permanent.exp 182 passes, 4 failures
> > >   gdb.base/kill-after-signal.exp 3 passes, 1 failure
> > > 
> > > The following patch fixes the tests on Power 10. The patch does
> > > not
> > > introduce regessions on Power 9 or Intel systems.
> > > ---
> > >  gdb/testsuite/gdb.base/bp-permanent.exp      |  8 ++++++++
> > >  gdb/testsuite/gdb.base/kill-after-signal.exp | 15
> > > ++++++++++++++-
> > >  2 files changed, 22 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/gdb/testsuite/gdb.base/bp-permanent.exp
> > > b/gdb/testsuite/gdb.base/bp-permanent.exp
> > > index 8be46e96238..f3f47e675ff 100644
> > > --- a/gdb/testsuite/gdb.base/bp-permanent.exp
> > > +++ b/gdb/testsuite/gdb.base/bp-permanent.exp
> > > @@ -258,8 +258,16 @@ proc test {always_inserted sw_watchpoint} {
> > >  		set test "single-step to handler"
> > >  		gdb_test_multiple "stepi" $test {
> > >  		    -re "Program received signal SIGTRAP.*$gdb_prompt
> > > $" {
> > > +			# Intel needs one stepi to get to the signal
> > > handler.
> > 
> > A bit odd to single out "Intel" here, when what this is is really
> > "architectures other than PowerPC".
> > But given Ulrich explained this isn't really about the hardware,
> > but instead the kernel version,
> > I think these comments should be updated to match reality better.
> 
> Interesting, I did not see the comment from Uli in my inbox here, but
> I
> did find it in the mailing list archives.
> 
> Uli stated:
> > I believe this is related to a kernel change (on Power), not the
> > hardware level as such.  The sigtramp trampoline was introduced
> > only recently.
> 
> I thought this (kernel source for handling signals) was investigated
> as
> part of the process to figure out the underlying cause of the
> issue.  
> I strongly agree the detail needs to be clarified.  :-)

I have investigated the Kernel signal handler
__kernel_start_sigtramp_rt64 in arch/powerpc/kernel/vdso64/sigtramp.S
where gdb stops.  Unfortunately, I don't see anything Power 10 specific
code paths.  I don't think it is a HW issue but clearly Power 10
specific.  I have not found anything in GLIBC that would explain it as 
there are no Power 10 specific code paths in that code.  

I agree it must be in the kernel somewhere, I just don't know where and
have yet to find it.  Unfortunately I don't know the kernel signal
handling code and am a bit lost as to where else to look.   It would be
nice to identify the specifc kernel change so we could add that to the
commit log description.  If anyone has thoughts as to where to look I
will look to see if I can find the change and add a reference in the
comit log before committing the patch.  Thanks for reviewing and
approving the patch.

                                  Carl 

Re: [PATCH] PowerPC: bp-permanent.exp, kill-after-signal fix

[Pedro Alves]

On 2022-05-02 16:19, Carl Love wrote:
> On Mon, 2022-05-02 at 09:55 -0500, will schmidt wrote:
>> On Mon, 2022-05-02 at 15:32 +0100, Pedro Alves wrote:
>>> On 2022-04-29 02:06, Carl Love via Gdb-patches wrote:
>>>> GDB maintainers:
>>>>
>>>> The gdb.base/bp-permanent.exp and the gdb.base/kill-after-signal
>>>> tests
>>>> fail on Power 10  The tests pass without the patch on Power 9 and
>>>> Intel.  As stated in the commit log below, the tests have been
>>>> run on
>>>> Power 10 Linux version 5.14.0-70.9.1.el9_0.ppc64le and on a Power
>>>> 9
>>>> 5.4.0-96-generic kernels.  I have examined the code for the
>>>> __kernel_start_sigtramp_rt64 function on both systems and the
>>>> file is
>>>> identical.  As far as I can tell, the failure is hardware
>>>> specific.
>>>>
>>>> The following patch fixes the issue on Power 10 without
>>>> introducing any
>>>> regression failures on Power 9 or Intel.
>>>>
>>>> Please let me know if the patch is acceptable for mainline gdb. 
>>>> Thanks.
>>>>
>>>>                     Carl Love
>>>>
>>>> ---------------------------------------------------------------
>>>> -------
>>>> PowerPC: bp-permanent.exp, kill-after-signal fix
>>>>
>>>> The break point after the stepi on Intel is the entry point of
>>>> the user
>>>> signal handler function test_signal_handler.  The code at the
>>>> break point
>>>> looks like:
>>>>
>>>>      0x<hex address> <test_signal_handler>: endbr64
>>>>
>>>> On Power 10, the address where gdb stops after the stepi is in
>>>> the kernel.
>>>> The code at the breakpoint looks like:
>>>>
>>>>   0x<hex address>  <__kernel_start_sigtramp_rt64>:	bctrl
>>>>
>>>> Power 10 requires one more stepi to reach the user signal
>>>> handler.
>>>>
>>>> The tests run fine on Power 9.  The tests have been run on
>>>> multiple Power 10
>>>> systems.  The tests were done with newer and older Linux kernels
>>>> and gcc
>>>> compiler versions from the Power 9 system.  The tests fail
>>>> identically on
>>>> the two Power 10 systems but pass on the Power 9 system.
>>>>
>>>> The two tests were run on the following PowerPC configurations:
>>>>
>>>> Power 9, Ubuntu 20.04 LE, linux 5.4.0-96-generic,
>>>> gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0
>>>>
>>>>   gdb.base/bp-permanent.exp 186 passes, no failures
>>>>   gdb.base/kill-after-signal.exp 4 passes, no failures
>>>>
>>>> Power 10, RHEL 9, Linux 5.14.0-70.9.1.el9_0.ppc64le,
>>>> gcc (GCC) 11.2.1 20220127 (Red Hat 11.2.1-9)
>>>>   gdb.base/bp-permanent.exp 182 passes, 4 failures
>>>>   gdb.base/kill-after-signal.exp 3 passes, 1 failure
>>>>
>>>> Power 10, SLE 15 SP3 , Linux 5.3.18-150300.59.63-default,
>>>> gcc (SUSE Linux) 7.5.0
>>>>   gdb.base/bp-permanent.exp 182 passes, 4 failures
>>>>   gdb.base/kill-after-signal.exp 3 passes, 1 failure
>>>>
>>>> The following patch fixes the tests on Power 10. The patch does
>>>> not
>>>> introduce regessions on Power 9 or Intel systems.
>>>> ---
>>>>  gdb/testsuite/gdb.base/bp-permanent.exp      |  8 ++++++++
>>>>  gdb/testsuite/gdb.base/kill-after-signal.exp | 15
>>>> ++++++++++++++-
>>>>  2 files changed, 22 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/gdb/testsuite/gdb.base/bp-permanent.exp
>>>> b/gdb/testsuite/gdb.base/bp-permanent.exp
>>>> index 8be46e96238..f3f47e675ff 100644
>>>> --- a/gdb/testsuite/gdb.base/bp-permanent.exp
>>>> +++ b/gdb/testsuite/gdb.base/bp-permanent.exp
>>>> @@ -258,8 +258,16 @@ proc test {always_inserted sw_watchpoint} {
>>>>  		set test "single-step to handler"
>>>>  		gdb_test_multiple "stepi" $test {
>>>>  		    -re "Program received signal SIGTRAP.*$gdb_prompt
>>>> $" {
>>>> +			# Intel needs one stepi to get to the signal
>>>> handler.
>>>
>>> A bit odd to single out "Intel" here, when what this is is really
>>> "architectures other than PowerPC".
>>> But given Ulrich explained this isn't really about the hardware,
>>> but instead the kernel version,
>>> I think these comments should be updated to match reality better.
>>
>> Interesting, I did not see the comment from Uli in my inbox here, but
>> I
>> did find it in the mailing list archives.
>>
>> Uli stated:
>>> I believe this is related to a kernel change (on Power), not the
>>> hardware level as such.  The sigtramp trampoline was introduced
>>> only recently.
>>
>> I thought this (kernel source for handling signals) was investigated
>> as
>> part of the process to figure out the underlying cause of the
>> issue.  
>> I strongly agree the detail needs to be clarified.  :-)
> 
> I have investigated the Kernel signal handler
> __kernel_start_sigtramp_rt64 in arch/powerpc/kernel/vdso64/sigtramp.S
> where gdb stops.  Unfortunately, I don't see anything Power 10 specific
> code paths.  I don't think it is a HW issue but clearly Power 10
> specific.  I have not found anything in GLIBC that would explain it as 
> there are no Power 10 specific code paths in that code.

Ulrich explained this in another message.

> 
> I agree it must be in the kernel somewhere, I just don't know where and
> have yet to find it.  Unfortunately I don't know the kernel signal
> handling code and am a bit lost as to where else to look.   It would be
> nice to identify the specifc kernel change so we could add that to the
> commit log description.  If anyone has thoughts as to where to look I
> will look to see if I can find the change and add a reference in the
> comit log before committing the patch.  Thanks for reviewing and
> approving the patch.

Please put the extra info in comments in the testcase itself, just not
the commit log.  Note I made other review comments other than the one
being discussed.  Please reply to my review email acking each of them
when you've addressed them.  Thanks.

Re: [PATCH] PowerPC: bp-permanent.exp, kill-after-signal fix

[will schmidt]

On Mon, 2022-05-02 at 15:10 +0000, Ulrich Weigand wrote:
> will schmidt <will_schmidt@vnet.ibm.com> wrote:
> 
> > So..  Is this directly related to the amount of kernel code
> > handling
> > the signals?  i.e. Would this need to be updated if another
> instruction
> > is added to the kernel code?
> 
> To clarify, this not *kernel* code, it's user space code, but code
> that is placed by the kernel into user space (in the vdso).
> 
> Many platforms today use the vdso in the signal *return* path, but
> Power seems special in that it is using a vdso instruction also to
> *enter* a signal handler in the first place.  (So instead of the
> kernel directly dispatching to the installed signal handler, it
> first dispatches to vdso code which in turn calls into the signal
> handler.)
> 
> Reading the kernel logs, this seems to have been a performance
> optimization to prevent the call/return stack from getting out
> of balance.
> 
> That vdso code path when entering a signal handler is just a
> single "brctl"; I don't see any particular reason why this would
> ever need to become any longer.

Ok, thanks for clarifying. 
I look forward to seeing the updated
comments in the next version of the patch by Carl.  :-)

Thanks
-Will


> 
> Bye,
> Ulrich
> 

RE: [PATCH] PowerPC: bp-permanent.exp, kill-after-signal fix

[Carl Love]

Pedro, Ulrich, Will, GCC maintainers:

On Mon, 2022-05-02 at 15:32 +0100, Pedro Alves wrote:
<snip>

> 
> > 
> > diff --git a/gdb/testsuite/gdb.base/bp-permanent.exp
> > b/gdb/testsuite/gdb.base/bp-permanent.exp
> > index 8be46e96238..f3f47e675ff 100644
> > --- a/gdb/testsuite/gdb.base/bp-permanent.exp
> > +++ b/gdb/testsuite/gdb.base/bp-permanent.exp
> > @@ -258,8 +258,16 @@ proc test {always_inserted sw_watchpoint} {
> >  		set test "single-step to handler"
> >  		gdb_test_multiple "stepi" $test {
> >  		    -re "Program received signal SIGTRAP.*$gdb_prompt
> > $" {
> > +			# Intel needs one stepi to get to the signal
> > handler.
> 
> A bit odd to single out "Intel" here, when what this is is really
> "architectures other than PowerPC".
> But given Ulrich explained this isn't really about the hardware, but
> instead the kernel version,
> I think these comments should be updated to match reality better.

True, it is probably not just Intel.  Intel is the only other
architecture I can test.  But yea, removed the comment.

> 
> >  			fail $test
> >  		    }
> > +		    -re ".*signal handler called.*" {
> 
> This is missing expecting the prompt, with either $gdb_prompt, or "-
> re -wrap".

Added $gdb_prompt $.

> 
> > +			# PowerPC needs one more stepi to reach the
> > user
> > +			# signal handler.

Added additional comments added with references to the kernel commits
and summary of how gdb behaves with the kernel changes.  

> > +			gdb_test "p \$pc"
> > ".*__kernel_start_sigtramp_rt64.*" \
> > +			    "In kernel"
> 
> lowercase "In".

Changed to "in kernel code"

> 
> > +			gdb_test "stepi" "$decimal.*\{" $test
> 
> If this got us to the user handler, why isn't it expecting the same
> exact
> regexp as the -re below?

Yes, the orignal test also works.  Changed to: 

     gdb_test "stepi" "handler .*" $test

> 
> > +		    }
> >  		    -re "handler .*$gdb_prompt $" {
> >  			pass $test
> >  		    }
> > diff --git a/gdb/testsuite/gdb.base/kill-after-signal.exp
> > b/gdb/testsuite/gdb.base/kill-after-signal.exp
> > index 09f5cbc39a6..fc1f82bc70a 100644
> > --- a/gdb/testsuite/gdb.base/kill-after-signal.exp
> > +++ b/gdb/testsuite/gdb.base/kill-after-signal.exp
> > @@ -36,7 +36,20 @@ if ![runto_main] {
> >  }
> >  
> >  gdb_test "continue" "Program received signal SIGUSR1, .*"
> > -gdb_test "stepi" "\r\nhandler .*"
> > +
> > +set test "handler"
> > +gdb_test_multiple "stepi" $test {
> > +    -re "\r\nhandler .*" {
> > +	# Intel requires one stepi to get to the signal handler.
> > +	pass $test
> > +    }
> > +    -re ".*signal handler called.*" {
> > +	# PowerPC needs one more stepi to reach the user signal
> > handler.
> > +	gdb_test "p \$pc" ".*__kernel_start_sigtramp_rt64.*" "In
> > kernel"
> > +	gdb_test "stepi" "$decimal.*\{" $test
> > +    }
> > +}
> 
> Same comments apply here.

Same changes applied to the second test.

> 
> > +
> >  gdb_test_multiple "kill" "kill" {
> >      -re "Kill the program being debugged\\? \\(y or n\\) $" {
> >         gdb_test "y" "\\\[Inferior $decimal \\(.*\\) killed\\\]"
> > "kill"

The updated patch has been tested on Power 9 and Power 10 with linux
5.14 or newer kernels to verify the patch works and does not generate
regressions on PowerPC.  The patch was also tested on Intel to ensure
the patch didn't create any unintended regressions on that platform.

Please let me know if the updated patch is acceptable.   Thanks.

                           Carl Love

---------------------------------------------------

PowerPC: bp-permanent.exp, kill-after-signal fix

The break point after the stepi on Intel is the entry point of the user
signal handler function test_signal_handler.  The code at the break point
looks like:

     0x<hex address> <test_signal_handler>: endbr64

On PowerPC with a Linux 5.9 kernel or latter, the address where gdb stops
after the stepi is in the vdso code inserted by the kernel.  The code at the
breakpoint looks like:

  0x<hex address>  <__kernel_start_sigtramp_rt64>:	bctrl

This is different from other architectures.  As dicussed below, recent kernel
changes involving the vdso for PowerPC have been made changes to the signal
handler code flow.  PowerPC is now stoping in function
__kernel_start_sigtramp_rt64.  Powerpc now requires an additional stepi to
reach the user signal handler unlike other architectures.

The bp-permanent.exp and kill-after-signal tests run fine on PowerPC with an
kernel that is older than Linux 5.9.

The Powerpc 64 signal handler was updated by the Linux kernel 5.9-rc1:

    commit id: 0138ba5783ae0dcc799ad401a1e8ac8333790df9
    powerpc/64/signal: Balance return predictor stack in signal trampoline

An additional change to the Powerpc 64 signal handler was made in Linux kernel
version 5.11-rc7 :

     commit id: 24321ac668e452a4942598533d267805f291fdc9
     powerpc/64/signal: Fix regression in __kernel_sigtramp_rt64() semantics

The first kernel change, puts code into the user space signal handler (in the
vdso) as a performance optimization to prevent the call/return stack from
getting out of balance.  The patch ensures that the entire user/kernel/vdso
cycle is balanced with the addition of the "brctl" instruction.

The second patch, fixes the semantics of __kernel_sigtramp_rt64().  A new
symbol is introduced to serve as the jump target from the kernel to the
trampoline which now consists of two parts.

The above changes for PowerPC signal handler, causes gdb to stop in the kernel
code not the user signal handler as expected.  The kernel dispatches to the
vdso code which in turn calls into the signal handler.  PowerPC is special in
that the kernel is using a vdso instruction (bctrl) to enter the signal handler.

I do not have access to a system with the first patch but not the second.  I did
test on Power 9 with the Linux 5.15.0-27-generic kernel.  Both tests fail on
this Power 9 system.  The two tests also fail on Power 10 with the Linux
5.14.0-70.9.1.el9_0.ppc64le kernel.

The following patch fixes the issue by checking if gdb stoped at "signal handler
called".  If gdb stopped there, the tests verifies gdb is in the kernel function
__kernel_start_sigtramp_rt64 then does an additional stepi to reach the user
signal handler.  With the patch below, the tests run without erros on both the
Power 9 and Power 10 systems with out any failures.
---
 gdb/testsuite/gdb.base/bp-permanent.exp      | 25 +++++++++++++++
 gdb/testsuite/gdb.base/kill-after-signal.exp | 33 +++++++++++++++++++-
 2 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.base/bp-permanent.exp b/gdb/testsuite/gdb.base/bp-permanent.exp
index 8be46e96238..11f5562fcc6 100644
--- a/gdb/testsuite/gdb.base/bp-permanent.exp
+++ b/gdb/testsuite/gdb.base/bp-permanent.exp
@@ -260,6 +260,31 @@ proc test {always_inserted sw_watchpoint} {
 		    -re "Program received signal SIGTRAP.*$gdb_prompt $" {
 			fail $test
 		    }
+		    -re ".*signal handler called.*$gdb_prompt $" {
+			# PowerPC Linux kernel patchs:
+			#   commit: 0138ba5783ae0dcc799ad401a1e8ac8333790df9
+			#   powerpc/64/signal: Balance return predictor
+			#   stack in signal trampoline.
+			#
+			# the kernel places an additional brctl instruction
+			# in the vdso to call the user hadler.
+			#
+			#   commit 24321ac668e452a4942598533d267805f291fdc9
+			#   powerpc/64/signal: Fix regression in
+			#   __kernel_sigtramp_rt64() semantics
+			#
+			# Updates the semantics of __kernel_sigtramp_rt64().
+			# It adds a new symbol to serve as a jump target from
+			# the kernel to the trampoline.
+			#
+			# The net result of these changes is that gdb stops
+			# at  __kernel_start_sigtramp_rt64.  Need to do one
+			# more stepi to reach the expected location in the user
+			# signal handler.
+			gdb_test "p \$pc" ".*__kernel_start_sigtramp_rt64.*" \
+			    "in kernel code"
+			gdb_test "stepi" "handler .*" $test
+		    }
 		    -re "handler .*$gdb_prompt $" {
 			pass $test
 		    }
diff --git a/gdb/testsuite/gdb.base/kill-after-signal.exp b/gdb/testsuite/gdb.base/kill-after-signal.exp
index 09f5cbc39a6..1bf5a3e5789 100644
--- a/gdb/testsuite/gdb.base/kill-after-signal.exp
+++ b/gdb/testsuite/gdb.base/kill-after-signal.exp
@@ -36,7 +36,38 @@ if ![runto_main] {
 }
 
 gdb_test "continue" "Program received signal SIGUSR1, .*"
-gdb_test "stepi" "\r\nhandler .*"
+
+set test "handler"
+gdb_test_multiple "stepi" $test {
+    -re "\r\nhandler .*" {
+	pass $test
+    }
+    -re ".*signal handler called.*$gdb_prompt $" {
+	# PowerPC Linux kernel patchs:
+	#   commit: 0138ba5783ae0dcc799ad401a1e8ac8333790df9
+	#   powerpc/64/signal: Balance return predictor
+	#   stack in signal trampoline.
+	#
+	# the kernel places an additional brctl instruction
+	# in the vdso to call the user hadler.
+	#
+	#   commit 24321ac668e452a4942598533d267805f291fdc9
+	#   powerpc/64/signal: Fix regression in
+	#   __kernel_sigtramp_rt64() semantics
+	#
+	# Updates the semantics of __kernel_sigtramp_rt64().
+	# It adds a new symbol to serve as a jump target from
+	# the kernel to the trampoline.
+	#
+	# The net result of these changes is that gdb stops
+	# at  __kernel_start_sigtramp_rt64.  Need to do one
+	# more stepi to reach the expected location in the user
+	# signal handler.
+	gdb_test "p \$pc" ".*__kernel_start_sigtramp_rt64.*" "in kernel code"
+	gdb_test "stepi" "\r\nhandler .*" $test
+    }
+}
+
 gdb_test_multiple "kill" "kill" {
     -re "Kill the program being debugged\\? \\(y or n\\) $" {
        gdb_test "y" "\\\[Inferior $decimal \\(.*\\) killed\\\]" "kill"
-- 
2.31.1

Re: [PATCH] PowerPC: bp-permanent.exp, kill-after-signal fix

[Pedro Alves]

On 2022-05-03 21:10, Carl Love wrote:

> The updated patch has been tested on Power 9 and Power 10 with linux
> 5.14 or newer kernels to verify the patch works and does not generate
> regressions on PowerPC.  The patch was also tested on Intel to ensure
> the patch didn't create any unintended regressions on that platform.
> 
> Please let me know if the updated patch is acceptable.   Thanks.
> 
>                            Carl Love
> 
> ---------------------------------------------------
> 
> PowerPC: bp-permanent.exp, kill-after-signal fix
> 
> The break point after the stepi on Intel is the entry point of the user
> signal handler function test_signal_handler.  The code at the break point
> looks like:
> 
>      0x<hex address> <test_signal_handler>: endbr64
> 
> On PowerPC with a Linux 5.9 kernel or latter, the address where gdb stops
> after the stepi is in the vdso code inserted by the kernel.  The code at the
> breakpoint looks like:
> 
>   0x<hex address>  <__kernel_start_sigtramp_rt64>:	bctrl
> 
> This is different from other architectures.  As dicussed below, recent kernel

dicussed -> discussed

> changes involving the vdso for PowerPC have been made changes to the signal
> handler code flow.  PowerPC is now stoping in function

stoping -> stopping

> __kernel_start_sigtramp_rt64.  Powerpc now requires an additional stepi to

Powerpc -> PowerPC

> reach the user signal handler unlike other architectures.
> 
> The bp-permanent.exp and kill-after-signal tests run fine on PowerPC with an
> kernel that is older than Linux 5.9.
> 
> The Powerpc 64 signal handler was updated by the Linux kernel 5.9-rc1:

Powerpc -> PowerPC

> 
>     commit id: 0138ba5783ae0dcc799ad401a1e8ac8333790df9
>     powerpc/64/signal: Balance return predictor stack in signal trampoline
> 
> An additional change to the Powerpc 64 signal handler was made in Linux kernel
> version 5.11-rc7 :

Powerpc -> PowerPC

> 
>      commit id: 24321ac668e452a4942598533d267805f291fdc9
>      powerpc/64/signal: Fix regression in __kernel_sigtramp_rt64() semantics
> 
> The first kernel change, puts code into the user space signal handler (in the
> vdso) as a performance optimization to prevent the call/return stack from
> getting out of balance.  The patch ensures that the entire user/kernel/vdso
> cycle is balanced with the addition of the "brctl" instruction.
> 
> The second patch, fixes the semantics of __kernel_sigtramp_rt64().  A new
> symbol is introduced to serve as the jump target from the kernel to the
> trampoline which now consists of two parts.
> 
> The above changes for PowerPC signal handler, causes gdb to stop in the kernel
> code not the user signal handler as expected.  The kernel dispatches to the
> vdso code which in turn calls into the signal handler.  PowerPC is special in
> that the kernel is using a vdso instruction (bctrl) to enter the signal handler.
> 
> I do not have access to a system with the first patch but not the second.  I did
> test on Power 9 with the Linux 5.15.0-27-generic kernel.  Both tests fail on
> this Power 9 system.  The two tests also fail on Power 10 with the Linux
> 5.14.0-70.9.1.el9_0.ppc64le kernel.
> 
> The following patch fixes the issue by checking if gdb stoped at "signal handler

stoped -> stopped

> called".  If gdb stopped there, the tests verifies gdb is in the kernel function
> __kernel_start_sigtramp_rt64 then does an additional stepi to reach the user
> signal handler.  With the patch below, the tests run without erros on both the
> Power 9 and Power 10 systems with out any failures.

erros -> errors 

> ---
>  gdb/testsuite/gdb.base/bp-permanent.exp      | 25 +++++++++++++++
>  gdb/testsuite/gdb.base/kill-after-signal.exp | 33 +++++++++++++++++++-
>  2 files changed, 57 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/testsuite/gdb.base/bp-permanent.exp b/gdb/testsuite/gdb.base/bp-permanent.exp
> index 8be46e96238..11f5562fcc6 100644
> --- a/gdb/testsuite/gdb.base/bp-permanent.exp
> +++ b/gdb/testsuite/gdb.base/bp-permanent.exp
> @@ -260,6 +260,31 @@ proc test {always_inserted sw_watchpoint} {
>  		    -re "Program received signal SIGTRAP.*$gdb_prompt $" {
>  			fail $test
>  		    }
> +		    -re ".*signal handler called.*$gdb_prompt $" {
> +			# PowerPC Linux kernel patchs:

patchs -> patch

Maybe say "after", and "commit", as in:

			# After PowerPC Linux kernel commit:
			#
			#   commit: 0138ba5783ae0dcc799ad401a1e8ac8333790df9
                        ...


> +			#   commit: 0138ba5783ae0dcc799ad401a1e8ac8333790df9
> +			#   powerpc/64/signal: Balance return predictor
> +			#   stack in signal trampoline.
> +			#
> +			# the kernel places an additional brctl instruction
> +			# in the vdso to call the user hadler.

hadler -> handler

> +			#
> +			#   commit 24321ac668e452a4942598533d267805f291fdc9
> +			#   powerpc/64/signal: Fix regression in
> +			#   __kernel_sigtramp_rt64() semantics
> +			#
> +			# Updates the semantics of __kernel_sigtramp_rt64().

Say:

			# And then this commit:
			#
			#   commit 24321ac668e452a4942598533d267805f291fdc9
			#   powerpc/64/signal: Fix regression in
			#   __kernel_sigtramp_rt64() semantics
			#
			# updated the semantics of __kernel_sigtramp_rt64().

(note lowercase "updated")

> +			# It adds a new symbol to serve as a jump target from

"It adds" -> "It added"

> +			# the kernel to the trampoline.
> +			#
> +			# The net result of these changes is that gdb stops
> +			# at  __kernel_start_sigtramp_rt64.  Need to do one
> +			# more stepi to reach the expected location in the user
> +			# signal handler.
> +			gdb_test "p \$pc" ".*__kernel_start_sigtramp_rt64.*" \
> +			    "in kernel code"

FYI, leading ".*" is never needed with gdb_test or gdb_test_multiple.  You can just write:

			gdb_test "p \$pc" "__kernel_start_sigtramp_rt64.*" \
			    "in kernel code"

> +			gdb_test "stepi" "handler .*" $test
> +		    }
>  		    -re "handler .*$gdb_prompt $" {
>  			pass $test
>  		    }
> diff --git a/gdb/testsuite/gdb.base/kill-after-signal.exp b/gdb/testsuite/gdb.base/kill-after-signal.exp
> index 09f5cbc39a6..1bf5a3e5789 100644
> --- a/gdb/testsuite/gdb.base/kill-after-signal.exp
> +++ b/gdb/testsuite/gdb.base/kill-after-signal.exp
> @@ -36,7 +36,38 @@ if ![runto_main] {
>  }
>  
>  gdb_test "continue" "Program received signal SIGUSR1, .*"
> -gdb_test "stepi" "\r\nhandler .*"
> +
> +set test "handler"
> +gdb_test_multiple "stepi" $test {
> +    -re "\r\nhandler .*" {
> +	pass $test
> +    }
> +    -re ".*signal handler called.*$gdb_prompt $" {

No need for leading ".*".  Note -wrap would be a little shorter:

       -re -wrap "signal handler called.*" {

> +	# PowerPC Linux kernel patchs:
> +	#   commit: 0138ba5783ae0dcc799ad401a1e8ac8333790df9
> +	#   powerpc/64/signal: Balance return predictor
> +	#   stack in signal trampoline.
> +	#
> +	# the kernel places an additional brctl instruction
> +	# in the vdso to call the user hadler.
> +	#
> +	#   commit 24321ac668e452a4942598533d267805f291fdc9
> +	#   powerpc/64/signal: Fix regression in
> +	#   __kernel_sigtramp_rt64() semantics
> +	#
> +	# Updates the semantics of __kernel_sigtramp_rt64().
> +	# It adds a new symbol to serve as a jump target from
> +	# the kernel to the trampoline.
> +	#
> +	# The net result of these changes is that gdb stops
> +	# at  __kernel_start_sigtramp_rt64.  Need to do one
> +	# more stepi to reach the expected location in the user
> +	# signal handler.

Please update this comment too per comments above.

> +	gdb_test "p \$pc" ".*__kernel_start_sigtramp_rt64.*" "in kernel code"
> +	gdb_test "stepi" "\r\nhandler .*" $test
> +    }
> +}
> +

OK with those changes.  Thanks.

Re: [PATCH] PowerPC: bp-permanent.exp, kill-after-signal fix

[Tom de Vries]

On 5/3/22 22:10, Carl Love via Gdb-patches wrote:
> -gdb_test "stepi" "\r\nhandler .*"
> +
> +set test "handler"
> +gdb_test_multiple "stepi" $test {
> +    -re "\r\nhandler .*" {
> +	pass $test
> +    }

Hi Carl,

gdb_test expects a gdb prompt after "\r\nhandler .*", but 
gdb_test_multiple doesn't.

This causes:
...
(gdb) PASS: gdb.base/kill-after-signal.exp: continue
stepi^M
handler (signo=32767) at 
/home/vries/gdb_versions/devel/src/gdb/testsuite/gdb.base/kill-after\
-signal.c:25^M
25      {^M
PASS: gdb.base/kill-after-signal.exp: handler
kill^M
(gdb) FAIL: gdb.base/kill-after-signal.exp: kill
...

Triggered on x86_64-linux with taskset -c 0.

This fixes it for me:
...
-    -re "\r\nhandler .*" {
+    -re -wrap "\r\nhandler .*" {
...

Can you verify and commit?

Thanks,
- Tom

RE: [PATCH] PowerPC: bp-permanent.exp, kill-after-signal fix

[Carl Love]

Tom:

                  
On Mon, 2022-05-09 at 19:35 +0200, Tom de Vries wrote:
> On 5/3/22 22:10, Carl Love via Gdb-patches wrote:
> > -gdb_test "stepi" "\r\nhandler .*"
> > +
> > +set test "handler"
> > +gdb_test_multiple "stepi" $test {
> > +    -re "\r\nhandler .*" {
> > +	pass $test
> > +    }
> 
> Hi Carl,
> 
> gdb_test expects a gdb prompt after "\r\nhandler .*", but 
> gdb_test_multiple doesn't.
> 
> This causes:
> ...
> (gdb) PASS: gdb.base/kill-after-signal.exp: continue
> stepi^M
> handler (signo=32767) at 
> /home/vries/gdb_versions/devel/src/gdb/testsuite/gdb.base/kill-after\
> -signal.c:25^M
> 25      {^M
> PASS: gdb.base/kill-after-signal.exp: handler
> kill^M
> (gdb) FAIL: gdb.base/kill-after-signal.exp: kill
> ...
> 
> Triggered on x86_64-linux with taskset -c 0.
> 
> This fixes it for me:
> ...
> -    -re "\r\nhandler .*" {
> +    -re -wrap "\r\nhandler .*" {
> ...
> 
> Can you verify and commit?

Yes, I will re-check it and fix it.

                 Carl

[PATCH Fixup] PowerPC: bp-permanent.exp, kill-after-signal fix

[Carl Love]

Tom:

I took a look at the issue.  I retested gdb.base/kill-after-signal.exp
on PowerPC and my Intel x86-64 machine.  I don't see the failure when I
run the test as follows:
 
  make check RUNTESTFLAGS='GDB=/home/carll/bin/gdb   gdb.base/kill-after-signal.exp '

But I was able to reproduce the issue on Intel when I ran with taskset
-c 0 as follows:

 taskset -c 0 make check  RUNTESTFLAGS='GDB=/home/carll/bin/gdb   gdb.base/kill-after-signal.exp '

The -wrap fix you suggested takes care of the issue.  I find it curious
that it requires taskset -c 0 to trigger the issue.

I made the same -wrap fix to gdb.base/bp-permanent.exp.

I have added the fix to the patch that I was working on to fixup my
previous commit with changes that got missed in that commit.

I re-tested gdb.base/kill-after-signal.exp and bp-permanent.exp with
and without taskset -c 0.  The tests run correctly on PowerPC and x86-
64.

Please let me know if the patch looks OK and runs correctly on your
system as well.  Thanks for letting me know about the issue.

                  Carl Love

------------------------------------------------------------------

PowerPC: bp-permanent.exp, kill-after-signal fix

Fix changes that didn't make it into commit:
dd9cd55e990bcc9f8448cac38d242d53974b3604.

Fix missing -wrap on gdb_test_multiple in gdb.base/kill-after-signal.exp
that is causing regression test on x86_64-linux with taskset -c 0.  Also
added the -wrap to gdb.base/bp-permanent.exp.
---
 gdb/testsuite/gdb.base/bp-permanent.exp      | 17 ++++++++++-------
 gdb/testsuite/gdb.base/kill-after-signal.exp | 15 +++++++++------
 2 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/gdb/testsuite/gdb.base/bp-permanent.exp b/gdb/testsuite/gdb.base/bp-permanent.exp
index 21b0bc7bb2d..ede25ce39aa 100644
--- a/gdb/testsuite/gdb.base/bp-permanent.exp
+++ b/gdb/testsuite/gdb.base/bp-permanent.exp
@@ -257,31 +257,34 @@ proc test {always_inserted sw_watchpoint} {
 
 		set test "single-step to handler"
 		gdb_test_multiple "stepi" $test {
-		    -re "Program received signal SIGTRAP.*$gdb_prompt $" {
+		    -re -wrap "Program received signal SIGTRAP.*$gdb_prompt $" {
 			fail $test
 		    }
-		    -re ".*signal handler called.*$gdb_prompt $" {
-			# PowerPC Linux kernel patchs:
+		    -re "signal handler called.*$gdb_prompt $" {
+			# After PowerPC Linux kernel commit:
+			#
 			#   commit: 0138ba5783ae0dcc799ad401a1e8ac8333790df9
 			#   powerpc/64/signal: Balance return predictor
 			#   stack in signal trampoline.
 			#
 			# The kernel places an additional brctl instruction
-			# in the vdso to call the user hadler.
+			# in the vdso to call the user handler.
+			#
+			# And then this commit:
 			#
 			#   commit 24321ac668e452a4942598533d267805f291fdc9
 			#   powerpc/64/signal: Fix regression in
 			#   __kernel_sigtramp_rt64() semantics
 			#
-			# Updates the semantics of __kernel_sigtramp_rt64().
-			# It adds a new symbol to serve as a jump target from
+			# updates the semantics of __kernel_sigtramp_rt64().
+			# It added a new symbol to serve as a jump target from
 			# the kernel to the trampoline.
 			#
 			# The net result of these changes is that gdb stops
 			# at  __kernel_start_sigtramp_rt64.  Need to do one
 			# more stepi to reach the expected location in the user
 			# signal handler.
-			gdb_test "p \$pc" ".*__kernel_start_sigtramp_rt64.*" \
+			gdb_test "p \$pc" "__kernel_start_sigtramp_rt64.*" \
 			    "in kernel code"
 			gdb_test "stepi" "handler .*" $test
 		    }
diff --git a/gdb/testsuite/gdb.base/kill-after-signal.exp b/gdb/testsuite/gdb.base/kill-after-signal.exp
index fcbec9a1c2e..a3e30a67cc4 100644
--- a/gdb/testsuite/gdb.base/kill-after-signal.exp
+++ b/gdb/testsuite/gdb.base/kill-after-signal.exp
@@ -39,24 +39,27 @@ gdb_test "continue" "Program received signal SIGUSR1, .*"
 
 set test "handler"
 gdb_test_multiple "stepi" $test {
-    -re "\r\nhandler .*" {
+    -re -wrap "\r\nhandler .*" {
 	pass $test
     }
-    -re ".*signal handler called.*$gdb_prompt $" {
-	# PowerPC Linux kernel patchs:
+    -re "signal handler called.*$gdb_prompt $" {
+	# After PowerPC Linux kernel commit:
+	#
 	#   commit: 0138ba5783ae0dcc799ad401a1e8ac8333790df9
 	#   powerpc/64/signal: Balance return predictor
 	#   stack in signal trampoline.
 	#
 	# The kernel places an additional brctl instruction
-	# in the vdso to call the user hadler.
+	# in the vdso to call the user handler.
+	#
+	# And then this commit:
 	#
 	#   commit 24321ac668e452a4942598533d267805f291fdc9
 	#   powerpc/64/signal: Fix regression in
 	#   __kernel_sigtramp_rt64() semantics
 	#
-	# Updates the semantics of __kernel_sigtramp_rt64().
-	# It adds a new symbol to serve as a jump target from
+	# updates the semantics of __kernel_sigtramp_rt64().
+	# It added a new symbol to serve as a jump target from
 	# the kernel to the trampoline.
 	#
 	# The net result of these changes is that gdb stops
-- 
2.32.0

Re: [PATCH Fixup] PowerPC: bp-permanent.exp, kill-after-signal fix

[Tom de Vries]

On 5/9/22 22:43, Carl Love wrote:
>   		gdb_test_multiple "stepi" $test {
> -		    -re "Program received signal SIGTRAP.*$gdb_prompt $" {
> +		    -re -wrap "Program received signal SIGTRAP.*$gdb_prompt $" {
>   			fail $test
>   		    }

Hi Carl,

This doesn't look right.

So, there are essentially 3 things that do the same:

1.
gdb_test <cmd> <pat> <name>

2.
gdb_test_multiple <cmd> <name> {
   -re -wrap <pat> {
     pass $gdb_test_name
   }
}

3.
gdb_test_multiple <cmd> <name> {
   -re "\[\r\n\]*(?:<pat>)\[\r\n\]+$gdb_prompt $" {
     pass $gdb_test_name
   }
}

Put differently, -wrap is a shorthand that can be used in 
gdb_test_multiple to match a pattern in the same way that gdb_test 
would, which includes matching the prompt.

Specifying this:
...
   -re -wrap "Program received signal SIGTRAP.*$gdb_prompt $" {
...
means you're trying to match the prompt twice, once using -wrap, once 
using $gdb_prompt.

Thanks,
- Tom

RE: [PATCH Fixup] PowerPC: bp-permanent.exp, kill-after-signal fix

[Carl Love]

Tom:

On Tue, 2022-05-10 at 11:27 +0200, Tom de Vries wrote:
> On 5/9/22 22:43, Carl Love wrote:
> >   		gdb_test_multiple "stepi" $test {
> > -		    -re "Program received signal SIGTRAP.*$gdb_prompt
> > $" {
> > +		    -re -wrap "Program received signal
> > SIGTRAP.*$gdb_prompt $" {
> >   			fail $test
> >   		    }
> 
> Hi Carl,
> 
> This doesn't look right.
> 
> So, there are essentially 3 things that do the same:
> 
> 1.
> gdb_test <cmd> <pat> <name>
> 
> 2.
> gdb_test_multiple <cmd> <name> {
>    -re -wrap <pat> {
>      pass $gdb_test_name
>    }
> }
> 
> 3.
> gdb_test_multiple <cmd> <name> {
>    -re "\[\r\n\]*(?:<pat>)\[\r\n\]+$gdb_prompt $" {
>      pass $gdb_test_name
>    }
> }
> 
> Put differently, -wrap is a shorthand that can be used in 
> gdb_test_multiple to match a pattern in the same way that gdb_test 
> would, which includes matching the prompt.
> 
> Specifying this:
> ...
>    -re -wrap "Program received signal SIGTRAP.*$gdb_prompt $" {
> ...
> means you're trying to match the prompt twice, once using -wrap,
> once 
> using $gdb_prompt.

This is really helpful.  I have been a bit confused as to when to use
wrap.  This really helps.  So, I will take the -wrap that I just added
to the bp-permanent.exp test.  Adding that was a mistake.  Thanks.

                                   Carl 

RE: [PATCH Fixup V2] PowerPC: bp-permanent.exp, kill-after-signal fix

[Carl Love]

Tom, Pedro, GDB maintainers:

Per the feedback from Tom, I removed the -wrap that I had added
previously to the bp-permanent.exp test.  It was incorrect per Tom's
comments.  

Patch was re-tested on Power 10 with no test failures.

                       Carl Love

-------------------------------------------------------

PowerPC: bp-permanent.exp, kill-after-signal fix

Fix changes that didn't make it into commit:
dd9cd55e990bcc9f8448cac38d242d53974b3604.

Fix missing -wrap on gdb_test_multiple in gdb.base/kill-after-signal.exp
that is causing regression test on x86_64-linux with taskset -c 0.
---
 gdb/testsuite/gdb.base/bp-permanent.exp      | 15 +++++++++------
 gdb/testsuite/gdb.base/kill-after-signal.exp | 15 +++++++++------
 2 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/gdb/testsuite/gdb.base/bp-permanent.exp b/gdb/testsuite/gdb.base/bp-permanent.exp
index 21b0bc7bb2d..1ad9a698db8 100644
--- a/gdb/testsuite/gdb.base/bp-permanent.exp
+++ b/gdb/testsuite/gdb.base/bp-permanent.exp
@@ -260,28 +260,31 @@ proc test {always_inserted sw_watchpoint} {
 		    -re "Program received signal SIGTRAP.*$gdb_prompt $" {
 			fail $test
 		    }
-		    -re ".*signal handler called.*$gdb_prompt $" {
-			# PowerPC Linux kernel patchs:
+		    -re "signal handler called.*$gdb_prompt $" {
+			# After PowerPC Linux kernel commit:
+			#
 			#   commit: 0138ba5783ae0dcc799ad401a1e8ac8333790df9
 			#   powerpc/64/signal: Balance return predictor
 			#   stack in signal trampoline.
 			#
 			# The kernel places an additional brctl instruction
-			# in the vdso to call the user hadler.
+			# in the vdso to call the user handler.
+			#
+			# And then this commit:
 			#
 			#   commit 24321ac668e452a4942598533d267805f291fdc9
 			#   powerpc/64/signal: Fix regression in
 			#   __kernel_sigtramp_rt64() semantics
 			#
-			# Updates the semantics of __kernel_sigtramp_rt64().
-			# It adds a new symbol to serve as a jump target from
+			# updates the semantics of __kernel_sigtramp_rt64().
+			# It added a new symbol to serve as a jump target from
 			# the kernel to the trampoline.
 			#
 			# The net result of these changes is that gdb stops
 			# at  __kernel_start_sigtramp_rt64.  Need to do one
 			# more stepi to reach the expected location in the user
 			# signal handler.
-			gdb_test "p \$pc" ".*__kernel_start_sigtramp_rt64.*" \
+			gdb_test "p \$pc" "__kernel_start_sigtramp_rt64.*" \
 			    "in kernel code"
 			gdb_test "stepi" "handler .*" $test
 		    }
diff --git a/gdb/testsuite/gdb.base/kill-after-signal.exp b/gdb/testsuite/gdb.base/kill-after-signal.exp
index fcbec9a1c2e..a3e30a67cc4 100644
--- a/gdb/testsuite/gdb.base/kill-after-signal.exp
+++ b/gdb/testsuite/gdb.base/kill-after-signal.exp
@@ -39,24 +39,27 @@ gdb_test "continue" "Program received signal SIGUSR1, .*"
 
 set test "handler"
 gdb_test_multiple "stepi" $test {
-    -re "\r\nhandler .*" {
+    -re -wrap "\r\nhandler .*" {
 	pass $test
     }
-    -re ".*signal handler called.*$gdb_prompt $" {
-	# PowerPC Linux kernel patchs:
+    -re "signal handler called.*$gdb_prompt $" {
+	# After PowerPC Linux kernel commit:
+	#
 	#   commit: 0138ba5783ae0dcc799ad401a1e8ac8333790df9
 	#   powerpc/64/signal: Balance return predictor
 	#   stack in signal trampoline.
 	#
 	# The kernel places an additional brctl instruction
-	# in the vdso to call the user hadler.
+	# in the vdso to call the user handler.
+	#
+	# And then this commit:
 	#
 	#   commit 24321ac668e452a4942598533d267805f291fdc9
 	#   powerpc/64/signal: Fix regression in
 	#   __kernel_sigtramp_rt64() semantics
 	#
-	# Updates the semantics of __kernel_sigtramp_rt64().
-	# It adds a new symbol to serve as a jump target from
+	# updates the semantics of __kernel_sigtramp_rt64().
+	# It added a new symbol to serve as a jump target from
 	# the kernel to the trampoline.
 	#
 	# The net result of these changes is that gdb stops
-- 
2.32.0

Re: [PATCH PING Fixup V2] PowerPC: bp-permanent.exp, kill-after-signal fix

[Carl Love]

Tom:

I updated the patch per your comments.  Wanted to make sure it looks
ok.  Thanks for your help on this.

                     Carl

------------------------------------------------------

On Tue, 2022-05-10 at 12:07 -0700, Carl Love wrote:
> Tom, Pedro, GDB maintainers:
> 
> Per the feedback from Tom, I removed the -wrap that I had added
> previously to the bp-permanent.exp test.  It was incorrect per Tom's
> comments.  
> 
> Patch was re-tested on Power 10 with no test failures.
> 
>                        Carl Love
> 
> -------------------------------------------------------

PowerPC: bp-permanent.exp, kill-after-signal fix

Fix changes that didn't make it into commit:
dd9cd55e990bcc9f8448cac38d242d53974b3604.

Fix missing -wrap on gdb_test_multiple in gdb.base/kill-after-signal.exp
that is causing regression test on x86_64-linux with taskset -c 0.
---
 gdb/testsuite/gdb.base/bp-permanent.exp      | 15 +++++++++------
 gdb/testsuite/gdb.base/kill-after-signal.exp | 15 +++++++++------
 2 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/gdb/testsuite/gdb.base/bp-permanent.exp b/gdb/testsuite/gdb.base/bp-permanent.exp
index 21b0bc7bb2d..1ad9a698db8 100644
--- a/gdb/testsuite/gdb.base/bp-permanent.exp
+++ b/gdb/testsuite/gdb.base/bp-permanent.exp
@@ -260,28 +260,31 @@ proc test {always_inserted sw_watchpoint} {
 		    -re "Program received signal SIGTRAP.*$gdb_prompt $" {
 			fail $test
 		    }
-		    -re ".*signal handler called.*$gdb_prompt $" {
-			# PowerPC Linux kernel patchs:
+		    -re "signal handler called.*$gdb_prompt $" {
+			# After PowerPC Linux kernel commit:
+			#
 			#   commit: 0138ba5783ae0dcc799ad401a1e8ac8333790df9
 			#   powerpc/64/signal: Balance return predictor
 			#   stack in signal trampoline.
 			#
 			# The kernel places an additional brctl instruction
-			# in the vdso to call the user hadler.
+			# in the vdso to call the user handler.
+			#
+			# And then this commit:
 			#
 			#   commit 24321ac668e452a4942598533d267805f291fdc9
 			#   powerpc/64/signal: Fix regression in
 			#   __kernel_sigtramp_rt64() semantics
 			#
-			# Updates the semantics of __kernel_sigtramp_rt64().
-			# It adds a new symbol to serve as a jump target from
+			# updates the semantics of __kernel_sigtramp_rt64().
+			# It added a new symbol to serve as a jump target from
 			# the kernel to the trampoline.
 			#
 			# The net result of these changes is that gdb stops
 			# at  __kernel_start_sigtramp_rt64.  Need to do one
 			# more stepi to reach the expected location in the user
 			# signal handler.
-			gdb_test "p \$pc" ".*__kernel_start_sigtramp_rt64.*" \
+			gdb_test "p \$pc" "__kernel_start_sigtramp_rt64.*" \
 			    "in kernel code"
 			gdb_test "stepi" "handler .*" $test
 		    }
diff --git a/gdb/testsuite/gdb.base/kill-after-signal.exp b/gdb/testsuite/gdb.base/kill-after-signal.exp
index fcbec9a1c2e..a3e30a67cc4 100644
--- a/gdb/testsuite/gdb.base/kill-after-signal.exp
+++ b/gdb/testsuite/gdb.base/kill-after-signal.exp
@@ -39,24 +39,27 @@ gdb_test "continue" "Program received signal SIGUSR1, .*"
 
 set test "handler"
 gdb_test_multiple "stepi" $test {
-    -re "\r\nhandler .*" {
+    -re -wrap "\r\nhandler .*" {
 	pass $test
     }
-    -re ".*signal handler called.*$gdb_prompt $" {
-	# PowerPC Linux kernel patchs:
+    -re "signal handler called.*$gdb_prompt $" {
+	# After PowerPC Linux kernel commit:
+	#
 	#   commit: 0138ba5783ae0dcc799ad401a1e8ac8333790df9
 	#   powerpc/64/signal: Balance return predictor
 	#   stack in signal trampoline.
 	#
 	# The kernel places an additional brctl instruction
-	# in the vdso to call the user hadler.
+	# in the vdso to call the user handler.
+	#
+	# And then this commit:
 	#
 	#   commit 24321ac668e452a4942598533d267805f291fdc9
 	#   powerpc/64/signal: Fix regression in
 	#   __kernel_sigtramp_rt64() semantics
 	#
-	# Updates the semantics of __kernel_sigtramp_rt64().
-	# It adds a new symbol to serve as a jump target from
+	# updates the semantics of __kernel_sigtramp_rt64().
+	# It added a new symbol to serve as a jump target from
 	# the kernel to the trampoline.
 	#
 	# The net result of these changes is that gdb stops
-- 
2.32.0

Re: [PATCH PING Fixup V2] PowerPC: bp-permanent.exp, kill-after-signal fix

[Tom de Vries]

On 5/16/22 17:46, Carl Love wrote:
> Tom:
> 
> I updated the patch per your comments.  Wanted to make sure it looks
> ok.  Thanks for your help on this.
> 

Hi Carl,

LGTM.

Thanks,
- Tom

>                       Carl
> 
> ------------------------------------------------------
> 
> On Tue, 2022-05-10 at 12:07 -0700, Carl Love wrote:
>> Tom, Pedro, GDB maintainers:
>>
>> Per the feedback from Tom, I removed the -wrap that I had added
>> previously to the bp-permanent.exp test.  It was incorrect per Tom's
>> comments.
>>
>> Patch was re-tested on Power 10 with no test failures.
>>
>>                         Carl Love
>>
>> -------------------------------------------------------
> 
> PowerPC: bp-permanent.exp, kill-after-signal fix
> 
> Fix changes that didn't make it into commit:
> dd9cd55e990bcc9f8448cac38d242d53974b3604.
> 
> Fix missing -wrap on gdb_test_multiple in gdb.base/kill-after-signal.exp
> that is causing regression test on x86_64-linux with taskset -c 0.
> ---
>   gdb/testsuite/gdb.base/bp-permanent.exp      | 15 +++++++++------
>   gdb/testsuite/gdb.base/kill-after-signal.exp | 15 +++++++++------
>   2 files changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.base/bp-permanent.exp b/gdb/testsuite/gdb.base/bp-permanent.exp
> index 21b0bc7bb2d..1ad9a698db8 100644
> --- a/gdb/testsuite/gdb.base/bp-permanent.exp
> +++ b/gdb/testsuite/gdb.base/bp-permanent.exp
> @@ -260,28 +260,31 @@ proc test {always_inserted sw_watchpoint} {
>   		    -re "Program received signal SIGTRAP.*$gdb_prompt $" {
>   			fail $test
>   		    }
> -		    -re ".*signal handler called.*$gdb_prompt $" {
> -			# PowerPC Linux kernel patchs:
> +		    -re "signal handler called.*$gdb_prompt $" {
> +			# After PowerPC Linux kernel commit:
> +			#
>   			#   commit: 0138ba5783ae0dcc799ad401a1e8ac8333790df9
>   			#   powerpc/64/signal: Balance return predictor
>   			#   stack in signal trampoline.
>   			#
>   			# The kernel places an additional brctl instruction
> -			# in the vdso to call the user hadler.
> +			# in the vdso to call the user handler.
> +			#
> +			# And then this commit:
>   			#
>   			#   commit 24321ac668e452a4942598533d267805f291fdc9
>   			#   powerpc/64/signal: Fix regression in
>   			#   __kernel_sigtramp_rt64() semantics
>   			#
> -			# Updates the semantics of __kernel_sigtramp_rt64().
> -			# It adds a new symbol to serve as a jump target from
> +			# updates the semantics of __kernel_sigtramp_rt64().
> +			# It added a new symbol to serve as a jump target from
>   			# the kernel to the trampoline.
>   			#
>   			# The net result of these changes is that gdb stops
>   			# at  __kernel_start_sigtramp_rt64.  Need to do one
>   			# more stepi to reach the expected location in the user
>   			# signal handler.
> -			gdb_test "p \$pc" ".*__kernel_start_sigtramp_rt64.*" \
> +			gdb_test "p \$pc" "__kernel_start_sigtramp_rt64.*" \
>   			    "in kernel code"
>   			gdb_test "stepi" "handler .*" $test
>   		    }
> diff --git a/gdb/testsuite/gdb.base/kill-after-signal.exp b/gdb/testsuite/gdb.base/kill-after-signal.exp
> index fcbec9a1c2e..a3e30a67cc4 100644
> --- a/gdb/testsuite/gdb.base/kill-after-signal.exp
> +++ b/gdb/testsuite/gdb.base/kill-after-signal.exp
> @@ -39,24 +39,27 @@ gdb_test "continue" "Program received signal SIGUSR1, .*"
>   
>   set test "handler"
>   gdb_test_multiple "stepi" $test {
> -    -re "\r\nhandler .*" {
> +    -re -wrap "\r\nhandler .*" {
>   	pass $test
>       }
> -    -re ".*signal handler called.*$gdb_prompt $" {
> -	# PowerPC Linux kernel patchs:
> +    -re "signal handler called.*$gdb_prompt $" {
> +	# After PowerPC Linux kernel commit:
> +	#
>   	#   commit: 0138ba5783ae0dcc799ad401a1e8ac8333790df9
>   	#   powerpc/64/signal: Balance return predictor
>   	#   stack in signal trampoline.
>   	#
>   	# The kernel places an additional brctl instruction
> -	# in the vdso to call the user hadler.
> +	# in the vdso to call the user handler.
> +	#
> +	# And then this commit:
>   	#
>   	#   commit 24321ac668e452a4942598533d267805f291fdc9
>   	#   powerpc/64/signal: Fix regression in
>   	#   __kernel_sigtramp_rt64() semantics
>   	#
> -	# Updates the semantics of __kernel_sigtramp_rt64().
> -	# It adds a new symbol to serve as a jump target from
> +	# updates the semantics of __kernel_sigtramp_rt64().
> +	# It added a new symbol to serve as a jump target from
>   	# the kernel to the trampoline.
>   	#
>   	# The net result of these changes is that gdb stops