Skip to content

Commit d00da47

Browse files
bmeurerCommit bot
bmeurer
authored and
Commit bot
committed
[turbofan] Don't use the CompareIC in JSGenericLowering.
The CompareICStub produces an untagged raw word value, which has to be translated to true or false manually in the TurboFan code. But for lazy bailout after the CompareIC, we immediately go back to fullcodegen or Ignition with the raw value, to a location where both fullcodegen and Ignition expect a boolean value, which might crash or in the worst case (depending on the exact computation inside the CompareIC) could lead to arbitrary memory access. Short-term fix is to use the proper runtime functions (unified with the interpreter now) for comparisons. Next task is to provide optimized versions of these based on the CodeStubAssembler, which can then be used via code stubs in TurboFan or directly in handlers in the interpreter. [email protected] BUG=v8:4788 LOG=n Review URL: https://codereview.chromium.org/1738153002 Cr-Commit-Position: refs/heads/master@{#34335}
1 parent 81f12a7 commit d00da47

17 files changed

Lines changed: 132 additions & 277 deletions

‎src/arm/code-stubs-arm.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -675,7 +675,7 @@ void CompareICStub::GenerateGeneric(MacroAssembler* masm) {
675675
{
676676
FrameAndConstantPoolScope scope(masm, StackFrame::INTERNAL);
677677
__ Push(lhs, rhs);
678-
__ CallRuntime(strict() ? Runtime::kStrictEquals : Runtime::kEquals);
678+
__ CallRuntime(strict() ? Runtime::kStrictEqual : Runtime::kEqual);
679679
}
680680
// Turn true into 0 and false into some non-zero value.
681681
STATIC_ASSERT(EQUAL == 0);

‎src/arm64/code-stubs-arm64.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -632,7 +632,7 @@ void CompareICStub::GenerateGeneric(MacroAssembler* masm) {
632632
{
633633
FrameScope scope(masm, StackFrame::INTERNAL);
634634
__ Push(lhs, rhs);
635-
__ CallRuntime(strict() ? Runtime::kStrictEquals : Runtime::kEquals);
635+
__ CallRuntime(strict() ? Runtime::kStrictEqual : Runtime::kEqual);
636636
}
637637
// Turn true into 0 and false into some non-zero value.
638638
STATIC_ASSERT(EQUAL == 0);

‎src/compiler/js-generic-lowering.cc‎

Lines changed: 12 additions & 106 deletions
Original file line numberDiff line numberDiff line change
@@ -83,117 +83,22 @@ REPLACE_BINARY_OP_IC_CALL(JSDivide, Token::DIV)
8383
REPLACE_BINARY_OP_IC_CALL(JSModulus, Token::MOD)
8484
#undef REPLACE_BINARY_OP_IC_CALL
8585

86-
87-
// These ops are not language mode dependent; we arbitrarily pass Strength::WEAK
88-
// here.
89-
#define REPLACE_COMPARE_IC_CALL(op, token) \
90-
void JSGenericLowering::Lower##op(Node* node) { \
91-
ReplaceWithCompareIC(node, token); \
92-
}
93-
REPLACE_COMPARE_IC_CALL(JSEqual, Token::EQ)
94-
REPLACE_COMPARE_IC_CALL(JSNotEqual, Token::NE)
95-
REPLACE_COMPARE_IC_CALL(JSStrictEqual, Token::EQ_STRICT)
96-
REPLACE_COMPARE_IC_CALL(JSStrictNotEqual, Token::NE_STRICT)
97-
REPLACE_COMPARE_IC_CALL(JSLessThan, Token::LT)
98-
REPLACE_COMPARE_IC_CALL(JSGreaterThan, Token::GT)
99-
REPLACE_COMPARE_IC_CALL(JSLessThanOrEqual, Token::LTE)
100-
REPLACE_COMPARE_IC_CALL(JSGreaterThanOrEqual, Token::GTE)
101-
#undef REPLACE_COMPARE_IC_CALL
102-
103-
10486
#define REPLACE_RUNTIME_CALL(op, fun) \
10587
void JSGenericLowering::Lower##op(Node* node) { \
10688
ReplaceWithRuntimeCall(node, fun); \
10789
}
90+
REPLACE_RUNTIME_CALL(JSEqual, Runtime::kEqual)
91+
REPLACE_RUNTIME_CALL(JSNotEqual, Runtime::kNotEqual)
92+
REPLACE_RUNTIME_CALL(JSStrictEqual, Runtime::kStrictEqual)
93+
REPLACE_RUNTIME_CALL(JSStrictNotEqual, Runtime::kStrictNotEqual)
94+
REPLACE_RUNTIME_CALL(JSLessThan, Runtime::kLessThan)
95+
REPLACE_RUNTIME_CALL(JSGreaterThan, Runtime::kGreaterThan)
96+
REPLACE_RUNTIME_CALL(JSLessThanOrEqual, Runtime::kLessThanOrEqual)
97+
REPLACE_RUNTIME_CALL(JSGreaterThanOrEqual, Runtime::kGreaterThanOrEqual)
10898
REPLACE_RUNTIME_CALL(JSCreateWithContext, Runtime::kPushWithContext)
10999
REPLACE_RUNTIME_CALL(JSCreateModuleContext, Runtime::kPushModuleContext)
110100
REPLACE_RUNTIME_CALL(JSConvertReceiver, Runtime::kConvertReceiver)
111-
#undef REPLACE_RUNTIME
112-
113-
114-
static CallDescriptor::Flags FlagsForNode(Node* node) {
115-
CallDescriptor::Flags result = CallDescriptor::kNoFlags;
116-
if (OperatorProperties::GetFrameStateInputCount(node->op()) > 0) {
117-
result |= CallDescriptor::kNeedsFrameState;
118-
}
119-
return result;
120-
}
121-
122-
void JSGenericLowering::ReplaceWithCompareIC(Node* node, Token::Value token) {
123-
Callable callable = CodeFactory::CompareIC(isolate(), token);
124-
125-
// Create a new call node asking a CompareIC for help.
126-
NodeVector inputs(zone());
127-
inputs.reserve(node->InputCount() + 1);
128-
inputs.push_back(jsgraph()->HeapConstant(callable.code()));
129-
inputs.push_back(NodeProperties::GetValueInput(node, 0));
130-
inputs.push_back(NodeProperties::GetValueInput(node, 1));
131-
inputs.push_back(NodeProperties::GetContextInput(node));
132-
// Some comparisons (StrictEqual) don't have an effect, control or frame
133-
// state inputs, so handle those cases here.
134-
if (OperatorProperties::GetFrameStateInputCount(node->op()) > 0) {
135-
inputs.push_back(NodeProperties::GetFrameStateInput(node, 0));
136-
}
137-
Node* effect = (node->op()->EffectInputCount() > 0)
138-
? NodeProperties::GetEffectInput(node)
139-
: graph()->start();
140-
inputs.push_back(effect);
141-
Node* control = (node->op()->ControlInputCount() > 0)
142-
? NodeProperties::GetControlInput(node)
143-
: graph()->start();
144-
inputs.push_back(control);
145-
CallDescriptor* desc_compare = Linkage::GetStubCallDescriptor(
146-
isolate(), zone(), callable.descriptor(), 0,
147-
CallDescriptor::kPatchableCallSiteWithNop | FlagsForNode(node),
148-
Operator::kNoProperties, MachineType::IntPtr());
149-
Node* compare =
150-
graph()->NewNode(common()->Call(desc_compare),
151-
static_cast<int>(inputs.size()), &inputs.front());
152-
153-
// Decide how the return value from the above CompareIC can be converted into
154-
// a JavaScript boolean oddball depending on the given token.
155-
Node* false_value = jsgraph()->FalseConstant();
156-
Node* true_value = jsgraph()->TrueConstant();
157-
const Operator* op = nullptr;
158-
switch (token) {
159-
case Token::EQ: // a == 0
160-
case Token::EQ_STRICT:
161-
op = machine()->WordEqual();
162-
break;
163-
case Token::NE: // a != 0 becomes !(a == 0)
164-
case Token::NE_STRICT:
165-
op = machine()->WordEqual();
166-
std::swap(true_value, false_value);
167-
break;
168-
case Token::LT: // a < 0
169-
op = machine()->IntLessThan();
170-
break;
171-
case Token::GT: // a > 0 becomes !(a <= 0)
172-
op = machine()->IntLessThanOrEqual();
173-
std::swap(true_value, false_value);
174-
break;
175-
case Token::LTE: // a <= 0
176-
op = machine()->IntLessThanOrEqual();
177-
break;
178-
case Token::GTE: // a >= 0 becomes !(a < 0)
179-
op = machine()->IntLessThan();
180-
std::swap(true_value, false_value);
181-
break;
182-
default:
183-
UNREACHABLE();
184-
}
185-
Node* booleanize = graph()->NewNode(op, compare, jsgraph()->ZeroConstant());
186-
187-
// Finally patch the original node to select a boolean.
188-
NodeProperties::ReplaceUses(node, node, compare, compare, compare);
189-
node->TrimInputCount(3);
190-
node->ReplaceInput(0, booleanize);
191-
node->ReplaceInput(1, true_value);
192-
node->ReplaceInput(2, false_value);
193-
NodeProperties::ChangeOp(node,
194-
common()->Select(MachineRepresentation::kTagged));
195-
}
196-
101+
#undef REPLACE_RUNTIME_CALL
197102

198103
void JSGenericLowering::ReplaceWithStubCall(Node* node, Callable callable,
199104
CallDescriptor::Flags flags) {
@@ -209,11 +114,12 @@ void JSGenericLowering::ReplaceWithStubCall(Node* node, Callable callable,
209114
void JSGenericLowering::ReplaceWithRuntimeCall(Node* node,
210115
Runtime::FunctionId f,
211116
int nargs_override) {
117+
CallDescriptor::Flags flags = AdjustFrameStatesForCall(node);
212118
Operator::Properties properties = node->op()->properties();
213119
const Runtime::Function* fun = Runtime::FunctionForId(f);
214120
int nargs = (nargs_override < 0) ? fun->nargs : nargs_override;
215-
CallDescriptor* desc = Linkage::GetRuntimeCallDescriptor(
216-
zone(), f, nargs, properties, CallDescriptor::kNeedsFrameState);
121+
CallDescriptor* desc =
122+
Linkage::GetRuntimeCallDescriptor(zone(), f, nargs, properties, flags);
217123
Node* ref = jsgraph()->ExternalConstant(ExternalReference(f, isolate()));
218124
Node* arity = jsgraph()->Int32Constant(nargs);
219125
node->InsertInput(zone(), 0, jsgraph()->CEntryStubConstant(fun->result_size));

‎src/compiler/js-generic-lowering.h‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,6 @@ class JSGenericLowering final : public Reducer {
3636
#undef DECLARE_LOWER
3737

3838
// Helpers to replace existing nodes with a generic call.
39-
void ReplaceWithCompareIC(Node* node, Token::Value token);
4039
void ReplaceWithStubCall(Node* node, Callable c, CallDescriptor::Flags flags);
4140
void ReplaceWithRuntimeCall(Node* node, Runtime::FunctionId f, int args = -1);
4241

‎src/ia32/code-stubs-ia32.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1447,7 +1447,7 @@ void CompareICStub::GenerateGeneric(MacroAssembler* masm) {
14471447
FrameScope scope(masm, StackFrame::INTERNAL);
14481448
__ Push(edx);
14491449
__ Push(eax);
1450-
__ CallRuntime(strict() ? Runtime::kStrictEquals : Runtime::kEquals);
1450+
__ CallRuntime(strict() ? Runtime::kStrictEqual : Runtime::kEqual);
14511451
}
14521452
// Turn true into 0 and false into some non-zero value.
14531453
STATIC_ASSERT(EQUAL == 0);

‎src/interpreter/interpreter.cc‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1144,23 +1144,23 @@ void Interpreter::DoNewWide(InterpreterAssembler* assembler) {
11441144
//
11451145
// Test if the value in the <src> register equals the accumulator.
11461146
void Interpreter::DoTestEqual(InterpreterAssembler* assembler) {
1147-
DoBinaryOp(Runtime::kEquals, assembler);
1147+
DoBinaryOp(Runtime::kEqual, assembler);
11481148
}
11491149

11501150

11511151
// TestNotEqual <src>
11521152
//
11531153
// Test if the value in the <src> register is not equal to the accumulator.
11541154
void Interpreter::DoTestNotEqual(InterpreterAssembler* assembler) {
1155-
DoBinaryOp(Runtime::kNotEquals, assembler);
1155+
DoBinaryOp(Runtime::kNotEqual, assembler);
11561156
}
11571157

11581158

11591159
// TestEqualStrict <src>
11601160
//
11611161
// Test if the value in the <src> register is strictly equal to the accumulator.
11621162
void Interpreter::DoTestEqualStrict(InterpreterAssembler* assembler) {
1163-
DoBinaryOp(Runtime::kStrictEquals, assembler);
1163+
DoBinaryOp(Runtime::kStrictEqual, assembler);
11641164
}
11651165

11661166

@@ -1169,23 +1169,23 @@ void Interpreter::DoTestEqualStrict(InterpreterAssembler* assembler) {
11691169
// Test if the value in the <src> register is not strictly equal to the
11701170
// accumulator.
11711171
void Interpreter::DoTestNotEqualStrict(InterpreterAssembler* assembler) {
1172-
DoBinaryOp(Runtime::kStrictNotEquals, assembler);
1172+
DoBinaryOp(Runtime::kStrictNotEqual, assembler);
11731173
}
11741174

11751175

11761176
// TestLessThan <src>
11771177
//
11781178
// Test if the value in the <src> register is less than the accumulator.
11791179
void Interpreter::DoTestLessThan(InterpreterAssembler* assembler) {
1180-
DoBinaryOp(Runtime::kInterpreterLessThan, assembler);
1180+
DoBinaryOp(Runtime::kLessThan, assembler);
11811181
}
11821182

11831183

11841184
// TestGreaterThan <src>
11851185
//
11861186
// Test if the value in the <src> register is greater than the accumulator.
11871187
void Interpreter::DoTestGreaterThan(InterpreterAssembler* assembler) {
1188-
DoBinaryOp(Runtime::kInterpreterGreaterThan, assembler);
1188+
DoBinaryOp(Runtime::kGreaterThan, assembler);
11891189
}
11901190

11911191

@@ -1194,7 +1194,7 @@ void Interpreter::DoTestGreaterThan(InterpreterAssembler* assembler) {
11941194
// Test if the value in the <src> register is less than or equal to the
11951195
// accumulator.
11961196
void Interpreter::DoTestLessThanOrEqual(InterpreterAssembler* assembler) {
1197-
DoBinaryOp(Runtime::kInterpreterLessThanOrEqual, assembler);
1197+
DoBinaryOp(Runtime::kLessThanOrEqual, assembler);
11981198
}
11991199

12001200

@@ -1203,7 +1203,7 @@ void Interpreter::DoTestLessThanOrEqual(InterpreterAssembler* assembler) {
12031203
// Test if the value in the <src> register is greater than or equal to the
12041204
// accumulator.
12051205
void Interpreter::DoTestGreaterThanOrEqual(InterpreterAssembler* assembler) {
1206-
DoBinaryOp(Runtime::kInterpreterGreaterThanOrEqual, assembler);
1206+
DoBinaryOp(Runtime::kGreaterThanOrEqual, assembler);
12071207
}
12081208

12091209

‎src/mips/code-stubs-mips.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -722,7 +722,7 @@ void CompareICStub::GenerateGeneric(MacroAssembler* masm) {
722722
{
723723
FrameScope scope(masm, StackFrame::INTERNAL);
724724
__ Push(lhs, rhs);
725-
__ CallRuntime(strict() ? Runtime::kStrictEquals : Runtime::kEquals);
725+
__ CallRuntime(strict() ? Runtime::kStrictEqual : Runtime::kEqual);
726726
}
727727
// Turn true into 0 and false into some non-zero value.
728728
STATIC_ASSERT(EQUAL == 0);

‎src/mips64/code-stubs-mips64.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -719,7 +719,7 @@ void CompareICStub::GenerateGeneric(MacroAssembler* masm) {
719719
{
720720
FrameScope scope(masm, StackFrame::INTERNAL);
721721
__ Push(lhs, rhs);
722-
__ CallRuntime(strict() ? Runtime::kStrictEquals : Runtime::kEquals);
722+
__ CallRuntime(strict() ? Runtime::kStrictEqual : Runtime::kEqual);
723723
}
724724
// Turn true into 0 and false into some non-zero value.
725725
STATIC_ASSERT(EQUAL == 0);

‎src/objects-inl.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1353,6 +1353,10 @@ int HeapNumber::get_sign() {
13531353

13541354

13551355
bool Simd128Value::Equals(Simd128Value* that) {
1356+
// TODO(bmeurer): This doesn't match the SIMD.js specification, but it seems
1357+
// to be consistent with what the CompareICStub does, and what is tested in
1358+
// the current SIMD.js testsuite.
1359+
if (this == that) return true;
13561360
#define SIMD128_VALUE(TYPE, Type, type, lane_count, lane_type) \
13571361
if (this->Is##Type()) { \
13581362
if (!that->Is##Type()) return false; \

‎src/runtime/runtime-interpreter.cc‎

Lines changed: 0 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -16,62 +16,6 @@
1616
namespace v8 {
1717
namespace internal {
1818

19-
RUNTIME_FUNCTION(Runtime_InterpreterLessThan) {
20-
HandleScope scope(isolate);
21-
DCHECK_EQ(2, args.length());
22-
CONVERT_ARG_HANDLE_CHECKED(Object, x, 0);
23-
CONVERT_ARG_HANDLE_CHECKED(Object, y, 1);
24-
Maybe<bool> result = Object::LessThan(x, y);
25-
if (result.IsJust()) {
26-
return isolate->heap()->ToBoolean(result.FromJust());
27-
} else {
28-
return isolate->heap()->exception();
29-
}
30-
}
31-
32-
33-
RUNTIME_FUNCTION(Runtime_InterpreterGreaterThan) {
34-
HandleScope scope(isolate);
35-
DCHECK_EQ(2, args.length());
36-
CONVERT_ARG_HANDLE_CHECKED(Object, x, 0);
37-
CONVERT_ARG_HANDLE_CHECKED(Object, y, 1);
38-
Maybe<bool> result = Object::GreaterThan(x, y);
39-
if (result.IsJust()) {
40-
return isolate->heap()->ToBoolean(result.FromJust());
41-
} else {
42-
return isolate->heap()->exception();
43-
}
44-
}
45-
46-
47-
RUNTIME_FUNCTION(Runtime_InterpreterLessThanOrEqual) {
48-
HandleScope scope(isolate);
49-
DCHECK_EQ(2, args.length());
50-
CONVERT_ARG_HANDLE_CHECKED(Object, x, 0);
51-
CONVERT_ARG_HANDLE_CHECKED(Object, y, 1);
52-
Maybe<bool> result = Object::LessThanOrEqual(x, y);
53-
if (result.IsJust()) {
54-
return isolate->heap()->ToBoolean(result.FromJust());
55-
} else {
56-
return isolate->heap()->exception();
57-
}
58-
}
59-
60-
61-
RUNTIME_FUNCTION(Runtime_InterpreterGreaterThanOrEqual) {
62-
HandleScope scope(isolate);
63-
DCHECK_EQ(2, args.length());
64-
CONVERT_ARG_HANDLE_CHECKED(Object, x, 0);
65-
CONVERT_ARG_HANDLE_CHECKED(Object, y, 1);
66-
Maybe<bool> result = Object::GreaterThanOrEqual(x, y);
67-
if (result.IsJust()) {
68-
return isolate->heap()->ToBoolean(result.FromJust());
69-
} else {
70-
return isolate->heap()->exception();
71-
}
72-
}
73-
74-
7519
RUNTIME_FUNCTION(Runtime_InterpreterToBoolean) {
7620
SealHandleScope shs(isolate);
7721
DCHECK_EQ(1, args.length());

0 commit comments

Comments
 (0)