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

[will schmidt <will_schmidt@vnet.ibm.com>]

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"