runtime/pprof: improve output of TestLabelSystemstack

The current output of TestLabelSystemstack is a bit cryptic. This CL
improves various messages and hopefully simplifies the logic in the
test.

Simplifying the logic leads to three changes in possible outcomes,
which I verified by running the logic before and after this change
through all 2^4 possibilities (https://go.dev/play/p/bnfb-OQCT4j):

1. If a sample both must be labeled and must not be labeled, the test
now reports that explicitly rather than giving other confusing output.

2. If a sample must not be labeled but is, the current logic will
print two identical error messages. The new logic prints only one.

3. If the test finds no frames at all that it recognizes, but the
sample is labeled, it will currently print a confusing "Sample labeled
got true want false" message. The new logic prints nothing. We've seen
this triggered by empty stacks in profiles.

Fixes #51550. This bug was caused by case 3 above, where it was
triggered by a profile label on an empty stack. It's valid for empty
stacks to appear in a profile if we sample a goroutine just as it's
exiting (and that goroutine may have a profile label), so the test
shouldn't fail in this case.

Change-Id: I1593ec4ac33eced5bb89572a3ba7623e56f2fb3d
Reviewed-on: https://go-review.googlesource.com/c/go/+/460516
Run-TryBot: Austin Clements <austin@google.com>
Reviewed-by: Felix Geisendörfer <felix.geisendoerfer@datadoghq.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>

diff --git a/src/runtime/pprof/pprof_test.go b/src/runtime/pprof/pprof_test.go
index 39bc9d11f1e4e90cfd8dff0e5626df78fe6bfacf..53688ad825b514d7aac1a4a659dc0b4f573ca502 100644 (file)
--- a/src/runtime/pprof/pprof_test.go
+++ b/src/runtime/pprof/pprof_test.go
@@ -607,7 +607,7 @@ func matchAndAvoidStacks(matches sampleMatchFunc, need []string, avoid []string)
                var total uintptr
                for i, name := range need {
                        total += have[i]
-                       t.Logf("%s: %d\n", name, have[i])
+                       t.Logf("found %d samples in expected function %s\n", have[i], name)
                }
                if total == 0 {
                        t.Logf("no samples in expected functions")
@@ -727,6 +727,9 @@ func TestGoroutineSwitch(t *testing.T) {
 }
 
 func fprintStack(w io.Writer, stk []*profile.Location) {
+       if len(stk) == 0 {
+               fmt.Fprintf(w, " (stack empty)")
+       }
        for _, loc := range stk {
                fmt.Fprintf(w, " %#x", loc.Address)
                fmt.Fprintf(w, " (")
@@ -1867,14 +1870,14 @@ func TestLabelSystemstack(t *testing.T) {
                isLabeled := s.Label != nil && contains(s.Label["key"], "value")
                var (
                        mayBeLabeled     bool
-                       mustBeLabeled    bool
-                       mustNotBeLabeled bool
+                       mustBeLabeled    string
+                       mustNotBeLabeled string
                )
                for _, loc := range s.Location {
                        for _, l := range loc.Line {
                                switch l.Function.Name {
                                case "runtime/pprof.labelHog", "runtime/pprof.parallelLabelHog", "runtime/pprof.parallelLabelHog.func1":
-                                       mustBeLabeled = true
+                                       mustBeLabeled = l.Function.Name
                                case "runtime/pprof.Do":
                                        // Do sets the labels, so samples may
                                        // or may not be labeled depending on
@@ -1886,7 +1889,7 @@ func TestLabelSystemstack(t *testing.T) {
                                        // (such as those identified by
                                        // runtime.isSystemGoroutine). These
                                        // should never be labeled.
-                                       mustNotBeLabeled = true
+                                       mustNotBeLabeled = l.Function.Name
                                case "gogo", "gosave_systemstack_switch", "racecall":
                                        // These are context switch/race
                                        // critical that we can't do a full
@@ -1908,25 +1911,28 @@ func TestLabelSystemstack(t *testing.T) {
                                }
                        }
                }
-               if mustNotBeLabeled {
-                       // If this must not be labeled, then mayBeLabeled hints
-                       // are not relevant.
-                       mayBeLabeled = false
-               }
-               if mustBeLabeled && !isLabeled {
+               errorStack := func(f string, args ...any) {
                        var buf strings.Builder
                        fprintStack(&buf, s.Location)
-                       t.Errorf("Sample labeled got false want true: %s", buf.String())
+                       t.Errorf("%s: %s", fmt.Sprintf(f, args...), buf.String())
                }
-               if mustNotBeLabeled && isLabeled {
-                       var buf strings.Builder
-                       fprintStack(&buf, s.Location)
-                       t.Errorf("Sample labeled got true want false: %s", buf.String())
+               if mustBeLabeled != "" && mustNotBeLabeled != "" {
+                       errorStack("sample contains both %s, which must be labeled, and %s, which must not be labeled", mustBeLabeled, mustNotBeLabeled)
+                       continue
                }
-               if isLabeled && !(mayBeLabeled || mustBeLabeled) {
-                       var buf strings.Builder
-                       fprintStack(&buf, s.Location)
-                       t.Errorf("Sample labeled got true want false: %s", buf.String())
+               if mustBeLabeled != "" || mustNotBeLabeled != "" {
+                       // We found a definitive frame, so mayBeLabeled hints are not relevant.
+                       mayBeLabeled = false
+               }
+               if mayBeLabeled {
+                       // This sample may or may not be labeled, so there's nothing we can check.
+                       continue
+               }
+               if mustBeLabeled != "" && !isLabeled {
+                       errorStack("sample must be labeled because of %s, but is not", mustBeLabeled)
+               }
+               if mustNotBeLabeled != "" && isLabeled {
+                       errorStack("sample must not be labeled because of %s, but is", mustNotBeLabeled)
                }
        }
 }