status-im / nim-blscurve

Nim implementation of BLS signature scheme (Boneh-Lynn-Shacham) over Barreto-Lynn-Scott (BLS) curve BLS12-381
Apache License 2.0
26 stars 11 forks source link

CI: test with multiple Nim versions #133

Closed stefantalpalaru closed 2 years ago

stefantalpalaru commented 2 years ago

Those Linux CI failures are due to https://github.com/supranational/blst/issues/94

There are workarounds in place, but they somehow fail for Nim-1.6, which includes headers it should not include in its C output:

diff -ur nimcache_1.4/@m..@sblscurve@seth2_keygen@seth2_keygen.nim.c nimcache_1.6/@m..@sblscurve@seth2_keygen@seth2_keygen.nim.c
--- nimcache_1.4/@m..@sblscurve@seth2_keygen@seth2_keygen.nim.c 2022-01-03 03:16:29.296758105 +0100
+++ nimcache_1.6/@m..@sblscurve@seth2_keygen@seth2_keygen.nim.c 2022-01-03 03:17:13.474760284 +0100
@@ -1,16 +1,15 @@
-/* Generated by Nim Compiler v1.4.9 */
-/*   (c) 2021 Andreas Rumpf */
-/* The generated code is subject to the original license. */
+/* Generated by Nim Compiler v1.6.3 */
 /* Compiled for: Linux, amd64, gcc */
 /* Command for C compiler:
    gcc -c  -w -fmax-errors=3   -I/mnt/sde1/storage/nimbus-eth2/vendor/nimbus-build-system/vendor/Nim/lib -I/mnt/sde1/storage/nimbus-eth2/vendor/nim-bl
scurve/tests -o /home/stefan/.cache/nim/eip2333_key_derivation_d/@m..@sblscurve@seth2_keygen@seth2_keygen.nim.c.o /home/stefan/.cache/nim/eip2333_key_d
erivation_d/@m..@sblscurve@seth2_keygen@seth2_keygen.nim.c */
 #define NIM_INTBITS 64
 #define NIM_EmulateOverflowChecks

-/* section: NIM_merge_HEADERS */
-
 #include "nimbase.h"
 #include <string.h>
+#include "/mnt/sde1/storage/nimbus-eth2/vendor/nim-blscurve/blscurve/blst/../../vendor/blst/src/sha256.h"
+#include <math.h>
+#include "/mnt/sde1/storage/nimbus-eth2/vendor/nim-blscurve/blscurve/eth2_keygen/../../vendor/blst/src/vect.h"
 #include "/mnt/sde1/storage/nimbus-eth2/vendor/nim-blscurve/blscurve/blst/../../vendor/blst/bindings/blst.h"
 #undef LANGUAGE_C
 #undef MIPSEB
stefantalpalaru commented 2 years ago

This part of the recursive diff might be the key to our problem:

Only in nimcache_1.4: @m..@sblscurve@sblst@ssha256_abi.nim.c
Only in nimcache_1.4: @m..@sblscurve@sblst@ssha256_abi.nim.c.o

So Nim-1.4 generates a separate C file for that Nim module, while Nim-1.6 inlines it.

Araq commented 2 years ago

That's mysterious, Nim does not "inline" Nim modules.

mratsim commented 2 years ago

I'll confirm later but I think everything that uses specific SIMD in separate Nim files (Arraymancer for instance) is also broken on 1.6

mratsim commented 2 years ago

See:

stefantalpalaru commented 2 years ago
$ diff -ur nimcache_1.4 nimcache_1.6 | grep "^Only.*\.c$" | sort
Only in nimcache_1.4: @m..@sblscurve@sblst@ssha256_abi.nim.c
Only in nimcache_1.4: @m..@sblscurve@seth2_keygen@shkdf.nim.c
Only in nimcache_1.4: @m..@s..@snimcrypto@snimcrypto@shash.nim.c
Only in nimcache_1.4: @m..@s..@snimcrypto@snimcrypto@shmac.nim.c
Only in nimcache_1.4: @m..@s..@snim-stint@sstint@sio.nim.c
Only in nimcache_1.4: @m..@s..@snim-stint@sstint@sprivate@suint_div.nim.c
Only in nimcache_1.4: stdlib_algorithm.nim.c
Only in nimcache_1.4: stdlib_sets.nim.c
Only in nimcache_1.6: @m..@sblscurve@sbls_batch_verifier.nim.c
Only in nimcache_1.6: @m..@s..@snim-stint@sstint.nim.c
Only in nimcache_1.6: stdlib_digitsutils.nim.c
Only in nimcache_1.6: stdlib_dollars.nim.c
SteadBytes commented 2 years ago

@mratsim it looks like you're right about this hitting SIMD in separate files. Compiling Arraymancer with Nim 1.4.8 generates separate files for matrix multiplcation, whereas Nim 1.6.0 and above does not:

Only in nimcache_1.4.8: @m..@ssrc@sarraymancer@slaser@sprimitives@smatrix_multiplication@sgemm_ukernel_avx512.nim.c
Only in nimcache_1.4.8: @m..@ssrc@sarraymancer@slaser@sprimitives@smatrix_multiplication@sgemm_ukernel_generic.nim.c
Only in nimcache_1.4.8: @m..@ssrc@sarraymancer@slaser@sprimitives@smatrix_multiplication@sgemm_ukernel_sse2.nim.c
Vindaar commented 2 years ago

I just did a git bisect on an arraymancer example that breaks with the AVX512 related error. The following is the PR that introduced the regression:

https://github.com/nim-lang/Nim/pull/17311

edit: it even has a "fix arraymancer regression" todo...

stefantalpalaru commented 2 years ago

Could be this change in logic where types from an imported module are moved to the parent module: https://github.com/nim-lang/Nim/pull/17311/files#diff-1208ab8dfbcc04a982aeb83a28655324f8880fa9b756b83db873163dfe1fec6cL1425

ringabout commented 2 years ago

Related: https://github.com/nim-lang/Nim/pull/19363