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.

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

cmd/cgo: C functions that return void incorrectly return Go values #21878

Open
bcmills opened this Issue Sep 14, 2017 · 9 comments

Comments

Projects
None yet
4 participants
@bcmills
Member

bcmills commented Sep 14, 2017

cgo fails to reject the following program — despite numerous errors involving values of type C.void, which ought to be completely uninstantiable. (The word “void” itself means “empty”, so it's nonsensical for the type “void” to contain a value!)

I'm not sure to what extent this is fixable, given that the fix for #3729 added a test verifying that Go callers can bind _, err to the result of a call to a void-returning function.

package p

/*
static void return_void() { return; }
*/
import "C"

func F() {
	x := C.return_void() // ERROR HERE
	var y C.void = x     // ERROR HERE
	var z *C.void = &y   // ERROR HERE
	var b [0]byte = *z   // ERROR HERE
	_ = b
}
@gopherbot

This comment has been minimized.

gopherbot commented Sep 14, 2017

Change https://golang.org/cl/63830 mentions this issue: cmd/cgo: use a named type to indicate syntactic context

@gopherbot

This comment has been minimized.

gopherbot commented Sep 14, 2017

Change https://golang.org/cl/63831 mentions this issue: cmd/cgo: do not instantiate C.void

@bcmills

This comment has been minimized.

Member

bcmills commented Sep 15, 2017

Some more examples of why this is a problem:

  • We (confusingly) map C.void to a value when it is a return-type, but not when it is a parameter type:
//

package p

/*
static void return_void() { return; }
static void consume_void(void) { return; }
*/
import "C"

func F() {
	impossible := C.return_void()  // Today: no error here.
	C.consume_void(impossible)
}

The error today:

src/issue21878d.go:15: too many arguments in call to _Cfunc_consume_void
	have (C.void)
	want ()
  • We report an error that unsafe.Pointer does not match *C.void, even though we map functions that return void* to return unsafe.Pointer.
//
package p

/*
static void return_void() { return; }
static void consume_void_ptr(void* unused) { return; }
*/
import "C"

func F() {
	impossible := C.return_void()  // Today: no error here.
	C.consume_void_ptr(&impossible)
}

The error today:

# command-line-arguments
src/issue21878e.go:15: cannot use &impossible (type *C.void) as type unsafe.Pointer in argument to func literal
  • On the other hand, if we allow *C.void in cgo source, this program should be correct:
package cgotest

/*
static void* return_void_ptr() { return 0; }
static void consume_void_ptr(void* unused) { return; }
*/
import "C"

func testVoidPtrMustCompile() {
	var p *C.void = C.return_void_ptr()
	C.consume_void_ptr(p)
}

But today it fails to compile:

./issue21818.go:14: cannot use _Cfunc_return_void_ptr() (type unsafe.Pointer) as type *C.void in assignment
./issue21818.go:15: cannot use p (type *C.void) as type unsafe.Pointer in argument to func literal
@bcmills

This comment has been minimized.

Member

bcmills commented Sep 15, 2017

The root of the problem is that we're treating C.void as though it were a complete, instantiable type when it's not. That means that we currently translate void* to two different Go types: when void* appears in C code, we translate it to unsafe.Pointer, but when it appears in Go code, we translate it to *_Ctype_void.

@hirochachacha notes that we could fix that by having the cgo command rewrite *C.void to unsafe.Pointer whenever it occurs, similar to what I proposed to do for other incomplete types in #19487, but if we don't also eliminate C.void itself we would still end up with really confusing error messages for the second example above.

@bcmills

This comment has been minimized.

Member

bcmills commented Sep 15, 2017

The only code that I have been able to find to date that actually uses *C.void is this file by @paulsmith:
https://github.com/paulsmith/gogeos/blob/master/geos/cwrappers.go

It is a mechanically-generated wrapper, and the erroneously-wrapped *C.void functions appear not to actually be called.

(But I will note that searching for C.void in GitHub is severely hindered by GitHub's stubborn insistence on ignoring all dots in the query string.)

@bcmills

This comment has been minimized.

Member

bcmills commented Sep 15, 2017

As another point of reference, the string C.void does not appear in any source code in the Go Corpus v0.01.

bcmills:~/go-corpus-0.01$ grep 'C\.int' -r . | wc -l
2082
bcmills:~/go-corpus-0.01$ grep 'C\.void' -r . | wc -l
0

gopherbot pushed a commit that referenced this issue Sep 19, 2017

cmd/cgo: use a named type to indicate syntactic context
We previously used bare strings, which made it difficult to see (and
to cross-reference) the set of allowed context values.

This change is purely cosmetic, but makes it easier for me to
understand how to address #21878.

updates #21878

Change-Id: I9027d94fd5997a0fe857c0055dea8719e1511f03
Reviewed-on: https://go-review.googlesource.com/63830
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@dmitshur

This comment has been minimized.

Member

dmitshur commented Sep 19, 2017

The word “void” itself means “empty”, so it's nonsensical for the type “void” to contain a value!

The root of the problem is that we're treating C.void as though it were a complete, instantiable type when it's not.

Just wondering, but is it wrong to think of void as equivalent to Go's struct{} type (the empty struct)? While the empty struct cannot contain any values, it's still a real type that can be instantiated and assigned. Is void different from that?

@bcmills

This comment has been minimized.

Member

bcmills commented Sep 19, 2017

While the empty struct cannot contain any values, it's still a real type that can be instantiated and assigned. Is void different from that?

void is different. See §6.3.2.2 of the C11 spec:

“The (nonexistent) value of a void expression (an expression that has type void) shall not
be used in any way, and implicit or explicit conversions (except to void) shall not be
applied to such an expression.”

(Note, however, that there is no C type corresponding to the Go type struct{}:
“The behavior is undefined [when … a] structure or union is defined as containing no named members, no anonymous structures, and no anonymous unions (6.7.2.1).”)

In contrast, there is exactly one Go value of type struct{}, written as struct{}{}. It can be converted explicitly to other types that have struct{} as their underlying type, and can be distinguished from nil when stored in a variable of type interface{}.

@bcmills

This comment has been minimized.

Member

bcmills commented Sep 19, 2017

(But see also #20275 (comment): having a non-instantiable unsafe.Void type in Go would potentially allow for better wrapping of C structs with trailing members of incomplete type.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment