Large switch case jump extension bug

VERIFIED FIXED in mozilla1.0.2

Status

()

VERIFIED FIXED
17 years ago
17 years ago

People

(Reporter: brendan, Assigned: brendan)

Tracking

({js1.5})

Trunk
mozilla1.0.2
js1.5
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

17 years ago
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
(Assignee)

Comment 1

17 years ago
Created attachment 92025 [details] [diff] [review]
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
(Assignee)

Comment 2

17 years ago
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
Status: NEW → ASSIGNED
Keywords: js1.5, mozilla1.0.1, mozilla1.1
Target Milestone: --- → mozilla1.0.1
(Assignee)

Comment 3

17 years ago
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
Summary: Large switch bug (no test case, may not bite?) → Large switch case jump extension bug
(Assignee)

Comment 4

17 years ago
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

17 years ago
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.
(Assignee)

Comment 6

17 years ago
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
Keywords: mozilla1.0.1, mozilla1.1 → mozilla1.2
Target Milestone: mozilla1.0.1 → mozilla1.0.2
Comment on attachment 92025 [details] [diff] [review]
proposed fix, plus a name cleanup confined to jsemit.h

r=khanson
Attachment #92025 - Flags: review+
Comment on attachment 92025 [details] [diff] [review]
proposed fix, plus a name cleanup confined to jsemit.h

sr=shaver.
Attachment #92025 - Flags: superreview+
(Assignee)

Comment 9

17 years ago
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
(Assignee)

Comment 10

17 years ago
Fixed, I say.

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 11

17 years ago
Patch verified on trunk and has caused no test regressions.
Marking Verified -
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.