Bug 181509 – When inserting Unreachable in byte code parser we need to flush all the right things

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Matt Lewis]

Created attachment 330997 [details]
Crash Log

The following layout test is failing on macOS

webgl/1.0.2/conformance/uniforms/uniform-default-values.html

Probable cause:

Talked with Mark and thought this may be a good place to start.
https://trac.webkit.org/changeset/226655/webkit

Flakiness Dashboard:

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=webgl%2F1.0.2%2Fconformance%2Funiforms%2Funiform-default-values.html

build:
https://build.webkit.org/results/Apple%20El%20Capitan%20Release%20WK1%20(Tests)/r226666%20(7511)/results.html

[Radar WebKit Bug Importer]

<rdar://problem/36423110>

[Saam Barati]

(In reply to Matt Lewis from comment #0)
> Created attachment 330997 [details]
> Crash Log
> 
> The following layout test is failing on macOS
> 
> webgl/1.0.2/conformance/uniforms/uniform-default-values.html
> 
> Probable cause:
> 
> Talked with Mark and thought this may be a good place to start.
> https://trac.webkit.org/changeset/226655/webkit
Agreed. Seems like it's the likely cause. I'll build to check it out.

> 
> Flakiness Dashboard:
> 
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=webgl%2F1.0.2%2Fconformance%2Funiforms%2Funiform-
> default-values.html
> 
> build:
> https://build.webkit.org/results/
> Apple%20El%20Capitan%20Release%20WK1%20(Tests)/r226666%20(7511)/results.html

[Saam Barati]

I think I spot a bug in my original patch. Will confirm once I have a build.

[Saam Barati]

I can't reproduce this locally....

[Saam Barati]

Created attachment 331016 [details]
WIP

If the bug is what I think it is, then this is a fix. I still need to repro it though.

[EWS Watchlist]

Attachment 331016 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:619:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:620:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:627:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:629:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:631:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:633:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:636:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:646:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:647:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:649:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:671:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6668:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6669:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6673:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6684:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 15 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 331020 [details]
WIP

[Saam Barati]

Here is a reduced repro of what I believe is the bug:

```
function assert(b) {
    if (!b)
        throw new Error()
}
noInline(assert);

function test(b, f) {
    if (b)
        return f(b);
}
noInline(test);

function throwError(b) {
    if (b) {
        try {
            throw new Error;
        } catch(e) {
            print(e.stack);
            print();
        }
    }
    return 2;
}
noInline(throwError);

function num() { return 25; }

function makeFoo() {
    return function foo(b) {
        throwError(b);
        OSRExit();
    }
}

let foos = [makeFoo(), makeFoo()];
function bar() { }
for (let i = 0; i < 10000; ++i) {
    test(!!(i%2), foos[((Math.random() * 100) | 0) % foos.length], 10, 20);
}
```

Basically, I'm forgetting to flush a couple of things that need to be flushed.

[Saam Barati]

Created attachment 331024 [details]
WIP

[Saam Barati]

(In reply to Saam Barati from comment #8)
> Here is a reduced repro of what I believe is the bug:
> 
> ```
> function assert(b) {
>     if (!b)
>         throw new Error()
> }
> noInline(assert);
> 
> function test(b, f) {
>     if (b)
>         return f(b);
> }
> noInline(test);
> 
> function throwError(b) {
>     if (b) {
>         try {
>             throw new Error;
>         } catch(e) {
>             print(e.stack);
>             print();
>         }
>     }
>     return 2;
> }
> noInline(throwError);
> 
> function num() { return 25; }
> 
> function makeFoo() {
>     return function foo(b) {
>         throwError(b);
>         OSRExit();
>     }
> }
> 
> let foos = [makeFoo(), makeFoo()];
> function bar() { }
> for (let i = 0; i < 10000; ++i) {
>     test(!!(i%2), foos[((Math.random() * 100) | 0) % foos.length], 10, 20);
> }
> ```
> 
> Basically, I'm forgetting to flush a couple of things that need to be
> flushed.

Yup. This must be the bug. I've verified my WIP fixes it.

[Saam Barati]

Created attachment 331084 [details]
patch

[Saam Barati]

Created attachment 331085 [details]
patch

[EWS Watchlist]

Attachment 331085 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6624:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6625:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Comment on attachment 331085 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=331085&action=review

r=me with suggestions.

> JSTests/stress/proper-flushing-when-we-insert-unreachable-after-force-exit-in-bytecode-parser.js:5
> +function assert(b) {
> +    if (!b)
> +        throw new Error()
> +}
> +noInline(assert);

This is not used.  Did you intend to assert something?  Otherwise, please remove.

> JSTests/stress/proper-flushing-when-we-insert-unreachable-after-force-exit-in-bytecode-parser.js:31
> +function bar() { }

Is this needed?  If not, please remove.

> JSTests/stress/proper-flushing-when-we-insert-unreachable-after-force-exit-in-bytecode-parser.js:33
> +    test(!!(i%2), foos[((Math.random() * 100) | 0) % foos.length], 10, 20);

The 10, 20 args appear to be unused.  Are they really needed for this? If not, please remove.

[Saam Barati]

Created attachment 331106 [details]
patch for landing

[Saam Barati]

(In reply to Mark Lam from comment #14)
> Comment on attachment 331085 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331085&action=review
> 
> r=me with suggestions.
> 
> > JSTests/stress/proper-flushing-when-we-insert-unreachable-after-force-exit-in-bytecode-parser.js:5
> > +function assert(b) {
> > +    if (!b)
> > +        throw new Error()
> > +}
> > +noInline(assert);
> 
> This is not used.  Did you intend to assert something?  Otherwise, please
> remove.
> 
> > JSTests/stress/proper-flushing-when-we-insert-unreachable-after-force-exit-in-bytecode-parser.js:31
> > +function bar() { }
> 
> Is this needed?  If not, please remove.
> 
> > JSTests/stress/proper-flushing-when-we-insert-unreachable-after-force-exit-in-bytecode-parser.js:33
> > +    test(!!(i%2), foos[((Math.random() * 100) | 0) % foos.length], 10, 20);
> 
> The 10, 20 args appear to be unused.  Are they really needed for this? If
> not, please remove.

Fixed all of these.

[EWS Watchlist]

Attachment 331106 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6624:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6625:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 331106 [details]
patch for landing

Clearing flags on attachment: 331106

Committed r226811: <https://trac.webkit.org/changeset/226811>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.