Page MenuHomePhabricator

[llvm-cov] Reset ExecutionCount only if first line segment is start of a new region.
AbandonedPublic

Authored by zequanwu on Feb 3 2021, 5:00 PM.

Details

Reviewers
vsk
pirama
rnk
Summary

To fix the bug, introduced by D85036, llvm-cov shows the following:

1|       |#include <stdlib.h>
2|       |
3|      1|int main() {
4|      1|  return getenv(
5|      0|      "TEST") ? 1
6|      1|              : 0;
7|      1|}

Diff Detail

Unit TestsFailed

TimeTest
260 msx64 debian > libarcher.races::task-dependency.c
Script: -- : 'RUN: at line 13'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -Wl,-rpath,/mnt/disks/ssd0/agent/llvm-project/build/lib /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/task-dependency.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-dependency.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/deflake.bash /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-dependency.c.tmp 2>&1 | tee /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-dependency.c.tmp.log | /mnt/disks/ssd0/agent/llvm-project/build/./bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/task-dependency.c
280 msx64 debian > libarcher.races::task-taskgroup-unrelated.c
Script: -- : 'RUN: at line 13'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -Wl,-rpath,/mnt/disks/ssd0/agent/llvm-project/build/lib /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/task-taskgroup-unrelated.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-taskgroup-unrelated.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/deflake.bash /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-taskgroup-unrelated.c.tmp 2>&1 | tee /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-taskgroup-unrelated.c.tmp.log | /mnt/disks/ssd0/agent/llvm-project/build/./bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/task-taskgroup-unrelated.c
260 msx64 debian > libarcher.races::task-taskwait-nested.c
Script: -- : 'RUN: at line 13'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -Wl,-rpath,/mnt/disks/ssd0/agent/llvm-project/build/lib /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/task-taskwait-nested.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-taskwait-nested.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/deflake.bash /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-taskwait-nested.c.tmp 2>&1 | tee /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-taskwait-nested.c.tmp.log | /mnt/disks/ssd0/agent/llvm-project/build/./bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/task-taskwait-nested.c
260 msx64 debian > libarcher.races::task-two.c
Script: -- : 'RUN: at line 13'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -Wl,-rpath,/mnt/disks/ssd0/agent/llvm-project/build/lib /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/task-two.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-two.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/deflake.bash /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-two.c.tmp 2>&1 | tee /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-two.c.tmp.log | /mnt/disks/ssd0/agent/llvm-project/build/./bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/task-two.c
270 msx64 debian > libarcher.task::task-barrier.c
Script: -- : 'RUN: at line 15'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -Wl,-rpath,/mnt/disks/ssd0/agent/llvm-project/build/lib /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/task/task-barrier.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/task/Output/task-barrier.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/task/Output/task-barrier.c.tmp 2>&1 | tee /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/task/Output/task-barrier.c.tmp.log | /mnt/disks/ssd0/agent/llvm-project/build/./bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/task/task-barrier.c
View Full Test Results (13 Failed)

Event Timeline

zequanwu created this revision.Feb 3 2021, 5:00 PM
zequanwu requested review of this revision.Feb 3 2021, 5:00 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 3 2021, 5:00 PM
Herald added subscribers: llvm-commits, Restricted Project. · View Herald Transcript

Thanks for the patch Zequan. This fixes the C++ test case that prompted this issue.

The underlying Rust issue is not fixed though:

 1|       |fn notatest() -> Result<(), std::env::VarError> {
 2|      1|        let result = std::env::var("MEOW")
 3|      0|                    .or(Ok("a".to_string()))?;
 4|      1|            println!("{}", result);
 5|      1|                Ok(())
 6|      1|}
 7|       | 
 8|      1|fn main() {
 9|      1|        println!("Hello, world! {:?}", notatest());
10|      1|}

Here're the regions from -debug-only=coverage-mapping:

Segment at 2:13 (count = 1), RegionEntry
Segment at 2:19 (count = 0), Skipped                                                          
Segment at 2:22 (count = 1), RegionEntry                                                      
Segment at 3:45 (count = 0), RegionEntry
Segment at 3:46 (count = 0), Skipped
Segment at 4:13 (count = 1), RegionEntry
Segment at 5:23 (count = 0), Skipped
Segment at 6:1 (count = 1), RegionEntry
Segment at 6:2 (count = 0), Skipped
Segment at 8:11 (count = 1), RegionEntry
Segment at 10:2 (count = 0), Skipped

The ternary operator case in the C++ test case has a gap region corresponding to the ?. For the Rust ? operator, there is no similar gap region. In fact, the rust frontend associates the counter corresponding to the error path of the ? operator to the ? token . Is there a better sequence of Segments that the Rust frontend can emit to handle this case?

Thanks for the patch Zequan. This fixes the C++ test case that prompted this issue.

The underlying Rust issue is not fixed though:

 1|       |fn notatest() -> Result<(), std::env::VarError> {
 2|      1|        let result = std::env::var("MEOW")
 3|      0|                    .or(Ok("a".to_string()))?;
 4|      1|            println!("{}", result);
 5|      1|                Ok(())
 6|      1|}
 7|       | 
 8|      1|fn main() {
 9|      1|        println!("Hello, world! {:?}", notatest());
10|      1|}

Here're the regions from -debug-only=coverage-mapping:

Segment at 2:13 (count = 1), RegionEntry
Segment at 2:19 (count = 0), Skipped                                                          
Segment at 2:22 (count = 1), RegionEntry                                                      
Segment at 3:45 (count = 0), RegionEntry
Segment at 3:46 (count = 0), Skipped
Segment at 4:13 (count = 1), RegionEntry
Segment at 5:23 (count = 0), Skipped
Segment at 6:1 (count = 1), RegionEntry
Segment at 6:2 (count = 0), Skipped
Segment at 8:11 (count = 1), RegionEntry
Segment at 10:2 (count = 0), Skipped

The ternary operator case in the C++ test case has a gap region corresponding to the ?. For the Rust ? operator, there is no similar gap region. In fact, the rust frontend associates the counter corresponding to the error path of the ? operator to the ? token . Is there a better sequence of Segments that the Rust frontend can emit to handle this case?

Sorry, I don't know about Rust. I just read the ? operator doc on https://doc.rust-lang.org/edition-guide/rust-2018/error-handling-and-panics/the-question-mark-operator-for-easier-error-handling.html.

For the example you provided, I am not sure if it's a bug or not. Segment at 3:45 (count = 0), RegionEntry corresponds to the error path of the ? operator, which means error path is not executed. So, the 0 count looks reasonable. If we mark line 3 as executed regardless of which path is chosen, we may lost the information of path choices.

For the example you provided, I am not sure if it's a bug or not. Segment at 3:45 (count = 0), RegionEntry corresponds to the error path of the ? operator, which means error path is not executed. So, the 0 count looks reasonable. If we mark line 3 as executed regardless of which path is chosen, we may lost the information of path choices.

I just realized that this also applies to C++ ? operator. In the following case, if we always mark line 5 as executed, we don't know which branch is chosen (But when generating html report by llvm-cov, we will see that the not executed branch is marked with red color).

1|       |#include <stdlib.h>
2|       |
3|      1|int main() {
4|      1|  return getenv(
5|      0|      "TEST") ? 1
6|      1|              : 0;
7|      1|}

For the example you provided, I am not sure if it's a bug or not. Segment at 3:45 (count = 0), RegionEntry corresponds to the error path of the ? operator, which means error path is not executed. So, the 0 count looks reasonable. If we mark line 3 as executed regardless of which path is chosen, we may lost the information of path choices.

I just realized that this also applies to C++ ? operator. In the following case, if we always mark line 5 as executed, we don't know which branch is chosen (But when generating html report by llvm-cov, we will see that the not executed branch is marked with red color).

I agree and think this patch may be unnecessary. The region coverage information in the html reports are accurate than line coverage and we have to use some heuristics here when computing line coverage.

1|       |#include <stdlib.h>
2|       |
3|      1|int main() {
4|      1|  return getenv(
5|      0|      "TEST") ? 1
6|      1|              : 0;
7|      1|}
zequanwu abandoned this revision.Feb 5 2021, 2:13 PM

Abandoned since this is not a bug.