610026 - JSOP_BLOCKCHAIN processing does not handle JSOP_INDEXBASE* ops

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Brendan Eich [:brendan]]

Large programs are possible (need to fix a JSParseNode recycling leak first to get the demo to work). Maybe be s-s, marking that way for now.

This is a regression from the patch for bug 535912.

/be

Comment 1

[Brendan Eich [:brendan]]

Attachment: fix, with test

The test fails without this patch as follows in a release build:

./js1_8_5/regress/regress-610026.js:60: Error: Assertion failed: got (new ReferenceError("x is not defined", "./js1_8_5/regress/regress-610026.js", 55)), expected 42

In an unpatched debug shell it fails like so:

Assertion failure: invalid opcode for fast block chain access, at ../jsinterp.cpp:271

because of the unexpected JSOP_RESETBASE.

We should really simplify our bytecode to use a single immediate format for short and long jumps, regular and "big" literal indexes, etc. It might not even slow the interpreter down. Followup fodder.

/be

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: alternate fix idea

This deals with GetBlockChain in a different way.

Comment 3

[Brendan Eich [:brendan]]

Thanks Bill -- I will use that GetBlockChain approach (tried it but wanted to kill the too-many-overcomplicated-immediate-encodings bird, will defer that to another bug), merged with other changes and tests. The jsparse.cpp change is for yet another bug.

/be

Comment 4

[Brendan Eich [:brendan]]

Attachment: fix, v2

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 489073 [details] [diff] [review]
fix, v2

Great, thanks. It might be good to add a comment to js_AdvanceOverBlockchain along the lines of "watch out for INDEXBASE instructions".

Comment 6

[Brendan Eich [:brendan]]

Attachment: patch to commit

I renamed js_AdvanceOverBlockchain js::AdvanceOverBlockchainOp (the second lowercase-c in "Blockchain" is better with "Op" after to clarify this refers to JSOP_*BLOCKCHAIN; normal CamelCaps spelling would want BlockChain but that seemed less obviously about the two bytecodes).

/be

Comment 7

[Brendan Eich [:brendan]]

Attachment: rebased patch to commit, with test cleanup

Zapped a stray print, and commented the magic numbers.

/be

Comment 8

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/805c1a5d5cc6

/be

Comment 9

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/805c1a5d5cc6