302439 - (spandepfu) spandep fu doesn't skip unused JSOP_TABLESWITCH jump table entries

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brendan Eich [:brendan]]

Easily fixed, dumb oversight.  Surprised it hasn't come up.  Clearly our tests
and the real-world testcases (tellme.com, e.g.) have had only switches of density 1!

/be

Comment 1

[Brendan Eich [:brendan]]

Attachment: fix

This is the canonical, regression-free, one-line fix to a crash bug.  We want
it everywhere.	The only objection could be that it doesn't fix other bugs, but
one bug at a time (and please report other bugs!).

/be

Comment 2

[Mike Shaver (:shaver emeritus)]

Comment on attachment 190798 [details] [diff] [review]
fix

r=shaver.

Comment 3

[Brendan Eich [:brendan]]

Attachment: Silviu's test input

Comment 4

[Brendan Eich [:brendan]]

Someone please check in so I can stop worrying about this for 1.8b4.

/be

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

I checked this fix into the trunk earlier today. Marking FIXED.

Comment 6

[Uri Bernstein]

*** Bug 304719 has been marked as a duplicate of this bug. ***

Comment 7

[Brendan Eich [:brendan]]

This bug's patch caused bug 308085.  So much for simple one-line patches!  My
reviewers and I forgot that, per the comment in EmitSwitch:

    /*
     * Emit switchOp followed by switchSize bytes of jump or lookup table.
     *
     * If switchOp is JSOP_LOOKUPSWITCH or JSOP_TABLESWITCH, it is crucial
     * to emit the immediate operand(s) by which bytecode readers such as
     * BuildSpanDepTable discover the length of the switch opcode *before*
     * calling js_SetJumpOffset (which may call BuildSpanDepTable).  It's
     * also important to zero all unknown jump offset immediate operands,
     * so they can be converted to span dependencies with null targets to
     * be computed later (js_EmitN zeros switchSize bytes after switchOp).
     */

So we can't EmitSpanDep only if off != 0 -- duh!  Patch coming in bug 308085.

/be

Comment 8

[Bob Clary [:bc] (inactive)]

Checking in regress-302439.js;
/cvsroot/mozilla/js/tests/ecma_3/Statements/regress-302439.js,v  <-- 
regress-302439.js
initial revision: 1.1
done

I tried reducing the test case, but kept losing the crash so this uses the
original test source.

Comment 9

[Bob Clary [:bc] (inactive)]

verified fixed 20060401 1.8.0.2, 1.8, 1.9a1 win/mac, 1.9a1 linux