Skip to content

test, subprocess: Improve coverage report correctness #30075

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

test, subprocess: Improve coverage report correctness #30075

wants to merge 1 commit into from

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented May 9, 2024 •
edited
Loading

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:

/* A wrapper for the execvp function.  Flushes the accumulated
   profiling data, so that they are not lost.  */


int
__gcov_execvp (const char *path, char *const argv[])
{
  /* Dump counters only, they will be lost after exec.  */
  __gcov_dump ();
  int ret = execvp (path, argv);
  /* We reach this code only when execv fails, reset counter then here.  */
  __gcov_reset ();
  return ret;
}

@hebasto hebasto added the Tests label May 9, 2024
@DrahtBot
Copy link
Contributor

DrahtBot commented May 9, 2024 •
edited
Loading

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/30075.

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.

@theuni
Copy link
Member

theuni commented May 9, 2024

Are there some docs somewhere that suggest this?

@hebasto
Copy link
Member Author

hebasto commented May 9, 2024 •
edited
Loading

Are there some docs somewhere that suggest this?

https://gcc.gnu.org/onlinedocs/gcc/Gcov-and-Optimization.html

UPD. The code has been taken from flux-framework/flux-core@a9bf417.

ajtowns
ajtowns reviewed May 10, 2024
View reviewed changes
Copy link
Contributor

@ajtowns ajtowns left a comment

Choose a reason for hiding this comment

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

Approach ACK

src/util/subprocess.h Outdated
@@ -1366,6 +1375,11 @@ namespace detail {
util::write_n(err_wr_pipe_, err_msg.c_str(), err_msg.length());
}

#ifdef CODE_COVERAGE_ENABLED
__gcov_dump();
__gcov_reset();
Copy link
Contributor

@ajtowns ajtowns May 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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()]

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, reset seem unneeded here. I'm going to drop it.

src/util/subprocess.h
@@ -1353,6 +1357,11 @@ namespace detail {
if (stream.err_write_ != -1 && stream.err_write_ > 2)
close(stream.err_write_);

#ifdef CODE_COVERAGE_ENABLED
__gcov_dump();
__gcov_reset();
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

@hebasto hebasto force-pushed the 240509-coverage branch from e3e3144 to cc49aed Compare May 10, 2024 13:27
@hebasto
Copy link
Member Author

hebasto commented May 10, 2024

Addressed @ajtowns's feedback. Added comments. Updated the PR description.

@DrahtBot DrahtBot mentioned this pull request Aug 16, 2024
Merged
@hebasto hebasto mentioned this pull request Aug 28, 2024
Closed
@DrahtBot DrahtBot mentioned this pull request Aug 30, 2024
Closed
@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 2, 2024

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

@hebasto hebasto marked this pull request as draft October 11, 2024 19:06
@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 8, 2025

⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?

  • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
  • Is it no longer relevant? ➡️ Please close.
  • Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

2 similar comments
@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 7, 2025

⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?

  • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
  • Is it no longer relevant? ➡️ Please close.
  • Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 5, 2025

⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?

  • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
  • Is it no longer relevant? ➡️ Please close.
  • Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

@maflcko
Copy link
Member

maflcko commented Jul 7, 2025

@hebasto are you still working on this?

@hebasto
Copy link
Member Author

hebasto commented Jul 10, 2025

@hebasto are you still working on this?

No, not at the moment.

@hebasto hebasto closed this Jul 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@ajtowns ajtowns ajtowns left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Needs rebase Tests Up for grabs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hebasto @DrahtBot @theuni @maflcko @ajtowns