code review 149400043: cmd/objdump: use cmd/internal/objfile

cmd/objdump: use cmd/internal/objfile

This removes a bunch of ugly duplicate code.
The end goal is to factor the disassembly code
into cmd/internal/objfile too, so that pprof can use it,
but one step at a time.

[rsc, 2014-10-29 03:57:32]

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

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

[r, 2014-10-29 04:04:35]

LGTM

[brainman, 2014-10-29 06:04:40]

https://codereview.appspot.com/149400043/diff/60001/src/cmd/internal/objfile/...
File src/cmd/internal/objfile/pe.go (right):

https://codereview.appspot.com/149400043/diff/60001/src/cmd/internal/objfile/...
src/cmd/internal/objfile/pe.go:193: // Look in symbol table for telltale rt0
symbol.
f.pe.OptionalHeader.(type) should tell you.

[rsc, 2014-10-29 14:04:08]

https://codereview.appspot.com/149400043/diff/60001/src/cmd/internal/objfile/...
File src/cmd/internal/objfile/pe.go (right):

https://codereview.appspot.com/149400043/diff/60001/src/cmd/internal/objfile/...
src/cmd/internal/objfile/pe.go:193: // Look in symbol table for telltale rt0
symbol.
On 2014/10/29 06:04:40, brainman wrote:
> f.pe.OptionalHeader.(type) should tell you.

I looked at that but wasn't sure.
What if the optional header is not present?
Is it guaranteed that 64-bit binaries don't have 32-bit headers?
What about other architectures? Is PE x86-only?
Is there really no architecture bit somewhere?

[iant, 2014-10-29 15:15:59]

https://codereview.appspot.com/149400043/diff/60001/src/cmd/internal/objfile/...
File src/cmd/internal/objfile/pe.go (right):

https://codereview.appspot.com/149400043/diff/60001/src/cmd/internal/objfile/...
src/cmd/internal/objfile/pe.go:193: // Look in symbol table for telltale rt0
symbol.
On 2014/10/29 14:04:08, rsc wrote:
> On 2014/10/29 06:04:40, brainman wrote:
> > f.pe.OptionalHeader.(type) should tell you.
> 
> I looked at that but wasn't sure.
> What if the optional header is not present?
> Is it guaranteed that 64-bit binaries don't have 32-bit headers?
> What about other architectures? Is PE x86-only?
> Is there really no architecture bit somewhere?

The Machine field in the FileHeader tells you the architecture.  The values are
those listed as IMAGE_FILE_MACHINE_xxx in debug/pe/pe.go.

[rsc, 2014-10-29 15:17:36]

Thanks. I don't know why I went looking on the internet instead of reading
our own code.

[iant, 2014-10-29 15:21:44]

LGTM

[rsc, 2014-10-29 22:07:30]

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

cmd/objdump: use cmd/internal/objfile

This removes a bunch of ugly duplicate code.
The end goal is to factor the disassembly code
into cmd/internal/objfile too, so that pprof can use it,
but one step at a time.

LGTM=r, iant
R=r, alex.brainman, iant
CC=golang-codereviews
https://codereview.appspot.com/149400043

[brainman, 2014-10-29 22:34:49]

On 2014/10/29 14:04:08, rsc wrote:
> What if the optional header is not present?

I don't think it is possible. At least for executables. All important fields
(AddressOfEntryPoint, BaseOfCode and many others) live there.

> Is it guaranteed that 64-bit binaries don't have 32-bit headers?

Yes. 64-bit binaries have PE64_IMAGE_OPTIONAL_HEADER, and 32-bit have
IMAGE_OPTIONAL_HEADER. Both have Magic as first field of the structure, and the
value of Magic determines what is to follow.

> What about other architectures? Is PE x86-only?

I don't know. But I have never used (seen) anything but x86 versions of windows.

> Is there really no architecture bit somewhere?

PE64_IMAGE_OPTIONAL_HEADER / IMAGE_OPTIONAL_HEADER Magic would be my choice for
distinguishing 386 and amd64. But, like Ian said, IMAGE_FILE_HEADER.Machine
should do the trick too.

BTW, you decided to leave your (*peFile).goarch as is - you are still looking
for _rt0_386_windows or _rt0_amd64_windows symbol. But it won't work for
anything but Go binaries. Even for Go binaries, it will break, if you decide to
rename these symbols. But, I guess, it is OK for now.

Alex

[gobot, 2014-10-29 22:57:09]

This CL appears to have broken the linux-arm-arm5 builder.
See http://build.golang.org/log/2da7b7893978086ae426911bccfdee9a018531eb

[rsc, 2014-10-30 00:07:46]

I'll follow up with a CL to do what Ian suggested.