port-hppa/56866: hppa: kernel gets confused between actual breakpoints and single-step breakpoints

[tgl%sss.pgh.pa.us@localhost]

>Number:         56866
>Category:       port-hppa
>Synopsis:       hppa: kernel gets confused between actual breakpoints and single-step breakpoints
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    port-hppa-maintainer
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Tue Jun 07 01:45:00 +0000 2022
>Originator:     Tom Lane
>Release:        HEAD/202206030100Z
>Organization:
PostgreSQL Global Development Group
>Environment:
NetBSD sss2.sss.pgh.pa.us 9.99.97 NetBSD 9.99.97 (SD2) #0: Fri Jun  3 12:30:06 EDT 2022  tgl%nuc1.sss.pgh.pa.us@localhost:/home/tgl/netbsd-H-202206030100Z/obj.hppa/sys/arch/hppa/compile/SD2 hppa
>Description:
The stepN and setstepN (for N>1) test cases in t_ptrace_wait and sibling test programs frequently fail with

failed: /home/tgl/netbsd-H-202206030100Z/usr/src/tests/lib/libc/sys/t_ptrace_step_wait.h:143: info.psi_siginfo.si_code != TRAP_TRACE

Investigation shows that the kernel is reporting si_code = TRAP_BRKPT rather than the expected TRAP_TRACE.  I believe the reason for this is that the identical "break 0,4" instruction is inserted for both true breakpoints (those inserted from userland using PTRACE_BREAKPOINT or PTRACE_BREAKPOINT_ASM as prototype code) and single-stepping (which temporarily inserts SSBREAKPOINT which is the same thing).  In hppa's trap.c, we report SIGTRAP with si_code = TRAP_BRKPT if the trapping instruction was SSBREAKPOINT, or si_code = TRAP_TRACE if it wasn't.  Thus, we're going to produce TRAP_BRKPT during single-stepping, which if not wrong is at least not what the test case expects.
>How-To-Repeat:
$ cd /usr/tests/
$ atf-run lib/libc/sys/t_ptrace_wait

Note that this test has other issues too.  What I'm on about right now are the reports of "si_code != TRAP_TRACE".

>Fix:
I assume we don't want to change PTRACE_BREAKPOINT since that's known to userland.  Changing trap.c to use a different break code for single-stepping is a much more localized change, and it seems to fix this issue (though I've not tested very extensively for other side effects).  An annoying cosmetic problem is that PTRACE_BREAKPOINT is defined in terms of HPPA_BREAK_SS, which is completely misleading if we separate these functions.  I did this as a quick hack for testing:

Index: sys/arch/hppa/include/trap.h
===================================================================
RCS file: /cvsroot/src/sys/arch/hppa/include/trap.h,v
retrieving revision 1.5
diff -u -r1.5 trap.h
--- sys/arch/hppa/include/trap.h        6 Sep 2021 21:56:03 -0000       1.5
+++ sys/arch/hppa/include/trap.h        7 Jun 2022 01:13:05 -0000
@@ -99,6 +99,7 @@
 /* im13 */
 #define        HPPA_BREAK_SS           4
 #define        HPPA_BREAK_KGDB         5
+#define        HPPA_BREAK_SS1          6
 #define        HPPA_BREAK_GET_PSW      9
 #define        HPPA_BREAK_SET_PSW      10
 
Index: sys/arch/hppa/hppa/trap.c
===================================================================
RCS file: /cvsroot/src/sys/arch/hppa/hppa/trap.c,v
retrieving revision 1.117
diff -u -r1.117 trap.c
--- sys/arch/hppa/hppa/trap.c   28 May 2022 21:14:56 -0000      1.117
+++ sys/arch/hppa/hppa/trap.c   7 Jun 2022 01:13:05 -0000
@@ -108,8 +108,10 @@
 int ss_put_value(struct lwp *, vaddr_t, u_int);
 int ss_get_value(struct lwp *, vaddr_t, u_int *);
 
+/* externally set breakpoint (see PTRACE_BREAKPOINT) */
+#define EXTBREAKPOINT  (HPPA_BREAK_KERNEL | (HPPA_BREAK_SS << 13))
 /* single-step breakpoint */
-#define SSBREAKPOINT   (HPPA_BREAK_KERNEL | (HPPA_BREAK_SS << 13))
+#define SSBREAKPOINT   (HPPA_BREAK_KERNEL | (HPPA_BREAK_SS1 << 13))
 
 #endif
 
@@ -690,7 +692,7 @@
                ksi.ksi_addr = (void *)(frame->tf_iioq_head & ~HPPA_PC_PRIV_MASK);
 #ifdef PTRACE
                ss_clear_breakpoints(l);
-               if (opcode == SSBREAKPOINT)
+               if (opcode == EXTBREAKPOINT)
                        ksi.ksi_code = TRAP_BRKPT;
 #endif
                /* pass to user debugger */

It would be better to rename HPPA_BREAK_SS to HPPA_BREAK_BR or something like that, but I don't know if that's OK from an API stability standpoint.