I'm not sure how to reason about correctness on fields that are not the code pointer. I have the impression that we are doing a raw write, when we probably should use a proper write barrier. If this is indeed the case, then the fix would be to loop over the precise closure representation, instead of handling all fields as raw data. (I don't think there is OCaml-side code doing this yet, and it might benefit from some auxiliary functions in Obj.)

I added a type Obj.Closure.info = { arity: int; start_env : int }, which is now used to use set_raw_field on closure metadata fields and set_field on the others.

You're right that closures need as special case. However this code is not good for the reason you mention.

Here I think it's enough to copy field 0 using raw-field accesses and all other fields using normal accesses. In block o there is only one code pointer at offset 0, the rest is integers. As mentioned elsewhere, caml_modify (i.e. Obj.set_field) of a code pointer over an integer is safe.

In your new code you still need to justify why you're taking the startenv of the new block and not the one of the old block.

In block o there is only one code pointer at offset 0, the rest is integers.

My understanding is that overwrite_closure may be called with n an arbitrary closure and o being the template value of the init_mod case:

Obj.repr (fun _ -> raise (Undefined_recursive_module loc))

Here we have a non-integer closure field for loc.

In your new code you still need to justify why you're taking the startenv of the new block and not the one of the old block.

Can I start by filling the segment [o.start_env; n.start_env] with unit values?

Well spotted. I stand corrected. But I think this is also a problem with your approach.

There are TWO kinds of assignments between fields that must be avoided:

• assigning via caml_modify (i.e. Obj.set_field) when the old value is a code pointer
• assigning directly (i.e. Obj.set_raw_field) when the old value is a pointer within the major heap, because then it may escape marking even though it is live elsewhere. (This is why caml_modify does a caml_darken on the old value.)

If you Obj.set_raw over the field of o that contains the loc free variable, you're running into the second forbidden case.

One solution is to first set fields 1...size - 1 of o to an integer value

  Obj.set_field o i (Obj.repr 0)

so that the loc value is properly seen by the GC.

Then you can do your copy with raw assignments up to startenv(n), and normal assignments above.

  let n_start_env = Obj.Closure.((info n).start_env) in
  let o_start_env = Obj.Closure.((info o).start_env) in
  (* if the environment of n starts before the one of o,
     clear the raw fields in between. *)
  for i = n_start_env to o_start_env - 1 do
    Obj.set_raw_field o i Nativeint.one
  done;
  (* if the environment of o starts before the one of n,
     clear the environment fields in between. *)
  for i = o_start_env to n_start_env - 1 do
    Obj.set_field o i (Obj.repr ())
  done;
  for i = 0 to n_start_env - 1 do
    (* code pointers, closure info fields, infix headers *)
    Obj.set_raw_field o i (Obj.raw_field n i)
  done;
  for i = n_start_env to Obj.size n - 1 do
    (* environment fields *)
    Obj.set_field o i (Obj.field n i)
  done;
  ()

what do you think?

Arity is signed (negative arity = tupled function), so please use shift_right and update the comment.

Thanks! This should now be fixed.

Typo: "seconnd".

Cosmetic: switch (tg) { case Closure_tag: ... case String_tag: ... } could look good. Maybe.

I did the switch (haha) in a separate commit, let me know if you like it.

I'm sorry to be difficult, but the Custom_tag case must be rejected BEFORE doing caml_alloc. The reason is as follows: if the block is big enough, it is allocated in the major heap, with tag Custom_tag but an invalid "operations" field. Then, we raise an exception, so the block is unreachable. So, at the next major GC sweep, it will be collected, and the sweeper will look into the "operations" field to see if there is a finalization function. This will segfault.

I see. I noticed that the allocation would happen, but I figured that this would be fine given that this exception corresponds to a programming error, that is not supposed to be handled by the program. Your point about the segfault is very reasonable :-)

For the other failure cases in the function, there is no such issue: there are failure cases that can lead to ill-formed values on the heap, but they are not ill-formed enough that it would be a problem for the GC, which is the only case we have to consider for this allocation that is dead on arrival.

@xavierleroy I moved back the Custom_tag check, but then I pushed a second commit that moves the allocation within the switch, so that it is always after the failure test. Let me know what you prefer.