Bug 139500 – PutById inline caching causing segfault in attached test.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Matthew Mirman]

The attached test fails when LLInt is on and thresholdForJITAfterWarmUp=100:

[Matthew Mirman]

Created attachment 243056 [details]
failing put-by-id resursive test

[Matthew Mirman]

Created attachment 243308 [details]
Fixes operationPutByIdOptimizes such that they save the structure before the put

[Geoffrey Garen]

Comment on attachment 243308 [details]
Fixes operationPutByIdOptimizes such that they save the structure before the put

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

Code looks good, but you should improve this layout test before landing it.

> Source/JavaScriptCore/tests/stress/put-by-id-build-list-order-recurse.js:15
> +    var str = "for (var i = " + currentCount + "; i < " + (100 + currentCount) + "; ++i)\n"
> +            + "    o.f\n";
> +    eval(str);

An incorrect read or write can sometimes survive without crashing. This test should explicitly check its result for some sensible value.

[Geoffrey Garen]

It looks like this crash is in the JIT. Why does it only happen when the LLInt is enabled?

[Matthew Mirman]

It looks like the major thing that was changing the occurrence of the bug was thresholdForJITAfterWarmUp.  Setting it to 100 causes this test specifically to crash, but I have been able to get smaller tests to crash with it set much lower (6).  Changing when the JIT is invoked changes when the inline cache gets rewritten.

[Matthew Mirman]

Comment on attachment 243308 [details]
Fixes operationPutByIdOptimizes such that they save the structure before the put

Committed r177380: <http://trac.webkit.org/changeset/177380>

[Matthew Mirman]

Reviewed patch landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 139707

[Matthew Mirman]

Created attachment 243702 [details]
Fixes operationPutByIdOptimizes such that they save the structure before the put

It appears as though there still might be some failing tests due to rdar://problem/19339106 so this isn't to be committed quite yet.

[Matthew Mirman]

Comment on attachment 243702 [details]
Fixes operationPutByIdOptimizes such that they save the structure before the put

Nvm the last comment.  run-javascript-tests works.

[Filip Pizlo]

Comment on attachment 243702 [details]
Fixes operationPutByIdOptimizes such that they save the structure before the put

r=me if there is no perf impact

[Matthew Mirman]

Created attachment 244546 [details]
Fixes operationPutByIdOptimizes such that they save the structure before the put

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/jit/Repatch.cpp:997:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/jit/Repatch.cpp:1003:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/Repatch.cpp:1007:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/jit/Repatch.cpp:1010:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/jit/Repatch.cpp:1012:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 5 in 10 files


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

[Matthew Mirman]

Created attachment 244549 [details]
Fixes operationPutByIdOptimizes such that they save the structure before the put

fixes some style from the last patch.

[Filip Pizlo]

Comment on attachment 244549 [details]
Fixes operationPutByIdOptimizes such that they save the structure before the put

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

> Source/JavaScriptCore/tests/mozilla/mozilla-tests.yaml:793
> +# - path: ecma/LexicalConventions/7.7.3.js
> +#   cmd: defaultRunMozillaTest :normal, "../shell.js"

What's going on here?

[Matthew Mirman]

Created attachment 244550 [details]
Fixes operationPutByIdOptimizes such that they save the structure before the put

[Filip Pizlo]

Comment on attachment 244550 [details]
Fixes operationPutByIdOptimizes such that they save the structure before the put

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

You should rebase this patch.

> Source/JavaScriptCore/tests/mozilla/mozilla-tests.yaml:793
> -- path: ecma/LexicalConventions/7.7.3.js
> -  cmd: defaultRunMozillaTest :normal, "../shell.js"
> +# - path: ecma/LexicalConventions/7.7.3.js
> +#   cmd: defaultRunMozillaTest :normal, "../shell.js"

You should put a comment above this referencing the bug.

[Matthew Mirman]

(In reply to comment #15)
> Comment on attachment 244549 [details]
> Fixes operationPutByIdOptimizes such that they save the structure before the
> put
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=244549&action=review
> 
> > Source/JavaScriptCore/tests/mozilla/mozilla-tests.yaml:793
> > +# - path: ecma/LexicalConventions/7.7.3.js
> > +#   cmd: defaultRunMozillaTest :normal, "../shell.js"
> 
> What's going on here?

<rdar://problem/19339106>

[Matthew Mirman]

Created attachment 244553 [details]
Fixes operationPutByIdOptimizes such that they save the structure before the put

[Filip Pizlo]

Comment on attachment 244553 [details]
Fixes operationPutByIdOptimizes such that they save the structure before the put

Can you rebase this?

[Matthew Mirman]

Created attachment 244625 [details]
Fixes operationPutByIdOptimizes such that they save the structure before the put

I accidentally included a change to a non existent file in the last patch.  This should be better.

[Matthew Mirman]

Comment on attachment 244625 [details]
Fixes operationPutByIdOptimizes such that they save the structure before the put

Committed r177380: <http://trac.webkit.org/changeset/178441>

[Matthew Mirman]

Reviewed patch landed.  Closing bug.