Add arm8 tests to the testing configuration.

gmaxwell commented at 9:21 pm on August 7, 2020: contributor

This also adds a config set for valgrind without ASM.

Add arm8 tests to the testing configuration. 63f7ea6f3b

gmaxwell commented at 9:24 pm on August 7, 2020: contributor

The fact that the matrix exclusions needs to match the arguments exactly seems extremely inelegant to me.

gmaxwell commented at 10:07 pm on August 7, 2020: contributor

This should not go in before #558 because it would needlessly conflict it.

real-or-random commented at 5:45 pm on August 8, 2020: contributor

The fact that the matrix exclusions needs to match the arguments exactly seems extremely inelegant to me.

Indeed. Could we do better with ASM=auto maybe?

This should not go in before #558 because it would needlessly conflict it.

Yeah, well, #558. needs a trivial rebase anyway already now. But we can wait of course.

gmaxwell commented at 6:59 pm on August 8, 2020: contributor

Does anyone have any idea how much overhead each travis configuration has? If it’s a lot the test script could be configured to test: (non)endo * 32/64/asm (as applicable). Right now travis is taking an utterly absurd amount of time: >4 hours to run tests that I can run locally in a few minutes.

It also might be wise to hide scalar/field size from end users: The mixed configurations never make sense– I don’t even think they’re worth testing (except that a user could accidentally set one so they should be tested so long as users can do that), and the only times you’d set 32-bit on a 64-bit platform are for development testing or to use some static analysis tooling or compiler that doesn’t support the 128 bit type– which configure will detect.

real-or-random commented at 10:39 pm on August 8, 2020: contributor

This is a summary of the failures: https://travis-ci.org/github/bitcoin-core/secp256k1/jobs/715979716#L361 valgrind and memcpy again https://travis-ci.org/github/bitcoin-core/secp256k1/jobs/715979683#L395 haven’t seen this one, seems another valgrind false positive type? https://travis-ci.org/github/bitcoin-core/secp256k1/jobs/715979697#L359 clang, multiple of the second but also conditional jump depends on unit value (not in ct test)

gmaxwell commented at 11:35 pm on August 8, 2020: contributor

Well that’s a bummer– I tried a number of configs on my own repository before opening the PR. My personal experience is that valgrind doesn’t generally have false positives– though sometimes if you use old valgrind with newer compilers the compilers will emit something valgrind doesn’t like.

The overlapping memcpy is a known thing. I’m not sure what to do about that, it’s just luck of compiler versions that you’re not seeing those prior to this PR.

I’m trying to reproduce the other ones locally.

gmaxwell commented at 0:18 am on August 9, 2020: contributor

 0$ CC=clang ./configure --enable-experimental=yes --enable-endomorphism=yes --with-field=auto --with-bignum=no --with-asm=no --with-scalar=auto --enable-ecmult-static-precomputation=yes --with-ecmult-gen-precision=auto --enable-module-ecdh=yes --enable-module-recovery=yes --host= --disable-openssl-tests
 1...
 2
 3Build Options:
 4  with endomorphism       = yes
 5  with ecmult precomp     = yes
 6  with external callbacks = no
 7  with benchmarks         = yes
 8  with coverage           = no
 9  module ecdh             = yes
10  module recovery         = yes
11
12  asm                     = no
13  bignum                  = no
14  field                   = 64bit
15  scalar                  = 64bit
16  ecmult window size      = 15
17  ecmult gen prec. bits   = 4
18
19  valgrind                = yes
20  CC                      = clang
21  CFLAGS                  = -O2 -fvisibility=hidden -std=c89 -pedantic -Wall -Wextra -Wcast-align -Wnested-externs -Wshadow -Wstrict-prototypes -Wno-unused-function -Wno-long-long -Wno-overlength-strings -W -g
22  CPPFLAGS                = 
23  LDFLAGS                 = 
24...
25$ valgrind --error-exitcode=42 ./tests 16 98b5e437ba59fc426028099f52dba793
26==57570== Memcheck, a memory error detector
27==57570== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
28==57570== Using Valgrind-3.15.0 and LibVEX; rerun with -h for copyright info
29==57570== Command: ./tests 16 98b5e437ba59fc426028099f52dba793
30==57570== 
31test count = 16
32random seed = 98b5e437ba59fc426028099f52dba793
33random run = 019f5d169e93e54d93c8e819630a4c3d
34no problems found
35==57570== 
36==57570== HEAP SUMMARY:
37==57570==     in use at exit: 0 bytes in 0 blocks
38==57570==   total heap usage: 31,654 allocs, 31,654 frees, 98,960,674,908 bytes allocated
39==57570== 
40==57570== All heap blocks were freed -- no leaks are possible
41==57570== 
42==57570== For lists of detected and suppressed errors, rerun with: -s
43==57570== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
44$ clang -v
45clang version 10.0.0-4ubuntu1

Maybe a product of travis using an older valgrind version?

real-or-random commented at 10:33 am on August 9, 2020: contributor

For the memcpy, we should really talk to valgrind.

Maybe a product of travis using an older valgrind version?

There’s a valgrind “snap”, see https://docs.travis-ci.com/user/installing-dependencies/#installing-snap-packages-with-the-snaps-addon and https://snapcraft.io/valgrind. May be worth trying, or we could just install it from source. But yeah, it’s not nice.

elichai commented at 10:39 am on August 9, 2020: contributor

For the memcpy, we should really talk to valgrind.

FWIW It sounds like it is really doable to change the standard on that, and It’s on my queue to write the proposal draft for this (working on way too many things right now) but trying to re-ask valgrind about this is a good idea

real-or-random commented at 11:24 am on August 9, 2020: contributor

FWIW It sounds like it is really doable to change the standard on that, and It’s on my queue to write the proposal draft for this (working on way too many things right now)

I don’t want to stop you and I don’t have experience with the process but I doubt that this is an efficient use of time.

gmaxwell commented at 11:51 am on August 9, 2020: contributor

or we could just install it from source. But yeah, it’s not nice.

Well one advantage of that is that it’s trivial to patch it to get rid of the overlapping warning when the addresses are equal. But I’m generally concerned about the overall ecosystem effect, authors of security critical software shouldn’t have their ability to valgrind undermined. (they often don’t already have the problem: libsecp256k1’s use of struct assignments where the structs are sometimes equal and where the struct is over the size threshold where the compiler will memcpy is weird and doesn’t often show up in other codebases)

real-or-random commented at 11:37 am on June 10, 2021: contributor

I think this was solved in #930 .

real-or-random closed this on Jun 10, 2021

Add arm8 tests to the testing configuration. #790