From aaf6340eb3bb114aeee22e19625ea7280b7a3ff7 Mon Sep 17 00:00:00 2001 From: "svenpanne@chromium.org" Date: Thu, 14 Mar 2013 14:42:00 +0000 Subject: [PATCH] Fixed two register allocator bugs (off-by-one error/failure propagation). Minor cleanups on the way, e.g. making sure that we never use something after an allocation failed. Style question: Should we switch to some kind of MUST_USE_RESULT-style to ensure that we handle failures consistently? Not sure... BUG=v8:2576 Review URL: https://codereview.chromium.org/12867002 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@13946 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/arm/lithium-arm.cc | 8 ++++++-- src/ia32/lithium-ia32.cc | 6 ++++-- src/lithium-allocator.cc | 10 ++++++++-- src/lithium-allocator.h | 4 +++- src/mips/lithium-mips.cc | 8 ++++++-- src/x64/lithium-x64.cc | 8 ++++++-- test/cctest/test-heap-profiler.cc | 4 ++-- 7 files changed, 35 insertions(+), 13 deletions(-) diff --git a/src/arm/lithium-arm.cc b/src/arm/lithium-arm.cc index 6961a0d..7272f54 100644 --- a/src/arm/lithium-arm.cc +++ b/src/arm/lithium-arm.cc @@ -675,8 +675,12 @@ LInstruction* LChunkBuilder::AssignPointerMap(LInstruction* instr) { LUnallocated* LChunkBuilder::TempRegister() { LUnallocated* operand = new(zone()) LUnallocated(LUnallocated::MUST_HAVE_REGISTER); - operand->set_virtual_register(allocator_->GetVirtualRegister()); - if (!allocator_->AllocationOk()) Abort("Not enough virtual registers."); + int vreg = allocator_->GetVirtualRegister(); + if (!allocator_->AllocationOk()) { + Abort("Out of virtual registers while trying to allocate temp register."); + return NULL; + } + operand->set_virtual_register(vreg); return operand; } diff --git a/src/ia32/lithium-ia32.cc b/src/ia32/lithium-ia32.cc index f98e6c0..4a380ef 100644 --- a/src/ia32/lithium-ia32.cc +++ b/src/ia32/lithium-ia32.cc @@ -725,10 +725,12 @@ LInstruction* LChunkBuilder::AssignPointerMap(LInstruction* instr) { LUnallocated* LChunkBuilder::TempRegister() { LUnallocated* operand = new(zone()) LUnallocated(LUnallocated::MUST_HAVE_REGISTER); - operand->set_virtual_register(allocator_->GetVirtualRegister()); + int vreg = allocator_->GetVirtualRegister(); if (!allocator_->AllocationOk()) { - Abort("Not enough virtual registers (temps)."); + Abort("Out of virtual registers while trying to allocate temp register."); + return NULL; } + operand->set_virtual_register(vreg); return operand; } diff --git a/src/lithium-allocator.cc b/src/lithium-allocator.cc index e3b9a88..6ee9ef2 100644 --- a/src/lithium-allocator.cc +++ b/src/lithium-allocator.cc @@ -839,8 +839,9 @@ void LAllocator::MeetConstraintsBetween(LInstruction* first, ASSERT(!cur_input->IsUsedAtStart()); LUnallocated* input_copy = cur_input->CopyUnconstrained(zone()); - cur_input->set_virtual_register(GetVirtualRegister()); + int vreg = GetVirtualRegister(); if (!AllocationOk()) return; + cur_input->set_virtual_register(vreg); if (RequiredRegisterKind(input_copy->virtual_register()) == DOUBLE_REGISTERS) { @@ -1924,6 +1925,7 @@ void LAllocator::AllocateBlockedReg(LiveRange* current) { LiveRange* tail = SplitBetween(current, current->Start(), block_pos[reg].InstructionStart()); + if (!AllocationOk()) return; AddToUnhandledSorted(tail); } @@ -1954,6 +1956,7 @@ void LAllocator::SplitAndSpillIntersecting(LiveRange* current) { } else { SpillBetween(range, split_pos, next_pos->pos()); } + if (!AllocationOk()) return; ActiveToHandled(range); --i; } @@ -1972,6 +1975,7 @@ void LAllocator::SplitAndSpillIntersecting(LiveRange* current) { next_intersection = Min(next_intersection, next_pos->pos()); SpillBetween(range, split_pos, next_intersection); } + if (!AllocationOk()) return; InactiveToHandled(range); --i; } @@ -1997,8 +2001,9 @@ LiveRange* LAllocator::SplitRangeAt(LiveRange* range, LifetimePosition pos) { ASSERT(pos.IsInstructionStart() || !chunk_->instructions()->at(pos.InstructionIndex())->IsControl()); - LiveRange* result = LiveRangeFor(GetVirtualRegister()); + int vreg = GetVirtualRegister(); if (!AllocationOk()) return NULL; + LiveRange* result = LiveRangeFor(vreg); range->SplitAt(pos, result, zone_); return result; } @@ -2075,6 +2080,7 @@ void LAllocator::SpillBetween(LiveRange* range, second_part, second_part->Start().InstructionEnd(), end.PrevInstruction().InstructionEnd()); + if (!AllocationOk()) return; ASSERT(third_part != second_part); diff --git a/src/lithium-allocator.h b/src/lithium-allocator.h index 2ca8537..0e24d54 100644 --- a/src/lithium-allocator.h +++ b/src/lithium-allocator.h @@ -427,8 +427,10 @@ class LAllocator BASE_EMBEDDED { Zone* zone() const { return zone_; } int GetVirtualRegister() { - if (next_virtual_register_ > LUnallocated::kMaxVirtualRegisters) { + if (next_virtual_register_ >= LUnallocated::kMaxVirtualRegisters) { allocation_ok_ = false; + // Maintain the invariant that we return something below the maximum. + return 0; } return next_virtual_register_++; } diff --git a/src/mips/lithium-mips.cc b/src/mips/lithium-mips.cc index 757ae1c..f04a462 100644 --- a/src/mips/lithium-mips.cc +++ b/src/mips/lithium-mips.cc @@ -668,8 +668,12 @@ LInstruction* LChunkBuilder::AssignPointerMap(LInstruction* instr) { LUnallocated* LChunkBuilder::TempRegister() { LUnallocated* operand = new(zone()) LUnallocated(LUnallocated::MUST_HAVE_REGISTER); - operand->set_virtual_register(allocator_->GetVirtualRegister()); - if (!allocator_->AllocationOk()) Abort("Not enough virtual registers."); + int vreg = allocator_->GetVirtualRegister(); + if (!allocator_->AllocationOk()) { + Abort("Out of virtual registers while trying to allocate temp register."); + return NULL; + } + operand->set_virtual_register(vreg); return operand; } diff --git a/src/x64/lithium-x64.cc b/src/x64/lithium-x64.cc index 394637c..d9c2f95 100644 --- a/src/x64/lithium-x64.cc +++ b/src/x64/lithium-x64.cc @@ -683,8 +683,12 @@ LInstruction* LChunkBuilder::AssignPointerMap(LInstruction* instr) { LUnallocated* LChunkBuilder::TempRegister() { LUnallocated* operand = new(zone()) LUnallocated(LUnallocated::MUST_HAVE_REGISTER); - operand->set_virtual_register(allocator_->GetVirtualRegister()); - if (!allocator_->AllocationOk()) Abort("Not enough virtual registers."); + int vreg = allocator_->GetVirtualRegister(); + if (!allocator_->AllocationOk()) { + Abort("Out of virtual registers while trying to allocate temp register."); + return NULL; + } + operand->set_virtual_register(vreg); return operand; } diff --git a/test/cctest/test-heap-profiler.cc b/test/cctest/test-heap-profiler.cc index 6962484..2277c3d 100644 --- a/test/cctest/test-heap-profiler.cc +++ b/test/cctest/test-heap-profiler.cc @@ -1731,9 +1731,9 @@ TEST(MapHasDescriptorsAndTransitions) { TEST(ManyLocalsInSharedContext) { v8::HandleScope scope; LocalContext env; - int num_objects = 5000; + int num_objects = 6000; CompileRun( - "var n = 5000;" + "var n = 6000;" "var result = [];" "result.push('(function outer() {');" "for (var i = 0; i < n; i++) {" -- 2.7.4