search

ocaml-flambda / flambda-backend

The Flambda backend project for OCaml
92 stars 65 forks source link

Fix missing restore/save_runtime_state in caml_c_thread_register for runtime5 #2739

Closed mshinwell closed 4 days ago

mshinwell commented 1 week ago

The systhreads lock was being taken prior to allocation on the OCaml heap during caml_c_thread_register, but the members of Caml_state were not being updated to correspond to the new thread prior to such allocation, nor restored afterwards. This could lead to segfaults. This PR was soak tested using a repro case that previously segfaulted.

stedolan commented 4 days ago

The fix looks good to me. It restores some logic that was present in 4: the call to restore_runtime_state is a clearer and more efficient version of this sequence from 4:

  /* Release the master lock */
  release_runtime_lock();
  /* Now we can re-enter the run-time system and heap-allocate the descriptor */
  caml_leave_blocking_section();

because leave_blocking_section has the side effect of restoring runtime state, as well as re-acquiring the lock.

I think it's worth comparing this PR to ocaml/ocaml#12834, in both directions:

mshinwell commented 4 days ago

It looks like this bug is fixed by the upstream PR in effectively the same way. Regarding other changes in that PR and the upstream trunk runtime more widely, we'll discuss separately.