code review 180059: Use BitSize instead of the field's type native ByteSize...

Use BitSize instead of the field's type native ByteSize when calculating
structs containing bitfields.

Fixes issue 163.

[dho, 2009-12-16 03:03:29]

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

I'd like you to review the following change.

[rsc, 2009-12-16 03:15:06]

Have you tested this?  It looks to me like you are adding
a number of bits (f.BitSize) to a number of bytes (off).
The right thing to do is probably to skip over the field
entirely: if f.BitSize != 0 { continue }, up before allocating
the new ast.Field.

If someone needs to extract bit fields from a struct
they'll need to write little C accessor functions rather
than try to do it in Go directly.

Russ

[dho, 2009-12-16 03:21:51]

2009/12/15 Russ Cox <rsc@golang.org>:
> Have you tested this?  It looks to me like you are adding
> a number of bits (f.BitSize) to a number of bytes (off).
> The right thing to do is probably to skip over the field
> entirely: if f.BitSize != 0 { continue }, up before allocating
> the new ast.Field.
>
> If someone needs to extract bit fields from a struct
> they'll need to write little C accessor functions rather
> than try to do it in Go directly.

It seemed to work, but I guess I could have been doing something
horribly wrong. I'll re-review.

--dho

> Russ
>

[dho, 2009-12-16 15:22:56]

PTAL. Now only adds the first member of the bitfield with the full
range, and skips any trailing members of the bitfield after that. I
was able to get this working (sort of). It seems that for e.g.

 21 struct bits {
 22         unsigned int a : 1;
 23         unsigned int b : 1;
 24         unsigned int c : 2;
 25 };
 26
 27 struct bits foo = { 1, 0, 3 };
...
 45 var Foo = C.foo

cgo generates:

var Foo = *_C_foo

which causes an error when compiling with 6g. I can look into fixing
this, but should that go into a separate cl?

--dho

[dho, 2009-12-16 16:22:19]

It occurs to me that it's worth pointing out the error:

/home/dho/bin/6g -o _go_.6 file.cgo1.go _cgo_gotypes.go
file.cgo1.go:47: fatal error: sgen copy -1000000000

If I change the

var Foo = *_C_foo

to

var Foo = _C_foo

I can access the struct properly.

Also, please ignore that PTAL, it's still broken.

--dho

[dho, 2009-12-16 16:57:06]

(re-PTAL)

[rsc, 2009-12-16 17:25:21]

http://codereview.appspot.com/180059/diff/9/10
File src/cmd/cgo/gcc.go (right):

http://codereview.appspot.com/180059/diff/9/10#newcode698
src/cmd/cgo/gcc.go:698: if bitsRemaining > 0 {
I don't think it makes sense to show the first bit field,
since it will be the wrong value anyway. 
It looks like the code already knows to insert padding
when things are missing, so it should suffice to do

if f.BitSize > 0 && f.BitSize != f.ByteSize*8 {
    continue
}

[dho, 2009-12-16 18:30:42]

> http://codereview.appspot.com/180059/diff/9/10#newcode698
> src/cmd/cgo/gcc.go:698: if bitsRemaining > 0 {
> I don't think it makes sense to show the first bit field,
> since it will be the wrong value anyway. 
> It looks like the code already knows to insert padding
> when things are missing, so it should suffice to do
> 
> if f.BitSize > 0 && f.BitSize != f.ByteSize*8 {
>     continue
> }

I figured it'd be the right size and contain the entirety of the data, which
would be easy enough to get with masking. As it is, now, the struct gets
converted to a struct { _ [4]byte }, but I guess it's simple enough to make a Go
struct for that and get the right type. 

Thanks

PTAL

--dho

[rsc, 2009-12-17 20:33:57]

LGTM

[rsc, 2009-12-17 20:38:32]

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

Use BitSize instead of the field's type native ByteSize when calculating
structs containing bitfields.

Fixes issue 163.

R=rsc
CC=golang-dev
http://codereview.appspot.com/180059

Committer: Russ Cox <rsc@golang.org>