Skip to content
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

Avoid possible filename collision in coverage tests #85283

Merged
merged 1 commit into from
Jun 16, 2021
Merged

Avoid possible filename collision in coverage tests #85283

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions src/test/run-make-fulldeps/coverage-reports/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ endif
# Run it in order to generate some profiling data,
# with `LLVM_PROFILE_FILE=<profdata_file>` environment variable set to
# output the coverage stats for this run.
LLVM_PROFILE_FILE="$(TMPDIR)"/$@-%p.profraw \
LLVM_PROFILE_FILE="$(TMPDIR)"/$@.profraw \
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 I support the change on this line (removing pid from the non-doc-test run), even though it shouldn't make a difference in the result.

$(call RUN,$@) || \
( \
status=$$?; \
Expand All @@ -97,16 +97,19 @@ endif
) \
)

# Run it through rustdoc as well to cover doctests
LLVM_PROFILE_FILE="$(TMPDIR)"/$@-%p.profraw \
# Run it through rustdoc as well to cover doctests.
# `%p` is the pid, and `%m` the binary signature. We suspect that the pid alone
# might result in overwritten files and failed tests, as rustdoc spawns each
# doctest as its own process, so make sure the filename is as unique as possible.
LLVM_PROFILE_FILE="$(TMPDIR)"/$@-%p-%m.profraw \
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will help, assuming our theory is correct that there could be PID-reuse on Windows. As I understand it, every doc test produces a new, program (new main() and unique source) so the binary signature (%m) should be different for each one.

In theory, adding the PID %p value is probably superfluous, because %m should be unique enough, but maybe keeping %p is an added safeguard to ensuring we don't have filename collisions, and there's probably harm keeping both.

$(RUSTDOC) --crate-name workaround_for_79771 --test $(SOURCEDIR)/$@.rs \
$$( sed -n 's/^\/\/ compile-flags: \([^#]*\).*/\1/p' $(SOURCEDIR)/$@.rs ) \
-L "$(TMPDIR)" -Zinstrument-coverage \
-Z unstable-options --persist-doctests=$(TMPDIR)/rustdoc-$@

# Postprocess the profiling data so it can be used by the llvm-cov tool
"$(LLVM_BIN_DIR)"/llvm-profdata merge --sparse \
"$(TMPDIR)"/$@-*.profraw \
"$(TMPDIR)"/$@*.profraw \
-o "$(TMPDIR)"/$@.profdata

# Generate a coverage report using `llvm-cov show`.
Expand All @@ -118,8 +121,7 @@ endif
--instr-profile="$(TMPDIR)"/$@.profdata \
$(call BIN,"$(TMPDIR)"/$@) \
$$( \
for file in $(TMPDIR)/rustdoc-$@/*/rust_out; \
do \
for file in $(TMPDIR)/rustdoc-$@/*/rust_out; do \
[ -x "$$file" ] && printf "%s %s " -object $$file; \
done \
) \
Expand Down