From e1c0349a7c607cdfcfa8f7c0c6b70aceff9d3752 Mon Sep 17 00:00:00 2001 From: Michael Pratt Date: Wed, 6 Dec 2023 13:29:03 -0500 Subject: internal/profile: fully decode proto even if there are no samples This is a partial revert of CL 483137. CL 483137 started checking errors in postDecode, which is good. Now we can catch more malformed pprof protos. However this made TestEmptyProfile fail, so an early return was added when the profile was "empty" (no samples). Unfortunately, this was problematic. Profiles with no samples can still be valid, but skipping postDecode meant that the resulting Profile was missing values from the string table. In particular, net/http/pprof needs to parse empty profiles in order to pass through the sample and period types to a final output proto. CL 483137 broke this behavior. internal/profile.Parse is only used in two places: in cmd/compile to parse PGO pprof profiles, and in net/http/pprof to parse before/after pprof profiles for delta profiles. In both cases, the input is never literally empty (0 bytes). Even a pprof proto with no samples still contains some header fields, such as sample and period type. Upstream github.com/google/pprof/profile even has an explicit error on 0 byte input, so `go tool pprof` will not support such an input. Thus TestEmptyProfile was misleading; this profile doesn't need to support empty input at all. Resolve this by removing TestEmptyProfile and replacing it with an explicit error on empty input, as upstream github.com/google/pprof/profile has. For non-empty input, always run postDecode to ensure the string table is processed. TestConvertCPUProfileEmpty is reverted back to assert the values from before CL 483137. Note that in this case "Empty" means no samples, not a 0 byte input. Continue to allow empty files for PGO in order to minimize the chance of last minute breakage if some users have empty files. Fixes #64566. Change-Id: I83a1f0200ae225ac6da0009d4b2431fe215b283f Reviewed-on: https://go-review.googlesource.com/c/go/+/547996 Reviewed-by: Michael Knyszek LUCI-TryBot-Result: Go LUCI Reviewed-by: Cherry Mui --- src/runtime/pprof/proto_test.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) (limited to 'src/runtime') diff --git a/src/runtime/pprof/proto_test.go b/src/runtime/pprof/proto_test.go index e1a7f2306d..85cd066946 100644 --- a/src/runtime/pprof/proto_test.go +++ b/src/runtime/pprof/proto_test.go @@ -45,7 +45,7 @@ func fmtJSON(x any) string { return string(js) } -func TestConvertCPUProfileEmpty(t *testing.T) { +func TestConvertCPUProfileNoSamples(t *testing.T) { // A test server with mock cpu profile data. var buf bytes.Buffer @@ -64,9 +64,13 @@ func TestConvertCPUProfileEmpty(t *testing.T) { } // Expected PeriodType and SampleType. - sampleType := []*profile.ValueType{{}, {}} + periodType := &profile.ValueType{Type: "cpu", Unit: "nanoseconds"} + sampleType := []*profile.ValueType{ + {Type: "samples", Unit: "count"}, + {Type: "cpu", Unit: "nanoseconds"}, + } - checkProfile(t, p, 2000*1000, nil, sampleType, nil, "") + checkProfile(t, p, 2000*1000, periodType, sampleType, nil, "") } func f1() { f1() } -- cgit v1.3