Skip to content

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Aug 10, 2025

Additionally, this comment has been addressed.

@hebasto
Copy link
Member Author

hebasto commented Aug 10, 2025

FWIW, on Ubuntu 24.04, when using GCC 13.3.0 and gcovr 7.0, I need to pass an extra --merge-mode-functions=separate option to generate a coverage report.

@real-or-random real-or-random added assurance meta/development processes, conventions, developer documentation, etc. labels Aug 11, 2025
@real-or-random
Copy link
Contributor

FWIW, on Ubuntu 24.04, when using GCC 13.3.0 and gcovr 7.0, I need to pass an extra --merge-mode-functions=separate option to generate a coverage report.

Let's add it, and I also need --gcov-suspicious-hits-threshold=140737488355330 (because we hit some lines in int128_impl.h more than 2^32 times...) but this was introduced only in gcovr 8.3. I assume older versions error out when they get this flag?

Otherwise, commands fail with the error:
```
Stderr of gcov was >><< End of stderr
Exception was >>Got function secp256k1_scalar_split_lambda on multiple lines: 67, 142.
	You can run gcovr with --merge-mode-functions=MERGE_MODE.
	The available values for MERGE_MODE are described in the documentation.<< End of stderr
```
@hebasto hebasto force-pushed the 250810-autotools-coverage branch from b1b8869 to 1aecce5 Compare August 11, 2025 10:36
@hebasto
Copy link
Member Author

hebasto commented Aug 11, 2025

FWIW, on Ubuntu 24.04, when using GCC 13.3.0 and gcovr 7.0, I need to pass an extra --merge-mode-functions=separate option to generate a coverage report.

Let's add it...

Thanks! Done.

... and I also need --gcov-suspicious-hits-threshold=140737488355330 (because we hit some lines in int128_impl.h more than 2^32 times...) but this was introduced only in gcovr 8.3. I assume older versions error out when they get this flag?

Yes, it outputs:

usage: gcovr [options] [search_paths...]
gcovr: error: unrecognized arguments: --gcov-suspicious-hits-threshold=140737488355330

@real-or-random
Copy link
Contributor

Yes, it outputs:

usage: gcovr [options] [search_paths...]
gcovr: error: unrecognized arguments: --gcov-suspicious-hits-threshold=140737488355330

Then I guess the best thing for now is to add a note/comment that this should be omitted on gcovr before 8.3.

@hebasto
Copy link
Member Author

hebasto commented Aug 11, 2025

FWIW, on Ubuntu 24.04, when using GCC 13.3.0 and gcovr 7.0, I need to pass an extra --merge-mode-functions=separate option to generate a coverage report.

Let's add it, and I also need --gcov-suspicious-hits-threshold=140737488355330 (because we hit some lines in int128_impl.h more than 2^32 times...) but this was introduced only in gcovr 8.3. I assume older versions error out when they get this flag?

What GCC version are you using? Any other specific steps to reproduce the issue?

EDIT: nm, I can reproduce it on Fedora 42 with GCC 15.1.1.

@real-or-random
Copy link
Contributor

I think an alternative for <8.3 is --gcov-ignore-parse-errors=suspicious_hits.warn, which should work on >=6.0 if I understand the changelog correctly.

Fwiw:

gcovr 8.4.dev0+gfe536afac.d20250125
gcc (GCC) 15.1.1 20250729

@hebasto
Copy link
Member Author

hebasto commented Aug 11, 2025

I think an alternative for <8.3 is --gcov-ignore-parse-errors=suspicious_hits.warn, which should work on >=6.0 if I understand the changelog correctly.

It was introduced in gcovr/gcovr#898 and has been available since version 8.0.

The --gcov-ignore-parse-errors=all option can be used instead.

@hebasto
Copy link
Member Author

hebasto commented Aug 11, 2025

The --gcov-ignore-parse-errors=all option can be used instead.

Taken this:

$ gcovr --gcov-ignore-parse-errors=all --merge-mode-functions=separate --exclude 'src/bench*' --exclude 'src/modules/.*/bench_impl.h' --print-summary
(INFO) Reading coverage data...
(WARNING) Ignoring suspicious hits in line 'static SECP256K1_INLINE void secp256k1_u128_accum_mul(secp256k1_uint128 *r, uint64_t a, uint64_t b) {'.
(WARNING) Ignoring suspicious hits in line '   *r += (uint128_t)a * b;'.
(WARNING) Ignoring suspicious hits in line '}'.
(WARNING) Ignoring suspicious hits in line 'static SECP256K1_INLINE uint64_t secp256k1_u128_to_u64(const secp256k1_uint128 *a) {'.
(WARNING) Ignoring suspicious hits in line '   return (uint64_t)(*a);'.
(WARNING) Ignoring suspicious hits in line '                for (p = 0; p < 16; ++p) { /* p loops over the bit positions in mul2[j]. */'.
(WARNING) Ignoring suspicious hits in line '                for (p = 0; p < 16; ++p) { /* p loops over the bit positions in mul2[j]. */'.
(WARNING) Ignoring suspicious hits in line '                    int bitpos = j * 16 - i + p; /* bitpos is the correspond bit position in m. */'.
(WARNING) Ignoring suspicious hits in line '                    if (bitpos >= 0 && bitpos < 256) {'.
(WARNING) Ignoring suspicious hits in line '                    if (bitpos >= 0 && bitpos < 256) {'.
(WARNING) Ignoring suspicious hits in line '                    if (bitpos >= 0 && bitpos < 256) {'.
(WARNING) Ignoring suspicious hits in line '                        sub |= ((m[bitpos >> 4] >> (bitpos & 15)) & 1) << p;'.
(WARNING) Ignoring suspicious hits in line 'static SECP256K1_INLINE void secp256k1_u128_accum_mul(secp256k1_uint128 *r, uint64_t a, uint64_t b) {'.
(WARNING) Ignoring suspicious hits in line '   *r += (uint128_t)a * b;'.
(WARNING) Ignoring suspicious hits in line '}'.
(WARNING) Ignoring suspicious hits in line 'static SECP256K1_INLINE void secp256k1_u128_rshift(secp256k1_uint128 *r, unsigned int n) {'.
(WARNING) Ignoring suspicious hits in line '   *r >>= n;'.
(WARNING) Ignoring suspicious hits in line '}'.
(WARNING) Ignoring suspicious hits in line 'static SECP256K1_INLINE uint64_t secp256k1_u128_to_u64(const secp256k1_uint128 *a) {'.
(WARNING) Ignoring suspicious hits in line '   return (uint64_t)(*a);'.
(INFO) Writing coverage report...
------------------------------------------------------------------------------
                           GCC Code Coverage Report
<snip>

@real-or-random
Copy link
Contributor

lgtm but I suggest adding a note like this:

On gcovr >=8.3, --gcov-ignore-parse-errors=all can be replaced with --gcov-suspicious-hits-threshold=140737488355330.

Then we have a built-in reminder that we can remove the old argument in the future.

Otherwise, commands might fail due to bugs in the `gcov` tool.
@hebasto hebasto force-pushed the 250810-autotools-coverage branch from 9982c5c to 0458def Compare August 12, 2025 08:12
@hebasto
Copy link
Member Author

hebasto commented Aug 12, 2025

lgtm but I suggest adding a note like this:

On gcovr >=8.3, --gcov-ignore-parse-errors=all can be replaced with --gcov-suspicious-hits-threshold=140737488355330.

Then we have a built-in reminder that we can remove the old argument in the future.

Thanks! The note has been added.

Copy link
Contributor

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

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

utACK 0458def

@real-or-random
Copy link
Contributor

@josibake Want to review this (with whatever tool versions you have on your system)?

@josibake
Copy link
Member

Nice! Thanks for adding this, I ran into this recently and added --gcov-ignore-parse-errors=all after some googling. When I run with --gcov-ignore-parse-errors=all with gcovr@8.3, I see the following warnings:

gcovr --gcov-ignore-parse-errors=all --merge-mode-functions=separate --exclude 'src/bench*' --exclude 'src/modules/.*/bench_impl.h' --print-summary
(INFO) Reading coverage data...
(WARNING) Ignoring suspicious hits in line 'static SECP256K1_INLINE void secp256k1_u128_accum_mul(secp256k1_uint128 *r, uint64_t a, uint64_t b) {'.
(WARNING) Ignoring suspicious hits in line '   *r += (uint128_t)a * b;'.
(WARNING) Ignoring suspicious hits in line '}'.
(WARNING) Ignoring suspicious hits in line 'static SECP256K1_INLINE void secp256k1_u128_rshift(secp256k1_uint128 *r, unsigned int n) {'.
(WARNING) Ignoring suspicious hits in line '   *r >>= n;'.
(WARNING) Ignoring suspicious hits in line '}'.
(WARNING) Ignoring suspicious hits in line 'static SECP256K1_INLINE uint64_t secp256k1_u128_to_u64(const secp256k1_uint128 *a) {'.
(WARNING) Ignoring suspicious hits in line '   return (uint64_t)(*a);'.
(INFO) Writing coverage report...
------------------------------------------------------------------------------
                           GCC Code Coverage Report
Directory: .
------------------------------------------------------------------------------
<snip>

The warning goes away when running with --gcov-suspicious-hits-threshold=140737488355330, i.e.:

gcovr --gcov-suspicious-hits-threshold=140737488355330 --merge-mode-functions=separate --exclude 'src/bench*' --exclude 'src/modules/.*/bench_impl.h' --print-summary
(INFO) Reading coverage data...
(INFO) Writing coverage report...
------------------------------------------------------------------------------
                           GCC Code Coverage Report
Directory: .
------------------------------------------------------------------------------
<snip>

Also confirmed I got the same coverage report with both flags.

Copy link
Member

@josibake josibake left a comment

Choose a reason for hiding this comment

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

ACK 0458def

Tested with gcovr@8.3 and confirmed I get a warning with the old flag (--gcovr-ignore-parse-errors=all), and that those warnings go away when running with the new flag suggest for gcovr >= 8.3

@real-or-random real-or-random merged commit d599714 into bitcoin-core:master Aug 13, 2025
116 checks passed
@hebasto hebasto deleted the 250810-autotools-coverage branch August 13, 2025 20:06
josibake added a commit to josibake/bitcoin that referenced this pull request Sep 5, 2025
aa85bfb530 docs: update README
9f42a30b82 ci: enable silentpayments module
d504e48145 tests: add sha256 tag test
124750d580 tests: add constant time tests
b35ffa2e30 tests: add BIP-352 test vectors
038c5b9c9d silentpayments: add benchmarks for scanning
88eb3d4545 silentpayments: add examples/silentpayments.c
22b20fd617 silentpayments: receiving
df1de93765 silentpayments: recipient label support
76a0451c76 silentpayments: sending
3cd3a93bff build: add skeleton for new silentpayments (BIP352) module
f36afb8b3d Merge bitcoin-core/secp256k1#1725: tests: refactor tagged hash verification
5153cf1c91 tests: refactor tagged hash tests
d2dcf52091 Merge bitcoin-core/secp256k1#1726: docs: fix broken link to Tromer's cache.pdf paper
489a43d1bf docs: fix broken link to eprint cache.pdf paper
d599714147 Merge bitcoin-core/secp256k1#1722: docs: Exclude modules' `bench_impl.h` headers from coverage report
0458def51e doc: Add `--gcov-ignore-parse-errors=all` option to `gcovr` invocations
1aecce5936 doc: Add `--merge-mode-functions=separate` option to `gcovr` invocations
106a7cbf41 doc: Exclude modules' `bench_impl.h` headers from coverage report
a9e955d3ea autotools, docs: Adjust help string for `--enable-coverage` option
e523e4f90e Merge bitcoin-core/secp256k1#1720: chore(ci): Fix typo in Dockerfile comment
24ba8ff168 chore(ci): Fix typo in Dockerfile comment
74b8068c5d Merge bitcoin-core/secp256k1#1717: test: update wycheproof test vectors
c25c3c8a88 test: update wycheproof test vectors
20e3b44746 Merge bitcoin-core/secp256k1#1688: cmake: Avoid contaminating parent project's cache with `BUILD_SHARED_LIBS`
2c076d907a Merge bitcoin-core/secp256k1#1711: tests: update Wycheproof
7b07b22957 cmake: Avoid contaminating parent project's cache with BUILD_SHARED_LIBS
5433648ca0 Fix typos and spellings
9ea54c69b7 tests: update Wycheproof files

git-subtree-dir: src/secp256k1
git-subtree-split: aa85bfb530b9ffc3dde6eaa7a976e129b8bd2f58
vmta added a commit to umkoin/umkoin that referenced this pull request Sep 21, 2025
36e76952c Merge bitcoin-core/secp256k1#1738: check-abi: remove support for obsolete CMake library output location (src/libsecp256k1.so)
4985ac0f8 Merge bitcoin-core/secp256k1#1737: doc: mention ctx requirement for `_ellswift_create` (not secp256k1_context_static)
7ebaa134a check-abi: remove support for obsolete CMake library output location (src/libsecp256k1.so)
806de38bf doc: mention ctx requirement for `_ellswift_create` (not secp256k1_context_static)
03fb60ad2 Merge bitcoin-core/secp256k1#1681: doc: Recommend clang-cl when building on Windows
d93380fb3 Merge bitcoin-core/secp256k1#1731: schnorrsig: Securely clear buf containing k or its negation
8113671f8 Merge bitcoin-core/secp256k1#1729: hash: Use size_t instead of int for RFC6979 outlen copy
325d65a8c Rename and clear var containing k or -k
960ba5f9c Use size_t instead of int for RFC6979 outlen copy
737912430 ci: Add more tests for clang-cl
7379a5bed doc: Recommend clang-cl when building on Windows
f36afb8b3 Merge bitcoin-core/secp256k1#1725: tests: refactor tagged hash verification
5153cf1c9 tests: refactor tagged hash tests
d2dcf5209 Merge bitcoin-core/secp256k1#1726: docs: fix broken link to Tromer's cache.pdf paper
489a43d1b docs: fix broken link to eprint cache.pdf paper
d59971414 Merge bitcoin-core/secp256k1#1722: docs: Exclude modules' `bench_impl.h` headers from coverage report
0458def51 doc: Add `--gcov-ignore-parse-errors=all` option to `gcovr` invocations
1aecce593 doc: Add `--merge-mode-functions=separate` option to `gcovr` invocations
106a7cbf4 doc: Exclude modules' `bench_impl.h` headers from coverage report
a9e955d3e autotools, docs: Adjust help string for `--enable-coverage` option
e523e4f90 Merge bitcoin-core/secp256k1#1720: chore(ci): Fix typo in Dockerfile comment
24ba8ff16 chore(ci): Fix typo in Dockerfile comment
74b8068c5 Merge bitcoin-core/secp256k1#1717: test: update wycheproof test vectors
c25c3c8a8 test: update wycheproof test vectors
20e3b4474 Merge bitcoin-core/secp256k1#1688: cmake: Avoid contaminating parent project's cache with `BUILD_SHARED_LIBS`
2c076d907 Merge bitcoin-core/secp256k1#1711: tests: update Wycheproof
7b07b2295 cmake: Avoid contaminating parent project's cache with BUILD_SHARED_LIBS
5433648ca Fix typos and spellings
9ea54c69b tests: update Wycheproof files
b9313c6e1 Merge bitcoin-core/secp256k1#1708: release cleanup: bump version after 0.7.0
a660a4976 Merge bitcoin-core/secp256k1#1707: release: Prepare for 0.7.0
7ab8b0cc0 release cleanup: bump version after 0.7.0
a3e742d94 release: Prepare for 0.7.0
f67b0ac1a ci: Don't hardcode ABI version
020ee6049 Merge bitcoin-core/secp256k1#1706: musig/tests: initialize keypair
cde413089 musig/tests: initialize keypair
6037833c9 Merge bitcoin-core/secp256k1#1702: changelog: update
40b4a0652 changelog: update
5e74086dc Merge bitcoin-core/secp256k1#1705: musig/test: Remove dead code
7c3380423 Merge bitcoin-core/secp256k1#1696: build: Refactor visibility logic and add override
8d967a602 musig/test: Remove dead code
983711cd6 musig/tests: Refactor vectors_signverify
73a695958 Merge bitcoin-core/secp256k1#1704: cmake: Make `secp256k1_objs` inherit interface defines from `secp256k1`
bf082221f cmake: Make `secp256k1_objs` inherit interface defines from `secp256k1`
c82d84bb8 build: add CMake option for disabling symbol visibility attributes
ce7923874 build: Add SECP256K1_NO_API_VISIBILITY_ATTRIBUTES
e5297f6d7 build: Refactor visibility logic
cbbbf3bd6 Merge bitcoin-core/secp256k1#1699: ci: enable musig module for native macOS arm64 job
943479a7a Merge bitcoin-core/secp256k1#1694: Revert "cmake: configure libsecp256k1.pc during install"
3352f9d66 ci: enable musig module for native macOS arm64 job
ad60ef7ea Merge bitcoin-core/secp256k1#1689: ci: Convert `arm64` Cirrus tasks to GHA jobs
c49877909 Merge bitcoin-core/secp256k1#1687: cmake: support the use of launchers in ctest -S scripts
44b205e9e Revert "cmake: configure libsecp256k1.pc during install"
0dfe387db cmake: support the use of launchers in ctest -S scripts
89096c234 Merge bitcoin-core/secp256k1#1692: cmake: configure libsecp256k1.pc during install
7106dce6f cmake: configure libsecp256k1.pc during install
29e73f4ba Merge bitcoin-core/secp256k1#1685: cmake: Emulate Libtool's behavior on FreeBSD
746e36b14 Merge bitcoin-core/secp256k1#1678: cmake: add a helper for linking into static libs
a28c2ffa5 Merge bitcoin-core/secp256k1#1683: README: add link to musig example
2a9d37473 Merge bitcoin-core/secp256k1#1690: ci: Bump GCC snapshot major version to 16
add146e10 ci: Bump GCC snapshot major version to 16
004f57fcd ci: Move Valgrind build for `arm64` from Cirrus to GHA
5fafdfc30 ci: Move `gcc-snapshot` build for `arm64` from Cirrus to GHA
e814b79a8 ci: Switch `arm64_debian` from QEMU to native `arm64` Docker image
bcf77346b ci: Add `arm64` architecture to `docker_cache` job
b77aae922 ci: Rename Docker image tag to reflect architecture
145ae3e28 cmake: add a helper for linking into static libs
819210974 README: add link to musig example, generalize module enabling hint
95db29b14 Merge bitcoin-core/secp256k1#1679: cmake: Use `PUBLIC_HEADER` target property in installation logic
37dd422b5 cmake: Emulate Libtool's behavior on FreeBSD
f24b838be Merge bitcoin-core/secp256k1#1680: doc: Promote "Building with CMake" to standard procedure
3f31ac43e doc: Promote "Building with CMake" to standard procedure
6f67151ee cmake: Use `PUBLIC_HEADER` target property
c32715b2a cmake, move-only: Move module option processing to `src/CMakeLists.txt`
201b2b8f0 Merge bitcoin-core/secp256k1#1675: cmake: Bump minimum required CMake version to 3.22
3af71987a cmake: Bump minimum required CMake version to 3.22
92394476e Merge bitcoin-core/secp256k1#1673: Assert field magnitude at control-flow join
3a4f448cb Assert field magnitude at control-flow join

git-subtree-dir: src/secp256k1
git-subtree-split: 36e76952cbf1cf54ddd2d8756cc31a486e2ba1d9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assurance meta/development processes, conventions, developer documentation, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants