coq / platform

Multi platform setup for Coq, Coq libraries and tools
Creative Commons Zero v1.0 Universal
188 stars 49 forks source link

fiat cryptography #178

Closed spitters closed 1 year ago

spitters commented 2 years ago

As requested at https://github.com/coq/platform/issues/28 opening an issue tracking: https://github.com/mit-plv/fiat-crypto/issues/925

MSoegtropIMC commented 2 years ago

@JasonGross : I prefer example files which exist in the projects, but I can take these lines. Also I would like to have one file for each project. But for packages in the extended level it is not a strict requirement (there are currently no exceptions, though).

MSoegtropIMC commented 2 years ago

Well actually there are a few exceptions already ... but still I would like to have smoke test files. The smoke test proved useful quite frequently already.

MSoegtropIMC commented 2 years ago

@JasonGross : an editorial note: the bedrock2 opam package says "A verified compiler targeting RISC-V from this language exists but is not yet on opam." As far as I understand this does exist in opam now.

andres-erbsen commented 2 years ago

Here are some fast-ish non-leaf files form the other developments: coqutil: src/coqutil/Map/SortedListWord.v riscv: src/riscv/Examples/MulTrapHandler.v bedrock2: bedrock2/bedrock2Examples/ipow.v bedrock2 compiler: compiler/src/compiler/Pipeline.v rupicola: src/Rupicola/Examples/Uppercase.v

JasonGross commented 2 years ago

@andres-erbsen as per

The smoke test files should mostly test that installed .vo files are accessible under the intended log path, that plugins work and they should run as fast as possible (at most total 10s per package).

I think we're supposed to suggest short scripts that are not things that are installed. Are you suggesting just Require Import on each of those files as the smoke test? Or copy-pasting the entire file and making sure it works as intended when you compile it against the installed version? (But will it work when binding the package with -Q instead of -R?)

@MSoegtropIMC https://github.com/coq/opam-coq-archive/pull/2341 should hopefully work. Once merged, the 0.0.15 version of fiat-crypto should automatically require the correct version of bedrock2.

andres-erbsen commented 2 years ago

Hmm, I thought we wanted existing files. \_o_/ For Require Import, it would be neat to test GarageDoor.v if that Requires in less than 10s.

@JasonGross : I prefer example files which exist in the projects, but I can take these lines.

JasonGross commented 2 years ago

Ah, I missed that, thanks!

JasonGross commented 2 years ago

Good smoke test files for rewriter: https://github.com/mit-plv/rewriter/blob/master/src/Rewriter/Demo.v (12s) or https://github.com/mit-plv/rewriter/blob/master/src/Rewriter/Rewriter/Examples.v (54s) Decent smoke test files for fiat-crypto: https://github.com/mit-plv/fiat-crypto/blob/master/src/Demo.v (46s), https://github.com/mit-plv/fiat-crypto/blob/master/src/CLI.v (4s, but doesn't really test any reduction behavior)

The Requires in these files might have to be adjusted if you want them to work externally.

MSoegtropIMC commented 2 years ago

Thanks! I think the 46s are fine for a large project like fiat-crypto. For rewriter I will take the Demo.v. file. For the others I will test the files suggested by Andres.

I can patch the requires on copy (quite a few projects need this).

MSoegtropIMC commented 2 years ago

The suggested tests are all fine (and don't require patching of Require statements).

I will do tests on Linux and Windows and then merge.

Thanks a lot for the support!

MSoegtropIMC commented 2 years ago

There is one more issue: bedrock2 does not compile in 32 bit Windows - as far as I understand some 128 bit C stuff:

2022-10-04T23:10:23.8937576Z    194 i686-w64-mingw32-gcc.exe -fsyntax-only special/TypecheckExprToCString.c
2022-10-04T23:10:23.8938186Z    195 special/TypecheckExprToCString.c: In function ‘main’:
2022-10-04T23:10:23.8938783Z    196 special/TypecheckExprToCString.c:32:105: error: ‘__uint128_t’ undeclared (first use in this function); did you mean ‘__int128__’?
2022-10-04T23:10:23.8939429Z    197    32 |   _Generic((uintptr_t)(sizeof(intptr_t) == 4 ? ((uint64_t)((uintptr_t)42ULL)*((uintptr_t)7ULL))>>32 : ((__uint128_t)((uintptr_t)42ULL)*((uintptr_t)7ULL))>>64), uintptr_t: 0);
2022-10-04T23:10:23.8939891Z    198       |                                                                                                         ^~~~~~~~~~~
2022-10-04T23:10:23.8940233Z    199       |                                                                                                         __int128__
2022-10-04T23:10:23.8940668Z    200 special/TypecheckExprToCString.c:32:105: note: each undeclared identifier is reported only once for each function it appears in
2022-10-04T23:10:23.8941211Z    201 make[1]: *** [Makefile:83: typecheckExprToCString] Error 1

For the time being I will solve this by excluding fiat-crypto in 32 bit Windows, but it might be worthwhile to fix it. For Coq 32 bit Windows is not completely obsolete, because it needs only about half the amount of memory which also makes it faster (better cache utilization). Since memory is an issue with Coq especially on laptops, even some researchers prefer the 32 bit Windows version.

Otherwise everything is fine except that Coq Platform CI now again touches the 6h limit on Windows. Coq Platform meanwhile has an option to exclude or individually select "large packages" and I will put fiat-crypto and its dependencies in this group. Do you have a good name for fiat-crypto and its dependencies? IMHO it is not adequate to treat bedrock2 as fiat-crypto dependency in package selection. Well, these questions are one pagers anyway, so I will include the list of packages (but make them selectable only as one set), but a better name would be nice. How about MIT-SW since as far as I understand this is the majority of the SW oriented tools of the MIT Coq group(s).

andres-erbsen commented 2 years ago

I created https://github.com/mit-plv/bedrock2/commit/1c9f0ed7acd640747691ed8ee42b11f96395aa91 which I hope will fix the 32-bit issue.

The build-times of these repositories are indeed annoying. I'm sorry about that. Is that timeout a blocker for anything?

I agree that grouping all these packages under the name of any one of them would not be fair, but I'm not sure what a better presentation would be -- riscv-coq isn't even clearly about software on its own. Perhaps the closest existing categorization is that all of these projects started in the MIT PLV ("programming languages and verification") group.

Is the information that would appear in this page somewhere in the repository so that I could pass it by other maintainers of these projects?

JasonGross commented 2 years ago

Please update to coq-fiat-crypto.0.0.17 once https://github.com/coq/opam-coq-archive/pull/2353 is merged

MSoegtropIMC commented 1 year ago

I updated fiat-crypto to version 0.0.17 in the 2022.09.1 prep branch.

MSoegtropIMC commented 1 year ago

FTR: the update of fiat-crypto required these updates of dependencies:

-        PACKAGES="${PACKAGES} coq-riscv.0.0.2"
+        PACKAGES="${PACKAGES} coq-riscv.0.0.3"
-        PACKAGES="${PACKAGES} coq-bedrock2.0.0.3"
+        PACKAGES="${PACKAGES} coq-bedrock2.0.0.4"
-        PACKAGES="${PACKAGES} coq-bedrock2-compiler.0.0.3"
+        PACKAGES="${PACKAGES} coq-bedrock2-compiler.0.0.4"
-        PACKAGES="${PACKAGES} coq-rupicola.0.0.5"
+        PACKAGES="${PACKAGES} coq-rupicola.0.0.6"

Since this is all in the extended level I would say a more formal process is not required and I take your request to update fiat-crypto as request to update the dependencies as well.