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

Issue 131910043: code review 131910043: runtime: keep g->syscallsp consistent after cgo->Go cal...

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 2 months ago by marcan
Modified:
6 years, 1 month ago
Reviewers:
rsc, dvyukov
CC:
golang-codereviews, dvyukov, minux, bradfitz, iant, gobot, rsc
Visibility:
Public.

Description

runtime: keep g->syscallsp consistent after cgo->Go callbacks Normally, the caller to runtime.entersyscall() must not return before calling runtime.exitsyscall(), lest g->syscallsp become a dangling pointer. runtime.cgocallbackg() violates this constraint. To work around this, save g->syscallsp and g->syscallpc around cgo->Go callbacks, then restore them after calling runtime.entersyscall(), which restores the syscall stack frame pointer saved by cgocall. This allows the GC to correctly trace a goroutine that is currently returning from a Go->cgo->Go chain. This also adds a check to proc.c that panics if g->syscallsp is clearly invalid. It is not 100% foolproof, as it will not catch a case where the stack was popped then pushed back beyond g->syscallsp, but it does catch the present cgo issue and makes existing tests fail without the bugfix. Fixes issue 7978.

Patch Set 1 #

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

Total comments: 4

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

Total comments: 9

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

Total comments: 2

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

Total comments: 8

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

Total comments: 6

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

Total comments: 9

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

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

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

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+145 lines, -9 lines) Patch
M misc/cgo/test/cgo_test.go View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
A misc/cgo/test/issue7978.go View 1 2 3 4 5 6 1 chunk +99 lines, -0 lines 0 comments Download
M src/run.bash View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -0 lines 0 comments Download
M src/run.bat View 1 2 3 4 5 6 7 8 9 1 chunk +7 lines, -0 lines 2 comments Download
M src/runtime/cgocall.go View 1 2 3 4 5 7 1 chunk +10 lines, -2 lines 0 comments Download
M src/runtime/proc.c View 1 2 3 4 5 6 7 8 9 6 chunks +24 lines, -7 lines 0 comments Download
M src/runtime/runtime.h View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M src/runtime/stubs.go View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 46
marcan
Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
6 years, 2 months ago (2014-08-19 05:31:41 UTC) #1
marcan
This change is incomplete: I forgot that GC can already run after entersyscall(), and might ...
6 years, 2 months ago (2014-08-19 11:07:19 UTC) #2
dvyukov
How does it currently manifest itself? Stack trace in callback is incorrect? If so, please ...
6 years, 2 months ago (2014-08-19 15:53:28 UTC) #3
marcan
On 2014/08/19 15:53:28, dvyukov wrote: > How does it currently manifest itself? Stack trace in ...
6 years, 2 months ago (2014-08-19 16:35:26 UTC) #4
dvyukov
https://codereview.appspot.com/131910043/diff/60001/src/pkg/runtime/cgocall.c File src/pkg/runtime/cgocall.c (right): https://codereview.appspot.com/131910043/diff/60001/src/pkg/runtime/cgocall.c#newcode237 src/pkg/runtime/cgocall.c:237: uintptr saved_sp, saved_pc; we generally don't use underscores in ...
6 years, 2 months ago (2014-08-20 11:46:40 UTC) #5
dvyukov
thanks for this this you need to sign CLA as described in contribution guidelines
6 years, 2 months ago (2014-08-20 11:47:19 UTC) #6
minux
could this be tested? ideally, i'd like test(s) to be added to misc/cgo/test before submiting ...
6 years, 2 months ago (2014-08-21 04:03:58 UTC) #7
dvyukov
runtime.GC from a cgo callback should trigger the bug today On Thu, Aug 21, 2014 ...
6 years, 2 months ago (2014-08-21 06:13:09 UTC) #8
marcan
PTAL. I've refactored runtime.entersyscall and added runtime.reentersyscall, which takes a syscallpc/sp to use from the ...
6 years, 2 months ago (2014-08-21 06:46:36 UTC) #9
dvyukov
On Thu, Aug 21, 2014 at 10:46 AM, <hector@marcansoft.com> wrote: > PTAL. I've refactored runtime.entersyscall ...
6 years, 2 months ago (2014-08-21 07:37:49 UTC) #10
marcan
On 2014/08/21 07:37:49, dvyukov wrote: > On Thu, Aug 21, 2014 at 10:46 AM, <mailto:hector@marcansoft.com> ...
6 years, 2 months ago (2014-08-21 08:11:15 UTC) #11
marcan
On 2014/08/21 08:11:15, marcan wrote: > Yes, if a traceback is requested at any point ...
6 years, 2 months ago (2014-08-21 08:12:14 UTC) #12
marcan
Friendly ping? :-)
6 years, 2 months ago (2014-08-27 01:50:54 UTC) #13
dvyukov
On 2014/08/27 01:50:54, marcan wrote: > Friendly ping? :-) Please CC rsc on this change. ...
6 years, 2 months ago (2014-08-27 10:18:46 UTC) #14
dvyukov
update to tip (hg sync) proc.c has changed recently
6 years, 2 months ago (2014-08-27 10:20:23 UTC) #15
marcan
On 2014/08/27 10:18:46, dvyukov wrote: > On 2014/08/27 01:50:54, marcan wrote: > > Friendly ping? ...
6 years, 2 months ago (2014-08-27 10:44:31 UTC) #16
dvyukov
https://codereview.appspot.com/131910043/diff/120001/src/pkg/runtime/proc.c File src/pkg/runtime/proc.c (right): https://codereview.appspot.com/131910043/diff/120001/src/pkg/runtime/proc.c#newcode1535 src/pkg/runtime/proc.c:1535: entersyscallcommon(void *arg0) name the parameter argp, this is the ...
6 years, 2 months ago (2014-08-27 10:58:41 UTC) #17
dvyukov
https://codereview.appspot.com/131910043/diff/120001/src/pkg/runtime/cgocall.c File src/pkg/runtime/cgocall.c (right): https://codereview.appspot.com/131910043/diff/120001/src/pkg/runtime/cgocall.c#newcode240 src/pkg/runtime/cgocall.c:240: void *savedsp; Stack space is scarce here, and linker ...
6 years, 2 months ago (2014-08-27 11:24:51 UTC) #18
dvyukov
https://codereview.appspot.com/131910043/diff/130001/src/pkg/runtime/proc.c File src/pkg/runtime/proc.c (right): https://codereview.appspot.com/131910043/diff/130001/src/pkg/runtime/proc.c#newcode1626 src/pkg/runtime/proc.c:1626: save(&syscallpc); this saves the same incorrect pc/sp into g->sched ...
6 years, 2 months ago (2014-08-27 14:22:49 UTC) #19
dvyukov
No test, no guarantees that it continues to work. Coming C->Go cgocall conversion can break ...
6 years, 2 months ago (2014-08-27 17:51:31 UTC) #20
marcan
On 2014/08/27 17:51:31, dvyukov wrote: > No test, no guarantees that it continues to work. ...
6 years, 2 months ago (2014-08-28 11:56:27 UTC) #21
dvyukov
Sorry for the delays. We are busy with runtime C->Go conversion at the moment. Please ...
6 years, 2 months ago (2014-08-29 10:01:25 UTC) #22
marcan
Issue 7978 is already open for this bug. On 29 August 2014 19:01:25 GMT+09:00, dvyukov@google.com ...
6 years, 2 months ago (2014-08-29 12:25:54 UTC) #23
dvyukov
ah, OK On Fri, Aug 29, 2014 at 4:25 PM, Hector Martin <hector@marcansoft.com> wrote: > ...
6 years, 2 months ago (2014-08-29 12:39:38 UTC) #24
bradfitz
Ping. Could somebody review this? Internal Google users suspect this is the cause of some ...
6 years, 1 month ago (2014-09-11 02:44:08 UTC) #25
iant
A few comments. Nice test. https://codereview.appspot.com/131910043/diff/150001/misc/cgo/test/issue7978.go File misc/cgo/test/issue7978.go (right): https://codereview.appspot.com/131910043/diff/150001/misc/cgo/test/issue7978.go#newcode18 misc/cgo/test/issue7978.go:18: while(__atomic_load_n(sync, __ATOMIC_SEQ_CST) != 0); ...
6 years, 1 month ago (2014-09-11 04:33:02 UTC) #26
marcan
I've updated the patch against current head. Luckily, syscallstack/guard went away, so the current patch ...
6 years, 1 month ago (2014-09-11 05:40:53 UTC) #27
gobot
R=rsc@golang.org (assigned by iant@golang.org)
6 years, 1 month ago (2014-09-11 15:49:25 UTC) #28
dvyukov
https://codereview.appspot.com/131910043/diff/190001/misc/cgo/test/issue7978.go File misc/cgo/test/issue7978.go (right): https://codereview.appspot.com/131910043/diff/190001/misc/cgo/test/issue7978.go#newcode18 misc/cgo/test/issue7978.go:18: while(__atomic_load_n(sync, __ATOMIC_SEQ_CST) != 0) These builtins require at least ...
6 years, 1 month ago (2014-09-13 01:00:40 UTC) #29
dvyukov
I am going to say LGTM after the fixes
6 years, 1 month ago (2014-09-13 01:00:57 UTC) #30
marcan
https://codereview.appspot.com/131910043/diff/190001/misc/cgo/test/issue7978.go File misc/cgo/test/issue7978.go (right): https://codereview.appspot.com/131910043/diff/190001/misc/cgo/test/issue7978.go#newcode18 misc/cgo/test/issue7978.go:18: while(__atomic_load_n(sync, __ATOMIC_SEQ_CST) != 0) On 2014/09/13 01:00:39, dvyukov wrote: ...
6 years, 1 month ago (2014-09-16 07:08:31 UTC) #31
dvyukov
LGTM Russ, do you want to take a look as well?
6 years, 1 month ago (2014-09-16 17:12:34 UTC) #32
rsc
The changes to save are unnecessary and wrong. https://codereview.appspot.com/131910043/diff/250001/src/runtime/cgocall.go File src/runtime/cgocall.go (right): https://codereview.appspot.com/131910043/diff/250001/src/runtime/cgocall.go#newcode191 src/runtime/cgocall.go:191: savedpc ...
6 years, 1 month ago (2014-09-16 17:24:06 UTC) #33
dvyukov
https://codereview.appspot.com/131910043/diff/250001/src/runtime/proc.c File src/runtime/proc.c (right): https://codereview.appspot.com/131910043/diff/250001/src/runtime/proc.c#newcode1748 src/runtime/proc.c:1748: g->sched.pc = (uintptr)runtime·getcallerpc(argp); On 2014/09/16 17:24:05, rsc wrote: > ...
6 years, 1 month ago (2014-09-16 17:32:15 UTC) #34
rsc
https://codereview.appspot.com/131910043/diff/250001/src/runtime/proc.c File src/runtime/proc.c (right): https://codereview.appspot.com/131910043/diff/250001/src/runtime/proc.c#newcode1856 src/runtime/proc.c:1856: // Reconsider whether we need save in entersyscall at ...
6 years, 1 month ago (2014-09-16 17:46:25 UTC) #35
marcan
PTAL. I moved the calls to runtime·getcaller{sp,pc} back to the callee functions and made save() ...
6 years, 1 month ago (2014-09-17 06:22:07 UTC) #36
rsc
Please put save back EXACTLY as it was. Refactoring distracts from understanding the bug fix.
6 years, 1 month ago (2014-09-17 18:10:32 UTC) #37
marcan
On 2014/09/17 18:10:32, rsc wrote: > Please put save back EXACTLY as it was. Refactoring ...
6 years, 1 month ago (2014-09-18 04:27:27 UTC) #38
rsc
I think you can simplify the proc.c changes even further. If you start with the ...
6 years, 1 month ago (2014-09-18 17:24:27 UTC) #39
marcan
On 19/09/14 02:24, rsc@golang.org wrote: > Then entersyscall is just > > #pragma textflag NOSPLIT ...
6 years, 1 month ago (2014-09-18 19:55:37 UTC) #40
dvyukov
On Thu, Sep 18, 2014 at 12:55 PM, Hector Martin <hector@marcansoft.com> wrote: > On 19/09/14 ...
6 years, 1 month ago (2014-09-18 20:34:01 UTC) #41
rsc
As Dmitriy wrote, it's fine. restartsyscall, getcallerpc, and getcallersp are all 'nosplit', so that they ...
6 years, 1 month ago (2014-09-18 23:21:35 UTC) #42
marcan
PTAL. Could also save a few casts by either having the first arg to reentersyscall ...
6 years, 1 month ago (2014-09-22 06:01:31 UTC) #43
rsc
LGTM https://codereview.appspot.com/131910043/diff/310001/src/run.bat File src/run.bat (right): https://codereview.appspot.com/131910043/diff/310001/src/run.bat#newcode95 src/run.bat:95: set OLDGOTRACEBACK=%GOTRACEBACK% This needs to be one line ...
6 years, 1 month ago (2014-09-24 17:10:39 UTC) #44
rsc
*** Submitted as https://code.google.com/p/go/source/detail?r=98e896f92a90 *** runtime: keep g->syscallsp consistent after cgo->Go callbacks Normally, the caller ...
6 years, 1 month ago (2014-09-24 17:20:30 UTC) #45
marcan
6 years, 1 month ago (2014-09-25 05:45:19 UTC) #46
https://codereview.appspot.com/131910043/diff/310001/src/run.bat
File src/run.bat (right):

https://codereview.appspot.com/131910043/diff/310001/src/run.bat#newcode95
src/run.bat:95: set OLDGOTRACEBACK=%GOTRACEBACK%
On 2014/09/24 17:10:39, rsc wrote:
> This needs to be one line earlier. I will fix it when I submit.

D'oh. Thanks.
Sign in to reply to this message.

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