test, subprocess: Improve coverage report correctness #30075

pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:240509-coverage changing 2 files +16 −0
  1. hebasto commented at 5:00 pm on May 9, 2024: member

    As a child process uses the execvp() call, an explicit dumping of the collected profile information is required.

    Coverage:

    • on the master branch: image

    • with this PR: image

    This PR mostly follows gcc/libgcc/libgcov-interface.c#L320-L332:

     0/* A wrapper for the execvp function.  Flushes the accumulated
     1   profiling data, so that they are not lost.  */
     2
     3
     4int
     5__gcov_execvp (const char *path, char *const argv[])
     6{
     7  /* Dump counters only, they will be lost after exec.  */
     8  __gcov_dump ();
     9  int ret = execvp (path, argv);
    10  /* We reach this code only when execv fails, reset counter then here.  */
    11  __gcov_reset ();
    12  return ret;
    13}
    
  2. hebasto added the label Tests on May 9, 2024
  3. DrahtBot commented at 5:00 pm on May 9, 2024: contributor

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

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Approach ACK ajtowns

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #30756 (run_command: Close non-std fds when execing slave processes by luke-jr)
    • #30664 (build: Remove Autotools-based build system by hebasto)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  4. theuni commented at 7:07 pm on May 9, 2024: member
    Are there some docs somewhere that suggest this?
  5. hebasto commented at 7:12 pm on May 9, 2024: member
  6. in src/util/subprocess.h:1380 in e3e31446c3 outdated
    1374@@ -1366,6 +1375,11 @@ namespace detail {
    1375       util::write_n(err_wr_pipe_, err_msg.c_str(), err_msg.length());
    1376     }
    1377 
    1378+#ifdef CODE_COVERAGE_ENABLED
    1379+      __gcov_dump();
    1380+      __gcov_reset();
    


    ajtowns commented at 1:20 am on May 10, 2024:

    These lines are indented too much compared to surrounding code.

    Are they necessary at all? [EDIT: Probably yes, because they precede an _exit() call which presumably skips the cleanup/dump code that is only included in regular exit()]


    hebasto commented at 1:19 pm on May 10, 2024:
    Well, reset seem unneeded here. I’m going to drop it.
  7. in src/util/subprocess.h:1364 in e3e31446c3 outdated
    1356@@ -1353,6 +1357,11 @@ namespace detail {
    1357       if (stream.err_write_ != -1 && stream.err_write_ > 2)
    1358         close(stream.err_write_);
    1359 
    1360+#ifdef CODE_COVERAGE_ENABLED
    1361+      __gcov_dump();
    1362+      __gcov_reset();
    


    ajtowns commented at 1:24 am on May 10, 2024:
    The code in https://inbox.sourceware.org/gcc-cvs/20200505141638.8B852388F43D@sourceware.org/T/ suggests that __gcov_dump() should be called before exec() because otherwise a successful exec will cause the information to be discarded, and __gcov_reset() should be called after exec() returns, because that means exec failed and the information wasn’t discarded, so coverage stats should continue to be gathered without duplicating the info that’s already been dumped. Doing the reset unconditionally prior to the exec function this way seems like a reasonable alternative approach to that to me.

    hebasto commented at 1:18 pm on May 10, 2024:

    Doing the reset unconditionally prior to the exec function this way seems like a reasonable alternative approach to that to me.

    It looks better to me because it counts the failed execvp(), which returns.

  8. ajtowns commented at 1:24 am on May 10, 2024: contributor
    Approach ACK
  9. test, subprocess: Improve coverage report correctness
    See: https://gcc.gnu.org/onlinedocs/gcc/Gcov-and-Optimization.html
    cc49aed550
  10. hebasto force-pushed on May 10, 2024
  11. hebasto commented at 1:28 pm on May 10, 2024: member
    Addressed @ajtowns’s feedback. Added comments. Updated the PR description.
  12. hebasto added the label Needs CMake port on Aug 16, 2024
  13. DrahtBot added the label Needs rebase on Sep 2, 2024
  14. DrahtBot commented at 10:44 pm on September 2, 2024: contributor

    🐙 This pull request conflicts with the target branch and needs rebase.

  15. maflcko removed the label Needs CMake port on Sep 3, 2024

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: 2024-09-28 22:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me