Bug 174924 – Strings need to be in some kind of gigacage

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

...

[Filip Pizlo]

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]

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]

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]

Created attachment 318071 [details]
the patch

[Build Bot]

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 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 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]

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 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]

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 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]

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 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]

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]

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]

Created attachment 318555 [details]
more

[Filip Pizlo]

Created attachment 318780 [details]
more

Fixed a bunch of things.

[Build Bot]

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 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]

Created attachment 318785 [details]
the patch

[Build Bot]

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]

Created attachment 318787 [details]
the patch

Removed some debugging code

[Build Bot]

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]

Created attachment 318794 [details]
the patch

[Build Bot]

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]

Created attachment 318808 [details]
the patch

Fixing WebCore's uses of String::adopt

[Build Bot]

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]

Created attachment 318841 [details]
the patch

[Build Bot]

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]

Created attachment 318845 [details]
the patch

Trying to fix GTK

[Build Bot]

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]

Created attachment 318848 [details]
the patch

try again to fix gtk

[Build Bot]

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]

Created attachment 318953 [details]
the patch

[Build Bot]

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]

Created attachment 319297 [details]
the ptach

[Build Bot]

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 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]

(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]

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

[Radar WebKit Bug Importer]

<rdar://problem/34166003>

[Matt Lewis]

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]

Reverted r221384 for reason:

This patch caused multiple 32-bit JSC test failures.

Committed r221402: <http://trac.webkit.org/changeset/221402>

[Yusuke Suzuki]

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

[Filip Pizlo]

(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]

Relanded in https://trac.webkit.org/changeset/221422/webkit

OOM tests on strings work differently now.

[Filip Pizlo]

(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]

(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]

http://trac.webkit.org/r221471