Closed Bug 1050219 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect)

Other
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: rankov, Assigned: rankov)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Assignee: nobody → branislav.rankov
Status: NEW → ASSIGNED
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.
Attached patch Fix-timeout.patch (obsolete) — Splinter Review
Attachment #8472434 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8472434 - Flags: review?(nicolas.b.pierron) → feedback+
Attached patch Fix-timeout.patch (obsolete) — Splinter Review
Attachment #8472434 - Attachment is obsolete: true
Attachment #8473577 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8473577 - Flags: review?(nicolas.b.pierron) → review+
Fixed comments.
Carry the review from previous patch.
Attachment #8473577 - Attachment is obsolete: true
Attachment #8474573 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/32628ddca30c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Depends on: 1055911
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.