code review 4564043: godefs: do not assume forward type references are enums

godefs: do not assume forward type references are enums

Fixes issue 1466.

[rh, 2011-05-30 22:35:16]

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

I'd like you to review this change to
https://go.googlecode.com/hg/

[mikio, 2011-05-31 11:28:34]

LGTM, no side effect for issue 432 fix?

On Tue, May 31, 2011 at 7:35 AM,  <robert.hencke@gmail.com> wrote:
> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> godefs: do not assume forward type references are enums
>
> Fixes issue 1466.
>
> Please review this at http://codereview.appspot.com/4564043/
>
> Affected files:
>  M src/cmd/godefs/stabs.c
>  M src/pkg/syscall/ztypes_darwin_386.go
>  M src/pkg/syscall/ztypes_darwin_amd64.go
>
>
> Index: src/cmd/godefs/stabs.c
> ===================================================================
> --- a/src/cmd/godefs/stabs.c
> +++ b/src/cmd/godefs/stabs.c
> @@ -381,14 +381,6 @@
>
>                        while(f->type->kind == Typedef)
>                                f->type = f->type->type;
> -                       if(f->type->kind == 0 && f->size <= 64 &&
> (f->size&(f->size-1)) == 0) {
> -                               // unknown type but <= 64 bits and bit size
> is a power of two.
> -                               // could be enum - make Uint64 and then let
> it reduce
> -                               tt = emalloc(sizeof *tt);
> -                               *tt = *f->type;
> -                               f->type = tt;
> -                               tt->kind = Uint64;
> -                       }
>
>                        // rewrite
>                        //      uint32 x : 8;
> Index: src/pkg/syscall/ztypes_darwin_386.go
> ===================================================================
> --- a/src/pkg/syscall/ztypes_darwin_386.go
> +++ b/src/pkg/syscall/ztypes_darwin_386.go
> @@ -2,10 +2,6 @@
>
>  // MACHINE GENERATED - DO NOT EDIT.
>
> -// Manual corrections: TODO: need to fix godefs (issue 1466)
> -//     change Msghdr field to Iov *Iovec (was uint32/64)
> -//     change BpfProgram field to Insns *BpfInsn (was uint32/64)
> -
>  package syscall
>
>  // Constants
> Index: src/pkg/syscall/ztypes_darwin_amd64.go
> ===================================================================
> --- a/src/pkg/syscall/ztypes_darwin_amd64.go
> +++ b/src/pkg/syscall/ztypes_darwin_amd64.go
> @@ -2,10 +2,6 @@
>
>  // MACHINE GENERATED - DO NOT EDIT.
>
> -// Manual corrections: TODO: need to fix godefs (issue 1466)
> -//     change Msghdr field to Iov *Iovec (was uint32/64)
> -//     change BpfProgram field to Insns *BpfInsn (was uint32/64)
> -
>  package syscall
>
>  // Constants
>
>
>

[rsc, 2011-05-31 14:41:14]

I am worried about this rebreaking issue 432.
Can you try the SDL test case?

[rh, 2011-06-01 02:26:22]

Thank you for the reviews.

You're right... this breaks 432.  I'll dig some more.

[rh, 2011-06-01 03:15:41]

Hello golang-dev@googlegroups.com, mikioh.mikioh@gmail.com, rsc@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[rh, 2011-06-01 03:16:32]

Sorry, this is missing a file... one sec.

[rh, 2011-06-01 03:21:26]

PTAL.

This generates the following for issue 432 before and after patching on my box. 

type keysym struct {
	Scancode uint8;
	Pad_godefs_0 [3]byte;
	Sym uint32;
	Mod uint32;
	Unicode uint16;
	Pad_godefs_1 [2]byte;
}

ztypes_darwin regenerates the same as well (sans gofmt).

(Should I try to add any testing to godefs?)

[mikio, 2011-06-01 10:06:13]

I've just confirmed that the updated CL works well on freebsd and
linux except linux/arm. ztypes files are generated the same as
previous.

On Wed, Jun 1, 2011 at 12:21 PM,  <robert.hencke@gmail.com> wrote:

> PTAL.
>
> This generates the following for issue 432 before and after patching on
> my box.
>
> type keysym struct {
>        Scancode uint8;
>        Pad_godefs_0 [3]byte;
>        Sym uint32;
>        Mod uint32;
>        Unicode uint16;
>        Pad_godefs_1 [2]byte;
> }
>
> ztypes_darwin regenerates the same as well (sans gofmt).
>
> (Should I try to add any testing to godefs?)
>
> http://codereview.appspot.com/4564043/
>

[mikio, 2011-06-01 10:16:59]

On Wed, Jun 1, 2011 at 7:06 PM, Mikio Hara <mikioh.mikioh@gmail.com> wrote:

> I've just confirmed that the updated CL works well on freebsd and
> linux except linux/arm.

I mean, this fix does not solve godefs's another limitation for linux/arm.

ztypes_linux_arm.go
// Manual corrections: TODO(rsc): need to fix godefs
//      remove duplicate PtraceRegs type
//      change RawSockaddrUnix field to Path [108]int8 (was uint8)
//      add padding to EpollEvent

Looks like no side effects, LGTM.

[rh, 2011-06-01 13:37:19]

Thank you for testing this on freebsd/linux!

[rsc, 2011-06-02 14:17:29]

This is great, thanks.

http://codereview.appspot.com/4564043/diff/16001/src/cmd/godefs/main.c
File src/cmd/godefs/main.c (right):

http://codereview.appspot.com/4564043/diff/16001/src/cmd/godefs/main.c#newcod...
src/cmd/godefs/main.c:379: for (k = Uint64; k >= Uint8 && kindsize[k] >=
f->size; k -= 2) {
I'm a bit confused by this loop.  Maybe it is supposed to be

k = Uint64
while (...)
    k -= 2;
tt = emalloc...
...

?

[rh, 2011-06-03 02:38:25]

PTAL

http://codereview.appspot.com/4564043/diff/22002/src/cmd/godefs/main.c
File src/cmd/godefs/main.c (right):

http://codereview.appspot.com/4564043/diff/22002/src/cmd/godefs/main.c#newcod...
src/cmd/godefs/main.c:383: while(tt->kind > Uint8 && kindsize[tt->kind] >
f->size)
Is this closer?

[rsc, 2011-06-07 18:28:12]

*** Submitted as http://code.google.com/p/go/source/detail?r=8fc8a4975c03 ***

godefs: do not assume forward type references are enums

Fixes issue 1466.

R=mikioh.mikioh, rsc
CC=golang-dev
http://codereview.appspot.com/4564043

Committer: Russ Cox <rsc@golang.org>

[rsc, 2011-06-07 18:28:21]

LGTM