Closed
Bug 1213732
Opened 9 years ago
Closed 9 years ago
Code coverage branch results are only reporting the taken part of the branches.
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: jcranmer, Assigned: nbp)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
11.29 KB,
patch
|
bhackett1024
:
review+
|
Details | Diff | Splinter Review |
2.51 KB,
patch
|
bhackett1024
:
review+
|
Details | Diff | Splinter Review |
14.17 KB,
patch
|
bhackett1024
:
review+
|
Details | Diff | Splinter Review |
See <https://www.tjhsst.edu/~jcranmer/test-ui/mailnews/mime/src/jsmime.jsm.html>. Notice how each of the branches has only one [ - ] or [ + ] in a bracket? In comparison to stuff like <https://www.tjhsst.edu/~jcranmer/c-ccov/mailnews/compose/src/nsMsgSend.cpp.html>, where a typical two-way branch has a [ - - ] or [ - + ], etc. I haven't tested if switch statements are similarly borked.
In essence, this derives from a misunderstanding of the lcov format. In a BRDA line, there are four entries: line number, block number, branch number, count. LCOV is basically a summarization of gcov here, so you need to understand how gcov is instrumenting to see what makes sense.
gcov basically builds a control-flow graph, and instruments counters for the targets of the conditional at the end of each basic block if there are more than one entries. The block number is the number of the basic block within the function, and the branch number is the number of the taken edge within the list of possible targets. So, for a normal branch statement, the branch number is 0 for the "true" edge and 1 for the "false" edge (and switch edges number from 0-n for each of the case statements in lexical order, default at end). [Note: I don't know what happens for exceptions, and clang may operate differently there].
Assignee | ||
Comment 1•9 years ago
|
||
(In reply to Joshua Cranmer [:jcranmer] from comment #0)
> I haven't tested if switch statements are similarly borked.
Note that the semantic of switch statements in JavaScript is literally to iterate on all conditions, as the execution of a case statement expression can have side-effects. So a "case .. expr...:" statement is expected to be equivalent to
} else if (x == ...expr...) {
Assignee | ||
Updated•9 years ago
|
Summary: Code coverage branch results are nonsense → Code coverage branch results are only reporting the taken part of the branches.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → nicolas.b.pierron
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8683162 -
Flags: review?(bhackett1024)
Assignee | ||
Comment 3•9 years ago
|
||
This adds the test cases mentionned in Bug 1209515 (part 6).
Attachment #8683164 -
Flags: review?(bhackett1024)
Assignee | ||
Comment 4•9 years ago
|
||
As opposed to what I mentionned in comment 1. We do have switch statements
which are jumping to multiple targets, and unfortunately, we cannot easily
reconstruct the semantic of the original switch case.
Thus this code record the jump targets of the table switch statements, and
instrument the LCov generation code in order to report the proper output.
Attachment #8683167 -
Flags: review?(bhackett1024)
Assignee | ||
Comment 5•9 years ago
|
||
Comment on attachment 8683167 [details] [diff] [review]
part 3 - SM LCov: Add code coverage support for TableSwitch statements.
Review of attachment 8683167 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jsscript.cpp
@@ +1338,5 @@
> +
> + for (int i = 0; i < high-low+1; i++) {
> + pc2 += JUMP_OFFSET_LEN;
> + int32_t off = (int32_t) GET_JUMP_OFFSET(pc2);
> + if (off != len) {
self-nit: s/off != len/off/ (0 is used as a mean to mention the default case)
Updated•9 years ago
|
Attachment #8683162 -
Flags: review?(bhackett1024) → review+
Updated•9 years ago
|
Attachment #8683164 -
Flags: review?(bhackett1024) → review+
Updated•9 years ago
|
Attachment #8683167 -
Flags: review?(bhackett1024) → review+
Assignee | ||
Comment 6•9 years ago
|
||
Note: Landing part 3 is currently blocked Bug 1139235 changes, as we no longer produce line numbers for case statements which have literals.
Comment 8•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/21f613700470
https://hg.mozilla.org/mozilla-central/rev/56e52e58c31f
https://hg.mozilla.org/mozilla-central/rev/cb9e7ee52f26
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in
before you can comment on or make changes to this bug.
Description
•