1007156 - IonMonkey MIPS: Ensure all double and Value arguments are properly aligned.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Branislav Rankov [:rankov]]

MIPS has a requirement that every double variable is aligned to 8 bytes. This is mostly the problem when passing arguments to C++ functions from JIT code.

If we need to pass a pointer to a Value or to a double, we have to ensure it is aligned. There is a chance that compiler will generate sdc1 or ldc1 instruction to access these values. If they are not aligned, program will crash with BUS error.

Comment 1

[Branislav Rankov [:rankov]]

Attachment: Value-alignment-part1.patch

Comment 2

[Branislav Rankov [:rankov]]

The first patch is just follow-up on the code that is already submitted.

Comment 3

[Branislav Rankov [:rankov]]

Attachment: Value-alignment-part2.patch

This patch is temporary fix for issue with passing arguments to C++ functions. 

The problem is that every argument of type WordByRef and DoubleByRef has to be aligned.

This has been done for outParam, but for explicit arguments it is more complicated.

To fix it properly, there are changes to be made in VMFunctions.h, MarkJitExitFrame and JitRuntime::generateVMWrapper.

I would like to have a temporary fix for this so we can start building and testing code soon.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8419396 [details] [diff] [review]
Value-alignment-part1.patch

Sorry but Nicolas is probably a better reviewer for this. Also my review queue is pretty overloaded atm.

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8419398 [details] [diff] [review]
Value-alignment-part2.patch

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

Hm this is pretty hackish... Where's the problematic PrimitiveToObject call?

Comment 6

[Branislav Rankov [:rankov]]

(In reply to Jan de Mooij [:jandem] from comment #5)
> Comment on attachment 8419398 [details] [diff] [review]
> Value-alignment-part2.patch
> 
> Review of attachment 8419398 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hm this is pretty hackish... Where's the problematic PrimitiveToObject call?

The problem starts when you pass a Value pointer that is not 8-byte aligned to a C++ call. If this value is a double, it will be cast into a double and compiler will generate ldc1 instruction to access it. Which will fail. 

Here is where the cast happens.
https://hg.mozilla.org/mozilla-central/file/cf89b5d018f8/js/public/Value.h#l1142

I wanted to make a temporary fix. Once I figure out how to fix this this properly in the places mentioned above, I would remove this hack.

Comment 7

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8419396 [details] [diff] [review]
Value-alignment-part1.patch

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

::: js/src/jit/IonFrames.cpp
@@ +359,5 @@
> +#ifdef JS_CODEGEN_MIPS
> +    // Double values are aligned. If spill address is not aligned, add 4 bytes
> +    // to match code in MacroAssembler::PushRegsInMask.
> +    spill = (uintptr_t *)(uint32_t(spill) & (~(StackAlignment - 1)));
> +#endif

Ok, this match the code in PushRegsInMask, but you need to be aware that I have some allergy regarding the additions of #ifdef in the middle of the code.

Create a static function named "void alignDoubleSpill(uintptr_t **spill)".  And you will define a no-op for other architecture and the previous aligment for MIPS.

@@ +1187,5 @@
> +// be aligned in JitRuntime::generateVMWrapper()
> +#ifdef JS_CODEGEN_MIPS
> +template <>
> +Value *
> +IonExitFooterFrame::outParam<Value>()

We will also be needed other kind of out-params[1], this is the generic case to access any rootable out-param.
We should move the generic definitions in this file.  And same thing as above, put the difference in a small named function used by the generic version.

[1] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/mips/Trampoline-mips.cpp?from=Trampoline-mips.cpp#790,800,819

::: js/src/jit/IonFrames.h
@@ +458,5 @@
> +// We need to specialize this for MIPS because the Value address is forced to
> +// be aligned in JitRuntime::generateVMWrapper()
> +#ifdef JS_CODEGEN_MIPS
> +template <>
> +Value *IonExitFooterFrame::outParam<Value>();

Move the previous definition in IonFrames.cpp, as well as this specialization declaration.

Comment 8

[Branislav Rankov [:rankov]]

Attachment: Value-alignment.patch

I have changed the trampoline to copy the double-sized arguments to an aligned location on stack. This requires some intervention in MarkJitExitFrame.

This change removes the need for the hack from Value-alignment-part2.patch

Comment 9

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8446497 [details] [diff] [review]
Value-alignment.patch

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

::: js/src/jit/IonCaches.cpp
@@ +916,5 @@
>  
>          // masm.leaveExitFrame & pop locals
>          masm.adjustStack(IonOOLNativeExitFrameLayout::Size(0));
> +
> +        masm.restoreStackAlignedForDoubleData();

Can you explain me why this would be needed for this function call and not others in this file?

::: js/src/jit/IonFrames.cpp
@@ +1070,5 @@
> +    uint32_t address = reinterpret_cast<uint32_t>(*pointer);
> +    address = (address - offset) & ~(StackAlignment - 1);
> +    *pointer = reinterpret_cast<uint8_t *>(address);
> +}
> +static void

style-nit: add spaces around functions.

::: js/src/jit/arm/MacroAssembler-arm.h
@@ +1456,5 @@
>  
> +    void alignStackForDoubleData() {
> +        // Exists for MIPS compatibility.
> +    }
> +    void restoreStackAlignedForDoubleData() {

declare these functions in IonMacroAssembler.h
And define them in the MacroAssembler*.cpp of each architecture.

Comment 10

[Branislav Rankov [:rankov]]

(In reply to Nicolas B. Pierron [:nbp] from comment #9)
> ::: js/src/jit/IonCaches.cpp
> @@ +916,5 @@
> >  
> >          // masm.leaveExitFrame & pop locals
> >          masm.adjustStack(IonOOLNativeExitFrameLayout::Size(0));
> > +
> > +        masm.restoreStackAlignedForDoubleData();
> 
> Can you explain me why this would be needed for this function call and not
> others in this file?

This is the only function that currently manifests the problem. Others currently work. I will need to investigate why this is the case. There is a good chance that many other function calls will need this kind of intervention as code changes.

I would like to leave this bug open (or create a new one) to investigate this. We might also want to think about creating a better solution for calls that don't use the VMWrapper.

Comment 11

[Branislav Rankov [:rankov]]

Attachment: Value-alignment.patch

Comment 12

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8448685 [details] [diff] [review]
Value-alignment.patch

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

Note that this is not because it is really hard to test ICs that we should prevent our-self from making the right choices.

::: js/src/jit/IonCaches.cpp
@@ +866,5 @@
>      bool callNative = IsCacheableGetPropCallNative(obj, holder, shape);
>      JS_ASSERT_IF(!callNative, IsCacheableGetPropCallPropertyOp(obj, holder, shape));
>  
>      if (callNative) {
> +        masm.alignStackForDoubleData();

I want to see the same kind of guards for every call within this file.
To make it clear, this should be part of isSaveLive() function.

@@ +916,5 @@
>  
>          // masm.leaveExitFrame & pop locals
>          masm.adjustStack(IonOOLNativeExitFrameLayout::Size(0));
> +
> +        masm.restoreStackAlignedForDoubleData();

and same thing with icRestoreLive.

::: js/src/jit/IonMacroAssembler.h
@@ +958,5 @@
>          return ret;
>      }
>  
> +    void alignStackForDoubleData();
> +    void restoreStackAlignedForDoubleData();

nit: …ForArguments

Comment 13

[Branislav Rankov [:rankov]]

Attachment: 01-Arguments-alignment-VMWrapper.patch

I have split the previous patch into two parts:
- Alignment for VMWrapper 
- Alignment for IonCaches

Comment 14

[Branislav Rankov [:rankov]]

Attachment: 02-Arguments-alignment-IonCaches.patch

The alignStackForDoubleData cannot be used here because it breaks the framePushed() which is used in buildOOLFakeExitFrame.

I changed solution to align the framePushed().

Comment 15

[Branislav Rankov [:rankov]]

Here is the try link:
https://tbpl.mozilla.org/?tree=Try&rev=4956dc88a2fb

Comment 16

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8450129 [details] [diff] [review]
01-Arguments-alignment-VMWrapper.patch

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

::: js/src/jit/IonFrames.cpp
@@ +1088,5 @@
> +}
> +#else
> +void
> +alignDoubleSpillWithOffset(uint8_t **pointer, int32_t offset)
> +{

can you define these no-op in IonFrames.h, and before its use case, such as they are inlined and removed by the compiler.  And for MIPS just add the pre-declarations as you are doing at the moment.

Also avoid "uint8_t **", as this implies that we have to allocate the "uint8_t *address" on the stack to be able to take its address.  Return the new address instead of mutating it.

::: js/src/jit/mips/MacroAssembler-mips.cpp
@@ +3218,5 @@
>  #endif
>  }
>  
>  void
> +MacroAssemblerMIPSCompat::alignStack()

nit: alignStackPointer. (see why below)

@@ +3227,5 @@
> +    storePtr(SecondScratchReg, Address(StackPointer, 0));
> +}
> +
> +void
> +MacroAssemblerMIPSCompat::restoreStackAlignment()

nit: restoreStackPointer.

I think this is better this way, as the goal of the previous function is to align the stack in the case where it was not aligned.  So naming the restore function "restoreAlignment" is weird knowing that the stack was not aligned correctly before.

::: js/src/jit/mips/Trampoline-mips.cpp
@@ +778,5 @@
> +    // Reserve stack for double sized args that are copied to be aligned.
> +    outParamOffset += f.doubleByRefArgs() * sizeof(double);
> +
> +    Register doubleArgs = t0;
> +    masm.reserveStack(outParamOffset);

As a follow-up, we could avoid doing a second reserveStack here by adding the previous if into the switch case, and checking that there is no doubleByRefArgs() here.

@@ +810,5 @@
>              break;
>            case VMFunction::DoubleByRef:
> +            // Copy double sized argument to aligned place.
> +            masm.ma_ld(ScratchFloatReg, Address(argsBase, argDisp));
> +            masm.as_sd(ScratchFloatReg, doubleArgs, doubleArgDisp);

Is it safe to manipulate these bits in float registers?

Comment 17

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8450137 [details] [diff] [review]
02-Arguments-alignment-IonCaches.patch

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

Nice :)

::: js/src/jit/mips/MacroAssembler-mips.cpp
@@ +3235,5 @@
>  
>  void
> +MacroAssembler::alignFrameForICArguments(AfterICSaveLive &aic)
> +{
> +    if (framePushed() % StackAlignment != 0){

style-nit: add space before the curly brace. ")␣{"

Comment 18

[Branislav Rankov [:rankov]]

(In reply to Nicolas B. Pierron [:nbp] from comment #16)
> @@ +810,5 @@
> >              break;
> >            case VMFunction::DoubleByRef:
> > +            // Copy double sized argument to aligned place.
> > +            masm.ma_ld(ScratchFloatReg, Address(argsBase, argDisp));
> > +            masm.as_sd(ScratchFloatReg, doubleArgs, doubleArgDisp);
> 
> Is it safe to manipulate these bits in float registers?

It is save because loadDouble and storeDouble instructions don't interpret data on MIPS. All bits will be preserved regardless of value.

Comment 19

[Branislav Rankov [:rankov]]

Attachment: 01-Arguments-alignment-VMWrapper.patch

Carry reviews from previous patch.
Made requested changes.

Comment 20

[Branislav Rankov [:rankov]]

Attachment: 02-Arguments-alignment-IonCaches.patch

Carry review from previous patch.

Comment 21

[Branislav Rankov [:rankov]]

Attachment: 02-Arguments-alignment-IonCaches.patch

Rebased patch.
Carry reviews from previous patch.

Comment 22

[Branislav Rankov [:rankov]]

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/d7225b8606de
https://hg.mozilla.org/integration/mozilla-inbound/rev/72b191c155ef

Comment 23

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

https://hg.mozilla.org/mozilla-central/rev/d7225b8606de
https://hg.mozilla.org/mozilla-central/rev/72b191c155ef

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c18c3ad45bef
https://hg.mozilla.org/mozilla-central/rev/b5f83ef10359