Skip to content
This repository was archived by the owner on Jun 24, 2022. It is now read-only.

Commit dc90496

Browse files
author
[email protected]
committed
[ESNext][JIT] Add support for UntypedUse on PutPrivateName's base operand
https://bugs.webkit.org/show_bug.cgi?id=217373 Reviewed by Yusuke Suzuki. JSTests: * stress/get-private-name-with-primitive.js: Added. * stress/put-private-name-untyped-use.js: Added. * stress/put-private-name-with-primitive.js: Added. Source/JavaScriptCore: This patch is adding UntypedUse for `PutPrivateName`'s base operand to avoid a OSR when we have a non-cell base. Also, it is fixing a bug on private field operations `get_private_name` and `put_private_name` to call `ToObject` on base to properly support class fields spec text[1][2]. [1] - https://tc39.es/proposal-class-fields/#sec-getvalue [2] - https://tc39.es/proposal-class-fields/#sec-putvalue * dfg/DFGFixupPhase.cpp: (JSC::DFG::FixupPhase::fixupNode): * dfg/DFGSpeculativeJIT.cpp: (JSC::DFG::SpeculativeJIT::compilePutPrivateName): * ftl/FTLLowerDFGToB3.cpp: (JSC::FTL::DFG::LowerDFGToB3::compilePutPrivateName): * jit/JITOperations.cpp: (JSC::setPrivateField): (JSC::definePrivateField): (JSC::JSC_DEFINE_JIT_OPERATION): (JSC::getPrivateName): * jit/JITPropertyAccess.cpp: (JSC::JIT::emit_op_put_private_name): * jit/JITPropertyAccess32_64.cpp: (JSC::JIT::emit_op_put_private_name): * llint/LLIntSlowPaths.cpp: (JSC::LLInt::LLINT_SLOW_PATH_DECL): * runtime/CommonSlowPaths.cpp: Previous implementation was wrongly considering that base was always an object, causing segmentation fault when base was not an object. We changed this to handle cases when base is not and object, following what spec text specifies. git-svn-id: http://svn.webkit.org/repository/webkit/trunk@268656 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent dc3b802 commit dc90496

13 files changed

+244
-44
lines changed

‎JSTests/ChangeLog

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,14 @@
1+
2020-10-18 Caio Lima <[email protected]>
2+
3+
[ESNext][JIT] Add support for UntypedUse on PutPrivateName's base operand
4+
https://bugs.webkit.org/show_bug.cgi?id=217373
5+
6+
Reviewed by Yusuke Suzuki.
7+
8+
* stress/get-private-name-with-primitive.js: Added.
9+
* stress/put-private-name-untyped-use.js: Added.
10+
* stress/put-private-name-with-primitive.js: Added.
11+
112
2020-10-17 Ross Kirsling <[email protected]>
213

314
Ensure %TypedArray% essential internal methods adhere to spec

‎JSTests/stress/get-private-name-with-primitive.js

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
//@ requireOptions("--usePrivateClassFields=true")
2+
3+
let assert = {
4+
shouldThrow: function(exception, functor) {
5+
let threwException;
6+
try {
7+
functor();
8+
threwException = false;
9+
} catch(e) {
10+
threwException = true;
11+
if (!e instanceof exception)
12+
throw new Error("Expected to throw: " + exception.name + " but it throws: " + e.name);
13+
}
14+
15+
if (!threwException)
16+
throw new Error("Expected to throw: " + exception.name + " but executed without exception");
17+
}
18+
}
19+
20+
class C {
21+
#field = 'test';
22+
23+
getField(v) {
24+
this.#field;
25+
}
26+
}
27+
noInline(C.constructor);
28+
noInline(C.prototype.getField);
29+
30+
31+
let c = new C();
32+
33+
assert.shouldThrow(TypeError, () => {
34+
c.getField.call(15);
35+
});
36+
assert.shouldThrow(TypeError, () => {
37+
c.getField.call('test');
38+
});
39+
assert.shouldThrow(TypeError, () => {
40+
c.getField.call(Symbol);
41+
});
42+
assert.shouldThrow(TypeError, () => {
43+
c.getField.call(null);
44+
});
45+
assert.shouldThrow(TypeError, () => {
46+
c.getField.call(undefined);
47+
});
48+
assert.shouldThrow(TypeError, () => {
49+
c.getField.call(10n);
50+
});
51+

‎JSTests/stress/put-private-name-untyped-use.js

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
//@ requireOptions("--allowUnsupportedTiers=true", "--usePrivateClassFields=true", "--useAccessInlining=false")
2+
3+
let assert = {
4+
shouldThrow: function(exception, functor) {
5+
let threwException;
6+
try {
7+
functor();
8+
threwException = false;
9+
} catch(e) {
10+
threwException = true;
11+
if (!e instanceof exception)
12+
throw new Error("Expected to throw: " + exception.name + " but it throws: " + e.name);
13+
}
14+
15+
if (!threwException)
16+
throw new Error("Expected to throw: " + exception.name + " but executed without exception");
17+
}
18+
}
19+
20+
class C {
21+
#field;
22+
23+
setField(v) {
24+
let o = this;
25+
// This branch is here to avoid PutPrivateNameById to be folded as
26+
// PutByOffset.
27+
if (v > 100)
28+
o = {};
29+
o.#field = v;
30+
}
31+
}
32+
noInline(C.constructor);
33+
noInline(C.prototype.setField);
34+
35+
let c = new C();
36+
37+
// Let's trigger JIT compilation for `C.prototype.setField`
38+
for (let i = 0; i < 10000; i++) {
39+
c.setField(15);
40+
}
41+
42+
for (let i = 0; i < 10000; i++) {
43+
assert.shouldThrow(TypeError, () => {
44+
c.setField.call(15, 'test');
45+
});
46+
47+
assert.shouldThrow(TypeError, () => {
48+
c.setField.call('string', 'test');
49+
});
50+
51+
assert.shouldThrow(TypeError, () => {
52+
c.setField.call(Symbol('symbol'), 'test');
53+
});
54+
}
55+

‎JSTests/stress/put-private-name-with-primitive.js

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
//@ requireOptions("--usePrivateClassFields=true")
2+
3+
let assert = {
4+
shouldThrow: function(exception, functor) {
5+
let threwException;
6+
try {
7+
functor();
8+
threwException = false;
9+
} catch(e) {
10+
threwException = true;
11+
if (!e instanceof exception)
12+
throw new Error("Expected to throw: " + exception.name + " but it throws: " + e.name);
13+
}
14+
15+
if (!threwException)
16+
throw new Error("Expected to throw: " + exception.name + " but executed without exception");
17+
}
18+
}
19+
20+
class C {
21+
#field = 'test';
22+
23+
setField(v) {
24+
this.#field = v;
25+
}
26+
}
27+
noInline(C.constructor);
28+
noInline(C.prototype.setField);
29+
30+
let c = new C();
31+
32+
// We repeat this to make operationPutPrivateNameOptimize repatch to operationPutPrivateNameGeneric
33+
for (let i = 0; i < 5; i++) {
34+
assert.shouldThrow(TypeError, () => {
35+
c.setField.call(15, 0);
36+
});
37+
assert.shouldThrow(TypeError, () => {
38+
c.setField.call('test', 0);
39+
});
40+
assert.shouldThrow(TypeError, () => {
41+
c.setField.call(Symbol, 0);
42+
});
43+
assert.shouldThrow(TypeError, () => {
44+
c.setField.call(null, 0);
45+
});
46+
assert.shouldThrow(TypeError, () => {
47+
c.setField.call(undefined, 0);
48+
});
49+
assert.shouldThrow(TypeError, () => {
50+
c.setField.call(10n, 0);
51+
});
52+
}
53+

‎Source/JavaScriptCore/ChangeLog

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,43 @@
1+
2020-10-18 Caio Lima <[email protected]>
2+
3+
[ESNext][JIT] Add support for UntypedUse on PutPrivateName's base operand
4+
https://bugs.webkit.org/show_bug.cgi?id=217373
5+
6+
Reviewed by Yusuke Suzuki.
7+
8+
This patch is adding UntypedUse for `PutPrivateName`'s base operand to
9+
avoid a OSR when we have a non-cell base.
10+
Also, it is fixing a bug on private field operations `get_private_name` and
11+
`put_private_name` to call `ToObject` on base to properly support
12+
class fields spec text[1][2].
13+
14+
[1] - https://tc39.es/proposal-class-fields/#sec-getvalue
15+
[2] - https://tc39.es/proposal-class-fields/#sec-putvalue
16+
17+
* dfg/DFGFixupPhase.cpp:
18+
(JSC::DFG::FixupPhase::fixupNode):
19+
* dfg/DFGSpeculativeJIT.cpp:
20+
(JSC::DFG::SpeculativeJIT::compilePutPrivateName):
21+
* ftl/FTLLowerDFGToB3.cpp:
22+
(JSC::FTL::DFG::LowerDFGToB3::compilePutPrivateName):
23+
* jit/JITOperations.cpp:
24+
(JSC::setPrivateField):
25+
(JSC::definePrivateField):
26+
(JSC::JSC_DEFINE_JIT_OPERATION):
27+
(JSC::getPrivateName):
28+
* jit/JITPropertyAccess.cpp:
29+
(JSC::JIT::emit_op_put_private_name):
30+
* jit/JITPropertyAccess32_64.cpp:
31+
(JSC::JIT::emit_op_put_private_name):
32+
* llint/LLIntSlowPaths.cpp:
33+
(JSC::LLInt::LLINT_SLOW_PATH_DECL):
34+
* runtime/CommonSlowPaths.cpp:
35+
36+
Previous implementation was wrongly considering that base was always
37+
an object, causing segmentation fault when base was not an object.
38+
We changed this to handle cases when base is not and object, following
39+
what spec text specifies.
40+
141
2020-10-17 Ross Kirsling <[email protected]>
242

343
Unreviewed fix for r268640

‎Source/JavaScriptCore/dfg/DFGFixupPhase.cpp

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1907,7 +1907,6 @@ class FixupPhase : public Phase {
19071907

19081908

19091909
case PutPrivateName: {
1910-
fixEdge<CellUse>(node->child1());
19111910
fixEdge<SymbolUse>(node->child2());
19121911
break;
19131912
}

‎Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3526,19 +3526,20 @@ void SpeculativeJIT::compileGetByValWithThis(Node* node)
35263526

35273527
void SpeculativeJIT::compilePutPrivateName(Node* node)
35283528
{
3529-
SpeculateCellOperand base(this, node->child1());
3529+
ASSERT(node->child1().useKind() == UntypedUse);
3530+
JSValueOperand base(this, node->child1());
35303531
SpeculateCellOperand propertyValue(this, node->child2());
35313532
JSValueOperand value(this, node->child3());
35323533

35333534
JSValueRegs valueRegs = value.jsValueRegs();
3535+
JSValueRegs baseRegs = base.jsValueRegs();
35343536

3535-
GPRReg baseGPR = base.gpr();
35363537
GPRReg propertyGPR = propertyValue.gpr();
35373538

35383539
speculateSymbol(node->child2(), propertyGPR);
35393540

35403541
flushRegisters();
3541-
callOperation(operationPutPrivateNameGeneric, TrustedImmPtr::weakPointer(m_graph, m_graph.globalObjectFor(node->origin.semantic)), CCallHelpers::CellValue(baseGPR), CCallHelpers::CellValue(propertyGPR), valueRegs, TrustedImmPtr(nullptr), TrustedImm32(node->privateFieldPutKind().value()));
3542+
callOperation(operationPutPrivateNameGeneric, TrustedImmPtr::weakPointer(m_graph, m_graph.globalObjectFor(node->origin.semantic)), baseRegs, CCallHelpers::CellValue(propertyGPR), valueRegs, TrustedImmPtr(nullptr), TrustedImm32(node->privateFieldPutKind().value()));
35423543
m_jit.exceptionCheck();
35433544

35443545
noResult(node);

‎Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3971,10 +3971,10 @@ class LowerDFGToB3 {
39713971

39723972
void compilePutPrivateName()
39733973
{
3974-
DFG_ASSERT(m_graph, m_node, m_node->child1().useKind() == CellUse, m_node->child1().useKind());
3974+
DFG_ASSERT(m_graph, m_node, m_node->child1().useKind() == UntypedUse, m_node->child1().useKind());
39753975
JSGlobalObject* globalObject = m_graph.globalObjectFor(m_node->origin.semantic);
39763976

3977-
LValue base = lowCell(m_node->child1());
3977+
LValue base = lowJSValue(m_node->child1());
39783978
LValue property = lowSymbol(m_node->child2());
39793979
LValue value = lowJSValue(m_node->child3());
39803980

‎Source/JavaScriptCore/jit/JITOperations.cpp

Lines changed: 18 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -677,7 +677,8 @@ ALWAYS_INLINE static void setPrivateField(VM& vm, JSGlobalObject* globalObject,
677677
Identifier ident = Identifier::fromUid(vm, identifier.uid());
678678
ASSERT(ident.isPrivateName());
679679

680-
JSObject* baseObject = asObject(baseValue);
680+
JSObject* baseObject = baseValue.toObject(globalObject);
681+
RETURN_IF_EXCEPTION(scope, void());
681682
CodeBlock* codeBlock = callFrame->codeBlock();
682683
Structure* oldStructure = baseObject->structure(vm);
683684

@@ -696,7 +697,8 @@ ALWAYS_INLINE static void definePrivateField(VM& vm, JSGlobalObject* globalObjec
696697
Identifier ident = Identifier::fromUid(vm, identifier.uid());
697698
ASSERT(ident.isPrivateName());
698699

699-
JSObject* baseObject = asObject(baseValue);
700+
JSObject* baseObject = baseValue.toObject(globalObject);
701+
RETURN_IF_EXCEPTION(scope, void());
700702
CodeBlock* codeBlock = callFrame->codeBlock();
701703
Structure* oldStructure = baseObject->structure(vm);
702704

@@ -730,9 +732,10 @@ JSC_DEFINE_JIT_OPERATION(operationPutByIdDefinePrivateFieldStrictOptimize, void,
730732
CacheableIdentifier identifier = CacheableIdentifier::createFromRawBits(rawCacheableIdentifier);
731733
AccessType accessType = static_cast<AccessType>(stubInfo->accessType);
732734
JSValue value = JSValue::decode(encodedValue);
733-
JSObject* baseObject = asObject(JSValue::decode(encodedBase));
734-
735-
definePrivateField(vm, globalObject, callFrame, baseObject, identifier, value, [=](VM& vm, CodeBlock* codeBlock, Structure* oldStructure, PutPropertySlot& putSlot, const Identifier& ident) {
735+
JSValue baseValue = JSValue::decode(encodedBase);
736+
737+
definePrivateField(vm, globalObject, callFrame, baseValue, identifier, value, [=](VM& vm, CodeBlock* codeBlock, Structure* oldStructure, PutPropertySlot& putSlot, const Identifier& ident) {
738+
JSObject* baseObject = asObject(baseValue);
736739
LOG_IC((ICEvent::OperationPutByIdDefinePrivateFieldFieldStrictOptimize, baseObject->classInfo(vm), ident, putSlot.base() == baseObject));
737740

738741
ASSERT_UNUSED(accessType, accessType == static_cast<AccessType>(stubInfo->accessType));
@@ -765,9 +768,10 @@ JSC_DEFINE_JIT_OPERATION(operationPutByIdSetPrivateFieldStrictOptimize, void, (J
765768
CacheableIdentifier identifier = CacheableIdentifier::createFromRawBits(rawCacheableIdentifier);
766769
AccessType accessType = static_cast<AccessType>(stubInfo->accessType);
767770
JSValue value = JSValue::decode(encodedValue);
768-
JSObject* baseObject = asObject(JSValue::decode(encodedBase));
771+
JSValue baseValue = JSValue::decode(encodedBase);
769772

770-
setPrivateField(vm, globalObject, callFrame, baseObject, identifier, value, [&](VM& vm, CodeBlock* codeBlock, Structure* oldStructure, PutPropertySlot& putSlot, const Identifier& ident) {
773+
setPrivateField(vm, globalObject, callFrame, baseValue, identifier, value, [&](VM& vm, CodeBlock* codeBlock, Structure* oldStructure, PutPropertySlot& putSlot, const Identifier& ident) {
774+
JSObject* baseObject = asObject(baseValue);
771775
LOG_IC((ICEvent::OperationPutByIdPutPrivateFieldFieldStrictOptimize, baseObject->classInfo(vm), ident, putSlot.base() == baseObject));
772776

773777
ASSERT_UNUSED(accessType, accessType == static_cast<AccessType>(stubInfo->accessType));
@@ -1101,6 +1105,9 @@ JSC_DEFINE_JIT_OPERATION(operationPutPrivateNameOptimize, void, (JSGlobalObject*
11011105
JSValue subscript = JSValue::decode(encodedSubscript);
11021106
JSValue value = JSValue::decode(encodedValue);
11031107

1108+
auto baseObject = baseValue.toObject(globalObject);
1109+
RETURN_IF_EXCEPTION(scope, void());
1110+
11041111
auto propertyName = subscript.toPropertyKey(globalObject);
11051112
EXCEPTION_ASSERT(!scope.exception());
11061113

@@ -1149,7 +1156,6 @@ JSC_DEFINE_JIT_OPERATION(operationPutPrivateNameOptimize, void, (JSGlobalObject*
11491156
// Private fields can only be accessed within class lexical scope
11501157
// and class methods are always in strict mode
11511158
const bool isStrictMode = true;
1152-
auto baseObject = asObject(baseValue);
11531159
PutPropertySlot slot(baseObject, isStrictMode);
11541160
if (putKind.isDefine())
11551161
baseObject->definePrivateField(globalObject, propertyName, value, slot);
@@ -1170,6 +1176,9 @@ JSC_DEFINE_JIT_OPERATION(operationPutPrivateNameGeneric, void, (JSGlobalObject*
11701176
JSValue subscript = JSValue::decode(encodedSubscript);
11711177
JSValue value = JSValue::decode(encodedValue);
11721178

1179+
auto baseObject = baseValue.toObject(globalObject);
1180+
RETURN_IF_EXCEPTION(scope, void());
1181+
11731182
auto propertyName = subscript.toPropertyKey(globalObject);
11741183
EXCEPTION_ASSERT(!scope.exception());
11751184

@@ -1178,7 +1187,6 @@ JSC_DEFINE_JIT_OPERATION(operationPutPrivateNameGeneric, void, (JSGlobalObject*
11781187
// Private fields can only be accessed within class lexical scope
11791188
// and class methods are always in strict mode
11801189
const bool isStrictMode = true;
1181-
auto baseObject = asObject(baseValue);
11821190
PutPropertySlot slot(baseObject, isStrictMode);
11831191
if (privateFieldPutKind.isDefine())
11841192
baseObject->definePrivateField(globalObject, propertyName, value, slot);
@@ -2241,12 +2249,11 @@ ALWAYS_INLINE static JSValue getPrivateName(JSGlobalObject* globalObject, CallFr
22412249
VM& vm = globalObject->vm();
22422250
auto scope = DECLARE_THROW_SCOPE(vm);
22432251

2244-
baseValue.requireObjectCoercible(globalObject);
2252+
JSObject* base = baseValue.toObject(globalObject);
22452253
RETURN_IF_EXCEPTION(scope, JSValue());
22462254
auto fieldName = fieldNameValue.toPropertyKey(globalObject);
22472255
RETURN_IF_EXCEPTION(scope, JSValue());
22482256

2249-
JSObject* base = baseValue.toObject(globalObject);
22502257
PropertySlot slot(base, PropertySlot::InternalMethodType::GetOwnProperty);
22512258
base->getPrivateField(globalObject, fieldName, slot);
22522259
RETURN_IF_EXCEPTION(scope, JSValue());

‎Source/JavaScriptCore/jit/JITPropertyAccess.cpp

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -380,6 +380,8 @@ void JIT::emit_op_put_private_name(const Instruction* currentInstruction)
380380
emitGetVirtualRegister(base, regT0);
381381
emitGetVirtualRegister(property, regT1);
382382

383+
emitJumpSlowCaseIfNotJSCell(regT0, base);
384+
383385
PatchableJump fastPathJmp = patchableJump();
384386
addSlowCase(fastPathJmp);
385387

0 commit comments

Comments
 (0)