code review 5367043: godefs: delete, replaced by cgo -godefs

godefs: delete, replaced by cgo -godefs

Godefs was a C program that ran gcc and then parsed the
stabs debugging information in the resulting object file to
generate C or Go code for bootstrapping as part of
package runtime or package syscall.

Cgo does the same work, but using the dwarf debugging
information.  Add -godefs and -cdefs options to cgo that
mimic godefs's output, albeit with different input
(a Go program, not a C program).

This has been a "nice to have" for a while but was forced
by Apple removing stabs debugging output from their
latest compilers.

Fixes issue 835.
Fixes issue 2338.

[rsc, 2011-11-09 05:25:14]

Hello golang-dev@googlegroups.com,

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

[bradfitz, 2011-11-09 05:34:25]

http://codereview.appspot.com/5367043/diff/2002/src/cmd/cgo/gcc.go
File src/cmd/cgo/gcc.go (right):

http://codereview.appspot.com/5367043/diff/2002/src/cmd/cgo/gcc.go#newcode354
src/cmd/cgo/gcc.go:354: // Turn decimal into hex, just for consistency
You may want to skip this for zero.

Your other CL's diff had a lot of 0 turning into longer 0x0.

*shrug*

[rsc, 2011-11-09 05:39:37]

On Wed, Nov 9, 2011 at 00:34,  <bradfitz@golang.org> wrote:
> Your other CL's diff had a lot of 0 turning into longer 0x0.

Nah, that's just a bug in ANSI C.  Glad to see it fixed.

$ printf '%#x\n' 1
0x1
$ printf '%#x\n' 0
0
$

[r, 2011-11-09 05:49:04]

i haven't absorbed all this yet

http://codereview.appspot.com/5367043/diff/2002/src/cmd/cgo/gcc.go
File src/cmd/cgo/gcc.go (right):

http://codereview.appspot.com/5367043/diff/2002/src/cmd/cgo/gcc.go#newcode27
src/cmd/cgo/gcc.go:27: "utf8"
these aren't right any more

http://codereview.appspot.com/5367043/diff/2002/src/cmd/cgo/gcc.go#newcode1448
src/cmd/cgo/gcc.go:1448: // Rewrite field names for use in godefs.
what happened to

godefsFields rewrites...

http://codereview.appspot.com/5367043/diff/2002/src/cmd/cgo/gcc.go#newcode1484
src/cmd/cgo/gcc.go:1484: func fieldPrefix(fld []*ast.Field) string {
comment
i don't even see why c does nothing and go does magic here

http://codereview.appspot.com/5367043/diff/2002/src/cmd/cgo/godefs.go
File src/cmd/cgo/godefs.go (right):

http://codereview.appspot.com/5367043/diff/2002/src/cmd/cgo/godefs.go#newcode29
src/cmd/cgo/godefs.go:29: // like in_addr becomes [4]byte.
this is not english grammar.
perhaps

such as in_addr becoming [4]byte.

even better would be to put an example into the comment because the code is
intricate and the format hard to discern

http://codereview.appspot.com/5367043/diff/2002/src/cmd/cgo/godefs.go#newcode92
src/cmd/cgo/godefs.go:92: // [4]byte Go types in the typedef map.  Substitute
those.
looks like this loop only does unions; the comment implies a more general
rewriting.

http://codereview.appspot.com/5367043/diff/2002/src/cmd/cgo/godefs.go#newcode109
src/cmd/cgo/godefs.go:109: godefsOutput := p.godefs(f, srcfile)
arguably this functionality should be in cdefs.go

http://codereview.appspot.com/5367043/diff/2002/src/cmd/cgo/godefs.go#newcode122
src/cmd/cgo/godefs.go:122: // Delete package x.
s/package x/"&"/
or indent it as you do in the next example

http://codereview.appspot.com/5367043/diff/2002/src/cmd/cgo/godefs.go#newcode144
src/cmd/cgo/godefs.go:144: if inConst && line == ")" {
all these booleans are messy, but perhaps it's the simplest approach.

also you could fold some of these together and make it a switch statement
(actually two with an interlude), with some improvement in structure. all those
continues...

http://codereview.appspot.com/5367043/diff/2002/src/cmd/cgo/godefs.go#newcode157
src/cmd/cgo/godefs.go:157: // Convert const A = 1 to enum { A = 1 };
again, unfold

http://codereview.appspot.com/5367043/diff/2002/src/cmd/cgo/godefs.go#newcode174
src/cmd/cgo/godefs.go:174: fmt.Fprintf(&out, "#pragma pack on\n")
where did this come from?

http://codereview.appspot.com/5367043/diff/2002/src/cmd/cgo/godefs.go#newcode207
src/cmd/cgo/godefs.go:207: fmt.Fprintf(os.Stderr, "cannot parse struct field:
%s\n", line)
throughout: it would be nice if these errors had "cgo: " at the front. maybe in
another CL.

also you could lighten the ink load by making printf a method of *Package. maybe
error too

http://codereview.appspot.com/5367043/diff/2002/src/cmd/cgo/godefs.go#newcode265
src/cmd/cgo/godefs.go:265: func gofmt(n interface{}) string {
peculiar function deserves a comment

http://codereview.appspot.com/5367043/diff/2002/src/cmd/cgo/main.go
File src/cmd/cgo/main.go (right):

http://codereview.appspot.com/5367043/diff/2002/src/cmd/cgo/main.go#newcode245
src/cmd/cgo/main.go:245: // newPackage returns a new package that will invoke
s/package/Package/

http://codereview.appspot.com/5367043/diff/2002/src/cmd/cgo/main.go#newcode248
src/cmd/cgo/main.go:248: // Copy it to a new slice so it can grow.
s/it/args/
s/it/the list/

http://codereview.appspot.com/5367043/diff/2002/src/cmd/cgo/main.go#newcode265
src/cmd/cgo/main.go:265: // Clear locale variables so gcc emits English errors
[sic].
s/Clear/Reset/

[rsc, 2011-11-09 05:53:44]

On Wed, Nov 9, 2011 at 00:49,  <r@golang.org> wrote:
> src/cmd/cgo/godefs.go:174: fmt.Fprintf(&out, "#pragma pack on\n")
> where did this come from?

i'm just matching what godefs emits now.
#pragma pack is the former #pragma hjdicks.

[r, 2011-11-09 05:54:45]

yay! hjdicks.

i'm all teary-eyed.

-rob

[dave_cheney.net, 2011-11-09 06:00:05]

This looks pretty awesome, but where does it leave Linux/arm. Will we have to
regenerate the syscall z-files under qemu, or in some cross compiling
environment?

Sent from my iPhone

On 09/11/2011, at 16:25, rsc@golang.org wrote:

> Reviewers: golang-dev_googlegroups.com,
> 
> Message:
> Hello golang-dev@googlegroups.com,
> 
> I'd like you to review this change to
> https://go.googlecode.com/hg/
> 
> 
> Description:
> godefs: delete, replaced by cgo -godefs
> 
> Godefs was a C program that ran gcc and then parsed the
> stabs debugging information in the resulting object file to
> generate C or Go code for bootstrapping as part of
> package runtime or package syscall.
> 
> Cgo does the same work, but using the dwarf debugging
> information.  Add -godefs and -cdefs options to cgo that
> mimic godefs's output, albeit with different input
> (a Go program, not a C program).
> 
> This has been a "nice to have" for a while but was forced
> by Apple removing stabs debugging output from their
> latest compilers.
> 
> Fixes issue 835.
> Fixes issue 2338.
> 
> Please review this at http://codereview.appspot.com/5367043/
> 
> Affected files:
>  M src/cmd/Makefile
>  M src/cmd/cgo/Makefile
>  M src/cmd/cgo/ast.go
>  M src/cmd/cgo/gcc.go
>  A src/cmd/cgo/godefs.go
>  M src/cmd/cgo/main.go
>  M src/cmd/cgo/out.go
>  R src/cmd/godefs/Makefile
>  R src/cmd/godefs/a.h
>  R src/cmd/godefs/doc.go
>  R src/cmd/godefs/main.c
>  R src/cmd/godefs/stabs.c
>  R src/cmd/godefs/test.sh
>  R src/cmd/godefs/testdata.c
>  R src/cmd/godefs/testdata_darwin_386.golden
>  R src/cmd/godefs/testdata_darwin_amd64.golden
>  R src/cmd/godefs/testdata_linux_arm.golden
>  R src/cmd/godefs/util.c
> 
> 

[rsc, 2011-11-09 15:14:39]

On Wed, Nov 9, 2011 at 00:59, Dave Cheney <dave@cheney.net> wrote:
> This looks pretty awesome, but where does it leave Linux/arm.
> Will we have to regenerate the syscall z-files under qemu, or
> in some cross compiling environment?

It should be no different than generating with godefs.
The easiest way to regenerate the files is to patch this CL and
the other one into a standard Debian ARM installation.
That's how they were generated the first time.  If no one else
does this I plan to do it once these CLs are checked in.

Russ

[iant, 2011-11-09 16:09:38]

FYI

http://codereview.appspot.com/5367043/diff/2002/src/cmd/cgo/godefs.go
File src/cmd/cgo/godefs.go (right):

http://codereview.appspot.com/5367043/diff/2002/src/cmd/cgo/godefs.go#newcode163
src/cmd/cgo/godefs.go:163: // On first type definition, emit all the typedefs
s/emit all the typedefs/typedef all the structs/

http://codereview.appspot.com/5367043/diff/2002/src/cmd/cgo/main.go
File src/cmd/cgo/main.go (right):

http://codereview.appspot.com/5367043/diff/2002/src/cmd/cgo/main.go#newcode267
src/cmd/cgo/main.go:267: os.Setenv("LC_ALL", "C")
There is no particular reason to set both LC_ALL and LANG.  If LC_ALL is set,
LANG is ignored.

[rsc, 2011-11-09 20:36:02]

PTAL

I have addressed Rob's and Ian's comments.

[r, 2011-11-09 21:27:41]

LGTM
the comments definitely help

http://codereview.appspot.com/5367043/diff/12003/src/cmd/cgo/gcc.go
File src/cmd/cgo/gcc.go (right):

http://codereview.appspot.com/5367043/diff/12003/src/cmd/cgo/gcc.go#newcode1448
src/cmd/cgo/gcc.go:1448: // godefsFields rewrites field names for use in godefs.
"godefs" is a flag, but otherwise it's a mere echo of an old name. maybe "Go
definitions"?

http://codereview.appspot.com/5367043/diff/12003/src/cmd/cgo/gcc.go#newcode1461
src/cmd/cgo/gcc.go:1461: n.Name = "Pad_godefs_" + strconv.Itoa(npad)
shouldn't this be Pad_cgo_?

http://codereview.appspot.com/5367043/diff/12003/src/cmd/cgo/gcc.go#newcode1478
src/cmd/cgo/gcc.go:1478: *p = ast.NewIdent("*byte")
this seems surprising.

http://codereview.appspot.com/5367043/diff/12003/src/cmd/cgo/godefs.go
File src/cmd/cgo/godefs.go (right):

http://codereview.appspot.com/5367043/diff/12003/src/cmd/cgo/godefs.go#newcod...
src/cmd/cgo/godefs.go:120: func (p *Package) cdefs(f *File, srcfile string)
string {
i'd still like to see the c stuff separated from the go stuff and put in another
file, since "godefs" is really just the old name of a now-deleted program, but i
won't fight for it.

[iant, 2011-11-09 21:43:42]

LGTM

http://codereview.appspot.com/5367043/diff/12003/src/cmd/cgo/gcc.go
File src/cmd/cgo/gcc.go (right):

http://codereview.appspot.com/5367043/diff/12003/src/cmd/cgo/gcc.go#newcode616
src/cmd/cgo/gcc.go:616: // the xxx.  In *godefs mode, rewriteRef replaces the
names with
s/*godefs/*godefs or *cdefs/

[rsc, 2011-11-09 22:18:16]

On Wed, Nov 9, 2011 at 16:27,  <r@golang.org> wrote:
> i'd still like to see the c stuff separated from the go stuff and put in
> another file, since "godefs" is really just the old name of a
> now-deleted program, but i won't fight for it.

Sorry, I meant to reply to this comment in the first round.
The godefs + cdefs code are really two different variants
of the same tool, depending in practice whether we're
generating code for package syscall or package runtime.
I think it makes sense to keep them in the same file
because they are so closely related.

Russ

[rsc, 2011-11-11 00:08:10]

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

godefs: delete, replaced by cgo -godefs

Godefs was a C program that ran gcc and then parsed the
stabs debugging information in the resulting object file to
generate C or Go code for bootstrapping as part of
package runtime or package syscall.

Cgo does the same work, but using the dwarf debugging
information.  Add -godefs and -cdefs options to cgo that
mimic godefs's output, albeit with different input
(a Go program, not a C program).

This has been a "nice to have" for a while but was forced
by Apple removing stabs debugging output from their
latest compilers.

Fixes issue 835.
Fixes issue 2338.

R=golang-dev, bradfitz, r, dave, iant
CC=golang-dev
http://codereview.appspot.com/5367043