Specifically, catch statement shouldn't cause all variables to be captured. We should only capture those used inside the catch block. Variable resolution in BytecodeGenerator should resolve variables where resolution passes through a catch scope.
(In reply to comment #0) > Specifically, catch statement shouldn't cause all variables to be captured. > We should only > capture those used inside the catch block. This is wrong. We should just use normal capture rules and treat a catch block like any other lexical scope.
Created attachment 257516 [details] patch Still need to write a change log.
Attachment 257516 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:585: The parameter name "environment" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 257516 [details] patch Attachment 257516 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5313643080056832 New failing tests: js/dom/eval-cache-scoped-lookup.html
Created attachment 257517 [details] Archive of layout-test-results from ews103 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 257516 [details] patch Attachment 257516 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4607432344993792 New failing tests: js/dom/eval-cache-scoped-lookup.html
Created attachment 257518 [details] Archive of layout-test-results from ews104 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Looks like I should claim that the symbol table corresponds to a lexical scope. Otherwise, we will unsoundly cache evals in a catch scope.
Created attachment 257521 [details] patch
Attachment 257521 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:585: The parameter name "environment" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 257599 [details] patch
Comment on attachment 257599 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=257599&action=review r=me > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1437 > + for (auto entry : environment) { auto& > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3030 > + if (m_shouldEmitDebugHooks) > + environment.markAllVariablesAsCaptured(); Can we markAllVariablesAsCaptured when constructing the VariableEnvironment instead? Then we couldn't forget to do it. > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1894 > + RegisterID* result = generator.emitNode(dst, m_right); // Execute side effects first. > bool threwException = generator.emitReadOnlyExceptionIfNeeded(var); > if (threwException) > - return generator.emitNode(dst, m_right); > + return result; Does this have a test? Good thing to test. > Source/JavaScriptCore/runtime/SymbolTable.h:655 > + ALWAYS_INLINE bool correspondsToVarScope() const { return m_scopeType == VarScope; } > + ALWAYS_INLINE bool correspondsToLexicalScope() const { return m_scopeType == LexicalScope; } > + ALWAYS_INLINE bool correspondsToCatchScope() const { return m_scopeType == CatchScope; } > + void setDoesCorrespondToLexicalScope() { m_scopeType = LexicalScope; } > + void setDoesCorrespondToCatchScope() { m_scopeType = CatchScope; } Can this just be a ScopeType accessor, rather than one method for each kind of ScopeType?
(In reply to comment #12) > Comment on attachment 257599 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=257599&action=review > > r=me > > > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1437 > > + for (auto entry : environment) { > > auto& > > > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3030 > > + if (m_shouldEmitDebugHooks) > > + environment.markAllVariablesAsCaptured(); > > Can we markAllVariablesAsCaptured when constructing the VariableEnvironment > instead? Then we couldn't forget to do it. This line of code is actually not needed. pushLexicalScopeInternal has this same line of code. That said, I think the byte code generator is the best place to handle "mark everything as captured". The other option would be in the parser, which feels wrong. > > > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1894 > > + RegisterID* result = generator.emitNode(dst, m_right); // Execute side effects first. > > bool threwException = generator.emitReadOnlyExceptionIfNeeded(var); > > if (threwException) > > - return generator.emitNode(dst, m_right); > > + return result; > > Does this have a test? Good thing to test. Yeah, we do have tests. I actually broke a test in this patch. I needed to change this line because the test, before, relied on us throwing an error when assigning dynamically. Now, because we resolve try/catch statically, we converted this to throw static error in byte code. This change revealed this bug (from a previously written test). > > > Source/JavaScriptCore/runtime/SymbolTable.h:655 > > + ALWAYS_INLINE bool correspondsToVarScope() const { return m_scopeType == VarScope; } > > + ALWAYS_INLINE bool correspondsToLexicalScope() const { return m_scopeType == LexicalScope; } > > + ALWAYS_INLINE bool correspondsToCatchScope() const { return m_scopeType == CatchScope; } > > + void setDoesCorrespondToLexicalScope() { m_scopeType = LexicalScope; } > > + void setDoesCorrespondToCatchScope() { m_scopeType = CatchScope; } > > Can this just be a ScopeType accessor, rather than one method for each kind > of ScopeType? Sure.
Attachment 257599 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:585: The parameter name "environment" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.
landed in: http://trac.webkit.org/changeset/187515