proposal: cmd/go: add pkgspec support to -toolexec

[mknyszek]

Today, our best way of benchmarking the compiler is golang.org/x/tools/cmd/compilebench, which mostly works great, but unfortunately has to encode knowledge about internal compiler and linker flags to actually individually benchmark the compiler and linker. This has some issues when trying to benchmark builds of packages outside the standard library, because of things like the -importcfg flag, and setting all that up.

Although a tool could theoretically work around this by parsing the output of go build -x, this is complex and fraught with issues. This doesn't even include working around all the caching that the go command does, which can be worked around with -a, but it means that there's a lot more that needs to execute with each benchmark iteration that isn't even measured.

Instead (at @mdempsky's suggestion), I propose adding a pkgspec pattern to -toolexec's syntax, similar to how -gcflags and -ldflags work. In this way, it will be possible to execute a build while wrapping only a specific part of the build. More specifically, it will wrap all commands related to compiling a specific package with the given command prefix. Furthermore, the -toolexec flag can propagate into the go command's caching, to always prevent caching of the specified packages.

One downside to this proposal is that it will break existing scripts using -toolexec. By adding in the pkgspec pattern, -toolexec will have to, by default, only instrument the target package, just like -gcflags does, and all will need to be explicitly mentioned to get back the original behavior. I'm not sure this breakage will be particularly impactful, but maybe others feel differently.

Lastly, although I'm motivated by improving compilebench, there's some value to this functionality for debugging purposes. For instance, wrapping specific compiler invocations with the Linux perf record command instead of having to figure out the compiler invocation, extract it, set up the build configuration by passing -work, etc.

[mknyszek]

It has been brought to my attention that TOOLEXEC_IMPORTPATH exists, and it's only recently become documented (as of Go 1.17).

There may still be value here, but that actually fulfills the need I have.

[mknyszek]

To be more clear, that fulfills the need I have, when used in combination with #27628 (comment).

[ianlancetaylor]

The discussion at #41145 may be relevant.

[mknyszek]

Thanks Ian! I think that even #41145 is superseded by the approved proposal at #27628, since that'll mean toolexec executions will only be cached if the tool doesn't actually print anything, which basically fits any use-case I can think of for toolexec.

[mvdan]

Chiming in since I authored #41145 and implemented TOOLEXEC_IMPORTPATH. My use case is https://github.com/burrowers/garble, which might be one of the more complex use cases for -toolexec out there.

Note that garble modifies the output of go tool compile -V, for the sake of using a different set of cache entries for built packages. It's akin to how using different build flags results in different build cache entries. This is because, as an obfuscator, it modifies the source code being built, so it can't use the regular cache entries.

Furthermore, the -toolexec flag can propagate into the go command's caching, to always prevent caching of the specified packages.

As some of the comments have stated, I think #27628 is what we want to do for caching instead.

If this proposal were accepted and a toolexec command isn't used for a sub-tree of the built packages, then I think the build tool could simply use its build cache as usual, as if the toolexec command for those packages hadn't printed anything nor modified the output of go tool compile -V.

To be more clear, that fulfills the need I have, when used in combination with #27628 (comment).

I agree that TOOLEXEC_IMPORTPATH plus the control of whether or not package builds are cached should be enough to do pretty much anything I can think of in a reasonably efficient way.

Beware that TOOLEXEC_IMPORTPATH is somewhat ambiguous in 1.16.x; that's fixed in 4fe324d.

[rsc]

FWiW, I don't see how we can change -toolexec to admit the new patterns.
Today you can do things like

-toolexec='env X=Y time'

That would mean something else entirely in the usual -gcflags syntax.

It sounds like having TOOLEXEC_IMPORTPATH plus the new caching logic suffices for the use cases we know for more fine-grained tool selection. And certainly that fine-grained tool selection could be implemented in the tool itself by using TOOLEXEC_IMPORTPATH.

Should we close this issue?

[rsc]

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

[rsc]

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

[rsc]

No change in consensus, so declined.
— rsc for the proposal review group