Skip to content

Commit

Permalink
Rollup merge of #111179 - Zalathar:sort-groups, r=Mark-Simulacrum
Browse files Browse the repository at this point in the history
Fix instrument-coverage tests by using Python to sort instantiation groups

#110942 was intended to fix a set of `-Cinstrument-coverage` tests, but it ended up silently *breaking* those tests on Linux, for annoying reasons detailed at #111171.

Dealing with `diff --ignore-matching-lines` across multiple platforms has been such a hassle that I've instead written a simple Python script that can detect instantiation groups in the output of `llvm-cov show`, and sort them in a predictable order so that they can be used as snapshots for an ordinary invocation of `diff`.

This approach should be much less error-prone, because it can't accidentally ignore the wrong lines, and any unforeseen problems will tend to result in a Python exception or a failing diff.
  • Loading branch information
matthiaskrgr authored May 12, 2023
2 parents 26e0c57 + 27a3ce2 commit ea332b5
Show file tree
Hide file tree
Showing 7 changed files with 148 additions and 33 deletions.
25 changes: 9 additions & 16 deletions tests/run-make/coverage-reports/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ endif
) \
2> "$(TMPDIR)"/show_coverage_stderr.$@.txt \
| "$(PYTHON)" $(BASEDIR)/normalize_paths.py \
| "$(PYTHON)" $(BASEDIR)/sort_subviews.py \
> "$(TMPDIR)"/actual_show_coverage.$@.txt || \
( status=$$? ; \
>&2 cat "$(TMPDIR)"/show_coverage_stderr.$@.txt ; \
Expand All @@ -158,23 +159,15 @@ ifdef RUSTC_BLESS_TEST
else
# Compare the show coverage output (`--bless` refreshes `typical` files).
#
# FIXME(richkadel): None of the Rust test source samples have the
# `// ignore-llvm-cov-show-diffs` anymore. This directive exists to work around a limitation
# with `llvm-cov show`. When reporting coverage for multiple instantiations of a generic function,
# with different type substitutions, `llvm-cov show` prints these in a non-deterministic order,
# breaking the `diff` comparison.
# `llvm-cov show` normally prints instantiation groups in an unpredictable
# order, but we have used `sort_subviews.py` to sort them, so we can still
# check the output directly with `diff`.
#
# A partial workaround is implemented below, with `diff --ignore-matching-lines=RE`
# to ignore each line prefixing each generic instantiation coverage code region.
#
# This workaround only works if the coverage counts are identical across all reported
# instantiations. If there is no way to ensure this, you may need to apply the
# `// ignore-llvm-cov-show-diffs` directive, and check for differences using the
# `.json` files to validate that results have not changed. (Until then, the JSON
# files are redundant, so there is no need to generate `expected_*.json` files or
# compare actual JSON results.)

$(DIFF) --ignore-matching-lines='^ \| .*::<.*>.*:$$' --ignore-matching-lines='^ \| <.*>::.*:$$' \
# Some of the test cases are currently not working (since #110393) and have
# been marked with `// ignore-llvm-cov-show-diffs` so that they don't fail
# the build.

$(DIFF) \
expected_show_coverage.$@.txt "$(TMPDIR)"/actual_show_coverage.$@.txt || \
( grep -q '^\/\/ ignore-llvm-cov-show-diffs' $(SOURCEDIR)/$@.rs && \
>&2 echo 'diff failed, but suppressed with `// ignore-llvm-cov-show-diffs` in $(SOURCEDIR)/$@.rs' \
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,9 @@
41| 1| // executed asynchronously.
42| 1| match x {
43| 1| y if c(x).await == y + 1 => { d().await; }
^0 ^0 ^0 ^0
^0 ^0 ^0 ^0
44| 1| y if f().await == y + 1 => (),
^0 ^0 ^0
^0 ^0 ^0
45| 1| _ => (),
46| | }
47| 1|}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
1| |// compile-flags: --edition=2021
2| |
3| |// Demonstrate that `sort_subviews.py` can sort instantiation groups into a
4| |// predictable order, while preserving their heterogeneous contents.
5| |
6| 1|fn main() {
7| 1| let cond = std::env::args().len() > 1;
8| 1| generic_fn::<()>(cond);
9| 1| generic_fn::<&'static str>(!cond);
10| 1| if false {
11| 0| generic_fn::<char>(cond);
12| 1| }
13| 1| generic_fn::<i32>(cond);
14| 1| other_fn();
15| 1|}
16| |
17| 3|fn generic_fn<T>(cond: bool) {
18| 3| if cond {
19| 1| println!("{}", std::any::type_name::<T>());
20| 2| }
21| 3|}
------------------
| Unexecuted instantiation: sort_groups::generic_fn::<char>
------------------
| sort_groups::generic_fn::<&str>:
| 17| 1|fn generic_fn<T>(cond: bool) {
| 18| 1| if cond {
| 19| 1| println!("{}", std::any::type_name::<T>());
| 20| 1| }
| ^0
| 21| 1|}
------------------
| sort_groups::generic_fn::<()>:
| 17| 1|fn generic_fn<T>(cond: bool) {
| 18| 1| if cond {
| 19| 0| println!("{}", std::any::type_name::<T>());
| 20| 1| }
| 21| 1|}
------------------
| sort_groups::generic_fn::<i32>:
| 17| 1|fn generic_fn<T>(cond: bool) {
| 18| 1| if cond {
| 19| 0| println!("{}", std::any::type_name::<T>());
| 20| 1| }
| 21| 1|}
------------------
22| |
23| 1|fn other_fn() {}

Original file line number Diff line number Diff line change
Expand Up @@ -19,29 +19,29 @@
18| 2| println!("used_only_from_bin_crate_generic_function with {:?}", arg);
19| 2|}
------------------
| used_crate::used_only_from_bin_crate_generic_function::<&str>:
| Unexecuted instantiation: used_crate::used_only_from_bin_crate_generic_function::<_>
------------------
| used_crate::used_only_from_bin_crate_generic_function::<&alloc::vec::Vec<i32>>:
| 17| 1|pub fn used_only_from_bin_crate_generic_function<T: Debug>(arg: T) {
| 18| 1| println!("used_only_from_bin_crate_generic_function with {:?}", arg);
| 19| 1|}
------------------
| used_crate::used_only_from_bin_crate_generic_function::<&alloc::vec::Vec<i32>>:
| used_crate::used_only_from_bin_crate_generic_function::<&str>:
| 17| 1|pub fn used_only_from_bin_crate_generic_function<T: Debug>(arg: T) {
| 18| 1| println!("used_only_from_bin_crate_generic_function with {:?}", arg);
| 19| 1|}
------------------
| Unexecuted instantiation: used_crate::used_only_from_bin_crate_generic_function::<_>
------------------
20| |// Expect for above function: `Unexecuted instantiation` (see below)
21| 2|pub fn used_only_from_this_lib_crate_generic_function<T: Debug>(arg: T) {
22| 2| println!("used_only_from_this_lib_crate_generic_function with {:?}", arg);
23| 2|}
------------------
| used_crate::used_only_from_this_lib_crate_generic_function::<alloc::vec::Vec<i32>>:
| used_crate::used_only_from_this_lib_crate_generic_function::<&str>:
| 21| 1|pub fn used_only_from_this_lib_crate_generic_function<T: Debug>(arg: T) {
| 22| 1| println!("used_only_from_this_lib_crate_generic_function with {:?}", arg);
| 23| 1|}
------------------
| used_crate::used_only_from_this_lib_crate_generic_function::<&str>:
| used_crate::used_only_from_this_lib_crate_generic_function::<alloc::vec::Vec<i32>>:
| 21| 1|pub fn used_only_from_this_lib_crate_generic_function<T: Debug>(arg: T) {
| 22| 1| println!("used_only_from_this_lib_crate_generic_function with {:?}", arg);
| 23| 1|}
Expand All @@ -51,12 +51,12 @@
26| 2| println!("used_from_bin_crate_and_lib_crate_generic_function with {:?}", arg);
27| 2|}
------------------
| used_crate::used_from_bin_crate_and_lib_crate_generic_function::<alloc::vec::Vec<i32>>:
| used_crate::used_from_bin_crate_and_lib_crate_generic_function::<&str>:
| 25| 1|pub fn used_from_bin_crate_and_lib_crate_generic_function<T: Debug>(arg: T) {
| 26| 1| println!("used_from_bin_crate_and_lib_crate_generic_function with {:?}", arg);
| 27| 1|}
------------------
| used_crate::used_from_bin_crate_and_lib_crate_generic_function::<&str>:
| used_crate::used_from_bin_crate_and_lib_crate_generic_function::<alloc::vec::Vec<i32>>:
| 25| 1|pub fn used_from_bin_crate_and_lib_crate_generic_function<T: Debug>(arg: T) {
| 26| 1| println!("used_from_bin_crate_and_lib_crate_generic_function with {:?}", arg);
| 27| 1|}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@
40| 2| println!("used_only_from_bin_crate_generic_function with {:?}", arg);
41| 2|}
------------------
| Unexecuted instantiation: used_inline_crate::used_only_from_bin_crate_generic_function::<_>
------------------
| used_inline_crate::used_only_from_bin_crate_generic_function::<&alloc::vec::Vec<i32>>:
| 39| 1|pub fn used_only_from_bin_crate_generic_function<T: Debug>(arg: T) {
| 40| 1| println!("used_only_from_bin_crate_generic_function with {:?}", arg);
Expand All @@ -51,8 +53,6 @@
| 39| 1|pub fn used_only_from_bin_crate_generic_function<T: Debug>(arg: T) {
| 40| 1| println!("used_only_from_bin_crate_generic_function with {:?}", arg);
| 41| 1|}
------------------
| Unexecuted instantiation: used_inline_crate::used_only_from_bin_crate_generic_function::<_>
------------------
42| |// Expect for above function: `Unexecuted instantiation` (see notes in `used_crate.rs`)
43| |
Expand All @@ -77,15 +77,15 @@
51| 3| println!("used_from_bin_crate_and_lib_crate_generic_function with {:?}", arg);
52| 3|}
------------------
| used_inline_crate::used_from_bin_crate_and_lib_crate_generic_function::<alloc::vec::Vec<i32>>:
| 50| 1|pub fn used_from_bin_crate_and_lib_crate_generic_function<T: Debug>(arg: T) {
| 51| 1| println!("used_from_bin_crate_and_lib_crate_generic_function with {:?}", arg);
| 52| 1|}
------------------
| used_inline_crate::used_from_bin_crate_and_lib_crate_generic_function::<&str>:
| 50| 2|pub fn used_from_bin_crate_and_lib_crate_generic_function<T: Debug>(arg: T) {
| 51| 2| println!("used_from_bin_crate_and_lib_crate_generic_function with {:?}", arg);
| 52| 2|}
------------------
| used_inline_crate::used_from_bin_crate_and_lib_crate_generic_function::<alloc::vec::Vec<i32>>:
| 50| 1|pub fn used_from_bin_crate_and_lib_crate_generic_function<T: Debug>(arg: T) {
| 51| 1| println!("used_from_bin_crate_and_lib_crate_generic_function with {:?}", arg);
| 52| 1|}
------------------
53| |
54| |#[inline(always)]
Expand Down
50 changes: 50 additions & 0 deletions tests/run-make/coverage-reports/sort_subviews.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
#!/usr/bin/env python3

# `llvm-cov show` prints grouped subviews (e.g. for generic functions) in an
# unstable order, which is inconvenient when checking output snapshots with
# `diff`. To work around that, this script detects consecutive subviews in its
# piped input, and sorts them while preserving their contents.

from __future__ import print_function

import sys


def main():
subviews = []

def flush_subviews():
if not subviews:
return

# The last "subview" should be just a boundary line on its own, so
# temporarily remove it before sorting the accumulated subviews.
terminator = subviews.pop()
subviews.sort()
subviews.append(terminator)

for view in subviews:
for line in view:
print(line, end="")

subviews.clear()

for line in sys.stdin:
if line.startswith(" ------------------"):
# This is a subview boundary line, so start a new subview.
subviews.append([line])
elif line.startswith(" |"):
# Add this line to the current subview.
subviews[-1].append(line)
else:
# This line is not part of a subview, so sort and print any
# accumulated subviews, and then print the line as-is.
flush_subviews()
print(line, end="")

flush_subviews()
assert not subviews


if __name__ == "__main__":
main()
23 changes: 23 additions & 0 deletions tests/run-make/coverage/sort_groups.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
// compile-flags: --edition=2021

// Demonstrate that `sort_subviews.py` can sort instantiation groups into a
// predictable order, while preserving their heterogeneous contents.

fn main() {
let cond = std::env::args().len() > 1;
generic_fn::<()>(cond);
generic_fn::<&'static str>(!cond);
if false {
generic_fn::<char>(cond);
}
generic_fn::<i32>(cond);
other_fn();
}

fn generic_fn<T>(cond: bool) {
if cond {
println!("{}", std::any::type_name::<T>());
}
}

fn other_fn() {}

0 comments on commit ea332b5

Please sign in to comment.