Closed Bug 732950 Opened 8 years ago Closed 8 years ago

IonMonkey: some bugs in the new CallWithABI on ARM


(Core :: JavaScript Engine, defect)

Not set





(Reporter: mjrosenb, Unassigned)



(1 file)

the argc check was in place, but otherwise non-existent, a typo, and a logical error in copying code from x86 a very long time ago.
Attachment #602881 - Flags: review?(nicolas.b.pierron)
Comment on attachment 602881 [details] [diff] [review]

Review of attachment 602881 [details] [diff] [review]:

I think both x86/arm implementations are lacking comments.  I tried to explain most of the immediate values which are present in x86 implementation, which is currently working fine.
Your fix is almost correct (issue with dynamic. aligned & calls with odd number of arguments), but you should reuse the same idea as x86 to having uncorrelated changes between ARM and x86 callWithABI implementations.

::: js/src/ion/arm/MacroAssembler-arm.cpp
@@ +2029,2 @@
>      ma_and(Imm32(~(StackAlignment - 1)), sp, sp);
> +    ma_str(scratch, Operand(sp,0));

Improve the comment above to state that you are allocating space for the saving the previous stack pointer state while aligning the stack.

The way it is handled in x86 is by providing an extra STACK_SLOT for the push as part of the alignment, so you can get rid of this modification by using

// STACK_SLOT_SIZE account for the saved stack pointer pushed by setupUnalignedABICall
stackAdjust += ComputeByteAlignment(stackAdjust + STACK_SLOT_SIZE, StackAlignment);

instead of

stackAdjust += ComputeByteAlignment(stackAdjust, StackAlignment);

(as suggested below).

@@ +2091,5 @@
>  void
>  MacroAssemblerARMCompat::callWithABI(void *fun, Result result)
>  {
>      JS_ASSERT(inCall_);
> +    uint32 stackAdjust = ((usedSlots_ > 4) ? usedSlots_ - 4 : 0) * STACK_SLOT_SIZE;

you may want to use « NumArgRegs » instead of «4».

@@ +2096,2 @@
>      if (!dynamicAlignment_)
> +        stackAdjust += (framePushed_ + stackAdjust) & 7;

I think you still need the «8 - …» to aligned the head of the stack because you want to aadd the lacking bytes (8 - …) and you should use ComputeByteAlignment functions like other implementations:

stackAdjust += ComputeByteAlignment(framePushed + stackAdjust, StackAlignment);

If there is a dynamic alignment, you must ensure that the stack space is still a multiple of the alignment, otherwise unaligned calls with odd number of arguments would be unaligned.

stackAdjust += ComputeByteAlignment(stackAdjust, StackAlignment);

::: js/src/ion/arm/Trampoline-arm.cpp
@@ +651,1 @@
>      JS_ASSERT(f.argc() == argc);

On x86, These argc++ are part of passABIArg and this assert is checked at the beginning of callWithABI.  You may want to keep the implementation similar.
Attachment #602881 - Flags: review?(nicolas.b.pierron)
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.