Bug 139500 - PutById inline caching causing segfault in attached test.
Summary: PutById inline caching causing segfault in attached test.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Matthew Mirman
URL:
Keywords:
Depends on: 139707
Blocks:
  Show dependency treegraph
 
Reported: 2014-12-10 11:58 PST by Matthew Mirman
Modified: 2015-01-14 13:28 PST (History)
4 users (show)

See Also:


Attachments
failing put-by-id resursive test (622 bytes, text/plain)
2014-12-10 12:01 PST, Matthew Mirman
no flags Details
Fixes operationPutByIdOptimizes such that they save the structure before the put (8.54 KB, patch)
2014-12-15 14:16 PST, Matthew Mirman
ggaren: review+
Details | Formatted Diff | Diff
Fixes operationPutByIdOptimizes such that they save the structure before the put (11.37 KB, patch)
2014-12-23 15:34 PST, Matthew Mirman
fpizlo: review+
Details | Formatted Diff | Diff
Fixes operationPutByIdOptimizes such that they save the structure before the put (28.51 KB, patch)
2015-01-13 14:12 PST, Matthew Mirman
no flags Details | Formatted Diff | Diff
Fixes operationPutByIdOptimizes such that they save the structure before the put (30.50 KB, patch)
2015-01-13 14:20 PST, Matthew Mirman
no flags Details | Formatted Diff | Diff
Fixes operationPutByIdOptimizes such that they save the structure before the put (28.96 KB, patch)
2015-01-13 14:35 PST, Matthew Mirman
fpizlo: review+
fpizlo: commit-queue-
Details | Formatted Diff | Diff
Fixes operationPutByIdOptimizes such that they save the structure before the put (29.00 KB, patch)
2015-01-13 15:07 PST, Matthew Mirman
fpizlo: commit-queue-
Details | Formatted Diff | Diff
Fixes operationPutByIdOptimizes such that they save the structure before the put (28.53 KB, patch)
2015-01-14 11:26 PST, Matthew Mirman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matthew Mirman 2014-12-10 11:58:04 PST
The attached test fails when LLInt is on and thresholdForJITAfterWarmUp=100:
Comment 1 Matthew Mirman 2014-12-10 12:01:38 PST
Created attachment 243056 [details]
failing put-by-id resursive test
Comment 2 Matthew Mirman 2014-12-15 14:16:58 PST
Created attachment 243308 [details]
Fixes operationPutByIdOptimizes such that they save the structure before the put
Comment 3 Geoffrey Garen 2014-12-15 16:30:37 PST
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.
Comment 4 Geoffrey Garen 2014-12-15 16:34:14 PST
It looks like this crash is in the JIT. Why does it only happen when the LLInt is enabled?
Comment 5 Matthew Mirman 2014-12-16 11:27:41 PST
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.
Comment 6 Matthew Mirman 2014-12-16 12:14:14 PST
Comment on attachment 243308 [details]
Fixes operationPutByIdOptimizes such that they save the structure before the put

Committed r177380: <http://trac.webkit.org/changeset/177380>
Comment 7 Matthew Mirman 2014-12-16 12:14:52 PST
Reviewed patch landed.  Closing bug.
Comment 8 WebKit Commit Bot 2014-12-16 15:02:04 PST
Re-opened since this is blocked by bug 139707
Comment 9 Matthew Mirman 2014-12-23 15:34:21 PST
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.
Comment 10 Matthew Mirman 2014-12-23 15:38:37 PST
Comment on attachment 243702 [details]
Fixes operationPutByIdOptimizes such that they save the structure before the put

Nvm the last comment.  run-javascript-tests works.
Comment 11 Filip Pizlo 2015-01-05 11:05:21 PST
Comment on attachment 243702 [details]
Fixes operationPutByIdOptimizes such that they save the structure before the put

r=me if there is no perf impact
Comment 12 Matthew Mirman 2015-01-13 14:12:39 PST
Created attachment 244546 [details]
Fixes operationPutByIdOptimizes such that they save the structure before the put
Comment 13 WebKit Commit Bot 2015-01-13 14:14:58 PST
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.
Comment 14 Matthew Mirman 2015-01-13 14:20:14 PST
Created attachment 244549 [details]
Fixes operationPutByIdOptimizes such that they save the structure before the put

fixes some style from the last patch.
Comment 15 Filip Pizlo 2015-01-13 14:24:01 PST
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?
Comment 16 Matthew Mirman 2015-01-13 14:35:34 PST
Created attachment 244550 [details]
Fixes operationPutByIdOptimizes such that they save the structure before the put
Comment 17 Filip Pizlo 2015-01-13 14:57:59 PST
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.
Comment 18 Matthew Mirman 2015-01-13 15:02:13 PST
(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>
Comment 19 Matthew Mirman 2015-01-13 15:07:24 PST
Created attachment 244553 [details]
Fixes operationPutByIdOptimizes such that they save the structure before the put
Comment 20 Filip Pizlo 2015-01-13 15:10:52 PST
Comment on attachment 244553 [details]
Fixes operationPutByIdOptimizes such that they save the structure before the put

Can you rebase this?
Comment 21 Matthew Mirman 2015-01-14 11:26:01 PST
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.
Comment 22 Matthew Mirman 2015-01-14 13:28:15 PST
Comment on attachment 244625 [details]
Fixes operationPutByIdOptimizes such that they save the structure before the put

Committed r177380: <http://trac.webkit.org/changeset/178441>
Comment 23 Matthew Mirman 2015-01-14 13:28:36 PST
Reviewed patch landed.  Closing bug.