Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cmd/cgo: synthesized _cgoCheckPointer calls weaken function calling type checking #16591

Closed
mdempsky opened this issue Aug 3, 2016 · 3 comments
Closed

cmd/cgo: synthesized _cgoCheckPointer calls weaken function calling type checking #16591

mdempsky opened this issue Aug 3, 2016 · 3 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.8Maybe

Comments

@mdempsky
Copy link
Contributor

mdempsky commented Aug 3, 2016

Trying to build this code:

package p

// extern void foo(int** p) {}
import "C"

type x *C.int

func bar(**C.int) {
}

func f(p *x) {
    C.foo(p)
    bar(p)
}

fails with:

./foo.go:13: cannot use p (type *x) as type **C.int in argument to bar

I.e., the bar(p) call is rejected, but the C.foo(p) call is not, even though they both logically have a parameter of type **C.int.

The C.foo(p) is being rewritten by cgo into:

    _Cfunc_foo(_cgoCheckPointer((**_Ctype_int)(p)).(**_Ctype_int))

The Go spec says function call arguments should be assignable to their corresponding parameters. But because of the explicit conversion introduced by cgo, this requirement is being relaxed from assignability to convertibility.

Probably not an issue in practice, but I noticed it while addressing mdempsky/unconvert#16.

I think we could address this by generating type-specific _cgoCheckPointer wrappers, but until we can inline calls to non-leaf functions, that might be undesirable.

/cc @ianlancetaylor
@mdempsky mdempsky added this to the Go1.8Maybe milestone Aug 3, 2016
@mdempsky mdempsky mentioned this issue Aug 5, 2016
Open
@quentinmit quentinmit added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Oct 10, 2016
@rsc
Copy link
Contributor

rsc commented Oct 10, 2016

It seems like we should fix this. It was broken in Go 1.7 so it's not terribly urgent, but the longer we wait the harder it is to fix. I don't really mind about having extra calls even without non-leaf inlining. OK to drop for Go 1.8 but if it's easy to fix, please do.

@rsc rsc added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Oct 10, 2016
@gopherbot
Copy link
Contributor

CL https://golang.org/cl/30973 mentions this issue.

gopherbot pushed a commit that referenced this issue Oct 13, 2016
@ianlancetaylor
cmd/cgo: use alias for unsafe rather than separate functions
3c1e1c3 
When we need to generate a call to _cgoCheckPointer, we need to type
assert the result back to the desired type. That is harder when the type
is unsafe.Pointer, as the package can have values of unsafe.Pointer
types without actually importing unsafe, by mixing C void* and :=. We
used to handle this by generating a special function for each needed
type, and defining that function in a separate file where we did import
unsafe.

Simplify the code by not generating those functions, but instead just
import unsafe under the alias _cgo_unsafe. This is a simplification step
toward a fix for issue #16591.

Change-Id: I0edb3e04b6400ca068751709fe063397cf960a54
Reviewed-on: https://go-review.googlesource.com/30973
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot
Copy link
Contributor

CL https://golang.org/cl/31233 mentions this issue.

@ianlancetaylor ianlancetaylor mentioned this issue Oct 24, 2016
Closed
@golang golang locked and limited conversation to collaborators Oct 19, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.8Maybe
Development

No branches or pull requests
4 participants
@mdempsky @rsc @quentinmit @gopherbot