bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

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

RESOLVED FIXED in mozilla34

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: rankov, Assigned: rankov)

Tracking

Trunk
mozilla34
Other
Linux
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
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)

Updated

4 years ago
Assignee: nobody → branislav.rankov
Status: NEW → ASSIGNED
(Assignee)

Comment 1

4 years ago
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.
(Assignee)

Comment 2

4 years ago
Created attachment 8472434 [details] [diff] [review]
Fix-timeout.patch
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+
(Assignee)

Comment 4

4 years ago
Created attachment 8473577 [details] [diff] [review]
Fix-timeout.patch
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+
(Assignee)

Comment 6

4 years ago
Created attachment 8474573 [details] [diff] [review]
Fix-timeout.patch

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
Last Resolved: 4 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.