Closed Bug 692114 Opened 13 years ago Closed 13 years ago

IonMonkey: ABICall: Handle Value size objects as returned value.

Categories

(Core :: JavaScript Engine, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: nbp, Assigned: nbp)

References

Details

Attachments

(1 file, 3 obsolete files)

+++ This bug was initially created as a clone of Bug #685228 +++

On x86 the size of js::Value is biger than a word size.  This cause cdecl to use the first argument place to store a pointer.  The pointer targets a memory area of the size of the returned value.  This "argument" is unwind by the callee instead of the caller which is the opposite of the cdecl convention call for arguments.

Due to the fact that this first special argument is handled differently than other arguments, we have to take care of it inside the setup*ABICall, setABIArg and callWithABI.

The pointer given as first argument is returned in the return register eax.  Thus we can even forget about the allocated space and use eax to recover the content of the area.

I suggest that setup*ABICall and callWithABI functions allocate/free the space for the returned value while hiding the shift of arguments.  The returned value can still be reached before the next call.
Attached patch Handle fat returned value. (obsolete) — Splinter Review
Attachment #564858 - Flags: review?(dvander)
Comment on attachment 564858 [details] [diff] [review]
Handle fat returned value.

Flipping review to Sean since he is closer to the ABI right now
Attachment #564858 - Flags: review?(dvander) → review?(sstangl)
Comment on attachment 564858 [details] [diff] [review]
Handle fat returned value.

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

The general concept is reasonable, but there are issues with stack management, hopefully explained in the comments below. I am reasonably certain that the only way to implement these calls without violating abstractions is to require the caller of setupABICall() to provide memory which the implied 0th argument will reference -- but then it no longer makes sense to have a single generic function for all ABI calls. The caller must know that it requires this 0th argument, and must itself handle the memory involved.

::: js/src/ion/IonMacroAssembler.cpp
@@ +79,5 @@
>  
> +    returnSize_ = returnSize;
> +    reserveReturn_ = returnSize_ > sizeof(void *);
> +    if (reserveReturn_)
> +        reserveStack(returnSize_);

These changes make setupAlignedABICall() and setupUnalignedABICall() a little bit too large to justify code duplication -- but they only actually differ on 2 lines. Could we eliminate the redundancy by moving the code into a private helper function that sets dynamicAlignment_ based on a boolean parameter?

@@ +81,5 @@
> +    reserveReturn_ = returnSize_ > sizeof(void *);
> +    if (reserveReturn_)
> +        reserveStack(returnSize_);
> +
> +    args += reserveReturn_ ? 1 : 0;

This branch/addition can be integrated into the "if (reserveReturn_)" block above. The entire block may then deserve a comment.

@@ +94,5 @@
>      dynamicAlignment_ = true;
>      reserveStack(stackAdjust_);
> +
> +    if (reserveReturn_)
> +        setABIArgOrRet(0, MoveOperand(scratch));

This is not clear to me -- if the memory for the return value is reserved by this function, why isn't it being loaded into a register? It seems that only this function could know the right address for the 0th argument, without violating abstractions.

@@ +119,4 @@
>  void
>  MacroAssembler::callWithABI(void *fun)
>  {
> +    // Perform argument move resolution now.

How about "if (!NumArgRegs && !reserveReturn_) return;" ?

@@ +135,5 @@
>      call(fun);
>  
> +    // cdecl call convention pop the return pointer argument and its value
> +    // inside ReturnReg.
> +    stackAdjust_ -= reserveReturn_ ? STACK_SLOT_SIZE : 0;

This logic looks incorrect. Above, the stack is one allocation, calculated by stackForArgs, with the implied 0th argument counting as an arg.

Now below, in the case of dynamic alignment, we free *part* of this stack, then attempt to pop %esp. But %esp is not located there (since we did a single allocation of greater size than is freed); and the free below is nonsensical anyway, since %esp has already been popped to its original location.

@@ +142,5 @@
>      if (dynamicAlignment_)
>          restoreStackFromDynamicAlignment();
>  
> +    if (reserveReturn_)
> +        freeStack(returnSize_);

This is unsafe: the return value is stored to stack, but this code frees that memory region. Thus the return value may be clobbered before it is read. There needs to be a way to pass the return value to the code that performs callWithABI() without violating abstractions.

::: js/src/ion/IonMacroAssembler.h
@@ +96,3 @@
>      bool enoughMemory_;
>  
> +    void setABIArgOrRet(uint32 arg, const MoveOperand &from);

This function name is a bit misleading: it's not that it can set the return value, but that there exists an additional implied argument that provides a safe memory location into which the return value may be written.

@@ +147,5 @@
>      // temporarily use more stack, in which case esp-relative addresses will be
>      // automatically adjusted. It is extremely important that esp-relative
>      // addresses are computed *after* setupABICall().
> +    void setABIArg(uint32 arg, const MoveOperand &from)
> +    {

SpiderMonkey style leaves { on the first line for inlined functions.
(https://wiki.mozilla.org/JavaScript:SpiderMonkey:C++_Coding_Style : Inline Methods, paragraph 2.)

@@ +155,3 @@
>  
> +    void setABIArg(uint32 arg, const Register &reg)
> +    {

Brace to first line.
Attachment #564858 - Flags: review?(sstangl)
(In reply to Sean Stangl from comment #3)
> Comment on attachment 564858 [details] [diff] [review] [diff] [details] [review]
> Handle fat returned value.
> 
> Review of attachment 564858 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> The general concept is reasonable, but there are issues with stack
> management, hopefully explained in the comments below. I am reasonably
> certain that the only way to implement these calls without violating
> abstractions is to require the caller of setupABICall() to provide memory
> which the implied 0th argument will reference -- but then it no longer makes
> sense to have a single generic function for all ABI calls. The caller must
> know that it requires this 0th argument, and must itself handle the memory
> involved.

I agree that at best the caller must allocate the space it-self, but doing so create a difference between register returned value and memory returned value.  We don't want to show any differences in the caller generator code to keep the ability to use ABICall in shared assembly code.

> ::: js/src/ion/IonMacroAssembler.cpp
> @@ +79,5 @@
> >  
> > +    returnSize_ = returnSize;
> > +    reserveReturn_ = returnSize_ > sizeof(void *);
> > +    if (reserveReturn_)
> > +        reserveStack(returnSize_);
> 
> These changes make setupAlignedABICall() and setupUnalignedABICall() a
> little bit too large to justify code duplication -- but they only actually
> differ on 2 lines. Could we eliminate the redundancy by moving the code into
> a private helper function that sets dynamicAlignment_ based on a boolean
> parameter?

Sure.

> @@ +94,5 @@
> >      dynamicAlignment_ = true;
> >      reserveStack(stackAdjust_);
> > +
> > +    if (reserveReturn_)
> > +        setABIArgOrRet(0, MoveOperand(scratch));
> 
> This is not clear to me -- if the memory for the return value is reserved by
> this function, why isn't it being loaded into a register? It seems that only
> this function could know the right address for the 0th argument, without
> violating abstractions.

In case of the setupUnalignedABICall, the space is allocated before the stack is aligned.  The function proceed as follow:

- The space of the return value is reserved by moving the stack pointer. (sp = &ret)
- The stack pointer is saved in the scratch register. (scratch = &ret)
- The stack pointer is dynamically aligned. (scratch = &ret)
- The first operand is defined by using the scratch register.

One change compared to the previous version is that the scratch register now should live until the callWithABI, otherwise the return address would be erase.  The stack pointer cannot be used because it is modified by the aligned procedure.  Another option would be to allocate the space for the return value after the stack alignment, which would make it identical for aligned/unaligned setup.

> @@ +119,4 @@
> >  void
> >  MacroAssembler::callWithABI(void *fun)
> >  {
> > +    // Perform argument move resolution now.
> 
> How about "if (!NumArgRegs && !reserveReturn_) return;" ?

I am not sure to understand.  Do you mean:

  if (stackAdjust_) {
      // moveResolver stuff
  }

?

> @@ +135,5 @@
> >      call(fun);
> >  
> > +    // cdecl call convention pop the return pointer argument and its value
> > +    // inside ReturnReg.
> > +    stackAdjust_ -= reserveReturn_ ? STACK_SLOT_SIZE : 0;
> 
> This logic looks incorrect. Above, the stack is one allocation, calculated
> by stackForArgs, with the implied 0th argument counting as an arg.
> 
> Now below, in the case of dynamic alignment, we free *part* of this stack,
> then attempt to pop %esp. But %esp is not located there (since we did a
> single allocation of greater size than is freed); and the free below is
> nonsensical anyway, since %esp has already been popped to its original
> location.

When compiled with cdecl, the pointer to the return value is given as the first argument but as commented in Comment 0, the first argument is unwind by the callee and not the caller.

        ; eax targets the memory area of the return value.
  ret 4 ; unwind the first push corresponding to the return value pointer, but the rest of the
        ; arguments have to be unwind by the callee.

One thing which troubles me is about the stack alignment, what should it be, I found no reference to this fat value return related to stack alignment of made by gcc.  I may have to dig into gcc code to find out.

> @@ +142,5 @@
> >      if (dynamicAlignment_)
> >          restoreStackFromDynamicAlignment();
> >  
> > +    if (reserveReturn_)
> > +        freeStack(returnSize_);
> 
> This is unsafe: the return value is stored to stack, but this code frees
> that memory region. Thus the return value may be clobbered before it is
> read. There needs to be a way to pass the return value to the code that
> performs callWithABI() without violating abstractions.

One way would be to provide a function which needs to be called right after callWithABI for copying the returned value in a safe location.

> @@ +147,5 @@
> >      // temporarily use more stack, in which case esp-relative addresses will be
> >      // automatically adjusted. It is extremely important that esp-relative
> >      // addresses are computed *after* setupABICall().
> > +    void setABIArg(uint32 arg, const MoveOperand &from)
> > +    {
> 
> SpiderMonkey style leaves { on the first line for inlined functions.
> (https://wiki.mozilla.org/JavaScript:SpiderMonkey:C++_Coding_Style : Inline
> Methods, paragraph 2.)

Thanks for the pointer.
I applied the modification and tried to avoid violation by introducing a pattern for fetching the result similar as the pattern for setting arguments.  Thus the allocated space should be safe until finalizeABICall is called.
Attachment #564858 - Attachment is obsolete: true
Attachment #565974 - Flags: review?(sstangl)
Comment on attachment 565974 [details] [diff] [review]
Handle larger-than-word-size return value.

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

This is very, very close -- just some memory management in IonMacroAssembler.cpp requires changes. We need the caller /of setupABICall()/ to allocate the rval memory itself.

We also will need Trampoline-arm.cpp support as well; a follow-up patch is fine.
I asked mjrosenb how this is handled -- apparently, ARM will return a value of size 2*WORD_SIZE in %r0 and %r1. Above 2*WORD_SIZE, the convention looks very much like with x86, but we don't have to deal with that.

::: js/src/ion/IonMacroAssembler.cpp
@@ +167,5 @@
> +    MoveOperand from;
> +
> +    callProperties_ |= hasGetRes;
> +    if (callProperties_ & largeReturnValue)
> +    {

if (callProperties_ & largeReturnValue) {

@@ +172,5 @@
> +        // large return values are stored where the return register is pointing at.
> +        from = MoveOperand(ReturnReg, offset);
> +    }
> +    else
> +    {

} else {

@@ +182,5 @@
> +    enoughMemory_ &= moveResolver_.addMove(from, to, Move::GENERAL);
> +}
> +
> +void
> +MacroAssembler::finalizeABICall()

What is the benefit of finalizeABICall()? It looks like it's only useful after callWithABI().

@@ +188,5 @@
> +    JS_ASSERT(inCall_);
> +
> +    // Perform result move resolution now.
> +    if (callProperties_ & hasGetRes)
> +    {

if (callProperties_ & hasGetRes) {

@@ +206,4 @@
>  
>      freeStack(stackAdjust_);
>      if (dynamicAlignment_)
>          restoreStackFromDynamicAlignment();

1) The memory for the return value is freed before it is consumed by the caller (it is part of the reserveStack() allocation) -- but there is an abstraction violation, since the caller of finalizeABICall() needs to know where that memory is and how to free it, without being told. We really want the memory for the return value to be explicitly allocated by the /caller of setupABICall()/, then /passed into that function as an Operand()/, then /explicitly freed/. All of this memory traffic should be dead-obvious to people reading code in, e.g., GenerateBailoutThunk().

2) With the current scheme, in the case of dynamic memory alignment, the location of the return value is unknown. The solution to this is also addressed by having the memory region passed into setupABICall() as on Operand.

::: js/src/ion/IonMacroAssembler.h
@@ +104,5 @@
>  
> +    // Compute space needed for the function call and set the properties of the
> +    // callee.  It returns the space which has to be allocated for calling the
> +    // function.
> +    uint32 setupABICall(uint32 arg, uint32 returnSize);

Please use "size_t returnSize" in all the locations this occurs, asserting that the value is either sizeof(void *) or 2*sizeof(void *).

::: js/src/ion/x64/Trampoline-x64.cpp
@@ +251,4 @@
>  
>      // Load the value the interpreter returned.
>      masm.movq(Operand(rsp, 0), JSReturnReg);
>      masm.addq(Imm32(8), rsp);

masm.pop(JSReturnReg); // I do realize this isn't your code :)

::: js/src/ion/x86/Trampoline-x86.cpp
@@ +268,5 @@
>  
>      // Load the value the interpreter returned.
>      masm.movl(Operand(esp, 4), JSReturnReg_Type);
>      masm.movl(Operand(esp, 0), JSReturnReg_Data);
>      masm.addl(Imm32(8), esp);

masm.pop(JSReturnReg_Data);
masm.pop(JSReturnReg_Type);
Attachment #565974 - Flags: review?(sstangl)
(In reply to Sean Stangl from comment #6)
> Comment on attachment 565974 [details] [diff] [review] [diff] [details] [review]
> Handle larger-than-word-size return value.
> 
> Review of attachment 565974 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> This is very, very close -- just some memory management in
> IonMacroAssembler.cpp requires changes. We need the caller /of
> setupABICall()/ to allocate the rval memory itself.

That's exactly what I want to avoid in order to merge C wrapper generators of x86 and x64.  Currently they are extremely similar and would prefer to merge them instead of keeping 2 almost identical pieces of code.  handling the return value outside the ABI call is one source of error which break the ability to merge wrapper generators.

Unless handling return value memory can be made as noop for architecture on which is value is returned as usual, the solution is not helpful.  The problem is that handling an imaginary return value consume one register which is not used by the call.

The current live scope of the result is between setupABICall and finalizeABICall and you need to call getABIRes to access the returned value.

While writing the C code generator for x86 and x64, I've focused on expressing the same way the things which are similar.  Thus I don't write plain register names but I prefer to allocate them from a register set.  This may slow down the generation a bit (crossing fingers to get help from constant propagation of the compiler) but this ease maintaining the two versions.

> We also will need Trampoline-arm.cpp support as well; a follow-up patch is
> fine.
> I asked mjrosenb how this is handled -- apparently, ARM will return a value
> of size 2*WORD_SIZE in %r0 and %r1. Above 2*WORD_SIZE, the convention looks
> very much like with x86, but we don't have to deal with that.

Then we will have to modify getABIArg to handle the case of NumResReg & GetResReg for the result which would be similar to NumArgReg & GetArgReg for the arguments.

> @@ +182,5 @@
> > +    enoughMemory_ &= moveResolver_.addMove(from, to, Move::GENERAL);
> > +}
> > +
> > +void
> > +MacroAssembler::finalizeABICall()
> 
> What is the benefit of finalizeABICall()? It looks like it's only useful
> after callWithABI().

It is only useful after finalizeABICall.  It handles the Moves registered by getABIRes as callWithABI does with setABIArg and does the clean-up of the stack.  Thus after the call of finalizeABICall, you should not expect to access the returned value.

> @@ +206,4 @@
> >  
> >      freeStack(stackAdjust_);
> >      if (dynamicAlignment_)
> >          restoreStackFromDynamicAlignment();
> 
> 1) The memory for the return value is freed before it is consumed by the
> caller (it is part of the reserveStack() allocation) -- but there is an
> abstraction violation, since the caller of finalizeABICall() needs to know
> where that memory is and how to free it, without being told.

There is no violation at all, the caller need to "copy" the result by using getABIRes.  The caller should not use any result that has not been "copied" by getABIRes.  Thus, to be safe you should write:

masm.callWithABI(f);
masm.getABIRes(0, MoveOperand(ReturnReg));
masm.finalizeABICall();
// Now it is safe to use ReturnReg here.

> We really want
> the memory for the return value to be explicitly allocated by the /caller of
> setupABICall()/, then /passed into that function as an Operand()/, then
> /explicitly freed/. All of this memory traffic should be dead-obvious to
> people reading code in, e.g., GenerateBailoutThunk().
>
> 2) With the current scheme, in the case of dynamic memory alignment, the
> location of the return value is unknown. The solution to this is also
> addressed by having the memory region passed into setupABICall() as on
> Operand.

see my previoous point.  I would prefer to avoid it in order to merge codes which are extremely similar.

With this patch you should use the following pattern:

setupABICall
[setABIArg] // set the arguments.
callWithABI
[getABIRes] // fetch the return value. (whatever is used to store it, register*s* or stack slots or both)
finalizeABICall

> ::: js/src/ion/IonMacroAssembler.h
> @@ +104,5 @@
> >  
> > +    // Compute space needed for the function call and set the properties of the
> > +    // callee.  It returns the space which has to be allocated for calling the
> > +    // function.
> > +    uint32 setupABICall(uint32 arg, uint32 returnSize);
> 
> Please use "size_t returnSize" in all the locations this occurs, asserting
> that the value is either sizeof(void *) or 2*sizeof(void *).

I don't understand why asserting a limitation of  2 * sizeof(void *)  which does not seems to exists.  Maybe what you want to express is

JS_ASSERT_IF(returnSize > sizeof(void *), !(returnSize % sizeof(void *)));

> ::: js/src/ion/x64/Trampoline-x64.cpp
> @@ +251,4 @@
> >  
> >      // Load the value the interpreter returned.
> >      masm.movq(Operand(rsp, 0), JSReturnReg);
> >      masm.addq(Imm32(8), rsp);
> 
> masm.pop(JSReturnReg); // I do realize this isn't your code :)
> 
> ::: js/src/ion/x86/Trampoline-x86.cpp
> @@ +268,5 @@
> >  
> >      // Load the value the interpreter returned.
> >      masm.movl(Operand(esp, 4), JSReturnReg_Type);
> >      masm.movl(Operand(esp, 0), JSReturnReg_Data);
> >      masm.addl(Imm32(8), esp);
> 
> masm.pop(JSReturnReg_Data);
> masm.pop(JSReturnReg_Type);

The best would be to have:

masm.loadValue(const ValueOperand&);

and maybe

masm.pushValue(const ValueOperand&);
masm.popValue(const ValueOperand&);
I do not make the previous patch obsolete because I do not use this one for the reasons explained in the previous comment but this featured has been asked.
Attachment #566200 - Flags: review?(sstangl)
Comment on attachment 566200 [details] [diff] [review]
With explicit allocation outside setupABICall.

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

Looks good.

::: js/src/ion/IonMacroAssembler.cpp
@@ +149,5 @@
> +    JS_ASSERT(inCall_);
> +
> +    // Perform argument move resolution now.
> +    if (stackAdjust_ >= sizeof(void *))
> +    {

{ to first line

@@ +196,5 @@
> +    JS_ASSERT(inCall_);
> +
> +    // Perform result move resolution now.
> +    if (callProperties_ & hasGetRes)
> +    {

{ to first line
Attachment #566200 - Flags: review?(sstangl) → review+
Comment on attachment 566200 [details] [diff] [review]
With explicit allocation outside setupABICall.

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

Nice work. Drive by naming comments :)

::: js/src/ion/IonMacroAssembler.h
@@ +93,5 @@
> +    enum CallProperty {
> +        largeReturnValue = 1 << 0,
> +        returnArgConsumeStack = 1 << 1,
> +        stackAllocated = 1 << 2,
> +        hasGetRes = 1 << 3,

Nit: Capitalize first letters here

@@ +195,5 @@
> +    //
> +    // This function restore the stack as it was before setupABICall.  This
> +    // function has to be called after callWithABI.  The return value must be
> +    // saved before this call.
> +    void finalizeABICall();

Nit: "finishABICall"
Attachment #565974 - Attachment is obsolete: true
Update (old patch: r=sstangl)
Attachment #566200 - Attachment is obsolete: true
Attachment #567789 - Flags: review+
https://hg.mozilla.org/projects/ionmonkey/rev/352b7590a566
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Apparently I failed to rebase this properly and the tree went red

http://hg.mozilla.org/projects/ionmonkey/rev/6fe92d3bb4a3
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to David Anderson [:dvander] from comment #12)
> https://hg.mozilla.org/projects/ionmonkey/rev/352b7590a566

Apparently you forget to include most of the modifications made to IonMacroAssembler.cpp.
The patch did not apply cleanly. Fixed now:

https://hg.mozilla.org/projects/ionmonkey/rev/324d270b8e17
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.