diff --git a/src/compiler/effect-control-linearizer.cc b/src/compiler/effect-control-linearizer.cc index ed907f69f9..fc0ce0ce62 100644 --- a/src/compiler/effect-control-linearizer.cc +++ b/src/compiler/effect-control-linearizer.cc @@ -694,6 +694,9 @@ bool EffectControlLinearizer::TryWireInStateEffect(Node* node, case IrOpcode::kCheckIf: LowerCheckIf(node, frame_state); break; + case IrOpcode::kCheckStringAdd: + result = LowerCheckStringAdd(node, frame_state); + break; case IrOpcode::kCheckedInt32Add: result = LowerCheckedInt32Add(node, frame_state); break; @@ -1544,6 +1547,32 @@ void EffectControlLinearizer::LowerCheckIf(Node* node, Node* frame_state) { __ DeoptimizeIfNot(p.reason(), p.feedback(), value, frame_state); } +Node* EffectControlLinearizer::LowerCheckStringAdd(Node* node, + Node* frame_state) { + Node* lhs = node->InputAt(0); + Node* rhs = node->InputAt(1); + + Node* lhs_length = __ LoadField(AccessBuilder::ForStringLength(), lhs); + Node* rhs_length = __ LoadField(AccessBuilder::ForStringLength(), rhs); + Node* check = __ IntLessThan(__ IntAdd(lhs_length, rhs_length), + __ SmiConstant(String::kMaxLength)); + + __ DeoptimizeIfNot(DeoptimizeReason::kOverflow, VectorSlotPair(), check, + frame_state); + + Callable const callable = + CodeFactory::StringAdd(isolate(), STRING_ADD_CHECK_NONE, NOT_TENURED); + auto call_descriptor = Linkage::GetStubCallDescriptor( + graph()->zone(), callable.descriptor(), 0, CallDescriptor::kNoFlags, + Operator::kNoDeopt | Operator::kNoWrite | Operator::kNoThrow); + + Node* value = + __ Call(call_descriptor, jsgraph()->HeapConstant(callable.code()), lhs, + rhs, __ NoContextConstant()); + + return value; +} + Node* EffectControlLinearizer::LowerCheckedInt32Add(Node* node, Node* frame_state) { Node* lhs = node->InputAt(0); diff --git a/src/compiler/effect-control-linearizer.h b/src/compiler/effect-control-linearizer.h index aa174ed45e..f6a1421290 100644 --- a/src/compiler/effect-control-linearizer.h +++ b/src/compiler/effect-control-linearizer.h @@ -67,6 +67,7 @@ class V8_EXPORT_PRIVATE EffectControlLinearizer { Node* LowerCheckString(Node* node, Node* frame_state); Node* LowerCheckSymbol(Node* node, Node* frame_state); void LowerCheckIf(Node* node, Node* frame_state); + Node* LowerCheckStringAdd(Node* node, Node* frame_state); Node* LowerCheckedInt32Add(Node* node, Node* frame_state); Node* LowerCheckedInt32Sub(Node* node, Node* frame_state); Node* LowerCheckedInt32Div(Node* node, Node* frame_state); diff --git a/src/compiler/js-call-reducer.cc b/src/compiler/js-call-reducer.cc index a06f4490a6..0ebd0663d0 100644 --- a/src/compiler/js-call-reducer.cc +++ b/src/compiler/js-call-reducer.cc @@ -5454,7 +5454,6 @@ Reduction JSCallReducer::ReduceStringPrototypeConcat( } Node* effect = NodeProperties::GetEffectInput(node); Node* control = NodeProperties::GetControlInput(node); - Node* context = NodeProperties::GetContextInput(node); Node* receiver = effect = graph()->NewNode(simplified()->CheckString(p.feedback()), NodeProperties::GetValueInput(node, 1), effect, control); @@ -5463,26 +5462,17 @@ Reduction JSCallReducer::ReduceStringPrototypeConcat( ReplaceWithValue(node, receiver, effect, control); return Replace(receiver); } + + if (!isolate()->IsStringLengthOverflowIntact()) { + return NoChange(); + } + Node* argument = effect = graph()->NewNode(simplified()->CheckString(p.feedback()), NodeProperties::GetValueInput(node, 2), effect, control); - Callable const callable = - CodeFactory::StringAdd(isolate(), STRING_ADD_CHECK_NONE, NOT_TENURED); - auto call_descriptor = - Linkage::GetStubCallDescriptor(graph()->zone(), callable.descriptor(), 0, - CallDescriptor::kNeedsFrameState, - Operator::kNoDeopt | Operator::kNoWrite); - - // TODO(turbofan): Massage the FrameState of the {node} here once we - // have an artificial builtin frame type, so that it looks like the - // exception from StringAdd overflow came from String.prototype.concat - // builtin instead of the calling function. - Node* outer_frame_state = NodeProperties::GetFrameStateInput(node); - - Node* value = effect = control = graph()->NewNode( - common()->Call(call_descriptor), jsgraph()->HeapConstant(callable.code()), - receiver, argument, context, outer_frame_state, effect, control); + Node* value = effect = graph()->NewNode(simplified()->CheckStringAdd(), + receiver, argument, effect, control); ReplaceWithValue(node, value, effect, control); return Replace(value); diff --git a/src/compiler/js-typed-lowering.cc b/src/compiler/js-typed-lowering.cc index 56b1f224c7..ade66952e0 100644 --- a/src/compiler/js-typed-lowering.cc +++ b/src/compiler/js-typed-lowering.cc @@ -530,41 +530,15 @@ Reduction JSTypedLowering::ReduceJSAdd(Node* node) { NodeProperties::ReplaceValueInput(node, reduction.replacement(), 0); } } - // We might be able to constant-fold the String concatenation now. - if (r.BothInputsAre(Type::String())) { - HeapObjectBinopMatcher m(node); - if (m.IsFoldable()) { - StringRef left = m.left().Ref(js_heap_broker()).AsString(); - StringRef right = m.right().Ref(js_heap_broker()).AsString(); - if (left.length() + right.length() > String::kMaxLength) { - // No point in trying to optimize this, as it will just throw. - return NoChange(); - } - // TODO(mslekova): get rid of these allows by doing either one of: - // 1. remove the optimization and check if it ruins the performance - // 2. leave a placeholder and do the actual allocations once back on the - // MT - AllowHandleDereference allow_handle_dereference; - AllowHandleAllocation allow_handle_allocation; - AllowHeapAllocation allow_heap_allocation; - ObjectRef cons( - js_heap_broker(), - factory() - ->NewConsString(left.object(), right.object()) - .ToHandleChecked()); - Node* value = jsgraph()->Constant(cons); - ReplaceWithValue(node, value); - return Replace(value); - } - } // We might know for sure that we're creating a ConsString here. if (r.ShouldCreateConsString()) { return ReduceCreateConsString(node); } - // Eliminate useless concatenation of empty string. if (r.BothInputsAre(Type::String())) { Node* effect = NodeProperties::GetEffectInput(node); Node* control = NodeProperties::GetControlInput(node); + + // Eliminate useless concatenation of empty string. if (r.LeftInputIs(empty_string_type_)) { Node* value = effect = graph()->NewNode(simplified()->CheckString(VectorSlotPair()), @@ -578,7 +552,18 @@ Reduction JSTypedLowering::ReduceJSAdd(Node* node) { ReplaceWithValue(node, value, effect, control); return Replace(value); } + + // TODO(mslekova): Make sure this is executed for cons string + // as well. + if (isolate()->IsStringLengthOverflowIntact()) { + Node* value = graph()->NewNode(simplified()->CheckStringAdd(), r.left(), + r.right(), effect, control); + + ReplaceWithValue(node, value, value); + return Replace(value); + } } + StringAddFlags flags = STRING_ADD_CHECK_NONE; if (!r.LeftInputIs(Type::String())) { flags = STRING_ADD_CONVERT_LEFT; @@ -592,6 +577,7 @@ Reduction JSTypedLowering::ReduceJSAdd(Node* node) { // effects; it can still throw obviously. properties = Operator::kNoWrite | Operator::kNoDeopt; } + // JSAdd(x:string, y) => CallStub[StringAdd](x, y) // JSAdd(x, y:string) => CallStub[StringAdd](x, y) Callable const callable = diff --git a/src/compiler/opcodes.h b/src/compiler/opcodes.h index d6ea247fbc..3fd17fdd11 100644 --- a/src/compiler/opcodes.h +++ b/src/compiler/opcodes.h @@ -372,6 +372,7 @@ V(CheckNotTaggedHole) \ V(CheckEqualsInternalizedString) \ V(CheckEqualsSymbol) \ + V(CheckStringAdd) \ V(CompareMaps) \ V(ConvertReceiver) \ V(ConvertTaggedHoleToUndefined) \ diff --git a/src/compiler/redundancy-elimination.cc b/src/compiler/redundancy-elimination.cc index 5ecef0408b..9ac484bbd3 100644 --- a/src/compiler/redundancy-elimination.cc +++ b/src/compiler/redundancy-elimination.cc @@ -32,6 +32,7 @@ Reduction RedundancyElimination::Reduce(Node* node) { case IrOpcode::kCheckSmi: case IrOpcode::kCheckString: case IrOpcode::kCheckSymbol: + case IrOpcode::kCheckStringAdd: case IrOpcode::kCheckedFloat64ToInt32: case IrOpcode::kCheckedInt32Add: case IrOpcode::kCheckedInt32Div: diff --git a/src/compiler/simplified-lowering.cc b/src/compiler/simplified-lowering.cc index 2d82fc99bc..adc6315dac 100644 --- a/src/compiler/simplified-lowering.cc +++ b/src/compiler/simplified-lowering.cc @@ -3140,6 +3140,7 @@ class RepresentationSelector { case IrOpcode::kArgumentsLengthState: case IrOpcode::kUnreachable: case IrOpcode::kRuntimeAbort: + case IrOpcode::kCheckStringAdd: // All JavaScript operators except JSToNumber have uniform handling. #define OPCODE_CASE(name) case IrOpcode::k##name: JS_SIMPLE_BINOP_LIST(OPCODE_CASE) diff --git a/src/compiler/simplified-operator.cc b/src/compiler/simplified-operator.cc index 0c331bce5e..b47bdda9b6 100644 --- a/src/compiler/simplified-operator.cc +++ b/src/compiler/simplified-operator.cc @@ -790,7 +790,8 @@ bool operator==(CheckMinusZeroParameters const& lhs, V(CheckedInt32Mod, 2, 1) \ V(CheckedInt32Sub, 2, 1) \ V(CheckedUint32Div, 2, 1) \ - V(CheckedUint32Mod, 2, 1) + V(CheckedUint32Mod, 2, 1) \ + V(CheckStringAdd, 2, 1) #define CHECKED_WITH_FEEDBACK_OP_LIST(V) \ V(CheckBounds, 2, 1) \ diff --git a/src/compiler/simplified-operator.h b/src/compiler/simplified-operator.h index df44e899cd..aac85187a1 100644 --- a/src/compiler/simplified-operator.h +++ b/src/compiler/simplified-operator.h @@ -678,6 +678,8 @@ class V8_EXPORT_PRIVATE SimplifiedOperatorBuilder final const Operator* CheckString(const VectorSlotPair& feedback); const Operator* CheckSymbol(); + const Operator* CheckStringAdd(); + const Operator* CheckedFloat64ToInt32(CheckForMinusZeroMode, const VectorSlotPair& feedback); const Operator* CheckedInt32Add(); diff --git a/src/compiler/typer.cc b/src/compiler/typer.cc index 7627d27b08..9a7679d902 100644 --- a/src/compiler/typer.cc +++ b/src/compiler/typer.cc @@ -2001,6 +2001,8 @@ Type Typer::Visitor::TypeCheckSymbol(Node* node) { return Type::Intersect(arg, Type::Symbol(), zone()); } +Type Typer::Visitor::TypeCheckStringAdd(Node* node) { return Type::String(); } + Type Typer::Visitor::TypeCheckFloat64Hole(Node* node) { return typer_->operation_typer_.CheckFloat64Hole(Operand(node, 0)); } diff --git a/src/compiler/verifier.cc b/src/compiler/verifier.cc index 55eaf07711..761b5b83df 100644 --- a/src/compiler/verifier.cc +++ b/src/compiler/verifier.cc @@ -1430,7 +1430,11 @@ void Verifier::Visitor::Check(Node* node, const AllNodes& all) { CheckValueInputIs(node, 0, Type::Any()); CheckTypeIs(node, Type::Symbol()); break; - + case IrOpcode::kCheckStringAdd: + CheckValueInputIs(node, 0, Type::String()); + CheckValueInputIs(node, 1, Type::String()); + CheckTypeIs(node, Type::String()); + break; case IrOpcode::kConvertReceiver: // (Any, Any) -> Receiver CheckValueInputIs(node, 0, Type::Any()); diff --git a/test/js-perf-test/TurboFan/typedLowering.js b/test/js-perf-test/TurboFan/typedLowering.js index d2ce15cc6e..663951f99c 100644 --- a/test/js-perf-test/TurboFan/typedLowering.js +++ b/test/js-perf-test/TurboFan/typedLowering.js @@ -7,7 +7,9 @@ function NumberToString() { var num = 10240; var obj = {}; - for ( var i = 0; i < num; i++ ) + for ( var i = 0; i < num; i++ ) { ret = obj["test" + num]; + } } + createSuite('NumberToString', 1000, NumberToString); diff --git a/test/mjsunit/compiler/string-add-try-catch.js b/test/mjsunit/compiler/string-add-try-catch.js index d7a3d2583c..5ae5b00d18 100644 --- a/test/mjsunit/compiler/string-add-try-catch.js +++ b/test/mjsunit/compiler/string-add-try-catch.js @@ -4,6 +4,9 @@ // Flags: --allow-natives-syntax +// Test that string concatenation overflow (going over string max length) +// is handled gracefully, i.e. an error is thrown + var a = "a".repeat(%StringMaxLength()); (function() { @@ -37,3 +40,57 @@ var a = "a".repeat(%StringMaxLength()); foo("a"); assertInstanceof(foo(a), RangeError); })(); + +(function() { + function foo(a, b) { + try { + return "0123456789012".concat(a); + } catch (e) { + return e; + } + } + + foo("a"); + foo("a"); + %OptimizeFunctionOnNextCall(foo); + foo("a"); + assertInstanceof(foo(a), RangeError); +})(); + +var obj = { + toString: function() { + throw new Error('toString has thrown'); + } +}; + +(function() { + function foo(a, b) { + try { + return "0123456789012" + obj; + } catch (e) { + return e; + } + } + + foo("a"); + foo("a"); + %OptimizeFunctionOnNextCall(foo); + foo("a"); + assertInstanceof(foo(a), Error); +})(); + +(function() { + function foo(a, b) { + try { + return a + 123; + } catch (e) { + return e; + } + } + + foo("a"); + foo("a"); + %OptimizeFunctionOnNextCall(foo); + foo("a"); + assertInstanceof(foo(a), RangeError); +})(); diff --git a/test/unittests/compiler/js-typed-lowering-unittest.cc b/test/unittests/compiler/js-typed-lowering-unittest.cc index 9ce837cd8c..d78b9dccd0 100644 --- a/test/unittests/compiler/js-typed-lowering-unittest.cc +++ b/test/unittests/compiler/js-typed-lowering-unittest.cc @@ -401,12 +401,7 @@ TEST_F(JSTypedLoweringTest, JSAddWithString) { Reduction r = Reduce(graph()->NewNode(javascript()->Add(hint), lhs, rhs, context, frame_state, effect, control)); ASSERT_TRUE(r.Changed()); - EXPECT_THAT(r.replacement(), - IsCall(_, IsHeapConstant( - CodeFactory::StringAdd( - isolate(), STRING_ADD_CHECK_NONE, NOT_TENURED) - .code()), - lhs, rhs, context, frame_state, effect, control)); + EXPECT_THAT(r.replacement(), IsCheckStringAdd(lhs, rhs)); } } // namespace compiler diff --git a/test/unittests/compiler/node-test-utils.cc b/test/unittests/compiler/node-test-utils.cc index 56f18931b4..004b017656 100644 --- a/test/unittests/compiler/node-test-utils.cc +++ b/test/unittests/compiler/node-test-utils.cc @@ -2126,6 +2126,7 @@ IS_BINOP_MATCHER(Float64Sub) IS_BINOP_MATCHER(Float64Mul) IS_BINOP_MATCHER(Float64InsertLowWord32) IS_BINOP_MATCHER(Float64InsertHighWord32) +IS_BINOP_MATCHER(CheckStringAdd) #undef IS_BINOP_MATCHER diff --git a/test/unittests/compiler/node-test-utils.h b/test/unittests/compiler/node-test-utils.h index 30ac330f7f..ddacbe140d 100644 --- a/test/unittests/compiler/node-test-utils.h +++ b/test/unittests/compiler/node-test-utils.h @@ -496,6 +496,9 @@ Matcher IsStackSlot(); Matcher IsSpeculativeToNumber(const Matcher& value_matcher); +Matcher IsCheckStringAdd(const Matcher& lhs_matcher, + const Matcher& rhs_matcher); + // Helpers static inline Matcher IsIntPtrConstant(const intptr_t value) { return kPointerSize == 8 ? IsInt64Constant(static_cast(value))