cmd/trace: don't filter events for profile by whether they have stack

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 golang#74850.

Change-Id: I943b97ecf6b82803e4a778a0f83a14473d32254e
Reviewed-on: https://go-review.googlesource.com/c/go/+/694156
Reviewed-by: Rhys Hiltner <[email protected]>
Reviewed-by: Michael Pratt <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Carlos Amedee <[email protected]>

Author: mknyszek

src/cmd/trace/pprof.go

@@ -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()

src/cmd/trace/pprof_test.go

@@ -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)
+}

src/go/build/deps_test.go

@@ -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".

src/internal/trace/testtrace/platforms.go

@@ -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
+}