Bug 130134 – REGRESSION(r165459): It broke 109 jsc stress test on ARM Thumb2 and Mac 32 bit

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Csaba Osztrogonác]

before: 265 failures - http://build.webkit.sed.hu/builders/EFL%20ARMv7%20Linux%20Release%20%28Build%29/builds/3308
after: 374 failures - http://build.webkit.sed.hu/builders/EFL%20ARMv7%20Linux%20Release%20%28Build%29/builds/3306

new failures:
--------------
jsc-layout-tests.yaml/js/script-tests/dfg-getter.js.layout
jsc-layout-tests.yaml/js/script-tests/dfg-getter.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/dfg-getter.js.layout-no-llint
jsc-layout-tests.yaml/js/script-tests/dfg-getter-throw.js.layout
jsc-layout-tests.yaml/js/script-tests/dfg-getter-throw.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/dfg-getter-throw.js.layout-no-llint
jsc-layout-tests.yaml/js/script-tests/dfg-inline-arguments-use-from-all-the-places.js.layout
jsc-layout-tests.yaml/js/script-tests/dfg-inline-arguments-use-from-all-the-places.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/dfg-inline-arguments-use-from-all-the-places.js.layout-no-llint
jsc-layout-tests.yaml/js/script-tests/dfg-inline-arguments-use-from-getter.js.layout
jsc-layout-tests.yaml/js/script-tests/dfg-inline-arguments-use-from-getter.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/dfg-inline-arguments-use-from-getter.js.layout-no-llint
regress/script-tests/get-by-id-proto-or-self.js.always-trigger-copy-phase
regress/script-tests/get-by-id-proto-or-self.js.default
regress/script-tests/get-by-id-proto-or-self.js.no-cjit-validate-phases
regress/script-tests/get-by-id-self-or-proto.js.always-trigger-copy-phase
regress/script-tests/get-by-id-self-or-proto.js.default
regress/script-tests/get-by-id-self-or-proto.js.dfg-eager
regress/script-tests/get-by-id-self-or-proto.js.dfg-eager-no-cjit-validate
regress/script-tests/get-by-id-self-or-proto.js.no-cjit-validate-phases
regress/script-tests/get-by-id-self-or-proto.js.no-llint
regress/script-tests/polymorphic-get-by-id.js.always-trigger-copy-phase
regress/script-tests/polymorphic-get-by-id.js.default
regress/script-tests/polymorphic-get-by-id.js.dfg-eager
regress/script-tests/polymorphic-get-by-id.js.dfg-eager-no-cjit-validate
regress/script-tests/polymorphic-get-by-id.js.no-cjit-validate-phases
regress/script-tests/polymorphic-get-by-id.js.no-llint
regress/script-tests/polymorphic-put-by-id.js.always-trigger-copy-phase
regress/script-tests/polymorphic-put-by-id.js.default
regress/script-tests/polymorphic-put-by-id.js.dfg-eager
regress/script-tests/polymorphic-put-by-id.js.dfg-eager-no-cjit-validate
regress/script-tests/polymorphic-put-by-id.js.no-cjit-validate-phases
regress/script-tests/polymorphic-put-by-id.js.no-llint
regress/script-tests/polymorphic-structure.js.dfg-eager
regress/script-tests/polymorphic-structure.js.dfg-eager-no-cjit-validate
stress/fold-multi-get-by-offset-to-get-by-offset.js.default
stress/fold-multi-get-by-offset-to-get-by-offset.js.no-cjit
stress/fold-multi-get-by-offset-to-get-by-offset-with-watchpoint.js.default
stress/fold-multi-get-by-offset-to-get-by-offset-with-watchpoint.js.no-cjit
stress/fold-multi-put-by-offset-to-put-by-offset.js.dfg-eager
stress/fold-multi-put-by-offset-to-put-by-offset.js.dfg-eager-no-cjit-validate
stress/get-by-id-throw-from-getter-through-optimized-code.js.always-trigger-copy-phase
stress/get-by-id-throw-from-getter-through-optimized-code.js.default
stress/get-by-id-throw-from-getter-through-optimized-code.js.dfg-eager
stress/get-by-id-throw-from-getter-through-optimized-code.js.dfg-eager-no-cjit-validate
stress/get-by-id-throw-from-getter-through-optimized-code.js.no-cjit-validate-phases
stress/get-by-id-throw-from-getter-through-optimized-code.js.no-llint
stress/get-by-id-throw-from-unexpected-getter-through-optimized-code.js.always-trigger-copy-phase
stress/get-by-id-throw-from-unexpected-getter-through-optimized-code.js.default
stress/get-by-id-throw-from-unexpected-getter-through-optimized-code.js.dfg-eager
stress/get-by-id-throw-from-unexpected-getter-through-optimized-code.js.dfg-eager-no-cjit-validate
stress/get-by-id-throw-from-unexpected-getter-through-optimized-code.js.no-cjit-validate-phases
stress/get-by-id-throw-from-unexpected-getter-through-optimized-code.js.no-llint
stress/get-by-id-throw-from-unexpected-getter-through-optimized-code-that-does-not-exit.js.always-trigger-copy-phase
stress/get-by-id-throw-from-unexpected-getter-through-optimized-code-that-does-not-exit.js.default
stress/get-by-id-throw-from-unexpected-getter-through-optimized-code-that-does-not-exit.js.dfg-eager
stress/get-by-id-throw-from-unexpected-getter-through-optimized-code-that-does-not-exit.js.dfg-eager-no-cjit-validate
stress/get-by-id-throw-from-unexpected-getter-through-optimized-code-that-does-not-exit.js.no-cjit-validate-phases
stress/get-by-id-throw-from-unexpected-getter-through-optimized-code-that-does-not-exit.js.no-llint
stress/get-by-id-untyped.js.dfg-eager
stress/get-by-id-untyped.js.dfg-eager-no-cjit-validate
stress/getter.js.always-trigger-copy-phase
stress/getter.js.default
stress/getter.js.no-cjit-validate-phases
stress/getter.js.no-llint
stress/multi-get-by-offset-proto-and-self.js.no-llint
stress/multi-put-by-offset-reallocation-butterfly-cse.js.always-trigger-copy-phase
stress/multi-put-by-offset-reallocation-butterfly-cse.js.default
stress/multi-put-by-offset-reallocation-butterfly-cse.js.dfg-eager
stress/multi-put-by-offset-reallocation-butterfly-cse.js.dfg-eager-no-cjit-validate
stress/multi-put-by-offset-reallocation-butterfly-cse.js.no-cjit-validate-phases
stress/multi-put-by-offset-reallocation-butterfly-cse.js.no-llint
stress/multi-put-by-offset-reallocation-cases.js.always-trigger-copy-phase
stress/multi-put-by-offset-reallocation-cases.js.default
stress/multi-put-by-offset-reallocation-cases.js.dfg-eager
stress/multi-put-by-offset-reallocation-cases.js.dfg-eager-no-cjit-validate
stress/multi-put-by-offset-reallocation-cases.js.no-cjit-validate-phases
stress/multi-put-by-offset-reallocation-cases.js.no-llint
stress/prototype-getter.js.always-trigger-copy-phase
stress/prototype-getter.js.default
stress/prototype-getter.js.no-cjit-validate-phases
stress/prototype-getter.js.no-llint
stress/simple-polyvariant-get-by-id-inlining-example.js.always-trigger-copy-phase
stress/simple-polyvariant-get-by-id-inlining-example.js.default
stress/simple-polyvariant-get-by-id-inlining-example.js.no-cjit-validate-phases
stress/to-this-polymorphic.js.always-trigger-copy-phase
stress/to-this-polymorphic.js.default
stress/to-this-polymorphic.js.no-cjit-validate-phases
stress/to-this-polymorphic.js.no-llint
v8-v6/v8-crypto.js.dfg-eager
v8-v6/v8-crypto.js.dfg-eager-no-cjit-validate
v8-v6/v8-deltablue.js.always-trigger-copy-phase
v8-v6/v8-deltablue.js.default
v8-v6/v8-deltablue.js.dfg-eager
v8-v6/v8-deltablue.js.dfg-eager-no-cjit-validate
v8-v6/v8-deltablue.js.no-cjit-validate-phases
v8-v6/v8-deltablue.js.no-llint
v8-v6/v8-raytrace.js.always-trigger-copy-phase
v8-v6/v8-raytrace.js.default
v8-v6/v8-raytrace.js.dfg-eager
v8-v6/v8-raytrace.js.dfg-eager-no-cjit-validate
v8-v6/v8-raytrace.js.no-cjit-validate-phases
v8-v6/v8-raytrace.js.no-llint
v8-v6/v8-splay.js.always-trigger-copy-phase
v8-v6/v8-splay.js.default
v8-v6/v8-splay.js.dfg-eager
v8-v6/v8-splay.js.dfg-eager-no-cjit-validate
v8-v6/v8-splay.js.no-cjit-validate-phases
v8-v6/v8-splay.js.no-llint

[Csaba Osztrogonác]

Same regression occured on 32 bit Apple Mac build too.
But https://bugs.webkit.org/show_bug.cgi?id=130203
fixed 94 failures from the 110 , only 16 failures remains.

[Csaba Osztrogonác]

the remaining failures:

** The following JSC stress test failures have been introduced:
        mozilla-tests.yaml/js1_3/inherit/proto_10.js.mozilla-dfg-eager-no-cjit-validate-phases
        stress/get-by-id-throw-from-getter-through-optimized-code.js.default
        stress/get-by-id-throw-from-getter-through-optimized-code.js.no-llint
        stress/get-by-id-throw-from-getter-through-optimized-code.js.always-trigger-copy-phase
        stress/get-by-id-throw-from-getter-through-optimized-code.js.no-cjit-validate-phases
        stress/getter.js.default
        stress/getter.js.no-llint
        stress/getter.js.always-trigger-copy-phase
        stress/getter.js.no-cjit-validate-phases
        stress/prototype-getter.js.default
        stress/prototype-getter.js.no-llint
        stress/prototype-getter.js.always-trigger-copy-phase
        stress/prototype-getter.js.no-cjit-validate-phases
        jsc-layout-tests.yaml/js/script-tests/dfg-inline-arguments-use-from-all-the-places.js.layout
        jsc-layout-tests.yaml/js/script-tests/dfg-inline-arguments-use-from-all-the-places.js.layout-no-llint
        jsc-layout-tests.yaml/js/script-tests/dfg-inline-arguments-use-from-all-the-places.js.layout-no-cjit

Results for JSC stress tests:
    16 failures found.

[Csaba Osztrogonác]

Nowadays regressions occure on 32 bit regularly. 

Don't you want to make "Apple MountainLion Release (32-bit Build)"
bot to run jsc stress tests too?

(Unfortunately our ARMv7 tester is slow, jsc stress tests takes ~55 minutes.)

[Csaba Osztrogonác]

I tried reverting r165459 and its followup fix r165559,
and after it all jsc tests passed on 32 bit Mac.

[Mark Lam]

(In reply to comment #2)
> Results for JSC stress tests:
>     16 failures found.

Thanks for bringing this up.  I thought the tests were all passing, but I guess it regressed after I last checked.  I’ll look into it.

[Mark Lam]

<rdar://problem/16326745>

[Mark Lam]

I see a crasher in a LLINT slow path, but this was only because the slow path code was stepping on a bad JSValue.  The damage was done a long time ago by something else before getting here.

If I run with just the LLINT or LLINT + baslineJIT, all the tests run fine.  Hence,  the issue lies in the DFG.

[Mark Lam]

Created attachment 227208 [details]
the patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/jit/Repatch.cpp:233:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 2 files


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

[Mark Hahnenberg]

Comment on attachment 227208 [details]
the patch

I think Phil is working on this right now, so this might not be necessary.

[Filip Pizlo]

Comment on attachment 227208 [details]
the patch

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

> Source/JavaScriptCore/ChangeLog:16
> +        The new fix is to change generateGetByIdStub() to ensure that we aren't
> +        using the same register for baseGPR and resultTagGPR.  We'll move baseGPR
> +        to a new scratch register if needed.

This is just not the right approach at all.  The invariant of inline caching is that baseGPR is never resultTagGPR.  You can arrange to have the register allocator guarantee this.  That is a much simpler solution.

In fact, right now the register allocator *thinks* that it should be preventing baseGPR from aliasing resultGPR.  That's unnecessary.

Your solution adds a ton of complexity and makes it so that:

- The register allocator is working hard to prevent baseGPR from aliasing resultGPR even though Repatch is totally fine with it
- Repatch is working hard to allow baseGPR to alias resultTagGPR even though the register allocator could be told to prevent that

Let's just do a simpler fix that tells the register allocator to that resultTagGPR, not resultGPR, is the thing that baseGPR ought not alias.  I'm working on it now.

[Filip Pizlo]

Created attachment 227210 [details]
the patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:116:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:98:  The parameter name "codeBlock" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:98:  The parameter name "codeOrigin" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:99:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 6 files


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

[Mark Hahnenberg]

Comment on attachment 227210 [details]
the patch

r=me

[Filip Pizlo]

This new fix makes 32-bit pass, and it actually fixes the root cause, which was the DFG register allocator.

[Geoffrey Garen]

Comment on attachment 227210 [details]
the patch

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

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:421
> +            if (node->child1()->shouldSpeculateString() && node->child2()->shouldSpeculateString() && (GPRInfo::numberOfRegisters >= 7 || isFTL(m_graph.m_plan.mode))) {

Where does this 7 come from?

Does anything in the pipeline verify that we did this right?

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:452
>                  fixEdge<StringUse>(node->child1());

Same question re: 8.

[Geoffrey Garen]

> The invariant of inline caching is that baseGPR is never resultTagGPR.

Does anything check this invariant? Can we add a check for it?

[Filip Pizlo]

(In reply to comment #16)
> (From update of attachment 227210 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227210&action=review
> 
> > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:421
> > +            if (node->child1()->shouldSpeculateString() && node->child2()->shouldSpeculateString() && (GPRInfo::numberOfRegisters >= 7 || isFTL(m_graph.m_plan.mode))) {
> 
> Where does this 7 come from?
> 
> Does anything in the pipeline verify that we did this right?
> 
> > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:452
> >                  fixEdge<StringUse>(node->child1());
> 
> Same question re: 8.

You have to just count the number of registers that we allocate in the DFG::SpeculativeJIT code.  Yes, it can change.  And if it changes, you will get an immediate test failure.

[Filip Pizlo]

(In reply to comment #17)
> > The invariant of inline caching is that baseGPR is never resultTagGPR.
> 
> Does anything check this invariant? Can we add a check for it?

This patch adds a RELEASE_ASSERT() to that effect.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/165912