Fix BRDA syntax in lcov files

[liamappelbe]

Fix BRDA syntax in lcov files

[liamappelbe]

More context: flutter/flutter#108313 (comment)

Branch coverage is the +/- symbols on the left, and line coverage is the hilited lines:

The lcov.info for this looks like:

...
DA:18,1
DA:22,1
DA:24,0
DA:28,1
BRDA:18,0,0,1
BRDA:21,0,0,1
BRDA:23,0,0,0
...

Historically, the VM only showed coverage information on function calls. This meant that cases like that if (isFloat) { line would be reported as uncoverable. "Branch coverage", as we use the term, was added to cover these cases, by adding a RecordCoverage instruction to every basic block to report coverage info. package:coverage translates these into BRDA tags that are treated exactly like the ordinary line coverage (DA) tags: BRDA: [line], 0, 0, [hit count]. The block and expr fields are just being set to 0.

This is not how other coverage tools define "branch coverage". We can continue to ignore the block field (it's not relevant in Dart), but there should be multiple BRDA tags on the same line with different expr fields. It's considered weird/exceptional/problematic to have only 1 BRDA tag for a line, because it implies the control flow isn't really branching at all. Some tools will ignore these tags.

Instead we should give all the BRDA tags for a branch point the same line field, and distinguish them by their expr field (and probably just omit BRDA tags when there isn't actually a branch). Our example lcov.info should look more like this:

...
DA:18,1
DA:22,1
DA:24,0
DA:28,1
BRDA:21,0,0,1    <---
BRDA:21,0,1,0    <---
...

The else case's BRDA tag has moved to the if line. This is probably less readable for users, but we're supposed to be able to make the expr tag a human readable string. For example we could call them "if" and "else" instead. Unfortunately this doesn't seem to work in genhtml (BRDA lines with non-integer block or expr fields are ignored), so we'll need to investigate a bit more.

Change 1: To fix this we just have to tweak the RecordCoverage instruction and source_report.cc to change the line that's being reported for else cases, and other bits of control flow like switch statements. We might need to add another flag to the instruction to indicate it's a branch coverage flag, and/or an int to indicate which branch it is. This may also require changes to the getSourceReport RPC and package:coverage to support it. Details TBD.

Change 2: It's probably still useful to keep the RecordCoverage instructions on every basic block, since there's a lot of lines that are marked as uncoverable atm. Instead we should report these non-branching RecordCoverage instructions as ordinary line coverage, so we continue to get the improved line coverage that these instructions give us:

...
DA:18,1
DA:21,1    <---
DA:22,1
DA:23,0    <---
DA:24,0
DA:28,1
BRDA:21,0,0,1
BRDA:21,0,1,0
...

Change 3: We're still missing coverage on that conditional expression, so as a stretch goal we should also try to add branching RecordCoverage instructions there. With change 2 that would give us both branch and line coverage for these expressions:

...
DA:18,1
DA:21,1
DA:22,1
DA:23,0
DA:24,0
DA:27,1    <---
DA:28,1
BRDA:21,0,0,1
BRDA:21,0,1,0
BRDA:27,0,0,1    <---
BRDA:27,0,1,0    <---
...

Change 4: It was also mentioned on those context bugs that branch coverage for simple if/else cases isn't very interesting. As an extra-stretchy stretch goal, it would be even better if early out statements also included branch coverage. In our example, cond1 is true, so cond2 is never evaluated:

...
DA:18,1
DA:21,1
DA:22,1
DA:23,0
DA:24,0
DA:27,1
DA:28,1
BRDA:21,0,0,1
BRDA:21,0,1,0    <---
BRDA:21,0,2,0    <---
BRDA:27,0,0,1
BRDA:27,0,1,0
...

Change 5: Unfortunately change 4 would be really unreadable for users unless we solve the expr naming issue I mentioned. genhtml surfaces the expression index/name to users when they hover over the +/-, so it would be good to give them a human readable name:

...
DA:18,1
DA:21,1
DA:22,1
DA:23,0
DA:24,0
DA:27,1
DA:28,1
BRDA:21,0,if cond1,1    <---
BRDA:21,0,if cond2,0    <---
BRDA:21,0,else,0    <---
BRDA:27,0,if cond1,1    <---
BRDA:27,0,else,0    <---
...

@henry2cox How does that plan sound?

cc @a-siva

[henry2cox]

Unfortunately change 4 would be really unreadable for users unless we solve the expr naming issue

Yes and no. As mentioned in the other issue: I had (inadertently) lied to you about the BRDA syntax for branch expressions. I believe the feature works.
This is more readable - but the simple sorted/numbered approach is not wrong.
It does require the user to understand expressions and operator precedence in order to match indices to edges, but it isn't rocket science, and users are going to have to know that if they want to add tests to exercise uncovered conditions (or to rewrite the code to remove unreacable branches).

There is a lot of branch expression discussion elsewhere regarding reachability and realistic branch targets.

Henry

[henry2cox]

FWIW: gcc/llvm would not mark the 'else' keyword as a line (statement).
It is just a jump label.

} else if (...) {

is a statement - but only because the 'if' statement is on the same line as the label.

This likely doesn't matter either way - as long as the treatment is consistent.
If it gets marked sometimes and not marked sometimes (as we see in some of your presumably-manually-created-thus-not-guaranteed examples) - then differential coverage analysis may become problematic.
(See the paper from the LCOV 'readme' for a discussion of the hows and whys.)