932180 - Rewrite decompiler's bytecode parser to not need SRC_HIDDEN annotations

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andy Wingo [:wingo]]

The attached patch ports the bytecode parser in jsanalyze.cpp to jsopcode.cpp, and uses it to determine the stack depth and defining opcode stack for each instruction in a function.  It then ports the decompiler to use that parser to attempt to reconstruct stack operands.

I ported the parser instead of using it directly because I hear that jsanalyze is on its way out.  I don't need SSA information anyway.

Once GetBlockChainAtPC uses a side table (bug 927782), this will remove all uses of SRC_HIDDEN, so we can make the bytecode emitter stop adding those notes.  Yays.  Also it fixes bug 931414.

Comment 1

[Andy Wingo [:wingo]]

Attachment: Rewrite decompiler's bytecode parser to not need SRC_HIDDEN annotations

Comment 2

[Andy Wingo [:wingo]]

Attachment: Rewrite decompiler's bytecode parser to not need SRC_HIDDEN annotations v2

Comment 3

[Andy Wingo [:wingo]]

Updated patch fixed decompilation of stack operands whose definition could not be found.

Comment 4

[Andy Wingo [:wingo]]

Comment on attachment 823844 [details] [diff] [review]
Rewrite decompiler's bytecode parser to not need SRC_HIDDEN annotations v2

Jan has kindly agreed to take a look here.  Thanks!

Comment 5

[Andy Wingo [:wingo]]

The patch is broken for release mode;  I just need to move the BytecodeParser outside of the #ifdef DEBUG.  Running this test:

 var t = new Date; (function(){ for (var i = 0; i < 1000000; i++) { try { var x = 0; x(); } catch (e) {} } })(); print(new Date - t);

Before the patch I get around 4350ms, and after I get 4200ms or so; so perf is a wash.

Comment 6

[Andy Wingo [:wingo]]

Attachment: Rewrite decompiler's bytecode parser to not need SRC_HIDDEN annotations v3

Comment 7

[Andy Wingo [:wingo]]

Comment on attachment 823892 [details] [diff] [review]
Rewrite decompiler's bytecode parser to not need SRC_HIDDEN annotations v3

Updated patch compiles BytecodeParser in release mode as well.  Previously quoted perf numbers are for release mode.

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 823892 [details] [diff] [review]
Rewrite decompiler's bytecode parser to not need SRC_HIDDEN annotations v3

Review of attachment 823892 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent! I tried to debug this a few times and still have no idea how SRC_HIDDEN is supposed to work.

::: js/src/jsopcode.cpp
@@ +362,5 @@
> +
> +        // Whether this instruction has been analyzed to get its output defines
> +        // and stack. */
> +        bool parsed : 1;
> +        

Nit: trailing whitespace.

@@ +404,5 @@
> +    JSContext *cx_;
> +    LifoAllocScope allocScope_;
> +#ifdef DEBUG
> +    //LifoAlloc::Mark mark_;
> +#endif

Can remove this and similar #ifdef DEBUG blocks below.

@@ +430,5 @@
> +
> +    bool parse();
> +
> +    uint32_t stackDepthAtPC(uint32_t offset) {
> +        Bytecode *code = maybeCode(offset);

Can we use getCode here instead of maybeCode?

@@ +554,5 @@
> +
> +bool
> +BytecodeParser::addJump(unsigned offset,
> +                        unsigned *currentOffset, unsigned *forwardJump, unsigned *forwardLoop,
> +                        unsigned stackDepth, const unsigned *offsetStack)

BytecodeParser::simulateOp uses uint32_t for offset, stackDepth and offsetStack instead of unsigned. The rest of the code mostly uses unsigned.

uint32_t for the offsetStack seems a bit more efficient on x64 so we should probably use that everywhere for the offsets?

@@ +594,5 @@
> +}
> +
> +bool
> +BytecodeParser::parse()
> +{

Nit: can you assert here we are not calling parse() twice? JS_ASSERT(!codeArray_) or something.

@@ +615,5 @@
> +    unsigned forwardLoop = 0;
> +
> +    // If we are in the middle of a try block, the offset of the highest
> +    // catch/finally/enditer.
> +    unsigned forwardCatch = 0;

forwardJump, forwardLoop and forwardCatch are set/checked in a few places but not really used anywhere. Removing them will simplify the code even more.

@@ +1879,5 @@
>      /*
> +     * Fall back on *valuepc as start pc if this frame is calling .apply and the
> +     * methodjit has "splatted" the arguments array, bumping the caller's stack
> +     * pointer and skewing it from what static analysis in BytecodeParser would
> +     * compute.

Remove this comment. There used to be a StackFrame check here but it was removed with JM.

Comment 9

[Jan de Mooij [:jandem]]

You can also remove Bytecode's jumpTarget, fallthrough, jumpFallthrough and exceptionEntry fields. We can always add them back later if we have to.

Comment 10

[Andy Wingo [:wingo]]

Comment on attachment 823892 [details] [diff] [review]
Rewrite decompiler's bytecode parser to not need SRC_HIDDEN annotations v3

Re-adding jandem as reviewer; got lost in the shuffle.

Comment 11

[Andy Wingo [:wingo]]

Comment on attachment 823892 [details] [diff] [review]
Rewrite decompiler's bytecode parser to not need SRC_HIDDEN annotations v3

Sorry, I was confused; didn't see the review, I thought I cleared it accidentally.  Thanks for the comments, will get back to you :)

Comment 12

[Andy Wingo [:wingo]]

Attachment: Rewrite decompiler's bytecode parser to not need SRC_HIDDEN annotations v4

Comment 13

[Andy Wingo [:wingo]]

Comment on attachment 824015 [details] [diff] [review]
Rewrite decompiler's bytecode parser to not need SRC_HIDDEN annotations v4

Updated patch addresses nits.

Regarding the question about using getCode instead of maybeCode in getStackDepthForPC, I did the change, then found one case that was asserting.  The backtrace:

#0  0x00000000005e9faa in getCode (this=<optimized out>, this=<optimized out>, offset=96) at /home/wingo/src/mozilla-central/js/src/jsopcode.cpp:444
#1  stackDepthAtPC (offset=96, this=0x7fffffff7d30) at /home/wingo/src/mozilla-central/js/src/jsopcode.cpp:405
#2  stackDepthAtPC (pc=0x7ffff5941940 "(\021\225\365\377\177", this=0x7fffffff7d30) at /home/wingo/src/mozilla-central/js/src/jsopcode.cpp:406
#3  js_ReconstructStackDepth (cx=<optimized out>, script=script@entry=0x7ffff5941940, pc=pc@entry=0x1788c79 " \240\r\245\220\022\231\016рψ\203\246\220\022\231\016рψ\203\256$\230\006\232\b")
    at /home/wingo/src/mozilla-central/js/src/jsopcode.cpp:1993
#4  0x000000000092abfe in js::jit::CodeGeneratorShared::encode (this=this@entry=0x17b60b0, snapshot=0x17ad170) at /home/wingo/src/mozilla-central/js/src/jit/shared/CodeGenerator-shared.cpp:282
#5  0x000000000092dad3 in js::jit::CodeGeneratorShared::markOsiPoint (this=this@entry=0x17b60b0, ins=ins@entry=0x17ad1b8, callPointOffset=callPointOffset@entry=0x7fffffff7f00)
    at /home/wingo/src/mozilla-central/js/src/jit/shared/CodeGenerator-shared.cpp:426

This corresponded to getting the stack depth at the JSOP_STOP in this function:

(gdb) fr 3
#3  js_ReconstructStackDepth (cx=<optimized out>, script=script@entry=0x7ffff5941940, pc=pc@entry=0x1788c79 " \240\r\245\220\022\231\016рψ\203\246\220\022\231\016рψ\203\256$\230\006\232\b")
    at /home/wingo/src/mozilla-central/js/src/jsopcode.cpp:1993
1993	    return parser.stackDepthAtPC(pc);
(gdb) p js_DumpScriptDepth(parser.cx_, script, pc)
js1_8_1/regress/regress-479430-02.js:30
    sn stack loc   line  op
    -- ----- ----- ----  --
main:
             00000:  32  getarg 0
             00003:  32  zero
             00004:  32  ne
    01       00005:  32  ifeq 94 (+89)
    18       00010:  33  try 33 (+23)
             00011:  33  callname "f"
             00016:  33  undefined
             00017:  33  notearg
             00018:  33  getarg 0
             00021:  33  one
             00022:  33  sub
             00023:  33  notearg
             00024:  33  call 1
             00027:  33  pop
    16       00028:  33  goto 52 (+24)
             00033:  33  enterblock object
             00038:  33  exception
             00039:  33  setlocal 0
             00042:  33  pop
    17       00043:  33  leaveblock 1
    16       00046:  33  goto 52 (+6)
             00051:  33  nop
    18       00052:  34  try 75 (+23)
             00053:  34  callname "f"
             00058:  34  undefined
             00059:  34  notearg
             00060:  34  getarg 0
             00063:  34  one
             00064:  34  sub
             00065:  34  notearg
             00066:  34  call 1
             00069:  34  pop
    16       00070:  34  goto 94 (+24)
             00075:  34  enterblock object
             00080:  34  exception
             00081:  34  setlocal 0
             00084:  34  pop
    17       00085:  34  leaveblock 1
    16       00088:  34  goto 94 (+6)
             00093:  34  nop
             00094:  36  zero
             00095:  36  throw
-->          00096:  36  stop

So I added code to mark JSOP_STOP as always reachable.  TBH I don't know why JSOP_STOP is needed at all; it would be easy to ensure all functions end with JSOP_RETURN.  But whatever, an issue for some other day.

Comment 14

[Andy Wingo [:wingo]]

Attachment: Rewrite decompiler's bytecode parser to not need SRC_HIDDEN annotations v5

Comment 15

[Andy Wingo [:wingo]]

Comment on attachment 824017 [details] [diff] [review]
Rewrite decompiler's bytecode parser to not need SRC_HIDDEN annotations v5

Sorry for the churn; forgot to remove a comment and noticed some trailing whitespace.

Comment 16

[Jan de Mooij [:jandem]]

Comment on attachment 824017 [details] [diff] [review]
Rewrite decompiler's bytecode parser to not need SRC_HIDDEN annotations v5

Review of attachment 824017 [details] [diff] [review]:
-----------------------------------------------------------------

Nice! r=me with nits addressed.

::: js/src/jsopcode.cpp
@@ +395,5 @@
> +
> +  public:
> +    BytecodeParser(JSContext *cx, JSScript *script)
> +      : cx_(cx),
> +        allocScope_(&cx->typeLifoAlloc()),

s/typeLifoAlloc/tempLifoAlloc

@@ +511,5 @@
> +    return stackDepth;
> +}
> +
> +bool
> +BytecodeParser::addJump(uint32_t offset, uint32_t *currentOffset, 

Nit: trailing whitespace

@@ +597,5 @@
> +            // FrameRegs::setToEndOfScript that can reach it.
> +            if (op == JSOP_STOP) {
> +                // Simulate a jump coming in to the STOP from who-knows where,
> +                // with zero operands on the stack.
> +                addJump(offset, &offset, 0, nullptr);

Nit: OOM check.

Comment 17

[Andy Wingo [:wingo]]

Attachment: Rewrite decompiler's bytecode parser to not need SRC_HIDDEN annotations

Comment 18

[Andy Wingo [:wingo]]

Attachment: Rewrite decompiler's bytecode parser to not need SRC_HIDDEN annotations r=jandem

Comment 19

[Andy Wingo [:wingo]]

Comment on attachment 824045 [details] [diff] [review]
Rewrite decompiler's bytecode parser to not need SRC_HIDDEN annotations  r=jandem

Thanks for the review :)

Comment 20

[Andy Wingo [:wingo]]

Clearing checkin-needed, while I run a try build just in case:

https://tbpl.mozilla.org/?tree=Try

Comment 21

[Andy Wingo [:wingo]]

https://tbpl.mozilla.org/?tree=Try&rev=cdc6ef35351f rather

Comment 22

[Andy Wingo [:wingo]]

Though the release trybuild is fine, running debug with --ion-eager I get another assertion failure:

gdb --args +debug/js --ion-eager /home/wingo/src/mozilla-central/js/src/jit-test/tests/auto-regress/bug912379.js
(gdb) r
Assertion failure: codeArray_[offset], at /home/wingo/src/mozilla-central/js/src/jsopcode.cpp:444

Catchpoint 1 (signal SIGSEGV), 0x00000000005e9e03 in getCode (this=<optimized out>, this=<optimized out>, offset=7) at /home/wingo/src/mozilla-central/js/src/jsopcode.cpp:444
444	        JS_ASSERT(codeArray_[offset]);
(gdb) fr 3
#3  js_ReconstructStackDepth (cx=<optimized out>, script=script@entry=0x7ffff595a100, pc=pc@entry=0x1796ac0 "\006") at /home/wingo/src/mozilla-central/js/src/jsopcode.cpp:1999
1999	    return parser.stackDepthAtPC(pc);
(gdb) p js_DumpScriptDepth(parser.cx_, script, pc)
/home/wingo/src/mozilla-central/js/src/jit-test/tests/auto-regress/bug912379.js:9
    sn stack loc   line  op
    -- ----- ----- ----  --
main:
    18       00000:   9  try 12 (+12)
             00001:   9  getgname "StopIteration"
             00006:   9  throw
--> 16       00007:   9  goto 45 (+38)
             00012:   9  enterblock object
             00017:   9  exception
             00018:   9  setlocal 0
             00021:   9  pop
             00022:   9  bindgname "x"
             00027:   9  getlocal 0
             00030:   9  setgname "x"
             00035:   9  popv
    17       00036:   9  leaveblock 1
    16       00039:   9  goto 45 (+6)
             00044:   9  nop
             00045:   9  callgname "wrap"
             00050:   9  undefined
             00051:   9  notearg
             00052:   9  getgname "x"
             00057:   9  notearg
             00058:   9  call 1
             00061:   9  popv
             00062:   9  stop

And indeed this instruction is not reachable.  So I think the best thing to do is to go back to maybeCode, and assume that the stack depth is 0 if there is no code -- because it shouldn't be possible to get to an unreachable opcode while there are values on the stack.  Jan, WDYT?

Comment 23

[Andy Wingo [:wingo]]

Attachment: Rewrite decompiler's bytecode parser to not need SRC_HIDDEN annotations v8

Comment 24

[Andy Wingo [:wingo]]

Comment on attachment 824587 [details] [diff] [review]
Rewrite decompiler's bytecode parser to not need SRC_HIDDEN annotations v8

Uploaded patch reverting to maybeCode and removing the JSOP_STOP case.  r? to jandem.

Comment 25

[Andy Wingo [:wingo]]

https://tbpl.mozilla.org/?tree=Try&rev=c67f0494ab9a

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3510684869de

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3510684869de