Skip to content

Improve precision of code coverage and add report to CI #954

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Improve precision of code coverage and add report to CI #954

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
af0b3e6
ci: Add code coverage report
real-or-random Jun 17, 2021
be2df91
Use CHECK only in the tests
real-or-random Jun 22, 2021
38f84ce
Don't actually CHECK in tests in COVERAGE mode
real-or-random Jun 17, 2021
249cdee
Fix unused variable warnings in COVERAGE mode
real-or-random Jun 17, 2021
6b8dd8a
Exclude valgrind ctime test from coverage reports
real-or-random Jun 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 31 additions & 1 deletion .cirrus.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,37 @@ merge_base_script_snippet: &MERGE_BASE
- git config --global user.name "ci"
- git merge FETCH_HEAD # Merge base to detect silent merge conflicts

task:
name: Code coverage
container:
dockerfile: ci/linux-debian.Dockerfile
cpu: 1
memory: 1G
env:
EXTRAFLAGS: "--enable-coverage"
ASM: no
ECDH: yes
RECOVERY: yes
EXPERIMENTAL: yes
SCHNORRSIG: yes
CTIMETEST: no
BENCH: no
matrix:
- env:
WIDEMUL: int128
STATICPRECOMPUTATION: yes
- env:
WIDEMUL: int64
STATICPRECOMPUTATION: no
<< : *MERGE_BASE
test_script:
- ./ci/cirrus.sh
<< : *CAT_LOGS
gcovr_script:
- gcovr --print-summary --exclude 'src/bench*' --exclude 'src/valgrind_ctime_test.c' --html --html-title "${CIRRUS_CHANGE_IN_REPO:0:7}, STATICPRECOMPUTATION=$STATICPRECOMPUTATION, WIDEMUL=$WIDEMUL" --html-details -o index.html
coverage_report_artifacts:
path: "**.html"

task:
name: "x86_64: Linux (Debian stable)"
container:
Expand Down Expand Up @@ -312,4 +343,3 @@ task:
test_script:
- ./ci/cirrus.sh
<< : *CAT_LOGS

4 changes: 2 additions & 2 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,12 +92,12 @@ Run the tests:

To create a report, `gcovr` is recommended, as it includes branch coverage reporting:

$ gcovr --exclude 'src/bench*' --print-summary
$ gcovr --exclude 'src/bench*' --exclude 'src/valgrind_ctime_test.c' --print-summary

To create a HTML report with coloured and annotated source code:

$ mkdir -p coverage
$ gcovr --exclude 'src/bench*' --html --html-details -o coverage/coverage.html
$ gcovr --exclude 'src/bench*' --exclude 'src/valgrind_ctime_test.c' --html --html-details -o coverage/coverage.html

Reporting a vulnerability
------------
Expand Down
2 changes: 1 addition & 1 deletion ci/linux-debian.Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ RUN apt-get update
# llvm: for llvm-symbolizer, which is used by clang's UBSan for symbolized stack traces
RUN apt-get install --no-install-recommends --no-upgrade -y \
git ca-certificates \
make automake libtool pkg-config dpkg-dev valgrind qemu-user \
make automake libtool pkg-config dpkg-dev valgrind qemu-user gcovr \
gcc clang llvm libc6-dbg \
gcc-i686-linux-gnu libc6-dev-i386-cross libc6-dbg:i386 libubsan1:i386 libasan5:i386 \
gcc-s390x-linux-gnu libc6-dev-s390x-cross libc6-dbg:s390x \
Expand Down
7 changes: 3 additions & 4 deletions src/ecmult_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,12 +417,11 @@ static int secp256k1_ecmult_wnaf(int *wnaf, int len, const secp256k1_scalar *a,

bit += now;
}
#ifdef VERIFY
CHECK(carry == 0);
VERIFY_CHECK(carry == 0);
while (bit < 256) {
CHECK(secp256k1_scalar_get_bits(&s, bit++, 1) == 0);
VERIFY_CHECK(secp256k1_scalar_get_bits(&s, bit, 1) == 0);
bit++;
}
#endif
return last_set_bit + 1;
}

Expand Down
24 changes: 21 additions & 3 deletions src/util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,11 @@ static SECP256K1_INLINE void secp256k1_callback_call(const secp256k1_callback *
cb->fn(text, (void*)cb->data);
}

#ifdef DETERMINISTIC
#if defined(COVERAGE)
/* Do nothing in COVERAGE mode. This will make the compiler optimize away the actual branch,
and we get useful branch coverage within our test files. */
#define TEST_FAILURE(msg)
#elif defined(DETERMINISTIC)
Comment on lines +28 to +32
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be unnecessary now that the definition of CHECK is changed.

#define TEST_FAILURE(msg) do { \
fprintf(stderr, "%s\n", msg); \
abort(); \
Expand All @@ -43,7 +47,17 @@ static SECP256K1_INLINE void secp256k1_callback_call(const secp256k1_callback *
#define EXPECT(x,c) (x)
#endif

#ifdef DETERMINISTIC
/* CHECK() is like assert(). We use it only in the tests. */
#if defined(COVERAGE)
/* Don't abort in coverage mode.
This avoids branches which are not expected to be taken.
We still use cond as usual to avoid unused variable warnings. */
Copy link
Contributor Author

@real-or-random real-or-random Jun 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/unused variable/unused return values/

(or actually both, but the "unused variables" can be suppressed by casting to void, which would the thing most people would expect.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps also note that this works because an if statement with an empty consequent is not treated as a branch.

#define CHECK(cond) do { \
if (EXPECT(!(cond), 0)) { \
; \
} \
} while (0)
#elif defined(DETERMINISTIC)
#define CHECK(cond) do { \
if (EXPECT(!(cond), 0)) { \
TEST_FAILURE("test condition failed"); \
Expand All @@ -59,7 +73,11 @@ static SECP256K1_INLINE void secp256k1_callback_call(const secp256k1_callback *

/* Like assert(), but when VERIFY is defined, and side-effect safe. */
#if defined(COVERAGE)
#define VERIFY_CHECK(check)
/* Do nothing in coverage mode but try to stay syntactically correct.
This suppresses a lot of implicit branches introduced by shortcutting
operators at the cost of not being side-effect safe in coverage mode.
We rely on the compiler to eliminate the if (0) statement entirely. */
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what GCC does. I can't really test with clang because the coverage report is empty, even though it should work with clang too when passing --gcov-executable="llvm-cov gcov" to gcovr. No idea what's wrong or if this is a bug somewhere in the tools.

#define VERIFY_CHECK(cond) do { if (0) (void)(cond); } while(0)
#define VERIFY_SETUP(stmt)
#elif defined(VERIFY)
#define VERIFY_CHECK CHECK
Expand Down