Update extracted C files from hacl-star-raw.0.2.1

mirage / hacl

Archived. Curve25519 support has been integrated into mirage-crypto-ec (via fiat-crypto). Hacl bindings are available from the hacl-star opam package. OCaml bindings for HACL* elliptic curves

https://github.com/mirage/mirage-crypto

Other

20 stars 5 forks source link

Update extracted C files from hacl-star-raw.0.2.1 #36

Closed dinosaure closed 4 years ago

dinosaure commented 4 years ago

I'm not very happy with this PR which integrates so much *.h but they are needed to be able to compile *.c. The C code comes from the local extraction of hacl-star used by hacl-star-raw OPAM package (reproducible with a simple a call to ./build-local.sh).

It should fix, at least, #34 but I did not test yet.

dinosaure commented 4 years ago

The bad point is a use of ln -s to be able to use the same directory which contains *.h. This is the less expensive way where dune is not able to copy a directory.

hannesm commented 4 years ago

thanks for the update. What I do not really like about this:

using symlinks :/
now Hacl_Ed25519 depends on Hacl_hash, which includes MD5, SHA1, SHA2 -- while only SHA512 is used

compilation emits a warning:

kremlin/include/kremlin/internal/target.h:53:19: warning: implicit declaration of function 'time' is invalid in C99 [-Wimplicit-function-declaration]
return (int32_t)time(NULL);
              ^

I don't see a good solution for (1) (since there's no recursive copy and -I .. does not work IIRC - please correct me if I'm wrong); (2) we could manually remove the not used code (from Hacl_hash.c); (3) pass -DKRML_HOST_TIME (at least for the cross-build(s) -- the function is not used at all).

The resulting libhacl_stubs_x25519.a is actually smaller than the one from master, this is good.

dinosaure commented 4 years ago

Thanks for review, so about: 1) I'm not a big fan too but we need to wait https://github.com/ocaml/dune/issues/3387 2) [ ] Ok to delete unused codes in fact :+1: 3) [ ] Ok to pass this option too

So, I will update the PR tonight I think

hannesm commented 4 years ago

@dinosaure I pushed two commits about (2) and (3) here.

dinosaure commented 4 years ago

Thanks, CI is happy so ready to merge I think.

hannesm commented 4 years ago

@dinosaure one remaining item before merge is to update README where the extracted stuff was taken from. IIUC, it uses the hacl-star-raw opam package in version 0.2.1? (and then maybe refer to this PR, and mention that Hacl_Hash.{c,h} were manually modified)

dinosaure commented 4 years ago

Ah you did it already :D, ok let's merge so.