1405567 - LCOV data from js::GetCodeCoverageSummary() ignores line 1 and not-taken branches

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Philip Chimento [:ptomato]]

Attachment: Minimal demonstration of problem

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.100 Safari/537.36

Steps to reproduce:

There are a couple of tweaks that should be made in order to get the LCOV output similar to that produced by GCC or Clang:

- Line 1 is never marked as executable or executed
- Branches not taken are marked with hit count "-" instead of 0 (a dash means the whole branch instruction was never executed, 0 should mean the branch instruction was executed but the other branch was taken every time)

See the attachment for a minimal C++ program that demonstrates both problems.


Actual results:

The JS program being instrumented is:

1 let x = function () { return 0; };
2 let y = 0;
3 if (x() > 0)
4     y++;
5 else
6    y++;

The LCOV output is:

TN:Compartment_5f7fbb0700a800
SF:cov.js
FN:1,top-level
FN:1,x
FNDA:1,top-level
FNDA:1,x
FNF:2
FNH:2
BRDA:3,0,0,1
BRDA:3,0,1,-
BRF:2
BRH:1
DA:2,1
DA:3,1
DA:4,0
DA:6,1
LF:4
LH:3
end_of_record

Note that line 1 is not counted as executable and the totals of executable and executed lines are wrong. (Adding a \n to the beginning of the program does yield the correct totals and another DA: entry for line 2.)


Expected results:

The LCOV output should instead be this:

TN:Compartment_5f7fbb0700a800
SF:cov.js
FN:1,top-level
FN:1,x
FNDA:1,top-level
FNDA:1,x
FNF:2
FNH:2
BRDA:3,0,0,1
BRDA:3,0,1,0
BRF:2
BRH:1
DA:1,1
DA:2,1
DA:3,1
DA:4,0
DA:6,1
LF:5
LH:4
end_of_record

Comment 1

[Nicolas B. Pierron [:nbp]]

Thanks again for making these minimal test cases :)

(In reply to Philip Chimento [:ptomato] from comment #0)
> - Branches not taken are marked with hit count "-" instead of 0 (a dash
> means the whole branch instruction was never executed, 0 should mean the
> branch instruction was executed but the other branch was taken every time)

You are right!
I miss-interpreted the documentation from http://ltp.sourceforge.net/coverage/lcov/geninfo.1.php when I implemented it.

Comment 2

[Philip Chimento [:ptomato]]

Glad to help make it easier for you. I wonder if I can help out with this one - it seems like it should be a straightforward patch.

Comment 3

[Nicolas B. Pierron [:nbp]]

(In reply to Philip Chimento [:ptomato] from comment #2)
> Glad to help make it easier for you. I wonder if I can help out with this
> one - it seems like it should be a straightforward patch.

That would be the fastest way to get this fixed ;)

The LCov output produced by SpiderMonkey is all located in:
  http://searchfox.org/mozilla-central/source/js/src/vm/CodeCoverage.cpp

The dash symbols are manually checked and added here:
  http://searchfox.org/mozilla-central/rev/7ba03cebf1dcf3b4c40c85a4efad42a5022cf1ec/js/src/vm/CodeCoverage.cpp#238,244,342,407

there are several test cases that you might have to update, which are checking for the LCov output, such as:
  http://searchfox.org/mozilla-central/source/js/src/jit-test/tests/coverage

You can run these tests with:
  python ./jit-test/jit_test.py  ./path/to/js  coverage

Comment 4

[Philip Chimento [:ptomato]]

Attachment: Get LCOV stats for first line of script, and handle lines that belong to more than one script

Comment 5

[Philip Chimento [:ptomato]]

Attachment: Get LCOV stats for first line of script, and handle lines that belong to more than one script

Comment 6

[Philip Chimento [:ptomato]]

Attachment: Mark LCOV branches not taken with 0, and not executed with -

Comment 7

[Philip Chimento [:ptomato]]

The -/0 issue was indeed quite straightforward.

The off-by-one issue not so much, because leaving out the first line sidestepped another problem: a JSScript's first line can also be part of another JSScript. I'm not sure if the solution that I chose to ensure that each line's hit count was only written out once is the correct one, but so far it works for my purposes.

Comment 8

[Philip Chimento [:ptomato]]

Attachment: Get LCOV stats for first line of script, and handle lines that belong to more than one script

Comment 9

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8926624 [details] [diff] [review]
Get LCOV stats for first line of script, and handle lines that belong to more than one script

Review of attachment 8926624 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/CodeCoverage.cpp
@@ +207,5 @@
>              }
>  
> +            if ((oldLine != lineno || lineno == script->lineno()) &&
> +                fallsthrough &&
> +                !linesWritten_.has(lineno)) {

nit: put the { on a new line, when the condition span on multiple lines.

::: js/src/vm/CodeCoverage.h
@@ +77,5 @@
>      size_t numLinesInstrumented_;
>      size_t numLinesHit_;
> +    // Which lines have had their statistics printed, since a line may be part
> +    // of more than one JSScript.
> +    HashSet<size_t, DefaultHasher<size_t>, SystemAllocPolicy> linesWritten_;

I don't think we want this behaviour.  For example if we have the following code:

  function f() { return function g() { if (arguments.length) return;
    … some code …
  } }

The reports that we might get will show the first line displayed, which does not correspond to the sum of all the lines.

This issues sounds like a post-processing issue to me than a lcov report generation issue.

Comment 10

[Philip Chimento [:ptomato]]

I buy that, it indeed depends on what the expected behaviour is. Just to be clear, you want the sum of all the execution counts from each script for a particular line displayed?

Comment 11

[Nicolas B. Pierron [:nbp]]

(In reply to Philip Chimento [:ptomato] from comment #10)
> I buy that, it indeed depends on what the expected behaviour is. Just to be
> clear, you want the sum of all the execution counts from each script for a
> particular line displayed?

Yes, because you have multiple sources of information. Otherwise, the alternative of picking the first one depends on the creation & clean-up order which is not desirable, and potentially confusing.

Comment 12

[Philip Chimento [:ptomato]]

Attachment: Get LCOV stats for first line of script, and handle lines that belong to more than one script

Comment 13

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8928053 [details] [diff] [review]
Get LCOV stats for first line of script, and handle lines that belong to more than one script

Review of attachment 8928053 [details] [diff] [review]:
-----------------------------------------------------------------

2 remarks:
 - If this is only per JSScript, then why should it be shared across all functions?
 - It seems you are trying to solve the fact that lines are not being visited in an increasing line number, is that right?

::: js/src/vm/CodeCoverage.h
@@ +72,5 @@
>      size_t numBranchesFound_;
>      size_t numBranchesHit_;
>  
> +    // Holds lines statistics. A line may be part of more than one JSScript,
> +    // so this is shared between all scripts in the source file.

nit: add a comment saying that line values are added, if the same line appears in multiple function, across multiple JSScript or multiple times within the same function.

Comment 14

[Philip Chimento [:ptomato]]

(In reply to Nicolas B. Pierron [:nbp] from comment #13)
>  - If this is only per JSScript, then why should it be shared across all
> functions?

My understanding was that there is one JSScript per function plus one for the toplevel in the source file. I may be using the wrong term? The intention is one coverage hash map per source file.

>  - It seems you are trying to solve the fact that lines are not being
> visited in an increasing line number, is that right?

They seem to be visited in an increasing line number within each function, but not across functions. The problem I'm really trying to solve with the hash map is  So if you have this source

let x = function () { return 0; };
let y = 0;
if (x() > 0)
    y++;
else
    y++;

Then without the hashmap you get

DA:1,1
DA:2,1
DA:3,1
DA:4,0
DA:6,1
DA:1,1

Line 1 is repeated because it's covered both by the toplevel JSScript and the JSScript for function x, and added to outDA_ by two subsequent invocations of LCovSource::writeScript(). So it's not just a question of reordering the lines.

> ::: js/src/vm/CodeCoverage.h
> @@ +72,5 @@
> >      size_t numBranchesFound_;
> >      size_t numBranchesHit_;
> >  
> > +    // Holds lines statistics. A line may be part of more than one JSScript,
> > +    // so this is shared between all scripts in the source file.
> 
> nit: add a comment saying that line values are added, if the same line
> appears in multiple function, across multiple JSScript or multiple times
> within the same function.

Will do.

Comment 15

[Philip Chimento [:ptomato]]

Attachment: Get LCOV stats for first line of script, and handle lines that belong to more than one script

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6341e29e55f5
Get LCOV stats for first line of script, and handle lines that belong to more than one script. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/994d32bb01a3
Mark LCOV branches not taken with 0, and not executed with -. r=nbp

Comment 17

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/6341e29e55f5
https://hg.mozilla.org/mozilla-central/rev/994d32bb01a3