code review 49970044: debug/plan9obj: implement parsing of Plan 9 a.out execu...

debug/plan9obj: implement parsing of Plan 9 a.out executables

It implements parsing of the header and symbol table for both
32-bit and 64-bit Plan 9 binaries. The nm tool was updated to
use this package.

[0intro, 2014-01-09 21:14:51]

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

I'd like you to review this change to
https://code.google.com/p/go

[0intro, 2014-01-09 21:15:24]

This is a first draft, please take a look.

I've tried to keep the structure of the code and interface
close to debug/elf. The parsing of the symbol table is done
by the functions walksymtab and NewTable, derived from
debug/gosym.

[aram, 2014-01-11 22:33:14]

Thank you for working on this.

A superficial review (no check of functional correctness) follows.

https://codereview.appspot.com/49970044/diff/100001/src/cmd/nm/plan9obj.go
File src/cmd/nm/plan9obj.go (right):

https://codereview.appspot.com/49970044/diff/100001/src/cmd/nm/plan9obj.go#ne...
src/cmd/nm/plan9obj.go:29: // We infer the size of a symbol by looking at where
the next symbol begins.
There's no size calculation here. It's better to move this comment down.

https://codereview.appspot.com/49970044/diff/100001/src/cmd/nm/plan9obj.go#ne...
src/cmd/nm/plan9obj.go:40: i := sort.Search(len(addrs), func(x int) bool {
return addrs[x] > s.Value })
Looks inefficient.

I'm not concerned with efficiency much (this is most likely very fast), but the
better-performing alternative is just as short, and probably clearer.

https://codereview.appspot.com/49970044/diff/100001/src/cmd/nm/plan9obj.go#ne...
src/cmd/nm/plan9obj.go:43: }
Is it ok not to have an else here?

https://codereview.appspot.com/49970044/diff/100001/src/pkg/debug/plan9obj/fi...
File src/pkg/debug/plan9obj/file.go (right):

https://codereview.appspot.com/49970044/diff/100001/src/pkg/debug/plan9obj/fi...
src/pkg/debug/plan9obj/file.go:40: 
No newline (or newline after every header).

https://codereview.appspot.com/49970044/diff/100001/src/pkg/debug/plan9obj/fi...
src/pkg/debug/plan9obj/file.go:110: // Open opens the named file using os.Open
and prepares it for use as an Plan 9 a.out binary.
Mentioning os.Open seems superfluous and it's bewildering.

https://codereview.appspot.com/49970044/diff/100001/src/pkg/debug/plan9obj/fi...
src/pkg/debug/plan9obj/file.go:125: // Close closes the File.
Close closes the File. Subsequent operations on the file will ...

https://codereview.appspot.com/49970044/diff/100001/src/pkg/debug/plan9obj/fi...
src/pkg/debug/plan9obj/file.go:129: var err error
If you make the error a named parameter, you save a line without sacrificing
clarity.

https://codereview.appspot.com/49970044/diff/100001/src/pkg/debug/plan9obj/fi...
src/pkg/debug/plan9obj/file.go:148: func NewFile(r io.ReaderAt) (*File, error) {
Does this function need to be exported?

https://codereview.appspot.com/49970044/diff/100001/src/pkg/debug/plan9obj/fi...
src/pkg/debug/plan9obj/file.go:160: f := new(File)
Declare and initialise with a literal?

https://codereview.appspot.com/49970044/diff/100001/src/pkg/debug/plan9obj/fi...
src/pkg/debug/plan9obj/file.go:169: 
No whitespace.

https://codereview.appspot.com/49970044/diff/100001/src/pkg/debug/plan9obj/fi...
src/pkg/debug/plan9obj/file.go:216: }
In general, there is too much white space, but this function is one of the worst
offenders.

https://codereview.appspot.com/49970044/diff/100001/src/pkg/debug/plan9obj/fi...
src/pkg/debug/plan9obj/file.go:219: var order binary.ByteOrder =
binary.BigEndian
No need to specify the type here.

Also, for use in just two instances it seems hardly necessary.

https://codereview.appspot.com/49970044/diff/100001/src/pkg/debug/plan9obj/fi...
src/pkg/debug/plan9obj/file.go:272: // NewTable decodes the Go symbol table in
data,
Who is data?

https://codereview.appspot.com/49970044/diff/100001/src/pkg/debug/plan9obj/fi...
src/pkg/debug/plan9obj/file.go:275: var n int
n := 0

https://codereview.appspot.com/49970044/diff/100001/src/pkg/debug/plan9obj/fi...
src/pkg/debug/plan9obj/file.go:286: err = walksymtab(symtab, ptrsz, func(s sym)
error {
Why do we need to walk this twice? Is it possible to rewrite so that we don't
need n upfront? It is possible to get n from somewhere else?

https://codereview.appspot.com/49970044/diff/100001/src/pkg/debug/plan9obj/fi...
File src/pkg/debug/plan9obj/file_test.go (right):

https://codereview.appspot.com/49970044/diff/100001/src/pkg/debug/plan9obj/fi...
src/pkg/debug/plan9obj/file_test.go:40: },
arm?

[0intro, 2014-01-12 19:40:11]

Most of your comments make sense, but I've tried to let the code as close as
possible to nm/macho.go, debug/elf/file.go and debug/gosym/symtab.go.

I'm in favor of fixing them, but that would be a departure from homogeneity.

Speaking about homogeneity, I've noticed that the elf and macho/pe packages use
a different interface to access the symbol table. The elf package returns the
symbol array with a Symbols() method, while the macho/pe package return it as
part of the File structure.

What would be the preferred way?

https://codereview.appspot.com/49970044/diff/100001/src/cmd/nm/plan9obj.go
File src/cmd/nm/plan9obj.go (right):

https://codereview.appspot.com/49970044/diff/100001/src/cmd/nm/plan9obj.go#ne...
src/cmd/nm/plan9obj.go:29: // We infer the size of a symbol by looking at where
the next symbol begins.
> There's no size calculation here. It's better to move this comment down.

Same as src/cmd/nm/macho.go.

https://codereview.appspot.com/49970044/diff/100001/src/cmd/nm/plan9obj.go#ne...
src/cmd/nm/plan9obj.go:40: i := sort.Search(len(addrs), func(x int) bool {
return addrs[x] > s.Value })
> Looks inefficient.
> 
> I'm not concerned with efficiency much (this is most likely very fast), but
the
> better-performing alternative is just as short, and probably clearer.

Same as src/cmd/nm/macho.go.

https://codereview.appspot.com/49970044/diff/100001/src/cmd/nm/plan9obj.go#ne...
src/cmd/nm/plan9obj.go:43: }
> Is it ok not to have an else here?

Yes, sym.Size would be zero in this case.

https://codereview.appspot.com/49970044/diff/100001/src/pkg/debug/plan9obj/fi...
File src/pkg/debug/plan9obj/file.go (right):

https://codereview.appspot.com/49970044/diff/100001/src/pkg/debug/plan9obj/fi...
src/pkg/debug/plan9obj/file.go:40: 
> No newline (or newline after every header).

Forgot the comment from src/pkg/debug/elf/file.go. Done.

https://codereview.appspot.com/49970044/diff/100001/src/pkg/debug/plan9obj/fi...
src/pkg/debug/plan9obj/file.go:110: // Open opens the named file using os.Open
and prepares it for use as an Plan 9 a.out binary.
> Mentioning os.Open seems superfluous and it's bewildering.

Same as src/pkg/debug/elf/file.go.

https://codereview.appspot.com/49970044/diff/100001/src/pkg/debug/plan9obj/fi...
src/pkg/debug/plan9obj/file.go:125: // Close closes the File.
> Close closes the File. Subsequent operations on the file will ...

Same as src/pkg/debug/elf/file.go.

https://codereview.appspot.com/49970044/diff/100001/src/pkg/debug/plan9obj/fi...
src/pkg/debug/plan9obj/file.go:129: var err error
> If you make the error a named parameter, you save a line without sacrificing
> clarity.

Same as src/pkg/debug/elf/file.go.

https://codereview.appspot.com/49970044/diff/100001/src/pkg/debug/plan9obj/fi...
src/pkg/debug/plan9obj/file.go:148: func NewFile(r io.ReaderAt) (*File, error) {
> Does this function need to be exported?

Yes, this is the main interface.

https://codereview.appspot.com/49970044/diff/100001/src/pkg/debug/plan9obj/fi...
src/pkg/debug/plan9obj/file.go:160: f := new(File)
> Declare and initialise with a literal?

Done.

https://codereview.appspot.com/49970044/diff/100001/src/pkg/debug/plan9obj/fi...
src/pkg/debug/plan9obj/file.go:169: 
On 2014/01/11 22:33:14, aram wrote:
> No whitespace.

Done.

https://codereview.appspot.com/49970044/diff/100001/src/pkg/debug/plan9obj/fi...
src/pkg/debug/plan9obj/file.go:216: }
> In general, there is too much white space, but this function is one of the
worst
> offenders.

Same as src/pkg/debug/gosym/symtab.go.

https://codereview.appspot.com/49970044/diff/100001/src/pkg/debug/plan9obj/fi...
src/pkg/debug/plan9obj/file.go:219: var order binary.ByteOrder =
binary.BigEndian
> No need to specify the type here.
> 
> Also, for use in just two instances it seems hardly necessary.

Same as src/pkg/debug/gosym/symtab.go.

https://codereview.appspot.com/49970044/diff/100001/src/pkg/debug/plan9obj/fi...
src/pkg/debug/plan9obj/file.go:272: // NewTable decodes the Go symbol table in
data,
> Who is data?

Same as src/pkg/debug/gosym/symtab.go.

https://codereview.appspot.com/49970044/diff/100001/src/pkg/debug/plan9obj/fi...
src/pkg/debug/plan9obj/file.go:275: var n int
> n := 0

Same as src/pkg/debug/gosym/symtab.go.

https://codereview.appspot.com/49970044/diff/100001/src/pkg/debug/plan9obj/fi...
src/pkg/debug/plan9obj/file.go:286: err = walksymtab(symtab, ptrsz, func(s sym)
error {
> Why do we need to walk this twice? Is it possible to rewrite so that we don't
> need n upfront? It is possible to get n from somewhere else?

We have to walk the symtab twice to be able to allocate the syms array to the
right size. We already know the symtab size, but not the number of elements,
because the names are nil-terminated strings of variable length.

https://codereview.appspot.com/49970044/diff/100001/src/pkg/debug/plan9obj/fi...
File src/pkg/debug/plan9obj/file_test.go (right):

https://codereview.appspot.com/49970044/diff/100001/src/pkg/debug/plan9obj/fi...
src/pkg/debug/plan9obj/file_test.go:40: },
> arm?

I was hesitant to add tests for ARM and MIPS executables, but it doesn't seem
necessary, since their format is identical to 386 binaries. Testing with both
32-bit and 64-bit executables seems sufficient to me.

[rsc, 2014-01-22 21:32:07]

thanks very much for working on this.

https://codereview.appspot.com/49970044/diff/140001/src/pkg/debug/plan9obj/fi...
File src/pkg/debug/plan9obj/file.go (right):

https://codereview.appspot.com/49970044/diff/140001/src/pkg/debug/plan9obj/fi...
src/pkg/debug/plan9obj/file.go:17: * Internal Plan 9 a.out representation
delete comment (use // if you need it, but you don't need it)

https://codereview.appspot.com/49970044/diff/140001/src/pkg/debug/plan9obj/fi...
src/pkg/debug/plan9obj/file.go:94: Type  byte
maybe make this a rune to suggest that it is printable?

https://codereview.appspot.com/49970044/diff/140001/src/pkg/debug/plan9obj/fi...
src/pkg/debug/plan9obj/file.go:279: func NewTable(symtab []byte, ptrsz int)
([]Sym, error) {
I don't think this needs to be exported. s/New/new/

https://codereview.appspot.com/49970044/diff/140001/src/pkg/debug/plan9obj/fi...
src/pkg/debug/plan9obj/file.go:326: func (f *File) Symbols() ([]Sym, error) {
// Symbols returns the symbol table for f.

https://codereview.appspot.com/49970044/diff/140001/src/pkg/debug/plan9obj/pl...
File src/pkg/debug/plan9obj/plan9obj.go (right):

https://codereview.appspot.com/49970044/diff/140001/src/pkg/debug/plan9obj/pl...
src/pkg/debug/plan9obj/plan9obj.go:37: HDR_MAGIC = 0x00008000 /* header
expansion */
Please unexported all these constants until we know they are needed. Put a _ at
the beginning of each (_HDR_MAGIC, _A_MAGIC, ...)

[0intro, 2014-01-22 21:51:00]

Hello rsc@golang.org, aram@mgk.ro (cc: golang-codereviews@googlegroups.com),

Please take another look.

[rsc, 2014-01-22 21:58:59]

LGTM

It looks like you lost the compiled testdata binaries from the CL. Please add
them back before submitting.

[0intro, 2014-01-22 22:20:57]

> It looks like you lost the compiled testdata binaries from the CL. Please add
> them back before submitting.

Well spotted! Fixed, thanks.

[0intro, 2014-01-22 22:31:05]

*** Submitted as https://code.google.com/p/go/source/detail?r=931cc1aeb3be ***

debug/plan9obj: implement parsing of Plan 9 a.out executables

It implements parsing of the header and symbol table for both
32-bit and 64-bit Plan 9 binaries. The nm tool was updated to
use this package.

R=rsc, aram
CC=golang-codereviews
https://codereview.appspot.com/49970044