760457 - IonMonkey: Assertion failure: unsupported branch relocation, at js/src/ion/arm/Assembler-arm.cpp:531 or Crash [@ js::ion::Assembler::TraceJumpRelocations]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on ionmonkey-arm (private branch) revision  (run with --ion -n -m --ion-eager):


function startTest() {}
startTest();
gc();

Comment 1

[Christian Holler (:decoder)]

I guess this could be the GC failure that Marty mentioned is still present even on his branch.


Crash info on opt-build:

Program received signal SIGSEGV, Segmentation fault.
js::ion::Assembler::TraceJumpRelocations (trc=0x340758, code=0x40a0d080, reader=<value optimized out>) at js/src/ion/arm/Assembler-arm.cpp:588
588             IonCode *child = CodeFromJump((Instruction *) (code->raw() + iter.offset()));
(gdb) x /i $pc
=> 0x1f2a04 <js::ion::Assembler::TraceJumpRelocations(JSTracer*, js::ion::IonCode*, js::ion::CompactBufferReader&)+56>: ldr.w   r3, [r0, #-4]
(gdb) info reg r3 r0
r3             0x52df000        86896640
r0             0x40dc8450       1088193616
(gdb) bt
#0  js::ion::Assembler::TraceJumpRelocations (trc=0x340758, code=0x40a0d080, reader=<value optimized out>) at js/src/ion/arm/Assembler-arm.cpp:588
#1  0x001b727e in js::ion::IonCode::trace (this=0x40a0d080, trc=0x340758) at js/src/ion/Ion.cpp:333
#2  0x0014000a in MarkChildren (this=0x340758, budget=..., tag=<value optimized out>, addr=1084280960) at js/src/gc/Marking.cpp:887
#3  js::GCMarker::processMarkStackOther (this=0x340758, budget=..., tag=<value optimized out>, addr=1084280960) at js/src/gc/Marking.cpp:1076
#4  0x00140632 in processMarkStackTop (this=0x340758, budget=...) at js/src/gc/Marking.cpp:1150
#5  js::GCMarker::drainMarkStack (this=0x340758, budget=...) at js/src/gc/Marking.cpp:1246
#6  0x000505c4 in NonIncrementalMark (rt=0x340678, incremental=<value optimized out>, budget=0, gckind=js::GC_NORMAL) at js/src/jsgc.cpp:3310
#7  GCCycle (rt=0x340678, incremental=<value optimized out>, budget=0, gckind=js::GC_NORMAL) at js/src/jsgc.cpp:3660
#8  0x00050a48 in Collect (rt=0x340678, incremental=<value optimized out>, budget=<value optimized out>, gckind=js::GC_NORMAL, reason=js::gcreason::API) at js/src/jsgc.cpp:3769

Comment 2

[Marty Rosenberg [:mjrosenb]]

Attachment: /home/mrosenberg/patches/early_gc-r0.patch

The issue here is that before and after linking, there need to be two separate methods for indexing into code buffers, and getting the next instruction.  This patch does the dumb thing, and re-indexes into the buffer every time.

Comment 3

[Jacob Bramley [:jbramley]]

Comment on attachment 631666 [details] [diff] [review]
/home/mrosenberg/patches/early_gc-r0.patch

Review of attachment 631666 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ion/arm/Assembler-arm.cpp
@@ +564,2 @@
>  {
> +    Instruction *load = iter->cur();

Call it load1 for consistency with load2. Actually, it'd be best to call them ins1, ins2 and so on, so that the abstraction still works if the instructions aren't loads or branches. For example, in the case where you have "str pc, [sp]", the instruction at "branch" is not a branch.

@@ +585,5 @@
>          // bx r_temp
>          // OR
>          // movw r_temp, #imm1
>          // movt r_temp, #imm2
>          // str pc, [sp]

Assert that the third instruction is what you expect.

@@ +670,5 @@
>  
>  void
>  Assembler::TraceJumpRelocations(JSTracer *trc, IonCode *code, CompactBufferReader &reader)
>  {
>      RelocationIterator iter(reader);

Consider renaming this to "relociter" (or something similar) to be consistent with institer.

@@ +734,5 @@
>              JS_ASSERT(code == IonCode::FromExecutable((uint8*)rp.target));
>          }
>      }
>      if (tmpDataRelocations_.length()) {
> +        ::TraceDataRelocations(trc, &m_buffer, &tmpDataRelocations_);

Did you mean dataRelocations_? (That's what the original code did here.)

@@ +2317,3 @@
>      Register dest;
>      Assembler::RelocStyle rs;
> +    const uint32 *val = getPtr32Target(&iter, &dest, &rs);

Why not use getPointer to avoid all the nasty casts?

::: js/src/ion/arm/Assembler-arm.h
@@ +1948,5 @@
> +    Instruction *i;
> +  public:
> +    InstructionIterator(Instruction *i_) : i(i_) {}
> +    void next() {
> +        i = i->next();

If next() returned a value, you could simplify calling code that retrieves sequences of instructions.

Comment 4

[Marty Rosenberg [:mjrosenb]]

landed + fixed: http://hg.mozilla.org/projects/ionmonkey/rev/8a4f7d5a2fa1