]> Cypherpunks repositories - gostls13.git/commitdiff
cmd/trace: don't filter events for profile by whether they have stack
authorMichael Anthony Knyszek <mknyszek@google.com>
Thu, 7 Aug 2025 18:53:00 +0000 (18:53 +0000)
committerMichael Knyszek <mknyszek@google.com>
Thu, 4 Sep 2025 18:12:32 +0000 (11:12 -0700)
Right now the profile-from-trace code blindly discards events that don't
have a stack, but this means it can discard 'end' events for goroutine
time ranges that don't have stacks, like when a goroutine exits a
syscall. This means we drop stack samples we *do* have, because we
correctly already only use the stack trace of the corresponding 'start'
event for a time-range-of-interest anyway.

This change means that some events will be tracked that have no stack in
their start event, but that's fine. It won't end up in the profile
anyway because the stack is empty! And the rest of the code appears to
be robust to an empty stack already.

Thank you to Rhys Hiltner for reporting this issue and for the
reproducer, which I have worked into a test for this change.

Fixes #74850.

Change-Id: I943b97ecf6b82803e4a778a0f83a14473d32254e
Reviewed-on: https://go-review.googlesource.com/c/go/+/694156
Reviewed-by: Rhys Hiltner <rhys.hiltner@gmail.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Carlos Amedee <carlos@golang.org>
src/cmd/trace/pprof.go
src/cmd/trace/pprof_test.go [new file with mode: 0644]
src/go/build/deps_test.go
src/internal/trace/testtrace/platforms.go [new file with mode: 0644]

index a66419aedf764e614218e0034be62bf5cb3a571f..b472ffa759114b6cc8b6f3da511e66a2804705b5 100644 (file)
@@ -153,10 +153,6 @@ func makeComputePprofFunc(state trace.GoState, trackReason func(string) bool) co
                        if ev.Kind() != trace.EventStateTransition {
                                continue
                        }
-                       stack := ev.Stack()
-                       if stack == trace.NoStack {
-                               continue
-                       }
 
                        // The state transition has to apply to a goroutine.
                        st := ev.StateTransition()
diff --git a/src/cmd/trace/pprof_test.go b/src/cmd/trace/pprof_test.go
new file mode 100644 (file)
index 0000000..6d18e7d
--- /dev/null
@@ -0,0 +1,103 @@
+// Copyright 2025 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package main
+
+import (
+       "bytes"
+       "net/http"
+       "os"
+       "runtime/trace"
+       "strings"
+       "testing"
+       "testing/synctest"
+       "time"
+
+       "internal/trace/testtrace"
+)
+
+// Regression test for go.dev/issue/74850.
+func TestSyscallProfile74850(t *testing.T) {
+       testtrace.MustHaveSyscallEvents(t)
+
+       var buf bytes.Buffer
+       err := trace.Start(&buf)
+       if err != nil {
+               t.Fatalf("start tracing: %v", err)
+       }
+
+       synctest.Test(t, func(t *testing.T) {
+               go hidden1(t)
+               go hidden2(t)
+               go visible(t)
+               synctest.Wait()
+               time.Sleep(1 * time.Millisecond)
+               synctest.Wait()
+       })
+       trace.Stop()
+
+       if t.Failed() {
+               return
+       }
+
+       parsed, err := parseTrace(&buf, int64(buf.Len()))
+       if err != nil {
+               t.Fatalf("parsing trace: %v", err)
+       }
+
+       records, err := pprofByGoroutine(computePprofSyscall(), parsed)(&http.Request{})
+       if err != nil {
+               t.Fatalf("failed to generate pprof: %v\n", err)
+       }
+
+       for _, r := range records {
+               t.Logf("Record: n=%d, total=%v", r.Count, r.Time)
+               for _, f := range r.Stack {
+                       t.Logf("\t%s", f.Func)
+                       t.Logf("\t\t%s:%d @ 0x%x", f.File, f.Line, f.PC)
+               }
+       }
+       if len(records) == 0 {
+               t.Error("empty profile")
+       }
+
+       // Make sure we see the right frames.
+       wantSymbols := []string{"cmd/trace.visible", "cmd/trace.hidden1", "cmd/trace.hidden2"}
+       haveSymbols := make([]bool, len(wantSymbols))
+       for _, r := range records {
+               for _, f := range r.Stack {
+                       for i, s := range wantSymbols {
+                               if strings.Contains(f.Func, s) {
+                                       haveSymbols[i] = true
+                               }
+                       }
+               }
+       }
+       for i, have := range haveSymbols {
+               if !have {
+                       t.Errorf("expected %s in syscall profile", wantSymbols[i])
+               }
+       }
+}
+
+func stat(t *testing.T) {
+       _, err := os.Stat(".")
+       if err != nil {
+               t.Errorf("os.Stat: %v", err)
+       }
+}
+
+func hidden1(t *testing.T) {
+       stat(t)
+}
+
+func hidden2(t *testing.T) {
+       stat(t)
+       stat(t)
+}
+
+func visible(t *testing.T) {
+       stat(t)
+       time.Sleep(1 * time.Millisecond)
+}
index 41dde20bf9cc9b3234210823ae874fcd50e20361..d50a98d34c9092c2efd2446599ea0a0d0436c463 100644 (file)
@@ -742,12 +742,6 @@ var depsRules = `
        FMT, encoding/binary, internal/trace/version, internal/trace/internal/tracev1, container/heap, math/rand
        < internal/trace;
 
-       regexp, internal/trace, internal/trace/raw, internal/txtar
-       < internal/trace/testtrace;
-
-       regexp, internal/txtar, internal/trace, internal/trace/raw
-       < internal/trace/internal/testgen;
-
        # cmd/trace dependencies.
        FMT,
        embed,
@@ -792,14 +786,24 @@ var depsRules = `
        < testing/internal/testdeps;
 
        # Test-only packages can have anything they want
-       FMT, compress/gzip, embed, encoding/binary < encoding/json/internal/jsontest;
-       CGO, internal/syscall/unix < net/internal/cgotest;
-       FMT < math/big/internal/asmgen;
 
-       FMT, testing < internal/cgrouptest;
-       C, CGO < internal/runtime/cgobench;
+       FMT, compress/gzip, embed, encoding/binary
+       < encoding/json/internal/jsontest;
+
+       CGO, internal/syscall/unix
+       < net/internal/cgotest;
+
+       FMT, testing
+       < internal/cgrouptest;
+
+       regexp, internal/trace, internal/trace/raw, internal/txtar, testing
+       < internal/trace/testtrace;
+
+       C, CGO
+       < internal/runtime/cgobench;
+
+       # Generate-only packages can have anything they want.
 
-       # Generate-only packages can have anything they want
        container/heap,
        encoding/binary,
        fmt,
@@ -812,6 +816,12 @@ var depsRules = `
        strings,
        sync
        < internal/runtime/gc/internal/gen;
+
+       regexp, internal/txtar, internal/trace, internal/trace/raw
+       < internal/trace/internal/testgen;
+
+       FMT
+       < math/big/internal/asmgen;
 `
 
 // listStdPkgs returns the same list of packages as "go list std".
diff --git a/src/internal/trace/testtrace/platforms.go b/src/internal/trace/testtrace/platforms.go
new file mode 100644 (file)
index 0000000..937a2dc
--- /dev/null
@@ -0,0 +1,32 @@
+// Copyright 2025 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package testtrace
+
+import (
+       "runtime"
+       "testing"
+)
+
+// MustHaveSyscallEvents skips the current test if the current
+// platform does not support true system call events.
+func MustHaveSyscallEvents(t *testing.T) {
+       if HasSyscallEvents() {
+               return
+       }
+       t.Skip("current platform has no true syscall events")
+}
+
+// HasSyscallEvents returns true if the current platform
+// has real syscall events available.
+func HasSyscallEvents() bool {
+       switch runtime.GOOS {
+       case "js", "wasip1":
+               // js and wasip1 emulate system calls by blocking on channels
+               // while yielding back to the environment. They never actually
+               // call entersyscall/exitsyscall.
+               return false
+       }
+       return true
+}