ahrefs / atd

Static types for JSON APIs
Other
315 stars 52 forks source link

Assertion failure in Atdgen_runtime.Oj_run with flambda2 #221

Open lthls opened 3 years ago

lthls commented 3 years ago

Regarding the following test in atdgen-runtime/src/oj_run.ml:

(* We want an identity function that is not inlined *)
type identity_t = { mutable _identity : 'a. 'a -> 'a }
let identity_ref = { _identity = (fun x -> x) }
let identity x = identity_ref._identity x

(*
  Checking at runtime that our assumptions on unspecified compiler behavior
  still hold.
*)

type t = {
  _a : int option;
  _b : int;
}

let create () =
  { { _a = None; _b = Array.length Sys.argv } with _a = None }

let test () =
  let r = create () in
  let v = Some 17 in
  Obj.set_field (Obj.repr r) 0 (Obj.repr v);
  let safe_r = identity r in
  (* r._a is inlined by ocamlopt and equals None
     because the field is supposed to be immutable. *)
  assert (safe_r._a = v)

let () = test ()

With flambda2, the assertion fails (correctly, I might add). I know why, and I can easily adapt the test to make the assertion pass, but I assume that a failure here might reveal a problem elsewhere so I'd like to make sure that all the relevant code is patched.

The core problem is that flambda2 sees the identity function as a real identity. The reasons are a bit complex, as it involves various optimisations in various parts of the compiler, but I can't tell from this code if this test is meant to check that the given identity function is indeed opaque to the compiler (in which case there is likely some code to patch elsewhere), or if this code only needs an identity function that is opaque (as the first comment seems to suggest) to test something else. In the later case, I can suggest alternatives that are guaranteed to work (more or less hackish, depending on the minimal OCaml version that needs to be supported).

mjambon commented 3 years ago

The json mode of atdgen is supposed to no longer use the Obj module at all. This runtime check should just be removed.

Note that Obj is still used by biniou, and we might have a problem there.

zindel commented 2 years ago

@lthls should this one be closed?

lthls commented 2 years ago

As far as I can tell the check hasn't been removed, so this will still cause runtime errors with clever compilers.The PR I made that referred to this issue is only a temporary hack to work around this issue while testing the Flambda 2 compiler.