code review 11647043: runtime: Stop arm memmove corrupting its parameters

runtime: Stop arm memmove corrupting its parameters

Change use of x+(SP) to access the stack frame into x-(SP)

Fixes issue 5925.

[Nick Craig-Wood, 2013-07-21 07:35:58]

Hello golang-dev@googlegroups.com (cc: dave cheney <dave@cheney.net>),

I'd like you to review this change to
https://code.google.com/p/go

[bradfitz, 2013-07-22 00:40:01]

How did this manifest itself?

Can you write a test that failed before but passes now?

[dave_cheney.net, 2013-07-22 00:43:06]

I don't think it is possible to do so. As I understand the issue
correctly, the code is overwriting 4(FP), the 4th byte following the
frame pointer, ie the address of the first argument passed to the
function, not 4(SP), the first location on the stack. The stack
location is only needed if the unaligned slow path has to be taken,
and will corrupt one of the function arguments in the backtrace, but
that is about it.

On Mon, Jul 22, 2013 at 10:40 AM,  <bradfitz@golang.org> wrote:
> How did this manifest itself?
>
> Can you write a test that failed before but passes now?
>
>
> https://codereview.appspot.com/11647043/

[remyoudompheng, 2013-07-22 05:59:45]

Le 22 juil. 2013 02:40, <bradfitz@golang.org> a écrit :
>
> How did this manifest itself?
>
> Can you write a test that failed before but passes now?

He wanted to call memmove from ARM assembly for MD5 and make reuse of
arguments after the call.

Rémy

[bradfitz, 2013-07-22 06:02:46]

Sounds testable.
 On Jul 21, 2013 10:59 PM, "Rémy Oudompheng" <remyoudompheng@gmail.com>
wrote:

>
> Le 22 juil. 2013 02:40, <bradfitz@golang.org> a écrit :
> >
> > How did this manifest itself?
> >
> > Can you write a test that failed before but passes now?
>
> He wanted to call memmove from ARM assembly for MD5 and make reuse of
> arguments after the call.
>
> Rémy
>
> --
>
> ---
> You received this message because you are subscribed to the Google Groups
> "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>
>

[nick_craig-wood.com, 2013-07-22 08:59:12]

On 22/07/13 07:02, Brad Fitzpatrick wrote:
> Sounds testable.
> 
> On Jul 21, 2013 10:59 PM, "Rémy Oudompheng" <remyoudompheng@gmail.com
> <mailto:remyoudompheng@gmail.com>> wrote:
> 
> 
>     Le 22 juil. 2013 02:40, <bradfitz@golang.org
>     <mailto:bradfitz@golang.org>> a écrit :
>     >
>     > How did this manifest itself?
>     >
>     > Can you write a test that failed before but passes now?
> 
>     He wanted to call memmove from ARM assembly for MD5 and make reuse
>     of arguments after the call.

Yes you are right about calling memmove from assembler.

OK I'll see if I can make a failing test!

-- 
Nick Craig-Wood <nick@craig-wood.com> -- http://www.craig-wood.com/nick

[rsc, 2013-07-22 14:58:30]

I admire the desire to test everything, but this test would just be scar
tissue. Let's just fix the function and move on.

That said, the fix is wrong. The correct fix is not to start using R13, but
to use the correct notation for local variables. The various foo+4(SP)
should be foo-4(SP).

Russ

[nick_craig-wood.com, 2013-07-22 15:55:39]

On 22/07/13 15:58, Russ Cox wrote:
> That said, the fix is wrong. The correct fix is not to start using R13,
> but to use the correct notation for local variables. The various
> foo+4(SP) should be foo-4(SP).

Ah, that makes a whole lot of sense now, thanks! I've been struggling to
work this out.

So for ARM -4(SP) is the first local variable, -8(SP) is the second etc.
Which is quite different from 386/amd64 where 0(SP) is the first local
variable, 4(SP) is the second. Not sure why they are different though
since ARM/386/amd64 all have downward growing stacks.

-- 
Nick Craig-Wood <nick@craig-wood.com> -- http://www.craig-wood.com/nick

[rsc, 2013-07-22 16:06:51]

No, the name-offset(SP) syntax is consistent across all architectures. A
386 function with a frame of 24 should be using -4(SP), -8(SP) up to
-24(SP) to address local variables, and 0(SP), 4(SP) etc for preparing
arguments to a call. The SP in the two syntaxes are different: the former
is a virtual SP at the top of the local frame (basically, a negative number
means add the frame size before laying down the instruction).

The difference on ARM is that 0(SP), 4(SP) for arguments are written
0(R13), 4(R13) etc. That is, on ARM, SP is always the virtual SP, never the
actual R13 register. I think this was probably a mistake, but I'm
disinclined to change the semantics at this point. I do want to encourage
people to use the proper local variable syntax, though.

I am responsible for much of the misuse of 0(SP) etc for local variables on
amd64 and 386; I did not fully appreciate the distinction until relatively
recently.

Russ

[nick_craig-wood.com, 2013-07-22 16:16:59]

On 22/07/13 17:06, Russ Cox wrote:
> No, the name-offset(SP) syntax is consistent across all architectures. A
> 386 function with a frame of 24 should be using -4(SP), -8(SP) up to
> -24(SP) to address local variables, and 0(SP), 4(SP) etc for preparing
> arguments to a call. The SP in the two syntaxes are different: the
> former is a virtual SP at the top of the local frame (basically, a
> negative number means add the frame size before laying down the
> instruction).
> 
> The difference on ARM is that 0(SP), 4(SP) for arguments are written
> 0(R13), 4(R13) etc.

I think that should be 4(R13), 8(R13) as 0(R13) is your saved LR?

> That is, on ARM, SP is always the virtual SP, never
> the actual R13 register. I think this was probably a mistake, but I'm
> disinclined to change the semantics at this point. I do want to
> encourage people to use the proper local variable syntax, though.
> 
> I am responsible for much of the misuse of 0(SP) etc for local variables
> on amd64 and 386; I did not fully appreciate the distinction until
> relatively recently.

Ah OK!  Thanks for clearing that up.  I'll submit a fixed patch shortly.

-- 
Nick Craig-Wood <nick@craig-wood.com> -- http://www.craig-wood.com/nick

[rsc, 2013-07-22 16:44:36]

On Mon, Jul 22, 2013 at 12:16 PM, Nick Craig-Wood <nick@craig-wood.com>wrote:

> On 22/07/13 17:06, Russ Cox wrote:
> > No, the name-offset(SP) syntax is consistent across all architectures. A
> > 386 function with a frame of 24 should be using -4(SP), -8(SP) up to
> > -24(SP) to address local variables, and 0(SP), 4(SP) etc for preparing
> > arguments to a call. The SP in the two syntaxes are different: the
> > former is a virtual SP at the top of the local frame (basically, a
> > negative number means add the frame size before laying down the
> > instruction).
> >
> > The difference on ARM is that 0(SP), 4(SP) for arguments are written
> > 0(R13), 4(R13) etc.
>
> I think that should be 4(R13), 8(R13) as 0(R13) is your saved LR?


Yes, wherever it is the arguments go.

In general if you see an R13 that is not being used to prepare arguments,
it's wrong. I'd like at some point to move the saved LR to match the ABI
used by gcc on ARM, so eventually it really will be 0(R13). It will be nice
if arguments are the only uses of R13, because then they will be easy to
find and update.

Russ

[Nick Craig-Wood, 2013-07-22 18:24:23]

Hello golang-dev@googlegroups.com, bradfitz@golang.org, dave@cheney.net,
remyoudompheng@gmail.com, nick@craig-wood.com, rsc@golang.org (cc: dave cheney
<dave@cheney.net>, golang-dev@googlegroups.com),

Please take another look.

[Nick Craig-Wood, 2013-07-22 18:25:46]

On 2013/07/22 18:24:23, Nick Craig-Wood wrote:
> Hello mailto:golang-dev@googlegroups.com, mailto:bradfitz@golang.org,
mailto:dave@cheney.net,
> mailto:remyoudompheng@gmail.com, mailto:nick@craig-wood.com,
mailto:rsc@golang.org (cc: dave cheney
> <mailto:dave@cheney.net>, mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

I've changed the (R13) into (SP) access as per rsc suggestion

I've also run the test suite and checked the assembler is now correct

[rsc, 2013-07-22 18:31:07]

LGTM

Thanks, and apologies.

[dave_cheney.net, 2013-07-22 23:27:29]

LGTM. Thanks Russ and Nick. Submitting.

[dave_cheney.net, 2013-07-22 23:29:44]

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

runtime: Stop arm memmove corrupting its parameters

Change use of x+(SP) to access the stack frame into x-(SP)

Fixes issue 5925.

R=golang-dev, bradfitz, dave, remyoudompheng, nick, rsc
CC=dave cheney <dave, golang-dev
https://codereview.appspot.com/11647043

Committer: Dave Cheney <dave@cheney.net>