code review 131670043: runtime: convert cgocall to Go

runtime: convert cgocall to Go

[dvyukov, 2014-08-29 13:05:32]

Hello golang-codereviews@googlegroups.com (cc: khr@golang.org, rsc@golang.org),

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

[dvyukov, 2014-08-29 18:05:03]

https://codereview.appspot.com/131670043/diff/100001/src/pkg/runtime/cgocall.go
File src/pkg/runtime/cgocall.go (right):

https://codereview.appspot.com/131670043/diff/100001/src/pkg/runtime/cgocall....
src/pkg/runtime/cgocall.go:74: // m->g0's stack (the pointer is still in
m->g0.sched.sp), restores the old
this comment extensively refers to m->g0
do you want me to change it all to g.m.g0?

[dvyukov, 2014-09-03 13:59:27]

ping

[khr, 2014-09-03 19:50:13]

LGTM.

https://codereview.appspot.com/131670043/diff/100001/src/pkg/runtime/cgocall.go
File src/pkg/runtime/cgocall.go (right):

https://codereview.appspot.com/131670043/diff/100001/src/pkg/runtime/cgocall....
src/pkg/runtime/cgocall.go:74: // m->g0's stack (the pointer is still in
m->g0.sched.sp), restores the old
On 2014/08/29 18:05:03, dvyukov wrote:
> this comment extensively refers to m->g0
> do you want me to change it all to g.m.g0?

You can leave it for now.  Plenty of time to clean comments up during the
freeze.

https://codereview.appspot.com/131670043/diff/100001/src/pkg/runtime/cgocall....
src/pkg/runtime/cgocall.go:158: cgocall(cgoMalloc,
noescape(unsafe.Pointer(&args)))
Can we mark asmcgocall as noescape instead?  Or is that not valid for some uses?

[rsc, 2014-09-03 20:07:37]

Please wait for me.

[rsc, 2014-09-03 20:08:17]

Also please sync and fix up for recent cgocall_errno changes.

[rsc, 2014-09-04 03:05:46]

https://codereview.appspot.com/131670043/diff/100001/src/pkg/runtime/cgocall.go
File src/pkg/runtime/cgocall.go (right):

https://codereview.appspot.com/131670043/diff/100001/src/pkg/runtime/cgocall....
src/pkg/runtime/cgocall.go:86: if !iscgo && theGoos != "solaris" && theGoos !=
"windows" {
s/theGoos/GOOS/

https://codereview.appspot.com/131670043/diff/100001/src/pkg/runtime/cgocall....
src/pkg/runtime/cgocall.go:109: releasem(mp) // can't be rescheduled to another
m until unlockOSThread
What's the point then? Just write mp := getg().m

https://codereview.appspot.com/131670043/diff/100001/src/pkg/runtime/cgocall....
src/pkg/runtime/cgocall.go:158: cgocall(cgoMalloc,
noescape(unsafe.Pointer(&args)))
On 2014/09/03 19:50:13, khr wrote:
> Can we mark asmcgocall as noescape instead?  Or is that not valid for some
uses?

Yes please.

https://codereview.appspot.com/131670043/diff/100001/src/pkg/runtime/cgocall....
src/pkg/runtime/cgocall.go:207: switch theGoarch {
GOARCH

https://codereview.appspot.com/131670043/diff/100001/src/pkg/runtime/cgocall....
src/pkg/runtime/cgocall.go:218: default:
move error default to top of switch

https://codereview.appspot.com/131670043/diff/100001/src/pkg/runtime/cgocall....
src/pkg/runtime/cgocall.go:242: switch theGoarch {
GOARCH

[dvyukov, 2014-09-04 06:17:26]

PTAL

https://codereview.appspot.com/131670043/diff/100001/src/pkg/runtime/cgocall.go
File src/pkg/runtime/cgocall.go (right):

https://codereview.appspot.com/131670043/diff/100001/src/pkg/runtime/cgocall....
src/pkg/runtime/cgocall.go:74: // m->g0's stack (the pointer is still in
m->g0.sched.sp), restores the old
On 2014/09/03 19:50:12, khr wrote:
> On 2014/08/29 18:05:03, dvyukov wrote:
> > this comment extensively refers to m->g0
> > do you want me to change it all to g.m.g0?
> 
> You can leave it for now.  Plenty of time to clean comments up during the
> freeze.

Acknowledged.

https://codereview.appspot.com/131670043/diff/100001/src/pkg/runtime/cgocall....
src/pkg/runtime/cgocall.go:86: if !iscgo && theGoos != "solaris" && theGoos !=
"windows" {
On 2014/09/04 03:05:45, rsc wrote:
> s/theGoos/GOOS/

Done.

https://codereview.appspot.com/131670043/diff/100001/src/pkg/runtime/cgocall....
src/pkg/runtime/cgocall.go:109: releasem(mp) // can't be rescheduled to another
m until unlockOSThread
On 2014/09/04 03:05:45, rsc wrote:
> What's the point then? Just write mp := getg().m

Done.

https://codereview.appspot.com/131670043/diff/100001/src/pkg/runtime/cgocall....
src/pkg/runtime/cgocall.go:158: cgocall(cgoMalloc,
noescape(unsafe.Pointer(&args)))
On 2014/09/04 03:05:45, rsc wrote:
> On 2014/09/03 19:50:13, khr wrote:
> > Can we mark asmcgocall as noescape instead?  Or is that not valid for some
> uses?
> 
> Yes please.

Done.

https://codereview.appspot.com/131670043/diff/100001/src/pkg/runtime/cgocall....
src/pkg/runtime/cgocall.go:207: switch theGoarch {
On 2014/09/04 03:05:45, rsc wrote:
> GOARCH

Done.

https://codereview.appspot.com/131670043/diff/100001/src/pkg/runtime/cgocall....
src/pkg/runtime/cgocall.go:218: default:
On 2014/09/04 03:05:45, rsc wrote:
> move error default to top of switch

Done.

https://codereview.appspot.com/131670043/diff/100001/src/pkg/runtime/cgocall....
src/pkg/runtime/cgocall.go:242: switch theGoarch {
On 2014/09/04 03:05:45, rsc wrote:
> GOARCH

Done.

[rsc, 2014-09-04 18:40:04]

LGTM

[rsc, 2014-09-04 18:40:45]

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

runtime: convert cgocall to Go

LGTM=khr, rsc
R=golang-codereviews, khr, rsc
CC=golang-codereviews
https://codereview.appspot.com/131670043

Committer: Russ Cox <rsc@golang.org>

[gobot, 2014-09-04 19:03:10]

This CL appears to have broken the windows-amd64-race builder.
See http://build.golang.org/log/a7564fa43d30bd1832a8b3979a048df1feb7e800