Stop treating -symbolize=force as the default (#210)

* Stop treating -symbolize=force as the default

The code parsing symbolize options was treating the empty option as
"force", effectively making -symbolize=force the default. This would
unnecessarily symbolize profiles that were already symbolized.

* Add more testing for symbolize option parsing

Do all parsing of symbolize options on a single function and
do more thorough testing of it.

* Have symbolz honor -force

Currently -symbolize=remote will force remote symbolization of mappings
that already had mappings. Making that depend on an explicit
-symbolize=force to make it consistent with local symbolization.

* Applied fixes from latest review iteration

* Add comment on symbolz check for resymbolization

* Reformatting comment

internal/driver/driver_test.go

@@ -472,7 +472,7 @@ func testFetchSymbols(source, post string) ([]byte, error) {
 type testSymbolzSymbolizer struct{}
 
 func (testSymbolzSymbolizer) Symbolize(variables string, sources plugin.MappingSources, p *profile.Profile) error {
-	return symbolz.Symbolize(sources, testFetchSymbols, p, nil)
+	return symbolz.Symbolize(p, false, sources, testFetchSymbols, nil)
 }
 
 func fakeDemangler(name string) string {

internal/symbolizer/symbolizer.go

@@ -42,21 +42,26 @@ type Symbolizer struct {
 // test taps for dependency injection
 var symbolzSymbolize = symbolz.Symbolize
 var localSymbolize = doLocalSymbolize
+var demangleFunction = Demangle
 
 // Symbolize attempts to symbolize profile p. First uses binutils on
 // local binaries; if the source is a URL it attempts to get any
 // missed entries using symbolz.
 func (s *Symbolizer) Symbolize(mode string, sources plugin.MappingSources, p *profile.Profile) error {
-	remote, local, force, demanglerMode := true, true, false, ""
+	remote, local, fast, force, demanglerMode := true, true, false, false, ""
 	for _, o := range strings.Split(strings.ToLower(mode), ":") {
 		switch o {
+		case "":
+			continue
 		case "none", "no":
 			return nil
-		case "local", "fastlocal":
+		case "local":
 			remote, local = false, true
+		case "fastlocal":
+			remote, local, fast = false, true, true
 		case "remote":
 			remote, local = true, false
-		case "", "force":
+		case "force":
 			force = true
 		default:
 			switch d := strings.TrimPrefix(o, "demangle="); d {
@@ -75,17 +80,17 @@ func (s *Symbolizer) Symbolize(mode string, sources plugin.MappingSources, p *pr
 	var err error
 	if local {
 		// Symbolize locally using binutils.
-		if err = localSymbolize(mode, p, s.Obj, s.UI); err != nil {
+		if err = localSymbolize(p, fast, force, s.Obj, s.UI); err != nil {
 			s.UI.PrintErr("local symbolization: " + err.Error())
 		}
 	}
 	if remote {
-		if err = symbolzSymbolize(sources, postURL, p, s.UI); err != nil {
+		if err = symbolzSymbolize(p, force, sources, postURL, s.UI); err != nil {
 			return err // Ran out of options.
 		}
 	}
 
-	Demangle(p, force, demanglerMode)
+	demangleFunction(p, force, demanglerMode)
 	return nil
 }
 
@@ -134,18 +139,10 @@ func statusCodeError(resp *http.Response) error {
 // doLocalSymbolize adds symbol and line number information to all locations
 // in a profile. mode enables some options to control
 // symbolization.
-func doLocalSymbolize(mode string, prof *profile.Profile, obj plugin.ObjTool, ui plugin.UI) error {
-	force := false
-	// Disable some mechanisms based on mode string.
-	for _, o := range strings.Split(strings.ToLower(mode), ":") {
-		switch {
-		case o == "force":
-			force = true
-		case o == "fastlocal":
-			if bu, ok := obj.(*binutils.Binutils); ok {
-				bu.SetFastSymbolization(true)
-			}
-		default:
+func doLocalSymbolize(prof *profile.Profile, fast, force bool, obj plugin.ObjTool, ui plugin.UI) error {
+	if fast {
+		if bu, ok := obj.(*binutils.Binutils); ok {
+			bu.SetFastSymbolization(true)
 		}
 	}
 

internal/symbolizer/symbolizer_test.go

@@ -17,6 +17,7 @@ package symbolizer
 import (
 	"fmt"
 	"regexp"
+	"sort"
 	"strings"
 	"testing"
 
@@ -101,9 +102,11 @@ func TestSymbolization(t *testing.T) {
 	defer func() {
 		symbolzSymbolize = sSym
 		localSymbolize = lSym
+		demangleFunction = Demangle
 	}()
 	symbolzSymbolize = symbolzMock
 	localSymbolize = localMock
+	demangleFunction = demangleMock
 
 	type testcase struct {
 		mode        string
@@ -117,19 +120,35 @@ func TestSymbolization(t *testing.T) {
 	for i, tc := range []testcase{
 		{
 			"local",
-			"local=local",
+			"local=[]",
 		},
 		{
 			"fastlocal",
-			"local=fastlocal",
+			"local=[fast]",
 		},
 		{
 			"remote",
-			"symbolz",
+			"symbolz=[]",
 		},
 		{
 			"",
-			"local=:symbolz",
+			"local=[]:symbolz=[]",
+		},
+		{
+			"demangle=none",
+			"demangle=[none]:force:local=[force]:symbolz=[force]",
+		},
+		{
+			"remote:demangle=full",
+			"demangle=[full]:force:symbolz=[force]",
+		},
+		{
+			"local:demangle=templates",
+			"demangle=[templates]:force:local=[force]",
+		},
+		{
+			"force:remote",
+			"force:symbolz=[force]",
 		},
 	} {
 		prof := testProfile.Copy()
@@ -137,23 +156,44 @@ func TestSymbolization(t *testing.T) {
 			t.Errorf("symbolize #%d: %v", i, err)
 			continue
 		}
+		sort.Strings(prof.Comments)
 		if got, want := strings.Join(prof.Comments, ":"), tc.wantComment; got != want {
-			t.Errorf("got %s, want %s", got, want)
+			t.Errorf("%q: got %s, want %s", tc.mode, got, want)
 			continue
 		}
 	}
 }
 
-func symbolzMock(sources plugin.MappingSources, syms func(string, string) ([]byte, error), p *profile.Profile, ui plugin.UI) error {
-	p.Comments = append(p.Comments, "symbolz")
+func symbolzMock(p *profile.Profile, force bool, sources plugin.MappingSources, syms func(string, string) ([]byte, error), ui plugin.UI) error {
+	var args []string
+	if force {
+		args = append(args, "force")
+	}
+	p.Comments = append(p.Comments, "symbolz=["+strings.Join(args, ",")+"]")
 	return nil
 }
 
-func localMock(mode string, p *profile.Profile, obj plugin.ObjTool, ui plugin.UI) error {
-	p.Comments = append(p.Comments, "local="+mode)
+func localMock(p *profile.Profile, fast, force bool, obj plugin.ObjTool, ui plugin.UI) error {
+	var args []string
+	if fast {
+		args = append(args, "fast")
+	}
+	if force {
+		args = append(args, "force")
+	}
+	p.Comments = append(p.Comments, "local=["+strings.Join(args, ",")+"]")
 	return nil
 }
 
+func demangleMock(p *profile.Profile, force bool, mode string) {
+	if force {
+		p.Comments = append(p.Comments, "force")
+	}
+	if mode != "" {
+		p.Comments = append(p.Comments, "demangle=["+mode+"]")
+	}
+}
+
 func TestLocalSymbolization(t *testing.T) {
 	prof := testProfile.Copy()
 
@@ -165,7 +205,7 @@ func TestLocalSymbolization(t *testing.T) {
 	}
 
 	b := mockObjTool{}
-	if err := localSymbolize("", prof, b, &proftest.TestUI{T: t}); err != nil {
+	if err := localSymbolize(prof, false, false, b, &proftest.TestUI{T: t}); err != nil {
 		t.Fatalf("localSymbolize(): %v", err)
 	}
 

internal/symbolz/symbolz.go

@@ -36,12 +36,13 @@ var (
 
 // Symbolize symbolizes profile p by parsing data returned by a
 // symbolz handler. syms receives the symbolz query (hex addresses
-// separated by '+') and returns the symbolz output in a string. It
-// symbolizes all locations based on their addresses, regardless of
-// mapping.
-func Symbolize(sources plugin.MappingSources, syms func(string, string) ([]byte, error), p *profile.Profile, ui plugin.UI) error {
+// separated by '+') and returns the symbolz output in a string. If
+// force is false, it will only symbolize locations from mappings
+// not already marked as HasFunctions.
+func Symbolize(p *profile.Profile, force bool, sources plugin.MappingSources, syms func(string, string) ([]byte, error), ui plugin.UI) error {
 	for _, m := range p.Mapping {
-		if m.HasFunctions {
+		if !force && m.HasFunctions {
+			// Only check for HasFunctions as symbolz only populates function names.
 			continue
 		}
 		mappingSources := sources[m.File]

internal/symbolz/symbolz_test.go

@@ -41,12 +41,49 @@ func TestSymbolzURL(t *testing.T) {
 }
 
 func TestSymbolize(t *testing.T) {
+	s := plugin.MappingSources{
+		"buildid": []struct {
+			Source string
+			Start  uint64
+		}{
+			{Source: "http://localhost:80/profilez"},
+		},
+	}
+
+	for _, hasFunctions := range []bool{false, true} {
+		for _, force := range []bool{false, true} {
+			p := testProfile(hasFunctions)
+
+			if err := Symbolize(p, force, s, fetchSymbols, &proftest.TestUI{T: t}); err != nil {
+				t.Errorf("symbolz: %v", err)
+				continue
+			}
+			var wantSym, wantNoSym []*profile.Location
+			if force || !hasFunctions {
+				wantNoSym = p.Location[:1]
+				wantSym = p.Location[1:]
+			} else {
+				wantNoSym = p.Location
+			}
+
+			if err := checkSymbolized(wantSym, true); err != nil {
+				t.Errorf("symbolz hasFns=%v force=%v: %v", hasFunctions, force, err)
+			}
+			if err := checkSymbolized(wantNoSym, false); err != nil {
+				t.Errorf("symbolz hasFns=%v force=%v: %v", hasFunctions, force, err)
+			}
+		}
+	}
+}
+
+func testProfile(hasFunctions bool) *profile.Profile {
 	m := []*profile.Mapping{
 		{
-			ID:      1,
-			Start:   0x1000,
-			Limit:   0x5000,
-			BuildID: "buildid",
+			ID:           1,
+			Start:        0x1000,
+			Limit:        0x5000,
+			BuildID:      "buildid",
+			HasFunctions: hasFunctions,
 		},
 	}
 	p := &profile.Profile{
@@ -59,33 +96,25 @@ func TestSymbolize(t *testing.T) {
 		Mapping: m,
 	}
 
-	s := plugin.MappingSources{
-		"buildid": []struct {
-			Source string
-			Start  uint64
-		}{
-			{Source: "http://localhost:80/profilez"},
-		},
-	}
-
-	if err := Symbolize(s, fetchSymbols, p, &proftest.TestUI{T: t}); err != nil {
-		t.Errorf("symbolz: %v", err)
-	}
-
-	if l := p.Location[0]; len(l.Line) != 0 {
-		t.Errorf("unexpected symbolization for %#x: %v", l.Address, l.Line)
-	}
+	return p
+}
 
-	for _, l := range p.Location[1:] {
-		if len(l.Line) != 1 {
-			t.Errorf("failed to symbolize %#x", l.Address)
-			continue
+func checkSymbolized(locs []*profile.Location, wantSymbolized bool) error {
+	for _, loc := range locs {
+		if !wantSymbolized && len(loc.Line) != 0 {
+			return fmt.Errorf("unexpected symbolization for %#x: %v", loc.Address, loc.Line)
 		}
-		address := l.Address - l.Mapping.Start
-		if got, want := l.Line[0].Function.Name, fmt.Sprintf("%#x", address); got != want {
-			t.Errorf("symbolz %#x, got %s, want %s", address, got, want)
+		if wantSymbolized {
+			if len(loc.Line) != 1 {
+				return fmt.Errorf("expected symbolization for %#x: %v", loc.Address, loc.Line)
+			}
+			address := loc.Address - loc.Mapping.Start
+			if got, want := loc.Line[0].Function.Name, fmt.Sprintf("%#x", address); got != want {
+				return fmt.Errorf("symbolz %#x, got %s, want %s", address, got, want)
+			}
 		}
 	}
+	return nil
 }
 
 func fetchSymbols(source, post string) ([]byte, error) {