code review 91360046: objdump: implement disassembly

objdump: implement disassembly

There is some duplication here with cmd/nm.
There is a TODO to address that after 1.3 is out.

Update issue 7452

x86 disassembly works and is tested.

The arm disassembler does not exist yet
and is therefore not yet hooked up.

[rsc, 2014-05-14 18:33:50]

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

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

[crawshaw, 2014-05-14 19:30:02]

LGTM

https://codereview.appspot.com/91360046/diff/40001/src/cmd/objdump/main.go
File src/cmd/objdump/main.go (right):

https://codereview.appspot.com/91360046/diff/40001/src/cmd/objdump/main.go#ne...
src/cmd/objdump/main.go:97: keep := syms[:0]
// reuse memory for filtering

or some similar comment so no-one misses this.

https://codereview.appspot.com/91360046/diff/40001/src/cmd/objdump/main.go#ne...
src/cmd/objdump/main.go:141: func base(file string) string {
add a comment that you are deliberately avoiding filepath.Base because your
binary can work with both kinds of paths

[iant, 2014-05-14 21:33:47]

LGTM

https://codereview.appspot.com/91360046/diff/40001/src/cmd/objdump/main.go
File src/cmd/objdump/main.go (right):

https://codereview.appspot.com/91360046/diff/40001/src/cmd/objdump/main.go#ne...
src/cmd/objdump/main.go:26: // The disassembler is missing
(golang.org/issue/7452) but will be added
This comment about the missing disassembler can presumably go away.

https://codereview.appspot.com/91360046/diff/40001/src/cmd/objdump/main.go#ne...
src/cmd/objdump/main.go:52: var symregexp = flag.String("s", "", "only dump
symbols matching this regexp")
This flag should be mentioned in the doc comment.

https://codereview.appspot.com/91360046/diff/40001/src/cmd/objdump/main.go#ne...
src/cmd/objdump/main.go:56: fmt.Fprintf(os.Stderr, "usage: objdump [-s sym]
binary [start end]\n\n")
To me "sym" implies a symbol name.  Perhaps re or symre or regexp?

https://codereview.appspot.com/91360046/diff/40001/src/cmd/objdump/testdata/f...
File src/cmd/objdump/testdata/fmthello.go (right):

https://codereview.appspot.com/91360046/diff/40001/src/cmd/objdump/testdata/f...
src/cmd/objdump/testdata/fmthello.go:1: package main
What is this file for?

[rsc, 2014-05-14 23:43:17]

https://codereview.appspot.com/91360046/diff/40001/src/cmd/objdump/testdata/f...
File src/cmd/objdump/testdata/fmthello.go (right):

https://codereview.appspot.com/91360046/diff/40001/src/cmd/objdump/testdata/f...
src/cmd/objdump/testdata/fmthello.go:1: package main
On 2014/05/14 21:33:47, iant wrote:
> What is this file for?

It is compiled by TestDisasm to produce a test input.

[rsc, 2014-05-14 23:50:34]

https://codereview.appspot.com/91360046/diff/40001/src/cmd/objdump/main.go
File src/cmd/objdump/main.go (right):

https://codereview.appspot.com/91360046/diff/40001/src/cmd/objdump/main.go#ne...
src/cmd/objdump/main.go:26: // The disassembler is missing
(golang.org/issue/7452) but will be added
On 2014/05/14 21:33:47, iant wrote:
> This comment about the missing disassembler can presumably go away.

Done.

https://codereview.appspot.com/91360046/diff/40001/src/cmd/objdump/main.go#ne...
src/cmd/objdump/main.go:52: var symregexp = flag.String("s", "", "only dump
symbols matching this regexp")
On 2014/05/14 21:33:47, iant wrote:
> This flag should be mentioned in the doc comment.

Done.

https://codereview.appspot.com/91360046/diff/40001/src/cmd/objdump/main.go#ne...
src/cmd/objdump/main.go:56: fmt.Fprintf(os.Stderr, "usage: objdump [-s sym]
binary [start end]\n\n")
On 2014/05/14 21:33:47, iant wrote:
> To me "sym" implies a symbol name.  Perhaps re or symre or regexp?

Done.

https://codereview.appspot.com/91360046/diff/40001/src/cmd/objdump/main.go#ne...
src/cmd/objdump/main.go:97: keep := syms[:0]
On 2014/05/14 19:30:01, crawshaw wrote:
> // reuse memory for filtering
> 
> or some similar comment so no-one misses this.

Done.

https://codereview.appspot.com/91360046/diff/40001/src/cmd/objdump/main.go#ne...
src/cmd/objdump/main.go:141: func base(file string) string {
On 2014/05/14 19:30:01, crawshaw wrote:
> add a comment that you are deliberately avoiding filepath.Base because your
> binary can work with both kinds of paths

Done.

[rsc, 2014-05-14 23:51:07]

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

objdump: implement disassembly

There is some duplication here with cmd/nm.
There is a TODO to address that after 1.3 is out.

Update issue 7452

x86 disassembly works and is tested.

The arm disassembler does not exist yet
and is therefore not yet hooked up.

LGTM=crawshaw, iant
R=crawshaw, iant
CC=golang-codereviews
https://codereview.appspot.com/91360046

[gobot, 2014-05-15 00:00:04]

This CL appears to have broken the freebsd-amd64 builder.
See http://build.golang.org/log/40a55cc6aeb4477cb2f6c227eecca64c833bbcc3

[bradfitz, 2014-05-15 00:02:32]

Real.
On May 14, 2014 5:00 PM, <gobot@golang.org> wrote:

> This CL appears to have broken the freebsd-amd64 builder.
> See http://build.golang.org/log/40a55cc6aeb4477cb2f6c227eecca64c833bbcc3
>
> https://codereview.appspot.com/91360046/
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[brainman, 2014-05-15 01:49:24]

On 2014/05/15 00:02:32, bradfitz wrote:
> Real.

I suspect start of text pe section is wrong. I am investigating.

Alex

[brainman, 2014-05-15 02:42:22]

On 2014/05/15 01:49:24, brainman wrote:
> On 2014/05/15 00:02:32, bradfitz wrote:
> > Real.
> 
> I suspect start of text pe section is wrong. I am investigating.
> 

https://codereview.appspot.com/97500043/

Alex