kgdb: Handle stoppcbs compat

[markj]

Summary

Use __FreeBSD_version to decide whether to dereference the stoppcbs
symbol.

[emaste]

Logic LGTM but would like someone familiar with kgdb (i.e. @jhb) to approve.

[jhb]

Humm, this needs to unpush the target on error now. There is a helper class to do that for you that uses RAII (you reset it when you've finished setting up the target).

[markj]

In D39951#910891, @jhb wrote:

Humm, this needs to unpush the target on error now. There is a helper class to do that for you that uses RAII (you reset it when you've finished setting up the target).

You're referring to target_unpush_up?

[markj]

use target_unpusher

[markj]

Is there anything else I should do here? We should really get this integrated before 14.0. @jhb I can submit it as a PR against your gdb repo if that's easier.

[kevans]

I have the following diff applied atop this one: https://people.freebsd.org/~kevans/kgdb.diff -- I wasn't sure if there was a cleaner way to propagate osreldate around, so I just made it a global. There was one part I was less than certain about in handling fork_trampoline:

/* fork_exit hasn't been called (kthread has never run), so SP
   hasn't been initialized yet.  The stack pointer is stored in
   the X2 in the pcb.  */
sp = get_frame_register_unsigned (this_frame, AARCH64_X0_REGNUM + 2);

get_frame_pc (this_frame) == func implies we're still at the first instruction before sp has been stashed into x2, doesn't it? It's not quite clear to me why this wouldn't be AARCH64_SP_REGNUM instead, but I'm sure there's a reason. In any event, that definitely doesn't work in the new world order where we're not saving x2 in the pcb.

(edit to note: tested against /dev/mem both before and after the pcb and trapframe changes)

[kevans]

In D39951#949957, @kevans wrote:

/* fork_exit hasn't been called (kthread has never run), so SP
   hasn't been initialized yet.  The stack pointer is stored in
   the X2 in the pcb.  */
sp = get_frame_register_unsigned (this_frame, AARCH64_X0_REGNUM + 2);

get_frame_pc (this_frame) == func implies we're still at the first instruction before sp has been stashed into x2, doesn't it? It's not quite clear to me why this wouldn't be AARCH64_SP_REGNUM instead, but I'm sure there's a reason. In any event, that definitely doesn't work in the new world order where we're not saving x2 in the pcb.

It occurs to me that that doesn't make sense, we're relying on pcb state set in cpu_fork / cpu_copy_thread, which I don't think has ever pushed sp into x2 in the pcb.

[jhb]

This is ok. Both this and the aarch64 pcb one probably need to bump PORTREVISION.

[jhb]

In D39951#949958, @kevans wrote:

In D39951#949957, @kevans wrote:

/* fork_exit hasn't been called (kthread has never run), so SP
   hasn't been initialized yet.  The stack pointer is stored in
   the X2 in the pcb.  */
sp = get_frame_register_unsigned (this_frame, AARCH64_X0_REGNUM + 2);

get_frame_pc (this_frame) == func implies we're still at the first instruction before sp has been stashed into x2, doesn't it? It's not quite clear to me why this wouldn't be AARCH64_SP_REGNUM instead, but I'm sure there's a reason. In any event, that definitely doesn't work in the new world order where we're not saving x2 in the pcb.

It occurs to me that that doesn't make sense, we're relying on pcb state set in cpu_fork / cpu_copy_thread, which I don't think has ever pushed sp into x2 in the pcb.

Ah, I misread the assembly way back in the day. fork_trampoline saves SP in X2 at the start, not the other way around. I think instead this clause can just be removed.

[kevans]

In D39951#950207, @jhb wrote:

In D39951#949958, @kevans wrote:

In D39951#949957, @kevans wrote:

/* fork_exit hasn't been called (kthread has never run), so SP
   hasn't been initialized yet.  The stack pointer is stored in
   the X2 in the pcb.  */
sp = get_frame_register_unsigned (this_frame, AARCH64_X0_REGNUM + 2);

get_frame_pc (this_frame) == func implies we're still at the first instruction before sp has been stashed into x2, doesn't it? It's not quite clear to me why this wouldn't be AARCH64_SP_REGNUM instead, but I'm sure there's a reason. In any event, that definitely doesn't work in the new world order where we're not saving x2 in the pcb.

It occurs to me that that doesn't make sense, we're relying on pcb state set in cpu_fork / cpu_copy_thread, which I don't think has ever pushed sp into x2 in the pcb.

Ah, I misread the assembly way back in the day. fork_trampoline saves SP in X2 at the start, not the other way around. I think instead this clause can just be removed.

It's a fairly rare scenario to hit, to be fair. I created a dangling kthread on this here machine and tested the following patch:

diff --git a/devel/gdb/files/kgdb/aarch64-fbsd-kern.c b/devel/gdb/files/kgdb/aarch64-fbsd-kern.c
index be4d4e00ab90..394b0526f0c4 100644
--- a/devel/gdb/files/kgdb/aarch64-fbsd-kern.c
+++ b/devel/gdb/files/kgdb/aarch64-fbsd-kern.c
@@ -174,13 +174,6 @@ aarch64_fbsd_trapframe_cache (frame_info_ptr this_frame, void **this_cache)
   sp = get_frame_register_unsigned (this_frame, AARCH64_SP_REGNUM);
 
   find_pc_partial_function (func, &name, NULL, NULL);
-  if (strcmp(name, "fork_trampoline") == 0 && get_frame_pc (this_frame) == func)
-    {
-      /* fork_exit hasn't been called (kthread has never run), so SP
-        hasn't been initialized yet.  The stack pointer is stored in
-        the X2 in the pcb.  */
-      sp = get_frame_register_unsigned (this_frame, AARCH64_X0_REGNUM + 2);
-    }
 
   tf_size = regcache_map_entry_size (trapframe_map);
   trad_frame_set_reg_regmap (cache, trapframe_map, sp, tf_size);

Before:

(kgdb) thread 1702
[Switching to thread 1702 (Thread 100003)]
#0  <signal handler called>
(kgdb) bt
Python Exception <class 'gdb.error'>: Register 2 is not available
#0  <signal handler called>

After:

(kgdb) thread 1702                                                                                            
[Switching to thread 1702 (Thread 100003)]                                                                    
#0  <signal handler called>                                                                                   
(kgdb) bt                                                                                                     
#0  <signal handler called>

Nothing too exciting, as one might have hoped... I'll roll this into D41684 unless you'd prefer I didn't.

@markj are you wanting to commit this, or do you mind if I just grab it when I push the other one + bump PORTREVISION?

[markj]

In D39951#950771, @kevans wrote:

@markj are you wanting to commit this, or do you mind if I just grab it when I push the other one + bump PORTREVISION?

I don't mind at all, please go ahead. Thanks in advance.