Skip to content

Commit 7aebe2c

Browse files
author
Mark Lam
committed
ClonedArguments need to also support haveABadTime mode.
https://bugs.webkit.org/show_bug.cgi?id=164200 <rdar://problem/27211336> Reviewed by Geoffrey Garen. JSTests: * stress/have-a-bad-time-with-arguments.js: Added. Source/JavaScriptCore: For those who are not familiar with the parlance, "have a bad time" in the VM means that Object.prototype has been modified in such a way that we can no longer trivially do indexed property accesses without consulting the Object.prototype. This defeats JIT indexed put optimizations, and hence, makes the VM "have a bad time". Once the VM enters haveABadTime mode, all existing objects are converted to use slow put storage. Thereafter, JSArrays are always created with slow put storage. JSObjects are always created with a blank indexing type. When a new indexed property is put into the new object, its indexing type will be converted to the slow put array indexing type just before we perform the put operation. This is how we ensure that the objects will also use slow put storage. However, ClonedArguments is an object which was previously created unconditionally to use contiguous storage. Subsequently, if we try to call Object.preventExtensions() on that ClonedArguments object, Object.preventExtensions() will: 1. make the ClonedArguments enter dictionary indexing mode, which means it will 2. first ensure that the ClonedArguments is using slow put array storage via JSObject::ensureArrayStorageSlow(). However, JSObject::ensureArrayStorageSlow() expects that we never see an object with contiguous storage once we're in haveABadTime mode. Our ClonedArguments object did not obey this invariant. The fix is to make the ClonedArguments factories create objects that use slow put array storage when in haveABadTime mode. This means: 1. JSGlobalObject::haveABadTime() now changes m_clonedArgumentsStructure to use its slow put version. Also the caching of the slow put version of m_regExpMatchesArrayStructure, because we only need to create it when we are having a bad time. 2. The ClonedArguments factories now allocates a butterfly with slow put array storage if we're in haveABadTime mode. Also added some assertions in ClonedArguments' factory methods to ensure that the created object has the slow put indexing type when it needsSlowPutIndexing(). 3. DFGFixupPhase now watches the havingABadTimeWatchpoint because ClonedArguments' structure will change when having a bad time. 4. DFGArgumentEliminationPhase and DFGVarargsForwardingPhase need not be changed because it is still valid to eliminate the creation of the arguments object even having a bad time, as long as the arguments object does not escape. 5. The DFGAbstractInterpreterInlines now checks for haveABadTime, and sets the predicted type to be SpecObject. Note: this issue does not apply to DirectArguments and ScopedArguments because they use a blank indexing type (just like JSObject). * dfg/DFGAbstractInterpreterInlines.h: (JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects): * dfg/DFGArrayMode.cpp: (JSC::DFG::ArrayMode::dump): * dfg/DFGFixupPhase.cpp: (JSC::DFG::FixupPhase::fixupNode): * runtime/ClonedArguments.cpp: (JSC::ClonedArguments::createEmpty): (JSC::ClonedArguments::createWithInlineFrame): (JSC::ClonedArguments::createWithMachineFrame): (JSC::ClonedArguments::createByCopyingFrom): (JSC::ClonedArguments::createStructure): (JSC::ClonedArguments::createSlowPutStructure): * runtime/ClonedArguments.h: * runtime/JSGlobalObject.cpp: (JSC::JSGlobalObject::init): (JSC::JSGlobalObject::haveABadTime): (JSC::JSGlobalObject::visitChildren): * runtime/JSGlobalObject.h: Canonical link: https://commits.webkit.org/182118@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@208377 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent 0c24371 commit 7aebe2c

13 files changed

Lines changed: 304 additions & 31 deletions

‎JSTests/ChangeLog‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,13 @@
1+
2016-11-03 Mark Lam <mark.lam@apple.com>
2+
3+
ClonedArguments need to also support haveABadTime mode.
4+
https://bugs.webkit.org/show_bug.cgi?id=164200
5+
<rdar://problem/27211336>
6+
7+
Reviewed by Geoffrey Garen.
8+
9+
* stress/have-a-bad-time-with-arguments.js: Added.
10+
111
2016-11-03 Filip Pizlo <fpizlo@apple.com>
212

313
DFG plays fast and loose with the shadow values of a Phi

‎JSTests/stress/have-a-bad-time-with-arguments.js‎

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,140 @@
1+
//@ runFTLNoCJIT
2+
3+
// Tests accessing an element in arguments before and after having a bad time.
4+
// This test should not crash.
5+
6+
let verbose = false;
7+
8+
var utilities = 'function shouldEqual(testId, actual, expected) {' + '\n' +
9+
' if (actual != expected)' + '\n' +
10+
' throw testId + ": ERROR: expect " + expected + ", actual " + actual;' + '\n' +
11+
'}' + '\n';
12+
13+
var haveABadTime = 'Object.defineProperty(Object.prototype, 20, { get() { return 20; } });' + '\n';
14+
15+
var directArgumentsDecl = ' var args = arguments;' + '\n';
16+
17+
var scopedArgumentsDecl = ' var args = arguments;' + '\n' +
18+
' function closure() { return x; }' + '\n';
19+
20+
var clonedArgumentsDecl = ' "use strict";' + '\n' +
21+
' var args = arguments;' + '\n';
22+
23+
function testFunction(argsDecl, insertElementAction, indexToReturn) {
24+
var script = 'function test(x) {' + '\n' +
25+
argsDecl +
26+
insertElementAction +
27+
' return args[' + indexToReturn + '];' + '\n' +
28+
'}' + '\n' +
29+
'noInline(test);' + '\n';
30+
return script;
31+
}
32+
33+
function warmupFunction(tierWarmupCount, testArgs) {
34+
var script = 'function warmup() {' + '\n' +
35+
' for (var i = 0; i < ' + tierWarmupCount + '; i++) {' + '\n' +
36+
' test(' + testArgs + ');' + '\n' +
37+
' }' + '\n' +
38+
'}' + '\n';
39+
return script;
40+
}
41+
42+
let argumentsDecls = {
43+
direct: directArgumentsDecl,
44+
scoped: scopedArgumentsDecl,
45+
cloned: clonedArgumentsDecl
46+
};
47+
48+
let indicesToReturn = {
49+
inBounds: 0,
50+
outOfBoundsInsertedElement: 10,
51+
outOfBoundsInPrototype: 20
52+
};
53+
54+
let tierWarmupCounts = {
55+
llint: 1,
56+
baseline: 50,
57+
dfg: 1000,
58+
ftl: 10000
59+
};
60+
61+
let testArgsList = {
62+
noArgs: {
63+
args: '',
64+
result: {
65+
inBounds: { beforeBadTime: 'undefined', afterBadTime: 'undefined', },
66+
outOfBoundsInsertedElement: { beforeBadTime: '10', afterBadTime: '10', },
67+
outOfBoundsInPrototype: { beforeBadTime: 'undefined', afterBadTime: '20', },
68+
}
69+
},
70+
someArgs: {
71+
args: '1, 2, 3',
72+
result: {
73+
inBounds: { beforeBadTime: '1', afterBadTime: '1', },
74+
outOfBoundsInsertedElement: { beforeBadTime: '10', afterBadTime: '10', },
75+
outOfBoundsInPrototype: { beforeBadTime: 'undefined', afterBadTime: '20', },
76+
}
77+
}
78+
};
79+
80+
let insertElementActions = {
81+
insertElement: ' args[10] = 10;' + '\n',
82+
dontInsertElement: ''
83+
};
84+
85+
for (let argsDeclIndex in argumentsDecls) {
86+
let argsDecl = argumentsDecls[argsDeclIndex];
87+
88+
for (let indexToReturnIndex in indicesToReturn) {
89+
let indexToReturn = indicesToReturn[indexToReturnIndex];
90+
91+
for (let insertElementActionIndex in insertElementActions) {
92+
let insertElementAction = insertElementActions[insertElementActionIndex];
93+
94+
for (let tierWarmupCountIndex in tierWarmupCounts) {
95+
let tierWarmupCount = tierWarmupCounts[tierWarmupCountIndex];
96+
97+
for (let testArgsIndex in testArgsList) {
98+
let testArgs = testArgsList[testArgsIndex].args;
99+
let expectedResult = testArgsList[testArgsIndex].result[indexToReturnIndex];
100+
101+
if (indexToReturnIndex == 'outOfBoundsInsertedElement'
102+
&& insertElementActionIndex == 'dontInsertElement')
103+
expectedResult = 'undefined';
104+
105+
let testName =
106+
argsDeclIndex + '-' +
107+
indexToReturnIndex + '-' +
108+
insertElementActionIndex + '-' +
109+
tierWarmupCountIndex + '-' +
110+
testArgsIndex;
111+
112+
113+
let script = utilities +
114+
testFunction(argsDecl, insertElementAction, indexToReturn) +
115+
warmupFunction(tierWarmupCount, testArgs) +
116+
'warmup()' + '\n' +
117+
'shouldEqual(10000, test(' + testArgs + '), ' + expectedResult['beforeBadTime'] + ');' + '\n';
118+
haveABadTime +
119+
'shouldEqual(20000, test(' + testArgs + '), ' + expectedResult['afterBadTime'] + ');' + '\n';
120+
121+
if (verbose) {
122+
print('Running test configuration: ' + testName);
123+
print(
124+
'Test script: ====================================================\n' +
125+
script +
126+
'=== END script ==================================================');
127+
}
128+
129+
try {
130+
runString(script);
131+
} catch (e) {
132+
print('FAILED test configuration: ' + testName);
133+
print('FAILED test script:\n' + script);
134+
throw e;
135+
}
136+
}
137+
}
138+
}
139+
}
140+
}

‎Source/JavaScriptCore/ChangeLog‎

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,83 @@
1+
2016-11-03 Mark Lam <mark.lam@apple.com>
2+
3+
ClonedArguments need to also support haveABadTime mode.
4+
https://bugs.webkit.org/show_bug.cgi?id=164200
5+
<rdar://problem/27211336>
6+
7+
Reviewed by Geoffrey Garen.
8+
9+
For those who are not familiar with the parlance, "have a bad time" in the VM
10+
means that Object.prototype has been modified in such a way that we can no longer
11+
trivially do indexed property accesses without consulting the Object.prototype.
12+
This defeats JIT indexed put optimizations, and hence, makes the VM "have a
13+
bad time".
14+
15+
Once the VM enters haveABadTime mode, all existing objects are converted to use
16+
slow put storage. Thereafter, JSArrays are always created with slow put storage.
17+
JSObjects are always created with a blank indexing type. When a new indexed
18+
property is put into the new object, its indexing type will be converted to the
19+
slow put array indexing type just before we perform the put operation. This is
20+
how we ensure that the objects will also use slow put storage.
21+
22+
However, ClonedArguments is an object which was previously created unconditionally
23+
to use contiguous storage. Subsequently, if we try to call Object.preventExtensions()
24+
on that ClonedArguments object, Object.preventExtensions() will:
25+
1. make the ClonedArguments enter dictionary indexing mode, which means it will
26+
2. first ensure that the ClonedArguments is using slow put array storage via
27+
JSObject::ensureArrayStorageSlow().
28+
29+
However, JSObject::ensureArrayStorageSlow() expects that we never see an object
30+
with contiguous storage once we're in haveABadTime mode. Our ClonedArguments
31+
object did not obey this invariant.
32+
33+
The fix is to make the ClonedArguments factories create objects that use slow put
34+
array storage when in haveABadTime mode. This means:
35+
36+
1. JSGlobalObject::haveABadTime() now changes m_clonedArgumentsStructure to use
37+
its slow put version.
38+
39+
Also the caching of the slow put version of m_regExpMatchesArrayStructure,
40+
because we only need to create it when we are having a bad time.
41+
42+
2. The ClonedArguments factories now allocates a butterfly with slow put array
43+
storage if we're in haveABadTime mode.
44+
45+
Also added some assertions in ClonedArguments' factory methods to ensure that
46+
the created object has the slow put indexing type when it needsSlowPutIndexing().
47+
48+
3. DFGFixupPhase now watches the havingABadTimeWatchpoint because ClonedArguments'
49+
structure will change when having a bad time.
50+
51+
4. DFGArgumentEliminationPhase and DFGVarargsForwardingPhase need not be changed
52+
because it is still valid to eliminate the creation of the arguments object
53+
even having a bad time, as long as the arguments object does not escape.
54+
55+
5. The DFGAbstractInterpreterInlines now checks for haveABadTime, and sets the
56+
predicted type to be SpecObject.
57+
58+
Note: this issue does not apply to DirectArguments and ScopedArguments because
59+
they use a blank indexing type (just like JSObject).
60+
61+
* dfg/DFGAbstractInterpreterInlines.h:
62+
(JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
63+
* dfg/DFGArrayMode.cpp:
64+
(JSC::DFG::ArrayMode::dump):
65+
* dfg/DFGFixupPhase.cpp:
66+
(JSC::DFG::FixupPhase::fixupNode):
67+
* runtime/ClonedArguments.cpp:
68+
(JSC::ClonedArguments::createEmpty):
69+
(JSC::ClonedArguments::createWithInlineFrame):
70+
(JSC::ClonedArguments::createWithMachineFrame):
71+
(JSC::ClonedArguments::createByCopyingFrom):
72+
(JSC::ClonedArguments::createStructure):
73+
(JSC::ClonedArguments::createSlowPutStructure):
74+
* runtime/ClonedArguments.h:
75+
* runtime/JSGlobalObject.cpp:
76+
(JSC::JSGlobalObject::init):
77+
(JSC::JSGlobalObject::haveABadTime):
78+
(JSC::JSGlobalObject::visitChildren):
79+
* runtime/JSGlobalObject.h:
80+
181
2016-11-03 Filip Pizlo <fpizlo@apple.com>
282

383
DFG plays fast and loose with the shadow values of a Phi

‎Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1981,6 +1981,10 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
19811981
break;
19821982

19831983
case CreateClonedArguments:
1984+
if (!m_graph.isWatchingHavingABadTimeWatchpoint(node)) {
1985+
forNode(node).setType(m_graph, SpecObject);
1986+
break;
1987+
}
19841988
forNode(node).set(m_graph, m_codeBlock->globalObjectFor(node->origin.semantic)->clonedArgumentsStructure());
19851989
break;
19861990

‎Source/JavaScriptCore/dfg/DFGArrayMode.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -734,7 +734,7 @@ bool ArrayMode::permitsBoundsCheckLowering() const
734734

735735
void ArrayMode::dump(PrintStream& out) const
736736
{
737-
out.print(type(), arrayClass(), speculation(), conversion());
737+
out.print(type(), "+", arrayClass(), "+", speculation(), "+", conversion());
738738
}
739739

740740
} } // namespace JSC::DFG

‎Source/JavaScriptCore/dfg/DFGFixupPhase.cpp‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1550,6 +1550,11 @@ class FixupPhase : public Phase {
15501550
break;
15511551
}
15521552

1553+
case CreateClonedArguments: {
1554+
watchHavingABadTime(node);
1555+
break;
1556+
}
1557+
15531558
case CreateScopedArguments:
15541559
case CreateActivation:
15551560
case NewFunction:
@@ -1776,7 +1781,6 @@ class FixupPhase : public Phase {
17761781
case IsObjectOrNull:
17771782
case IsFunction:
17781783
case CreateDirectArguments:
1779-
case CreateClonedArguments:
17801784
case Jump:
17811785
case Return:
17821786
case TailCall:

‎Source/JavaScriptCore/interpreter/Interpreter.cpp‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,6 @@
3232

3333
#include "BatchedTransitionOptimizer.h"
3434
#include "CallFrameClosure.h"
35-
#include "ClonedArguments.h"
3635
#include "CodeBlock.h"
3736
#include "DirectArguments.h"
3837
#include "Heap.h"

‎Source/JavaScriptCore/runtime/ClonedArguments.cpp‎

Lines changed: 38 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -48,16 +48,23 @@ ClonedArguments* ClonedArguments::createEmpty(
4848
if (vectorLength > MAX_STORAGE_VECTOR_LENGTH)
4949
return 0;
5050

51-
void* temp = vm.heap.tryAllocateAuxiliary(nullptr, Butterfly::totalSize(0, structure->outOfLineCapacity(), true, vectorLength * sizeof(EncodedJSValue)));
52-
if (!temp)
53-
return 0;
54-
Butterfly* butterfly = Butterfly::fromBase(temp, 0, structure->outOfLineCapacity());
55-
butterfly->setVectorLength(vectorLength);
56-
butterfly->setPublicLength(length);
51+
Butterfly* butterfly;
52+
if (UNLIKELY(structure->needsSlowPutIndexing())) {
53+
butterfly = createArrayStorageButterfly(vm, nullptr, structure, length, vectorLength);
54+
butterfly->arrayStorage()->m_numValuesInVector = vectorLength;
55+
56+
} else {
57+
void* temp = vm.heap.tryAllocateAuxiliary(nullptr, Butterfly::totalSize(0, structure->outOfLineCapacity(), true, vectorLength * sizeof(EncodedJSValue)));
58+
if (!temp)
59+
return 0;
60+
butterfly = Butterfly::fromBase(temp, 0, structure->outOfLineCapacity());
61+
butterfly->setVectorLength(vectorLength);
62+
butterfly->setPublicLength(length);
63+
64+
for (unsigned i = length; i < vectorLength; ++i)
65+
butterfly->contiguous()[i].clear();
66+
}
5767

58-
for (unsigned i = length; i < vectorLength; ++i)
59-
butterfly->contiguous()[i].clear();
60-
6168
ClonedArguments* result =
6269
new (NotNull, allocateCell<ClonedArguments>(vm.heap))
6370
ClonedArguments(vm, structure, butterfly);
@@ -70,9 +77,12 @@ ClonedArguments* ClonedArguments::createEmpty(
7077

7178
ClonedArguments* ClonedArguments::createEmpty(ExecState* exec, JSFunction* callee, unsigned length)
7279
{
80+
VM& vm = exec->vm();
7381
// NB. Some clients might expect that the global object of of this object is the global object
7482
// of the callee. We don't do this for now, but maybe we should.
75-
return createEmpty(exec->vm(), exec->lexicalGlobalObject()->clonedArgumentsStructure(), callee, length);
83+
ClonedArguments* result = createEmpty(vm, exec->lexicalGlobalObject()->clonedArgumentsStructure(), callee, length);
84+
ASSERT(!result->structure(vm)->needsSlowPutIndexing() || shouldUseSlowPut(result->structure(vm)->indexingType()));
85+
return result;
7686
}
7787

7888
ClonedArguments* ClonedArguments::createWithInlineFrame(ExecState* myFrame, ExecState* targetFrame, InlineCallFrame* inlineCallFrame, ArgumentsMode mode)
@@ -117,12 +127,15 @@ ClonedArguments* ClonedArguments::createWithInlineFrame(ExecState* myFrame, Exec
117127
} }
118128

119129
ASSERT(myFrame->lexicalGlobalObject()->clonedArgumentsStructure() == result->structure());
130+
ASSERT(!result->structure(vm)->needsSlowPutIndexing() || shouldUseSlowPut(result->structure(vm)->indexingType()));
120131
return result;
121132
}
122133

123134
ClonedArguments* ClonedArguments::createWithMachineFrame(ExecState* myFrame, ExecState* targetFrame, ArgumentsMode mode)
124135
{
125-
return createWithInlineFrame(myFrame, targetFrame, nullptr, mode);
136+
ClonedArguments* result = createWithInlineFrame(myFrame, targetFrame, nullptr, mode);
137+
ASSERT(!result->structure()->needsSlowPutIndexing() || shouldUseSlowPut(result->structure()->indexingType()));
138+
return result;
126139
}
127140

128141
ClonedArguments* ClonedArguments::createByCopyingFrom(
@@ -134,21 +147,30 @@ ClonedArguments* ClonedArguments::createByCopyingFrom(
134147

135148
for (unsigned i = length; i--;)
136149
result->initializeIndex(vm, i, argumentStart[i].jsValue());
137-
150+
ASSERT(!result->structure(vm)->needsSlowPutIndexing() || shouldUseSlowPut(result->structure(vm)->indexingType()));
138151
return result;
139152
}
140153

141-
Structure* ClonedArguments::createStructure(VM& vm, JSGlobalObject* globalObject, JSValue prototype)
154+
Structure* ClonedArguments::createStructure(VM& vm, JSGlobalObject* globalObject, JSValue prototype, IndexingType indexingType)
142155
{
143-
// We use contiguous storage because optimizations in the FTL assume that cloned arguments creation always produces the same initial structure.
144-
145-
Structure* structure = Structure::create(vm, globalObject, prototype, TypeInfo(ObjectType, StructureFlags), info(), NonArrayWithContiguous);
156+
Structure* structure = Structure::create(vm, globalObject, prototype, TypeInfo(ObjectType, StructureFlags), info(), indexingType);
146157
PropertyOffset offset;
147158
structure = structure->addPropertyTransition(vm, structure, vm.propertyNames->length, DontEnum, offset);
148159
ASSERT(offset == clonedArgumentsLengthPropertyOffset);
149160
return structure;
150161
}
151162

163+
Structure* ClonedArguments::createStructure(VM& vm, JSGlobalObject* globalObject, JSValue prototype)
164+
{
165+
// We use contiguous storage because optimizations in the FTL assume that cloned arguments creation always produces the same initial structure.
166+
return createStructure(vm, globalObject, prototype, NonArrayWithContiguous);
167+
}
168+
169+
Structure* ClonedArguments::createSlowPutStructure(VM& vm, JSGlobalObject* globalObject, JSValue prototype)
170+
{
171+
return createStructure(vm, globalObject, prototype, NonArrayWithSlowPutArrayStorage);
172+
}
173+
152174
bool ClonedArguments::getOwnPropertySlot(JSObject* object, ExecState* exec, PropertyName ident, PropertySlot& slot)
153175
{
154176
ClonedArguments* thisObject = jsCast<ClonedArguments*>(object);

0 commit comments

Comments
 (0)