cmd/go: unnecessary stdlib recompilation when passing -m flag #22527
Comments
ianlancetaylor
added
NeedsDecision
|
The old behavior (do something different depending on the state of $GOROOT/pkg) is not coming back. It's a feature, not a bug, that the end result of a command no longer depends on the specific sequence of "go install" commands that have been run in the past. However, I certainly appreciate that there's a problem here. Today -gcflags means "apply these flags to all packages". It could be that we need some way to express to the go command "apply these flags to some packages". For example maybe /cc @aclements @dr2chase |
|
I understand where you're coming from but it seems to me that this is a rather big, and potentially risky, change in behavior. Consider for instance someone doing a build with If we're thinking about allowing flags to be set per-package, which I think would be great, why not default to only setting flags for the file/package being explicitly compiled? We already have |
|
@huguesb I don't think this is intended for general use, but it would be darn useful when debugging the compiler and runtime. Quite often I want to put a new feature under a flag and enable it for just one package (usually "main" or "foo") without trying that new feature on other code that is much more difficult to debug. |
|
@dr2chase Yes, that's pretty much my use case too. It was mostly working for me until now because my test cases fit in a single file and stdlib/runtime wouldn't get recompiled. An escape hatch would be enough for me although I'm still a little worried about the impact this behavior change may have on unsuspecting users in the wild. |
|
Notes from discussion with @aclements just now. Q1. Threshold question is do we want to be able to specify that a build should use different flags for different packages? The answer is apparently yes, or we wouldn't be having this discussion. Q2. Then the next question is how? Previously the answer to "how?" was repeated go installs, essentially keeping non-repeatable state in the $GOROOT/pkg directory. This was never a good answer and now it's not even a workable answer. It's very important to scalability and repeatability for the result of one build command not to depend on the exact history of previous build commands (see also #22196). That leaves some indication on the go command line that certain flags apply only to a subset of the packages being built. One option, which I will mention only to reject it, is to have bracketing like That would require a whole new flag parser, so we're not going to do that. That leaves putting the scoping into the individual options, as in: to rebuild mypkg with optimization disabled while building (only) package strings. The general form here would be Q3. Next question is what to do about multiple such specs. One option is to introduce more syntax inside a single -gcflags, like semicolon-separated list, but it seems better to make -gcflags repeatable: Q4. Next question is what to do about packages matched by multiple patterns. Today, when you repeat -gcflags the last one wins, so we should probably do that here. These commands: build strings with only Q5. Next question is what it means when you don't specify a pattern, as in My initial answer was that it means -N for all packages, because that's what it meant in earlier versions of Go if nothing was up-to-date. But normally dependencies are up-to-date, and in that case the -N would have applied only to mypkg itself. So maybe a more useful, if nuanced, answer is that without a pattern the flags apply only to the packages named on the command line (similar to how go install only applies to the packages named on the command line now). That would make these work as I think most users expect: To build a fully unoptimized binary you'd use: Q6. Next question is whether -a affects any of this. In particular whether -a should mean "apply -gcflags to all packages instead of just what's on the command line". That would essentially bless the current idiom for building a fully unoptimized binary: However, I think we should not do this. Despite popular misunderstandings, -a is and has always been about forcing the build not to reuse any previously built object files. It's a debugging override that has over time become less and less necessary, and as of Go 1.10 it should (in the absence of caching bugs) be completely unnecessary. Conflating that debugging with the meaning of other flags seems like a mistake. This means that people (or maybe just delve) will need to learn the new idiom to build an unoptimized binary. I'm OK with that. The confusion and non-orthogonality of redefining -a seems too destructive. (I've used -gcflags as the running example but this would apply to all (and only) the tool flags: -asmflags, -gccgoflags, -gcflags, -ldflags.) The proposal, then, is the following.
Normally something like this would not be changed this late in the cycle, but I think it's critical to fix along with deploying the new content-based staleness. The old semantics were never really right but that fact is exposed more clearly by content-based staleness. /cc @aclements @dr2chase @cherrymui @thanm @griesemer @mdempsky @khr @ianlancetaylor @crawshaw @heschik @derekparker |
|
Overall SGTM. I find
difficult to read... would it help to add some syntactic sugar like
Obviously these both mean the same, but the second is visually clearer (easier to pick out the meaning of each of the two ='s). |
|
Just to note that given this affects -ldflags it would have a serious performance impact on all builds we do here as we use it to set static build information, which is specific to a given package. I believe this is quite commonly used, here's an example for reference: go build -ldfags "-X package/directory/utils.rcsVersion=${GIT_REVISION} -X package/directory/utils.buildDate=${BUILD_DATE} -X package/directory/utils.rcsTag=${BUILD_TAG} -X package/directory/utils.buildDirty=${GIT_DIRTY}" package/to/buildI would agree with @rsc that this is critical to fix, as we wouldn't be able to update to 1.10 due to the performance implications without it. The proposed fix sounds good to me, however I wonder if -X ldflags could be treated as a special case and the effected package calculated automatically as the package information is already present. This would help eliminate what could otherwise be quite a painful upgrade for users of this feature? |
|
-ldflags here looks a bit confusing to me. Presumably, it only affects the invocation of the linker, which typically only runs once for the final binary, and it has nothing to do with compiling packages. So I'm not sure we need a per-package -ldflags (and what it means). |
I agree that it looks a little weird, but there's a fair amount of precedent for this in other places. E.g., It can be made easier to read simply using shell syntax, though: |
|
@stevenh I think that under @rsc's proposal your command line would not have to change at all. Since you are not specifying a package, the |
|
@cherrymui A package specific |
|
@ianlancetaylor Yes, it does apply in your example. Thanks. |
|
@rsc Your proposal seems problematic for any option that can itself contain an
Perhaps we can assume that no package path starts with Or, we could require that these options must always be written with a leading |
|
In the decision between |
|
Change https://golang.org/cl/76551 mentions this issue: |
1 similar comment
|
Change https://golang.org/cl/76551 mentions this issue: |
|
@andybons, why did gopherbot just comment twice about this CL? Are there two running when there should only be one? |
|
Only one pod is currently running for GopherBot (including both staging and prod). /cc @bradfitz |
|
@rsc Tested tip at |
|
@huguesb, I'm not sure what you mean. This is my output: (And note that if you run the same go command again it will print nothing, because of #22587, which will be fixed.) But it doesn't print anything about runtime like in your original report. Are you seeing something different? |
|
Hmm weird. I can't reproduce this anymore. I might have accidentally |
What version of Go are you using (
go version)?tip
bc723cf3Does this issue reproduce with the latest release?
No. The relevant changes were introduced in master recently.
What operating system and processor architecture are you using (
go env)?Darwin/amd64
What did you do?
The exact code doesn't matter as long as there's at least one import.
The default code at https://play.golang.org/ will exhibit the issue.
go run -gcflags='-m=2' main.goWhat did you expect to see?
What did you see instead?
It looks like @rsc 's recent changes to staleness detection cause spurious rebuilds. I would argue that any flag that doesn't change binary output, which includes
-m, should not cause the std lib to be rebuilt.I would personally prefer that even flags that do change binary output (e.g.
-l) do not cause the std lib to be rebuilt when only a single go file is passed togo run/go buildas it makes it considerably harder to test specific compiler features on specific code in isolation, which can be a problem for a variety of things, including, but not limited to, reduction of issue reproducers.The text was updated successfully, but these errors were encountered: