Enable MacOS binutils test. (#322)

.travis.yml

@@ -47,10 +47,11 @@ addons:
   apt:
     packages:
       - graphviz
-      
+
 before_install:
-  - go get -u github.com/golang/lint/golint honnef.co/go/tools/cmd/...   
-  - if [[ "$TRAVIS_OS_NAME" == "osx" && -z $SKIP_GRAPHVIZ ]]; then brew update          ; fi
+  - go get -u github.com/golang/lint/golint honnef.co/go/tools/cmd/...
+  - if [[ "$TRAVIS_OS_NAME" == "osx" ]]; then brew update ; fi
+  - if [[ "$TRAVIS_OS_NAME" == "osx" ]]; then brew install binutils ; fi
   - if [[ "$TRAVIS_OS_NAME" == "osx" && -z $SKIP_GRAPHVIZ ]]; then brew install graphviz; fi
 
 script:

internal/binutils/addr2liner.go

@@ -41,9 +41,11 @@ type addr2Liner struct {
 	rw   lineReaderWriter
 	base uint64
 
-	// nm holds an NM based addr2Liner which can provide
-	// better full names compared to addr2line, which often drops
-	// namespaces etc. from the names it returns.
+	// nm holds an addr2Liner using nm tool. Certain versions of addr2line
+	// produce incomplete names due to
+	// https://sourceware.org/bugzilla/show_bug.cgi?id=17541. As a workaround,
+	// the names from nm are used when they look more complete. See addrInfo()
+	// code below for the exact heuristic.
 	nm *addr2LinerNM
 }
 
@@ -215,17 +217,22 @@ func (d *addr2Liner) addrInfo(addr uint64) ([]plugin.Frame, error) {
 		return nil, err
 	}
 
-	// Get better name from nm if possible.
+	// Certain versions of addr2line produce incomplete names due to
+	// https://sourceware.org/bugzilla/show_bug.cgi?id=17541. Attempt to replace
+	// the name with a better one from nm.
 	if len(stack) > 0 && d.nm != nil {
 		nm, err := d.nm.addrInfo(addr)
 		if err == nil && len(nm) > 0 {
-			// Last entry in frame list should match since
-			// it is non-inlined. As a simple heuristic,
-			// we only switch to the nm-based name if it
-			// is longer.
+			// Last entry in frame list should match since it is non-inlined. As a
+			// simple heuristic, we only switch to the nm-based name if it is longer
+			// by 2 or more characters. We consider nm names that are longer by 1
+			// character insignificant to avoid replacing foo with _foo on MacOS (for
+			// unknown reasons read2line produces the former and nm produces the
+			// latter on MacOS even though both tools are asked to produce mangled
+			// names).
 			nmName := nm[len(nm)-1].Func
 			a2lName := stack[len(stack)-1].Func
-			if len(nmName) > len(a2lName) {
+			if len(nmName) > len(a2lName)+1 {
 				stack[len(stack)-1].Func = nmName
 			}
 		}

internal/binutils/binutils.go

@@ -111,6 +111,11 @@ func initTools(b *binrep, config string) {
 	defaultPath := paths[""]
 	b.llvmSymbolizer, b.llvmSymbolizerFound = findExe("llvm-symbolizer", append(paths["llvm-symbolizer"], defaultPath...))
 	b.addr2line, b.addr2lineFound = findExe("addr2line", append(paths["addr2line"], defaultPath...))
+	if !b.addr2lineFound {
+		// On MacOS, brew installs addr2line under gaddr2line name, so search for
+		// that if the tool is not found by its default name.
+		b.addr2line, b.addr2lineFound = findExe("gaddr2line", append(paths["addr2line"], defaultPath...))
+	}
 	b.nm, b.nmFound = findExe("nm", append(paths["nm"], defaultPath...))
 	b.objdump, b.objdumpFound = findExe("objdump", append(paths["objdump"], defaultPath...))
 }
@@ -306,9 +311,9 @@ func (f *fileNM) SourceLine(addr uint64) ([]plugin.Frame, error) {
 }
 
 // fileAddr2Line implements the binutils.ObjFile interface, using
-// 'addr2line' to map addresses to symbols (with file/line number
-// information). It can be slow for large binaries with debug
-// information.
+// llvm-symbolizer, if that's available, or addr2line to map addresses to
+// symbols (with file/line number information). It can be slow for large
+// binaries with debug information.
 type fileAddr2Line struct {
 	once sync.Once
 	file

internal/binutils/binutils_test.go

@@ -265,8 +265,6 @@ func TestObjFile(t *testing.T) {
 func TestMachoFiles(t *testing.T) {
 	skipUnlessDarwinAmd64(t)
 
-	t.Skip("Disabled because of issues with addr2line (see https://github.com/google/pprof/pull/313#issuecomment-364073010)")
-
 	// Load `file`, pretending it was mapped at `start`. Then get the symbol
 	// table. Check that it contains the symbol `sym` and that the address
 	// `addr` gives the `expected` stack trace.
@@ -291,7 +289,7 @@ func TestMachoFiles(t *testing.T) {
 		{"lib normal mapping", "lib_mac_64", 0, math.MaxUint64, 0,
 			0xfa0, "_bar",
 			[]plugin.Frame{
-				{Func: "bar", File: "/tmp/lib.c", Line: 6},
+				{Func: "bar", File: "/tmp/lib.c", Line: 5},
 			}},
 	} {
 		t.Run(tc.desc, func(t *testing.T) {

internal/binutils/testdata/build_mac.sh

@@ -0,0 +1,31 @@
+#!/bin/bash -x
+
+# This is a script that generates the test MacOS executables in this directory.
+# It should be needed very rarely to run this script. It is mostly provided
+# as a future reference on how the original binary set was created.
+
+set -o errexit
+
+cat <<EOF >/tmp/hello.cc
+#include <stdio.h>
+
+int main() {
+  printf("Hello, world!\n");
+  return 0;
+}
+EOF
+
+cat <<EOF >/tmp/lib.c
+int foo() {
+  return 1;
+}
+
+int bar() {
+  return 2;
+}
+EOF
+
+cd $(dirname $0)
+rm -rf exe_mac_64* lib_mac_64*
+clang -g -o exe_mac_64 /tmp/hello.c
+clang -g -o lib_mac_64 -dynamiclib /tmp/lib.c

internal/binutils/testdata/exe_mac_64.dSYM/Contents/Info.plist

@@ -0,0 +1,20 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE plist PUBLIC "-//Apple Computer//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
+<plist version="1.0">
+	<dict>
+		<key>CFBundleDevelopmentRegion</key>
+		<string>English</string>
+		<key>CFBundleIdentifier</key>
+		<string>com.apple.xcode.dsym.exe_mac_64</string>
+		<key>CFBundleInfoDictionaryVersion</key>
+		<string>6.0</string>
+		<key>CFBundlePackageType</key>
+		<string>dSYM</string>
+		<key>CFBundleSignature</key>
+		<string>????</string>
+		<key>CFBundleShortVersionString</key>
+		<string>1.0</string>
+		<key>CFBundleVersion</key>
+		<string>1</string>
+	</dict>
+</plist>

internal/binutils/testdata/lib_mac_64.dSYM/Contents/Info.plist

@@ -0,0 +1,20 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE plist PUBLIC "-//Apple Computer//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
+<plist version="1.0">
+	<dict>
+		<key>CFBundleDevelopmentRegion</key>
+		<string>English</string>
+		<key>CFBundleIdentifier</key>
+		<string>com.apple.xcode.dsym.lib_mac_64</string>
+		<key>CFBundleInfoDictionaryVersion</key>
+		<string>6.0</string>
+		<key>CFBundlePackageType</key>
+		<string>dSYM</string>
+		<key>CFBundleSignature</key>
+		<string>????</string>
+		<key>CFBundleShortVersionString</key>
+		<string>1.0</string>
+		<key>CFBundleVersion</key>
+		<string>1</string>
+	</dict>
+</plist>