cmd/cgo: -godefs doesn't handle embedded struct fields correctly #5253

Open
gopherbot opened this Issue Apr 10, 2013 · 9 comments

5 participants

@gopherbot

by Mortdeus@gocos2d.org:

Before filing a bug, please check whether it has been fixed since the
latest release. Search the issue tracker and check that you're running the
latest version of Go:

Run "go version" and compare against
http://golang.org/doc/devel/release.html  If a newer version of Go exists,
install it and retry what you did to reproduce the problem.

Thanks.

What steps will reproduce the problem?
This struct...

//<libdrm/drm.h>
struct drm_stats {
        unsigned long count;
    struct {
        unsigned long value;
        enum drm_stat_type type;
    } data[15];
};

When wrapped with cgo as

`
Stats C.struct_drm_stats
`

gets converted by godefs to

`
type Stats struct {
    Count uint64
    Data  [15]_Ctype_struct___0
}
`

What is the expected output?
type Stats struct {
         Count uint64
         Data [15]struct{
              Value uint64 //or uint32
              Type uint32
         }
}

Which compiler are you using (5g, 6g, 8g, gccgo)?
6g

Which operating system are you using?
archlinux x86-64

Which version are you using?  (run 'go version')
go version devel +4a712e80e9b1 Tue Apr 09 15:00:21 2013 -0700 linux/amd64

Please provide any additional information below.

https://github.com/mortdeus/egles/tree/master/drm
@minux
Go member

Comment 1:

$ cat test.go
package test

/*
struct A {
        unsigned long count;
        struct {
                long value;
        } data[15];
};
*/
import "C"

type A C.struct_A

$ go tool cgo -godefs test.go
// Created by cgo -godefs - DO NOT EDIT
// cgo -godefs test.go

package test

type A struct {
        Count   uint64
        Data    [15]_Ctype_struct___0
}


@Mortdeus, please provide minimal and standalone test cases (like above)
next time, thanks.

Labels changed: added priority-later, removed priority-triage.

Status changed to Accepted.

@mkb218

Comment 2:

Is there a workaround for this? It's breaking a project of mine that worked in 1.1.2
@gopherbot

Comment 3 by Mortdeus@gocos2d.org:

I just manually wrote the them in.
@rsc

Comment 5:

Labels changed: added go1.3maybe.

@rsc
rsc commented Dec 4, 2013

Comment 6:

Labels changed: added release-none, removed go1.3maybe.

@rsc
rsc commented Dec 4, 2013

Comment 7:

Labels changed: added repo-main.

@mdempsky
Go member

Comment 8:

I think this is reasonably fixable.  We just need to change the handling of unnamed
dwarf.StructType's to directly return a struct definition rather than indirectly via a
dummy identifier and typedef.  E.g., with a minor change on top of my patch for issue
8441 (mostly to make sure assigning to t.Go after the recursive call to c.Struct() is
safe), I get:

$ go tool cgo -godefs issue5253.go
// Created by cgo -godefs - DO NOT EDIT
// cgo -godefs issue5253.go

package test

type A struct {
        Count   uint64
        Data    [15]struct {
                Value int64
        }
}

There's a possible issue if C code does something silly like "typedef struct { ... } X,
Y;" then C.X and C.Y would now become distinct types, whereas previously they would be
treated as aliases of type __1.  I don't think that's an issue in practice, but it's
probably solvable if needed.

There's also an exponential code size blow up from pathological inputs like:

/*
typedef struct foo {
        struct {
                struct {
                        struct {
                                int x;
                        } a, b;
                } a, b;
        } a, b;
} A, B;
*/
import "C"

but that could be fixed by making (*typeConv).Struct() smarter when consecutive fields
reference the same DWARF type.  Though again, I don't expect it to be an issue in
practice.
@mdempsky
Go member

Comment 9:

I've put together https://golang.org/cl/122900043 to fix the issue as described
in the initial bug report here.  E.g., for the input provided by minux in #1, here's the
output from patched cgo:

<<<<
$ go tool cgo -godefs test.go
// Created by cgo -godefs - DO NOT EDIT
// cgo -godefs test.go

package test

type A struct {
        Count   uint64
        Data    [15]struct {
                Value int64
        }
}
>>>>

I'd appreciate if any users affected by this could test that this patch (applied to tip)
fixes their issues with cgo -godefs, or if there are more details to address.
@rsc rsc added this to the Unplanned milestone Apr 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment