This breaks the --code_comments option on ARM... # # Fatal error in src/assembler.cc, line 255 # CHECK(begin_pos - pos_ == RelocInfo::kRelocCommentSize) failed #
Alexandre On Tue, Feb 22, 2011 at 12:35 PM, <[email protected]> wrote: > Revision: 6894 > Author: [email protected] > Date: Tue Feb 22 04:28:33 2011 > Log: Add more generic version of reloc info padding to ensure enough space > for reloc patching during deoptimization (fixes issue 1174). > > The old version only added extra space when we did indirect calls, but > the problem remains the same with normal calls that can be represented > as a single byte. When doing patching each call will always be at > least 2 bytes long because we use RUNTIME_ENTY as the reloc mode. > > > Review URL: http://codereview.chromium.org/6541053 > http://code.google.com/p/v8/source/detail?r=6894 > > Added: > /branches/bleeding_edge/test/mjsunit/regress/regress-1174.js > Modified: > /branches/bleeding_edge/src/assembler.cc > /branches/bleeding_edge/src/assembler.h > /branches/bleeding_edge/src/ia32/lithium-codegen-ia32.cc > /branches/bleeding_edge/src/ia32/lithium-codegen-ia32.h > > ======================================= > --- /dev/null > +++ /branches/bleeding_edge/test/mjsunit/regress/regress-1174.js Tue > Feb 22 04:28:33 2011 > @@ -0,0 +1,43 @@ > +// 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 > + > +// Test that we do not crash when doing deoptimization of a function that > has > +// reloc info that only take up 1 byte per call (like KeyedStoreIC). > + > +function Regular() { > + this[0] >>= 0; > + this[1] ^= 1; > +} > + > +function foo() { > + var regular = new Regular(); > + %DeoptimizeFunction(Regular); > +} > + > +foo(); > ======================================= > --- /branches/bleeding_edge/src/assembler.cc Tue Feb 15 06:36:12 2011 > +++ /branches/bleeding_edge/src/assembler.cc Tue Feb 22 04:28:33 2011 > @@ -228,6 +228,7 @@ > WriteTaggedPC(pc_delta, kEmbeddedObjectTag); > } else if (rmode == RelocInfo::CODE_TARGET) { > WriteTaggedPC(pc_delta, kCodeTargetTag); > + ASSERT(begin_pos - pos_ <= RelocInfo::kMaxCallSize); > } else if (RelocInfo::IsPosition(rmode)) { > // Use signed delta-encoding for data. > intptr_t data_delta = rinfo->data() - last_data_; > @@ -251,6 +252,7 @@ > WriteExtraTaggedPC(pc_delta, kPCJumpTag); > WriteExtraTaggedData(rinfo->data() - last_data_, kCommentTag); > last_data_ = rinfo->data(); > + ASSERT(begin_pos - pos_ == RelocInfo::kRelocCommentSize); > } else { > // For all other modes we simply use the mode as the extra tag. > // None of these modes need a data component. > ======================================= > --- /branches/bleeding_edge/src/assembler.h Tue Feb 15 06:36:12 2011 > +++ /branches/bleeding_edge/src/assembler.h Tue Feb 22 04:28:33 2011 > @@ -184,6 +184,14 @@ > // we do not normally record relocation info. > static const char* kFillerCommentString; > > + // The size of a comment is equal to tree bytes for the extra tagged pc > + > + // the tag for the data, and kPointerSize for the actual pointer to the > + // comment. > + static const int kRelocCommentSize = 3 + kPointerSize; > + > + // The maximum size for a call instruction including pc-jump. > + static const int kMaxCallSize = 6; > + > enum Mode { > // Please note the order is important (see IsCodeTarget, > IsGCRelocMode). > CONSTRUCT_CALL, // code target that is a call to a JavaScript > constructor. > ======================================= > --- /branches/bleeding_edge/src/ia32/lithium-codegen-ia32.cc Mon Feb 21 > 03:29:45 2011 > +++ /branches/bleeding_edge/src/ia32/lithium-codegen-ia32.cc Tue Feb 22 > 04:28:33 2011 > @@ -55,7 +55,7 @@ > // 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_->EnsureRelocSpaceForDeoptimization(); > } > codegen_->RecordSafepoint(pointers_, deoptimization_index_); > } > @@ -78,6 +78,7 @@ > return GeneratePrologue() && > GenerateBody() && > GenerateDeferredCode() && > + GenerateRelocPadding() && > GenerateSafepointTable(); > } > > @@ -120,6 +121,16 @@ > memcpy(copy.start(), builder.Finalize(), copy.length()); > masm()->RecordComment(copy.start()); > } > + > + > +bool LCodeGen::GenerateRelocPadding() { > + int reloc_size = masm()->relocation_writer_size(); > + while (reloc_size < deoptimization_reloc_size.min_size) { > + __ RecordComment(RelocInfo::kFillerCommentString, true); > + reloc_size += RelocInfo::kRelocCommentSize; > + } > + return !is_aborted(); > +} > > > bool LCodeGen::GeneratePrologue() { > @@ -333,6 +344,22 @@ > AddToTranslation(translation, value, environment->HasTaggedValueAt(i)); > } > } > + > + > +void LCodeGen::EnsureRelocSpaceForDeoptimization() { > + // Since we patch the reloc info with RUNTIME_ENTRY calls every patch > + // site will take up 2 bytes + any pc-jumps. > + // We are conservative and always reserver 6 bytes in case where a > + // simple pc-jump is not enough. > + uint32_t pc_delta = > + masm()->pc_offset() - deoptimization_reloc_size.last_pc_offset; > + if (is_uintn(pc_delta, 6)) { > + deoptimization_reloc_size.min_size += 2; > + } else { > + deoptimization_reloc_size.min_size += 6; > + } > + deoptimization_reloc_size.last_pc_offset = masm()->pc_offset(); > +} > > > void LCodeGen::AddToTranslation(Translation* translation, > @@ -382,10 +409,13 @@ > ASSERT(instr != NULL); > LPointerMap* pointers = instr->pointer_map(); > RecordPosition(pointers->position()); > + > if (!adjusted) { > __ mov(esi, Operand(ebp, StandardFrameConstants::kContextOffset)); > } > __ call(code, mode); > + > + EnsureRelocSpaceForDeoptimization(); > RegisterLazyDeoptimization(instr); > > // Signal that we don't inline smi code before these stubs in the > @@ -2300,11 +2330,8 @@ > 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)); > + EnsureRelocSpaceForDeoptimization(); > } > > // Setup deoptimization. > ======================================= > --- /branches/bleeding_edge/src/ia32/lithium-codegen-ia32.h Mon Feb 14 > 15:41:47 2011 > +++ /branches/bleeding_edge/src/ia32/lithium-codegen-ia32.h Tue Feb 22 > 04:28:33 2011 > @@ -60,6 +60,7 @@ > status_(UNUSED), > deferred_(8), > osr_pc_offset_(-1), > + deoptimization_reloc_size(), > resolver_(this) { > PopulateDeoptimizationLiteralsWithInlinedFunctions(); > } > @@ -102,6 +103,8 @@ > // Emit frame translation commands for an environment. > void WriteTranslation(LEnvironment* environment, Translation* > translation); > > + void EnsureRelocSpaceForDeoptimization(); > + > // Declare methods that deal with the individual node types. > #define DECLARE_DO(type) void Do##type(L##type* node); > LITHIUM_CONCRETE_INSTRUCTION_LIST(DECLARE_DO) > @@ -151,6 +154,9 @@ > bool GeneratePrologue(); > bool GenerateBody(); > bool GenerateDeferredCode(); > + // Pad the reloc info to ensure that we have enough space to patch > during > + // deoptimization. > + bool GenerateRelocPadding(); > bool GenerateSafepointTable(); > > void CallCode(Handle<Code> code, RelocInfo::Mode mode, LInstruction* > instr, > @@ -251,6 +257,13 @@ > ZoneList<LDeferredCode*> deferred_; > int osr_pc_offset_; > > + struct DeoptimizationRelocSize { > + int min_size; > + int last_pc_offset; > + }; > + > + DeoptimizationRelocSize deoptimization_reloc_size; > + > // Builder that keeps track of safepoints in the code. The table > // itself is emitted at the end of the generated code. > SafepointTableBuilder safepoints_; > > -- > v8-dev mailing list > [email protected] > http://groups.google.com/group/v8-dev -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
