Make binutils thread safe. (#231)

* Make binutils thread safe.

binutils is now used from multiple goroutines when the
web interface is enabled (since http server spins up a new
goroutine for each request). Added appropriate synchronization
to various objects in binutils to provide the appropriate
thread-safety.

In addition, added a bunch of tests:

. Test parsing of nm output.
. Tests for Binutils methods.
. Test llvm-symbolizer interface (using an llvm-symbolizer emulator).

* Fix go vet errors about unkeyed literals.

* copy stderr to figure out test failures

* Make shell script portable.

* Update comments to match new var name.

* Give a name to the embedded Mutex field in addr2Liner.

internal/binutils/addr2liner.go

@@ -21,6 +21,7 @@ import (
 	"os/exec"
 	"strconv"
 	"strings"
+	"sync"
 
 	"github.com/google/pprof/internal/plugin"
 )
@@ -36,6 +37,7 @@ const (
 // addr2Liner is a connection to an addr2line command for obtaining
 // address and line number information from a binary.
 type addr2Liner struct {
+	mu   sync.Mutex
 	rw   lineReaderWriter
 	base uint64
 
@@ -170,9 +172,10 @@ func (d *addr2Liner) readFrame() (plugin.Frame, bool) {
 		Line: linenumber}, false
 }
 
-// addrInfo returns the stack frame information for a specific program
-// address. It returns nil if the address could not be identified.
-func (d *addr2Liner) addrInfo(addr uint64) ([]plugin.Frame, error) {
+func (d *addr2Liner) rawAddrInfo(addr uint64) ([]plugin.Frame, error) {
+	d.mu.Lock()
+	defer d.mu.Unlock()
+
 	if err := d.rw.write(fmt.Sprintf("%x", addr-d.base)); err != nil {
 		return nil, err
 	}
@@ -201,6 +204,16 @@ func (d *addr2Liner) addrInfo(addr uint64) ([]plugin.Frame, error) {
 			stack = append(stack, frame)
 		}
 	}
+	return stack, err
+}
+
+// addrInfo returns the stack frame information for a specific program
+// address. It returns nil if the address could not be identified.
+func (d *addr2Liner) addrInfo(addr uint64) ([]plugin.Frame, error) {
+	stack, err := d.rawAddrInfo(addr)
+	if err != nil {
+		return nil, err
+	}
 
 	// Get better name from nm if possible.
 	if len(stack) > 0 && d.nm != nil {

internal/binutils/addr2liner_llvm.go

@@ -21,6 +21,7 @@ import (
 	"os/exec"
 	"strconv"
 	"strings"
+	"sync"
 
 	"github.com/google/pprof/internal/plugin"
 )
@@ -32,6 +33,7 @@ const (
 // llvmSymbolizer is a connection to an llvm-symbolizer command for
 // obtaining address and line number information from a binary.
 type llvmSymbolizer struct {
+	sync.Mutex
 	filename string
 	rw       lineReaderWriter
 	base     uint64
@@ -150,6 +152,9 @@ func (d *llvmSymbolizer) readFrame() (plugin.Frame, bool) {
 // addrInfo returns the stack frame information for a specific program
 // address. It returns nil if the address could not be identified.
 func (d *llvmSymbolizer) addrInfo(addr uint64) ([]plugin.Frame, error) {
+	d.Lock()
+	defer d.Unlock()
+
 	if err := d.rw.write(fmt.Sprintf("%s 0x%x", d.filename, addr-d.base)); err != nil {
 		return nil, err
 	}

internal/binutils/addr2liner_nm.go

@@ -48,22 +48,23 @@ func newAddr2LinerNM(cmd, file string, base uint64) (*addr2LinerNM, error) {
 	if cmd == "" {
 		cmd = defaultNM
 	}
-
-	a := &addr2LinerNM{
-		m: []symbolInfo{},
-	}
-
 	var b bytes.Buffer
 	c := exec.Command(cmd, "-n", file)
 	c.Stdout = &b
-
 	if err := c.Run(); err != nil {
 		return nil, err
 	}
+	return parseAddr2LinerNM(base, &b)
+}
+
+func parseAddr2LinerNM(base uint64, nm io.Reader) (*addr2LinerNM, error) {
+	a := &addr2LinerNM{
+		m: []symbolInfo{},
+	}
 
 	// Parse nm output and populate symbol map.
 	// Skip lines we fail to parse.
-	buf := bufio.NewReader(&b)
+	buf := bufio.NewReader(nm)
 	for {
 		line, err := buf.ReadString('\n')
 		if line == "" && err != nil {

internal/binutils/binutils.go

@@ -24,14 +24,21 @@ import (
 	"path/filepath"
 	"regexp"
 	"strings"
+	"sync"
 
 	"github.com/google/pprof/internal/elfexec"
 	"github.com/google/pprof/internal/plugin"
 )
 
 // A Binutils implements plugin.ObjTool by invoking the GNU binutils.
-// SetConfig must be called before any of the other methods.
 type Binutils struct {
+	sync.Mutex
+	rep *binrep
+}
+
+// binrep is an immutable representation for Binutils.  It is atomically
+// replaced on every mutation to provide thread-safe access.
+type binrep struct {
 	// Commands to invoke.
 	llvmSymbolizer      string
 	llvmSymbolizerFound bool
@@ -47,11 +54,38 @@ type Binutils struct {
 	fast bool
 }
 
+// get returns the current representation for bu, initializing it if necessary.
+func (bu *Binutils) get() *binrep {
+	bu.Mutex.Lock()
+	r := bu.rep
+	if r == nil {
+		r = &binrep{}
+		initTools(r, "")
+		bu.rep = r
+	}
+	bu.Mutex.Unlock()
+	return r
+}
+
+// update modifies the rep for bu via the supplied function.
+func (bu *Binutils) update(fn func(r *binrep)) {
+	r := &binrep{}
+	bu.Mutex.Lock()
+	defer bu.Mutex.Unlock()
+	if bu.rep == nil {
+		initTools(r, "")
+	} else {
+		*r = *bu.rep
+	}
+	fn(r)
+	bu.rep = r
+}
+
 // SetFastSymbolization sets a toggle that makes binutils use fast
 // symbolization (using nm), which is much faster than addr2line but
 // provides only symbol name information (no file/line).
-func (b *Binutils) SetFastSymbolization(fast bool) {
-	b.fast = fast
+func (bu *Binutils) SetFastSymbolization(fast bool) {
+	bu.update(func(r *binrep) { r.fast = fast })
 }
 
 // SetTools processes the contents of the tools option. It
@@ -59,7 +93,11 @@ func (b *Binutils) SetFastSymbolization(fast bool) {
 // of the form t:path, where cmd will be used to look only for the
 // tool named t. If t is not specified, the path is searched for all
 // tools.
-func (b *Binutils) SetTools(config string) {
+func (bu *Binutils) SetTools(config string) {
+	bu.update(func(r *binrep) { initTools(r, config) })
+}
+
+func initTools(b *binrep, config string) {
 	// paths collect paths per tool; Key "" contains the default.
 	paths := make(map[string][]string)
 	for _, t := range strings.Split(config, ",") {
@@ -91,11 +129,8 @@ func findExe(cmd string, paths []string) (string, bool) {
 
 // Disasm returns the assembly instructions for the specified address range
 // of a binary.
-func (b *Binutils) Disasm(file string, start, end uint64) ([]plugin.Inst, error) {
-	if b.addr2line == "" {
-		// Update the command invocations if not initialized.
-		b.SetTools("")
-	}
+func (bu *Binutils) Disasm(file string, start, end uint64) ([]plugin.Inst, error) {
+	b := bu.get()
 	cmd := exec.Command(b.objdump, "-d", "-C", "--no-show-raw-insn", "-l",
 		fmt.Sprintf("--start-address=%#x", start),
 		fmt.Sprintf("--stop-address=%#x", end),
@@ -109,11 +144,8 @@ func (b *Binutils) Disasm(file string, start, end uint64) ([]plugin.Inst, error)
 }
 
 // Open satisfies the plugin.ObjTool interface.
-func (b *Binutils) Open(name string, start, limit, offset uint64) (plugin.ObjFile, error) {
-	if b.addr2line == "" {
-		// Update the command invocations if not initialized.
-		b.SetTools("")
-	}
+func (bu *Binutils) Open(name string, start, limit, offset uint64) (plugin.ObjFile, error) {
+	b := bu.get()
 
 	// Make sure file is a supported executable.
 	// The pprof driver uses Open to sniff the difference
@@ -140,7 +172,7 @@ func (b *Binutils) Open(name string, start, limit, offset uint64) (plugin.ObjFil
 	return nil, fmt.Errorf("unrecognized binary: %s", name)
 }
 
-func (b *Binutils) openMachO(name string, start, limit, offset uint64) (plugin.ObjFile, error) {
+func (b *binrep) openMachO(name string, start, limit, offset uint64) (plugin.ObjFile, error) {
 	of, err := macho.Open(name)
 	if err != nil {
 		return nil, fmt.Errorf("Parsing %s: %v", name, err)
@@ -153,7 +185,7 @@ func (b *Binutils) openMachO(name string, start, limit, offset uint64) (plugin.O
 	return &fileAddr2Line{file: file{b: b, name: name}}, nil
 }
 
-func (b *Binutils) openELF(name string, start, limit, offset uint64) (plugin.ObjFile, error) {
+func (b *binrep) openELF(name string, start, limit, offset uint64) (plugin.ObjFile, error) {
 	ef, err := elf.Open(name)
 	if err != nil {
 		return nil, fmt.Errorf("Parsing %s: %v", name, err)
@@ -202,7 +234,7 @@ func (b *Binutils) openELF(name string, start, limit, offset uint64) (plugin.Obj
 
 // file implements the binutils.ObjFile interface.
 type file struct {
-	b       *Binutils
+	b       *binrep
 	name    string
 	base    uint64
 	buildID string
@@ -263,22 +295,27 @@ func (f *fileNM) SourceLine(addr uint64) ([]plugin.Frame, error) {
 // information). It can be slow for large binaries with debug
 // information.
 type fileAddr2Line struct {
+	once sync.Once
 	file
 	addr2liner     *addr2Liner
 	llvmSymbolizer *llvmSymbolizer
 }
 
 func (f *fileAddr2Line) SourceLine(addr uint64) ([]plugin.Frame, error) {
+	f.once.Do(f.init)
 	if f.llvmSymbolizer != nil {
 		return f.llvmSymbolizer.addrInfo(addr)
 	}
 	if f.addr2liner != nil {
 		return f.addr2liner.addrInfo(addr)
 	}
+	return nil, fmt.Errorf("could not find local addr2liner")
+}
 
+func (f *fileAddr2Line) init() {
 	if llvmSymbolizer, err := newLLVMSymbolizer(f.b.llvmSymbolizer, f.name, f.base); err == nil {
 		f.llvmSymbolizer = llvmSymbolizer
-		return f.llvmSymbolizer.addrInfo(addr)
+		return
 	}
 
 	if addr2liner, err := newAddr2Liner(f.b.addr2line, f.name, f.base); err == nil {
@@ -290,13 +327,14 @@ func (f *fileAddr2Line) SourceLine(addr uint64) ([]plugin.Frame, error) {
 		if nm, err := newAddr2LinerNM(f.b.nm, f.name, f.base); err == nil {
 			f.addr2liner.nm = nm
 		}
-		return f.addr2liner.addrInfo(addr)
 	}
-
-	return nil, fmt.Errorf("could not find local addr2liner")
 }
 
 func (f *fileAddr2Line) Close() error {
+	if f.llvmSymbolizer != nil {
+		f.llvmSymbolizer.rw.close()
+		f.llvmSymbolizer = nil
+	}
 	if f.addr2liner != nil {
 		f.addr2liner.rw.close()
 		f.addr2liner = nil

internal/binutils/binutils_test.go

@@ -15,7 +15,13 @@
 package binutils
 
 import (
+	"bytes"
 	"fmt"
+	"math"
+	"path/filepath"
+	"reflect"
+	"regexp"
+	"runtime"
 	"testing"
 
 	"github.com/google/pprof/internal/plugin"
@@ -37,7 +43,7 @@ func functionName(level int) (name string) {
 func TestAddr2Liner(t *testing.T) {
 	const offset = 0x500
 
-	a := addr2Liner{&mockAddr2liner{}, offset, nil}
+	a := addr2Liner{rw: &mockAddr2liner{}, base: offset}
 	for i := 1; i < 8; i++ {
 		addr := i*0x1000 + offset
 		s, err := a.addrInfo(uint64(addr))
@@ -112,24 +118,23 @@ func (a *mockAddr2liner) close() {
 }
 
 func TestAddr2LinerLookup(t *testing.T) {
-	oddSizedMap := addr2LinerNM{
-		m: []symbolInfo{
-			{0x1000, "0x1000"},
-			{0x2000, "0x2000"},
-			{0x3000, "0x3000"},
-		},
-	}
-	evenSizedMap := addr2LinerNM{
-		m: []symbolInfo{
-			{0x1000, "0x1000"},
-			{0x2000, "0x2000"},
-			{0x3000, "0x3000"},
-			{0x4000, "0x4000"},
-		},
-	}
-	for _, a := range []*addr2LinerNM{
-		&oddSizedMap, &evenSizedMap,
-	} {
+	const oddSizedData = `
+00001000 T 0x1000
+00002000 T 0x2000
+00003000 T 0x3000
+`
+	const evenSizedData = `
+0000000000001000 T 0x1000
+0000000000002000 T 0x2000
+0000000000003000 T 0x3000
+0000000000004000 T 0x4000
+`
+	for _, d := range []string{oddSizedData, evenSizedData} {
+		a, err := parseAddr2LinerNM(0, bytes.NewBufferString(d))
+		if err != nil {
+			t.Errorf("nm parse error: %v", err)
+			continue
+		}
 		for address, want := range map[uint64]string{
 			0x1000: "0x1000",
 			0x1001: "0x1000",
@@ -141,6 +146,11 @@ func TestAddr2LinerLookup(t *testing.T) {
 				t.Errorf("%x: got %v, want %s", address, got, want)
 			}
 		}
+		for _, unknown := range []uint64{0x0fff, 0x4001} {
+			if got, _ := a.addrInfo(unknown); got != nil {
+				t.Errorf("%x: got %v, want nil", unknown, got)
+			}
+		}
 	}
 }
 
@@ -150,3 +160,116 @@ func checkAddress(got []plugin.Frame, address uint64, want string) bool {
 	}
 	return got[0].Func == want
 }
+
+func TestSetTools(t *testing.T) {
+	// Test that multiple calls work.
+	bu := &Binutils{}
+	bu.SetTools("")
+	bu.SetTools("")
+}
+
+func TestSetFastSymbolization(t *testing.T) {
+	// Test that multiple calls work.
+	bu := &Binutils{}
+	bu.SetFastSymbolization(true)
+	bu.SetFastSymbolization(false)
+}
+
+func skipUnlessLinuxAmd64(t *testing.T) {
+	if runtime.GOOS != "linux" || runtime.GOARCH != "amd64" {
+		t.Skip("Disasm only tested on x86-64 linux")
+	}
+}
+
+func TestDisasm(t *testing.T) {
+	skipUnlessLinuxAmd64(t)
+	bu := &Binutils{}
+	insts, err := bu.Disasm(filepath.Join("testdata", "hello"), 0, math.MaxUint64)
+	if err != nil {
+		t.Fatalf("Disasm: unexpected error %v", err)
+	}
+	mainCount := 0
+	for _, x := range insts {
+		if x.Function == "main" {
+			mainCount++
+		}
+	}
+	if mainCount == 0 {
+		t.Error("Disasm: found no main instructions")
+	}
+}
+
+func TestObjFile(t *testing.T) {
+	skipUnlessLinuxAmd64(t)
+	bu := &Binutils{}
+	f, err := bu.Open(filepath.Join("testdata", "hello"), 0, math.MaxUint64, 0)
+	if err != nil {
+		t.Fatalf("Open: unexpected error %v", err)
+	}
+	defer f.Close()
+	syms, err := f.Symbols(regexp.MustCompile("main"), 0)
+	if err != nil {
+		t.Fatalf("Symbols: unexpected error %v", err)
+	}
+
+	find := func(name string) *plugin.Sym {
+		for _, s := range syms {
+			for _, n := range s.Name {
+				if n == name {
+					return s
+				}
+			}
+		}
+		return nil
+	}
+	m := find("main")
+	if m == nil {
+		t.Fatalf("Symbols: did not find main")
+	}
+	frames, err := f.SourceLine(m.Start)
+	if err != nil {
+		t.Fatalf("SourceLine: unexpected error %v", err)
+	}
+	expect := []plugin.Frame{
+		{Func: "main", File: "/tmp/hello.c", Line: 3},
+	}
+	if !reflect.DeepEqual(frames, expect) {
+		t.Fatalf("SourceLine for main: expect %v; got %v\n", expect, frames)
+	}
+}
+
+func TestLLVMSymbolizer(t *testing.T) {
+	if runtime.GOOS != "linux" {
+		t.Skip("testtdata/llvm-symbolizer has only been tested on linux")
+	}
+
+	cmd := filepath.Join("testdata", "fake-llvm-symbolizer")
+	symbolizer, err := newLLVMSymbolizer(cmd, "foo", 0)
+	if err != nil {
+		t.Fatalf("newLLVMSymbolizer: unexpected error %v", err)
+	}
+	defer symbolizer.rw.close()
+
+	for _, c := range []struct {
+		addr   uint64
+		frames []plugin.Frame
+	}{
+		{0x10, []plugin.Frame{
+			{Func: "Inlined_0x10", File: "foo.h", Line: 0},
+			{Func: "Func_0x10", File: "foo.c", Line: 2},
+		}},
+		{0x20, []plugin.Frame{
+			{Func: "Inlined_0x20", File: "foo.h", Line: 0},
+			{Func: "Func_0x20", File: "foo.c", Line: 2},
+		}},
+	} {
+		frames, err := symbolizer.addrInfo(c.addr)
+		if err != nil {
+			t.Errorf("LLVM: unexpected error %v", err)
+			continue
+		}
+		if !reflect.DeepEqual(frames, c.frames) {
+			t.Errorf("LLVM: expect %v; got %v\n", c.frames, frames)
+		}
+	}
+}

internal/binutils/testdata/fake-llvm-symbolizer

@@ -0,0 +1,34 @@
+#!/bin/sh
+#
+# Copyright 2014 Google Inc. All Rights Reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+# Fake llvm-symbolizer to use in tests
+
+set -f
+IFS=" "
+
+while read line; do
+  # line has form:
+  #    filename 0xaddr
+  # Emit dummy output that matches llvm-symbolizer output format.
+  set -- $line
+  fname=$1
+  addr=$2
+  echo "Inlined_$addr"
+  echo "$fname.h"
+  echo "Func_$addr"
+  echo "$fname.c:2"
+  echo
+done