internal/report: fall back to $GOROOT/src when locating source files - #1010
internal/report: fall back to $GOROOT/src when locating source files#1010jespino wants to merge 3 commits into
Conversation
Profiles from Go programs refer to standard library sources under the GOROOT of the build machine, which breaks the source and weblist views when that path does not exist locally (e.g. a profile from another machine), or when the program was built with -trimpath and the files are recorded relative to $GOROOT/src. Teach openSourceFile to fall back to the local $GOROOT/src after the regular search fails: relative paths resolve against it directly, and absolute paths resolve by matching the components after a /src/ one. The fallback only runs when the existing lookup finds nothing, so previously resolved files are unaffected. Also introduce tryOpenFile, which rejects directories that os.Open would happily open, so a directory matching a candidate path no longer wins the search and then fails when its lines are read. Co-authored-by: Aliaksandr Valialkin <valyala@victoriametrics.com>
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
| parent := filepath.Dir(dir) | ||
| if parent == dir { | ||
| break | ||
| if f, err := tryOpenFile(path); err == nil { |
There was a problem hiding this comment.
All this changes are round allowing to fallback later to other "handlers", for now we are adding the goroot handler, but in other PRs we will add more special handlers like gomodcache or vendor.
|
|
||
| // tryOpenFile opens the file at filename if it exists and is not a directory, | ||
| // which os.Open would also happily open. | ||
| func tryOpenFile(filename string) (*os.File, error) { |
There was a problem hiding this comment.
Fail fast when you try to open a directory.
There was a problem hiding this comment.
Did you see cases where this practically happens? Asking because it seems unlikely that a source file path matches a directory name because source file names usually have an extension and directory names do not.
| // component against gorootSrc. Binaries built with -trimpath record them | ||
| // relative to $GOROOT/src (e.g. runtime/proc.go), so resolve relative paths | ||
| // against gorootSrc directly. | ||
| func openGorootSourceFile(path, gorootSrc string) (*os.File, error) { |
There was a problem hiding this comment.
this is the special handling for goroot, only executed if the openSourceFile previous machinery doesn't find anything.
|
Please sign the CLA. |
| // reports the GOROOT this binary was built with, which is the Go installation | ||
| // used to `go install` pprof in the common case. | ||
| var gorootSrc = func() string { | ||
| if goroot := runtime.GOROOT(); goroot != "" { |
There was a problem hiding this comment.
I think pprof binary's runtime.GOROOT() can be different from the goroot of the profiled binary? Not sure we can guarantee that this is the right default.
|
|
||
| // tryOpenFile opens the file at filename if it exists and is not a directory, | ||
| // which os.Open would also happily open. | ||
| func tryOpenFile(filename string) (*os.File, error) { |
There was a problem hiding this comment.
Did you see cases where this practically happens? Asking because it seems unlikely that a source file path matches a directory name because source file names usually have an extension and directory names do not.
| if f, err := tryOpenFile(filename); err == nil { | ||
| return f, nil | ||
| } | ||
| parent := filepath.Dir(dir) |
There was a problem hiding this comment.
Note to self: This is historical but I think we should remove using even parent path of the search paths. When I specify /foo/bar as the search path, I do not expect that the tool will attempt /foo/src/baz.cc paths or something like that. In particular, /foo/src can be a slow NFS / remote mounted directory and trying to look up those paths will slow down the scan.
|
|
||
| // openGorootSourceFile tries to open a Go standard library source file under | ||
| // gorootSrc. Binaries built without -trimpath record standard library files | ||
| // under the build machine's GOROOT (e.g. /usr/local/go/src/runtime/proc.go), |
There was a problem hiding this comment.
Is there a possibility we could instead 1) modify trimPath() below to trim the absolute path prefix; 2) add $GOROOT/src to the default search path? I assume that would be enough to locate the runtime files.
2 participants
Profiles from Go programs refer to standard library sources under the GOROOT of the build machine, which breaks the source and weblist views when that path does not exist locally (e.g. a profile from another machine), or when the program was built with -trimpath and the files are recorded relative to $GOROOT/src.
Teach openSourceFile to fall back to the local $GOROOT/src after the regular search fails: relative paths resolve against it directly, and absolute paths resolve by matching the components after a /src/ one. The fallback only runs when the existing lookup finds nothing, so previously resolved files are unaffected.
Also introduce tryOpenFile, which rejects directories that os.Open would happily open, so a directory matching a candidate path no longer wins the search and then fails when its lines are read.
This is the first PR of 4 to address the #1009