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

Fix parsing of BRDA lines in LcovParser #11899

Closed
wants to merge 3 commits into from
Closed

Fix parsing of BRDA lines in LcovParser #11899

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2846352
Fix parsing of BRDA lines in LcovParser
c-mita Aug 5, 2020
adfa7df
Fix broken test case
c-mita Aug 6, 2020
2c8fcf9
Rename BRDA '-' constant from NOT_TAKEN to NEVER_EVALUATED
c-mita Aug 6, 2020
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
13 changes: 8 additions & 5 deletions ...erageOutputGenerator/java/com/google/devtools/coverageoutputgenerator/BranchCoverage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,12 @@ abstract class BranchCoverage {

static BranchCoverage create(int lineNumber, long nrOfExecutions) {
return new AutoValue_BranchCoverage(
lineNumber, /*blockNumber=*/ "", /*branchNumber=*/ "", nrOfExecutions);
lineNumber, /*blockNumber=*/ "", /*branchNumber=*/ "", nrOfExecutions > 0, nrOfExecutions);
}

static BranchCoverage createWithBlockAndBranch(
int lineNumber, String blockNumber, String branchNumber, long nrOfExecutions) {
return new AutoValue_BranchCoverage(lineNumber, blockNumber, branchNumber, nrOfExecutions);
int lineNumber, String blockNumber, String branchNumber, boolean taken, long nrOfExecutions) {
return new AutoValue_BranchCoverage(lineNumber, blockNumber, branchNumber, taken, nrOfExecutions);
}

/**
Expand All @@ -45,14 +45,17 @@ static BranchCoverage merge(BranchCoverage first, BranchCoverage second) {
first.lineNumber(),
first.blockNumber(),
first.branchNumber(),
first.taken() || second.taken(),
first.nrOfExecutions() + second.nrOfExecutions());
}

abstract int lineNumber();
// The two numbers below should be -1 for non-gcc emitted coverage (e.g. Java).
abstract String blockNumber(); // internal gcc internal ID for the branch
abstract String blockNumber(); // internal gcc ID for the branch

abstract String branchNumber(); // internal gcc internal ID for the branch
abstract String branchNumber(); // internal gcc ID for the branch

abstract boolean taken();

abstract long nrOfExecutions();

Expand Down
2 changes: 1 addition & 1 deletion ...t/CoverageOutputGenerator/java/com/google/devtools/coverageoutputgenerator/Constants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class Constants {
static final String LH_MARKER = "LH:";
static final String LF_MARKER = "LF:";
static final String END_OF_RECORD_MARKER = "end_of_record";
static final String TAKEN = "-";
static final String NOT_TAKEN = "-";
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe this is better as "NEVER_EXECUTED", since it means the branches in a block were never executed, not just taken.

There's a bit of inconsistency with the usages of the words "taken" and "executed" when compared to the docs here: http://ltp.sourceforge.net/coverage/lcov/geninfo.1.php

  BRDA:<line number>,<block number>,<branch number>,<taken>`

       Block  number  and  branch  number are gcc internal IDs for the branch.
       Taken is either '-' if the basic block containing the branch was  never
       executed or a number indicating how often that branch was taken.

We seem to have reversed the words "taken" and "execution"...

static final String TRACEFILE_EXTENSION = ".dat";
static final String GCOV_EXTENSION = ".gcov";
static final String GCOV_JSON_EXTENSION = ".gcov.json.gz";
Expand Down
8 changes: 5 additions & 3 deletions .../CoverageOutputGenerator/java/com/google/devtools/coverageoutputgenerator/LcovParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
import static com.google.devtools.coverageoutputgenerator.Constants.LF_MARKER;
import static com.google.devtools.coverageoutputgenerator.Constants.LH_MARKER;
import static com.google.devtools.coverageoutputgenerator.Constants.SF_MARKER;
import static com.google.devtools.coverageoutputgenerator.Constants.TAKEN;
import static com.google.devtools.coverageoutputgenerator.Constants.NOT_TAKEN;
import static java.nio.charset.StandardCharsets.UTF_8;

import java.io.BufferedReader;
Expand Down Expand Up @@ -279,12 +279,14 @@ private boolean parseBRDALine(String line) {
String taken = lineData[3];

long executionCount = 0;
if (taken.equals(TAKEN)) {
boolean wasExecuted = false;
if (!taken.equals(NOT_TAKEN)) {
executionCount = Long.parseLong(taken);
wasExecuted = true;
}
BranchCoverage branchCoverage =
BranchCoverage.createWithBlockAndBranch(
lineNumber, blockNumber, branchNumber, executionCount);
lineNumber, blockNumber, branchNumber, wasExecuted, executionCount);

currentSourceFileCoverage.addBranch(lineNumber, branchCoverage);
} catch (NumberFormatException e) {
Expand Down
63 changes: 34 additions & 29 deletions ...CoverageOutputGenerator/java/com/google/devtools/coverageoutputgenerator/LcovPrinter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.io.OutputStream;
import java.io.OutputStreamWriter;
import java.io.Writer;
import java.util.List;
import java.util.Map.Entry;
import java.util.logging.Level;
import java.util.logging.Logger;
Expand Down Expand Up @@ -131,42 +132,46 @@ private void printFNHLine(SourceFileCoverage sourceFile) throws IOException {

// BRDA:<line number>,<block number>,<branch number>,<taken>
private void printBRDALines(SourceFileCoverage sourceFile) throws IOException {
for (BranchCoverage branch : sourceFile.getAllBranches()) {
if (branch.blockNumber().isEmpty() || branch.branchNumber().isEmpty()) {
// We skip printing this as a BRDA line and print it later as a BA line.
continue;
}
bufferedWriter.write(Constants.BRDA_MARKER);
bufferedWriter.write(Integer.toString(branch.lineNumber()));
bufferedWriter.write(Constants.DELIMITER);
bufferedWriter.write(branch.blockNumber());
bufferedWriter.write(Constants.DELIMITER);
bufferedWriter.write(branch.branchNumber());
bufferedWriter.write(Constants.DELIMITER);
if (branch.wasExecuted()) {
bufferedWriter.write(Long.toString(branch.nrOfExecutions()));
} else {
bufferedWriter.write(Constants.TAKEN);
for (List<BranchCoverage> branches : sourceFile.getAllBranches()) {
for (BranchCoverage branch : branches) {
if (branch.blockNumber().isEmpty() || branch.branchNumber().isEmpty()) {
// We skip printing this as a BRDA line and print it later as a BA line.
continue;
}
bufferedWriter.write(Constants.BRDA_MARKER);
bufferedWriter.write(Integer.toString(branch.lineNumber()));
bufferedWriter.write(Constants.DELIMITER);
bufferedWriter.write(branch.blockNumber());
bufferedWriter.write(Constants.DELIMITER);
bufferedWriter.write(branch.branchNumber());
bufferedWriter.write(Constants.DELIMITER);
if (branch.taken()) {
bufferedWriter.write(Long.toString(branch.nrOfExecutions()));
} else {
bufferedWriter.write(Constants.NOT_TAKEN);
}
bufferedWriter.newLine();
}
bufferedWriter.newLine();
}
}

// BA:<line number>,<taken>
private void printBALines(SourceFileCoverage sourceFile) throws IOException {
for (BranchCoverage branch : sourceFile.getAllBranches()) {
if (!branch.blockNumber().isEmpty() && !branch.branchNumber().isEmpty()) {
// This branch was already printed with more information as a BRDA line.
continue;
for (List<BranchCoverage> branches : sourceFile.getAllBranches()) {
for (BranchCoverage branch : branches) {
if (!branch.blockNumber().isEmpty() && !branch.branchNumber().isEmpty()) {
// This branch was already printed with more information as a BRDA line.
continue;
}
bufferedWriter.write(Constants.BA_MARKER);
bufferedWriter.write(Integer.toString(branch.lineNumber()));
bufferedWriter.write(Constants.DELIMITER);
// 0 = branch was not executed
// 1 = branch was executed but not taken
// 2 = branch was executed and taken
bufferedWriter.write(branch.wasExecuted() ? "2" : "0");
bufferedWriter.newLine();
}
bufferedWriter.write(Constants.BA_MARKER);
bufferedWriter.write(Integer.toString(branch.lineNumber()));
bufferedWriter.write(Constants.DELIMITER);
// 0 = branch was not executed
// 1 = branch was executed but not taken
// 2 = branch was executed and taken
bufferedWriter.write(branch.wasExecuted() ? "2" : "0");
bufferedWriter.newLine();
}
}

Expand Down
46 changes: 28 additions & 18 deletions ...eOutputGenerator/java/com/google/devtools/coverageoutputgenerator/SourceFileCoverage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,14 @@
package com.google.devtools.coverageoutputgenerator;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.MoreCollectors;

import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Optional;
import java.util.Set;
import java.util.TreeMap;
import java.util.stream.Collectors;
Expand All @@ -31,7 +36,7 @@ class SourceFileCoverage {
private String sourceFileName;
private final TreeMap<String, Integer> lineNumbers; // function name to line numbers
private final TreeMap<String, Long> functionsExecution; // function name to execution count
private final TreeMap<Integer, BranchCoverage> branches; // line number to branch
private final TreeMap<Integer, List<BranchCoverage>> branches; // line number to branch
private final TreeMap<Integer, LineCoverage> lines; // line number to line execution

SourceFileCoverage(String sourcefile) {
Expand Down Expand Up @@ -89,21 +94,17 @@ static TreeMap<String, Long> mergeFunctionsExecution(
* Returns the merged branches found in the two given {@code SourceFileCoverage}s.
*/
@VisibleForTesting
static TreeMap<Integer, BranchCoverage> mergeBranches(
static TreeMap<Integer, List<BranchCoverage>> mergeBranches(
SourceFileCoverage s1, SourceFileCoverage s2) {
return Stream.of(s1.branches, s2.branches)
.map(Map::entrySet)
.flatMap(Collection::stream)
.collect(
Collectors.toMap(
Map.Entry::getKey, Map.Entry::getValue, BranchCoverage::merge, TreeMap::new));
TreeMap<Integer, List<BranchCoverage>> merged = new TreeMap<>();
merged.putAll(s1.branches);
s2.branches.entrySet().stream().forEach(
e -> e.getValue().forEach(b -> addBranchToMap(e.getKey(), b, merged)));
return merged;
}

static int getNumberOfBranchesHit(SourceFileCoverage sourceFileCoverage) {
return (int)
sourceFileCoverage.branches.entrySet().stream()
.filter(branch -> branch.getValue().wasExecuted())
.count();
return (int) sourceFileCoverage.branches.values().stream().flatMap(Collection::stream).filter(e -> e.wasExecuted()).count();
}

/*
Expand Down Expand Up @@ -197,7 +198,7 @@ Set<Entry<String, Long>> getAllExecutionCount() {
return functionsExecution.entrySet();
}

Collection<BranchCoverage> getAllBranches() {
Collection<List<BranchCoverage>> getAllBranches() {
return branches.values();
}

Expand All @@ -223,14 +224,23 @@ void addAllFunctionsExecution(TreeMap<String, Long> functionsExecution) {
}

void addBranch(Integer lineNumber, BranchCoverage branch) {
if (this.branches.get(lineNumber) != null) {
this.branches.put(lineNumber, BranchCoverage.merge(this.branches.get(lineNumber), branch));
return;
addBranchToMap(lineNumber, branch, branches);
}

static void addBranchToMap(Integer lineNumber, BranchCoverage branch, TreeMap<Integer, List<BranchCoverage>> target) {
List<BranchCoverage> lineBranches = target.computeIfAbsent(lineNumber, ArrayList::new);
Optional<BranchCoverage> match =
lineBranches.stream().filter(b -> b.blockNumber().equals(branch.blockNumber()) && b.branchNumber().equals(branch.branchNumber())).collect(MoreCollectors.toOptional());
if (match.isPresent()) {
BranchCoverage b = match.get();
lineBranches.remove(b);
lineBranches.add(BranchCoverage.merge(b, branch));
} else {
lineBranches.add(branch);
}
this.branches.put(lineNumber, branch);
}

void addAllBranches(TreeMap<Integer, BranchCoverage> branches) {
void addAllBranches(TreeMap<Integer, List<BranchCoverage>> branches) {
this.branches.putAll(branches);
}

Expand Down
2 changes: 2 additions & 0 deletions .../test/CoverageOutputGenerator/javatests/com/google/devtools/coverageoutputgenerator/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ java_test(
"//third_party:guava",
"//third_party:junit4",
"//third_party:truth",
"//tools/test/CoverageOutputGenerator/java/com/google/devtools/coverageoutputgenerator:BranchCoverage",
"//tools/test/CoverageOutputGenerator/java/com/google/devtools/coverageoutputgenerator:Constants",
"//tools/test/CoverageOutputGenerator/java/com/google/devtools/coverageoutputgenerator:Coverage",
"//tools/test/CoverageOutputGenerator/java/com/google/devtools/coverageoutputgenerator:LcovPrinter",
Expand All @@ -67,6 +68,7 @@ java_test(
"//third_party:guava",
"//third_party:junit4",
"//third_party:truth",
"//tools/test/CoverageOutputGenerator/java/com/google/devtools/coverageoutputgenerator:BranchCoverage",
"//tools/test/CoverageOutputGenerator/java/com/google/devtools/coverageoutputgenerator:Constants",
"//tools/test/CoverageOutputGenerator/java/com/google/devtools/coverageoutputgenerator:Coverage",
"//tools/test/CoverageOutputGenerator/java/com/google/devtools/coverageoutputgenerator:LcovParser",
Expand Down
Loading