Reviewers: Jakob, Description: Fixed issue v8:1644 (bug in the chaining of fixup position)
The ARM and MIPS assemblers had a bug where they did not handle the last element in the list of code positions correctly during the fixup of offsets for forward jumps. This happened when the first instruction contained a forward jump to a
label, and that label was used in a forward jump later, too. Unified the code for Assembler::next on ARM and MIPS while we were there. Added test cases, even for ia32/x64, which seem to be correct, even I don't fully understand why... %-} Please review this at http://codereview.chromium.org/7786001/ SVN Base: http://v8.googlecode.com/svn/branches/bleeding_edge/ Affected files: M src/arm/assembler-arm.cc M src/mips/assembler-mips.cc M test/cctest/test-assembler-arm.cc M test/cctest/test-assembler-ia32.cc M test/cctest/test-assembler-mips.cc M test/cctest/test-assembler-x64.cc Index: src/arm/assembler-arm.cc =================================================================== --- src/arm/assembler-arm.cc (revision 9060) +++ src/arm/assembler-arm.cc (working copy) @@ -692,11 +692,11 @@ void Assembler::next(Label* L) { ASSERT(L->is_linked()); int link = target_at(L->pos()); - if (link > 0) { + if (link == kEndOfChain) { + L->Unuse(); + } else { + ASSERT(link >= 0); L->link_to(link); - } else { - ASSERT(link == kEndOfChain); - L->Unuse(); } } Index: src/mips/assembler-mips.cc =================================================================== --- src/mips/assembler-mips.cc (revision 9060) +++ src/mips/assembler-mips.cc (working copy) @@ -780,10 +780,10 @@ void Assembler::next(Label* L) { ASSERT(L->is_linked()); int link = target_at(L->pos()); - ASSERT(link > 0 || link == kEndOfChain); if (link == kEndOfChain) { L->Unuse(); - } else if (link > 0) { + } else { + ASSERT(link >= 0); L->link_to(link); } } Index: test/cctest/test-assembler-arm.cc =================================================================== --- test/cctest/test-assembler-arm.cc (revision 9060) +++ test/cctest/test-assembler-arm.cc (working copy) @@ -1010,4 +1010,18 @@ CHECK_EQ(0xffffffff, i.d); } + +TEST(12) { + // Test chaining of label usages within instructions (issue 1644). + InitializeVM(); + v8::HandleScope scope; + Assembler assm(Isolate::Current(), NULL, 0); + + Label target; + __ b(eq, &target); + __ b(ne, &target); + __ bind(&target); + __ nop(); +} + #undef __ Index: test/cctest/test-assembler-ia32.cc =================================================================== --- test/cctest/test-assembler-ia32.cc (revision 9060) +++ test/cctest/test-assembler-ia32.cc (working copy) @@ -394,4 +394,18 @@ CHECK_EQ(kNaN, f(OS::nan_value(), 1.1)); } + +TEST(AssemblerIa3210) { + // Test chaining of label usages within instructions (issue 1644). + InitializeVM(); + v8::HandleScope scope; + Assembler assm(Isolate::Current(), NULL, 0); + + Label target; + __ j(equal, &target); + __ j(not_equal, &target); + __ bind(&target); + __ nop(); +} + #undef __ Index: test/cctest/test-assembler-mips.cc =================================================================== --- test/cctest/test-assembler-mips.cc (revision 9060) +++ test/cctest/test-assembler-mips.cc (working copy) @@ -1259,4 +1259,18 @@ } } + +TEST(MIPS15) { + // Test chaining of label usages within instructions (issue 1644). + InitializeVM(); + v8::HandleScope scope; + Assembler assm(Isolate::Current(), NULL, 0); + + Label target; + __ beq(v0, v1, &target); + __ bne(v0, v1, &target); + __ bind(&target); + __ nop(); +} + #undef __ Index: test/cctest/test-assembler-x64.cc =================================================================== --- test/cctest/test-assembler-x64.cc (revision 9060) +++ test/cctest/test-assembler-x64.cc (working copy) @@ -46,6 +46,7 @@ using v8::internal::byte; using v8::internal::greater; using v8::internal::less_equal; +using v8::internal::equal; using v8::internal::not_equal; using v8::internal::r13; using v8::internal::r15; @@ -345,4 +346,17 @@ } } + +TEST(AssemblerX64LabelChaining) { + // Test chaining of label usages within instructions (issue 1644). + v8::HandleScope scope; + Assembler assm(Isolate::Current(), NULL, 0); + + Label target; + __ j(equal, &target); + __ j(not_equal, &target); + __ bind(&target); + __ nop(); +} + #undef __ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
