158382 - Large switch case jump extension bug

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brendan Eich [:brendan]]

I found a bug in OptimizeSpanDeps,the function in jsemit.c that extends
span-dependent branch and jump bytecodes.  The bug doesn't seem to have any bad
effects, due to the way things work, but the buggy code is misleading and should
be fixed.  Patch coming up, comments in it should explain everything.

/be

Comment 1

[Brendan Eich [:brendan]]

Attachment: proposed fix, plus a name cleanup confined to jsemit.h

The comments and the larger context (probably best to apply the patch and read
the first part of OptimizeSpanDeps, the do-while loop containing the outer for
loop that contains the inner for loop shown in the patch) should make clear
what's going on here.  The case where sd2 <= sd arises for all jump and switch
bytecodes, but the case where sd2 > sd arises only for switches.  If a switch
has cases that follow a very large case that follows some small ones, the small
cases' jumps won't need to be extended, but the jump that spans the large case
will -- and if any switch jump needs to be extended, all jumps in the switch
opcode do.

The bug was that JSSpanDep records for such a switch, up to and including the
one for the jump that needs to be extended, did not have their offsets updated
to include the cumulative effects of extending the earlier case's jumps from 2
to 4 byte immediate offset operands.

I'm not sure this bug is benign now that I think about it a bit.  It tends to
overstate the spans of the earlier cases' jumps, which could cause crashes
(jumping into the middle of an opcode, e.g.), once final spans are computed and
jump offsets updated.  Looking for pschwartau testing (Phil, can you make a
testcase with some short cases, and a very long one, then some more short
ones?), review, and super-review.

/be

Comment 2

[Brendan Eich [:brendan]]

Yeah, I think this bug can lead to crashes.  I'll let Phil confirm that and add
the crash keyword with a testcase.  This should be fixed ASAP.

/be

Comment 3

[Brendan Eich [:brendan]]

function f(x) {
  switch (x) {
    case 1:
      print(1);
      break;
    case 2:
      print(2);
      break;
    case 3:
      print(3);
      ... lots of code, more than 64K bytecodes worth ...;
      break;
    case 4:
      print(4);
  }
}

should do the trick.  The JSOP_TABLESWITCH jump offsets for case 2 and case 3
should be off by +2 and +4 bytes, respectively.

/be

Comment 4

[Brendan Eich [:brendan]]

Hmm, I think I was right initially -- this bug is benign, because spans for the
lookup and table switch bytecodes are relative to the top offset (the lowest in
bytecode order) of the switch, and top doesn't change in the inner for loop due
to jump extensions in the table or lookup switch.  Another way to put it is that
sd2->offset will be wrong, but is not used, for cases 2 and 3 in my testcase
sketch.  So this is a clarity and internal consistency bug only.

/be

Comment 5

[Phil Schwartau]

FWIW, I did try a testcase as outlined in Comment #3 above.
In case(3), I pasted the 87K body of testcase 

          js/test/js1_5/Regress/regress-89443.js

When I tried it out, I did not crash in either the debug or optimized
JS shell. I tried running it both via the -f option to the shell, and
from within the shell via the load() function.

Comment 6

[Brendan Eich [:brendan]]

Moving out a bit.  The js1.5 release (after an rc5 release candidate) should
come off off the 1.0 branch on or before 1.0.2 freezing, I say.  The trunk and
branch should be in sync for js/src/js*.[chmt]* and build files.

/be

Comment 7

[Kenton Hanson (gone)]

Comment on attachment 92025 [details] [diff] [review]
proposed fix, plus a name cleanup confined to jsemit.h

r=khanson

Comment 8

[Mike Shaver (:shaver emeritus)]

Comment on attachment 92025 [details] [diff] [review]
proposed fix, plus a name cleanup confined to jsemit.h

sr=shaver.

Comment 9

[Brendan Eich [:brendan]]

Fixed in trunk.  I think at this point we should get js1.5 in shape on the
trunk, then branch a release candidate there, and only after we're really ready,
lobby drivers to take the final 1.5 as a landing from the trunk onto the 1.0
branch.  Then we can cvs-tag 1.5 from there.

/be

Comment 10

[Brendan Eich [:brendan]]

Fixed, I say.

/be

Comment 11

[Phil Schwartau]

Patch verified on trunk and has caused no test regressions.
Marking Verified -