-
Notifications
You must be signed in to change notification settings - Fork 43
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
Support Microsoft CL.EXE compiler #137
Conversation
Added commits for all the Opam packages in mirage-crypto that can work with MSVC:
There are two Opam packages that won't work with MSVC:
Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for these comprehensive patches! I just left a few questions in the PR for my Windows-education.
Thanks for your PR. This adds quite some I'll also need much more time to think about the The two failing ocaml-ci jobs (mirage-crypto-rng-async on s390 and 4.12+domains) are expected. |
EDIT: GitHub Actions are possible but there is still work to-do; see https://github.com/diskuv/diskuv-ocaml-starter-ghmirror/runs/3844831608?check_suite_focus=true for an example. But GitHub Actions doesn't seem to resolve your main issue ("maintenance burden" below).
Yeah, this is related to your CI comment and my 'just patch/pin it' comment. Since the ifdefs will be a maintenance burden, let's figure out something else. Perhaps an expansion of the Dune configurator to simplify the ifdef clauses? Anyway ... there is no point proceeding with this PR without resolving how the PR will fit into the code base long-term! Action Item:
|
Any progress on the maintainability of ifdefs? |
Again, thanks for your work. Looking back into this, how to move forward?
|
I haven't forgotten about this PR. However, it may be quite a while before I get even to the first item (the MSVC CI, which is broader than this one package). |
According to https://reviews.llvm.org/D40285 the
So |
…age, mirage-crypto-rng-async, mirage-crypto-pk and mirage-crypto-ec (0.10.6) CHANGES: - Use _WIN32 instead of __WIN32__, as proposed by @jonahbeckford in mirage/mirage-crypto#137 - PKG_CONFIG_PATH via cygpath on Window (mirage/mirage-crypto#150 @MisterDA) - ocaml-solo5 (formerly ocaml-freestanding) defines __ocaml_solo5__, use this in ifdef (arm only, mirage/mirage-crypto#152 @hannesm) - mirag-crypto-rng-mirage test: require mirage-unix >= 5.0.0 (mirage/mirage-crypto#151 @hannesm) - use oUnit2 as dependency, instead of oUnit (mirage/mirage-crypto#149 @MisterDA) - support mipsel and mips64el compilation (mirage/mirage-crypto#148 @glondu) - bugfix: define _POSIX_C_SOURCE in entropy_cpu_stubs.c (otherwise clock_gettime is not defined - at least on armhf) (mirage/mirage-crypto#148 @glondu) - bugfix: compilation on kfreebsd-* (adding __FreeBSD_kernel__ to ifdef) (mirage/mirage-crypto#148 @glondu)
More than a year later, any news on this PR? I'm fine to integrate CL.EXE support into mirage-crypto. I'd be very happy to have a CI system. I proposed some changes above, if you could integrate them and rebase, I'm fine to review again and likely merge. |
Oops!! I thought I had gotten all of these MSVC PRs done! I don't how I forgot about this one. I'll stick an entry in my main issue queue, so I don't lose visibility. |
Now that the CI part is merged, would you mind to rebase this PR on top of the main branch? There are some minor comments, which you could address or just rebase and we check whether the CI is happy. :) |
Is there a version of mirage-crypto-pk that does not require I suspect originally I wasn't using mirage-crypto-pk. Or perhaps I used
|
Thanks for your work. In respect to gmp: there's no effort to make The good news is there's (esp. for the mirage build setup) an overlay of the zarith package that uses dune: https://github.com/dune-universe/opam-overlays/tree/master/packages/zarith/zarith.1.12%2Bdune (there's even a gmp-compiled-by-dune, released as "gmp" to opam-repository). There's some discussion ocaml/Zarith#73 ocaml/Zarith#143 about using dune for zarith, but I do not know the state thereof. |
6 months later, I'm curious whether there's any interest to move this to completion? As I see, opam 2.2 is supposed to support windows natively - would this help for this PR (my suspicion is that gmp & zarith from opam should then work?) I also have seen ocaml/Zarith#43 (open since 5 years without any comment). |
I think the way forward is just to check in the MSVC code as-is so it doesn't stagnate. Or provide a mock implementation of a subset of zarith (
Highly doubt that opam 2.2 is concerned with gmp and zarith. They will just mark those libraries as unavailable on Windows. But those libraries are very likely available in Cygwin, which isn't native Windows but is a stop-gap for some Windows users.
Oh, that is so sad! Eventually we (OCaml community) may need to port over (perhaps machine translate) something from Rust rather than rely on GMP. I assume that crypto just needs big integer not big decimal numbers, right? Regardless, a mock implementation defining exactly what is needed from the leading crypto package (mirage-crypto) would be the first step. |
Dear @jonahbeckford, thanks for your reply.
Since "a mock implementation" is quite some work, in addition to runtime failures which I do not appreciate, what about (a) solving the conflicts in this PR and (b) focus on mirage-crypto/mirage-crypto-rng/mirage-crypto-ec (but not mirage-crypto-pk which has the gmp dependency) for windows for now -- specifically, can we adapt the windows CI to not compile mirage-crypto-pk (is it as smooth as removing "mirage-crypto-pk" from "ci/build-test.sh")? |
I took a second look at virtual libraries: https://dune.readthedocs.io/en/stable/variants.html. No mock implementation needed, and we'd be able to avoid bitrot for the MSVC code since that code will compile. The precise steps would be:
I'm not asking for that today, but I am asking whether you or your team can do that sometime in the next year or two. If not (or something less Dune-specific; perhaps with functors, etc.), this whole exercise is pointless. After that |
Done. Although I can't test my windows CI changes because it does not look like the MSVC tests run as part of PRs. |
Dear @jonahbeckford, thanks for your rebase & comments. To move forward, would you mind to answer the inline code comments? They all look minor (see https://github.com/mirage/mirage-crypto/pull/137/files):
I don't have much experience with those. Your suggestion sounds good -- would you mind to open an issue with it? (Once the PR is merged, it is hard to discover the contents.) |
Thanks again for your work, there's only a minor nit to re-establish the Thus, fine to merge if the proposed re-restablishment of |
Dear @jonahbeckford, it would be great if you could re-establish the C flags as suggested above. Once this is done, maybe a rebase/merge of the main branch is useful, and then I'd move forward and merge this long-lasting PR finally into the main tree. |
Co-authored-by: Hannes Mehnert <[email protected]>
Squash and merge is good. Thanks! |
Thanks a lot, merged. Let's see the CI on the main branch :) |
Dear @jonahbeckford, I merged this. I also pushed 7f3887e which will hopefully run the DKML CI for each pull request. I noticed that the DKML CI does not work though:
(same for the x86_64 CI). I've no clue what the underlying issue is, maybe you have an idea? |
somehow it looks like in cfg.ml the std_flags doesn't take the |
and also the |
CHANGES: * mirage-crypto, mirage-crypto-rng{,lwt,mirage}: support CL.EXE compiler (mirage/mirage-crypto#137 @jonahbeckford) - mirage-crypto-pk not yet due to gmp dependency, mirage-crypto-ec doesn't pass testsuite * mirage-crypto-ec: use simpler square root for ed25519 - saving 3 multiplications and 2 squarings, details https://mailarchive.ietf.org/arch/msg/cfrg/qlKpMBqxXZYmDpXXIx6LO3Oznv4/ (mirage/mirage-crypto#196 @hannesm) * mirage-crypto-ec: use sliding window method with pre-computed calues of multiples of the generator point for NIST curves, speedup around 4x for P-256 sign (mirage/mirage-crypto#191 @Firobe, review @palainp @hannesm) * mirage-crypto-ec: documentation: warn about power timing analysis on `k` in Dsa.sign (mirage/mirage-crypto#195 @hannesm, as proposed by @edwintorok) * mirage-crypto-ec: replace internal Cstruct.t by string (speedup up to 2.5x) (mirage/mirage-crypto#146 @dinosaure @hannesm @reynir, review @Firobe @palainp @hannesm @reynir) * bench/speed: add EC (ECDSA & EdDSA generate/sign/verify, ECDH secret/share) operations (mirage/mirage-crypto#192 @hannesm) * mirage-crypto-rng: use rdtime instead of rdcycle on RISC-V (rdcycle is privileged since Linux kernel 6.6) (mirage/mirage-crypto#194 @AdrianBunk, review by @edwintorok) * mirage-crypto-rng: support Loongarch (mirage/mirage-crypto#190 @fangyaling, review @loongson-zn) * mirage-crypto-rng: support NetBSD (mirage/mirage-crypto#189 @drchrispinnock) * mirage-crypto-rng: allocate less in Fortuna when feeding (mirage/mirage-crypto#188 @hannesm, reported by @palainp) * mirage-crypto-ec: avoid mirage-crypto-pk and asn1-combinators test dependency (instead, craft our own asn.1 decoder -- mirage/mirage-crypto#200 @hannesm) ### Performance differences between v0.11.2 and v0.11.3 and OpenSSL The overall result is promising: P-256 sign operation improved 9.4 times, but is still a 4.9 times slower than OpenSSL. Numbers in operations per second (apart from speedup, which is a factor v0.11.3 / v0.11.2), gathered on a Intel i7-5600U CPU 2.60GHz using FreeBSD 14.0, OCaml 4.14.1, and OpenSSL 3.0.12. #### P224 | op | v0.11.2 | v0.11.3 | speedup | OpenSSL | |--------|---------|---------|---------|---------| | gen | 1160 | 20609 | 17.8 | | | sign | 931 | 8169 | 8.8 | 21319 | | verify | 328 | 1606 | 4.9 | 10719 | | dh-sec | 1011 | 12595 | 12.5 | | | dh-kex | 992 | 2021 | 2.0 | 16691 | #### P256 | op | v0.11.2 | v0.11.3 | speedup | OpenSSL | |--------|---------|---------|---------|---------| | gen | 990 | 19365 | 19.6 | | | sign | 792 | 7436 | 9.4 | 36182 | | verify | 303 | 1488 | 4.9 | 13383 | | dh-sec | 875 | 11508 | 13.2 | | | dh-kex | 895 | 1861 | 2.1 | 17742 | #### P384 | op | v0.11.2 | v0.11.3 | speedup | OpenSSL | |--------|---------|---------|---------|---------| | gen | 474 | 6703 | 14.1 | | | sign | 349 | 3061 | 8.8 | 900 | | verify | 147 | 544 | 3.7 | 1062 | | dh-sec | 378 | 4405 | 11.7 | | | dh-kex | 433 | 673 | 1.6 | 973 | #### P521 | op | v0.11.2 | v0.11.3 | speedup | OpenSSL | |--------|---------|---------|---------|---------| | gen | 185 | 1996 | 10.8 | | | sign | 137 | 438 | 3.2 | 2737 | | verify | 66 | 211 | 3.2 | 1354 | | dh-sec | 180 | 1535 | 8.5 | | | dh-kex | 201 | 268 | 1.3 | 2207 | #### 25519 | op | v0.11.2 | v0.11.3 | speedup | OpenSSL | |--------|---------|---------|---------|---------| | gen | 23271 | 22345 | 1.0 | | | sign | 11228 | 10985 | 1.0 | 21794 | | verify | 8149 | 8029 | 1.0 | 7729 | | dh-sec | 14075 | 13968 | 1.0 | | | dh-kex | 13487 | 14079 | 1.0 | 24824 |
__attribute__((unused))
: Convert to MSVC non-standard extensions rather than GCC non-standard extensions_WIN64
in addition to__x86_64__
to detect 64 bit