Skip to content

Properly modify Caml_state->backtrace_last_exn#12861

Merged
Octachron merged 1 commit intoocaml:trunkfrom
mshinwell:backtrace-last-exn-modification
Feb 1, 2024
Merged

Properly modify Caml_state->backtrace_last_exn#12861
Octachron merged 1 commit intoocaml:trunkfrom
mshinwell:backtrace-last-exn-modification

Conversation

@mshinwell
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

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

@Octachron Octachron merged commit 5f0cdb7 into ocaml:trunk Feb 1, 2024
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
Copy Markdown
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.

4 participants