code review 6131060: cgo: rename C names for Go types to avoid conflicting w...

cgo: rename C names for Go types to avoid conflicting with package

Fixes issue 3371.

[iant, 2012-05-01 00:06:46]

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

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

[bsiegert, 2012-05-01 09:43:58]

These typedefs Look Gross To Me.

--Benny.

[r2, 2012-05-01 10:05:07]

Is it feasible to write C that looks like C any more? I guess that ship has
sailed.

-rob

[mtj1, 2012-05-01 11:42:58]

...and Steve Bourne was its captain.

On Tue, May 1, 2012 at 6:05 AM, Rob 'Commander' Pike <r@google.com> wrote:
> Is it feasible to write C that looks like C any more? I guess that ship has
sailed.
>
> -rob
>



-- 
Michael T. Jones | Chief Technology Advocate  | mtj@google.com |  +1
650-335-5765

[r2, 2012-05-01 11:54:58]

On Tue, May 1, 2012 at 9:42 PM, Michael Jones <mtj@google.com> wrote:
> ...and Steve Bourne was its captain.

That's incorrect. Bourne's version of C was Algol-68. Gcc's version
is.... a lot of underscores on things.

By the way, srb took the macros out, well before Unix v8. I worked on
the shell after that, adding things he'd planned and doing some things
on my list too. In doing that work, I learned that underneath the
peculiar macros of the original was one of the cleanest, best-written
C programs I'd ever seen, before and since. It's a shame the macros
are all people know about that code.

-rob

[mtj1, 2012-05-01 12:06:15]

I did not say that he besmirched C, or that his code is bad, only that
he is famous as the Algol-68 compiler writer who brought his preferred
mental environment to the sh task via the flexible C/cpp environment.
I'm unaware of any dismissive or pejorative view, only my memory of
looking at the code. Do I remember correctly that there was:

#define EVER ;;

and

for (EVER) {
:
}

? Maybe it was 'forever' as one chunk.

I'm actually a huge fan of this kind of emacs-of-language scheme where
you define the language that makes sense for your code. To quote John
Lee Hooker's House Rent Blues, "it don't confront me none."

On Tue, May 1, 2012 at 7:54 AM, Rob 'Commander' Pike <r@google.com> wrote:
> On Tue, May 1, 2012 at 9:42 PM, Michael Jones <mtj@google.com> wrote:
>> ...and Steve Bourne was its captain.
>
> That's incorrect. Bourne's version of C was Algol-68. Gcc's version
> is.... a lot of underscores on things.
>
> By the way, srb took the macros out, well before Unix v8. I worked on
> the shell after that, adding things he'd planned and doing some things
> on my list too. In doing that work, I learned that underneath the
> peculiar macros of the original was one of the cleanest, best-written
> C programs I'd ever seen, before and since. It's a shame the macros
> are all people know about that code.
>
> -rob



-- 
Michael T. Jones | Chief Technology Advocate  | mtj@google.com |  +1
650-335-5765

[mtj1, 2012-05-01 12:07:30]

...oh, and to be clear, the lore is fascinating! Thanks!

On Tue, May 1, 2012 at 8:05 AM, Michael Jones <mtj@google.com> wrote:
> I did not say that he besmirched C, or that his code is bad, only that
> he is famous as the Algol-68 compiler writer who brought his preferred
> mental environment to the sh task via the flexible C/cpp environment.
> I'm unaware of any dismissive or pejorative view, only my memory of
> looking at the code. Do I remember correctly that there was:
>
> #define EVER ;;
>
> and
>
> for (EVER) {
> :
> }
>
> ? Maybe it was 'forever' as one chunk.
>
> I'm actually a huge fan of this kind of emacs-of-language scheme where
> you define the language that makes sense for your code. To quote John
> Lee Hooker's House Rent Blues, "it don't confront me none."
>
> On Tue, May 1, 2012 at 7:54 AM, Rob 'Commander' Pike <r@google.com> wrote:
>> On Tue, May 1, 2012 at 9:42 PM, Michael Jones <mtj@google.com> wrote:
>>> ...and Steve Bourne was its captain.
>>
>> That's incorrect. Bourne's version of C was Algol-68. Gcc's version
>> is.... a lot of underscores on things.
>>
>> By the way, srb took the macros out, well before Unix v8. I worked on
>> the shell after that, adding things he'd planned and doing some things
>> on my list too. In doing that work, I learned that underneath the
>> peculiar macros of the original was one of the cleanest, best-written
>> C programs I'd ever seen, before and since. It's a shame the macros
>> are all people know about that code.
>>
>> -rob
>
>
>
> --
> Michael T. Jones | Chief Technology Advocate  | mtj@google.com |  +1
> 650-335-5765



-- 
Michael T. Jones | Chief Technology Advocate  | mtj@google.com |  +1
650-335-5765

[iant2, 2012-05-01 13:19:49]

Rob 'Commander' Pike <r@google.com> writes:

> Is it feasible to write C that looks like C any more? I guess that ship has
sailed.

What we need here is not ordinary C, but the C types that precisely
correspond to Go types.  Go types have precise sizes and C types do not.
GCC provides extensions to C to get types with precise sizes, and that's
what I'm using here.

This does have the drawback that it will only be portable to compilers
that support these GCC extensions.  Perhaps I should put this code in
#ifdef __GNUC__ and use the old guesses for other compilers.

We could also use <inttype.h>/<stdint.h>, but I'm concerned that that
will break some user packages as in issue 3371, albeit in a different
way.

Ian

[rsc, 2012-05-01 13:56:04]

http://codereview.appspot.com/6131060/diff/1/src/cmd/cgo/out.go
File src/cmd/cgo/out.go (right):

http://codereview.appspot.com/6131060/diff/1/src/cmd/cgo/out.go#newcode453
src/cmd/cgo/out.go:453: switch unsafe.Sizeof(0) {
This assumes that the cgo binary is built for the same architecture as it is
building for.  Can we do this a different way?  There is already
ptrSizeMap[goarch] for the pointer size.

Separately, perhaps we do not need this section at all.  I do not remember cgo
emitting code that used an unsized int.  What breaks if we just omit the "int"
and "uint" entries from the table?

http://codereview.appspot.com/6131060/diff/1/src/cmd/cgo/out.go#newcode943
src/cmd/cgo/out.go:943: typedef int          GoInt8   __attribute__ ((mode
(QI)));
Issue 3371 is mainly concerned, I believe, with the names of the typedefs, not
their definitions.  Maybe instead of emitting typedefs at all we should change
the names in the map above to say 'unsigned char' instead of 'uchar', etc.

The specific definitions I chose - being careful to avoid long - work on every
32-bit and 64-bit system I am aware of.  Is it necessary to add the attributes? 
That is, is there some system being fixed by this beyond issue 3371?

If we fix the map above to use full names ("unsigned int", "signed char", "long
long" instead of "uint32", "int8", "int64") then all that should be necessary
here is the uintptr, which we can call GoUintptr.

[iant, 2012-05-01 14:05:22]

Before I read the last comment I realized that the attribute stuff is irrelevant
for this issue, so I just dropped it.  Switching to that can be a different CL.

Just using "int", etc., will break when and if we switch to 64-bit int.  Also
there are 64-bit systems that use 64-bit int.  Personally I think that using Go
names makes sense.

[rsc, 2012-05-01 14:12:03]

LGTM

On Tue, May 1, 2012 at 10:05,  <iant@golang.org> wrote:
> Just using "int", etc., will break when and if we switch to 64-bit int.
> Also there are 64-bit systems that use 64-bit int.  Personally I think
> that using Go names makes sense.

I don't understand this comment given the current code, which says

typedef int GoInt32;

but I agree that it can wait until we hit a system with 64-bit 'int' in C.
(If a system does that, how do you get a 32-bit int?  Surely not 'short'?)

Does cgo still work if we delete:
       "int":        {Size: 4, Align: 4, C: c("GoInt")},
       "uint":       {Size: 4, Align: 4, C: c("GoUint")},
from the map?  I suspect it will, because I don't think the cgo
code generator ever chooses to refer to an unsized int.

Russ

[iant2, 2012-05-01 16:00:05]

Russ Cox <rsc@golang.org> writes:

> LGTM
>
> On Tue, May 1, 2012 at 10:05,  <iant@golang.org> wrote:
>> Just using "int", etc., will break when and if we switch to 64-bit int.
>> Also there are 64-bit systems that use 64-bit int.  Personally I think
>> that using Go names makes sense.
>
> I don't understand this comment given the current code, which says
>
> typedef int GoInt32;

I postponed the issue of ensuring that we have the correct type sizes
for a different CL.


> but I agree that it can wait until we hit a system with 64-bit 'int' in C.
> (If a system does that, how do you get a 32-bit int?  Surely not 'short'?)

int32_t, I suppose.  In principle portable C code isn't supposed to
worry about the exact sizes of types.  I don't know of a system on which
short is not 16 bits (except for AVR where in some compilation modes
short and int are both 8 bits).


> Does cgo still work if we delete:
>        "int":        {Size: 4, Align: 4, C: c("GoInt")},
>        "uint":       {Size: 4, Align: 4, C: c("GoUint")},
> from the map?  I suspect it will, because I don't think the cgo
> code generator ever chooses to refer to an unsized int.

No, it fails.  In misc/cgo/test I get

exports.go:10:23: unrecognized Go type int
exports.go:10:23: unrecognized Go type int
issue1560.go:30:24: unrecognized Go type int
issue1560.go:30:24: unrecognized Go type int
issue2462.go:30:18: unrecognized Go type int
issue2462.go:30:18: unrecognized Go type int
issue2462.go:35:19: unrecognized Go type uint
issue2462.go:35:19: unrecognized Go type uint

Ian

[iant, 2012-05-01 16:04:25]

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

cgo: rename C names for Go types to avoid conflicting with package

Fixes issue 3371.

R=rsc, bsiegert, r, mtj, iant
CC=golang-dev
http://codereview.appspot.com/6131060

[rsc, 2012-05-01 16:24:15]

On Tue, May 1, 2012 at 12:00, Ian Lance Taylor <iant@google.com> wrote:
> No, it fails.  In misc/cgo/test I get
>
> exports.go:10:23: unrecognized Go type int
> exports.go:10:23: unrecognized Go type int
> issue1560.go:30:24: unrecognized Go type int
> issue1560.go:30:24: unrecognized Go type int
> issue2462.go:30:18: unrecognized Go type int
> issue2462.go:30:18: unrecognized Go type int
> issue2462.go:35:19: unrecognized Go type uint
> issue2462.go:35:19: unrecognized Go type uint

Thanks.  I created issue 3585 so I remember to come back and understand this.

Russ