Bug 174924

Summary: Strings need to be in some kind of gigacage
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, buildbot, ggaren, jlewis3, kling, oliver, rniwa, webkit-bug-importer, ysuzuki
Priority: P2 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+

Description Filip Pizlo 2017-07-27 19:47:02 PDT 
...
Comment 1 Filip Pizlo 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.
Comment 2 Filip Pizlo 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.
Comment 3 Filip Pizlo 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.
Comment 4 Filip Pizlo 2017-08-14 15:24:13 PDT 
Created attachment 318071 [details]
the patch
Comment 5 Build Bot 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.
Comment 6 Build Bot 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
Comment 7 Build Bot 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.
Comment 8 Build Bot 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
Comment 9 Build Bot 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.
Comment 10 Build Bot 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
Comment 11 Build Bot 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.
Comment 12 Build Bot 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
Comment 13 Build Bot 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.
Comment 14 Build Bot 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
Comment 15 Filip Pizlo 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.
Comment 16 Filip Pizlo 2017-08-18 16:26:46 PDT 
Created attachment 318555 [details]
more
Comment 17 Filip Pizlo 2017-08-22 12:10:21 PDT 
Created attachment 318780 [details]
more

Fixed a bunch of things.
Comment 18 Build Bot 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.
Comment 19 Build Bot 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
Comment 20 Filip Pizlo 2017-08-22 13:16:37 PDT 
Created attachment 318785 [details]
the patch
Comment 21 Build Bot 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.
Comment 22 Filip Pizlo 2017-08-22 13:26:54 PDT 
Created attachment 318787 [details]
the patch

Removed some debugging code
Comment 23 Build Bot 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.
Comment 24 Filip Pizlo 2017-08-22 13:59:58 PDT 
Created attachment 318794 [details]
the patch
Comment 25 Build Bot 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.
Comment 26 Filip Pizlo 2017-08-22 15:22:54 PDT 
Created attachment 318808 [details]
the patch

Fixing WebCore's uses of String::adopt
Comment 27 Build Bot 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.
Comment 28 Filip Pizlo 2017-08-22 18:20:57 PDT 
Created attachment 318841 [details]
the patch
Comment 29 Build Bot 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.
Comment 30 Filip Pizlo 2017-08-22 18:56:31 PDT 
Created attachment 318845 [details]
the patch

Trying to fix GTK
Comment 31 Build Bot 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.
Comment 32 Filip Pizlo 2017-08-22 20:09:10 PDT 
Created attachment 318848 [details]
the patch

try again to fix gtk
Comment 33 Build Bot 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.
Comment 34 Filip Pizlo 2017-08-23 18:25:19 PDT 
Created attachment 318953 [details]
the patch
Comment 35 Build Bot 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.
Comment 36 Filip Pizlo 2017-08-29 15:56:16 PDT 
Created attachment 319297 [details]
the ptach
Comment 37 Build Bot 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.
Comment 38 Oliver Hunt 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)
Comment 39 Filip Pizlo 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.
Comment 40 Filip Pizlo 2017-08-30 10:47:12 PDT 
Landed in https://trac.webkit.org/changeset/221384/webkit
Comment 41 Radar WebKit Bug Importer 2017-08-30 10:47:42 PDT 
<rdar://problem/34166003>
Comment 42 Matt Lewis 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
Comment 43 Matt Lewis 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>
Comment 44 Yusuke Suzuki 2017-08-31 01:07:13 PDT 
It seems that this causes regression in kraken-json-stringify. (maybe, due to JSStringBuilder change?)
Comment 45 Filip Pizlo 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.
Comment 46 Filip Pizlo 2017-08-31 09:38:44 PDT 
Relanded in https://trac.webkit.org/changeset/221422/webkit

OOM tests on strings work differently now.
Comment 47 Filip Pizlo 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?
Comment 48 Yusuke Suzuki 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.
Comment 49 Alex Christensen 2017-08-31 21:10:31 PDT 
http://trac.webkit.org/r221471