[runtime] Stricter exn_handler processing

[ghost]

This innocent-looking PR is intended to fix the issue noticed by @polytypic, and (very painfully) reproduceable using his testcase. (he did not open an issue for this in the OCaml project)

The bug being fixed here, is that, on platforms where the current value of the domain's exn_handler is kept in a register for speed (i.e. TRAP_PTR), reloading the register after its value may have changed has been missed in caml_c_call and/or caml_c_call_stack_args.

This wasn't noticed because:

1. this only happens if the invoked C code reaches back to OCaml code, and
2. the OCaml code needs to grow its stack, and
3. upon return from the invoked C code, there is an exception pending.

And then, as long as the debug runtime is not used and the exception is not reraised, TRAP_PTR will eventually return to a correct value without causing havoc.

Which is why this was so hard to reproduce (and of course, amd64 and s390x were not affected). With @polytypic's test case, I have been able to reproduce quite reliably, using the debug runtime, on OpenBSD/arm64, thanks to scheduler or libpthread behaviour on this platform causing caml_unix_read to return with an exception at the end of the test program, reasonably often (it nevertheless takes about 200 runs of the test program to reach this).

Also, since amd64 does not save this value into a dedicated register, other runtime .S files which have been using amd64 as inspiration could easily miss this - in fact it was more surprising to see s390x handle this correctly (at least in the caml_c_call case).

This fix should ideally be backported to 5.1.x and 5.2.x.

[xavierleroy]

Well spotted! You're correct that in OCaml 5, unlike in OCaml 4, growing the OCaml stack changes the value of the exception pointer (the address of the first exception handler in the stack), so caml_c_call must not assume that the exception pointer is not changed by the C code.

However, there's no need for "upon return from the invoked C code, there is an exception pending", nor for the debug runtime system to be used: as soon as caml_c_call returns with the exception pointer register pointing to an old stack (before resizing took place), raising an exception will access the old stack, which has been freed.

On a Mac M1, I'm getting a reproducible "bus error" with the modified test below and when setting MallocScribble=1, so that the old stack block is overwritten with garbage data.

diff --git a/testsuite/tests/callback/test1.ml b/testsuite/tests/callback/test1.ml
index c39be0c586..9a0f6c5de5 100644
--- a/testsuite/tests/callback/test1.ml
+++ b/testsuite/tests/callback/test1.ml
@@ -11,6 +11,9 @@ external mycallback3 : ('a -> 'b -> 'c -> 'd) -> 'a -> 'b -> 'c -> 'd
 external mycallback4 :
     ('a -> 'b -> 'c -> 'd -> 'e) -> 'a -> 'b -> 'c -> 'd -> 'e = "mycallback4"
 
+let rec growstack n =
+  if n <= 0 then 0 else 1 + growstack (n - 1)
+
 let rec tak (x, y, z as _tuple) =
   if x > y then tak(tak (x-1, y, z), tak (y-1, z, x), tak (z-1, x, y))
            else z
@@ -46,3 +49,5 @@ let _ =
   print_int(trapexit ()); print_newline();
   print_string(tripwire mypushroot); print_newline();
   print_string(tripwire mycamlparam); print_newline();
+  begin try ignore (mycallback1 growstack 100_000); raise Exit
+  with Exit -> () end

[shindere]

For those interested: https://developer.apple.com/library/archive/documentation/Performance/Conceptual/ManagingMemory/Articles/MallocDebug.html

[ghost]

Thanks @xavierleroy for that simple reproducer case! I'll borrow it, but with a smaller recursion bound to prevent failure with a Stack_overflow exception on arm64.

[gasche]

Note: if I understand correctly, all 64bit backends are affected, except amd64{,nt} where the trap pointer is not cached as a register. (I suppose this is because amd64 have less registers available than other isas.) This is probably a small mistake that was copied from one backend to the next when they were adapted to Multicore.

[gasche]

I reviewed the fix and believe it is correct.

[gasche]

Caching the trap pointer makes try/raise slightly faster (one less read from memory, although Caml_state is hot so certainly in cache), and it makes C calls slightly slower (one extra register to spill); it costs one register. I suppose this is worth it because we assume that typical OCaml code may rely on exceptions being fast, and does not use C calls that often.

[ghost]

Note: if I understand correctly, all 64bit backends are affected, except amd64{,nt} where the trap pointer is not cached as a register. (I suppose this is because amd64 have less registers available than other isas.) This is probably a small mistake that was copied from one backend to the next when they were adapted to Multicore.

Your understanding is correct.

[Octachron]

This fix should ideally be backported to 5.1.x and 5.2.x.

I agree that the fix should be backported to 5.2. If the backport to 5.1 is straightforward, it would make sense to have the fix in the 5.1 maintenance branch, but I am not expecting a 5.1.2 release any time soon (because I would rather focus on 5.2.1).

[gasche]

The change is pretty simple for each architecture (it's one line missing), so this should be easy to backport. It will probably not backport automatically due to some architectures being absent in earlier releases. I agree that backporting to 5.1 has unclear benefits -- I don't see any reason for users to stick to 5.1 instead of moving to more recent releases.

[ghost]

The change is pretty simple for each architecture (it's one line missing), so this should be easy to backport. It will probably not backport automatically due to some architectures being absent in earlier releases. I agree that backporting to 5.1 has unclear benefits -- I don't see any reason for users to stick to 5.1 instead of moving to more recent releases.

Actually, I had checked and all the .S files already exist in 5.1. But it is not worth applying the changes to power.S since it is in a non-working state in 5.1.

[xavierleroy]

I'm running a "CI precheck" on this PR, so that the architectures impacted by this PR are all tested.

[xavierleroy]

I suppose this is worth it because we assume that typical OCaml code may rely on exceptions being fast, and does not use C calls that often.

I think this is beneficial for target architectures having 32 integer registers, where it's no big deal to reserve a register to hold the exception pointer. With 16 integer registers, this is probably not true. So, for s390x, it could make more sense to not keep the exception pointer in a register.

[gasche]

(Once the precheck CI results are in, I think that it would reasonable to go ahead, merge, and backport to 5.2; but I will leave this decision to @xavierleroy. If we want to consider slightly more invasives change to s390x this can come as a further PR.)

[xavierleroy]

Looks good to me. CI precheck is positive. I'll merge next.

[xavierleroy]

5.2 backport: 1e8a91d