Is there's a scenario involving caml_callback where that corrupts the list?

Right, local_roots must be cleaned up. Also, the test

  if (exception_pointer == NULL) caml_fatal_uncaught_exception(v);

must be performed! Otherwise an out-of-bound array access without any try...with will crash the program instead of being reported.

I took the liberty to push a more prudent version directly on this branch. It adds a local caml_raise_internal function that can be instructed to skip the check for pending actions. Feel free to discard.

Thanks for pointing out the issue with this patch, but I think a slightly different fix would be simpler. The issue is that the requirements for raising an exception are very different when the exception is raised from C vs. from OCaml. In OCaml:

• there must not be a poll before the raise, since the compiler does not populate frametables around raises
• there is always an exception handler, since one is installed by caml_start_program
• there is no need to clean up local_roots, since there are no C frames before the nearest exception handler

while when an exception is raised from C:

• there may be a poll before the raise, since there are always populated frametables around C calls that may raise
• there may not be an active exception handler
• local_roots must be cleaned up before the raise

The problem is that caml_array_bound_error is sometimes called from OCaml (failed bounds checks call it) and sometimes called from C (it is exposed in caml/fail.h). The trunk implementation uses the C rules, which are incorrect for OCaml code and cause memory corruption.

My initial patch used the OCaml rules instead, by directly calling caml_raise_exception and skipping the poll / active handler check / local roots logic. While this is correct for bounds check failures in OCaml, I hadn't realised that caml_array_bound_error may also be called from C code.

The current version of this patch (thanks to @xavierleroy) manages to, I think, implement the safe common subset of the OCaml and C requirements, by not polling, checking for a handler, and optionally cleaning up local_roots. However, I think a simpler approach would be to separate the two uses, leaving caml_array_bound_error as it is (callable by C code), and introducing a new caml_ml_array_bound_error which is only invoked from ocamlopt-compiled code, not exposed in a C header, and works the way my original patch did.

(I'm about to go on vacation for a week-and-a-bit. I'd hoped to write and push another version of this patch beforehand, but oh well)

I've just rebased and pushed a new version of the code. This version keeps the original behaviour of caml_array_bound_error when called by C, but directly jumps to the OCaml exception handler when a bounds check failure is raised in OCaml code.

I've taken a look. I think the proposed solution handles the problem where a GC is triggered off the bounds exception in a straight forward way.

However I'm not convinced we can ignore the frame table construction in the case where backtraces are stashed on some platforms. On ARM64 MacOS, the native compiler gives a repeating (until the buffer is filled) backtrace for the following test:

(* TEST
   flags = "-g"
   ocamlrunparam += ",b=1"
*)
let xs = [| 0; 1; 2 |]

let bad_bound_fn x =
  !x + xs.(100)

let _ =
  try
    ignore (Sys.opaque_identity (bad_bound_fn (ref 0)));
  with exn ->
    Printf.printf "Uncaught exception %s\n" (Printexc.to_string exn);
    Printexc.print_backtrace stdout;

  print_endline "OK"

The issue is that the backtrace tries to walk the stack but on ARM64 (as it stands) we are not able to do this. x86 seems (at first glance) to not give garbage in this situation.

This backtrace problem might not be for this PR to fix. This PR doesn't make the backtrace situation worse but does fix segfaults and hangs as things stand.

However I'm not convinced we can ignore the frame table construction in the case where backtraces are stashed on some platforms

I don't think this PR does this. In the current code base, ocamlopt -g generates a frame descriptor for each call to caml_ml_array_bound_error, precisely so that backtraces can be produced. This PR does not change this, does it?

The issue is that the backtrace tries to walk the stack but on ARM64 (as it stands) we are not able to do this. x86 seems (at first glance) to not give garbage in this situation.

That sounds like perhaps an independent bug in the ARM64 backend? A frame table entry is created and it should have a correct stack frame length and debug info, which is all you need to construct the backtrace. (What's missing is that it does not have a valid set of live registers, so you can't do GC at this point if the handler is in the same function)

I don't think this PR does this. In the current code base, ocamlopt -g generates a frame descriptor for each call to caml_ml_array_bound_error, precisely so that backtraces can be produced. This PR does not change this, does it?

You've convinced me that the problem is probably in the ARM64 backend with -g and orthogonal to this PR. I'll do a bit more digging to try and track it down.

My understanding of the current status:

• @sadiqj and @xavierleroy have taken a look at a previous version
• @stedolan rewrote the PR after feedback from @xavierleroy, but the new version has not been reviewed yet (in particular, not approved)

Merged in trunk and cherry-picked to 5.0

FYI: this PR breaks frame-pointer support. The following tests produce no backtraces when the out-of-bound exception is raised.

    tests/backtrace/'backtrace_slots.ml' with 1 (native) 
    tests/backtrace/'backtrace_bounds_exn.ml' with 1 (native) 
    tests/backtrace/'raw_backtrace.ml' with 1 (native) 
    tests/backtrace/'backtrace.ml' with 1 (native) 
    tests/backtrace/'backtrace3.ml' with 1 (native) 
    tests/backtrace/'backtrace_deprecated.ml' with 1 (native) 
    tests/backtrace/'backtrace2.ml' with 1 (native) 

(CC: @fabbing)

Re-adding the ENTER_FUNCTION that was removed in this PR seems to fix the issue in frame-pointers mode and has no ill effects otherwise, so I'll push this fix straight to trunk.

Yes, you do need this ENTER_FUNCTION because caml_raise_exn https://github.com/stedolan/ocaml/blob/14a1d20cd00e5ff5b0b2985be12fbd55b0ba9507/runtime/amd64.S#L777 expects to have to jump over rbp to get the saved rip.
Without this instruction, the pc given to caml_stash_backtrace is wrong and it prevents us from finding the function frame_descr.

If anyone wonders: ENTER_FUNCTION maintains %rbp as frame pointer in "with frame pointers" mode, and does nothing otherwise. I guess it would have to be matched by a LEAVE_FUNCTION just before tail-calling caml_c_call. @stedolan chose to omit the ENTER_FUNCTION / LEAVE_FUNCTION dance entirely, which is correct too and probably just as good in practice.

(If I understand correctly, the only case where it might make a difference is in frame-pointer mode, if a profiling interrupt hits while executing the LEA_VAR instruction, in which case the %rbp-based backtrace will not be accurate. Nothing to worry about.)

LEAVE_FUNCTION isn't required here because caml_ml_array_bound_error tail calls into caml_c_call (after its own ENTER_FUNCION).