Add column numbers to profile.proto.

This change adds column numbers to the line message in profile.proto. This allows users to distinguish between souce code locations on the same line. Only the llvm addr2line is capable of reading column number information from dwarf debug information. Other changes include: * Add "columns" option for output granularity. * Account for column numbers during profile merge. * Update the encoder and decoder. * Update golden test files for legacy profiles. Fixes #687
google · Nov 16, 2023 · 3d8bf76 · 3d8bf76
1 parent 4ca4178
commit 3d8bf76
Show file tree

Hide file tree

Showing 21 changed files with 401 additions and 284 deletions.
diff --git a/driver/driver.go b/driver/driver.go
@@ -186,9 +186,10 @@ type ObjFile interface {
 
 // A Frame describes a single line in a source file.
 type Frame struct {
-	Func string // name of function
-	File string // source file name
-	Line int    // line in file
+	Func   string // name of function
+	File   string // source file name
+	Line   int    // line in file
+	Column int    // column in file
 }
 
 // A Sym describes a single symbol in an object file.

diff --git a/internal/binutils/addr2liner_llvm.go b/internal/binutils/addr2liner_llvm.go
@@ -129,6 +129,7 @@ func (d *llvmSymbolizer) readFrame() (plugin.Frame, bool) {
 	}
 
 	linenumber := 0
+	columnnumber := 0
 	// The llvm-symbolizer outputs the <file_name>:<line_number>:<column_number>.
 	// When it cannot identify the source code location, it outputs "??:0:0".
 	// Older versions output just the filename and line number, so we check for
@@ -137,22 +138,27 @@ func (d *llvmSymbolizer) readFrame() (plugin.Frame, bool) {
 		fileline = ""
 	} else {
 		switch split := strings.Split(fileline, ":"); len(split) {
-		case 1:
-			// filename
-			fileline = split[0]
-		case 2, 3:
-			// filename:line , or
-			// filename:line:disc , or
-			fileline = split[0]
+		case 3:
+			// filename:line:column
+			if col, err := strconv.Atoi(split[2]); err == nil {
+				columnnumber = col
+			}
+			fallthrough
+		case 2:
+			// filename:line
 			if line, err := strconv.Atoi(split[1]); err == nil {
 				linenumber = line
 			}
+			fallthrough
+		case 1:
+			// filename
+			fileline = split[0]
 		default:
 			// Unrecognized, ignore
 		}
 	}
 
-	return plugin.Frame{Func: funcname, File: fileline, Line: linenumber}, false
+	return plugin.Frame{Func: funcname, File: fileline, Line: linenumber, Column: columnnumber}, false
 }
 
 // addrInfo returns the stack frame information for a specific program

diff --git a/internal/binutils/binutils_test.go b/internal/binutils/binutils_test.go
@@ -461,8 +461,8 @@ func TestLLVMSymbolizer(t *testing.T) {
 		frames []plugin.Frame
 	}{
 		{0x10, false, []plugin.Frame{
-			{Func: "Inlined_0x10", File: "foo.h", Line: 0},
-			{Func: "Func_0x10", File: "foo.c", Line: 2},
+			{Func: "Inlined_0x10", File: "foo.h", Line: 0, Column: 0},
+			{Func: "Func_0x10", File: "foo.c", Line: 2, Column: 1},
 		}},
 		{0x20, true, []plugin.Frame{
 			{Func: "foo_0x20", File: "0x20 8"},

diff --git a/internal/driver/commands.go b/internal/driver/commands.go
@@ -247,6 +247,8 @@ var configHelp = map[string]string{
 	"noinlines": helpText(
 		"Ignore inlines.",
 		"Attributes inlined functions to their first out-of-line caller."),
+	"columns": helpText(
+		"Use column numbers when aggregating at the source code line level."),
 }
 
 func helpText(s ...string) string {

diff --git a/internal/driver/config.go b/internal/driver/config.go
@@ -51,6 +51,7 @@ type config struct {
 	TagShow      string  `json:"tagshow,omitempty"`
 	TagHide      string  `json:"taghide,omitempty"`
 	NoInlines    bool    `json:"noinlines,omitempty"`
+	Columns      bool    `json:"columns,omitempty"`
 
 	// Output granularity
 	Granularity string `json:"granularity,omitempty"`
@@ -124,7 +125,7 @@ func init() {
 	// take on one of a bounded set of values.
 	choices := map[string][]string{
 		"sort":        {"cum", "flat"},
-		"granularity": {"functions", "filefunctions", "files", "lines", "addresses"},
+		"granularity": {"functions", "filefunctions", "files", "lines", "columns", "addresses"},
 	}
 
 	// urlparam holds the mapping from a config field name to the URL
@@ -157,6 +158,7 @@ func init() {
 		"sort":                 "sort",
 		"granularity":          "g",
 		"noinlines":            "noinlines",
+		"columns":              "columns",
 	}
 
 	def := defaultConfig()

diff --git a/internal/driver/driver.go b/internal/driver/driver.go
@@ -231,7 +231,7 @@ func dropEmptyStrings(in []string) (out []string) {
 }
 
 func aggregate(prof *profile.Profile, cfg config) error {
-	var function, filename, linenumber, address bool
+	var function, filename, linenumber, columnnumber, address bool
 	inlines := !cfg.NoInlines
 	switch cfg.Granularity {
 	case "addresses":
@@ -246,6 +246,11 @@ func aggregate(prof *profile.Profile, cfg config) error {
 		function = true
 		filename = true
 		linenumber = true
+	case "columns":
+		function = true
+		filename = true
+		linenumber = true
+		columnnumber = true
 	case "files":
 		filename = true
 	case "functions":
@@ -256,7 +261,7 @@ func aggregate(prof *profile.Profile, cfg config) error {
 	default:
 		return fmt.Errorf("unexpected granularity")
 	}
-	return prof.Aggregate(inlines, function, filename, linenumber, address)
+	return prof.Aggregate(inlines, function, filename, linenumber, columnnumber, address)
 }
 
 func reportOptions(p *profile.Profile, numLabelUnits map[string]string, cfg config) (*report.Options, error) {

diff --git a/internal/driver/driver_test.go b/internal/driver/driver_test.go
@@ -1680,8 +1680,9 @@ func (m *mockFile) BuildID() string {
 func (*mockFile) SourceLine(addr uint64) ([]plugin.Frame, error) {
 	// Return enough data to support the SourceLine() calls needed for
 	// weblist on cpuProfile() contents.
-	frame := func(fn, file string, line int) plugin.Frame {
-		return plugin.Frame{Func: fn, File: file, Line: line}
+	frame := func(fn, file string, num int) plugin.Frame {
+		// Reuse the same num for line number and column number.
+		return plugin.Frame{Func: fn, File: file, Line: num, Column: num}
 	}
 	switch addr {
 	case 0x1000:

diff --git a/internal/driver/settings_test.go b/internal/driver/settings_test.go
@@ -86,6 +86,7 @@ func TestParseConfig(t *testing.T) {
 		Sort:                "cum",
 		Granularity:         "functions",
 		NoInlines:           true,
+		Columns:             true,
 	}
 	url, changed := cfg.makeURL(url.URL{})
 	if !changed {

diff --git a/internal/plugin/plugin.go b/internal/plugin/plugin.go
@@ -157,11 +157,12 @@ type ObjFile interface {
 	Close() error
 }
 
-// A Frame describes a single line in a source file.
+// A Frame describes a location in a single line in a source file.
 type Frame struct {
-	Func string // name of function
-	File string // source file name
-	Line int    // line in file
+	Func   string // name of function
+	File   string // source file name
+	Line   int    // line in file
+	Column int    // column in line (if available)
 }
 
 // A Sym describes a single symbol in an object file.

diff --git a/internal/symbolizer/symbolizer.go b/internal/symbolizer/symbolizer.go
@@ -181,6 +181,7 @@ func doLocalSymbolize(prof *profile.Profile, fast, force bool, obj plugin.ObjToo
 			l.Line[i] = profile.Line{
 				Function: f,
 				Line:     int64(frame.Line),
+				Column:   int64(frame.Column),
 			}
 		}
 

diff --git a/internal/symbolizer/symbolizer_test.go b/internal/symbolizer/symbolizer_test.go
@@ -242,23 +242,27 @@ func checkSymbolizedLocation(a uint64, got []profile.Line) error {
 		if g.Line != int64(w.Line) {
 			return fmt.Errorf("want lineno: %d, got %d", w.Line, g.Line)
 		}
+		if g.Column != int64(w.Column) {
+			return fmt.Errorf("want column: %d, got %d", w.Column, g.Column)
+		}
 	}
 	return nil
 }
 
 var mockAddresses = map[uint64][]plugin.Frame{
-	1000: {frame("fun11", "file11.src", 10)},
-	2000: {frame("fun21", "file21.src", 20), frame("fun22", "file22.src", 20)},
-	3000: {frame("fun31", "file31.src", 30), frame("fun32", "file32.src", 30), frame("fun33", "file33.src", 30)},
-	4000: {frame("fun41", "file41.src", 40), frame("fun42", "file42.src", 40), frame("fun43", "file43.src", 40), frame("fun44", "file44.src", 40)},
-	5000: {frame("fun51", "file51.src", 50), frame("fun52", "file52.src", 50), frame("fun53", "file53.src", 50), frame("fun54", "file54.src", 50), frame("fun55", "file55.src", 50)},
+	1000: {frame("fun11", "file11.src", 10, 1)},
+	2000: {frame("fun21", "file21.src", 20, 2), frame("fun22", "file22.src", 20, 2)},
+	3000: {frame("fun31", "file31.src", 30, 3), frame("fun32", "file32.src", 30, 3), frame("fun33", "file33.src", 30, 3)},
+	4000: {frame("fun41", "file41.src", 40, 4), frame("fun42", "file42.src", 40, 4), frame("fun43", "file43.src", 40, 4), frame("fun44", "file44.src", 40, 4)},
+	5000: {frame("fun51", "file51.src", 50, 5), frame("fun52", "file52.src", 50, 5), frame("fun53", "file53.src", 50, 5), frame("fun54", "file54.src", 50, 5), frame("fun55", "file55.src", 50, 5)},
 }
 
-func frame(fname, file string, line int) plugin.Frame {
+func frame(fname, file string, line int, column int) plugin.Frame {
 	return plugin.Frame{
-		Func: fname,
-		File: file,
-		Line: line}
+		Func:   fname,
+		File:   file,
+		Line:   line,
+		Column: column}
 }
 
 func TestDemangleSingleFunction(t *testing.T) {

diff --git a/profile/encode.go b/profile/encode.go
@@ -530,6 +530,7 @@ func (p *Line) decoder() []decoder {
 func (p *Line) encode(b *buffer) {
 	encodeUint64Opt(b, 1, p.functionIDX)
 	encodeInt64Opt(b, 2, p.Line)
+	encodeInt64Opt(b, 3, p.Column)
 }
 
 var lineDecoder = []decoder{
@@ -538,6 +539,8 @@ var lineDecoder = []decoder{
 	func(b *buffer, m message) error { return decodeUint64(b, &m.(*Line).functionIDX) },
 	// optional int64 line = 2
 	func(b *buffer, m message) error { return decodeInt64(b, &m.(*Line).Line) },
+	// optional int64 column = 3
+	func(b *buffer, m message) error { return decodeInt64(b, &m.(*Line).Column) },
 }
 
 func (p *Function) decoder() []decoder {

diff --git a/profile/legacy_java_profile.go b/profile/legacy_java_profile.go
@@ -56,7 +56,7 @@ func javaCPUProfile(b []byte, period int64, parse func(b []byte) (uint64, []byte
 	}
 
 	// Strip out addresses for better merge.
-	if err = p.Aggregate(true, true, true, true, false); err != nil {
+	if err = p.Aggregate(true, true, true, true, false, false); err != nil {
 		return nil, err
 	}
 
@@ -99,7 +99,7 @@ func parseJavaProfile(b []byte) (*Profile, error) {
 	}
 
 	// Strip out addresses for better merge.
-	if err = p.Aggregate(true, true, true, true, false); err != nil {
+	if err = p.Aggregate(true, true, true, true, false, false); err != nil {
 		return nil, err
 	}
 

diff --git a/profile/merge.go b/profile/merge.go
@@ -326,12 +326,13 @@ func (l *Location) key() locationKey {
 		key.addr -= l.Mapping.Start
 		key.mappingID = l.Mapping.ID
 	}
-	lines := make([]string, len(l.Line)*2)
+	lines := make([]string, len(l.Line)*3)
 	for i, line := range l.Line {
 		if line.Function != nil {
 			lines[i*2] = strconv.FormatUint(line.Function.ID, 16)
 		}
 		lines[i*2+1] = strconv.FormatInt(line.Line, 16)
+		lines[i*2+2] = strconv.FormatInt(line.Column, 16)
 	}
 	key.lines = strings.Join(lines, "|")
 	return key
@@ -418,6 +419,7 @@ func (pm *profileMerger) mapLine(src Line) Line {
 	ln := Line{
 		Function: pm.mapFunction(src.Function),
 		Line:     src.Line,
+		Column:   src.Column,
 	}
 	return ln
 }

diff --git a/profile/profile.go b/profile/profile.go
@@ -145,6 +145,7 @@ type Location struct {
 type Line struct {
 	Function *Function
 	Line     int64
+	Column   int64
 
 	functionIDX uint64
 }
@@ -436,7 +437,7 @@ func (p *Profile) CheckValid() error {
 // Aggregate merges the locations in the profile into equivalence
 // classes preserving the request attributes. It also updates the
 // samples to point to the merged locations.
-func (p *Profile) Aggregate(inlineFrame, function, filename, linenumber, address bool) error {
+func (p *Profile) Aggregate(inlineFrame, function, filename, linenumber, columnnumber, address bool) error {
 	for _, m := range p.Mapping {
 		m.HasInlineFrames = m.HasInlineFrames && inlineFrame
 		m.HasFunctions = m.HasFunctions && function
@@ -458,14 +459,20 @@ func (p *Profile) Aggregate(inlineFrame, function, filename, linenumber, address
 	}
 
 	// Aggregate locations
-	if !inlineFrame || !address || !linenumber {
+	if !inlineFrame || !address || !linenumber || !columnnumber {
 		for _, l := range p.Location {
 			if !inlineFrame && len(l.Line) > 1 {
 				l.Line = l.Line[len(l.Line)-1:]
 			}
 			if !linenumber {
 				for i := range l.Line {
 					l.Line[i].Line = 0
+					l.Line[i].Column = 0
+				}
+			}
+			if !columnnumber {
+				for i := range l.Line {
+					l.Line[i].Column = 0
 				}
 			}
 			if !address {
@@ -627,10 +634,11 @@ func (l *Location) string() string {
 	for li := range l.Line {
 		lnStr := "??"
 		if fn := l.Line[li].Function; fn != nil {
-			lnStr = fmt.Sprintf("%s %s:%d s=%d",
+			lnStr = fmt.Sprintf("%s %s:%d:%d s=%d",
 				fn.Name,
 				fn.Filename,
 				l.Line[li].Line,
+				l.Line[li].Column,
 				fn.StartLine)
 			if fn.Name != fn.SystemName {
 				lnStr = lnStr + "(" + fn.SystemName + ")"