Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Properly modify Caml_state->backtrace_last_exn #12861

Merged
merged 1 commit into from
Feb 1, 2024

Conversation

mshinwell
Copy link
Contributor

Caml_state->backtrace_last_exn cannot be updated using direct assignment, as it is registered as a generational global root. Direct assignment can (and does) lead to crashes.

(Will add Changes entry later.)

Copy link
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clearly correct. Thanks!

Copy link
Contributor

@kayceesrk kayceesrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

The roots management for systhreads went through several iterations. It looks like this particular bug was introduced in ocaml-multicore/ocaml-multicore#771. In bringing the roots management closer to trunk, the modification of the generational global root Caml_state->backtrace_last_exn in restore_runtime_state was incorrectly changed to a plain store.

@gasche
Copy link
Member

gasche commented Jan 5, 2024

@mshinwell this needs a Changes entry. Please put in in the 5.2 section as we want to include this trivial fix in the release branch.

@Octachron Octachron added this to the 5.2 milestone Jan 24, 2024
@Octachron
Copy link
Member

@mshinwell , would you prefer if I took care of the changelog ?

@Octachron Octachron merged commit 5f0cdb7 into ocaml:trunk Feb 1, 2024
8 of 9 checks passed
Octachron added a commit that referenced this pull request Feb 1, 2024
Properly modify Caml_state->backtrace_last_exn

(cherry picked from commit 5f0cdb7)
@Octachron
Copy link
Member

I took the liberty to add the Changes entry, merge the PR, and cherry-pick it to 5.2.
@mshinwell, don't hesitate to edit the Changes if you wish so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants