Bug 185149

Summary:

ToString constant folds without preserving checks, causing us to break assumptions that the code would OSR exit

Product:

WebKit

Reporter:

Saam Barati <saam>

Component:

JavaScriptCore

Assignee:

Saam Barati <saam>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

benjamin, commit-queue, ews-watchlist, fpizlo, ggaren, gskachkov, jfbastien, keith_miller, mark.lam, msaboff, rmorisset, ticaiolima, webkit-bug-importer, ysuzuki

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
patch	fpizlo: review+, ews-watchlist: commit-queue-
patch for landing	none

Saam Barati

Reported 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.

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
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Saam Barati

Comment 1 2018-04-30 16:49:32 PDT

<rdar://problem/39455917>

Saam Barati

Comment 2 2018-04-30 16:51:09 PDT

Created attachment 339162 [details] patch

EWS Watchlist

Comment 3 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

Saam Barati

Comment 4 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.

WebKit Commit Bot

Comment 5 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>

WebKit Commit Bot

Comment 6 2018-04-30 23:04:40 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.