Skip to content

Conversation

@recoules
Copy link
Contributor

Closes #11633.

Copy link
Contributor

@Ekdohibs Ekdohibs left a comment

Choose a reason for hiding this comment

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

This looks correct to me!

@gasche
Copy link
Member

gasche commented Dec 9, 2022

@Ekdohibs thanks! Do you know if this bug also requires a fix in 5.0?

(cc @Octachron: I'm happy to approve on @Ekdohibs' behalf, but can we include new fixes in 4.14 now that a RC is out?)

@Octachron
Copy link
Member

We are indeed lacking a development branch for 4.14 right now. It would be better to merge the PR after the 4.14.1 release in less than two weeks. Then the fix will be part of the 4.14.2 release.

@Octachron Octachron added this to the 4.14.2 milestone Dec 9, 2022
@gasche
Copy link
Member

gasche commented Dec 9, 2022

(If there is something to fix in trunk, possibly by just rebasing the proposed patch, we can merge it right now; ideally someone would do a separate PR for this.)

@Ekdohibs
Copy link
Contributor

Ekdohibs commented Dec 9, 2022

I'm not sure if there are any changes to do in trunk, since the logic for freeing frame descriptors changed quite a lot:

/* it's now safe to free the old table(s) */
caml_plat_lock(&descr_mutex);
if (ft->prev != NULL) {
struct frametable_version *p = ft->prev;
while (p != NULL) {
struct frametable_version *next = p->prev;
caml_stat_free(p->table.descriptors);
caml_stat_free(p);
p = next;
}
ft->prev = NULL;
atomic_store_rel(&ft->free_prev_after_cycle, No_need_to_free);
}
caml_plat_unlock(&descr_mutex);

Copy link
Contributor

@nojb nojb left a comment

Choose a reason for hiding this comment

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

This needs a Changes entry I think.

@nojb nojb added the bug label Dec 30, 2022
@gasche gasche self-assigned this Oct 4, 2023
@gasche gasche force-pushed the fix_unregister_frametable branch from fe8c9a9 to fe0ddc9 Compare October 4, 2023 13:28
@gasche gasche added the merge-me label Oct 4, 2023
@gasche gasche merged commit 9d3bd18 into ocaml:4.14 Oct 13, 2023
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.

6 participants