Bug 185149 - ToString constant folds without preserving checks, causing us to break assumptions that the code would OSR exit
Summary: ToString constant folds without preserving checks, causing us to break assump...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-04-30 16:09 PDT by Saam Barati
Modified: 2018-04-30 23:04 PDT (History)
14 users (show)

See Also:


Attachments
patch (5.18 KB, patch)
2018-04-30 16:51 PDT, Saam Barati
fpizlo: review+
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
patch for landing (5.76 KB, patch)
2018-04-30 21:39 PDT, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2018-04-30 16:09:19 PDT
For example, we transform code like:

a: DoubleConstant(1.0)
b: ToString(Check:Int32:@a)
c: Unreachable

to

b: LazyJSConstant
c: Unreachable

Where we no longer exit in the latter case, causing us to crash.
Comment 1 Saam Barati 2018-04-30 16:49:32 PDT
<rdar://problem/39455917>
Comment 2 Saam Barati 2018-04-30 16:51:09 PDT
Created attachment 339162 [details]
patch
Comment 3 EWS Watchlist 2018-04-30 18:39:15 PDT
Comment on attachment 339162 [details]
patch

Attachment 339162 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/7517368

New failing tests:
microbenchmarks/string-replace.js.ftl-eager-no-cjit-b3o1
microbenchmarks/v8-regexp-search.js.ftl-no-cjit-no-put-stack-validate
stress/string-replace-constant-folding-replacer-not-string.js.ftl-no-cjit-no-put-stack-validate
v8-v6/v8-regexp.js.ftl-no-cjit-no-inline-validate
microbenchmarks/string-replace.js.ftl-no-cjit-no-put-stack-validate
microbenchmarks/v8-regexp-search.js.dfg-maximal-flush-validate-no-cjit
microbenchmarks/v8-regexp-search.js.ftl-no-cjit-no-inline-validate
microbenchmarks/string-replace-empty.js.dfg-maximal-flush-validate-no-cjit
v8-v6/v8-regexp.js.dfg-maximal-flush-validate-no-cjit
microbenchmarks/string-replace-empty.js.ftl-no-cjit-no-inline-validate
v8-v6/v8-regexp.js.no-cjit-validate-phases
microbenchmarks/v8-regexp-search.js.dfg-eager-no-cjit-validate
microbenchmarks/string-replace-empty.js.ftl-eager-no-cjit-b3o1
microbenchmarks/v8-regexp-search.js.ftl-eager-no-cjit-b3o1
microbenchmarks/string-replace-empty.js.dfg-eager-no-cjit-validate
stress/v8-regexp-strict.js.dfg-maximal-flush-validate-no-cjit
stress/v8-regexp-strict.js.ftl-no-cjit-no-put-stack-validate
stress/v8-regexp-strict.js.ftl-no-cjit-validate-sampling-profiler
v8-v6/v8-regexp.js.ftl-eager-no-cjit
microbenchmarks/string-replace-empty.js.ftl-eager-no-cjit
microbenchmarks/string-replace.js.dfg-maximal-flush-validate-no-cjit
stress/v8-regexp-strict.js.no-cjit-validate-phases
microbenchmarks/string-replace.js.no-cjit-validate-phases
v8-v6/v8-regexp.js.ftl-no-cjit-no-put-stack-validate
stress/v8-regexp-strict.js.ftl-no-cjit-no-inline-validate
stress/string-replace-constant-folding-replacer-not-string.js.no-cjit-validate-phases
microbenchmarks/string-replace.js.ftl-no-cjit-no-inline-validate
stress/string-replace-constant-folding-replacer-not-string.js.dfg-maximal-flush-validate-no-cjit
stress/v8-regexp-strict.js.dfg-eager-no-cjit-validate
microbenchmarks/v8-regexp-search.js.ftl-eager-no-cjit
microbenchmarks/string-replace-empty.js.no-cjit-validate-phases
microbenchmarks/string-replace.js.ftl-no-cjit-validate-sampling-profiler
stress/v8-regexp-strict.js.ftl-eager-no-cjit-b3o1
microbenchmarks/string-replace.js.ftl-eager-no-cjit
microbenchmarks/string-replace-empty.js.ftl-no-cjit-validate-sampling-profiler
stress/string-replace-constant-folding-replacer-not-string.js.ftl-no-cjit-no-inline-validate
stress/v8-regexp-strict.js.ftl-eager-no-cjit
microbenchmarks/string-replace-empty.js.ftl-no-cjit-no-put-stack-validate
microbenchmarks/string-replace.js.dfg-eager-no-cjit-validate
microbenchmarks/v8-regexp-search.js.ftl-no-cjit-validate-sampling-profiler
stress/string-replace-constant-folding-replacer-not-string.js.ftl-no-cjit-validate-sampling-profiler
v8-v6/v8-regexp.js.dfg-eager-no-cjit-validate
microbenchmarks/v8-regexp-search.js.no-cjit-validate-phases
v8-v6/v8-regexp.js.ftl-eager-no-cjit-b3o1
v8-v6/v8-regexp.js.ftl-no-cjit-validate-sampling-profiler
Comment 4 Saam Barati 2018-04-30 21:39:40 PDT
Created attachment 339182 [details]
patch for landing

It appears I made tests fail when I converted one of the callsities. I switched that back to the non-check emitting version. However, I saw conversion also didn't preserve checks, so I added for it to retain its checks.
Comment 5 WebKit Commit Bot 2018-04-30 23:04:39 PDT
Comment on attachment 339182 [details]
patch for landing

Clearing flags on attachment: 339182

Committed r231193: <https://trac.webkit.org/changeset/231193>
Comment 6 WebKit Commit Bot 2018-04-30 23:04:40 PDT
All reviewed patches have been landed.  Closing bug.