I was also looking at this, but you are more advanced. If I can help with reviews or testing, let me know.

This looks rather good to me. Why don't you remove the Draft status so that it can get reviewed properly?

Ping. Why don't you remove the Draft status so that it can get reviewed properly?

Stack backtrace is now working. The next instruction still behaves a little strange around fibers, but the debugger is already usable. I'll do another PR for next but I think it's less urgent.

A bit of code may have been lost during the PR and this should probably be added because the trap barrier doesn't seem to work anymore:

diff --git a/runtime/debugger.c b/runtime/debugger.c
index 187f681135..a220915784 100644
--- a/runtime/debugger.c
+++ b/runtime/debugger.c
@@ -560,6 +560,8 @@ void caml_debugger(enum event_kind event, value param)
       caml_flush(dbg_out);
       break;
     case REQ_SET_TRAP_BARRIER:
+      i = caml_getword(dbg_in);
+      CAMLassert(i == frame_block_number(frame_block));
       i = caml_getword(dbg_in);
       Caml_state->trap_barrier_off = -i;
       break;

I noticed the problem, thanks to @OlivierNicole, while trying to review this PR (I'm very new to bytecode and ocamldebug).

However, I don't think it's enough and trap_barrier_off should probably be a tuple { block, offset }.
Even a tuple may still cause issue if we switch to a different fiber at the same depth (block), in this case raising an exception could trigger a trap barrier into the wrong fiber.

@fabbing Thanks for finding this problem. This breaks the finish debugger command. I'm working on a solution.

This is better than the "depth" in terms of number of fibers but will run into troubles if the fiber is reallocated since the struct stack_info* will change (see caml_try_realloc_stack from runtime/fiber.c).
As far as I know, there is currently no way to identify a fiber.
I was able to have a quick discussion on this topic with @kayceesrk and it seemed reasonable to add an unique identifier for each fiber. I'm working on a PR for this, I hope to have it ready soon.

Adapted to use fiber IDs instead of addresses.
Another possibility would be to have caml_try_realloc_stack update Caml_state->trap_barrier_block as needed.

I've resumed reviewing the PR and it looked good to me.
But I noticed that the tool-debugger tests from the testsuite where still skipped and tried to re-enable them again. All but one pass. The dynlink test fails with Fatal error: Fatal error during unlock: Operation not permitted.

It's not straightforward to debug such test as many fork are involved in the process.
I was able to pinpoint the issue at channel_mutex_unlock_default while attempting to create a checkpoint (debugger.c:485).
I don't understand why this only happens in this test and keep investigating!

With the OCaml 5 runtime you cannot safely call fork from C and then execute any OCaml code on the child that would release the runtime lock, you have to do a special dance first (and meditate on the sins of doing anything else than running an exec* function). The error I encountered in this situation is similar to yours, maybe look at #11111 to see if it is related; in your stead I would look at uses of fork in runtime/debugger.c.

Thanks a lot for the hint @gasche. I had a look in that direction but it turns out to be something different:

This test raises an exception (Not_found from Sys.getenv) which calls Unlock_exn / channel_mutex_unlock_exn_default and since dbg_out channel was locked last, causes it to be unlocked.
Later, when trying to create a checkpoint, an attempt is made to unlock dbg_out before forking which fails since it's no longer locked.

A not very graceful solution is to set last_channel_locked to NULL to avoid this:

--- a/runtime/caml/io.h
+++ b/runtime/caml/io.h
@@ -115,6 +115,8 @@ CAMLextern struct channel * caml_all_opened_channels;
 #define Flush_if_unbuffered(channel) \
   if (channel->flags & CHANNEL_FLAG_UNBUFFERED) caml_flush(channel)
 
+CAMLextern void caml_channel_forget_last(void);
+
 /* Conversion between file_offset and int64_t */
 
 #define Val_file_offset(fofs) caml_copy_int64(fofs)
diff --git a/runtime/debugger.c b/runtime/debugger.c
index 861c37e3bb..448098e2fb 100644
--- a/runtime/debugger.c
+++ b/runtime/debugger.c
@@ -143,6 +143,7 @@ static void open_connection(void)
      private to this code and never used concurrently. */
   Lock (dbg_in);
   Lock (dbg_out);
+  caml_channel_forget_last();
 
   if (!caml_debugger_in_use) caml_putword(dbg_out, -1); /* first connection */
 #ifdef _WIN32
@@ -479,6 +480,7 @@ void caml_debugger(enum event_kind event, value param)
       i = fork();
       Lock (dbg_in);
       Lock (dbg_out);
+      caml_channel_forget_last();
       caml_acquire_domain_lock ();
       if (i == 0) {
         close_connection();     /* Close parent connection. */
diff --git a/runtime/io.c b/runtime/io.c
index 78bfea6d8a..fae4d6239e 100644
--- a/runtime/io.c
+++ b/runtime/io.c
@@ -106,6 +106,11 @@ CAMLexport void (*caml_channel_mutex_unlock_exn) (void)
 /* List of opened channels */
 CAMLexport struct channel * caml_all_opened_channels = NULL;
 
+CAMLexport void caml_channel_forget_last(void)
+{
+  last_channel_locked = NULL;
+}
+
 /* Basic functions over type struct channel *.
    These functions can be called directly from C.
    No locking is performed. */

Thanks again to @OlivierNicole for his help in understanding channel behaviour.

Unfortunately, this dynlink test still fails because unable to place a breakpoint in Plugin:

Can't find any event there.

Investigation continues!

Can't find any event there.

Fixed by 3b34535, let's see if the testsuite passes now.

Do we also want a "symmetrical" behavior with resume that would trigger a trap like perform?

--- a/runtime/interp.c
+++ b/runtime/interp.c
@@ -1284,6 +1284,7 @@ value caml_interprete(code_t prog, asize_t prog_size)
       goto do_resume;
 
 do_resume: {
+      check_trap_barrier_for_effect(domain_state);
       struct stack_info* stk = Ptr_val(accu);
       if (stk == NULL) {
          accu = Field(caml_global_data, CONTINUATION_ALREADY_TAKEN_EXN);

Either way it looks good to me!

can we get an official review ?

Yes, but please leave me 1-2 weeks to work on it. I'm thinking about a radical solution to the locking problems that have plagued this PR. Stay tuned.

Here is a preview of the "radical solution" I propose: damiendoligez#7

Ideally, I should make a separate PR with the basic changes, have it merged, and then you would rebase this PR on top of this work. But first I'd like you (@damiendoligez , @fabbing , ...) to have a quick look at the approach and comment.

I took the liberty to rebase on trunk and hand-merge damiendoligez#7. I'm running a CI precheck test.

precheck is at https://ci.inria.fr/ocaml/job/precheck/726/

Cherry-picked in 5.0: b4a61f5