Improve handling of Go symbols with type parameters (#717)

* internal/symbolizer: add unit tests for Demangle

For the curious: `bar(int (*) [5])` is a pointer to an array of
length 5:

	typedef int IntArray[5];
	int bar(IntArray* arr);

For #705.

* internal/symbolizer: avoid matching generic Go symbols as C++

For #705.

* internal/graph: keep Go type parameter ellipsis in dot

Go symbols with type parameters are reported in Go pprof profiles as
something like `main.Set[...]`.

Today, multilinePrintableName mishandles this by replacing each with a
newline, resulting in a very strange looking node.

Instead, replace ... with a unicode ellipsis so that it isn't replaced
with a newline just below.

Note that the full symbol name (which is reported by tools like perf) is
something like `[go.shape.string_0,go.shape.int_1]`. This case is still
not handled very well, but we likely want to strip this out anyways in
ShortenFunctionName or demangling as future work.

Fixes #705.

Author: prattmic

internal/graph/dotgraph.go

@@ -385,6 +385,9 @@ func multilinePrintableName(info *NodeInfo) string {
 	infoCopy := *info
 	infoCopy.Name = escapeForDot(ShortenFunctionName(infoCopy.Name))
 	infoCopy.Name = strings.Replace(infoCopy.Name, "::", `\n`, -1)
+	// Go type parameters are reported as "[...]" by Go pprof profiles.
+	// Keep this ellipsis rather than replacing with newlines below.
+	infoCopy.Name = strings.Replace(infoCopy.Name, "[...]", "[…]", -1)
 	infoCopy.Name = strings.Replace(infoCopy.Name, ".", `\n`, -1)
 	if infoCopy.File != "" {
 		infoCopy.File = filepath.Base(infoCopy.File)

internal/graph/graph_test.go

@@ -445,6 +445,10 @@ func TestShortenFunctionName(t *testing.T) {
 			"github.com/BlahBlah/foo.Foo",
 			"foo.Foo",
 		},
+		{
+			"github.com/BlahBlah/foo.Foo[...]",
+			"foo.Foo[...]",
+		},
 		{
 			"github.com/blah-blah/foo_bar.(*FooBar).Foo",
 			"foo_bar.(*FooBar).Foo",

internal/symbolizer/symbolizer.go

@@ -205,49 +205,64 @@ func Demangle(prof *profile.Profile, force bool, demanglerMode string) {
 		}
 	}
 
-	var options []demangle.Option
+	options := demanglerModeToOptions(demanglerMode)
+	for _, fn := range prof.Function {
+		demangleSingleFunction(fn, options)
+	}
+}
+
+func demanglerModeToOptions(demanglerMode string) []demangle.Option {
 	switch demanglerMode {
 	case "": // demangled, simplified: no parameters, no templates, no return type
-		options = []demangle.Option{demangle.NoParams, demangle.NoTemplateParams}
+		return []demangle.Option{demangle.NoParams, demangle.NoTemplateParams}
 	case "templates": // demangled, simplified: no parameters, no return type
-		options = []demangle.Option{demangle.NoParams}
+		return []demangle.Option{demangle.NoParams}
 	case "full":
-		options = []demangle.Option{demangle.NoClones}
+		return []demangle.Option{demangle.NoClones}
 	case "none": // no demangling
-		return
+		return []demangle.Option{}
 	}
 
+	panic(fmt.Sprintf("unknown demanglerMode %s", demanglerMode))
+}
+
+func demangleSingleFunction(fn *profile.Function, options []demangle.Option) {
+	if fn.Name != "" && fn.SystemName != fn.Name {
+		return // Already demangled.
+	}
 	// Copy the options because they may be updated by the call.
 	o := make([]demangle.Option, len(options))
-	for _, fn := range prof.Function {
-		if fn.Name != "" && fn.SystemName != fn.Name {
-			continue // Already demangled.
-		}
-		copy(o, options)
-		if demangled := demangle.Filter(fn.SystemName, o...); demangled != fn.SystemName {
-			fn.Name = demangled
-			continue
-		}
-		// Could not demangle. Apply heuristics in case the name is
-		// already demangled.
-		name := fn.SystemName
-		if looksLikeDemangledCPlusPlus(name) {
-			if demanglerMode == "" || demanglerMode == "templates" {
+	copy(o, options)
+	if demangled := demangle.Filter(fn.SystemName, o...); demangled != fn.SystemName {
+		fn.Name = demangled
+		return
+	}
+	// Could not demangle. Apply heuristics in case the name is
+	// already demangled.
+	name := fn.SystemName
+	if looksLikeDemangledCPlusPlus(name) {
+		for _, o := range options {
+			switch o {
+			case demangle.NoParams:
 				name = removeMatching(name, '(', ')')
-			}
-			if demanglerMode == "" {
+			case demangle.NoTemplateParams:
 				name = removeMatching(name, '<', '>')
 			}
 		}
-		fn.Name = name
 	}
+	fn.Name = name
 }
 
 // looksLikeDemangledCPlusPlus is a heuristic to decide if a name is
 // the result of demangling C++. If so, further heuristics will be
 // applied to simplify the name.
 func looksLikeDemangledCPlusPlus(demangled string) bool {
-	if strings.Contains(demangled, ".<") { // Skip java names of the form "class.<init>"
+	// Skip java names of the form "class.<init>".
+	if strings.Contains(demangled, ".<") {
+		return false
+	}
+	// Skip Go names of the form "foo.(*Bar[...]).Method".
+	if strings.Contains(demangled, "]).") {
 		return false
 	}
 	return strings.ContainsAny(demangled, "<>[]") || strings.Contains(demangled, "::")

internal/symbolizer/symbolizer_test.go

@@ -261,6 +261,118 @@ func frame(fname, file string, line int) plugin.Frame {
 		Line: line}
 }
 
+func TestDemangleSingleFunction(t *testing.T) {
+	// All tests with default mode.
+	demanglerMode := ""
+	options := demanglerModeToOptions(demanglerMode)
+
+	cases := []struct {
+		symbol string
+		want   string
+	}{
+		{
+			// Trivial C symbol.
+			symbol: "printf",
+			want:   "printf",
+		},
+		{
+			// foo::bar(int)
+			symbol: "_ZN3foo3barEi",
+			want:   "foo::bar",
+		},
+		{
+			// Already demangled.
+			symbol: "foo::bar(int)",
+			want:   "foo::bar",
+		},
+		{
+			// int foo::baz<double>(double)
+			symbol: "_ZN3foo3bazIdEEiT",
+			want:   "foo::baz",
+		},
+		{
+			// Already demangled.
+			//
+			// TODO: The demangled form of this is actually
+			// 'int foo::baz<double>(double)', but our heuristic
+			// can't strip the return type. Should it be able to?
+			symbol: "foo::baz<double>(double)",
+			want:   "foo::baz",
+		},
+		{
+			// operator delete[](void*)
+			symbol: "_ZdaPv",
+			want:   "operator delete[]",
+		},
+		{
+			// Already demangled.
+			symbol: "operator delete[](void*)",
+			want:   "operator delete[]",
+		},
+		{
+			// bar(int (*) [5])
+			symbol: "_Z3barPA5_i",
+			want:   "bar",
+		},
+		{
+			// Already demangled.
+			symbol: "bar(int (*) [5])",
+			want:   "bar",
+		},
+		// Java symbols, do not demangle.
+		{
+			symbol: "java.lang.Float.parseFloat",
+			want:   "java.lang.Float.parseFloat",
+		},
+		{
+			symbol: "java.lang.Float.<init>",
+			want:   "java.lang.Float.<init>",
+		},
+		// Go symbols, do not demangle.
+		{
+			symbol: "example.com/foo.Bar",
+			want:   "example.com/foo.Bar",
+		},
+		{
+			symbol: "example.com/foo.(*Bar).Bat",
+			want:   "example.com/foo.(*Bar).Bat",
+		},
+		{
+			// Method on type with type parameters, as reported by
+			// Go pprof profiles (simplified symbol name).
+			symbol: "example.com/foo.(*Bar[...]).Bat",
+			want:   "example.com/foo.(*Bar[...]).Bat",
+		},
+		{
+			// Method on type with type parameters, as reported by
+			// perf profiles (actual symbol name).
+			symbol: "example.com/foo.(*Bar[go.shape.string_0,go.shape.int_1]).Bat",
+			want:   "example.com/foo.(*Bar[go.shape.string_0,go.shape.int_1]).Bat",
+		},
+		{
+			// Function with type parameters, as reported by Go
+			// pprof profiles (simplified symbol name).
+			symbol: "example.com/foo.Bar[...]",
+			want:   "example.com/foo.Bar[...]",
+		},
+		{
+			// Function with type parameters, as reported by perf
+			// profiles (actual symbol name).
+			symbol: "example.com/foo.Bar[go.shape.string_0,go.shape.int_1]",
+			want:   "example.com/foo.Bar[go.shape.string_0,go.shape.int_1]",
+		},
+	}
+	for _, tc := range cases {
+		fn := &profile.Function{
+			SystemName: tc.symbol,
+		}
+		demangleSingleFunction(fn, options)
+		if fn.Name != tc.want {
+			t.Errorf("demangleSingleFunction(%s) got %s want %s", tc.symbol, fn.Name, tc.want)
+		}
+	}
+}
+
 type mockObjTool struct{}
 
 func (mockObjTool) Open(file string, start, limit, offset uint64, relocationSymbol string) (plugin.ObjFile, error) {