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

[Pedro Alves <pedro@palves.net>]

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.