Test failure in test-callback_lifetime with Flambda 2

[lthls]

The following part of the test-callback_lifetime test fails with the Flambda 2 compiler:

    (* However, even with the careful implementation things can go wrong if we
       keep a reference to ret beyond the lifetime of the pair. *)
    let ret = Better.get (Better.make ~arg:(closure (int_of_string "3"))) in
    Gc.full_major ();
    assert_raises CallToExpiredClosure
      (fun () -> ret 5);

The reason is actually fairly straightforward. In the Flambda 2 compiler, this code gets rewritten to (in pseudo-syntax):

(* Code definition, at toplevel *)
let_code anon_fun_code param env =
  let ret = get_env_var "ret" env in
  ret 5

(* Actual term *)
let apply_result = (* Call to Better.make *) in
let ret = apply_result.ret in (* Better.get has been inlined *)
Gc.full_major ();
let anon_fun = allocate_closure ~code:anon_fun ~env:[ret] in
assert_raises CallToExpiredClosure anon_fun;

So far, so good. apply_result is still not live across the call to Gc.full_major, so everything should behave as expected. However, when generating Cmm, the compiler tries to inline let-bindings that are only used once and where the right-hand side is pure into their actual uses (because many Cmm optimisations assume a compact representation). So the code gets rewritten to:

let apply_result = (* Call to Better.make *) in
Gc.full_major ();
let anon_fun = allocate_closure ~code:anon_fun ~env:[apply_result.ret] in
assert_raises CallToExpiredClosure anon_fun;

Now the apply_result variable is live across the call to Gc.full_major, and so is not collected by the GC.

From our point of view (as developers of the Flambda 2 compiler), this optimisation is correct. We don't want to have to special-case the Gc.full_major function to disable let-inlining over it. So I'd suggest to change the definition of ret to:

    let ret = (Better.get[@inlined never]) (Better.make ~arg:(closure (int_of_string "3"))) in

It's a fairly simple change (just an additional annotation), and since the compiler is never allowed to move function calls around other function calls, this ensures that ret will be fully evaluated before the call to Gc.full_major, so that its englobing record can be collected. I've checked that with this patch, the tests pass with the Flambda 2 compiler.

[yallop]

Thank you for the report, and the clear explanation.

Could we achieve the same thing using Sys.opaque_identity (or use_value) in place of [@inlined never]?

[lthls]

I think so. Adding either use_value or Sys.opaque_identity before the call to Gc.full_major forces the field load at that point, so unless the compiler decides to duplicate the field load (which would be technically correct, but very unlikely as it's a strict increase in the number of instructions and live variables) you know that the block can't be live during GC.

[yallop]

683 is a fix for this issue. @lthis, if you could confirm that it does actually fix the problem with Flambda 2, it'd be appreciated!