code review 98580046: cmd/cgo: given typedef struct S T, make C.T and C.struc...

cmd/cgo: given typedef struct S T, make C.T and C.struct_S interchangeable

For incomplete struct S, C.T and C.struct_S were interchangeable in Go 1.2
and earlier, because all incomplete types were interchangeable
(even C.struct_S1 and C.struct_S2).

CL 76450043, which fixed issue 7409, made different incomplete types
different from Go's point of view, so that they were no longer completely
interchangeable.

However, imprecision about C.T and C.struct_S - really the same
underlying C type - is the one behavior enabled by the bug that
is most likely to be depended on by existing cgo code.
Explicitly allow it, to keep that code working.

Fixes issue 7786.

[rsc, 2014-05-28 04:00:14]

Hello golang-codereviews@googlegroups.com (cc: iant),

I'd like you to review this change to
https://code.google.com/p/go/

[rsc, 2014-05-28 04:00:25]

Hello golang-codereviews@googlegroups.com (cc:
golang-codereviews@googlegroups.com, iant, iant@golang.org, r),

Please take another look.

[rsc, 2014-05-28 04:00:36]

Added r for go1.3.html

[iant, 2014-05-28 14:08:42]

https://codereview.appspot.com/98580046/diff/60001/src/cmd/cgo/gcc.go
File src/cmd/cgo/gcc.go (right):

https://codereview.appspot.com/98580046/diff/60001/src/cmd/cgo/gcc.go#newcode...
src/cmd/cgo/gcc.go:1215: if dt.ByteSize < 0 {
I think you need && dt.Kind == "struct".

https://codereview.appspot.com/98580046/diff/60001/src/cmd/cgo/gcc.go#newcode...
src/cmd/cgo/gcc.go:1277: if id, ok := sub.Go.(*ast.Ident); *godefs || *cdefs ||
ok && strings.HasPrefix(id.Name, "_C") {
I could be wrong, but I think this code is running before mangleName has been
called on the AST.  So this "_C" is only going to match the _Ctype used for
struct types in typeConv.Type, and it may as well be "_Ctype" for clarity.

https://codereview.appspot.com/98580046/diff/60001/src/cmd/cgo/gcc.go#newcode...
src/cmd/cgo/gcc.go:1280: goIdent[id.Name] = id
Is this the right thing to do if *godefs || *cdefs and the string does not start
with "_C"?

https://codereview.appspot.com/98580046/diff/60001/src/cmd/cgo/gcc.go#newcode...
src/cmd/cgo/gcc.go:1351: if dt.StructName != "" {
&& dt.Kind == "struct"

[rsc, 2014-05-28 15:29:57]

PTAL

https://codereview.appspot.com/98580046/diff/60001/src/cmd/cgo/gcc.go
File src/cmd/cgo/gcc.go (right):

https://codereview.appspot.com/98580046/diff/60001/src/cmd/cgo/gcc.go#newcode...
src/cmd/cgo/gcc.go:1215: if dt.ByteSize < 0 {
On 2014/05/28 14:08:42, iant wrote:
> I think you need && dt.Kind == "struct".

The other cases will break on this too.
I left the condition as is but removed the hard-coded mentions of struct in the
code on the next few lines.
I added a union to the test case in this CL.

https://codereview.appspot.com/98580046/diff/60001/src/cmd/cgo/gcc.go#newcode...
src/cmd/cgo/gcc.go:1277: if id, ok := sub.Go.(*ast.Ident); *godefs || *cdefs ||
ok && strings.HasPrefix(id.Name, "_C") {
On 2014/05/28 14:08:42, iant wrote:
> I could be wrong, but I think this code is running before mangleName has been
> called on the AST.  So this "_C" is only going to match the _Ctype used for
> struct types in typeConv.Type, and it may as well be "_Ctype" for clarity.

I cleaned this up to check for _Ctype_struct_, _Ctype_union_, and _Ctype_class_
only.

https://codereview.appspot.com/98580046/diff/60001/src/cmd/cgo/gcc.go#newcode...
src/cmd/cgo/gcc.go:1280: goIdent[id.Name] = id
On 2014/05/28 14:08:42, iant wrote:
> Is this the right thing to do if *godefs || *cdefs and the string does not
start
> with "_C"?

It's certainly a change. I looked more closely and found that goIdent is used
only in those modes and not in normal cgo mode, so I don't need this and
reverted the change.

https://codereview.appspot.com/98580046/diff/60001/src/cmd/cgo/gcc.go#newcode...
src/cmd/cgo/gcc.go:1351: if dt.StructName != "" {
On 2014/05/28 14:08:42, iant wrote:
> && dt.Kind == "struct"

I think all the dt.Kinds are now handled.

[iant, 2014-05-28 15:33:53]

LGTM

[rsc, 2014-05-28 15:35:33]

Thanks. Will wait for r.

[r, 2014-05-28 16:06:09]

LGTM

https://codereview.appspot.com/98580046/diff/80001/doc/go1.3.html
File doc/go1.3.html (right):

https://codereview.appspot.com/98580046/diff/80001/doc/go1.3.html#newcode255
doc/go1.3.html:255: incomplete struct to a different named type.
</p>

<p>

[rsc, 2014-05-28 18:04:34]

*** Submitted as https://code.google.com/p/go/source/detail?r=177ed19da89f ***

cmd/cgo: given typedef struct S T, make C.T and C.struct_S interchangeable

For incomplete struct S, C.T and C.struct_S were interchangeable in Go 1.2
and earlier, because all incomplete types were interchangeable
(even C.struct_S1 and C.struct_S2).

CL 76450043, which fixed issue 7409, made different incomplete types
different from Go's point of view, so that they were no longer completely
interchangeable.

However, imprecision about C.T and C.struct_S - really the same
underlying C type - is the one behavior enabled by the bug that
is most likely to be depended on by existing cgo code.
Explicitly allow it, to keep that code working.

Fixes issue 7786.

LGTM=iant, r
R=golang-codereviews, iant, r
CC=golang-codereviews
https://codereview.appspot.com/98580046

[gobot, 2014-05-28 18:18:52]

This CL appears to have broken the netbsd-amd64-bsiegert builder.
See http://build.golang.org/log/9744b65d4aaf0be61d453a6e634ac4e432b7ca36