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.
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
test, subprocess: Improve coverage report correctness #30075
base: master
Are you sure you want to change the base?
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
Are there some docs somewhere that suggest this? |
https://gcc.gnu.org/onlinedocs/gcc/Gcov-and-Optimization.html
|
There was a problem hiding this 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(); |
There was a problem hiding this comment.
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()
]
There was a problem hiding this comment.
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.
@@ -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(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Addressed @ajtowns's feedback. Added comments. Updated the PR description. |
As a child process uses the
execvp()
call, an explicit dumping of the collected profile information is required.Coverage:
on the master branch:
with this PR:
This PR mostly follows gcc/libgcc/libgcov-interface.c#L320-L332: