Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(267)

Issue 7728045: code review 7728045: runtime: store asmcgocall return PC where the ARM unwin... (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 8 months ago by cshapiro
Modified:
8 years, 8 months ago
Reviewers:
rsc, cshapiro1, lucio
CC:
golang-dev, bradfitz, minux1
Visibility:
Public.

Description

runtime: store asmcgocall return PC where the ARM unwind expects it The ARM implementation of runtime.cgocallback_gofunc diverged from the calling convention by leaving a word of garbage at the top of the stack and storing the return PC above the locals. This change stores the return PC at the top of the stack and removes the save area above the locals. Update Issue 5124 This CL fixes first part of the ARM issues and added the unwind test.

Patch Set 1 #

Patch Set 2 : diff -r f9e10e016342 https://code.google.com/p/go/ #

Patch Set 3 : diff -r f9e10e016342 https://code.google.com/p/go/ #

Total comments: 4

Patch Set 4 : diff -r f9e10e016342 https://code.google.com/p/go/ #

Patch Set 5 : diff -r f9e10e016342 https://code.google.com/p/go/ #

Patch Set 6 : diff -r b4351de48c51 https://code.google.com/p/go/ #

Patch Set 7 : diff -r b4351de48c51 https://code.google.com/p/go/ #

Patch Set 8 : diff -r b4351de48c51 https://code.google.com/p/go/ #

Patch Set 9 : diff -r b3cfb8be2faf https://code.google.com/p/go/ #

Patch Set 10 : diff -r 2ac4f6f628f1 https://code.google.com/p/go/ #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+62 lines, -15 lines) Patch
M misc/cgo/test/callback.go View 1 2 3 4 5 6 2 chunks +50 lines, -0 lines 0 comments Download
M misc/cgo/test/cgo_test.go View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M src/pkg/runtime/asm_arm.s View 1 2 3 4 6 chunks +11 lines, -15 lines 2 comments Download

Messages

Total messages: 18
cshapiro
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go/
8 years, 8 months ago (2013-03-22 02:38:54 UTC) #1
bradfitz
All the tests already pass, though! :-) On Thu, Mar 21, 2013 at 7:38 PM, ...
8 years, 8 months ago (2013-03-22 02:46:14 UTC) #2
cshapiro
On Thu, Mar 21, 2013 at 7:46 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote: > All the tests ...
8 years, 8 months ago (2013-03-22 03:01:33 UTC) #3
bradfitz
Even without the GC, can't you test Go -> C -> Go -> runtime.Callers ? ...
8 years, 8 months ago (2013-03-22 03:04:43 UTC) #4
cshapiro
On Thu, Mar 21, 2013 at 8:04 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote: > Even without the ...
8 years, 8 months ago (2013-03-22 18:54:28 UTC) #5
minux1
looks mostly fine to me. https://codereview.appspot.com/7728045/diff/5001/src/pkg/runtime/asm_arm.s File src/pkg/runtime/asm_arm.s (right): https://codereview.appspot.com/7728045/diff/5001/src/pkg/runtime/asm_arm.s#newcode347 src/pkg/runtime/asm_arm.s:347: MOVW fn+0(FP), R0 why ...
8 years, 8 months ago (2013-03-22 19:25:05 UTC) #6
cshapiro1
https://codereview.appspot.com/7728045/diff/5001/src/pkg/runtime/asm_arm.s File src/pkg/runtime/asm_arm.s (right): https://codereview.appspot.com/7728045/diff/5001/src/pkg/runtime/asm_arm.s#newcode347 src/pkg/runtime/asm_arm.s:347: MOVW fn+0(FP), R0 On 2013/03/22 19:25:06, minux wrote: > ...
8 years, 8 months ago (2013-03-22 20:20:05 UTC) #7
cshapiro1
All review comments should be addressed.
8 years, 8 months ago (2013-03-22 22:53:44 UTC) #8
rsc
Can you please add a test for this? Otherwise it will just break again. It ...
8 years, 8 months ago (2013-03-23 20:13:04 UTC) #9
cshapiro1
On Sat, Mar 23, 2013 at 1:13 PM, Russ Cox <rsc@golang.org> wrote: > Can you ...
8 years, 8 months ago (2013-03-25 17:47:31 UTC) #10
cshapiro1
PTAL
8 years, 8 months ago (2013-03-25 20:28:38 UTC) #11
bradfitz
LGTM More for the test code than the assembly, but I'll trust you, especially now ...
8 years, 8 months ago (2013-03-25 20:36:08 UTC) #12
minux1
LGTM.
8 years, 8 months ago (2013-03-25 20:43:55 UTC) #13
minux1
Do you want to add: Update Issue 5124 This CL fixes first part of the ...
8 years, 8 months ago (2013-03-25 20:45:18 UTC) #14
cshapiro
*** Submitted as https://code.google.com/p/go/source/detail?r=17b2bed9d136 *** runtime: store asmcgocall return PC where the ARM unwind expects ...
8 years, 8 months ago (2013-03-25 21:11:24 UTC) #15
rsc
The changes look good otherwise. https://codereview.appspot.com/7728045/diff/32001/src/pkg/runtime/asm_arm.s File src/pkg/runtime/asm_arm.s (right): https://codereview.appspot.com/7728045/diff/32001/src/pkg/runtime/asm_arm.s#newcode370 src/pkg/runtime/asm_arm.s:370: MOVW fn+4(FP), R0 This ...
8 years, 8 months ago (2013-03-25 22:12:01 UTC) #16
cshapiro1
https://codereview.appspot.com/7728045/diff/32001/src/pkg/runtime/asm_arm.s File src/pkg/runtime/asm_arm.s (right): https://codereview.appspot.com/7728045/diff/32001/src/pkg/runtime/asm_arm.s#newcode370 src/pkg/runtime/asm_arm.s:370: MOVW fn+4(FP), R0 The motivation here is to make ...
8 years, 8 months ago (2013-03-25 22:21:04 UTC) #17
lucio
8 years, 8 months ago (2013-03-26 07:45:45 UTC) #18
Can I vote in favour of exploiting the "_arm" suffix and making the
code differ from _386?  It is the only place where previous art can be
treated as irrelevant and whereas I don't know whether the 386 code is
intentionally obfuscating or merely clever, I don't see why the arm
code should follow that example.

To be a bit facitious, would a comment like

  // let's do things like for the 386

not look silly in an arm module?

Lucio.

PS: The reason I'm speaking up is that I'm about to submit a CL to
port Go to plan9/386 and in helping prepare the CL I became convinced
that it is poor judgement to follow the 386 examples too far.


On 3/26/13, cshapiro@google.com <cshapiro@google.com> wrote:
>
> https://codereview.appspot.com/7728045/diff/32001/src/pkg/runtime/asm_arm.s
> File src/pkg/runtime/asm_arm.s (right):
>
>
https://codereview.appspot.com/7728045/diff/32001/src/pkg/runtime/asm_arm.s#n...
> src/pkg/runtime/asm_arm.s:370: MOVW	fn+4(FP), R0
> The motivation here is to make the code read like the x86 code.  Where
> this code was before, the preceeding comment was separated from its
> referent.
>
> I don't find this so confusing so I will defer to you on what might be
> more perspicuous.  The 2 options I see are 1) leave this where it is and
> add a note about the FP being adjusted 2) move this code below line 346
>
> Either way is fine with me.  Let me know what you like.
>
> https://codereview.appspot.com/7728045/
>
> --
>
> ---
> 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.
>
>
>
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b