1434979 - LCovSource::writeScripts mini-bug around odd source notes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Jason Orendorff [:jorendorff]]

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

Comment 1

[Jason Orendorff [:jorendorff]]

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...

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: LCovSource::writeScripts mini-bug with odd source notes

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.

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: LCovSource::writeScripts mini-bug with odd source notes

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.

Comment 4

[Nicolas B. Pierron [:nbp]]

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!

Comment 5

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/21d459aa8ca8fcf4bf646d586d15db42660e53a3
Bug 1434979 - LCovSource::writeScripts mini-bug with odd source notes. r=nbp.

Comment 6

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/21d459aa8ca8