test: Update coverage.cpp to drop linux restriction #32059

pull janb84 wants to merge 1 commits into bitcoin:master from janb84:fix_util_coverage changing 1 files +10 −11
  1. janb84 commented at 5:05 pm on March 13, 2025: contributor

    In PR #31901, Coverage.cpp was introduced as a separate utility file, based on existing code. However, the macro defined in Coverage.cpp was limited to Clang and Linux, which caused issues for users on macOS when using the newly introduced deterministic test tooling.

    This change adds fallback functions which are used when building without code coverage on non linux env. This adds support for macOS to ResetCoverageCounters. ResetCoverageCounters is used by the unit tests in g_rng_temp_path_init to support the deterministic unit test tooling. It is also used in fuzz tests to completely suppress coverage from anything init-related.

    See Readme on how to test this for deterministic unit & fuzz test.

    Suggestion for test files:

    • for unit test: util_string_tests
    • for fuzz test: addition_overflow

    These files should give deterministic results

  2. DrahtBot commented at 5:06 pm on March 13, 2025: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32059.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK maflcko, hodlinator
    Stale ACK Prabhat1308

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

  3. DrahtBot added the label Scripts and tools on Mar 13, 2025
  4. Sjors commented at 5:08 pm on March 13, 2025: member
    From the original PR it’s not obvious how to use this, is there a tl&dr for what to test?
  5. janb84 commented at 5:18 pm on March 13, 2025: contributor

    From the original PR it’s not obvious how to use this, is there a tl&dr for what to test?

    Use this config:

    0cmake -S . -B build -DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' -DCMAKE_CXX_FLAGS='-fprofile-instr-generate -fcoverage-mapping'

    build as normal :

    0 cmake --build build

    Run tooling for util_string_tests:

    0RUST_BACKTRACE=1 cargo run --manifest-path ./contrib/devtools/deterministic-unittest-coverage/Cargo.toml -- $PWD/build util_string_tests

    Should return :

    0*** No errors detected
1The coverage was deterministic across two runs.
  6. DrahtBot added the label CI failed on Mar 13, 2025
  7. DrahtBot commented at 6:28 pm on March 13, 2025: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/38726376360

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  8. in src/test/util/coverage.cpp:7 in 1d6626a8fb outdated 
    3@@ -4,7 +4,7 @@
4 
5 #include <test/util/coverage.h>
6 
7-#if defined(__clang__) && defined(__linux__)
8+#if defined(__clang__) && (defined(__linux__) || defined(__APPLE__))
    brunoerg commented at 7:47 pm on March 13, 2025:
    I think linux is more permissive than macOS that’s why CI is failing. The jobs do not set -fprofile-instr-generate -fcoverage-mapping. I did not test, but using lld could work?
    janb84 commented at 8:22 am on March 14, 2025:
    Thank you for the suggestion, have found a way to fix the code so that the linker does not break and the functionality is the same. Thank you for the review
  9. DrahtBot removed the label CI failed on Mar 13, 2025
  10. Prabhat1308 commented at 3:21 am on March 14, 2025: none
    tACK 611151e
  11. in src/test/util/coverage.cpp:7 in 611151eae7 outdated 
    3@@ -4,19 +4,18 @@
4 
5 #include <test/util/coverage.h>
6 
7-#if defined(__clang__) && defined(__linux__)
8+#if defined(__clang__) && (defined(__linux__) || defined(__APPLE__))
    fanquake commented at 3:49 am on March 14, 2025:
    Why do we need to specify platforms here, rather than just __clang__ (especially if you’re now going to add fallbacks)?
    janb84 commented at 8:20 am on March 14, 2025:
    Good point, I have removed the check in the macro and rebased my commits. Thank you for the review.
  12. maflcko commented at 6:57 am on March 14, 2025: member

    In PR #31901, Coverage.cpp was introduced

    For reference the cpp file was created in fa99c3b544b631cfe34d52fb5e71636aedb1b423, but the code was only moved. You can check this by observing it greyed out via git show fa99c3b544b631cfe34d52fb5e71636aedb1b423 --color-moved=dimmed-zebra.

  13. in src/test/util/coverage.cpp:12 in 611151eae7 outdated 
    12-{
13-    if (__llvm_profile_reset_counters) {
14-        __llvm_profile_reset_counters();
15-    }
16+// Fallback implementations
17+extern "C" void __llvm_profile_reset_counters(void) {}
    maflcko commented at 7:16 am on March 14, 2025:

    in commit “refactor: Improve ResetCoverageCounters function with fallback implem…”:

    I don’t think this is a “refactor” (non-behavior-change), given that CI fails before and passes after.

    maflcko commented at 7:16 am on March 14, 2025:
    Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
    janb84 commented at 8:19 am on March 14, 2025:
    Rebased the code and changed the PR description based on feedback , thank you for the review!
  14. janb84 force-pushed on Mar 14, 2025
  15. DrahtBot added the label CI failed on Mar 14, 2025
  16. DrahtBot commented at 8:01 am on March 14, 2025: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/38761520899

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  17. maflcko commented at 8:10 am on March 14, 2025: member

    No strong opinion, but it could make sense to reword the pull description. Otherwise, it is less clear what all the effects of this will be. Something like:

    This adds support for macOS to ResetCoverageCounters. ResetCoverageCounters is used by the unit tests in g_rng_temp_path_init to support the deterministic unit test tooling. It is also used in fuzz tests to completely suppress coverage from anything init-related.

    Refer to … Readme on how to test this for deterministic unit tests …

    But anything is fine, Concept ACK.

  18. janb84 force-pushed on Mar 14, 2025
  19. maflcko commented at 9:00 am on March 14, 2025: member

    See Readme on how to test this for deterministic unit test

    I guess this can be used to test both (unit and fuzz tests)? If yes, it could make sense to recommend a unit test and fuzz test, each. For example util_string_tests and addition_overflow (haven’t tested either)? Before, the coverage should be non-deterministic, and after this change, it should be deterministic.

  20. janb84 commented at 9:20 am on March 14, 2025: contributor

    See Readme on how to test this for deterministic unit test

    I guess this can be used to test both (unit and fuzz tests)? If yes, it could make sense to recommend a unit test and fuzz test, each. For example util_string_tests and addition_overflow (haven’t tested either)? Before, the coverage should be non-deterministic, and after this change, it should be deterministic.

    Have incorporated your suggestion and checked that the files are indeed deterministic (they are). Again thanks for the review

  21. DrahtBot removed the label CI failed on Mar 14, 2025
  22. in src/test/util/coverage.cpp:13 in f53f540725 outdated 
    14-    if (__llvm_profile_reset_counters) {
15-        __llvm_profile_reset_counters();
16-    }
17+// Fallback implementations
18+extern "C" void __llvm_profile_reset_counters(void) {}
19+extern "C" void __gcov_reset(void) {}
    hodlinator commented at 10:18 am on March 14, 2025:

    Not sure I’ve ever come across this corner of C/C++ before. So you define __llvm_profile_reset_counters(void) __attribute__((weak)) for it to hopefully link to something external, but then define local implements for them in case we don’t link to code coverage libs (build with code coverage enabled)? So for when we include this test-related file but only include unit tests, and build without coverage support?

    From the PR description:

    This change extends the functionality of the function with fallback code so that the linker does not fail on non-linux env.

    Wonder if it would be more correct to say:

    This change adds fallback functions which are used when building without code coverage.

    ?

    janb84 commented at 5:23 pm on March 14, 2025:

    Main problem:

    “macOS’s linker (ld64) doesn’t silently resolve unreferenced weak symbols to NULL when they’re called—it demands a definition”.

    Combined with calling __gcov_reset and __llvm_profile_reset_counters directly, forcing the linker to find a definition, will result in that the linker will break on trying to link the following code:

    0extern "C" void __llvm_profile_reset_counters(void) __attribute__((weak));
1extern "C" void __gcov_reset(void) __attribute__((weak));

    Solution is to add a fallback option :

    0extern "C" __attribute__((weak)) void __llvm_profile_reset_counters(void) {}
1extern "C" __attribute__((weak)) void __gcov_reset(void) {}

    These provide weak definitions—empty implementations that the linker can use if no stronger definitions exist.

    With this in place there, are 2 scenario’s

    1 When Profiling Is Disabled

    • Weak definitions are the only ones available. The linker doesn’t need to choose between multiple weak definitions or rely on order—it picks the {} one because they’re the sole candidates.
    • The weak attribute ensures no conflict if strong definitions appear later (e.g., with profiling enabled), but without profiling, the weak {} versions are sufficient to satisfy the linker.
    • This avoids the “undefined symbol” error as seen before on macOS because a definition exists, even if weak.

    2 When Profiling Is Enabled

    • The runtime provides strong definitions.
    • The weak definitions are overridden (strong beats weak).
    • ResetCoverageCounters() calls the runtime’s implementations.
    hodlinator commented at 9:08 pm on March 15, 2025:

    Thread #32059 (review):

    Thanks for the thorough explanation, it was helpful!

    nit: This seems to build fine on Linux with/without coverage:

    0extern "C" void __llvm_profile_reset_counters(void);
1extern "C" void __gcov_reset(void);

    My guess is that what is important now is that the fallback is weak, not the prototype, could that be? Mac CI seems to accept that, but I’m not sure if our CI tests both non-coverage and coverage builds on Mac.

    Iff the above is a problem on Mac/other platforms though, I have a different variant to suggest: Others might disagree, but I would be happy if you added a second commit which just refactors these existing prototypes to be consistent with their fallback implementations.

    0extern "C" __attribute__((weak)) void __llvm_profile_reset_counters(void);
1extern "C" __attribute__((weak)) void __gcov_reset(void);

    Makes them line up visually with the new fallbacks, and this is test-related code, so we’re a bit more lenient on touching unrelated lines.

    It’s also consistent with Clang docs:

    0__attribute__((weak)) int __llvm_profile_dump(void);

    (I attempted the alternative of instead changing the fallback lines to have __attribute__((weak)) after the function args but Clang warns about GCC incompatibility).

    janb84 commented at 11:21 am on March 16, 2025:

    I have made the fallback functions weak to make the code more robust, it will build without it but it highly depends on linker implementations (E.g. if a linker builder makes different choices it will break). Making the fallback weak is just removing the ambiguity from the equation. Resulting in more robust code.

    Removing the weak attribute from the declarations will result again being at the mercy of the linker. Strong declarations expect strong definitions. If profiling is disabled and only your weak fallbacks exist, some linkers may/will complain about missing symbols. Secondly Removing weak suggests these functions are always available, which isn’t true when profiling is off.

    Therefor I would argue to keep the weak attributes on the declarations but move the attributes to the front to keep them aligned with the rest of the code, as suggested.

    hodlinator commented at 5:00 pm on March 17, 2025:
    Thanks for lining up the attributes!
  23. maflcko commented at 11:50 am on March 14, 2025: member
    Looking at the diff, this seems to be dropping the linux restriction, so “adding support for macOS” doesn’t seem entirely accurate. It could be better to say “drop linux restriction” or “add support for non-linux”. For example, OpenBSD may work as well now.
  24. janb84 renamed this:
    contrib: Update coverage.cpp macro to support macOS
    contrib: Update coverage.cpp macro to drop linux restriction
    on Mar 14, 2025
  25. janb84 renamed this:
    contrib: Update coverage.cpp macro to drop linux restriction
    contrib: Update coverage.cpp to drop linux restriction
    on Mar 14, 2025
  26. janb84 force-pushed on Mar 14, 2025
  27. janb84 commented at 5:25 pm on March 14, 2025: contributor

    In my write-up of the explanation to hodlinator I discovered one edge case that needed fixing, sorry for the new commit.

    Reason: Weak fallbacks (attribute((weak))) were added instead of strong ones to ensure the profiling runtime’s implementations are used when profiling is enabled, avoiding linker conflicts or silent overrides. This guarantees correct counter resets while keeping empty fallbacks when profiling is off, improving portability.

  28. DrahtBot renamed this:
    contrib: Update coverage.cpp to drop linux restriction
    test: Update coverage.cpp to drop linux restriction
    on Mar 14, 2025
  29. maflcko removed the label Scripts and tools on Mar 14, 2025
  30. DrahtBot added the label Tests on Mar 14, 2025
  31. in src/test/util/coverage.cpp:1 in ecdbe72916
    hodlinator commented at 10:32 pm on March 14, 2025:

    nit: The PR title / description conforms fairly well to the contributing guidelines, but it would be nice if you also came a bit closer with the commit message.

    https://github.com/bitcoin/bitcoin/blob/ecdbe72916c1cabbf810e892d25fe6eed30b1306/CONTRIBUTING.md?plain=1#L119-L124 + conventions on non-merge commits make me suggest something closer to:

    0- Extended ResetCoverageCounters in coverage.cpp with fallback implementation for non-linux linker and removed linux macro limitation.
1+ test: Enable ResetCoverageCounters beyond Linux
2+
3+ Non-Linux linkers require a fallback implementation for when coverage is not enabled.
4+ The fallbacks are marked weak in order to have lower precedence than built-in when they are available.
    janb84 commented at 11:07 am on March 16, 2025:
    Ah thanks! Have changed the commit message accordingly.
  32. maflcko commented at 9:02 am on March 15, 2025: member
    review-only ACK ecdbe72916c1cabbf810e892d25fe6eed30b1306
  33. hodlinator approved
  34. hodlinator commented at 9:14 pm on March 15, 2025: contributor

    ACK ecdbe72916c1cabbf810e892d25fe6eed30b1306

    Tested on Linux by running the recommended Rust deterministic unit test coverage utility both without and with coverage support, with expected results.

  35. test: Enable ResetCoverageCounters beyond Linux 
    Non-Linux linkers require a fallback implementation for when coverage is not enabled.
The fallbacks are marked weak to have lower precedence than built-in implementations when available, removing ambiguity from the linker.
    54e6eacc1f
  36. janb84 force-pushed on Mar 16, 2025
  37. maflcko commented at 12:16 pm on March 16, 2025: member
    review-only ACK 54e6eacc1fccd602897d9e3025c62f83194ffd5b
  38. DrahtBot requested review from hodlinator on Mar 16, 2025
  39. hodlinator approved
  40. hodlinator commented at 5:01 pm on March 17, 2025: contributor

    re-ACK 54e6eacc1fccd602897d9e3025c62f83194ffd5b

    Changes since first ACK:

    • Moved weak attribute on the original declarations so they line up with the fallback definitions and mirror Clang docs. Thanks!
    • Improved commit message.

    Retested using Rust utility on coverage-enabled version of the code with successful determinism result.

  41. fanquake merged this on Mar 18, 2025
  42. fanquake closed this on Mar 18, 2025


janb84 DrahtBot Sjors brunoerg Prabhat1308 fanquake maflcko hodlinator

Labels
Tests

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-03-28 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me