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
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