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

Issue 144130043: code review 144130043: cgo: adjust return value location to account for stack ... (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 6 months ago by khr
Modified:
9 years, 6 months ago
Reviewers:
rsc, iant
CC:
iant, rsc, khr1, golang-codereviews
Visibility:
Public.

Description

cgo: adjust return value location to account for stack copies. During a cgo call, the stack can be copied. This copy invalidates the pointer that cgo has into the return value area. To fix this problem, pass the address of the location containing the stack top value (which is in the G struct). For cgo functions which return values, read the stktop before and after the cgo call to compute the adjustment necessary to write the return value. Fixes issue 8771

Patch Set 1 #

Patch Set 2 : diff -r 9517f108393aa4330a74159c445a5b01079d3add https://khr%40golang.org@code.google.com/p/go/ #

Patch Set 3 : diff -r 9517f108393aa4330a74159c445a5b01079d3add https://khr%40golang.org@code.google.com/p/go/ #

Patch Set 4 : diff -r 9517f108393aa4330a74159c445a5b01079d3add https://khr%40golang.org@code.google.com/p/go/ #

Total comments: 8

Patch Set 5 : diff -r 9517f108393aa4330a74159c445a5b01079d3add https://khr%40golang.org@code.google.com/p/go/ #

Patch Set 6 : diff -r 9517f108393aa4330a74159c445a5b01079d3add https://khr%40golang.org@code.google.com/p/go/ #

Patch Set 7 : diff -r 9b457baf611b2868591c5f00ce842bdecc3d2bfa https://khr%40golang.org@code.google.com/p/go/ #

Patch Set 8 : diff -r 328bdfbc4f9583ae8a14030d6cfc574dbad8f337 https://khr%40golang.org@code.google.com/p/go/ #

Total comments: 3

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

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

Patch Set 11 : diff -r 032fdf145bf989b31b6c7d369c36c4b54066f55a https://code.google.com/p/go/ #

Patch Set 12 : diff -r 032fdf145bf989b31b6c7d369c36c4b54066f55a https://code.google.com/p/go/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+161 lines, -50 lines) Patch
M misc/cgo/test/callback.go View 1 2 3 4 5 6 7 8 9 10 2 chunks +45 lines, -0 lines 0 comments Download
M misc/cgo/test/callback_c.c View 1 2 3 4 5 1 chunk +16 lines, -0 lines 0 comments Download
M misc/cgo/test/cgo_test.go View 1 2 3 4 5 6 7 1 chunk +50 lines, -48 lines 0 comments Download
M src/cmd/cgo/out.go View 1 2 3 4 5 6 7 8 4 chunks +15 lines, -1 line 0 comments Download
M src/runtime/asm_386.s View 1 2 3 4 5 6 7 1 chunk +10 lines, -0 lines 0 comments Download
M src/runtime/asm_amd64.s View 1 2 3 4 5 6 7 1 chunk +11 lines, -0 lines 0 comments Download
M src/runtime/asm_arm.s View 1 2 3 4 5 6 7 1 chunk +8 lines, -0 lines 0 comments Download
M src/runtime/cgo/callbacks.c View 1 2 3 4 5 6 1 chunk +3 lines, -0 lines 0 comments Download
M src/runtime/stack.c View 1 2 3 4 1 chunk +3 lines, -1 line 0 comments Download

Messages

Total messages: 15
khr
Hello iant@golang.org (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://khr%40golang.org@code.google.com/p/go/
9 years, 6 months ago (2014-09-18 22:57:31 UTC) #1
rsc
wow, ugly bug. nice fix. https://codereview.appspot.com/144130043/diff/60001/src/runtime/asm_386.s File src/runtime/asm_386.s (right): https://codereview.appspot.com/144130043/diff/60001/src/runtime/asm_386.s#newcode684 src/runtime/asm_386.s:684: MOVL (g_stack+stack_hi)(DI), CX not ...
9 years, 6 months ago (2014-09-18 23:31:44 UTC) #2
khr1
All fixed up. PTAL. On Thu, Sep 18, 2014 at 4:31 PM, <rsc@golang.org> wrote: > ...
9 years, 6 months ago (2014-09-19 00:12:55 UTC) #3
iant
LGTM This is clever. Unfortunately I can't quite use this for SWIG, since SWIG generated ...
9 years, 6 months ago (2014-09-19 00:15:00 UTC) #4
rsc
Re SWIG, what if instead of the changes here to pass a second argument we ...
9 years, 6 months ago (2014-09-19 00:26:28 UTC) #5
iant
On Thu, Sep 18, 2014 at 5:26 PM, Russ Cox <rsc@golang.org> wrote: > > Re ...
9 years, 6 months ago (2014-09-19 00:34:24 UTC) #6
khr1
This __go_topofstack function has to be written in gcc code, but it needs access to ...
9 years, 6 months ago (2014-09-19 02:33:34 UTC) #7
rsc
On Thu, Sep 18, 2014 at 10:33 PM, Keith Randall <khr@google.com> wrote: > This __go_topofstack ...
9 years, 6 months ago (2014-09-19 02:38:11 UTC) #8
iant
On Thu, Sep 18, 2014 at 7:38 PM, Russ Cox <rsc@golang.org> wrote: > On Thu, ...
9 years, 6 months ago (2014-09-19 14:26:23 UTC) #9
rsc
Ping. Keith, are you planning to revise this CL?
9 years, 6 months ago (2014-09-24 18:20:55 UTC) #10
khr1
Yes, I'm just fighting with the linker to make my asm stub link correctly. On ...
9 years, 6 months ago (2014-09-24 20:26:37 UTC) #11
khr
On 2014/09/24 20:26:37, khr1 wrote: > Yes, I'm just fighting with the linker to make ...
9 years, 6 months ago (2014-09-24 23:07:40 UTC) #12
rsc
LGTM https://codereview.appspot.com/144130043/diff/130001/src/cmd/cgo/out.go File src/cmd/cgo/out.go (right): https://codereview.appspot.com/144130043/diff/130001/src/cmd/cgo/out.go#newcode47 src/cmd/cgo/out.go:47: fmt.Fprintf(fm, "char* cgo_topofstack(void) { }\n") The others begin ...
9 years, 6 months ago (2014-09-24 23:23:28 UTC) #13
iant
LGTM https://codereview.appspot.com/144130043/diff/130001/misc/cgo/test/callback.go File misc/cgo/test/callback.go (right): https://codereview.appspot.com/144130043/diff/130001/misc/cgo/test/callback.go#newcode256 misc/cgo/test/callback.go:256: r = C.int(f(1024)) Not important, but 1024 might ...
9 years, 6 months ago (2014-09-25 00:15:26 UTC) #14
khr
9 years, 6 months ago (2014-09-25 14:59:08 UTC) #15
*** Submitted as https://code.google.com/p/go/source/detail?r=b4a82354d400 ***

cgo: adjust return value location to account for stack copies.

During a cgo call, the stack can be copied.  This copy invalidates
the pointer that cgo has into the return value area.  To fix this
problem, pass the address of the location containing the stack
top value (which is in the G struct).  For cgo functions which
return values, read the stktop before and after the cgo call to
compute the adjustment necessary to write the return value.

Fixes issue 8771

LGTM=iant, rsc
R=iant, rsc, khr
CC=golang-codereviews
https://codereview.appspot.com/144130043
Sign in to reply to this message.

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