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

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

cmd/cgo: inconsistent type for char arrays in structs #8428

snaury opened this issue Jul 27, 2014 · 7 comments
Labels
FrozenDueToAge
Milestone
Go1.4

Comments

@snaury
Copy link
Contributor

snaury 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
Copy link
Member

Comment 1:

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

Status changed to Accepted.

@mdempsky
Copy link
Contributor

mdempsky 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
Copy link
Contributor

mdempsky 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
Copy link
Contributor

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
Copy link
Contributor

Comment 5:

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

@ianlancetaylor
Copy link
Member

Comment 6:

This issue was closed by revision 078a9cb.

Status changed to Fixed.

@gopherbot
Copy link
Contributor

Comment 7:

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

@rsc rsc added this to the Go1.4 milestone Apr 14, 2015
@rsc rsc removed the release-go1.4 label Apr 14, 2015
@ianlancetaylor ianlancetaylor mentioned this issue Jul 30, 2015
Closed
@golang golang locked and limited conversation to collaborators Jun 25, 2016
wheatman pushed a commit to wheatman/go-akaros that referenced this issue Jun 25, 2018
@mdempsky @wheatman
cmd/cgo, debug/dwarf: fix translation of zero-size arrays
6eb3ee7 
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 pushed a commit to wheatman/go-akaros that referenced this issue Jul 9, 2018
@mdempsky @wheatman
cmd/cgo, debug/dwarf: fix translation of zero-size arrays
3a4baa4 
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.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.4
Development

No branches or pull requests
5 participants
@mdempsky @snaury @rsc @ianlancetaylor @gopherbot