Segfault with no-naked-pointers and recursive modules

[stedolan]

Trunk can currently segfault during GC, due to a bad interaction between the new closure representation (relied upon in no-naked-pointers mode) and the way that recursive modules are initialised.

During recursive module initialisation, a stub closure is directly overwritten with the contents of another closure, once the recursive closure has been built. The current code in camlInternalMod is careful to ensure that both the old and new closures are valid and the correct intrinsics in Obj are used to mutate them.

However, this isn't enough, as incremental garbage collection can still get mixed up between the old and new layouts. The sequence of events that goes wrong is:

• Stub closure created with env_offset = 2, length = 11
• GC begins, pushes an entry to the mark stack representing the interval [2, 11] of the stub closure's fields
• Recursive module initialisation constructs final closure (with env_offset = 3) and overwrites stub
• GC continues, popping closure fields [2, 11] from mark stack and starts marking

We end up marking field 2 of a closure which by that point has env_offset = 3. Field 2 of this closure is not a markable value (it's a code pointer), so this segfaults.

(This came up during an attempt to benchmark #10195 using sandmark: cubicle segfaults on startup due to this bug)

[xavierleroy]

Incremental GC is going too far! :-) I've always had mixed feelings about stopping GC marking in the middle of a heap block, and now we see what can happen...

I'm afraid the only solution is to always go through a trampoline function (the else case below) and not overwrite directly with the new closure.

ocaml/stdlib/camlinternalMod.ml

Lines 92 to 97 in 0553723

	if Obj.tag n = Obj.closure_tag
	&& (Obj.size n = Obj.size o
	|| (Sys.backend_type = Sys.Native
	&& Obj.size n <= Obj.size o))
	then begin overwrite_closure o n end
	else overwrite_closure o (Obj.repr (fun x -> (Obj.obj n : _ -> _) x))

Performance will suffer a bit, but never mind.

(As a side effect, overwrite_closure could be much simplified if we know that the old and the new closures have exactly the same shape.)

[gasche]

If I understand correctly, the suggestion is to use the trampoline when the shape of the closures are distinct (rather than always, for all function backpatching). The dummy closure used in init_mod has start_env = 2, which corresponds to all not-mutually-recursive functions, so the in-place backpatching is still available whenever the new function is not part of a mutually-recursive definition. So in this common case there should be no performance penalty.

Note that if a user observes a performance degradation due to the change (probably no one), they are initializing a publicly-visible function f inside a mutually-recursive module M. It is always possible to not use a mutually-recursive definition for f: if the definition was let rec f = ...g... and g = ...f..., the user can rephrase it as let rec g = ...M.f... ;; let f = ...g.... (I don't know if this rewriting is performance-preserving, though, it depends on how M.f is compiled; so this is not necessarily faster than the trampoline on entry.)

[lthls]

In native mode, functions with arity greater than one have start_env = 3 (or greater for mutually recurssive functions), so you would still end up needing the trampoline in many cases.
I think performance is abysmal in both cases anyway, so I'd favour the systematic use of trampolines to simplify the code.

Note that if a user observes a performance degradation due to the change (probably no one), they are initializing a publicly-visible function f inside a mutually-recursive module M. It is always possible to not use a mutually-recursive definition for f: if the definition was let rec f = ...g... and g = ...f..., the user can rephrase it as let rec g = ...M.f... ;; let f = ...g.... (I don't know if this rewriting is performance-preserving, though, it depends on how M.f is compiled; so this is not necessarily faster than the trampoline on entry.)

Performance will be much worse with this approach, at least with the native compiler, as direct calls to self (or a mutually recursive function) are much cheaper than indirect calls going through the recursive module being defined.

[stedolan]

I've prototyped an alternative approach to recursive module initialisation in #10205, which is simpler than the current one and I think should perform well (and doesn't overwrite closures).

[Octachron]

This was fixed by #10205 as far as I can see, isn't it?