From a8d4360d65e2089ee94eb660f22c14f9a2fcaa89 Mon Sep 17 00:00:00 2001 From: "ricow@chromium.org" Date: Tue, 15 Feb 2011 14:36:12 +0000 Subject: [PATCH] Make sure we always have room for patching the reloc info during lazy deoptimization (fixes issue 1156). Before we could have calls to builtins that would not be in the relocation info since this used a register as target. Whenever we have this case (from lithium codegen) we now emit a comment in the reloc info. Review URL: http://codereview.chromium.org/6499015 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@6795 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/assembler.cc | 2 +- src/assembler.h | 6 +++++ src/ia32/assembler-ia32.cc | 4 +-- src/ia32/assembler-ia32.h | 5 ++-- src/ia32/deoptimizer-ia32.cc | 4 ++- src/ia32/lithium-codegen-ia32.cc | 25 +++++++++++++++--- test/mjsunit/regress/regress-1156.js | 49 ++++++++++++++++++++++++++++++++++++ 7 files changed, 85 insertions(+), 10 deletions(-) create mode 100644 test/mjsunit/regress/regress-1156.js diff --git a/src/assembler.cc b/src/assembler.cc index ef2094f..42a61c2 100644 --- a/src/assembler.cc +++ b/src/assembler.cc @@ -68,7 +68,7 @@ const double DoubleConstant::min_int = kMinInt; const double DoubleConstant::one_half = 0.5; const double DoubleConstant::minus_zero = -0.0; const double DoubleConstant::negative_infinity = -V8_INFINITY; - +const char* RelocInfo::kFillerCommentString = "DEOPTIMIZATION PADDING"; // ----------------------------------------------------------------------------- // Implementation of Label diff --git a/src/assembler.h b/src/assembler.h index b089b09..1b71dfc 100644 --- a/src/assembler.h +++ b/src/assembler.h @@ -178,6 +178,12 @@ class RelocInfo BASE_EMBEDDED { // invalid/uninitialized position value. static const int kNoPosition = -1; + // This string is used to add padding comments to the reloc info in cases + // where we are not sure to have enough space for patching in during + // lazy deoptimization. This is the case if we have indirect calls for which + // we do not normally record relocation info. + static const char* kFillerCommentString; + enum Mode { // Please note the order is important (see IsCodeTarget, IsGCRelocMode). CONSTRUCT_CALL, // code target that is a call to a JavaScript constructor. diff --git a/src/ia32/assembler-ia32.cc b/src/ia32/assembler-ia32.cc index e4d09f2..6652df2 100644 --- a/src/ia32/assembler-ia32.cc +++ b/src/ia32/assembler-ia32.cc @@ -2607,8 +2607,8 @@ void Assembler::RecordDebugBreakSlot() { } -void Assembler::RecordComment(const char* msg) { - if (FLAG_code_comments) { +void Assembler::RecordComment(const char* msg, bool force) { + if (FLAG_code_comments || force) { EnsureSpace ensure_space(this); RecordRelocInfo(RelocInfo::COMMENT, reinterpret_cast(msg)); } diff --git a/src/ia32/assembler-ia32.h b/src/ia32/assembler-ia32.h index 11f324e..568b4d8 100644 --- a/src/ia32/assembler-ia32.h +++ b/src/ia32/assembler-ia32.h @@ -957,8 +957,9 @@ class Assembler : public Malloced { void RecordDebugBreakSlot(); // Record a comment relocation entry that can be used by a disassembler. - // Use --code-comments to enable. - void RecordComment(const char* msg); + // Use --code-comments to enable, or provide "force = true" flag to always + // write a comment. + void RecordComment(const char* msg, bool force = false); // Writes a single byte or word of data in the code stream. Used for // inline tables, e.g., jump-tables. diff --git a/src/ia32/deoptimizer-ia32.cc b/src/ia32/deoptimizer-ia32.cc index 9a69812..7a98f6f 100644 --- a/src/ia32/deoptimizer-ia32.cc +++ b/src/ia32/deoptimizer-ia32.cc @@ -80,6 +80,7 @@ void Deoptimizer::DeoptimizeFunction(JSFunction* function) { Address prev_address = code_start_address; for (unsigned i = 0; i < table.length(); ++i) { Address curr_address = code_start_address + table.GetPcOffset(i); + ASSERT_GE(curr_address - prev_address, patch_size()); ZapCodeRange(prev_address, curr_address); SafepointEntry safepoint_entry = table.GetEntry(i); @@ -97,7 +98,8 @@ void Deoptimizer::DeoptimizeFunction(JSFunction* function) { RelocInfo::RUNTIME_ENTRY, reinterpret_cast(deopt_entry)); reloc_info_writer.Write(&rinfo); - + ASSERT_GE(reloc_info_writer.pos(), + reloc_info->address() + ByteArray::kHeaderSize); curr_address += patch_size(); } prev_address = curr_address; diff --git a/src/ia32/lithium-codegen-ia32.cc b/src/ia32/lithium-codegen-ia32.cc index a25d678..a59b1a5 100644 --- a/src/ia32/lithium-codegen-ia32.cc +++ b/src/ia32/lithium-codegen-ia32.cc @@ -43,13 +43,20 @@ class SafepointGenerator : public PostCallGenerator { public: SafepointGenerator(LCodeGen* codegen, LPointerMap* pointers, - int deoptimization_index) + int deoptimization_index, + bool ensure_reloc_space = false) : codegen_(codegen), pointers_(pointers), - deoptimization_index_(deoptimization_index) { } + deoptimization_index_(deoptimization_index), + ensure_reloc_space_(ensure_reloc_space) { } virtual ~SafepointGenerator() { } virtual void Generate() { + // Ensure that we have enough space in the reloc info to patch + // this with calls when doing deoptimization. + if (ensure_reloc_space_) { + codegen_->masm()->RecordComment(RelocInfo::kFillerCommentString, true); + } codegen_->RecordSafepoint(pointers_, deoptimization_index_); } @@ -57,6 +64,7 @@ class SafepointGenerator : public PostCallGenerator { LCodeGen* codegen_; LPointerMap* pointers_; int deoptimization_index_; + bool ensure_reloc_space_; }; @@ -2221,7 +2229,8 @@ void LCodeGen::DoApplyArguments(LApplyArguments* instr) { RegisterEnvironmentForDeoptimization(env); SafepointGenerator safepoint_generator(this, pointers, - env->deoptimization_index()); + env->deoptimization_index(), + true); v8::internal::ParameterCount actual(eax); __ InvokeFunction(function, actual, CALL_FUNCTION, &safepoint_generator); } @@ -2292,6 +2301,10 @@ void LCodeGen::CallKnownFunction(Handle function, if (*function == *graph()->info()->closure()) { __ CallSelf(); } else { + // This is an indirect call and will not be recorded in the reloc info. + // Add a comment to the reloc info in case we need to patch this during + // deoptimization. + __ RecordComment(RelocInfo::kFillerCommentString, true); __ call(FieldOperand(edi, JSFunction::kCodeEntryOffset)); } @@ -3731,9 +3744,13 @@ void LCodeGen::DoDeleteProperty(LDeleteProperty* instr) { LEnvironment* env = instr->deoptimization_environment(); RecordPosition(pointers->position()); RegisterEnvironmentForDeoptimization(env); + // Create safepoint generator that will also ensure enough space in the + // reloc info for patching in deoptimization (since this is invoking a + // builtin) SafepointGenerator safepoint_generator(this, pointers, - env->deoptimization_index()); + env->deoptimization_index(), + true); __ mov(esi, Operand(ebp, StandardFrameConstants::kContextOffset)); __ push(Immediate(Smi::FromInt(strict_mode_flag()))); __ InvokeBuiltin(Builtins::DELETE, CALL_FUNCTION, &safepoint_generator); diff --git a/test/mjsunit/regress/regress-1156.js b/test/mjsunit/regress/regress-1156.js new file mode 100644 index 0000000..8ec7f81 --- /dev/null +++ b/test/mjsunit/regress/regress-1156.js @@ -0,0 +1,49 @@ +// Copyright 2011 the V8 project authors. All rights reserved. +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// +// * Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// * Redistributions in binary form must reproduce the above +// copyright notice, this list of conditions and the following +// disclaimer in the documentation and/or other materials provided +// with the distribution. +// * Neither the name of Google Inc. nor the names of its +// contributors may be used to endorse or promote products derived +// from this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +// Flags: --allow-natives-syntax --nouse-inlining + +// Test that we do not crash we invoke builtins from optimized code that +// is then deoptimized. + +function foo(a) { + delete a[1]; + delete a[2]; + delete a[3]; + delete a[4]; + delete a[5]; + return void 0; +} + +function call_and_deopt() { + var b = [1,2,3]; + foo(b); + foo(b); + %DeoptimizeFunction(foo); +} + +call_and_deopt(); -- 2.7.4