Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
sim642 opened this issue Jul 24, 2024 · 6 comments · Fixed by #1642
Closed

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

sim642 opened this issue Jul 24, 2024 · 6 comments · Fixed by #1642
Labels

Comments

@sim642
Copy link

sim642 commented Jul 24, 2024

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
Copy link
Member

hhugo commented Jul 24, 2024

Should be fixed by #1642.

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

  • 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 (Improve js_of_ocaml support ocaml/dune#5049)

  • 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.

  • 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 ?

@sim642
Copy link
Author

sim642 commented Jul 24, 2024

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 ocaml/dune#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 goblint/analyzer#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
Copy link
Member

hhugo commented Jul 24, 2024

I was able to use build info and dune site in #1643

@hhugo
Copy link
Member

hhugo commented Jul 24, 2024

I don't know all the details, but it was added by a student in goblint/analyzer#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
Copy link
Member

hhugo commented Jul 24, 2024

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
Copy link
Author

sim642 commented Jul 24, 2024

I tried goblint/analyzer@7b3b392 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): ocaml/dune#6832. Using dune >= 3.7 should allow us to get rid of the workaround then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants