Dune 2.0 and fix clash - Githubissues

dinosaure commented 4 years ago

This PR should fix clash of names (linking error) and add a prefix mirage__ into extracted C file of hacl - only for the UNIX/usual back-end but we should not apply that on freestanding and xen artifacts.

hannesm commented 4 years ago

hmm, I don't quite understand this PR (and CI is failing). Would you mind to briefly describe (maybe in symbols.ml) what is happening in there? I'm fine with the general idea to support cross-installation and linkage of hacl-raw and hacl (until they're merged into one).

hannesm commented 4 years ago

I'm curious: which symbols clash? why not use a sed script when importing a new hacl release into the source tree (and thus have less magic / objcopy being used at build time)?

hannesm commented 4 years ago

I looked a bit deeper into this issue, and cannot reproduce the issue to fix here. AFAICT more recent "hacl-star" use camlHacl_XXX symbols. If this issue is reproducible, I'd appreciate a test, I tried with:

(test
 (name colink)
 (package hacl_x25519)
 (modules colink)
 (libraries hacl_x25519 hacl-star))

colink.ml:

let () =
  let _, _ = Hacl_x25519.gen_key ~rng:Cstruct.create in
  Hacl_star.(EverCrypt.Curve25519.secret_to_public (Bytes.create 32) (Bytes.create 32))

and both hacl-star 0.2.0 and 0.3.0.

craigfe commented 4 years ago

Here's an example of a link failure due to a collision between hacl_x25519.0.2.1 and hacl-star-raw.0.3.0:

/usr/bin/ld: /home/craigfe/.opam/tezos/lib/hacl-star-raw/libevercrypt.a(Hacl_Hash.o): in function `Hacl_Hash_Core_SHA2_update_512':
/home/craigfe/.opam/tezos/.opam-switch/build/hacl-star-raw.0.3.0/raw/Hacl_Hash.c:3267: multiple definition of `Hacl_Hash_Core_SHA2_update_512'; /home/craigfe/.opam/tezos/lib/hacl_x25519/libhacl_x25519_stubs.a(Hacl_Hash.o):/home/craigfe/.opam/tezos/.opam-switch/build/hacl_x25519.0.2.1/_build/default/src/Hacl_Hash.c:130: first defined here
/usr/bin/ld: /home/craigfe/.opam/tezos/lib/hacl-star-raw/libevercrypt.a(Hacl_Hash.o): in function `Hacl_Hash_SHA2_update_multi_512':
/home/craigfe/.opam/tezos/.opam-switch/build/hacl-star-raw.0.3.0/raw/Hacl_Hash.c:3271: multiple definition of `Hacl_Hash_SHA2_update_multi_512'; /home/craigfe/.opam/tezos/lib/hacl_x25519/libhacl_x25519_stubs.a(Hacl_Hash.o):/home/craigfe/.opam/tezos/.opam-switch/build/hacl_x25519.0.2.1/_build/default/src/Hacl_Hash.c:29: first defined here
/usr/bin/ld: /home/craigfe/.opam/tezos/lib/hacl-star-raw/libevercrypt.a(Hacl_Hash.o): in function `Hacl_Hash_Core_SHA2_pad_512':
/home/craigfe/.opam/tezos/.opam-switch/build/hacl-star-raw.0.3.0/raw/Hacl_Hash.c:3379: multiple definition of `Hacl_Hash_Core_SHA2_pad_512'; /home/craigfe/.opam/tezos/lib/hacl_x25519/libhacl_x25519_stubs.a(Hacl_Hash.o):/home/craigfe/.opam/tezos/.opam-switch/build/hacl_x25519.0.2.1/_build/default/src/Hacl_Hash.c:207: first defined here
/usr/bin/ld: /home/craigfe/.opam/tezos/lib/hacl-star-raw/libevercrypt.a(Hacl_Hash.o): in function `Hacl_Hash_SHA2_update_last_512':
/home/craigfe/.opam/tezos/.opam-switch/build/hacl-star-raw.0.3.0/raw/Hacl_Hash.c:3388: multiple definition of `Hacl_Hash_SHA2_update_last_512'; /home/craigfe/.opam/tezos/lib/hacl_x25519/libhacl_x25519_stubs.a(Hacl_Hash.o):/home/craigfe/.opam/tezos/.opam-switch/build/hacl_x25519.0.2.1/_build/default/src/Hacl_Hash.c:44: first defined here
/usr/bin/ld: /home/craigfe/.opam/tezos/lib/hacl-star-raw/libevercrypt.a(Hacl_Hash.o): in function `__bswap_64':
/usr/include/bits/byteswap.h:73: multiple definition of `Hacl_Hash_Core_SHA2_finish_512'; /home/craigfe/.opam/tezos/lib/hacl_x25519/libhacl_x25519_stubs.a(Hacl_Hash.o):/home/craigfe/.opam/tezos/.opam-switch/build/hacl_x25519.0.2.1/_build/default/src/Hacl_Hash.c:244: first defined here
/usr/bin/ld: /home/craigfe/.opam/tezos/lib/hacl-star-raw/libevercrypt.a(Hacl_Hash.o): in function `Hacl_Hash_SHA2_hash_512':
/home/craigfe/.opam/tezos/.opam-switch/build/hacl-star-raw.0.3.0/raw/Hacl_Hash.c:3525: multiple definition of `Hacl_Hash_SHA2_hash_512'; /home/craigfe/.opam/tezos/lib/hacl_x25519/libhacl_x25519_stubs.a(Hacl_Hash.o):/home/craigfe/.opam/tezos/.opam-switch/build/hacl_x25519.0.2.1/_build/default/src/Hacl_Hash.c:76: first defined here
collect2: error: ld returned 1 exit status
File "caml_startup", line 1:
Error: Error during linking

I've tried to construct a minimal reproduction of this failure – which occurs in the wild for the Tezos monorepo – but haven't been successful. This patch fixes the issue, however.

avsm commented 3 years ago

I can't repro this either with the small test case, even though there is clearly a symbol clash.

$ nm ~/.opam/4.09.1/lib/hacl_x25519/libhacl_x25519_stubs.a |grep Hacl_Hash_Core_SHA2_
00000000000004c0 T Hacl_Hash_Core_SHA2_finish_512
00000000000002c0 T Hacl_Hash_Core_SHA2_pad_512
0000000000000000 T Hacl_Hash_Core_SHA2_update_512
$ nm ~/.opam/4.09.1/lib/hacl-star-raw/libevercrypt.a |grep Hacl_Hash_Core_SHA2_
0000000000003a70 T Hacl_Hash_Core_SHA2_finish_224
0000000000003b70 T Hacl_Hash_Core_SHA2_finish_256
0000000000003c80 T Hacl_Hash_Core_SHA2_finish_384
0000000000003da0 T Hacl_Hash_Core_SHA2_finish_512
0000000000002c40 T Hacl_Hash_Core_SHA2_init_224
0000000000002c60 T Hacl_Hash_Core_SHA2_init_256
0000000000002c80 T Hacl_Hash_Core_SHA2_init_384
0000000000002cc0 T Hacl_Hash_Core_SHA2_init_512
00000000000032f0 T Hacl_Hash_Core_SHA2_pad_224
00000000000034a0 T Hacl_Hash_Core_SHA2_pad_256
0000000000003650 T Hacl_Hash_Core_SHA2_pad_384
0000000000003860 T Hacl_Hash_Core_SHA2_pad_512

However, hacl-star appears to use ctypes-foreign (and hence libffi) to load in its bindings, which means we may need to work a little harder to trigger the clash.

dinosaure commented 3 years ago

Just to clear the status of this PR:

In my mind, it's not really possible to deal with sed and dune at the same time. A use of sed implies an introspection of symbols generated by the compilation of C stubs (which should not be updated because they come from the extraction of a certain version of hacl-star). From this introspection, we can get the symbols.txt currently generated (with nm). But we must put a second stage of the compilation which applies sed and recompile sources to generate another binding. So it will be: 1) compilation of libevercrypt.a 2) nm to get symbols 3) sed on sources 4) recompilation of libevercrypt.a from updated sources
The current solution is not good for many reasons: 1) we do some black magics with objcopy - I'm not sure (and the CI shows to us the problem) that such way is portable (currently x86_32 fails) 2) I did this patch as an intermediate possible patch, knowing that MirageOS 4 will probably fix our problem when the compilation of libevercrypt.a by hacl-star-raw will be compatible (with some works) with MirageOS/ocaml-freestanding
It seems that the reproduction is hard - even me I don't remember when I got such error. I would like to say that if Tezos is not able to reproduce in their context the clash, we should definitely close this (bad) PR.

dinosaure commented 3 years ago

Ok, the error is, indeed, not reproducible with the snippet of @hannesm but it is reproducible when we use:

(executable
 (name colink)
 (libraries hacl-star hacl_x25519)))

Into details, the real diff is between order of libraries at the link time (ocamlopt -o main.exe ...) where the @hannesm's case prepends hacl_x25515.cmxa before ocamlevercrypt.cmxa and the second case appends hacl_x25519.cmxa after ocamlevercrypt.cmxa.

~1) it's weird to have a different behavior from 2 artifacts which are ideally the same: a simple executable~ 2) I know that the order for the linker point-of-view is really important - and it seems that it's smart enough for one... 3) the bug is reproducible 4) the order of (libraries ...) is important finally

samoht commented 3 years ago

I confirm that I can reproduce @dinosaure's issue in linux but not on macos. No idea what the linker is doing here :-)

avsm commented 3 years ago

What's confusing me is why the symbols are appearing at link time at all, since hacl-star-raw appears to use ctypes-foreign to dynamically load them.

dinosaure commented 3 years ago

What's confusing me is why the symbols are appearing at link time at all, since hacl-star-raw appears to use ctypes-foreign to dynamically load them.

I don't really understand underlayer with ctypes & co.

In other side, I clean-up this PR - again, I'm not really convince about what I did but it seems a middle-term solution for us. When MirageOS 4 will be available (and the question about C flags solved), I will propose something else which is more sustainable. So if you want a release, we can cut one for Tezos.

mirage / hacl

Dune 2.0 and fix clash #33