Skip to content

Commit 59a7a2d

Browse files
author
Filip Pizlo
committed
Scopes that are not under TDZ should still push their variables onto the TDZ stack so that lifting TDZ doesn't bypass that scope
https://bugs.webkit.org/show_bug.cgi?id=159332 rdar://problem/27018958 Reviewed by Saam Barati. This fixes an instacrash in this code: try{}catch(e){}print(e);let e; We lift TDZ for "e" in "catch (e){}", but since that scope doesn't push anything onto the TDZ stack, we lift TDZ from "let e". The problem is that we weren't tracking the set of variables that do not have TDZ. We need to track them to "block" the traversal that lifts TDZ. This change fixes this issue by using a map that tracks all known variables, and tells you if they are under TDZ or not. * bytecode/CodeBlock.h: (JSC::CodeBlock::numParameters): * bytecode/CodeOrigin.h: * bytecompiler/BytecodeGenerator.cpp: (JSC::Label::setLocation): (JSC::Variable::dump): (JSC::BytecodeGenerator::generate): (JSC::BytecodeGenerator::BytecodeGenerator): (JSC::BytecodeGenerator::pushLexicalScopeInternal): (JSC::BytecodeGenerator::popLexicalScope): (JSC::BytecodeGenerator::popLexicalScopeInternal): (JSC::BytecodeGenerator::prepareLexicalScopeForNextForLoopIteration): (JSC::BytecodeGenerator::variable): (JSC::BytecodeGenerator::needsTDZCheck): (JSC::BytecodeGenerator::liftTDZCheckIfPossible): (JSC::BytecodeGenerator::pushTDZVariables): (JSC::BytecodeGenerator::getVariablesUnderTDZ): (JSC::BytecodeGenerator::endGenerator): (WTF::printInternal): * bytecompiler/BytecodeGenerator.h: (JSC::Variable::isConst): (JSC::Variable::setIsReadOnly): * interpreter/CallFrame.h: (JSC::ExecState::topOfFrame): * tests/stress/lift-tdz-bypass-catch.js: Added. (foo): (catch): Canonical link: https://commits.webkit.org/177509@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@202778 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent 7c70fca commit 59a7a2d

8 files changed

Lines changed: 171 additions & 45 deletions

File tree

‎Source/JavaScriptCore/ChangeLog‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,50 @@
1+
2016-06-30 Filip Pizlo <fpizlo@apple.com>
2+
3+
Scopes that are not under TDZ should still push their variables onto the TDZ stack so that lifting TDZ doesn't bypass that scope
4+
https://bugs.webkit.org/show_bug.cgi?id=159332
5+
rdar://problem/27018958
6+
7+
Reviewed by Saam Barati.
8+
9+
This fixes an instacrash in this code:
10+
11+
try{}catch(e){}print(e);let e;
12+
13+
We lift TDZ for "e" in "catch (e){}", but since that scope doesn't push anything onto the
14+
TDZ stack, we lift TDZ from "let e".
15+
16+
The problem is that we weren't tracking the set of variables that do not have TDZ. We need
17+
to track them to "block" the traversal that lifts TDZ. This change fixes this issue by
18+
using a map that tracks all known variables, and tells you if they are under TDZ or not.
19+
20+
* bytecode/CodeBlock.h:
21+
(JSC::CodeBlock::numParameters):
22+
* bytecode/CodeOrigin.h:
23+
* bytecompiler/BytecodeGenerator.cpp:
24+
(JSC::Label::setLocation):
25+
(JSC::Variable::dump):
26+
(JSC::BytecodeGenerator::generate):
27+
(JSC::BytecodeGenerator::BytecodeGenerator):
28+
(JSC::BytecodeGenerator::pushLexicalScopeInternal):
29+
(JSC::BytecodeGenerator::popLexicalScope):
30+
(JSC::BytecodeGenerator::popLexicalScopeInternal):
31+
(JSC::BytecodeGenerator::prepareLexicalScopeForNextForLoopIteration):
32+
(JSC::BytecodeGenerator::variable):
33+
(JSC::BytecodeGenerator::needsTDZCheck):
34+
(JSC::BytecodeGenerator::liftTDZCheckIfPossible):
35+
(JSC::BytecodeGenerator::pushTDZVariables):
36+
(JSC::BytecodeGenerator::getVariablesUnderTDZ):
37+
(JSC::BytecodeGenerator::endGenerator):
38+
(WTF::printInternal):
39+
* bytecompiler/BytecodeGenerator.h:
40+
(JSC::Variable::isConst):
41+
(JSC::Variable::setIsReadOnly):
42+
* interpreter/CallFrame.h:
43+
(JSC::ExecState::topOfFrame):
44+
* tests/stress/lift-tdz-bypass-catch.js: Added.
45+
(foo):
46+
(catch):
47+
148
2016-07-01 Benjamin Poulain <bpoulain@apple.com>
249

350
[JSC] RegExp.compile is not returning the regexp when it succeed

‎Source/JavaScriptCore/bytecode/CodeBlock.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,7 @@ class CodeBlock : public JSCell {
141141
CString sourceCodeForTools() const; // Not quite the actual source we parsed; this will do things like prefix the source for a function with a reified signature.
142142
CString sourceCodeOnOneLine() const; // As sourceCodeForTools(), but replaces all whitespace runs with a single space.
143143
void dumpAssumingJITType(PrintStream&, JITCode::JITType) const;
144-
void dump(PrintStream&) const;
144+
JS_EXPORT_PRIVATE void dump(PrintStream&) const;
145145

146146
int numParameters() const { return m_numParameters; }
147147
void setNumParameters(int newValue);

‎Source/JavaScriptCore/bytecode/CodeOrigin.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,7 @@ struct CodeOrigin {
109109
// Get the inline stack. This is slow, and is intended for debugging only.
110110
Vector<CodeOrigin> inlineStack() const;
111111

112-
void dump(PrintStream&) const;
112+
JS_EXPORT_PRIVATE void dump(PrintStream&) const;
113113
void dumpInContext(PrintStream&, DumpContext*) const;
114114

115115
private:

‎Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp‎

Lines changed: 79 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@
4545
#include "StrongInlines.h"
4646
#include "UnlinkedCodeBlock.h"
4747
#include "UnlinkedInstructionStream.h"
48+
#include <wtf/CommaPrinter.h>
4849
#include <wtf/StdLibExtras.h>
4950
#include <wtf/text/WTFString.h>
5051

@@ -61,6 +62,18 @@ void Label::setLocation(unsigned location)
6162
m_generator.instructions()[m_unresolvedJumps[i].second].u.operand = m_location - m_unresolvedJumps[i].first;
6263
}
6364

65+
void Variable::dump(PrintStream& out) const
66+
{
67+
out.print(
68+
"{ident = ", m_ident,
69+
", offset = ", m_offset,
70+
", local = ", RawPointer(m_local),
71+
", attributes = ", m_attributes,
72+
", kind = ", m_kind,
73+
", symbolTableConstantIndex = ", m_symbolTableConstantIndex,
74+
", isLexicallyScoped = ", m_isLexicallyScoped, "}");
75+
}
76+
6477
ParserError BytecodeGenerator::generate()
6578
{
6679
m_codeBlock->setThisRegister(m_thisRegister.virtualRegister());
@@ -603,7 +616,7 @@ BytecodeGenerator::BytecodeGenerator(VM& vm, FunctionNode* functionNode, Unlinke
603616

604617
// All "addVar()"s needs to happen before "initializeDefaultParameterValuesAndSetupFunctionScopeStack()" is called
605618
// because a function's default parameter ExpressionNodes will use temporary registers.
606-
pushTDZVariables(*parentScopeTDZVariables, TDZCheckOptimization::DoNotOptimize);
619+
pushTDZVariables(*parentScopeTDZVariables, TDZCheckOptimization::DoNotOptimize, TDZRequirement::UnderTDZ);
607620
initializeDefaultParameterValuesAndSetupFunctionScopeStack(parameters, isSimpleParameterList, functionNode, functionSymbolTable, symbolTableConstantIndex, captures, shouldCreateArgumentsVariableInParameterScope);
608621

609622
// If we don't have default parameter expression, then loading |this| inside an arrow function must be done
@@ -639,7 +652,7 @@ BytecodeGenerator::BytecodeGenerator(VM& vm, EvalNode* evalNode, UnlinkedEvalCod
639652

640653
m_codeBlock->setNumParameters(1);
641654

642-
pushTDZVariables(*parentScopeTDZVariables, TDZCheckOptimization::DoNotOptimize);
655+
pushTDZVariables(*parentScopeTDZVariables, TDZCheckOptimization::DoNotOptimize, TDZRequirement::UnderTDZ);
643656

644657
emitEnter();
645658

@@ -756,7 +769,7 @@ BytecodeGenerator::BytecodeGenerator(VM& vm, ModuleProgramNode* moduleProgramNod
756769
else
757770
constantSymbolTable = addConstantValue(moduleEnvironmentSymbolTable->cloneScopePart(*m_vm));
758771

759-
pushTDZVariables(lexicalVariables, TDZCheckOptimization::Optimize);
772+
pushTDZVariables(lexicalVariables, TDZCheckOptimization::Optimize, TDZRequirement::UnderTDZ);
760773
bool isWithScope = false;
761774
m_symbolTableStack.append(SymbolTableStackEntry { moduleEnvironmentSymbolTable, m_topMostScope, isWithScope, constantSymbolTable->index() });
762775
emitPrefillStackTDZVariables(lexicalVariables, moduleEnvironmentSymbolTable);
@@ -1933,8 +1946,7 @@ void BytecodeGenerator::pushLexicalScopeInternal(VariableEnvironment& environmen
19331946

19341947
bool isWithScope = false;
19351948
m_symbolTableStack.append(SymbolTableStackEntry{ symbolTable, newScope, isWithScope, symbolTableConstantIndex });
1936-
if (tdzRequirement == TDZRequirement::UnderTDZ)
1937-
pushTDZVariables(environment, tdzCheckOptimization);
1949+
pushTDZVariables(environment, tdzCheckOptimization, tdzRequirement);
19381950

19391951
if (tdzRequirement == TDZRequirement::UnderTDZ)
19401952
emitPrefillStackTDZVariables(environment, symbolTable);
@@ -2021,10 +2033,10 @@ void BytecodeGenerator::hoistSloppyModeFunctionIfNecessary(const Identifier& fun
20212033
void BytecodeGenerator::popLexicalScope(VariableEnvironmentNode* node)
20222034
{
20232035
VariableEnvironment& environment = node->lexicalVariables();
2024-
popLexicalScopeInternal(environment, TDZRequirement::UnderTDZ);
2036+
popLexicalScopeInternal(environment);
20252037
}
20262038

2027-
void BytecodeGenerator::popLexicalScopeInternal(VariableEnvironment& environment, TDZRequirement tdzRequirement)
2039+
void BytecodeGenerator::popLexicalScopeInternal(VariableEnvironment& environment)
20282040
{
20292041
// NOTE: This function only makes sense for scopes that aren't ScopeRegisterType::Var (only function name scope right now is ScopeRegisterType::Var).
20302042
// This doesn't make sense for ScopeRegisterType::Var because we deref RegisterIDs here.
@@ -2057,8 +2069,7 @@ void BytecodeGenerator::popLexicalScopeInternal(VariableEnvironment& environment
20572069
stackEntry.m_scope->deref();
20582070
}
20592071

2060-
if (tdzRequirement == TDZRequirement::UnderTDZ)
2061-
m_TDZStack.removeLast();
2072+
m_TDZStack.removeLast();
20622073
}
20632074

20642075
void BytecodeGenerator::prepareLexicalScopeForNextForLoopIteration(VariableEnvironmentNode* node, RegisterID* loopSymbolTable)
@@ -2182,7 +2193,7 @@ Variable BytecodeGenerator::variable(const Identifier& property, ThisResolutionT
21822193
result.setIsReadOnly();
21832194
return result;
21842195
}
2185-
2196+
21862197
return Variable(property);
21872198
}
21882199

@@ -2722,9 +2733,10 @@ void BytecodeGenerator::emitTDZCheck(RegisterID* target)
27222733
bool BytecodeGenerator::needsTDZCheck(const Variable& variable)
27232734
{
27242735
for (unsigned i = m_TDZStack.size(); i--;) {
2725-
VariableEnvironment& identifiers = m_TDZStack[i].first;
2726-
if (identifiers.contains(variable.ident().impl()))
2727-
return true;
2736+
auto iter = m_TDZStack[i].find(variable.ident().impl());
2737+
if (iter == m_TDZStack[i].end())
2738+
continue;
2739+
return iter->value != TDZNecessityLevel::NotNeeded;
27282740
}
27292741

27302742
return false;
@@ -2747,41 +2759,54 @@ void BytecodeGenerator::liftTDZCheckIfPossible(const Variable& variable)
27472759
{
27482760
RefPtr<UniquedStringImpl> identifier(variable.ident().impl());
27492761
for (unsigned i = m_TDZStack.size(); i--;) {
2750-
VariableEnvironment& environment = m_TDZStack[i].first;
2751-
if (environment.contains(identifier)) {
2752-
TDZCheckOptimization tdzCheckOptimizationCapability = m_TDZStack[i].second;
2753-
if (tdzCheckOptimizationCapability == TDZCheckOptimization::Optimize) {
2754-
bool wasRemoved = environment.remove(identifier);
2755-
RELEASE_ASSERT(wasRemoved);
2756-
}
2762+
auto iter = m_TDZStack[i].find(identifier);
2763+
if (iter != m_TDZStack[i].end()) {
2764+
if (iter->value == TDZNecessityLevel::Optimize)
2765+
iter->value = TDZNecessityLevel::NotNeeded;
27572766
break;
27582767
}
27592768
}
27602769
}
27612770

2762-
void BytecodeGenerator::pushTDZVariables(VariableEnvironment environment, TDZCheckOptimization optimization)
2771+
void BytecodeGenerator::pushTDZVariables(const VariableEnvironment& environment, TDZCheckOptimization optimization, TDZRequirement requirement)
27632772
{
27642773
if (!environment.size())
27652774
return;
2775+
2776+
TDZNecessityLevel level;
2777+
if (requirement == TDZRequirement::UnderTDZ) {
2778+
if (optimization == TDZCheckOptimization::Optimize)
2779+
level = TDZNecessityLevel::Optimize;
2780+
else
2781+
level = TDZNecessityLevel::DoNotOptimize;
2782+
} else
2783+
level = TDZNecessityLevel::NotNeeded;
2784+
2785+
TDZMap map;
2786+
for (const auto& entry : environment)
2787+
map.add(entry.key, entry.value.isFunction() ? TDZNecessityLevel::NotNeeded : level);
27662788

2767-
Vector<UniquedStringImpl*, 4> functionsToRemove;
2768-
for (const auto& entry : environment) {
2769-
if (entry.value.isFunction())
2770-
functionsToRemove.append(entry.key.get());
2771-
}
2772-
2773-
for (UniquedStringImpl* function : functionsToRemove)
2774-
environment.remove(function);
2775-
2776-
m_TDZStack.append(std::make_pair(WTFMove(environment), optimization));
2789+
m_TDZStack.append(WTFMove(map));
27772790
}
27782791

27792792
void BytecodeGenerator::getVariablesUnderTDZ(VariableEnvironment& result)
27802793
{
2781-
for (auto& pair : m_TDZStack) {
2782-
VariableEnvironment& environment = pair.first;
2783-
for (auto entry : environment)
2784-
result.add(entry.key.get());
2794+
// NOTE: This is conservative. If called at "...", it will report "x" as being under TDZ:
2795+
//
2796+
// {
2797+
// {
2798+
// let x;
2799+
// ...
2800+
// }
2801+
// let x;
2802+
// }
2803+
//
2804+
// FIXME: https://bugs.webkit.org/show_bug.cgi?id=159387
2805+
for (auto& map : m_TDZStack) {
2806+
for (auto& entry : map) {
2807+
if (entry.value != TDZNecessityLevel::NotNeeded)
2808+
result.add(entry.key.get());
2809+
}
27852810
}
27862811
}
27872812

@@ -3836,7 +3861,7 @@ void BytecodeGenerator::emitPushCatchScope(VariableEnvironment& environment)
38363861

38373862
void BytecodeGenerator::emitPopCatchScope(VariableEnvironment& environment)
38383863
{
3839-
popLexicalScopeInternal(environment, TDZRequirement::NotUnderTDZ);
3864+
popLexicalScopeInternal(environment);
38403865
}
38413866

38423867
void BytecodeGenerator::beginSwitch(RegisterID* scrutineeRegister, SwitchInfo::SwitchType type)
@@ -4631,3 +4656,21 @@ void BytecodeGenerator::endGenerator(Label* defaultLabel)
46314656
}
46324657

46334658
} // namespace JSC
4659+
4660+
namespace WTF {
4661+
4662+
void printInternal(PrintStream& out, JSC::Variable::VariableKind kind)
4663+
{
4664+
switch (kind) {
4665+
case JSC::Variable::NormalVariable:
4666+
out.print("Normal");
4667+
return;
4668+
case JSC::Variable::SpecialVariable:
4669+
out.print("Special");
4670+
return;
4671+
}
4672+
RELEASE_ASSERT_NOT_REACHED();
4673+
}
4674+
4675+
} // namespace WTF
4676+

‎Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h‎

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -238,6 +238,8 @@ namespace JSC {
238238
bool isConst() const { return isReadOnly() && m_isLexicallyScoped; }
239239
void setIsReadOnly() { m_attributes |= ReadOnly; }
240240

241+
void dump(PrintStream&) const;
242+
241243
private:
242244
Identifier m_ident;
243245
VarOffset m_offset;
@@ -728,7 +730,7 @@ namespace JSC {
728730
enum class ScopeRegisterType { Var, Block };
729731
void pushLexicalScopeInternal(VariableEnvironment&, TDZCheckOptimization, NestedScopeType, RegisterID** constantSymbolTableResult, TDZRequirement, ScopeType, ScopeRegisterType);
730732
void initializeBlockScopedFunctions(VariableEnvironment&, FunctionStack&, RegisterID* constantSymbolTable);
731-
void popLexicalScopeInternal(VariableEnvironment&, TDZRequirement);
733+
void popLexicalScopeInternal(VariableEnvironment&);
732734
template<typename LookUpVarKindFunctor>
733735
bool instantiateLexicalVariables(const VariableEnvironment&, SymbolTable*, ScopeRegisterType, LookUpVarKindFunctor);
734736
void emitPrefillStackTDZVariables(const VariableEnvironment&, SymbolTable*);
@@ -881,9 +883,15 @@ namespace JSC {
881883
int m_symbolTableConstantIndex;
882884
};
883885
Vector<SymbolTableStackEntry> m_symbolTableStack;
884-
Vector<std::pair<VariableEnvironment, TDZCheckOptimization>> m_TDZStack;
886+
enum class TDZNecessityLevel {
887+
NotNeeded,
888+
Optimize,
889+
DoNotOptimize
890+
};
891+
typedef HashMap<RefPtr<UniquedStringImpl>, TDZNecessityLevel, IdentifierRepHash> TDZMap;
892+
Vector<TDZMap> m_TDZStack;
885893
Optional<size_t> m_varScopeSymbolTableIndex;
886-
void pushTDZVariables(VariableEnvironment, TDZCheckOptimization);
894+
void pushTDZVariables(const VariableEnvironment&, TDZCheckOptimization, TDZRequirement);
887895

888896
ScopeNode* const m_scopeNode;
889897
Strong<UnlinkedCodeBlock> m_codeBlock;
@@ -965,4 +973,10 @@ namespace JSC {
965973

966974
}
967975

976+
namespace WTF {
977+
978+
void printInternal(PrintStream&, JSC::Variable::VariableKind);
979+
980+
} // namespace WTF
981+
968982
#endif // BytecodeGenerator_h

‎Source/JavaScriptCore/interpreter/CallFrame.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,7 @@ namespace JSC {
149149

150150
// This will get you a CodeOrigin. It will always succeed. May return
151151
// CodeOrigin(0) if we're in native code.
152-
CodeOrigin codeOrigin();
152+
JS_EXPORT_PRIVATE CodeOrigin codeOrigin();
153153

154154
Register* topOfFrame()
155155
{

‎Source/JavaScriptCore/tests/stress/lift-tdz-bypass-catch.js‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
//@ runDefault
2+
3+
function foo () {
4+
try{}catch(e){}print(e);let e;
5+
}
6+
7+
try {
8+
foo();
9+
} catch (e) {}
10+

0 commit comments

Comments
 (0)