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

[Carl Love <cel@us.ibm.com>]

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