1177016 - IonMonkey MIPS: Fix a unaligned access caused by bug 1169731

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[hev [:hev]]

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);
}

Comment 1

[hev [:hev]]

Attachment: Bug1177016.patch

Comment 2

[Jan de Mooij [:jandem]]

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?

Comment 3

[hev [:hev]]

(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);
}

Comment 4

[Jan de Mooij [:jandem]]

(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.

Comment 5

[hev [:hev]]

Attachment: Bug1177016.patch

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8625997 [details] [diff] [review]
Bug1177016.patch

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

Thanks!

Comment 7

[Carsten Book [:Tomcat]]

can we get a try run here ? thanks :)

Comment 8

[hev [:hev]]

(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.

Comment 9

[hev [:hev]]

(In reply to Carsten Book [:Tomcat] from comment #7)
> can we get a try run here ? thanks :)

x86_64, all jit-tests passed.

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c259755b88fe

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c259755b88fe