430293 - Eliminating JSOP_TRAP or read-only JSScript.code

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

+++ This is a spin-off of bug 422137 comment 68 +++

The recent fuzzer improvment to include the trap function from the js shell has reveled quite a few bugs where the code have not checked for JSOP_TRAP when accessing JSScript.code.

To eliminate such bugs once and for all the idea is to eliminate JSOP_TRAP all together. Instead traps can be stored as a bitmap in JSScript with explicit patching of jump tables in all currently running script stack frames. See bug 422137 comment 68 for details.

Comment 1

[Igor Bukanov]

Attachment: v1

Something that begins to work.

To ensure thread-safety JS_SetTrap patches the jump tables for scripts that are executed only on the current thread, the scripts on other threads will pick up the new traps only when they hit LOAD_INTERRUPT_HANDLER.

Comment 2

[Igor Bukanov]

Attachment: v2

This passess the test but I need to check that it does not regress SunSpider before asking for a review.

Comment 3

[Igor Bukanov]

Attachment: v3

This is a sync with the trunk of v2.

Comment 4

[Igor Bukanov]

Comment on attachment 318557 [details] [diff] [review]
v3

This is an alternative to fixing all those JSOP_TRAP bugs.

Comment 5

[Igor Bukanov]

Comment on attachment 318557 [details] [diff] [review]
v3

The patch regressed sunspider by 2% on my Pentium-M 1.1 GHz CPU under Fedora-9 with GCC-4.3. The culprit is an extra member in JSFrameRegs to store the trap register state. I added that so the action of JS_SetTrap will be immediately felt by the interpreter. But the tests has shown that this expensive. So I will try an alternative patch without such exposure.

Comment 6

[Igor Bukanov]

Attachment: v4

The new patch does not regress SunSpider. The drawback is that the traps will be sensed only when currently the interpreter detects interrupts, but in practice this should not reduce usability.

Comment 7

[Igor Bukanov]

Attachment: v5

This is a sync with CVS HEAD from 2008-05-05.

Comment 8

[Igor Bukanov]

Attachment: v6

I was testing v5 with -DJS_THREADED_INTERP=0 which hide a one-liner bug in JSOP_CHECK_INTERRUPT: the jump at the end should be done using normalJumpTable, not using jumpTable register which brings the control back to JSOP_CHECK_INTERRUPT.

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 319449 [details] [diff] [review]
v6

js_AllocScripTraps misspelled (no 't' before 'T').

s/immune/immutable/g

I didn't review the js_TraceOpcode factoring carefully.

+ * We check that JSOP_CHECK_INTERRUPT is 2**n - 1 for some n and is bigger

s/bigger/greater/

+     * to correctly manage "op" in all other cases.

s/to correctly manage "op"/to manage "op" correctly/ and re-wrap with previous line to avoid split infinitive (and wrap nicer).

+     * to the bytecode implementation case. When the script has traps or

s/When/But when/

Tested on Windows? Performance effects of switchMask?

Why move the inline call case of

                    LOAD_INTERRUPT_HANDLER(cx);

to be unconditional, instead of just after calling the callHook?

One-line comment for

#define JSOP_CHECK_INTERRUPT    255

in jsinterp.h would be nice -- just a "See js_Interpret and the static assertions just before it." would do.

+ * is only released in js_DestroyScript even if the trap would be removed.

s/is only released/is released only/
s/if/when/

but what does "even when the trap would be removed" mean here? Isn't js_DestroyScript called by definition after any JS_ClearTrap or equivalent?

+ * To allow for fast trap checks without any locks the debug API sets the

Add a comma after "locks".

/be

Comment 10

[Igor Bukanov]

(In reply to comment #9)
> Tested on Windows? Performance effects of switchMask?

On Linux there is no performance difference with JS_THREADED_INTERP={0,1}. But I have not tested this on Windows.

> 
> Why move the inline call case of
> 
>                     LOAD_INTERRUPT_HANDLER(cx);
> 
> to be unconditional, instead of just after calling the callHook?

This is to ensure when the code calls a function with traps from the one without traps, the traps would be activated.

Comment 11

[Igor Bukanov]

Attachment: v6b

The new version of the patch addresses the nits from the comment 9.

Comment 12

[Jesse Ruderman]

When I try to build with this patch, I get:

ld: Undefined symbols:
_js_AllocScriptTraps

Comment 13

[Brendan Eich [:brendan]]

(In reply to comment #12)
> When I try to build with this patch, I get:
> 
> ld: Undefined symbols:
> _js_AllocScriptTraps

jsscript.c misspells this function as js_AllocScripTraps. Igor, new patch?

/be

Comment 14

[Brendan Eich [:brendan]]

Ping for new patch...

/be

Comment 15

[Igor Bukanov]

(In reply to comment #14)
> Ping for new patch...

I think I will mark this as wont-fix. A cheap way to make SS benchmark to run 2% faster on average is to remove the the interrupt handler checks. Which suggests to make the interrupt handler to work in the same way as JSOP_TRAP works cuurently, exactly the opposite of the current patch. 

To solve missing JSOP_TRAP checks problem the trap handler can clone JSScript.code so only the interpreter see the bytecode with traps and then hide access to fp->regs->pc behind API that return untrapped version. This can be made threadsafe without any locks if the interpreter checks for traps and reloads its pc accordingly exactly at those places where it currently checks for the interrupt handler.  

Comment 16

[Brendan Eich [:brendan]]

Andreas Gal and I were talking about traps and such. I remembered what I could of our unrealized goals:

* thread-safety
* no penalty for other threads than a debugged thread sharing compiled code
* would like other kinds of traps (e.g. loop counter/tracer ops)

I'm in favor of hiding fp->regs->pc to JSOp computation, for sure. It's doable.

/be

Comment 17

[Brendan Eich [:brendan]]

Comment on attachment 319584 [details] [diff] [review]
v6b

Off radar for now. Please request again for a new patch (or this one if I am misremembering).

/be

Comment 18

[Igor Bukanov]

This is no longer relevant