Closed Bug 1177016 Opened 6 years ago Closed 6 years ago

IonMonkey MIPS: Fix a unaligned access caused by bug 1169731

Categories

(Core :: JavaScript Engine: JIT, defect)

Other
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox41 --- affected
firefox42 --- fixed

People

(Reporter: hev, Assigned: hev)

Details

Attachments

(1 file, 1 obsolete file)

In bug 1169731, a new function branchFunctionKind was added. It load a 16-bit word (JSFunction::flags_) by a 32-bit double word load instruction. so, a unaligned access caused on MIPS, may be and ARM?

void branchFunctionKind(Condition cond, JSFunction::FunctionKind kind, Register fun,
                        Register scratch, Label* label)
{
    Address flags(fun, JSFunction::offsetOfFlags());
    load32(flags, scratch);  // I think should replace it by load16ZeroExtend
    and32(Imm32(JSFunction::FUNCTION_KIND_MASK), scratch);
    branch32(cond, scratch, Imm32(kind << JSFunction::FUNCTION_KIND_SHIFT), label);
}
Attached patch Bug1177016.patch (obsolete) — Splinter Review
Attachment #8625643 - Flags: review?(jdemooij)
Assignee: nobody → r
Comment on attachment 8625643 [details] [diff] [review]
Bug1177016.patch

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

Good catch.

::: js/src/jit/MacroAssembler.h
@@ +949,4 @@
>                              Register scratch, Label* label)
>      {
>          Address flags(fun, JSFunction::offsetOfFlags());
> +        load16ZeroExtend(flags, scratch);

See the comments in branchIfFunctionHasNoScript and branchIfInterpreted: they do some magic to avoid 16 bit loads or unaligned 32 bit ones. Could you do something similar here?
Attachment #8625643 - Flags: review?(jdemooij)
(In reply to (PTO until 06/22) Jan de Mooij [:jandem] from comment #2)
> Comment on attachment 8625643 [details] [diff] [review]
> Bug1177016.patch
> 
> Review of attachment 8625643 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Good catch.
> 
> ::: js/src/jit/MacroAssembler.h
> @@ +949,4 @@
> >                              Register scratch, Label* label)
> >      {
> >          Address flags(fun, JSFunction::offsetOfFlags());
> > +        load16ZeroExtend(flags, scratch);
> 
> See the comments in branchIfFunctionHasNoScript and branchIfInterpreted:
> they do some magic to avoid 16 bit loads or unaligned 32 bit ones. Could you
> do something similar here?
Hi, Looks branchIfFunctionHasNoScript and branchIfInterpreted are no problem.

void branchIfFunctionHasNoScript(Register fun, Label* label) {
    // 16-bit loads are slow and unaligned 32-bit loads may be too so
    // perform an aligned 32-bit load and adjust the bitmask accordingly.
    MOZ_ASSERT(JSFunction::offsetOfNargs() % sizeof(uint32_t) == 0);
    MOZ_ASSERT(JSFunction::offsetOfFlags() == JSFunction::offsetOfNargs() + 2);
    Address address(fun, JSFunction::offsetOfNargs());  // It's offsetOfNargs, not offsetOfFlags, offsetOfNargs is 32-bit aligned.
    int32_t bit = IMM32_16ADJ(JSFunction::INTERPRETED);
    branchTest32(Assembler::Zero, address, Imm32(bit), label);
}
void branchIfInterpreted(Register fun, Label* label) {
    // 16-bit loads are slow and unaligned 32-bit loads may be too so
    // perform an aligned 32-bit load and adjust the bitmask accordingly.
    MOZ_ASSERT(JSFunction::offsetOfNargs() % sizeof(uint32_t) == 0);
    MOZ_ASSERT(JSFunction::offsetOfFlags() == JSFunction::offsetOfNargs() + 2);
    Address address(fun, JSFunction::offsetOfNargs());  // Same as the above case.
    int32_t bit = IMM32_16ADJ(JSFunction::INTERPRETED);
    branchTest32(Assembler::NonZero, address, Imm32(bit), label);
}
(In reply to Heiher from comment #3)
> Hi, Looks branchIfFunctionHasNoScript and branchIfInterpreted are no problem.

Yeah these functions are fine. I'm just suggesting changing branchFunctionKind to do something similar to what they are doing.
Attached patch Bug1177016.patchSplinter Review
Attachment #8625643 - Attachment is obsolete: true
Attachment #8625997 - Flags: review?(jdemooij)
Comment on attachment 8625997 [details] [diff] [review]
Bug1177016.patch

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

Thanks!
Attachment #8625997 - Flags: review?(jdemooij) → review+
Keywords: checkin-needed
can we get a try run here ? thanks :)
Keywords: checkin-needed
(In reply to Carsten Book [:Tomcat] from comment #7)
> can we get a try run here ? thanks :)

I have merge it into IonMonkey/MIPS64 (https://github.com/heiher/gecko-dev) and all jit-tests are passed. I have no test on other platforms.
(In reply to Carsten Book [:Tomcat] from comment #7)
> can we get a try run here ? thanks :)

x86_64, all jit-tests passed.
Heiher doesn't yet have tryserver access (needs a voucher in bug 1177980 to get that). Pushed this to try for them: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c80104f98cc

Hopefully that's all the relevant builds/tests this patch needs.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c259755b88fe
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.