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.

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: inconsistent type for char arrays in structs #8428

Closed
snaury opened this issue Jul 27, 2014 · 7 comments

Comments

@snaury
Copy link
Contributor

commented Jul 27, 2014

I noticed that Go 1.3 (and tip) with the following program:

http://play.golang.org/p/4Zr8yEmWVN

Produces inconsistent type for the data field in struct. In struct result1_t the data
field is correctly translated into data [0]_Ctype_char. However in result2_t, probably
due to an additional alignment field, it is translated into data [0]byte. It is very
frustrating, since [0]_Ctype_char and [0]byte are not compatible (being technically
different types), so depending on alignment (e.g. pointer size of the architecture) and
surrounding fields structures may be translated differently. Is it possible to always
emit correct type [0]_Ctype_char in this case, or would that break compatibility promise?

Attachments:

  1. mypackage.go (534 bytes)
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2014

Comment 1:

Labels changed: added repo-main, release-go1.4.

Status changed to Accepted.

@mdempsky

This comment has been minimized.

Copy link
Member

commented Aug 6, 2014

Comment 2:

I think this is related to the heuristic that was used to address issue #2806, but I'm
still trying to grok exactly what's going on.
For what it's worth, as a workaround, if the structs are defined using proper C99
flexible array members (i.e., "char data[];" instead of "char data[0];"), then cgo is
consistently uses [0]_Ctype_char for both structs.
The problem seems to be that for "struct { void *next; char flag; char data[0]; }"'s
data member, debug/dwarf is returning a dwarf.ArrayType with Count==-1, whereas changing
"[0]" to "[]" or removing the "next" field causes debug/dwarf to return one with
Count==0 instead.
@mdempsky

This comment has been minimized.

Copy link
Member

commented Aug 6, 2014

Comment 3:

Nevermind, the flexible array members "workaround" is actually just a side effect of a
bug in debug/dwarf.  debug/dwarf keeps a cache of already processed DWARF types, but it
then calls zeroArray() to mutate parsed dwarf.Type values based on their presence within
a particular struct.  However, that mutation might be invalid if the dwarf.Type is
shared by other structs that don't meet the same criteria to invoke zeroArray().
E.g., either disabling the type cache or removing any Go references to
C.struct_result1_t will cause C.struct_result2_t to be generated with [0]byte again
instead of [0]_Ctype_char.
@mdempsky

This comment has been minimized.

Copy link
Member

commented Aug 12, 2014

Comment 4:

As a quick update: I've made some progress on this issue and seem to have a fix, but I'm
still trying to make sure it doesn't accidentally break any of the cases that led to the
current code.
Incidentally, I've noticed a few additional bugs related to this that I think are worth
noting so I don't forget:
1. zeroArray() is a bit overzealous by recursing into child array types, which results
in "int x[][4];" getting translated to "x [0][0]int32" instead of "x [0][4]int32".
2. Currently zeroArray() doesn't take into account that the array element type might
already have size 0, so "int x[4][0];" gets translated to "x [0][0]int32".  (In practice
this probably isn't worth worrying about, but it's also easy to fix.)
3. The handling of arrays with more than two dimensions is wrong; e.g., it translates
"int z[1][2][3][4];" into "z [1][4][3][2]int32".
@gopherbot

This comment has been minimized.

Copy link

commented Aug 13, 2014

Comment 5:

CL https://golang.org/cl/127980043 mentions this issue.
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2014

Comment 6:

This issue was closed by revision 078a9cb.

Status changed to Fixed.

@gopherbot

This comment has been minimized.

Copy link

commented Aug 29, 2014

Comment 7:

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

@snaury snaury added fixed labels Aug 29, 2014

@rsc rsc added this to the Go1.4 milestone Apr 14, 2015

@rsc rsc removed the release-go1.4 label Apr 14, 2015

@golang golang locked and limited conversation to collaborators Jun 25, 2016

wheatman added a commit to wheatman/go-akaros that referenced this issue Jun 25, 2018
cmd/cgo, debug/dwarf: fix translation of zero-size arrays
In cgo, now that recursive calls to typeConv.Type() always work,
we can more robustly calculate the array sizes based on the size
of our element type.

Also, in debug/dwarf, the decision to call zeroType is made
based on a type's usage	within a particular struct, but dwarf.Type
values are cached in typeCache, so the modification might affect
uses of the type in other structs.  Current compilers don't appear
to share DWARF type entries for "[]foo" and "[0]foo", but they also
don't consistently share type entries in other cases.  Arguably
modifying the types is an improvement in some cases, but varying
translated types according to compiler whims seems like a bad idea.

Lastly, also in debug/dwarf, zeroType only needs to rewrite the
top-level dimension, and only if the rest of the array size is
non-zero.

Fixes golang#8428.

LGTM=iant
R=iant
CC=golang-codereviews
https://golang.org/cl/127980043
wheatman added a commit to wheatman/go-akaros that referenced this issue Jul 9, 2018
cmd/cgo, debug/dwarf: fix translation of zero-size arrays
In cgo, now that recursive calls to typeConv.Type() always work,
we can more robustly calculate the array sizes based on the size
of our element type.

Also, in debug/dwarf, the decision to call zeroType is made
based on a type's usage	within a particular struct, but dwarf.Type
values are cached in typeCache, so the modification might affect
uses of the type in other structs.  Current compilers don't appear
to share DWARF type entries for "[]foo" and "[0]foo", but they also
don't consistently share type entries in other cases.  Arguably
modifying the types is an improvement in some cases, but varying
translated types according to compiler whims seems like a bad idea.

Lastly, also in debug/dwarf, zeroType only needs to rewrite the
top-level dimension, and only if the rest of the array size is
non-zero.

Fixes golang#8428.

LGTM=iant
R=iant
CC=golang-codereviews
https://golang.org/cl/127980043
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.