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: permits reference to non-existent struct #19487

Open
ianlancetaylor opened this Issue Mar 10, 2017 · 12 comments

Comments

Projects
None yet
6 participants
@ianlancetaylor
Contributor

ianlancetaylor commented Mar 10, 2017

This code compiles and runs:

package main

import "C"

import "fmt"

var V C.struct_X

func main() {
        fmt.Println(V)
}

It prints {}. That makes no sense. There is no type X here. It should give an error somewhere.

@ianlancetaylor ianlancetaylor added this to the Go1.9 milestone Mar 10, 2017

@ghost

This comment has been minimized.

ghost commented Mar 23, 2017

Also applies to non-existent unions, but not enums.

@gopherbot

This comment has been minimized.

gopherbot commented Jun 4, 2017

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

@hirochachacha

This comment has been minimized.

Contributor

hirochachacha commented Jun 4, 2017

Once we refer C.struct_undefined, cgo emits __typeof__(struct undefined) *__cgo__0;.
The problem is that struct undefined doesn't have complete definition. We can check completeness of the type by __typeof__(typename) _[0];, because C prohibits array of an incomplete type.

I think one solid solution here is rejecting all incomplete types.
Incomplete types are sort of tentative placeholder, however we cannot complete those types on Go side. Thus I think exporting incomplete types to Go world have no semantic meanings.

That said, those changes brake some backward compatibility. So it may be controversial. I sent my CL, but please feel free to reject it :)

@bcmills

This comment has been minimized.

Member

bcmills commented Jun 5, 2017

That said, those changes [break] some backward compatibility.

Not just backward compatibility, but also compatibility with a wide variety of C APIs. It's fine to pass around pointers to an incomplete struct type: the problem occurs when you're trying to actually instantiate a struct of that type.

@bcmills

This comment has been minimized.

Member

bcmills commented Jun 5, 2017

But perhaps that implies that cgo shouldn't generate a Go type at all for an incomplete C type.

Instead of doing the equivalent of

type struct_x struct { _ [0]byte }

perhaps we could emit a Go struct type for *C.struct_x and rewrite pointers to values of that type:

type ptr_struct_x struct { _ unsafe.Pointer }

Assuming that the Go compiler generates correct code to copy structs with blank-identifier fields, that would allow Go code to copy values of type ptr_struct_x but not dereference them, achieving the same effect as a pointer to incomplete type in C.

@hirochachacha

This comment has been minimized.

Contributor

hirochachacha commented Jun 5, 2017

Not just backward compatibility, but also compatibility with a wide variety of C APIs. It's fine to pass around pointers to an incomplete struct type: the problem occurs when you're trying to actually instantiate a struct of that type.

The CL only rejects direct use of incomplete type in Go. It doesn't prohibit indirect use of incomplete type.
For example:

package main

/*
#cgo LDFLAGS: -L${SRCDIR} opaque.dylib
typedef struct PersonImpl *Person;
Person createPerson(int age);
void destroyPerson(Person p);
void printPerson(Person p);
*/
import "C"

func main() {
	p := C.createPerson(10)
	C.printPerson(p)
	C.destroyPerson(p)
}

will produce

type _Ctype_Person *_Ctype_struct_PersonImpl
type _Ctype_struct_PersonImpl struct{}

into _cgo_gotypes.go. This is the same behavior as current code. I confirmed that opaque pointer pattern is still working.

I roughly understand that 'incomplete type' means that the type cannot determine its own size at compile time. All pointers have the exact size (e.g. 8 byte on amd64), so 'pointer to incomplete type' aren't 'incomplete type' themselves.

But perhaps that implies that cgo shouldn't generate a Go type at all for an incomplete C type.

I'm not correctly sure what are you suggesting here. But if you are suggesting

type _Ctype_Person unsafe.Pointer

I agree with you. Current code have already done the similar decision for void* . It is the generalized decision of that.

@bcmills

This comment has been minimized.

Member

bcmills commented Jun 5, 2017

The CL only rejects direct use of incomplete type in Go. It doesn't prohibit indirect use of incomplete type.

If I'm reading it correctly, it would break cgo bindings for any API that provides "create" and "destroy" functions. (cbrotli is one such example.) Given that the existing cgo code is correct as written and that style is fairly common for C bindings (see also libsdl), I don't think that's acceptable.

But if you are suggesting
type _Ctype_Person unsafe.Pointer

No, I'm suggesting that we not require the Person typedef at all. To use your example:

package main

/*
#cgo LDFLAGS: -L${SRCDIR} opaque.dylib
#include "opaque.h"
*/
import "C"

func main() {
	p := C.createPerson(10)
	C.printPerson(p)
	C.destroyPerson(p)
}

opaque.h:

typedef struct Person Person;
Person* createPerson(int age);
void destroyPerson(Person* p);
void printPerson(Person* p);

In the cgo-translated code, p would have type _Ctype_ptr_Person, which would be defined as

type _Ctype_ptr_Person struct { _ unsafe.Pointer } 

That way, Go code could still copy and assign Person pointers directly, but it would be impossible to construct a non-nil *C.Person in Go code without explicit use of unsafe.

The one caveat to that approach is that we would also have to translate:

var p *C.person = nil

to

var p _Ctype_ptr_Person = _Ctype_ptr_Person{}

since nil is not a valid zero-value for a struct type (see also #19642 (comment)).

@hirochachacha

This comment has been minimized.

Contributor

hirochachacha commented Jun 6, 2017

If I'm reading it correctly, it would break cgo bindings for any API that provides "create" and "destroy" functions. (cbrotli is one such example.) Given that the existing cgo code is correct as written and that style is fairly common for C bindings (see also libsdl), I don't think that's acceptable.

I understand I was wrong. Thank you.

No, I'm suggesting that we not require the Person typedef at all. To use your example:

I cannot understand why do you need to wrap unsafe.Pointer in struct.
Is unsafe.Pointer assignable and copyable, isn't it?

@hirochachacha

This comment has been minimized.

Contributor

hirochachacha commented Jun 6, 2017

Anyway, I have no idea to solve this problem now.
I can reject var _ C.struct_undefined, but I cannot reject var _ *C.struct_undefined.

@bradfitz bradfitz added the NeedsFix label Jun 7, 2017

@bcmills

This comment has been minimized.

Member

bcmills commented Jun 7, 2017

I cannot understand why do you need to wrap unsafe.Pointer in struct.
Is unsafe.Pointer assignable and copyable, isn't it?

Yes, but it allows for some potentially-surprising conversions (see #20171).

@bcmills

This comment has been minimized.

Member

bcmills commented Sep 21, 2017

Thinking about this further, the type _Ctype_ptr_Person struct { _ unsafe.Pointer } approach I described above would not allow pointers to incomplete types to be converted to unsafe.Pointer. That seems like a fairly significant property to lose.

@rsc

This comment has been minimized.

Contributor

rsc commented Nov 22, 2017

I don't see that there's an obvious fix here. The current behavior might be the best of the possible choices. In any event, not for Go 1.10.

@rsc rsc modified the milestones: Go1.10, Go1.11 Nov 22, 2017

@gopherbot gopherbot modified the milestones: Go1.11, Unplanned May 23, 2018

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