cmd/pprof: graphviz node names are funny with generics

[bradfitz]

Go 1.18.3.

Not sure what I expect when I use `go tool pprof's web mode to see the graphviz SVG output on a node using generics, but not this:

Either without the newlines, or with the concrete types (if/when available)?

FWIW, that's from:

// Set populates an entry in a map, making the map if necessary.
//
// That is, it assigns (*m)[k] = v, making *m if it was nil.
func Set[K comparable, V any, T ~map[K]V](m *T, k K, v V) {
	if *m == nil {
		*m = make(map[K]V)
	}
	(*m)[k] = v
}

[prattmic]

I wonder if https://github.com/google/pprof/pull/689 will address this. I’m skeptical though, as bad escaping should most likely cause parse errors.

[prattmic]

No, this seems to be different.

In pprof, the type name is main.Set[...] in my test (your package is named mak, right?). pprof or dot (not sure where this lives) replaces . with a newline to separate the package and symbol name onto different lines. This seems to be getting caught up in that rule.

[prattmic]

There is an even worse case with pkg.(*T[...]).f turning into pkg..f, even in the text view. Both of these are covered by https://github.com/google/pprof/issues/705.

[prattmic]

Fix in https://github.com/google/pprof/pull/717

[gopherbot]

Change https://go.dev/cl/420234 mentions this issue: cmd: vendor github.com/google/pprof to fix mangled type parameter symbol names

[odeke-em]

Thank you for filing this bug @bradfitz and for the quick response and fix @prattmic! I think fixing this for Go1.19 is useful as it is just updating the dependency. I have sent a CL vendoring the fix in https://go-review.googlesource.com/c/go/+/420234. @ianlancetaylor @rsc can I kindly implore you to let this fly in the Go1.19 release :-) or perhaps should we wait until the next point release and also backport too?

[prattmic]

cc @golang/release

This would be nice to have in 1.19 or 1.19.1 (arguably backported to 1.18 too), but I am not sure we want to pull this in so close to the release. FWIW, this does have a clear workaround: use upstream pprof.

[prattmic]

This isn't going to make it into 1.19, but we can consider it for 1.19.1 (I'm personally still a bit unsure since there is a workaround).

[dmitshur]

I'll move this issue to Go 1.20 milestone since the fix will need to land on master branch first. To address this issue for minor releases, we'd need to use the https://go.dev/wiki/MinorReleases process to create children backport issues.

[prattmic]

@gopherbot Please backport to 1.19. This fix just barely missed 1.19. While it does have a workaround (use upstream pprof instead of go tool pprof), that is unlikely to be clear to users, and the fix is simple.

[gopherbot]

Backport issue(s) opened: #54420 (for 1.19).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

[gopherbot]

Change https://go.dev/cl/423356 mentions this issue: [release-branch.go1.19] cmd: vendor github.com/google/pprof to fix mangled type parameter symbol names

[dmitshur]

@prattmic The backporting approach described in https://github.com/golang/go/issues/34536#issuecomment-572188389 suggests we'll need to also backport the fix to 1.18, since the problem also exists in that version.

[prattmic]

Sure, I can do so. Do we have a standard process for backporting one-off fixes to vendored packages, rather than wholesale updates? I can easily manually apply the appropriate changes to the vendored pprof files, but what should go.mod say? The vendored code will no longer be any valid upstream version of github.com/google/pprof.

[prattmic]

I don't see any such similar cases on https://go-review.git.corp.google.com/q/project:go+is:merged+file:vendor+-branch:master+-branch:dev.link+-branch:dev.boringcrypto+-branch:dev.typeparams+-branch:dev.fuzz. :(

A full update of pprof on release-branch.go1.18 will pull in 20 commits:

$ git log --oneline f987b9c94b31..154dc81eb7b0
154dc81 all: update dependencies (#702)
59ca7ad Generalize the unit support in pprof a bit further. (#699)
83db2b7 Split monolithic webhtml.go into multiple files. (#695)
fdd30d9 Update minimum Go version to 1.17 in go.mod. (#693)
d8f96c0 Fix doc comments format to become compatible with tip gofmt. (#694)
b5a4dc8 allow rendering big flame graphs by avoiding stack overflow in JS parser (#684)
85950bb Fix tagroot to properly format unitless numeric tags. (#691)
c2158d7 Add Go 1.18 to testing, remove Go 1.16. (#692)
b2ab032 third_party: fix typo (#685)
5bba342 internal/graph: Support comments with double quotes (#688)
0368bd9 Handle either _text or _stext as the kernel relocation symbol. (#674)
43b8053 Parse and propagate the name of the kernel relocation symbol (#675)
513e8ac doc: clarify graph view docs to note negative values appear in profile comparison (#667)
8c355e5 internal/elfexec: Fix typos in elfexec.go (#681)
d25a53d Log build ID in local symbolization error messages. (#679)
6f57359 Update d3-flame-graph from 2.0.0-alpha to 4.1.3 (#677)
2007db6 Add badge link to Go API docs in pkg.go.dev (#676)
1daafda Update instructions to use "git clone" instead of "go get". (#673)
44fc4e8 proto/profile.proto: fix typo (#672)
e9b0287 Update mapassign regex to match both call variants (#668)

[dmitshur]

Do we have a standard process for backporting one-off fixes to vendored packages, rather than wholesale updates?

We have a standard process that applies to golang.org/x dependencies. It uses "internal-branch.go1.x-vendor" branches and is documented at https://go.dev/wiki/MinorReleases#cherry-pick-cls-for-vendored-golangorgx-packages.

This is the first backport request for a problem in the github.com/google/pprof dependency that I'm seeing, so not sure. Depending on how much the vendored github.com/google/pprof interacts with cmd/pprof and other parts of the standard library, it might be safest to update it to the same v0.0.0-20220729232143-a41b82acbcb1 version (that way, Go 1.19 and 1.18 will be using the same version). If that isn't safe, we'll need to figure out what to do instead.

[prattmic]

I can take a closer look tomorrow, but I don't think there are any technical blockers (or any changes to non-vendored code required) to updating pprof to v0.0.0-20220729232143-a41b82acbcb1 in 1.18, it's just that pulling in a bunch of unrelated commits has higher risk of introducing other issues. Sounds like taking that risk is the best bet right now.

[prattmic]

@gopherbot Please backport to 1.18. While it does have a workaround (use upstream pprof instead of go tool pprof), that is unlikely to be clear to users.

[gopherbot]

Change https://go.dev/cl/423576 mentions this issue: [release-branch.go1.18] cmd: upgrade github.com/google/pprof to v0.0.0-20220729232143-a41b82acbcb1