code review 89470043: cmd/ld: correct addresses in windows pe symbol table

cmd/ld: correct addresses in windows pe symbol table

This should have been part of 36eb4a62fbb6,
but I later discovered that addresses are all wrong.
Appropriate test added now.

[brainman, 2014-04-19 00:30:03]

Hello golang-codereviews@googlegroups.com,

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

[r, 2014-04-20 17:47:28]

https://codereview.appspot.com/89470043/diff/2/src/cmd/nm/nm_test.go
File src/cmd/nm/nm_test.go (right):

https://codereview.appspot.com/89470043/diff/2/src/cmd/nm/nm_test.go#newcode25
src/cmd/nm/nm_test.go:25: s, err := r.ReadString('\n')
why not use bufio.Scanner here? less code.

https://codereview.appspot.com/89470043/diff/2/src/cmd/nm/nm_test.go#newcode50
src/cmd/nm/nm_test.go:50: t.Error("nm does not displays checkSymbols symbol")
s/displays/display/ here and below.
but display is an odd verb for this. not wrong, just odd. i would use 'has',
which is nice and short. or 'shows' if you want to keep the sense but use a more
idiomatic word.

[brainman, 2014-04-21 04:18:44]

Thank you for review.

Alex

https://codereview.appspot.com/89470043/diff/2/src/cmd/nm/nm_test.go
File src/cmd/nm/nm_test.go (right):

https://codereview.appspot.com/89470043/diff/2/src/cmd/nm/nm_test.go#newcode25
src/cmd/nm/nm_test.go:25: s, err := r.ReadString('\n')
On 2014/04/20 17:47:29, r wrote:
> why not use bufio.Scanner here? less code.

Sure. Changed to use bufio.Scanner.

https://codereview.appspot.com/89470043/diff/2/src/cmd/nm/nm_test.go#newcode50
src/cmd/nm/nm_test.go:50: t.Error("nm does not displays checkSymbols symbol")
On 2014/04/20 17:47:29, r wrote:
> s/displays/display/ here and below.
> but display is an odd verb for this. not wrong, just odd. i would use 'has',
> which is nice and short. or 'shows' if you want to keep the sense but use a
more
> idiomatic word.

Sure. Changed to "nm shows no checkSymbols symbol". Same below.

[brainman, 2014-04-21 04:19:02]

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

Please take another look.

[r, 2014-04-21 06:39:13]

LGTM

https://codereview.appspot.com/89470043/diff/40001/src/cmd/nm/nm_test.go
File src/cmd/nm/nm_test.go (right):

https://codereview.appspot.com/89470043/diff/40001/src/cmd/nm/nm_test.go#newc...
src/cmd/nm/nm_test.go:41: t.Errorf("nm displays wrong address %v for testData
(%p)", addr, &testData)
for consistency, s/displays/shows/ here and above

[brainman, 2014-04-21 09:28:11]

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

cmd/ld: correct addresses in windows pe symbol table

This should have been part of 36eb4a62fbb6,
but I later discovered that addresses are all wrong.
Appropriate test added now.

LGTM=r
R=golang-codereviews, r
CC=golang-codereviews
https://codereview.appspot.com/89470043

https://codereview.appspot.com/89470043/diff/40001/src/cmd/nm/nm_test.go
File src/cmd/nm/nm_test.go (right):

https://codereview.appspot.com/89470043/diff/40001/src/cmd/nm/nm_test.go#newc...
src/cmd/nm/nm_test.go:41: t.Errorf("nm displays wrong address %v for testData
(%p)", addr, &testData)
On 2014/04/21 06:39:13, r wrote:
> for consistency, s/displays/shows/ here and above

Done.

[gobot, 2014-04-21 09:30:50]

This CL appears to have broken the linux-amd64-race builder.
See http://build.golang.org/log/5d2e8d909dc6383ab613ebdcf90ce230cd39320e

[brainman, 2014-04-21 09:37:15]

On 2014/04/21 09:30:50, gobot wrote:
> This CL appears to have broken the linux-amd64-race builder.
> See http://build.golang.org/log/5d2e8d909dc6383ab613ebdcf90ce230cd39320e

I tested this on windows/amd64 and linux/386 - it succeeds there.

I guess, some symbol lines are different:

--- FAIL: TestNM (0.87 seconds)
	nm_test.go:27: nm must have at least 3 columns
	nm_test.go:27: nm must have at least 3 columns
	nm_test.go:27: nm must have at least 3 columns
	nm_test.go:27: nm must have at least 3 columns
	nm_test.go:27: nm must have at least 3 columns
	nm_test.go:27: nm must have at least 3 columns
	nm_test.go:27: nm must have at least 3 columns
	nm_test.go:27: nm must have at least 3 columns

I will change test not to fail, if line is irregular. Hopefully this fixes
builders.

Alex

[brainman, 2014-04-21 10:15:43]

On 2014/04/21 09:37:15, brainman wrote:
> On 2014/04/21 09:30:50, gobot wrote:
> > This CL appears to have broken the linux-amd64-race builder.
> > See http://build.golang.org/log/5d2e8d909dc6383ab613ebdcf90ce230cd39320e
> 

I will disable new test on some systems:
https://codereview.appspot.com/89810043/.

Alex