code review 87500043: debug/pe: add optional header to File

debug/pe: add optional header to File

This information is required by cmd/nm
to calculate absolute symbol addresses.

Update issue 6936
Update issue 7738

[brainman, 2014-04-14 06:32:17]

Hello golang-codereviews@googlegroups.com,

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

[bradfitz, 2014-04-14 15:21:21]

https://codereview.appspot.com/87500043/diff/40001/src/pkg/debug/pe/file.go
File src/pkg/debug/pe/file.go (right):

https://codereview.appspot.com/87500043/diff/40001/src/pkg/debug/pe/file.go#n...
src/pkg/debug/pe/file.go:22: OptionalHeader interface{}
OptionalHeader interface{} // of type *Foo or *Bar (or Foo or Bar)

https://codereview.appspot.com/87500043/diff/40001/src/pkg/debug/pe/file.go#n...
src/pkg/debug/pe/file.go:203: binary.Read(sr, binary.LittleEndian,
&f.FileHeader)
ignoring error here and all the binary.Read below?

[brainman, 2014-04-14 23:26:28]

https://codereview.appspot.com/87500043/diff/40001/src/pkg/debug/pe/file.go
File src/pkg/debug/pe/file.go (right):

https://codereview.appspot.com/87500043/diff/40001/src/pkg/debug/pe/file.go#n...
src/pkg/debug/pe/file.go:22: OptionalHeader interface{}
On 2014/04/14 15:21:22, bradfitz wrote:
> OptionalHeader interface{} // of type *Foo or *Bar (or Foo or Bar)

Done. But maybe we should define new interface type like

type OptionalHeader interface {
}

and make new field of that type

OptionalHeader OptionalHeader

instead? I don't see any use for it now, but I am worried we won't be able to
extend interface{} in the future.

Alternatively, we can replace "OptionalHeader interface{}" with two fields of
OptionalHeader32 and OptionalHeader64 and some indicator to say which field to
use.

https://codereview.appspot.com/87500043/diff/40001/src/pkg/debug/pe/file.go#n...
src/pkg/debug/pe/file.go:203: binary.Read(sr, binary.LittleEndian,
&f.FileHeader)
On 2014/04/14 15:21:22, bradfitz wrote:
> ignoring error here and all the binary.Read below?

My mistake. Fixed now.

[brainman, 2014-04-14 23:26:41]

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

Please take another look.

[gobot, 2014-04-14 23:30:18]

R=rsc@golang.org (assigned by bradfitz@golang.org)

[rsc, 2014-04-17 02:09:00]

LGTM

[rsc, 2014-04-17 02:11:05]

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

debug/pe: add optional header to File

This information is required by cmd/nm
to calculate absolute symbol addresses.

Update issue 6936
Update issue 7738

LGTM=rsc
R=golang-codereviews, bradfitz, gobot, rsc
CC=golang-codereviews
https://codereview.appspot.com/87500043

Committer: Russ Cox <rsc@golang.org>

[gobot, 2014-04-17 08:36:06]

This CL appears to have broken the linux-386-387 builder.
See http://build.golang.org/log/a2af1626af5806870f376a26efaa05ad2674e857