Bug 130134

Summary: REGRESSION(r165459): It broke 109 jsc stress test on ARM Thumb2 and Mac 32 bit
Product: WebKit Reporter: Csaba Osztrogonác <ossy>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Critical CC: commit-queue, fpizlo, ggaren, mark.lam, mhahnenberg, mmirman, msaboff, oliver, ossy, rgabor, webkit-bug-importer, zherczeg
Priority: P1 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 108645, 129778    
Attachments:
Description Flags
the patch
mhahnenberg: review-
the patch mhahnenberg: review+

Description Csaba Osztrogonác 2014-03-12 09:20:54 PDT
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
Comment 1 Csaba Osztrogonác 2014-03-14 03:29:53 PDT
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.
Comment 2 Csaba Osztrogonác 2014-03-14 03:34:19 PDT
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.
Comment 3 Csaba Osztrogonác 2014-03-14 03:38:48 PDT
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.)
Comment 4 Csaba Osztrogonác 2014-03-14 04:08:31 PDT
I tried reverting r165459 and its followup fix r165559,
and after it all jsc tests passed on 32 bit Mac.
Comment 5 Mark Lam 2014-03-14 07:43:00 PDT
(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.
Comment 6 Mark Lam 2014-03-14 09:57:03 PDT
<rdar://problem/16326745>
Comment 7 Mark Lam 2014-03-14 13:19:56 PDT
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.
Comment 8 Mark Lam 2014-03-19 12:24:19 PDT
Created attachment 227208 [details]
the patch
Comment 9 WebKit Commit Bot 2014-03-19 12:25:13 PDT
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.
Comment 10 Mark Hahnenberg 2014-03-19 12:25:25 PDT
Comment on attachment 227208 [details]
the patch

I think Phil is working on this right now, so this might not be necessary.
Comment 11 Filip Pizlo 2014-03-19 12:28:54 PDT
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.
Comment 12 Filip Pizlo 2014-03-19 12:38:09 PDT
Created attachment 227210 [details]
the patch
Comment 13 WebKit Commit Bot 2014-03-19 12:39:16 PDT
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.
Comment 14 Mark Hahnenberg 2014-03-19 12:44:08 PDT
Comment on attachment 227210 [details]
the patch

r=me
Comment 15 Filip Pizlo 2014-03-19 12:44:40 PDT
This new fix makes 32-bit pass, and it actually fixes the root cause, which was the DFG register allocator.
Comment 16 Geoffrey Garen 2014-03-19 13:24:20 PDT
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.
Comment 17 Geoffrey Garen 2014-03-19 13:25:07 PDT
> The invariant of inline caching is that baseGPR is never resultTagGPR.

Does anything check this invariant? Can we add a check for it?
Comment 18 Filip Pizlo 2014-03-19 13:31:44 PDT
(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.
Comment 19 Filip Pizlo 2014-03-19 13:32:06 PDT
(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.
Comment 20 Filip Pizlo 2014-03-19 13:37:27 PDT
Landed in http://trac.webkit.org/changeset/165912