Patch forthcoming.
Created attachment 336220 [details] work in progress
Created attachment 336234 [details] it might be done
Attachment 336234 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/ScopedArguments.cpp:151: Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(). [security/assertion] [5] Total errors found: 1 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 336238 [details] the patch
Attachment 336238 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/ScopedArguments.cpp:151: Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(). [security/assertion] [5] Total errors found: 1 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 336240 [details] the patch
Attachment 336240 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/ScopedArguments.cpp:151: Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(). [security/assertion] [5] Total errors found: 1 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 336234 [details] it might be done View in context: https://bugs.webkit.org/attachment.cgi?id=336234&action=review r=me with issues addressed. > Source/JavaScriptCore/bytecode/AccessCase.cpp:407 > + jit.loadPtr( > + CCallHelpers::Address(baseGPR, DirectArguments::offsetOfStorage()), Shouldn't this be ScopedArguments::offsetOfStorage()? > Source/JavaScriptCore/runtime/ScopedArguments.h:184 > + static_assert(sizeof(StorageHeader) <= sizeof(WriteBarrier<Unknown>), "StorageHeader needs to be no bigger than a JSValue"); > + return *bitwise_cast<StorageHeader*>(storage - 1); You could have introduced a static method: static size_t storageHeaderSizeInJSValueCount() { return WTF::roundUpToMultipleOf<sizeof(WriteBarrier<Unknown>)>(sizeof(StorageHeader)) / sizeof(WriteBarrier<Unknown>); } ... and use that instead assuming the StorageHeader takes 1 JSValue. With that you can get rid of this assertion and make it such that we can add new fields to StorageHeader without breaking any code. That said, we can always defer this till we need it in the future.
(In reply to Mark Lam from comment #8) > Comment on attachment 336234 [details] > it might be done > > View in context: > https://bugs.webkit.org/attachment.cgi?id=336234&action=review > > r=me with issues addressed. > > > Source/JavaScriptCore/bytecode/AccessCase.cpp:407 > > + jit.loadPtr( > > + CCallHelpers::Address(baseGPR, DirectArguments::offsetOfStorage()), > > Shouldn't this be ScopedArguments::offsetOfStorage()? > > > Source/JavaScriptCore/runtime/ScopedArguments.h:184 > > + static_assert(sizeof(StorageHeader) <= sizeof(WriteBarrier<Unknown>), "StorageHeader needs to be no bigger than a JSValue"); > > + return *bitwise_cast<StorageHeader*>(storage - 1); > > You could have introduced a static method: > static size_t storageHeaderSizeInJSValueCount() { return > WTF:: > roundUpToMultipleOf<sizeof(WriteBarrier<Unknown>)>(sizeof(StorageHeader)) / > sizeof(WriteBarrier<Unknown>); } > > ... and use that instead assuming the StorageHeader takes 1 JSValue. With > that you can get rid of this assertion and make it such that we can add new > fields to StorageHeader without breaking any code. That said, we can always > defer this till we need it in the future. Lots of code assumes that StorageHeader is smaller than JSValue.
(In reply to Mark Lam from comment #8) > Comment on attachment 336234 [details] > it might be done > > View in context: > https://bugs.webkit.org/attachment.cgi?id=336234&action=review > > r=me with issues addressed. > > > Source/JavaScriptCore/bytecode/AccessCase.cpp:407 > > + jit.loadPtr( > > + CCallHelpers::Address(baseGPR, DirectArguments::offsetOfStorage()), > > Shouldn't this be ScopedArguments::offsetOfStorage()? Fixed! > > > Source/JavaScriptCore/runtime/ScopedArguments.h:184 > > + static_assert(sizeof(StorageHeader) <= sizeof(WriteBarrier<Unknown>), "StorageHeader needs to be no bigger than a JSValue"); > > + return *bitwise_cast<StorageHeader*>(storage - 1); > > You could have introduced a static method: > static size_t storageHeaderSizeInJSValueCount() { return > WTF:: > roundUpToMultipleOf<sizeof(WriteBarrier<Unknown>)>(sizeof(StorageHeader)) / > sizeof(WriteBarrier<Unknown>); } > > ... and use that instead assuming the StorageHeader takes 1 JSValue. With > that you can get rid of this assertion and make it such that we can add new > fields to StorageHeader without breaking any code. That said, we can always > defer this till we need it in the future.
Created attachment 336242 [details] patch for landing Addressed Mark's feedback
Attachment 336242 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/ScopedArguments.cpp:151: Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(). [security/assertion] [5] Total errors found: 1 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 336242 [details] patch for landing Attachment 336242 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/7057246 New failing tests: microbenchmarks/scoped-arguments-possibly-overridden-length.js.dfg-eager microbenchmarks/scoped-arguments-possibly-overridden-length.js.dfg-maximal-flush-validate-no-cjit microbenchmarks/scoped-arguments-possibly-overridden-length.js.ftl-eager-no-cjit microbenchmarks/scoped-arguments-possibly-overridden-length.js.no-cjit-validate-phases microbenchmarks/scoped-arguments-possibly-overridden-length.js.ftl-eager microbenchmarks/scoped-arguments-possibly-overridden-length.js.ftl-no-cjit-small-pool microbenchmarks/scoped-arguments-possibly-overridden-length.js.dfg-eager-no-cjit-validate microbenchmarks/scoped-arguments-possibly-overridden-length.js.ftl-eager-no-cjit-b3o1 microbenchmarks/scoped-arguments-possibly-overridden-length.js.ftl-no-cjit-no-put-stack-validate microbenchmarks/scoped-arguments-possibly-overridden-length.js.ftl-no-cjit-b3o1 microbenchmarks/scoped-arguments-possibly-overridden-length.js.no-ftl microbenchmarks/scoped-arguments-possibly-overridden-length.js.no-llint microbenchmarks/scoped-arguments-possibly-overridden-length.js.ftl-no-cjit-no-inline-validate microbenchmarks/scoped-arguments-possibly-overridden-length.js.default microbenchmarks/scoped-arguments-possibly-overridden-length.js.ftl-no-cjit-validate-sampling-profiler microbenchmarks/scoped-arguments-possibly-overridden-length.js.no-cjit-collect-continuously
Created attachment 336254 [details] patch for landing
Attachment 336254 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/ScopedArguments.cpp:151: Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(). [security/assertion] [5] Total errors found: 1 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.
Landed in https://trac.webkit.org/changeset/229842/webkit
<rdar://problem/38734279>