search

ocsigen / js_of_ocaml

Compiler from OCaml to Javascript.
http://ocsigen.org/js_of_ocaml/
Other
953 stars 186 forks source link

[BUG] Runtile failure with `-linkall` since 5.1.0 #1641

Closed sim642 closed 2 months ago

sim642 commented 2 months ago

Describe the bug Runtime failure during initialization when -linkall is used with the following exception:

exception Failure("caml_register_global: cannot locate Dynlink_compilerlibs.Binutils")

The large-ish project where this appears does not use dynlink in any way AFAIK. Only reference to dynlink is in _build/default/.js/default/dynlink/dynlink.cma.js, not any other libraries included into JS.

Expected behavior No failure.

Versions The issue appears at js_of_ocaml 5.1.0. On 5.0.1 it as still fine: -linkall did not want to include that dynlink business.

I suppose it's somehow related to #1378 but I don't really understand what it changed.

hhugo commented 2 months ago

Should be fixed by #1642.

I took a quick look at goblint and have few remark/question

sim642 commented 2 months ago

Thanks for the quick fix! (I didn't try it yet though.)

  • The codebase mention that dune-sites and build-info are incompatible with js_of_ocaml, has this been been reported upstream ? Is it still accurate ? I fixed build-info in dune.3.0.0

I don't recall the details, but I suspect it might have been an instance of https://github.com/ocaml/dune/issues/4444. Nobody has checked whether our workaround is still needed since then, but that's for your pointer. I'll see if our workarounds can now be just removed or whether there's still some unreported issue.

  • You should probably investigate the reason why you have dynlink in the dependency cone of your js target. dropping it would significantly reduce the size of the generated file.

Indeed, I was surprised to see it myself because we don't use it ourselves (instead we rely on -linkall to register things from various places).

  • It seems that you tried to use javascript numbers for ints up to 2^53, (I see you've patched the marshal runtime and zarith). I'm very surprised to see such change as I would expect it to not work. Can you explain why you need such change and how it works in practice ?

I don't know all the details, but it was added by a student in https://github.com/goblint/analyzer/pull/315. There's some longer write-up about it as well: https://github.com/goblint/Zarith/raw/goblint/goblint/main.pdf.

In our use case, we run Goblint natively and marshal out some data. Then Gobview, which is the js_of_ocaml front-end for it, can unmarshal this data in-browser and manipulate it with all existing Goblint code. I think the patching was necessary to make native marshaling output 32bit-sized values such that js_of_ocaml can unmarshal them correctly. It's a very exotic use case and it hasn't been super reliable, so it possibly doesn't work in general indeed.

hhugo commented 2 months ago

I was able to use build info and dune site in https://github.com/ocsigen/js_of_ocaml/pull/1643

hhugo commented 2 months ago

I don't know all the details, but it was added by a student in https://github.com/goblint/analyzer/pull/315. There's some longer write-up about it as well: https://github.com/goblint/Zarith/raw/goblint/goblint/main.pdf.

In our use case, we run Goblint natively and marshal out some data. Then Gobview, which is the js_of_ocaml front-end for it, can unmarshal this data in-browser and manipulate it with all existing Goblint code. I think the patching was necessary to make native marshaling output 32bit-sized values such that js_of_ocaml can unmarshal them correctly. It's a very exotic use case and it hasn't been super reliable, so it possibly doesn't work in general indeed.

Thanks for the explanation, I agree that there can be an issue trying to unmarshal a Z.t with jsoo but I suspect the approach doesn't do what you want. The reason is that the modified version of marshal (in marshal.js) will accept to read a 64bit marshalled integers but will cast it to 32bit. You'll end up with a truncated Z.t.

hhugo commented 2 months ago

Indeed, I was surprised to see it myself because we don't use it ourselves (instead we rely on -linkall to register things from various places).

goblint-cil depends on dynlink https://github.com/goblint/cil/blob/develop/src/dune#L6

sim642 commented 2 months ago

I tried https://github.com/goblint/analyzer/commit/7b3b3925a172c5c9886c720f64d6dd12ea6d55ce out in a fresh opam switch while making Gobview use the proper build-info and sites. I can reproduce the errors there. Upgrading dune from 3.6 to 3.7 seems to fix those actually, while still staying at (lang dune 3.6).

This might've been the dune fix (also related to -linkall): https://github.com/ocaml/dune/pull/6832. Using dune >= 3.7 should allow us to get rid of the workaround then.