Closed Bug 1434979 Opened 4 years ago Closed 4 years ago

LCovSource::writeScripts mini-bug around odd source notes

Categories

(Core :: JavaScript Engine, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: jorendorff, Assigned: jorendorff)

Details

Attachments

(1 file, 1 obsolete file)

The two sets of source notes below (for the same bytecode) should be equivalent, but LCovSource::writeScripts treats them differently.

(Both sets of source notes are bad. This stuff is amazing.)


Bytecode (same for both cases):
loc     op
-----   --
00000:  defvar "l"
00005:  lambda function f() { ... }
00010:  deffun
main:
00011:  bindgname "l"                   # GLOBAL
...

Old source notes:
 ofs line    pc  delta desc     args
---- ---- ----- ------ -------- ------
  0:    1    11 [  11] xdelta
  1:    1    11 [   0] newline 
  2:    2    11 [   0] colspan 11
  4:    2    11 [   0] setline  lineno 1
  6:    1    11 [   0] setline  lineno 6
  8:    6    11 [   0] colspan 6
 10:    6    33 [  22] xdelta
 ...

LCovSource::writeScripts log:
pc=0, sn=0, lineno=1
pc=5, sn=0, lineno=1
pc=10, sn=0, lineno=1
  processing sn=0 for snpc=11
  processing sn=1 for snpc=11
  processing sn=2 for snpc=11
  processing sn=4 for snpc=11
  processing sn=6 for snpc=11
  processing sn=8 for snpc=11
pc=11, sn=10, lineno=6
  1 hits in new line record

New source notes:
 ofs line    pc  delta desc     args
---- ---- ----- ------ -------- ------
  0:    1     4 [   4] xdelta
  1:    1    11 [   7] setline  lineno 6
  3:    6    11 [   0] colspan 6
  5:    6    33 [  22] xdelta
  ...

LCovSource::writeScripts log:
pc=0, sn=0, lineno=1
  processing sn=0 for snpc=4
pc=5, sn=1, lineno=1
  1 hits in new line record
pc=10, sn=1, lineno=1
  processing sn=1 for snpc=11
  processing sn=3 for snpc=11
pc=11, sn=5, lineno=6
  1 hits in new line record
Hmm. I have a fix for this, but it causes us to register a hit on the first line of almost every script.

That seems undesirable, so I guess I'll try ignoring prologue instructions...
Before this patch, we would only ever add hits on instructions that have source
notes. That is usually right, because that's the only time the line number
changes.

But at the start of a script, this makes us skip instructions until we reach
one with source notes. This interacts badly with SRC_XDELTA notes, which can
appear on any instruction, or even between instructions, because all it means
is "bump the source note pc". So "skip instructions until we see source notes"
is nondeterministic because of SRC_XDELTA's meaninglessness.

The fix is to add hits on the first non-prologue instruction of a script, as
well as instructions that have source notes.
Attachment #8947548 - Flags: review?(nicolas.b.pierron)
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
Before this patch, we would only ever add hits on instructions that have source
notes. That is usually right, because that's the only time the line number
changes.

But at the start of a script, this makes us skip instructions until we reach
one with source notes. This interacts badly with SRC_XDELTA notes, which can
appear on any instruction, or even between instructions, because all it means
is "bump the source note pc". So "skip instructions until we see source notes"
is nondeterministic because of SRC_XDELTA's meaninglessness.

The fix is to add hits on the first non-prologue instruction of a script, as
well as instructions that have source notes.
Attachment #8947557 - Flags: review?(nicolas.b.pierron)
Attachment #8947548 - Attachment is obsolete: true
Attachment #8947548 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8947557 [details] [diff] [review]
LCovSource::writeScripts mini-bug with odd source notes

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

Thanks for fixing this code!
Attachment #8947557 - Flags: review?(nicolas.b.pierron) → review+
Priority: -- → P3
https://hg.mozilla.org/mozilla-central/rev/21d459aa8ca8
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.