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

Fix "weak hash of serialised closures" from mantis #0005942. #330

Merged
merged 2 commits into from
Dec 4, 2015
Merged

Fix "weak hash of serialised closures" from mantis #0005942. #330

merged 2 commits into from
Dec 4, 2015

Conversation

bvaugon
Copy link
Contributor

@bvaugon bvaugon commented Dec 3, 2015

@bvaugon
Copy link
Contributor Author

bvaugon commented Dec 4, 2015

Hum, the failure report from travis seems to be related to a completely different problem:

/home/travis/local/bin/ocamlc.opt -c -g -w a -I camlp4/import -I camlp4/config -I camlp4/boot -o camlp4/boot/Camlp4.cmo camlp4/boot/Camlp4.ml
File "camlp4/boot/Camlp4.ml", line 15328, characters 47-57:
Error: This variant expression is expected to have type Parsetree.constant
The constructor Const_char does not belong to type Parsetree.constant
Hint: Did you mean PConst_char?
Command exited with code 2.
make: *** [byte] Error 10

@ghost
Copy link

ghost commented Dec 4, 2015

The failure was due to #170 that broke Camlp4. I just merged camlp4/camlp4#91 that should fix the mismatch between OCaml and Camlp4.

@alainfrisch
Copy link
Contributor

Thanks! Can you add an entry to Changes file?

@bvaugon
Copy link
Contributor Author

bvaugon commented Dec 4, 2015

It goes a step further, now, the installation of cudf for opam fails for another reason:
cd tmp && tar xfz ../cudf.0.7+opam.tar.gz
tar (child): ../cudf.0.7+opam.tar.gz: Cannot open: No such file or directory
tar (child): Error is not recoverable: exiting now
tar: Child returned status 2
tar: Error is not recoverable: exiting now
make[1]: *** [cudf.stamp] Error 2
make[1]: Leaving directory /home/travis/build/ocaml/ocaml/external-packages/opam/src_ext
make: *** [lib-ext] Error 2

@damiendoligez
Copy link
Member

MD5 for cudf.0.7+opam.tar.gz differ:
  expected: b12d933a96bd326e2443241faca7aa8c
    actual: 2c23f4a6c377270c34962574a1bed669

Somebody changed a file on some server and Travis fails. IMO in its current form the Travis test is not stable enough to be useful. If anyone wants to work on that I'll be happy, in the meantime I'll just ignore it.

@alainfrisch alainfrisch merged commit e9bb7e1 into ocaml:trunk Dec 4, 2015
@alainfrisch
Copy link
Contributor

I've merge to trunk, after fixings a few things in the testsuite.

A few things:

  • I'm not sure about what the tests actually test. In particular, we don't check that the output is as expected. And it is not very nice that the tests are not idempotent (write depends on the previously created data file).
  • This was probably the case before, but the logic of computing the digest of the unit seems to be broken under Windows at least. In particular, while running write32.byte twice in a row results in it printing "3.14", this is not the case for write32.native, which keeps saying "Incompatible closure, ok". My guess is that this is because the digest is computed on the code and data after relocation, and since the code is not PIC, two invocations of the same program result in different hashes. Any thoughts on this? An alternative strategy could be to compute a digest for object files (.o/.obj), store it in the .cmx, and use them to produce a digest of the main program (or the .cmx) when linking it. We could also add a flag to Marshal to explicitly ignore the digest check (the users are then on their own!).

@alainfrisch
Copy link
Contributor

According to INRIA's CI, tests/lib-marshal-closure/arch-32/byte is failing (on Linux 32, and MacOS 10.9 for now). I cannot reproduce the failure locally (under Windows / MSVC). Any idea?

@bvaugon
Copy link
Contributor Author

bvaugon commented Dec 4, 2015

I reproduce the problem under a native linux 32 (cross compilation 64->32 using gcc -m32 seems to work). The problem probably comes from testsuite/tests/lib-marshal-closure/Makefile, since the error log file "write32byte.err" contains:
/bin/sh: ./write32.byte : /tmp/ocaml/localinst/bin/ocamlrun : mauvais interpréteur: Aucun fichier ou dossier de ce type
The bytecode files generated by the command:
../../../boot/ocamlrun ../../../ocamlc -nostdlib -I ../../../stdlib write32.ml -o write32.byte
../../../boot/ocamlrun ../../../ocamlc -nostdlib -I ../../../stdlib read32.ml -o read32.byte
../../../boot/ocamlrun ../../../ocamlc -nostdlib -I ../../../stdlib write64.ml -o write64.byte
../../../boot/ocamlrun ../../../ocamlc -nostdlib -I ../../../stdlib read64.ml -o read64.byte
seems to use the installed ocamlrun interpreter instead of the local one.

@alainfrisch
Copy link
Contributor

I've committed a change to the Makefile to use ocamlc -custom. We test another branch of the code, but this is fine!

@alainfrisch
Copy link
Contributor

This was probably the case before, but the logic of computing the digest of the unit seems to be broken under Windows at least.

Actually, this seemed to work before...

@xavierleroy
Copy link
Contributor

Alternatively, just run the generated bytecode executables with ../../../byterun/ocamlrun foo.byte. This is what all the other tests in the testsuite do, I think. Why make a special case here?

@alainfrisch
Copy link
Contributor

The same piece of code is used to call the .byte and .native versions, so one cannot call ocamlrun explicitly. One should either duplicate or complexify the code, or set the PATH as I tried to do (but failed miserably).

@alainfrisch
Copy link
Contributor

Actually, this seemed to work before...

Could it be that the code is PIC enough, but not the data (absolute closure pointers). This actually makes me wonder how this can possibly work under Linux, since we have no guarantee that the code pointers in data will also be the same. Any idea, Benoit ?

@xavierleroy
Copy link
Contributor

@alainfrisch : yes, the Makefile should be written differently, separating the bytecode and native code cases and honoring the $(NATIVECODE_ONLY) and $(BYTECODE_ONLY) variables. See testsuite/makefiles/Makefile.one for inspiration.

alainfrisch added a commit that referenced this pull request Dec 8, 2015
1. Revert "Switch to -custom for bytecode tests."
This reverts commit 6b7f81c.

2. Revert "Fix Changelog."
This reverts commit d94488d.

3.Revert "Fix testsuite: use binary channels."
This reverts commit 840f7ca.

4. Revert "Fix testsuite: do not require a globally installed ocamlrun."
This reverts commit 0388ef4.

5. Revert "Merge branch 'trunk' of https://github.com/bvaugon/ocaml into bvaugon-trunk"
This reverts commit 1ff6db1, reversing
changes made to 89d116c.
@alainfrisch
Copy link
Contributor

Changes reverted in e888e09. Let's try to find a less risky solution!

@damiendoligez
Copy link
Member

IIUC, this PR should be reopened. Does anyone know how to reopen a PR that was merged then reverted?

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

Successfully merging this pull request may close these issues.

4 participants