1050219 - IonMonkey MIPS: Tests parallel/gc-timeout.js and parallel/timeout.js fail randomly.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Branislav Rankov [:rankov]]

Tests parallel/gc-timeout.js and parallel/timeout.js fail randomly.

The cause of failure is segfault. The failure happens approximately once in 20 runs of the test.

Comment 1

[Branislav Rankov [:rankov]]

The problem here is that backedges are patched while patched loop is running. Since patching changes 2 instrutions (lui + ori), patching can cause inconsistent state. If execution is interrupted between lui and ori, and then these instructions are patched, the jump can go to a wrong address.

I fixed this by adding two sets of lui + ori which will not change during runtime (one jumps to loop header, and the other one to interrupt check). There is single branch instruction that is patched during runtime. 

Also, long jumps (lui + ori) are not used if they are not needed.

Comment 2

[Branislav Rankov [:rankov]]

Attachment: Fix-timeout.patch

Comment 3

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8472434 [details] [diff] [review]
Fix-timeout.patch

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

::: js/src/jit/mips/Assembler-mips.cpp
@@ +172,5 @@
> +    uint32_t sourceAddr = (uint32_t)jump.raw();
> +    uint32_t targetAddr = (uint32_t)label.raw();
> +    InstImm *branch = (InstImm *)jump.raw();
> +
> +    MOZ_ASSERT(branch->extractOpcode() == ((uint32_t)op_beq >> OpcodeShift));

use C++ cast for uint32_t: uint32_t(…).

@@ +183,5 @@
> +            branch->setBOffImm16(BOffImm16(2 * sizeof(uint32_t)));
> +            Instruction *lui = &branch[1];
> +            Assembler::UpdateLuiOriValue(lui, lui->next(), targetAddr);
> +        } else {
> +            branch->setBOffImm16(BOffImm16(4 * sizeof(uint32_t)));

We should update this branch last, to ensure that other modification to lui & ori are correct.

::: js/src/jit/mips/MacroAssembler-mips.cpp
@@ +2815,5 @@
> +        MOZ_ASSERT(BOffImm16::IsInRange(offset));
> +        as_b(BOffImm16(offset));
> +    } else {
> +        // Jump to lui and ori pair is default.
> +        as_b(BOffImm16(2 * sizeof(uint32_t)));

Do we have a way to assert that this is the right jump size?

@@ +2818,5 @@
> +        // Jump to lui and ori pair is default.
> +        as_b(BOffImm16(2 * sizeof(uint32_t)));
> +    }
> +    // No need for nop here. We can safely put next instruction in delay slot.
> +    ma_liPatchable(ScratchRegister, Imm32(dest));

This deserve a comment because half of this instruction is always executed and the other half is only rarely executed in case of far backedges.

Comment 4

[Branislav Rankov [:rankov]]

Attachment: Fix-timeout.patch

Comment 5

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8473577 [details] [diff] [review]
Fix-timeout.patch

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

Nice work!

::: js/src/jit/mips/MacroAssembler-mips.cpp
@@ +2823,5 @@
> + *      ori at, addr2_lo
> + *      jr at
> + *      nop                # In delay slot.
> + *
> + * The backedge i done this way to avoid patching lui+ori pair while it is

typo: s/ i / is /

@@ +2843,5 @@
> +        MOZ_ASSERT(BOffImm16::IsInRange(offset));
> +        as_b(BOffImm16(offset));
> +    } else {
> +        // Jump to ori is default. The lui is executed in delay slot.
> +        as_b(BOffImm16(2 * sizeof(uint32_t)));

comment-nit: // Jump to "label1" by default to jump to the loop header.

Comment 6

[Branislav Rankov [:rankov]]

Attachment: Fix-timeout.patch

Fixed comments.
Carry the review from previous patch.

Comment 7

[Branislav Rankov [:rankov]]

https://tbpl.mozilla.org/?tree=Try&rev=b6db2f2de7e1

Comment 8

[Branislav Rankov [:rankov]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/32628ddca30c

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/32628ddca30c