Original bug ID: 4704
Reporter: @mshinwell
Assigned to: @xavierleroy
Status: closed (set by @xavierleroy on 2009-03-28T15:27:42Z)
Resolution: fixed
Priority: normal
Severity: crash
Version: 3.11.0
Fixed in version: 3.11.1+dev
Category: ~DO NOT USE (was: OCaml general)
Monitored by: rdouglass dvaillancourt BenediktGrundmann @mshinwell letaris sds yminsky @yakobowski pzimmer @alainfrisch @mmottl

Bug description

I believe the new generational global roots handling has a bug as follows. Suppose we initialize a variable to Val_unit and register its address as a generational global root. Nothing happens during the registration process because Is_block(Val_unit) does not hold. Suppose we then modify the contents of the root, using caml_modify_generational_global_root, to a value v such that Is_block(v) does hold. Unfortunately, since Is_block(oldval) doesn't hold in caml_modify_generational_global_root, the root isn't actually added to either the young or the old root lists as it should be. The root therefore never gets scanned, and a crash follows in due course.

I've been testing the patch below (against 3.11.0) which seems to work. I think the first new conditional is straightforward enough. The second is tricky though in the case where the root contained a boxed young value and is now being modified to contain an unboxed one. We could just leave the young global roots list alone in this case and wait for the next minor GC sweep to move the young roots onto the old roots list (at which point unboxed ones will be dropped). However, I think it better to explicitly remove it from the young list straight away, as the code does here. It avoids any possibility of subtle interactions with the user's expectations as to the root actually having been removed immediately following any subsequent call to the root-removal function after this boxed->unboxed modification. I can't actually spot any case in which there would actually be a crash if you don't remove it immediately, but some wierd stuff can certainly happen (for example if the user does "boxed->unboxed modify, remove root, directly set root contents to a non-young boxed value, insert root" then I think we could actually end up with the root on both lists).

--- byterun/globroots.c Thu Jan 08 16:32:00 2009 +0900
+++ byterun/globroots.c Thu Jan 29 09:42:45 2009 +0000
@@ -232,6 +232,26 @@
     caml_delete_global_root(&caml_global_roots_old, r);
     caml_insert_global_root(&caml_global_roots_young, r);
   }
+  else if (!Is_block(oldval) && Is_block(newval)) {
+    /* The previous value in the root was unboxed but now it is boxed.
+       The root won't appear in any of the root lists thus far (by virtue
+       of the operation of [caml_register_generational_global_root]), so we
+       need to make sure it gets in, or else it will never be scanned. */
+    if (Is_young(newval))
+      caml_insert_global_root(&caml_global_roots_young, r);
+    else if (Is_in_heap(newval))
+      caml_insert_global_root(&caml_global_roots_old, r);
+  }
+  else if (Is_block(oldval) && !Is_block(newval)) {
+    /* The previous value in the root was boxed but now it is unboxed, so
+       the root should be removed.  If [oldval] is young, this will happen
+       anyway at the next minor collection, but it is safer to delete it
+       here. */
+    if (Is_young(oldval))
+      caml_delete_global_root(&caml_global_roots_young, r);
+    else if (Is_in_heap(oldval))
+      caml_delete_global_root(&caml_global_roots_old, r);
+  }
   *r = newval;
 }