code review 76250043: runtime: fix 386 assembly for syscall.naclWrite

runtime: fix 386 assembly for syscall.naclWrite

It was using the wrong offset and returned random values
making "runoutput" compiler tests crash.

[remyoudompheng, 2014-03-14 21:25:03]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[iant, 2014-03-14 21:45:37]

LGTM

[remyoudompheng, 2014-03-14 21:49:54]

*** Submitted as https://code.google.com/p/go/source/detail?r=b1c5bf74b59e ***

runtime: fix 386 assembly for syscall.naclWrite

It was using the wrong offset and returned random values
making "runoutput" compiler tests crash.

LGTM=iant
R=golang-codereviews, iant
CC=golang-codereviews
https://codereview.appspot.com/76250043

[dfc, 2014-03-14 22:55:20]

Thanks for fixing that, this was my mistake. 

I made two mistakes in that original commit, the stack sizes, and the return
offset. 

Could you please explain what my mistake was so I can improve and avoid this
mistake in the future. 

Cheers

Dave

> On 15 Mar 2014, at 8:25, remyoudompheng@gmail.com wrote:
> 
> Reviewers: golang-codereviews,
> 
> Message:
> Hello golang-codereviews@googlegroups.com,
> 
> I'd like you to review this change to
> https://go.googlecode.com/hg/
> 
> 
> Description:
> runtime: fix 386 assembly for syscall.naclWrite
> 
> It was using the wrong offset and returned random values
> making "runoutput" compiler tests crash.
> 
> Please review this at https://codereview.appspot.com/76250043/
> 
> Affected files (+1, -1 lines):
>  M src/pkg/runtime/sys_nacl_386.s
> 
> 
> Index: src/pkg/runtime/sys_nacl_386.s
> ===================================================================
> --- a/src/pkg/runtime/sys_nacl_386.s
> +++ b/src/pkg/runtime/sys_nacl_386.s
> @@ -35,7 +35,7 @@
>    MOVL SI, 4(SP)
>    MOVL DX, 8(SP)
>    CALL runtime·write(SB)
> -    MOVL AX, ret+12(FP)
> +    MOVL AX, ret+16(FP)
>    RET
> 
> TEXT runtime·write(SB),NOSPLIT,$0
> 
> 
> -- 
> You received this message because you are subscribed to the Google Groups
"golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[gobot, 2014-03-14 22:57:29]

This CL appears to have broken the darwin-amd64-cheney builder.
See http://build.golang.org/log/30c7127359c3b4e5764ac60f0ddd257af50be494

[remyoudompheng, 2014-03-14 23:06:10]

On 2014/03/14 22:55:20, dfc wrote:
> Thanks for fixing that, this was my mistake. 
> 
> I made two mistakes in that original commit, the stack sizes, and the return
> offset. 
> 
> Could you please explain what my mistake was so I can improve and avoid this
> mistake in the future. 

naclWrite is a:

  func naclWrite(fd int, data []byte) int

So the return value is after fd (4 bytes) and data (12 bytes), that makes a +16
offset. In this case the code is exactly equivalent to amd64p32. Maybe you were
confused because cap(data) is not used by the function ?

Rémy.

[dfc, 2014-03-14 23:10:57]

> On 15 Mar 2014, at 10:06, remyoudompheng@gmail.com wrote:
> 
>> On 2014/03/14 22:55:20, dfc wrote:
>> Thanks for fixing that, this was my mistake.
> 
>> I made two mistakes in that original commit, the stack sizes, and the
> return
>> offset.
> 
>> Could you please explain what my mistake was so I can improve and
> avoid this
>> mistake in the future.
> 
> naclWrite is a:
> 
>  func naclWrite(fd int, data []byte) int
> 
> So the return value is after fd (4 bytes) and data (12 bytes), that
> makes a +16 offset. In this case the code is exactly equivalent to
> amd64p32. Maybe you were confused because cap(data) is not used by the
> function ?

Yes, that was the piece I was missing. Did I get the function signature right,
from memory I made it $12-16


> 
> Rémy.
> 
> https://codereview.appspot.com/76250043/