Bug 174924

Summary:

Strings need to be in some kind of gigacage

Product:

WebKit

Reporter:

Filip Pizlo <fpizlo>

Component:

JavaScriptCore

Assignee:

Filip Pizlo <fpizlo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

achristensen, buildbot, ggaren, jlewis3, kling, oliver, rniwa, webkit-bug-importer, ysuzuki

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

See Also:

https://bugs.webkit.org/show_bug.cgi?id=185218

Bug Depends on:

174919

Bug Blocks:

174917

Attachments:

Description	Flags
the patch	buildbot: commit-queue-
Archive of layout-test-results from ews123 for ios-simulator-wk2	none
Archive of layout-test-results from ews106 for mac-elcapitan-wk2	none
Archive of layout-test-results from ews105 for mac-elcapitan-wk2	none
Archive of layout-test-results from ews102 for mac-elcapitan	none
the problem with the last patch was that it wasn't big enough	none
more	none
more	buildbot: commit-queue-
the patch	none
the patch	none
the patch	none
the patch	none
the patch	none
the patch	none
the patch	none
the patch	none
the ptach	oliver: review+

Filip Pizlo

Reported 2017-07-27 19:47:02 PDT

...

Attachments
the patch (26.38 KB, patch) 2017-08-14 15:24 PDT, Filip Pizlo	buildbot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (737.80 KB, application/zip) 2017-08-14 17:39 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (986.67 KB, application/zip) 2017-08-14 18:06 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (1.06 MB, application/zip) 2017-08-14 19:53 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews102 for mac-elcapitan (666.55 KB, application/zip) 2017-08-15 05:14 PDT, Build Bot	no flags	Details
the problem with the last patch was that it wasn't big enough (69.62 KB, patch) 2017-08-15 13:40 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
more (74.75 KB, patch) 2017-08-18 16:26 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
more (85.21 KB, patch) 2017-08-22 12:10 PDT, Filip Pizlo	buildbot: commit-queue-	Details Formatted Diff Diff
the patch (91.11 KB, patch) 2017-08-22 13:16 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch (88.62 KB, patch) 2017-08-22 13:26 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch (89.85 KB, patch) 2017-08-22 13:59 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch (94.41 KB, patch) 2017-08-22 15:22 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch (96.92 KB, patch) 2017-08-22 18:20 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch (96.96 KB, patch) 2017-08-22 18:56 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch (96.53 KB, patch) 2017-08-22 20:09 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch (98.00 KB, patch) 2017-08-23 18:25 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
the ptach (99.48 KB, patch) 2017-08-29 15:56 PDT, Filip Pizlo	oliver: review+	Details Formatted Diff Diff
Show Obsolete (16) View All Add attachment proposed patch, testcase, etc.

Filip Pizlo

Comment 1 2017-08-13 15:58:43 PDT

I think that this means that the StringImpl objects themselves have to be allocated in the gigacage, which implies that StaticStringImpl has to be somewhat rethought. I think we can make StaticStringImpl lazily create a StringImpl inside the gigacage.

Filip Pizlo

Comment 2 2017-08-13 16:39:30 PDT

Actually, we need to be careful about which cage we put strings into, since that means allocating StringImpl's along with StringImpl::m_data in that cage. So, it would only make sense if StringImpl::m_data never pointed out of the cage. Otherwise, we cannot cage m_data, so primitive writes intended for other objects in the primitive cage could be used to redirect m_data. We cannot cage m_data without disabling the no-copy optimizations in things like ASCIILiteral. One way to get some of the benefit of caging is to allocate StringImpls and their backing stores in a string cage whenever possible, but don't cage accesses. This would mean that you could use string reads to explore the heap, but only after you construct a bogus string pointer. Alternatively, we could eat the perf cost of disabling the no-copy optimization in ASCIILiteral and introduce some lazy allocation in StaticStringImpl. In that world, we can put StringImpl and its backing stores in either their own cage or the primitive cage. The m_data pointer would stop being a real pointer, since it would be caged. So, it could be scrambled. It seems like the most secure situation would be one where we eat the perf cost and also give strings their own cage.

Filip Pizlo

Comment 3 2017-08-14 09:58:21 PDT

I think I'm going to go for this approach for now: - Strings get their own gigacage. - Accesses to strings are not caged.

Filip Pizlo

Comment 4 2017-08-14 15:24:13 PDT

Created attachment 318071 [details] the patch

Build Bot

Comment 5 2017-08-14 15:26:49 PDT

Attachment 318071 [details] did not pass style-queue: ERROR: Source/bmalloc/bmalloc/Gigacage.cpp:39: g_stringGigacageBasePtr is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/JavaScriptCore/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] ERROR: Source/WTF/wtf/text/StringBuffer.h:33: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WTF/wtf/text/StringImpl.h:182: More than one command on the same line [whitespace/newline] [4] Total errors found: 4 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.

Build Bot

Comment 6 2017-08-14 17:29:11 PDT

Comment on attachment 318071 [details] the patch Attachment 318071 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/4314528 New failing tests: stress/encode-decode-ascii.js.no-llint stress/escape-unescape-surrogates.js.dfg-maximal-flush-validate-no-cjit stress/escape-unescape-surrogates.js.ftl-eager-no-cjit stress/encode-decode-uri-component-surrogates.js.default stress/encode-decode-uri-component-surrogates.js.no-llint stress/encode-decode-uri-surrogates.js.ftl-eager stress/escape-unescape-surrogates.js.dfg-eager stress/encode-decode-unicode.js.ftl-eager stress/encode-decode-uri-surrogates.js.no-ftl stress/encode-decode-uri-surrogates.js.ftl-no-cjit-b3o1 stress/encode-decode-ascii.js.dfg-eager-no-cjit-validate stress/encode-decode-unicode.js.dfg-eager-no-cjit-validate stress/encode-decode-uri-surrogates.js.no-llint microbenchmarks/string-transcoding.js.ftl-eager stress/encode-decode-uri-component-surrogates.js.ftl-no-cjit-validate-sampling-profiler stress/encode-decode-ascii.js.default stress/encode-decode-ascii.js.dfg-maximal-flush-validate-no-cjit stress/encode-decode-ascii.js.ftl-no-cjit-no-inline-validate stress/encode-decode-uri-component-surrogates.js.ftl-no-cjit-small-pool stress/encode-decode-unicode.js.dfg-eager stress/encode-decode-unicode.js.ftl-eager-no-cjit stress/escape-unescape-surrogates.js.no-ftl stress/escape-unescape-surrogates.js.no-cjit-validate-phases microbenchmarks/string-transcoding.js.ftl-eager-no-cjit stress/encode-decode-uri-surrogates.js.no-cjit-validate-phases stress/escape-unescape-surrogates.js.ftl-no-cjit-no-put-stack-validate stress/encode-decode-uri-surrogates.js.dfg-maximal-flush-validate-no-cjit stress/encode-decode-ascii.js.ftl-no-cjit-small-pool stress/encode-decode-uri-component-surrogates.js.ftl-no-cjit-no-put-stack-validate stress/encode-decode-uri-component-surrogates.js.no-cjit-collect-continuously stress/encode-decode-ascii.js.ftl-no-cjit-no-put-stack-validate microbenchmarks/string-transcoding.js.dfg-eager-no-cjit-validate stress/encode-decode-uri-surrogates.js.dfg-eager-no-cjit-validate stress/escape-unescape-surrogates.js.default stress/escape-unescape-surrogates.js.no-llint stress/encode-decode-uri-surrogates.js.ftl-eager-no-cjit-b3o1 stress/encode-decode-ascii.js.no-cjit-validate-phases stress/encode-decode-ascii.js.ftl-eager stress/encode-decode-uri-component-surrogates.js.dfg-eager stress/encode-decode-uri-surrogates.js.no-cjit-collect-continuously stress/encode-decode-uri-component-surrogates.js.ftl-no-cjit-no-inline-validate microbenchmarks/string-transcoding.js.dfg-eager stress/encode-decode-ascii.js.ftl-eager-no-cjit microbenchmarks/string-transcoding.js.no-cjit-collect-continuously stress/escape-unescape-surrogates.js.ftl-no-cjit-validate-sampling-profiler stress/encode-decode-unicode.js.no-cjit-collect-continuously stress/encode-decode-ascii.js.ftl-no-cjit-validate-sampling-profiler stress/encode-decode-uri-surrogates.js.default stress/escape-unescape-surrogates.js.ftl-no-cjit-small-pool stress/escape-unescape-surrogates.js.ftl-eager-no-cjit-b3o1 stress/encode-decode-uri-component-surrogates.js.ftl-eager stress/encode-decode-uri-surrogates.js.ftl-no-cjit-validate-sampling-profiler stress/encode-decode-uri-component-surrogates.js.ftl-eager-no-cjit stress/encode-decode-uri-component-surrogates.js.no-ftl stress/encode-decode-uri-surrogates.js.ftl-no-cjit-no-put-stack-validate stress/encode-decode-uri-surrogates.js.ftl-eager-no-cjit stress/encode-decode-uri-component-surrogates.js.ftl-eager-no-cjit-b3o1 stress/encode-decode-ascii.js.dfg-eager stress/encode-decode-ascii.js.ftl-no-cjit-b3o1 stress/encode-decode-uri-component-surrogates.js.ftl-no-cjit-b3o1 stress/encode-decode-uri-component-surrogates.js.no-cjit-validate-phases stress/encode-decode-uri-surrogates.js.ftl-no-cjit-small-pool stress/encode-decode-uri-component-surrogates.js.dfg-maximal-flush-validate-no-cjit stress/escape-unescape-surrogates.js.dfg-eager-no-cjit-validate stress/encode-decode-uri-surrogates.js.ftl-no-cjit-no-inline-validate stress/encode-decode-uri-component-surrogates.js.dfg-eager-no-cjit-validate stress/escape-unescape-surrogates.js.ftl-no-cjit-no-inline-validate stress/escape-unescape-surrogates.js.ftl-no-cjit-b3o1 stress/encode-decode-ascii.js.no-ftl stress/escape-unescape-surrogates.js.ftl-eager stress/escape-unescape-surrogates.js.no-cjit-collect-continuously stress/encode-decode-ascii.js.no-cjit-collect-continuously stress/encode-decode-uri-surrogates.js.dfg-eager

Build Bot

Comment 7 2017-08-14 17:39:23 PDT

Comment on attachment 318071 [details] the patch Attachment 318071 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4314654 Number of test failures exceeded the failure limit.

Build Bot

Comment 8 2017-08-14 17:39:24 PDT

Created attachment 318094 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5

Build Bot

Comment 9 2017-08-14 18:06:05 PDT

Comment on attachment 318071 [details] the patch Attachment 318071 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4314450 Number of test failures exceeded the failure limit.

Build Bot

Comment 10 2017-08-14 18:06:07 PDT

Created attachment 318096 [details] Archive of layout-test-results from ews106 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6

Build Bot

Comment 11 2017-08-14 19:53:29 PDT

Comment on attachment 318071 [details] the patch Attachment 318071 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4315094 Number of test failures exceeded the failure limit.

Build Bot

Comment 12 2017-08-14 19:53:30 PDT

Created attachment 318102 [details] Archive of layout-test-results from ews105 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6

Build Bot

Comment 13 2017-08-15 05:14:41 PDT

Comment on attachment 318071 [details] the patch Attachment 318071 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4315583 Number of test failures exceeded the failure limit.

Build Bot

Comment 14 2017-08-15 05:14:43 PDT

Created attachment 318117 [details] Archive of layout-test-results from ews102 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Filip Pizlo

Comment 15 2017-08-15 13:40:25 PDT

Created attachment 318164 [details] the problem with the last patch was that it wasn't big enough This one isn't big enough yet, either.

Filip Pizlo

Comment 16 2017-08-18 16:26:46 PDT

Created attachment 318555 [details] more

Filip Pizlo

Comment 17 2017-08-22 12:10:21 PDT

Created attachment 318780 [details] more Fixed a bunch of things.

Build Bot

Comment 18 2017-08-22 12:12:22 PDT

Attachment 318780 [details] did not pass style-queue: ERROR: Source/WTF/wtf/text/StringImpl.h:183: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] ERROR: Source/WTF/wtf/text/StringBuffer.h:33: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/bmalloc/bmalloc/Gigacage.cpp:39: g_stringGigacageBasePtr is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/JavaScriptCore/runtime/StringConstructor.cpp:76: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/runtime/StringConstructor.cpp:77: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/runtime/StringConstructor.cpp:85: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/runtime/StringConstructor.cpp:88: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:679: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:680: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:681: Should have a space between // and comment [whitespace/comments] [4] Total errors found: 11 in 33 files If any of these errors are false positives, please file a bug against check-webkit-style.

Build Bot

Comment 19 2017-08-22 12:57:49 PDT

Comment on attachment 318780 [details] more Attachment 318780 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/4362757 New failing tests: jsc-layout-tests.yaml/fast/regex/script-tests/parentheses.js.layout-no-cjit mozilla-tests.yaml/ecma_2/String/split-003.js.mozilla-dfg-eager-no-cjit-validate-phases stress/to-lower-case-intrinsic-on-empty-rope.js.dfg-eager-no-cjit-validate wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-slow-memory stress/template-literal.js.ftl-no-cjit-b3o1 jsc-layout-tests.yaml/fast/regex/script-tests/ecma-regex-examples.js.layout stress/to-lower-case-intrinsic-on-empty-rope.js.dfg-maximal-flush-validate-no-cjit wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.default-wasm stress/string-compare.js.no-ftl stress/big-split.js.ftl-no-cjit-validate-sampling-profiler jsc-layout-tests.yaml/fast/regex/script-tests/dotstar.js.layout-no-llint stress/template-literal.js.ftl-no-cjit-validate-sampling-profiler jsc-layout-tests.yaml/fast/regex/script-tests/ecma-regex-examples.js.layout-ftl-no-cjit wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-eager-jettison stress/big-split.js.no-ftl stress/big-split.js.dfg-eager mozilla-tests.yaml/js1_2/regexp/RegExp_lastParen.js.mozilla-baseline jsc-layout-tests.yaml/js/script-tests/regexp-unicode.js.layout-no-cjit mozilla-tests.yaml/ecma_2/String/split-003.js.mozilla stress/string-compare.js.no-llint mozilla-tests.yaml/ecma_2/String/split-003.js.mozilla-no-ftl jsc-layout-tests.yaml/fast/regex/script-tests/dotstar.js.layout-dfg-eager-no-cjit jsc-layout-tests.yaml/fast/regex/script-tests/ecma-regex-examples.js.layout-no-cjit stress/big-split-captures.js.ftl-eager jsc-layout-tests.yaml/fast/regex/script-tests/parentheses.js.layout-no-llint wasm.yaml/wasm/function-tests/memory-alignment.js.wasm-no-cjit-yes-tls-context mozilla-tests.yaml/ecma_2/String/split-003.js.mozilla-llint jsc-layout-tests.yaml/fast/regex/script-tests/dotstar.js.layout jsc-layout-tests.yaml/fast/regex/script-tests/pcre-test-1.js.layout-ftl-eager-no-cjit stress/big-split.js.ftl-no-cjit-b3o1 stress/template-literal.js.ftl-eager-no-cjit jsc-layout-tests.yaml/fast/regex/script-tests/ecma-regex-examples.js.layout-no-llint jsc-layout-tests.yaml/fast/regex/script-tests/parentheses.js.layout stress/tagged-templates.js.dfg-eager-no-cjit-validate stress/big-split-captures.js.default stress/big-split-captures.js.no-llint stress/template-literal.js.default jsc-layout-tests.yaml/fast/regex/script-tests/pcre-test-4.js.layout-ftl-no-cjit jsc-layout-tests.yaml/fast/regex/script-tests/pcre-test-1.js.layout stress/big-split-captures.js.ftl-eager-no-cjit jsc-layout-tests.yaml/fast/regex/script-tests/pcre-test-4.js.layout-no-ftl stress/big-split-captures.js.ftl-no-cjit-no-put-stack-validate stress/string-compare.js.no-cjit-validate-phases stress/big-split-captures.js.dfg-eager-no-cjit-validate stress/big-split.js.ftl-no-cjit-no-inline-validate jsc-layout-tests.yaml/fast/regex/script-tests/assertion.js.layout-ftl-no-cjit stress/template-literal.js.no-llint stress/big-split-captures.js.ftl-eager-no-cjit-b3o1 stress/big-split-captures.js.ftl-no-cjit-b3o1 jsc-layout-tests.yaml/js/script-tests/regexp-unicode.js.layout-no-ftl stress/string-compare.js.dfg-eager jsc-layout-tests.yaml/fast/regex/script-tests/parentheses.js.layout-ftl-eager-no-cjit mozilla-tests.yaml/js1_2/regexp/RegExp_lastParen_as_array.js.mozilla-llint stress/template-literal.js.dfg-maximal-flush-validate-no-cjit stress/big-split-captures.js.dfg-maximal-flush-validate-no-cjit stress/big-split-captures.js.ftl-no-cjit-small-pool jsc-layout-tests.yaml/js/script-tests/regexp-unicode.js.layout-ftl-no-cjit jsc-layout-tests.yaml/js/script-tests/regexp-unicode.js.layout-dfg-eager-no-cjit wasm.yaml/wasm/function-tests/memory-alignment.js.wasm-eager-jettison stress/string-compare.js.no-cjit-collect-continuously stress/big-split.js.ftl-no-cjit-small-pool stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-eager-no-cjit-b3o1 jsc-layout-tests.yaml/js/script-tests/regexp-unicode.js.layout-ftl-eager-no-cjit stress/big-split-captures.js.ftl-no-cjit-no-inline-validate stress/tagged-templates.js.ftl-eager-no-cjit-b3o1 stress/template-literal.js.no-ftl jsc-layout-tests.yaml/fast/regex/script-tests/assertion.js.layout-no-llint stress/template-literal.js.ftl-no-cjit-no-put-stack-validate stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-no-cjit-b3o1 stress/big-split.js.ftl-eager-no-cjit-b3o1 mozilla-tests.yaml/js1_2/regexp/RegExp_lastParen.js.mozilla-no-ftl jsc-layout-tests.yaml/fast/regex/script-tests/parentheses.js.layout-no-ftl stress/big-split.js.no-llint stress/string-compare.js.ftl-no-cjit-small-pool jsc-layout-tests.yaml/fast/regex/script-tests/pcre-test-1.js.layout-no-ftl stress/template-literal.js.ftl-no-cjit-small-pool stress/big-split-captures.js.dfg-eager jsc-layout-tests.yaml/fast/regex/script-tests/dotstar.js.layout-no-cjit mozilla-tests.yaml/ecma_2/String/split-003.js.mozilla-baseline stress/big-split.js.dfg-eager-no-cjit-validate jsc-layout-tests.yaml/fast/regex/script-tests/pcre-test-1.js.layout-no-llint stress/big-split.js.ftl-eager mozilla-tests.yaml/js1_2/regexp/RegExp_lastParen.js.mozilla-llint stress/string-compare.js.dfg-maximal-flush-validate-no-cjit wasm.yaml/wasm/function-tests/memory-alignment.js.wasm-no-tls-context stress/template-literal.js.dfg-eager stress/to-lower-case-intrinsic-on-empty-rope.js.no-cjit-validate-phases stress/template-literal.js.ftl-no-cjit-no-inline-validate stress/big-split-captures.js.no-cjit-collect-continuously stress/template-literal.js.dfg-eager-no-cjit-validate wasm.yaml/wasm/function-tests/memory-alignment.js.wasm-slow-memory stress/to-lower-case-intrinsic-on-empty-rope.js.no-cjit-collect-continuously mozilla-tests.yaml/js1_2/regexp/RegExp_lastParen.js.mozilla jsc-layout-tests.yaml/fast/regex/script-tests/pcre-test-4.js.layout-no-llint wasm.yaml/wasm/function-tests/memory-alignment.js.default-wasm mozilla-tests.yaml/js1_2/regexp/RegExp_lastParen_as_array.js.mozilla-no-ftl stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-no-cjit-small-pool stress/big-split.js.dfg-maximal-flush-validate-no-cjit stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-eager-no-cjit jsc-layout-tests.yaml/fast/regex/script-tests/pcre-test-4.js.layout-no-cjit jsc-layout-tests.yaml/fast/regex/script-tests/parentheses.js.layout-ftl-no-cjit stress/big-split-captures.js.ftl-no-cjit-validate-sampling-profiler jsc-layout-tests.yaml/fast/regex/script-tests/assertion.js.layout-no-cjit mozilla-tests.yaml/js1_2/regexp/RegExp_lastParen_as_array.js.mozilla-baseline stress/to-lower-case-intrinsic-on-empty-rope.js.no-llint jsc-layout-tests.yaml/fast/regex/script-tests/pcre-test-1.js.layout-dfg-eager-no-cjit jsc-layout-tests.yaml/fast/regex/script-tests/dotstar.js.layout-no-ftl wasm.yaml/wasm/function-tests/memory-alignment.js.wasm-no-call-ic stress/string-compare.js.ftl-no-cjit-no-inline-validate jsc-layout-tests.yaml/js/script-tests/regexp-unicode.js.layout jsc-layout-tests.yaml/js/script-tests/regexp-unicode.js.layout-no-llint jsc-layout-tests.yaml/fast/regex/script-tests/dotstar.js.layout-ftl-no-cjit wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-call-ic stress/template-literal.js.ftl-eager-no-cjit-b3o1 stress/template-literal.js.ftl-eager stress/to-lower-case-intrinsic-on-empty-rope.js.dfg-eager stress/template-literal.js.no-cjit-collect-continuously stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-no-cjit-no-inline-validate stress/big-split.js.ftl-eager-no-cjit mozilla-tests.yaml/ecma_2/String/split-003.js.mozilla-ftl-eager-no-cjit-validate-phases jsc-layout-tests.yaml/fast/regex/script-tests/pcre-test-1.js.layout-no-cjit stress/big-split.js.ftl-no-cjit-no-put-stack-validate stress/string-compare.js.default jsc-layout-tests.yaml/fast/regex/script-tests/pcre-test-1.js.layout-ftl-no-cjit stress/big-split-captures.js.no-cjit-validate-phases stress/tagged-templates.js.ftl-eager-no-cjit stress/string-compare.js.ftl-eager-no-cjit jsc-layout-tests.yaml/fast/regex/script-tests/ecma-regex-examples.js.layout-no-ftl stress/template-literal.js.no-cjit-validate-phases stress/to-lower-case-intrinsic-on-empty-rope.js.default jsc-layout-tests.yaml/fast/regex/script-tests/pcre-test-4.js.layout stress/to-lower-case-intrinsic-on-empty-rope.js.no-ftl stress/string-compare.js.ftl-no-cjit-b3o1 jsc-layout-tests.yaml/fast/regex/script-tests/assertion.js.layout wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-cjit-yes-tls-context stress/string-compare.js.ftl-eager stress/big-split.js.no-cjit-collect-continuously stress/string-compare.js.dfg-eager-no-cjit-validate stress/string-compare.js.ftl-eager-no-cjit-b3o1 jsc-layout-tests.yaml/fast/regex/script-tests/assertion.js.layout-no-ftl stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-eager stress/big-split-captures.js.no-ftl jsc-layout-tests.yaml/fast/regex/script-tests/parentheses.js.layout-dfg-eager-no-cjit wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-tls-context jsc-layout-tests.yaml/fast/regex/script-tests/pcre-test-4.js.layout-dfg-eager-no-cjit stress/big-split.js.no-cjit-validate-phases stress/string-compare.js.ftl-no-cjit-no-put-stack-validate stress/big-split.js.default stress/string-compare.js.ftl-no-cjit-validate-sampling-profiler stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-no-cjit-validate-sampling-profiler mozilla-tests.yaml/js1_2/regexp/RegExp_lastParen_as_array.js.mozilla stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-no-cjit-no-put-stack-validate

Filip Pizlo

Comment 20 2017-08-22 13:16:37 PDT

Created attachment 318785 [details] the patch

Build Bot

Comment 21 2017-08-22 13:20:01 PDT

Attachment 318785 [details] did not pass style-queue: ERROR: Source/WTF/wtf/text/StringImpl.h:183: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WTF/wtf/text/StringBuffer.h:33: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/bmalloc/bmalloc/Gigacage.cpp:39: g_stringGigacageBasePtr is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/JavaScriptCore/runtime/StringConstructor.cpp:76: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/runtime/StringConstructor.cpp:77: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/runtime/StringConstructor.cpp:85: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/runtime/StringConstructor.cpp:88: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:679: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:680: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:681: Should have a space between // and comment [whitespace/comments] [4] Total errors found: 10 in 33 files If any of these errors are false positives, please file a bug against check-webkit-style.

Filip Pizlo

Comment 22 2017-08-22 13:26:54 PDT

Created attachment 318787 [details] the patch Removed some debugging code

Build Bot

Comment 23 2017-08-22 13:28:10 PDT

Attachment 318787 [details] did not pass style-queue: ERROR: Source/WTF/wtf/text/StringImpl.h:183: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WTF/wtf/text/StringBuffer.h:33: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/bmalloc/bmalloc/Gigacage.cpp:39: g_stringGigacageBasePtr is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 3 in 31 files If any of these errors are false positives, please file a bug against check-webkit-style.

Filip Pizlo

Comment 24 2017-08-22 13:59:58 PDT

Created attachment 318794 [details] the patch

Build Bot

Comment 25 2017-08-22 14:25:07 PDT

Attachment 318794 [details] did not pass style-queue: ERROR: Source/WTF/wtf/text/StringImpl.h:183: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WTF/wtf/text/StringBuffer.h:33: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/bmalloc/bmalloc/Gigacage.cpp:39: g_stringGigacageBasePtr is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 3 in 33 files If any of these errors are false positives, please file a bug against check-webkit-style.

Filip Pizlo

Comment 26 2017-08-22 15:22:54 PDT

Created attachment 318808 [details] the patch Fixing WebCore's uses of String::adopt

Build Bot

Comment 27 2017-08-22 15:25:04 PDT

Attachment 318808 [details] did not pass style-queue: ERROR: Source/WTF/wtf/text/StringImpl.h:183: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WTF/wtf/text/StringBuffer.h:33: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/bmalloc/bmalloc/Gigacage.cpp:39: g_stringGigacageBasePtr is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 3 in 40 files If any of these errors are false positives, please file a bug against check-webkit-style.

Filip Pizlo

Comment 28 2017-08-22 18:20:57 PDT

Created attachment 318841 [details] the patch

Build Bot

Comment 29 2017-08-22 18:22:59 PDT

Attachment 318841 [details] did not pass style-queue: ERROR: Source/WTF/wtf/text/StringImpl.h:183: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WTF/wtf/text/StringBuffer.h:33: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/bmalloc/bmalloc/Gigacage.cpp:39: g_stringGigacageBasePtr is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 3 in 41 files If any of these errors are false positives, please file a bug against check-webkit-style.

Filip Pizlo

Comment 30 2017-08-22 18:56:31 PDT

Created attachment 318845 [details] the patch Trying to fix GTK

Build Bot

Comment 31 2017-08-22 18:59:22 PDT

Attachment 318845 [details] did not pass style-queue: ERROR: Source/WTF/wtf/text/StringImpl.h:183: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WTF/wtf/text/StringBuffer.h:33: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/bmalloc/bmalloc/Gigacage.cpp:39: g_stringGigacageBasePtr is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 3 in 41 files If any of these errors are false positives, please file a bug against check-webkit-style.

Filip Pizlo

Comment 32 2017-08-22 20:09:10 PDT

Created attachment 318848 [details] the patch try again to fix gtk

Build Bot

Comment 33 2017-08-22 20:10:54 PDT

Attachment 318848 [details] did not pass style-queue: ERROR: Source/WTF/wtf/text/StringImpl.h:183: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WTF/wtf/text/StringBuffer.h:33: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/bmalloc/bmalloc/Gigacage.cpp:39: g_stringGigacageBasePtr is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 3 in 40 files If any of these errors are false positives, please file a bug against check-webkit-style.

Filip Pizlo

Comment 34 2017-08-23 18:25:19 PDT

Created attachment 318953 [details] the patch

Build Bot

Comment 35 2017-08-23 18:28:32 PDT

Attachment 318953 [details] did not pass style-queue: ERROR: Source/WTF/wtf/text/StringImpl.h:183: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WTF/wtf/text/StringBuffer.h:33: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/bmalloc/bmalloc/Gigacage.cpp:39: g_stringGigacageBasePtr is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 3 in 42 files If any of these errors are false positives, please file a bug against check-webkit-style.

Filip Pizlo

Comment 36 2017-08-29 15:56:16 PDT

Created attachment 319297 [details] the ptach

Build Bot

Comment 37 2017-08-29 15:59:09 PDT

Attachment 319297 [details] did not pass style-queue: ERROR: Source/WTF/wtf/text/StringImpl.h:183: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WTF/wtf/text/StringBuffer.h:33: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/bmalloc/bmalloc/Gigacage.cpp:39: g_stringGigacageBasePtr is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 3 in 42 files If any of these errors are false positives, please file a bug against check-webkit-style.

Oliver Hunt

Comment 38 2017-08-29 17:14:23 PDT

Comment on attachment 319297 [details] the ptach View in context: https://bugs.webkit.org/attachment.cgi?id=319297&action=review > Source/JavaScriptCore/runtime/JSString.cpp:182 > + [&] () { > + if (length() > maxLengthForOnStackResolve) { I'm curious about this change (I apparently don't have the context for why we want to do this in a block)

Filip Pizlo

Comment 39 2017-08-30 10:18:56 PDT

(In reply to Oliver Hunt from comment #38) > Comment on attachment 319297 [details] > the ptach > > View in context: > https://bugs.webkit.org/attachment.cgi?id=319297&action=review > > > Source/JavaScriptCore/runtime/JSString.cpp:182 > > + [&] () { > > + if (length() > maxLengthForOnStackResolve) { > > I'm curious about this change (I apparently don't have the context for why > we want to do this in a block) Because below the scope we say: m_value.releaseAssertCaged(); This allows us to continue to use early return as a control flow construct, but instead of actually returning, it takes us first to the releaseAssertCaged thing.

Filip Pizlo

Comment 40 2017-08-30 10:47:12 PDT

Landed in https://trac.webkit.org/changeset/221384/webkit

Radar WebKit Bug Importer

Comment 41 2017-08-30 10:47:42 PDT

<rdar://problem/34166003>

Matt Lewis

Comment 42 2017-08-30 15:35:11 PDT

This patch caused multiple 32-bit JSC test to fail: stress/regexp-prototype-exec-on-too-long-rope.js.misc-ftl-no-cjit stress/regexp-prototype-match-on-too-long-rope.js.misc-ftl-no-cjit stress/regexp-prototype-test-on-too-long-rope.js.misc-ftl-no-cjit stress/string-prototype-charCodeAt-on-too-long-rope.js.misc-ftl-no-cjit https://build.webkit.org/builders/Apple%20Sierra%2032-bit%20JSC%20%28BuildAndTest%29/builds/1691

Matt Lewis

Comment 43 2017-08-30 15:48:14 PDT

Reverted r221384 for reason: This patch caused multiple 32-bit JSC test failures. Committed r221402: <http://trac.webkit.org/changeset/221402>

Yusuke Suzuki

Comment 44 2017-08-31 01:07:13 PDT

It seems that this causes regression in kraken-json-stringify. (maybe, due to JSStringBuilder change?)

Filip Pizlo

Comment 45 2017-08-31 09:01:31 PDT

(In reply to Matt Lewis from comment #43) > Reverted r221384 for reason: > > This patch caused multiple 32-bit JSC test failures. > > Committed r221402: <http://trac.webkit.org/changeset/221402> Looking into that now.

Filip Pizlo

Comment 46 2017-08-31 09:38:44 PDT

Relanded in https://trac.webkit.org/changeset/221422/webkit OOM tests on strings work differently now.

Filip Pizlo

Comment 47 2017-08-31 09:38:52 PDT

(In reply to Yusuke Suzuki from comment #44) > It seems that this causes regression in kraken-json-stringify. (maybe, due > to JSStringBuilder change?) How big?

Yusuke Suzuki

Comment 48 2017-08-31 17:02:54 PDT

(In reply to Filip Pizlo from comment #47) > (In reply to Yusuke Suzuki from comment #44) > > It seems that this causes regression in kraken-json-stringify. (maybe, due > > to JSStringBuilder change?) > > How big? https://arewefastyet.com/#machine=29&view=single&suite=kraken&subtest=stringify-tinderbox A bit big, it's 22% regression. Since that bench stresses JSON.stringify, execution time is purely consumed in C++ runtime. I think this regression could be fixed once StringBuilder and JSON stringify are carefully optimized with gigacage.

Alex Christensen

Comment 49 2017-08-31 21:10:31 PDT

http://trac.webkit.org/r221471

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