code review 96960043: cmd/addr2line: works with windows pe executables now

cmd/addr2line: works with windows pe executables now

Update issue 7406
Fixes issue 7899

[brainman, 2014-05-02 07:40:30]

Hello golang-codereviews@googlegroups.com,

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

[bradfitz, 2014-05-02 17:31:20]

R=rsc

[brainman, 2014-05-05 06:28:35]

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

Please take another look.

[bradfitz, 2014-05-05 13:25:43]

https://codereview.appspot.com/96960043/diff/60001/src/cmd/addr2line/addr2lin...
File src/cmd/addr2line/addr2line_test.go (right):

https://codereview.appspot.com/96960043/diff/60001/src/cmd/addr2line/addr2lin...
src/cmd/addr2line/addr2line_test.go:24: scanner :=
bufio.NewScanner(bytes.NewBuffer(out))
bytes.NewReader is lighter and conveys intent better

https://codereview.appspot.com/96960043/diff/60001/src/cmd/addr2line/addr2lin...
src/cmd/addr2line/addr2line_test.go:39: cmd :=
exec.Command("./testaddr2line.exe", exepath)
.exe? this needs to work on non-Windows too, no? I see no +build lines.

https://codereview.appspot.com/96960043/diff/60001/src/cmd/addr2line/main.go
File src/cmd/addr2line/main.go (right):

https://codereview.appspot.com/96960043/diff/60001/src/cmd/addr2line/main.go#...
src/cmd/addr2line/main.go:156: func findPeSymbol(f *pe.File, name string)
(*pe.Symbol, error) {
findPESymbol ---
https://code.google.com/p/go-wiki/wiki/CodeReviewComments#Initialisms

And also because you have loadPETable below... stay consistent.

https://codereview.appspot.com/96960043/diff/60001/src/cmd/addr2line/main.go#...
src/cmd/addr2line/main.go:165: return nil, fmt.Errorf("symbol %s: section number
%d is large then max %d", name, s.SectionNumber, len(f.Sections))
two typos: large->larger then->than

[brainman, 2014-05-06 02:31:24]

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

Please take another look.

[brainman, 2014-05-06 02:31:50]

https://codereview.appspot.com/96960043/diff/60001/src/cmd/addr2line/addr2lin...
File src/cmd/addr2line/addr2line_test.go (right):

https://codereview.appspot.com/96960043/diff/60001/src/cmd/addr2line/addr2lin...
src/cmd/addr2line/addr2line_test.go:24: scanner :=
bufio.NewScanner(bytes.NewBuffer(out))
On 2014/05/05 13:25:43, bradfitz wrote:
> bytes.NewReader is lighter and conveys intent better

Done.

https://codereview.appspot.com/96960043/diff/60001/src/cmd/addr2line/addr2lin...
src/cmd/addr2line/addr2line_test.go:39: cmd :=
exec.Command("./testaddr2line.exe", exepath)
On 2014/05/05 13:25:43, bradfitz wrote:
> .exe? this needs to work on non-Windows too, no? I see no +build lines.

.exe is need for Windows. UNIX does not care what the executable name is. This
test runs fine on Linux here.

https://codereview.appspot.com/96960043/diff/60001/src/cmd/addr2line/main.go
File src/cmd/addr2line/main.go (right):

https://codereview.appspot.com/96960043/diff/60001/src/cmd/addr2line/main.go#...
src/cmd/addr2line/main.go:156: func findPeSymbol(f *pe.File, name string)
(*pe.Symbol, error) {
On 2014/05/05 13:25:43, bradfitz wrote:
> findPESymbol ---
> https://code.google.com/p/go-wiki/wiki/CodeReviewComments#Initialisms
> 
> And also because you have loadPETable below... stay consistent.

Done.

https://codereview.appspot.com/96960043/diff/60001/src/cmd/addr2line/main.go#...
src/cmd/addr2line/main.go:165: return nil, fmt.Errorf("symbol %s: section number
%d is large then max %d", name, s.SectionNumber, len(f.Sections))
On 2014/05/05 13:25:43, bradfitz wrote:
> two typos: large->larger then->than

Done. (I am bad with English. Sorry.)

[bradfitz, 2014-05-06 17:43:54]

LGTM after nits

https://codereview.appspot.com/96960043/diff/80001/src/cmd/addr2line/addr2lin...
File src/cmd/addr2line/addr2line_test.go (right):

https://codereview.appspot.com/96960043/diff/80001/src/cmd/addr2line/addr2lin...
src/cmd/addr2line/addr2line_test.go:69: out, err := exec.Command("go", "build",
"-o", "testaddr2line.exe", "cmd/addr2line").CombinedOutput()
I'd prefer you never write to GOROOT testdata directories during a test.

Make a tempdir instead and defer os.RemoveAll the temppdir, and "-o",
filepath.Join(tmpDir, "testaddr2line.exe") instead.

Otherwise interrupted/killed tests leave crap on the filesystem.

https://codereview.appspot.com/96960043/diff/80001/src/cmd/addr2line/addr2lin...
src/cmd/addr2line/addr2line_test.go:94: t.Fatalf("expected line number 68; got
%v", srcLineNo)
got first, then want:

line number = %v; want 68

[brainman, 2014-05-07 00:17:03]

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

cmd/addr2line: works with windows pe executables now

Update issue 7406
Fixes issue 7899

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

https://codereview.appspot.com/96960043/diff/80001/src/cmd/addr2line/addr2lin...
File src/cmd/addr2line/addr2line_test.go (right):

https://codereview.appspot.com/96960043/diff/80001/src/cmd/addr2line/addr2lin...
src/cmd/addr2line/addr2line_test.go:69: out, err := exec.Command("go", "build",
"-o", "testaddr2line.exe", "cmd/addr2line").CombinedOutput()
On 2014/05/06 17:43:53, bradfitz wrote:
> I'd prefer you never write to GOROOT testdata directories during a test.
> 
> Make a tempdir instead and defer os.RemoveAll the temppdir, and "-o",
> filepath.Join(tmpDir, "testaddr2line.exe") instead.
> 
> Otherwise interrupted/killed tests leave crap on the filesystem.

Done.

https://codereview.appspot.com/96960043/diff/80001/src/cmd/addr2line/addr2lin...
src/cmd/addr2line/addr2line_test.go:94: t.Fatalf("expected line number 68; got
%v", srcLineNo)
On 2014/05/06 17:43:54, bradfitz wrote:
> got first, then want:
> 
> line number = %v; want 68

Done.

[gobot, 2014-05-07 00:31:08]

This CL appears to have broken the plan9-386-cnielsen builder.
See http://build.golang.org/log/dbab195dc53ea334a434979a07daab7a1df0f15c

[brainman, 2014-05-07 00:38:55]

On 2014/05/07 00:31:08, gobot wrote:
> This CL appears to have broken the plan9-386-cnielsen builder.
> See http://build.golang.org/log/dbab195dc53ea334a434979a07daab7a1df0f15c

I will need help here:

--- FAIL: TestAddr2Line (1.31 seconds)
	addr2line_test.go:45: go tool addr2line
/tmp/go-build198541907/cmd/addr2line/_test/addr2line.test: exit status:
'testaddr2line.exe 1299885: 1'
		addr2line: reading /tmp/go-build198541907/cmd/addr2line/_test/addr2line.test:
unrecognized binary format
FAIL
FAIL	cmd/addr2line	1.401s

I will disable new test on plan9, if nothing else.

Alex

[dave_cheney.net, 2014-05-07 00:41:22]

Another odd failure on the darwin race builder

--- FAIL: TestAddr2Line (1.09 seconds)
addr2line_test.go:96: Stat failed: stat
/usr/local/go/src/cmd/addr2line/addr2line_test.go: no such file or
directory
FAIL
FAIL cmd/addr2line 1.126s

There is no Go installation in /usr/local/go on this machine.

On Wed, May 7, 2014 at 10:38 AM,  <alex.brainman@gmail.com> wrote:
> On 2014/05/07 00:31:08, gobot wrote:
>>
>> This CL appears to have broken the plan9-386-cnielsen builder.
>> See
>
> http://build.golang.org/log/dbab195dc53ea334a434979a07daab7a1df0f15c
>
> I will need help here:
>
> --- FAIL: TestAddr2Line (1.31 seconds)
>         addr2line_test.go:45: go tool addr2line
> /tmp/go-build198541907/cmd/addr2line/_test/addr2line.test: exit status:
> 'testaddr2line.exe 1299885: 1'
>                 addr2line: reading
> /tmp/go-build198541907/cmd/addr2line/_test/addr2line.test: unrecognized
> binary format
> FAIL
> FAIL    cmd/addr2line   1.401s
>
> I will disable new test on plan9, if nothing else.
>
> Alex
>
>
> https://codereview.appspot.com/96960043/
>
> --
> 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-07 00:43:04]

On 2014/05/07 00:41:22, dfc wrote:
> Another odd failure on the darwin race builder
> 
> --- FAIL: TestAddr2Line (1.09 seconds)
> addr2line_test.go:96: Stat failed: stat
> /usr/local/go/src/cmd/addr2line/addr2line_test.go: no such file or
> directory
> FAIL
> FAIL cmd/addr2line 1.126s
> 

Perhaps it is something to do with my use of os.SameFile here.

Alex

[brainman, 2014-05-07 01:33:41]

On 2014/05/07 00:41:22, dfc wrote:
> Another odd failure on the darwin race builder
> 
> --- FAIL: TestAddr2Line (1.09 seconds)
> addr2line_test.go:96: Stat failed: stat
> /usr/local/go/src/cmd/addr2line/addr2line_test.go: no such file or
> directory
> FAIL
> FAIL cmd/addr2line 1.126s
> 
> There is no Go installation in /usr/local/go on this machine.
> 

# export GOROOT_FINAL=/a/b/c
# cd $GOROOT/src
# ./make.bash
...
# go test cmd/addr2line
--- FAIL: TestAddr2Line (0.36 seconds)
        addr2line_test.go:96: Stat failed: stat
/a/b/c/src/cmd/addr2line/addr2line_test.go: no such file or directory
FAIL
FAIL    cmd/addr2line   0.359s

reproduces the problem. Should I consider this builder mis-configured? Should we
change builder code to exclude GOROOT_FINAL variable? Should I change test to
work around this issue?

Alex

[brainman, 2014-05-07 01:55:59]

On 2014/05/07 00:38:55, brainman wrote:
> 
> I will disable new test on plan9, if nothing else.
> 

I will skip that test on plan9 for now:
https://codereview.appspot.com/100180043/

Alex

[brainman, 2014-05-07 03:26:34]

On 2014/05/07 01:33:41, brainman wrote:
> ... Should I consider this builder mis-configured? Should we
> change builder code to exclude GOROOT_FINAL variable? Should I change test to
> work around this issue?
> 

The builder actually sets GOROOT_FINAL (I don't understand why). So I will have
to work around that.

Alex

[dave_cheney.net, 2014-05-07 03:30:24]

On 2014/05/07 03:26:34, brainman wrote:
> On 2014/05/07 01:33:41, brainman wrote:
> > ... Should I consider this builder mis-configured? Should we
> > change builder code to exclude GOROOT_FINAL variable? Should I change test
to
> > work around this issue?
> > 
> 
> The builder actually sets GOROOT_FINAL (I don't understand why). So I will
have
> to work around that.
> 
> Alex

Here is how I run the darwin-race builder

odessa:~ builder$ cat builder.bash 
#!/bin/bash

exec \
sh -c '
	PATH=$PATH:/opt/local/bin
	export PATH
	WORKSPACE=$HOME/workspace
	rm -rf $WORKSPACE/darwin-*
        exec $HOME/builder -v -commitInterval=0 -buildroot=$WORKSPACE
-cmdTimeout=10m darwin-386-cheney darwin-amd64-cheney darwin-amd64-race-cheney
2>&1
'

If GOROOT_FINAL is being set inside the builder process itself, maybe we can
stop doing that. I'm not 100% sure of the rational for setting that value, IMO
it's only used when building distributions via the misc/dist tool.

It might be simpler to unset the value inside race.{bash,bat}

[brainman, 2014-05-07 03:32:21]

On 2014/05/07 03:30:24, dfc wrote:
> 
> If GOROOT_FINAL is being set inside the builder process itself, maybe we can
> stop doing that. ...

I will try and work around it for now. I still don't understand why only one
builder fail. I will investigate ...

Alex

[brainman, 2014-05-07 06:21:39]

On 2014/05/07 03:32:21, brainman wrote:
> 
> I will investigate ...

Found real problem: https://codereview.appspot.com/98980043/

Alex