1276097 - Add a bytecode sanity check

Status: NEW

(Core :: JavaScript Engine, enhancement, P5)

Description

[Nicholas Nethercote [inactive]]

It would be useful to have a sanity-checking pass for bytecode. There are several options as to when this could be invoked:

- Immediately after building bytecode. (Debug builds, maybe?)

- On some sort of request. (Triggerable by fuzzers?)

But we can work that out once the bytecode checking pass is written.

Comment 1

[Shu-yu Guo [:shu]]

Sanity checking the various notes should be one of the top priorities. That the notes say well-formed things at the moment is not a clear thing by any means.

Concrete suggestions:
 - do try notes settle on sensible PCs to jump to for catch/finally?
 - do scope notes actually form a scope contour?

I also think the obvious candidate of stack depth checking and the such are not as high priority.

Comment 2

[Nicolas B. Pierron [:nbp]]

FYI, I added a section which includes all (2) the functions which are asserting the JSScript sanity:
https://dxr.mozilla.org/mozilla-central/source/js/src/jsscript.cpp#2980-2983

Currently these are called only when we create the JSScript after the bytecode emitter.  I guess we should make a separated function for these 2, and also call it after XDRDecode of JSScript.

(In reply to Shu-yu Guo [:shu] from comment #1)
> Concrete suggestions:
>  - do try notes settle on sensible PCs to jump to for catch/finally?

This is partially done as part of Bug 1261826, as we are checking that these are valid jump targets.

>  - do scope notes actually form a scope contour?

Yes, I recently added an assertion in IonBuilder for that, as the code did not accept the JSScript otherwise.

Comment 3

[Shu-yu Guo [:shu]]

(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> FYI, I added a section which includes all (2) the functions which are
> asserting the JSScript sanity:
> https://dxr.mozilla.org/mozilla-central/source/js/src/jsscript.cpp#2980-2983
> 
> Currently these are called only when we create the JSScript after the
> bytecode emitter.  I guess we should make a separated function for these 2,
> and also call it after XDRDecode of JSScript.
> 
> (In reply to Shu-yu Guo [:shu] from comment #1)
> > Concrete suggestions:
> >  - do try notes settle on sensible PCs to jump to for catch/finally?
> 
> This is partially done as part of Bug 1261826, as we are checking that these
> are valid jump targets.
> 

Very glad to here that's already work here.

> >  - do scope notes actually form a scope contour?
> 
> Yes, I recently added an assertion in IonBuilder for that, as the code did
> not accept the JSScript otherwise.

How hard would it be to factor that out of IonBuilder?

Comment 4

[Nicolas B. Pierron [:nbp]]

(In reply to Shu-yu Guo [:shu] from comment #3)
> (In reply to Nicolas B. Pierron [:nbp] from comment #2)
> > >  - do scope notes actually form a scope contour?
> > 
> > Yes, I recently added an assertion in IonBuilder for that, as the code did
> > not accept the JSScript otherwise.
> 
> How hard would it be to factor that out of IonBuilder?

This is part of the CFG reconstruction of IonBuilder. Currently this part is quite tangled with IonBuilder, as we used it for creating basic blocks and iterating over them in reverse post order.

These are mostly the enum and the first 3 structures under IonBuilder class [1].  If want to extract it from IonBuilder, we would have to abstract the creation of the basic blocks & edges.

I guess this might be a matter of a 1 or 2 weeks.
I should probably work on that at some point of nobody else does.

[1] https://dxr.mozilla.org/mozilla-central/source/js/src/jit/IonBuilder.h?from=IonBuilder#38

Comment 5

[Andrei Purice]

Hey Shu,
Can you still reproduce this issue or should we close it?

Comment 6

[Nicolas B. Pierron [:nbp]]

(In reply to Andrei Purice from comment #5)

Hey Shu,

Shu is no longer working at Mozilla and so I would not expect any answer from him.

Can you still reproduce this issue or should we close it?

This is not an issue but an enhancement.
It still makes sense to add more validation phases on our bytecode, to validate the control flow and the stack depth around the control flow.

Comment 7

[Tooru Fujisawa [:arai]]

If reusing JIT code is troublesome, disassemble (dis shell function) code has some sanity check for control flow and stack depth,
adding notes analysis there and run it for each bytecode emission would benefit as both bytecode validation and more info in disassembly output.