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

RESOLVED FIXED in mozilla33

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: rankov, Assigned: rankov)

Tracking

Trunk
mozilla33
Other
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 7 obsolete attachments)

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.
Assignee: nobody → branislav.rankov
Depends on: 969375
Posted patch Value-alignment-part1.patch (obsolete) — Splinter Review
Attachment #8419396 - Flags: review?(jdemooij)
The first patch is just follow-up on the code that is already submitted.
Posted patch Value-alignment-part2.patch (obsolete) — Splinter Review
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.
Attachment #8419398 - Flags: review?(jdemooij)
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.
Attachment #8419396 - Flags: review?(jdemooij) → review?(nicolas.b.pierron)
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?
Attachment #8419398 - Flags: review?(jdemooij)
(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 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.
Attachment #8419396 - Flags: review?(nicolas.b.pierron)
Posted patch Value-alignment.patch (obsolete) — Splinter Review
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
Attachment #8419396 - Attachment is obsolete: true
Attachment #8419398 - Attachment is obsolete: true
Attachment #8446497 - Flags: review?(nicolas.b.pierron)
Depends on: 991153
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.
Attachment #8446497 - Flags: review?(nicolas.b.pierron)
(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.
Posted patch Value-alignment.patch (obsolete) — Splinter Review
Attachment #8446497 - Attachment is obsolete: true
Attachment #8448685 - Flags: review?(nicolas.b.pierron)
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
Attachment #8448685 - Flags: review?(nicolas.b.pierron)
I have split the previous patch into two parts:
- Alignment for VMWrapper 
- Alignment for IonCaches
Attachment #8448685 - Attachment is obsolete: true
Attachment #8450129 - Flags: review?(nicolas.b.pierron)
The alignStackForDoubleData cannot be used here because it breaks the framePushed() which is used in buildOOLFakeExitFrame.

I changed solution to align the framePushed().
Attachment #8450137 - Flags: review?(nicolas.b.pierron)
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?
Attachment #8450129 - Flags: review?(nicolas.b.pierron) → review+
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. ")␣{"
Attachment #8450137 - Flags: review?(nicolas.b.pierron) → review+
(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.
Carry reviews from previous patch.
Made requested changes.
Attachment #8450129 - Attachment is obsolete: true
Attachment #8451092 - Flags: review+
Carry review from previous patch.
Attachment #8450137 - Attachment is obsolete: true
Attachment #8451093 - Flags: review+
Rebased patch.
Carry reviews from previous patch.
Attachment #8451093 - Attachment is obsolete: true
Attachment #8451612 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/d7225b8606de
https://hg.mozilla.org/mozilla-central/rev/72b191c155ef
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.