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: never generate Go fields for zero-sized C fields #20275

Open
zagrodzki opened this Issue May 7, 2017 · 6 comments

Comments

Projects
None yet
6 participants
@zagrodzki

zagrodzki commented May 7, 2017

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

tested with 1.7.5 and 1.8.1

What operating system and processor architecture are you using (go env)?

linux/amd64

What did you do?

package main

/*
struct foo {
	int x;
	char padding;
	char y[];
};
*/
import "C"
import "fmt"

var Y C.struct_foo

func main() {
	fmt.Println(Y.y)
}

What did you expect to see?

Y.y undefined (type C.struct_foo has no field or method y)

(because Y.y is a zero-length field and Go code cannot refer to these fields)

What did you see instead?

Compiles and prints:

[]

I've checked that it's dependent on memory alignment. If the padding field is removed from struct foo, generated cgo type doesn't have the y field. With the padding, the field is included.

@IanTayler

This comment has been minimized.

IanTayler commented May 7, 2017

I was able to reproduce with go version go1.8.1 linux/amd64 on Solus, using the code @zagrodzki provided. Both the case where Y.y exists and the case where it doesn't (i.e. without padding).

If we change the array to a pointer we get <nil> with or without padding (which, I assume, is the intended behaviour).

@ianlancetaylor ianlancetaylor added this to the Go1.9Maybe milestone May 8, 2017

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented May 8, 2017

This is happening because the implicit padding added by these lines (in typeconv.Struct in cmd/cgo/gcc.go)

	if off < dt.ByteSize {
		fld, sizes = c.pad(fld, sizes, dt.ByteSize-off)
		off = dt.ByteSize
	}

is adding a new trailing field and thus breaking the check of whether the actual trailing field has zero size. This does mean that the trailing zero-sized field is going to work in this case, but it would minimize confusion to drop the trailing field in this case as well. Actually, we don't want to drop it, we want to rename it to _ or something along those lines.

@ianlancetaylor ianlancetaylor changed the title from cgo: zero-sized fields sometimes visible to Go code to cmd/cgo: zero-sized fields sometimes visible to Go code May 8, 2017

@bcmills

This comment has been minimized.

Member

bcmills commented Jun 14, 2017

I'm not sure that we should even rename it. We should convert it to a zero-sized Go array so that it can still be addressed.

There's nothing obviously wrong with this program, for example, assuming that foo_create always returns a pointer with at least one character in y:

package main

/*
struct foo {
	int x;
	char padding;
	char y[];
};
extern struct foo* foo_create();
extern void foo_destroy(struct foo*);
*/
import "C"

import (
    "fmt"
    "unsafe"
)

func main() {
	y := C.foo_create()
	defer C.foo_destroy(y)
	c := (*C.char)((unsafe.Pointer)(&y.y))
	fmt.Println(*c)
}
@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Jun 14, 2017

@bcmills Offhand I don't see how to do that while still preserving the size of the struct as seen in Go as being the same as the size as seen in C. That seems like a difficult property to lose. See https://golang.org/cl/12864 .

@ianlancetaylor ianlancetaylor modified the milestones: Go1.10, Go1.9Maybe Jun 14, 2017

@bcmills

This comment has been minimized.

Member

bcmills commented Jun 15, 2017

I can see how to solve it with a fairly minimal language change, but not without one.

The language change would be to add a distinguished zero-size type (unsafe.Void?) that does not induce padding and cannot be instantiated in Go. That would prevent an unsafe.Void from ever occurring in the Go heap (except by casting through unsafe.Pointer, which is of course unsafe anyway). Then we could map the C struct as

type struct_foo struct{
  x C.int
  padding C.char
  y unsafe.Void
}

and the y field in C-allocated structs could still be accessed via unsafe.Pointer as in the sketch above.

@rsc

This comment has been minimized.

Contributor

rsc commented Nov 22, 2017

I don't want to change cgo this late in the cycle but I would be OK for Go 1.11 with never making that field accessible. That will end the inconsistency. We should not make language changes to Go just for this awful detail of C.

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

@rsc rsc changed the title from cmd/cgo: zero-sized fields sometimes visible to Go code to cmd/cgo: never generate Go fields for zero-sized C fields Nov 22, 2017

@rsc rsc added the NeedsFix label 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