Page MenuHomeFreeBSD

kgdb: Handle stoppcbs compat
ClosedPublic

Authored by markj on May 3 2023, 3:50 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Mar 14, 2:38 AM
Unknown Object (File)
Thu, Feb 29, 4:45 PM
Unknown Object (File)
Feb 6 2024, 9:52 AM
Unknown Object (File)
Jan 10 2024, 4:37 AM
Unknown Object (File)
Dec 20 2023, 7:30 AM
Unknown Object (File)
Dec 11 2023, 3:16 AM
Unknown Object (File)
Nov 3 2023, 12:29 AM
Unknown Object (File)
Oct 29 2023, 1:31 PM
Subscribers

Details

Summary

Use __FreeBSD_version to decide whether to dereference the stoppcbs
symbol.

Diff Detail

Repository
R11 FreeBSD ports repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

markj requested review of this revision.May 3 2023, 3:50 PM

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

This revision is now accepted and ready to land.May 3 2023, 4:45 PM

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).

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?

This revision now requires review to proceed.May 9 2023, 4:28 PM

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.

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)

/* 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.

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

This revision is now accepted and ready to land.Sep 1 2023, 11:25 PM
/* 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.

In D39951#950207, @jhb 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 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.