The bug can be reproduced in latest jsc version. code: class c extends(d = function() { return 0; }, d) {}; JSC print nothing, however, it should throw an error. According to ecma-262 • All parts of a ClassDeclaration or a ClassExpression are strict mode code. • In strict mode code, assignment to an undeclared identifier or otherwise unresolvable reference does not create a property in the global object. When a simple assignment occurs within strict mode code, its LeftHandSideExpression must not evaluate to an unresolvable Reference. If it does, a ReferenceError exception is thrown. So, assignment to ‘d’, which is undeclared, should throw a ReferenceError exception. ISec Lab 2019.12.24
<rdar://problem/58194589>
Created attachment 394293 [details] Patch
Comment on attachment 394293 [details] Patch Can you check whether bytecode execution works if we have `codeBlock->isStrictMode()` thing in runtime code? For example, we are checking `codeBlock()->isStrictMode()` in deleteById called from operationDeleteByIdOptimize.
Comment on attachment 394293 [details] Patch You are right, we use that in several places, this is not correct...
Created attachment 394733 [details] Patch
Created attachment 394759 [details] Patch Rebase
Comment on attachment 394759 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394759&action=review > Source/JavaScriptCore/bytecode/BytecodeList.rb:513 > + ecmaMode: ECMAMode, Can you merge ecmaMode into PutByIdFlags? size of put_by_id is important to make bytecodes small to save memory. > Source/JavaScriptCore/bytecode/BytecodeList.rb:790 > + ecmaMode: ECMAMode, I think this is dangerous. A lot of place is assuming that op_call / op_call_xxx etc.'s bytecode length are same IIRC. Can you add `call_eval_sloppy` / `call_eval_strict` instead to keep the above invariant? > Source/JavaScriptCore/bytecode/BytecodeList.rb:971 > + ecmaMode: ECMAMode, Can you add this into GetPutInfo to squeeze the length of put_to_scope. This is important to save memory. > Source/JavaScriptCore/runtime/JSCJSValue.h:87 > + static constexpr uint8_t NotStrictMode = 1; You can name it SloppyMode. This is not official term, but well-used term in practice. https://developer.mozilla.org/en-US/docs/Glossary/Sloppy_mode > Source/JavaScriptCore/runtime/JSCJSValue.h:92 > + static constexpr ECMAMode notStrict() { return ECMAMode(NotStrictMode); } change it to `sloppy()`.
Comment on attachment 394759 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394759&action=review >> Source/JavaScriptCore/bytecode/BytecodeList.rb:513 >> + ecmaMode: ECMAMode, > > Can you merge ecmaMode into PutByIdFlags? size of put_by_id is important to make bytecodes small to save memory. Done. >> Source/JavaScriptCore/bytecode/BytecodeList.rb:790 >> + ecmaMode: ECMAMode, > > I think this is dangerous. A lot of place is assuming that op_call / op_call_xxx etc.'s bytecode length are same IIRC. > Can you add `call_eval_sloppy` / `call_eval_strict` instead to keep the above invariant? I don't think that's true anymore. With the new bytecode format there should be no assumptions about the size of any bytecodes. >> Source/JavaScriptCore/bytecode/BytecodeList.rb:971 >> + ecmaMode: ECMAMode, > > Can you add this into GetPutInfo to squeeze the length of put_to_scope. This is important to save memory. Done >> Source/JavaScriptCore/runtime/JSCJSValue.h:87 >> + static constexpr uint8_t NotStrictMode = 1; > > You can name it SloppyMode. This is not official term, but well-used term in practice. > https://developer.mozilla.org/en-US/docs/Glossary/Sloppy_mode I thought about, but figured I'd keep the naming. I'll update it. >> Source/JavaScriptCore/runtime/JSCJSValue.h:92 >> + static constexpr ECMAMode notStrict() { return ECMAMode(NotStrictMode); } > > change it to `sloppy()`. 👍
Created attachment 394965 [details] Patch
Created attachment 394998 [details] Patch Rebase
Created attachment 395008 [details] Patch Fix WebCore build
Created attachment 395054 [details] Patch Fix Fits<GetPutInfo>
Created attachment 395089 [details] Patch Fix Windows build
Created attachment 395601 [details] Patch Try to fix i386 build
Created attachment 395607 [details] Patch Rebase
Created attachment 395623 [details] Patch Fix CMake builds
Comment on attachment 395623 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395623&action=review r=me, nice. > Source/JavaScriptCore/bytecode/CodeBlock.h:-239 > - if (reg == thisRegister() && !isStrictMode()) > - return true; > - Can we make it work by passing ECMAMode to this function? > Source/JavaScriptCore/bytecode/Fits.h:375 > + // a pair of (ResultType::Type, ResultType::Type) - try to fit each type into 4 bits > + // additionally, encode unknown types as 0 rather than the | of all types > + static constexpr unsigned typeWidth = 4; > + static constexpr unsigned maxType = (1 << typeWidth) - 1; ECMAMode does not include ResultType. Can you fix this implementation? I think ECMAMode is 1bit flag. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:1317 > + class StrictModeScope { Let's implement it by using WTF::SetForScope.
Comment on attachment 395623 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395623&action=review >> Source/JavaScriptCore/bytecode/CodeBlock.h:-239 >> - > > Can we make it work by passing ECMAMode to this function? That's what I started with, but it's used from many places where we don't currently have the mode. We'd need to add it to many more opcodes. I discussed it with Saam offline and he seemed to believe it should be ok to remove this optimization. >> Source/JavaScriptCore/bytecode/Fits.h:375 >> + static constexpr unsigned maxType = (1 << typeWidth) - 1; > > ECMAMode does not include ResultType. Can you fix this implementation? I think ECMAMode is 1bit flag. Oops, this is just left over from copy paste. The implementation below doesn't use it, I'll remove. >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:1317 >> + class StrictModeScope { > > Let's implement it by using WTF::SetForScope. Will do.
Created attachment 395722 [details] Patch for landing
Created attachment 395727 [details] Patch for landing
Comment on attachment 395727 [details] Patch for landing I'll add an extra test provided by Saam
Created attachment 395741 [details] Patch for landing
Committed r259676: <https://trac.webkit.org/changeset/259676> All reviewed patches have been landed. Closing bug and clearing flags on attachment 395741 [details].
Looks like this broke the 32-bit JSCOnly bots pretty badly. DFGSpeculativeJIT32_64.cpp was not updated.
(In reply to Michael Catanzaro from comment #24) > Looks like this broke the 32-bit JSCOnly bots pretty badly. > DFGSpeculativeJIT32_64.cpp was not updated. yes. I just filled a bug reporting the same: https://bugs.webkit.org/show_bug.cgi?id=210176