internal/counter: clarify relative line numbers

This CL changes the notation used in stack counters to make
the line numbering scheme more obvious to the reader. We use
"f:+3" to mean "3 lines after func f",
"f:-3" to mean "3 lines before func f", and
"f:=3" to mean "line 3 in the file containing f".

Previously, these were "f:3", "f:?abs" where abs
is the file-relative line number, and "f:??3", all
of which were more confusing to me.

This is a schema change, so we'll be forced to dedup the
new reports, but better to get this in now before the
real flow of crash reports begins.

Also, eliminate unnecessary locals, and remove unnecessary
zeroing of lastImport in f:=%d case.

Change-Id: Ifae4dbd6beee73703529952fa6e7ab1e54c72cd1
Reviewed-on: https://go-review.googlesource.com/c/telemetry/+/561495
Reviewed-by: Peter Weinberger <[email protected]>
Reviewed-by: Robert Findley <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

crashmonitor/monitor_test.go

@@ -74,9 +74,9 @@ func TestViaStderr(t *testing.T) {
 	got = sanitize(counter.DecodeStack(got))
 	want := "crash/crash\n" +
 		"runtime.gopanic:--\n" +
-		"golang.org/x/telemetry/crashmonitor.grandchild:1\n" +
-		"golang.org/x/telemetry/crashmonitor.child:2\n" +
-		"golang.org/x/telemetry/crashmonitor.TestMain:9\n" +
+		"golang.org/x/telemetry/crashmonitor.grandchild:+1\n" +
+		"golang.org/x/telemetry/crashmonitor.child:+2\n" +
+		"golang.org/x/telemetry/crashmonitor.TestMain:+9\n" +
 		"main.main:--\n" +
 		"runtime.main:--\n" +
 		"runtime.goexit:--"
@@ -113,9 +113,9 @@ func TestStart(t *testing.T) {
 		got := sanitize(counter.DecodeStack(string(data)))
 		want := "crash/crash\n" +
 			"runtime.gopanic:--\n" +
-			"golang.org/x/telemetry/crashmonitor.grandchild:1\n" +
-			"golang.org/x/telemetry/crashmonitor.child:2\n" +
-			"golang.org/x/telemetry/crashmonitor.TestMain.func2:1\n" +
+			"golang.org/x/telemetry/crashmonitor.grandchild:+1\n" +
+			"golang.org/x/telemetry/crashmonitor.child:+2\n" +
+			"golang.org/x/telemetry/crashmonitor.TestMain.func2:+1\n" +
 			"runtime.goexit:--"
 		if got != want {
 			t.Errorf("got counter name <<%s>>, want <<%s>>", got, want)

internal/counter/stackcounter.go

@@ -78,34 +78,29 @@ func EncodeStack(pcs []uintptr, prefix string) string {
 	var locs []string
 	lastImport := ""
 	frs := runtime.CallersFrames(pcs)
-	for i := 0; ; i++ {
+	for {
 		fr, more := frs.Next()
-		pcline := fr.Line
-		entryptr := fr.Entry
-		var locline string
 		// TODO(adonovan): this CutLast(".") operation isn't
 		// appropriate for generic function symbols.
 		path, fname := cutLastDot(fr.Function)
 		if path == lastImport {
-			path = "\""
+			path = `"` // (a ditto mark)
 		} else {
 			lastImport = path
 		}
+		var loc string
 		if fr.Func != nil {
-			_, entryline := fr.Func.FileLine(entryptr)
-			if pcline >= entryline {
-				locline = fmt.Sprintf("%s.%s:%d", path, fname, pcline-entryline)
-			} else {
-				// unexpected
-				locline = fmt.Sprintf("%s.%s:??%d", path, fname, pcline)
-				lastImport = ""
-			}
+			// Use function-relative line numbering.
+			// f:+2 means two lines into function f.
+			// f:-1 should never happen, but be conservative.
+			_, entryLine := fr.Func.FileLine(fr.Entry)
+			loc = fmt.Sprintf("%s.%s:%+d", path, fname, fr.Line-entryLine)
 		} else {
-			// might happen if the function is non-Go code or is fully inlined.
-			locline = fmt.Sprintf("%s.%s:?%d", path, fname, pcline)
-			lastImport = ""
+			// The function is non-Go code or is fully inlined:
+			// use absolute line number within enclosing file.
+			loc = fmt.Sprintf("%s.%s:=%d", path, fname, fr.Line)
 		}
-		locs = append(locs, locline)
+		locs = append(locs, loc)
 		if !more {
 			break
 		}