New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/go: unnecessary stdlib recompilation when passing -m flag #22527

Closed
huguesb opened this Issue Nov 1, 2017 · 21 comments

Comments

Projects
None yet
10 participants
@huguesb
Contributor

huguesb commented Nov 1, 2017

What version of Go are you using (go version)?

tip bc723cf3

Does 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.go

What did you expect to see?

main.go:7:6: cannot inline main: non-leaf function
main.go:8:14: "Hello" escapes to heap
main.go:8:14: 	from ... argument (arg to ...) at main.go:8:13
main.go:8:14: 	from *(... argument) (indirection) at main.go:8:13
main.go:8:14: 	from ... argument (passed to call[argument content escapes]) at main.go:8:13
main.go:8:13: main ... argument does not escape

What did you see instead?

...
# reflect
reflect/swapper.go:38:43: <S> capturing by value: ps (addr=false assign=false width=24)
reflect/swapper.go:42:43: <S> capturing by value: ss (addr=false assign=false width=24)
reflect/swapper.go:48:43: <S> capturing by value: is (addr=false assign=false width=24)
reflect/swapper.go:51:43: <S> capturing by value: is (addr=false assign=false width=24)
reflect/swapper.go:54:43: <S> capturing by value: is (addr=false assign=false width=24)
reflect/swapper.go:57:43: <S> capturing by value: is (addr=false assign=false width=24)
reflect/swapper.go:65:22: <S> capturing by value: s (addr=false assign=false width=8)
reflect/swapper.go:68:30: <S> capturing by value: size (addr=false assign=false width=8)
reflect/swapper.go:70:16: <S> capturing by value: typ (addr=false assign=false width=8)
reflect/swapper.go:70:21: <S> capturing by value: tmp (addr=false assign=false width=8)
reflect/type.go:1387:61: <S> capturing by value: name (addr=false assign=false width=16)
reflect/type.go:2031:40: <S> capturing by value: hash (addr=false assign=false width=4)
reflect/type.go:2440:26: <S> capturing by value: ifield (addr=false assign=false width=8)
reflect/type.go:2440:41: <S> capturing by value: imethod (addr=false assign=false width=8)
reflect/type.go:2448:26: <S> capturing by value: ifield (addr=false assign=false width=8)
reflect/type.go:2448:41: <S> capturing by value: imethod (addr=false assign=false width=8)
reflect/type.go:2457:26: <S> capturing by value: ifield (addr=false assign=false width=8)
reflect/type.go:2457:41: <S> capturing by value: imethod (addr=false assign=false width=8)
reflect/type.go:2465:26: <S> capturing by value: ifield (addr=false assign=false width=8)
reflect/type.go:2465:41: <S> capturing by value: imethod (addr=false assign=false width=8)
reflect/type.go:2654:41: <S> capturing by value: hash (addr=false assign=false width=4)
reflect/type.go:2749:23: <S> capturing by value: typ (addr=false assign=false width=8)
reflect/type.go:2759:23: <S> capturing by value: typ (addr=false assign=false width=8)
reflect/type.go:2974:20: <S> capturing by value: count (addr=false assign=false width=8)
reflect/type.go:2975:25: <S> capturing by value: esize (addr=false assign=false width=8)
reflect/type.go:2977:9: <S> capturing by value: eequal (addr=false assign=false width=8)
reflect/type.go:2989:20: <S> capturing by value: count (addr=false assign=false width=8)
reflect/type.go:2990:9: <S> capturing by value: ehash (addr=false assign=false width=8)
reflect/type.go:2990:31: <S> capturing by value: esize (addr=false assign=false width=8)
reflect/type.go:3121:21: <S> capturing by value: x (addr=false assign=false width=8)
reflect/value.go:1015:6: can inline Value.IsValid as: method(Value) func() bool { return v.flag != 0 }
reflect/type.go:774:6: can inline (*rtype).Kind as: method(*rtype) func() Kind { return Kind(t.kind & kindMask) }
reflect/type.go:644:6: cannot inline resolveTypeOff: no function body
reflect/type.go:683:6: cannot inline (*rtype).typeOff: non-leaf function
reflect/type.go:691:6: cannot inline (*rtype).uncommon: unhandled op DCLTYPE
...
on and on for hundreds of lines
...
fmt/scan.go:369:33: (*readRune).UnreadRune r does not escape
fmt/print.go:128:38: new(pp) escapes to heap
fmt/print.go:128:38: 	from ~r0 (return) at fmt/print.go:128:28
fmt/print.go:128:38: new(pp) escapes to heap
fmt/print.go:128:38: 	from new(pp) (interface-converted) at fmt/print.go:128:38
fmt/print.go:128:38: 	from ~r0 (return) at fmt/print.go:128:28
fmt/scan.go:379:38: new(ss) escapes to heap
fmt/scan.go:379:38: 	from ~r0 (return) at fmt/scan.go:379:28
fmt/scan.go:379:38: new(ss) escapes to heap
fmt/scan.go:379:38: 	from new(ss) (interface-converted) at fmt/scan.go:379:38
fmt/scan.go:379:38: 	from ~r0 (return) at fmt/scan.go:379:28
<autogenerated>:1:0: leaking param: io.p
<autogenerated>:1:0: 	from .this.Write(io.p) (parameter to indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.Write(io.p) (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: os.(*File).close .this does not escape
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.Flag(c) (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.Precision() (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.Width() (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: b
<autogenerated>:1:0: 	from .this.Write(b) (parameter to indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.Write(b) (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: io.p
<autogenerated>:1:0: 	from .this.Read(io.p) (parameter to indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.Read(io.p) (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.ReadRune() (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.UnreadRune() (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.Error() (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: buf
<autogenerated>:1:0: 	from .this.Read(buf) (parameter to indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.Read(buf) (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.ReadRune() (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.SkipSpace() (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: f
<autogenerated>:1:0: 	from .this.Token(skipSpace, f) (parameter to indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.Token(skipSpace, f) (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.UnreadRune() (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.Width() (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.Align() (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: reflect.u
<autogenerated>:1:0: 	from .this.AssignableTo(reflect.u) (parameter to indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.AssignableTo(reflect.u) (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.Bits() (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.ChanDir() (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.Comparable() (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: reflect.u
<autogenerated>:1:0: 	from .this.ConvertibleTo(reflect.u) (parameter to indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.ConvertibleTo(reflect.u) (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.Elem() (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.Field(reflect.i) (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.FieldAlign() (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: reflect.index
<autogenerated>:1:0: 	from .this.FieldByIndex(reflect.index) (parameter to indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.FieldByIndex(reflect.index) (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: reflect.name
<autogenerated>:1:0: 	from .this.FieldByName(reflect.name) (parameter to indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.FieldByName(reflect.name) (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: reflect.match
<autogenerated>:1:0: 	from .this.FieldByNameFunc(reflect.match) (parameter to indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.FieldByNameFunc(reflect.match) (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: reflect.u
<autogenerated>:1:0: 	from .this.Implements(reflect.u) (parameter to indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.Implements(reflect.u) (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.In(reflect.i) (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.IsVariadic() (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.Key() (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.Kind() (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.Len() (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.Method(.anon0) (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .anon0
<autogenerated>:1:0: 	from .this.MethodByName(.anon0) (parameter to indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.MethodByName(.anon0) (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.Name() (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.NumField() (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.NumIn() (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.NumMethod() (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.NumOut() (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.Out(reflect.i) (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.PkgPath() (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.Size() (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.String() (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.common() (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.uncommon() (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: f
fmt/format.go:168:19: index bounds check elided
fmt/scan.go:349:27: index bounds check elided
<autogenerated>:1:0: 	from .this.Format(f, c) (parameter to indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.Format(f, c) (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.GoString() (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: state
<autogenerated>:1:0: 	from .this.Scan(state, verb) (parameter to indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.Scan(state, verb) (receiver in indirect call) at <autogenerated>:1:0
<autogenerated>:1:0: leaking param: .this
<autogenerated>:1:0: 	from .this.String() (receiver in indirect call) at <autogenerated>:1:0
# command-line-arguments
main.go:7:6: cannot inline main: non-leaf function
main.go:8:14: "Hello" escapes to heap
main.go:8:14: 	from ... argument (arg to ...) at main.go:8:13
main.go:8:14: 	from *(... argument) (indirection) at main.go:8:13
main.go:8:14: 	from ... argument (passed to call[argument content escapes]) at main.go:8:13
main.go:8:13: main ... argument does not escape

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 to go run / go build as 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.

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Nov 2, 2017

Contributor

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

go run -gcflags='main:-m=2' main.go

/cc @aclements @dr2chase

Contributor

rsc commented Nov 2, 2017

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

go run -gcflags='main:-m=2' main.go

/cc @aclements @dr2chase

@huguesb

This comment has been minimized.

Show comment
Hide comment
@huguesb

huguesb Nov 2, 2017

Contributor

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 -race, -N, or -l=3, or really any other compiler flag. The new behavior means that on upgrade, compilation is suddenly much longer and the performance characteristics of the resulting binary may change dramatically as the option suddenly applies to stdlib and runtime instead of just application code.

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 -a to force flags to be applied to every packages.

Contributor

huguesb commented Nov 2, 2017

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 -race, -N, or -l=3, or really any other compiler flag. The new behavior means that on upgrade, compilation is suddenly much longer and the performance characteristics of the resulting binary may change dramatically as the option suddenly applies to stdlib and runtime instead of just application code.

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 -a to force flags to be applied to every packages.

@dr2chase

This comment has been minimized.

Show comment
Hide comment
@dr2chase

dr2chase Nov 2, 2017

Contributor

@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.

Contributor

dr2chase commented Nov 2, 2017

@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.

@huguesb

This comment has been minimized.

Show comment
Hide comment
@huguesb

huguesb Nov 2, 2017

Contributor

@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.

Contributor

huguesb commented Nov 2, 2017

@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.

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Nov 7, 2017

Contributor

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

go build -packagescope=strings -gcflags=-N -endpackagescope mypkg

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:

go build -gcflags=strings=-N mypkg

to rebuild mypkg with optimization disabled while building (only) package strings. The general form here would be -gcflags=<packagepattern>=<flags>, where packagepattern is a pattern the go command can check against each package it is building (see go help packages).

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:

go build -gcflags=strings=-N -gcflags=math=-l mypkg

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:

go build -gcflags=strings=-N -gcflags=strings=-l mypkg
go build -gcflags=all=-N -gcflags=strings=-l mypkg
go build -gcflags=s...=-N -gcflags=strings=-l mypkg

build strings with only -l not also -N.

Q5. Next question is what it means when you don't specify a pattern, as in

go build -gcflags=-N mypkg

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:

go build -gcflags=-m net/http # show optimization decisions in net/http
go build -gcflags=-S strings # show disassembly for package strings
go build -gcflags=-N myprog # build myprog with optimization disabled only for myprog itself (not all its dependencies)
go build -gcflags=net/http=-d=ssa/check/on # build net/http (only) with extra SSA checking

To build a fully unoptimized binary you'd use:

go build -gcflags=all=-N myprog

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:

go build -a -gcflags=-N myprog

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

go build -gcflags=all=-N myprog

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.

  1. The go command flags -asmflags, -gccgoflags, -gcflags, and -ldflags are adjusted to take an optional = prefix that causes the flags to apply only to packages matching that pattern (that is, packages that would show up in go list <pattern>). Using -gcflags below is a stand-in for any of the larger set.

  2. For each package being built, repeated -gcflags are considered in turn: the final match is the one that applies, and earlier matches are replaced (not augmented) by the final match.

  3. In the absence of a pattern, -gcflags applies only to the packages named on the command line (or the implicit "current directory" argument when no package is named on the command line).

  4. None of this is affected by the debugging flag -a, which has the completely orthogonal meaning "do not use previously built object files even if they look useful".

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

Contributor

rsc commented Nov 7, 2017

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

go build -packagescope=strings -gcflags=-N -endpackagescope mypkg

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:

go build -gcflags=strings=-N mypkg

to rebuild mypkg with optimization disabled while building (only) package strings. The general form here would be -gcflags=<packagepattern>=<flags>, where packagepattern is a pattern the go command can check against each package it is building (see go help packages).

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:

go build -gcflags=strings=-N -gcflags=math=-l mypkg

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:

go build -gcflags=strings=-N -gcflags=strings=-l mypkg
go build -gcflags=all=-N -gcflags=strings=-l mypkg
go build -gcflags=s...=-N -gcflags=strings=-l mypkg

build strings with only -l not also -N.

Q5. Next question is what it means when you don't specify a pattern, as in

go build -gcflags=-N mypkg

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:

go build -gcflags=-m net/http # show optimization decisions in net/http
go build -gcflags=-S strings # show disassembly for package strings
go build -gcflags=-N myprog # build myprog with optimization disabled only for myprog itself (not all its dependencies)
go build -gcflags=net/http=-d=ssa/check/on # build net/http (only) with extra SSA checking

To build a fully unoptimized binary you'd use:

go build -gcflags=all=-N myprog

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:

go build -a -gcflags=-N myprog

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

go build -gcflags=all=-N myprog

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.

  1. The go command flags -asmflags, -gccgoflags, -gcflags, and -ldflags are adjusted to take an optional = prefix that causes the flags to apply only to packages matching that pattern (that is, packages that would show up in go list <pattern>). Using -gcflags below is a stand-in for any of the larger set.

  2. For each package being built, repeated -gcflags are considered in turn: the final match is the one that applies, and earlier matches are replaced (not augmented) by the final match.

  3. In the absence of a pattern, -gcflags applies only to the packages named on the command line (or the implicit "current directory" argument when no package is named on the command line).

  4. None of this is affected by the debugging flag -a, which has the completely orthogonal meaning "do not use previously built object files even if they look useful".

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

@thanm

This comment has been minimized.

Show comment
Hide comment
@thanm

thanm Nov 7, 2017

Member

Overall SGTM.

I find

-gcflags=strings=-N

difficult to read... would it help to add some syntactic sugar like

-gcflags={strings=-N}

Obviously these both mean the same, but the second is visually clearer (easier to pick out the meaning of each of the two ='s).

Member

thanm commented Nov 7, 2017

Overall SGTM.

I find

-gcflags=strings=-N

difficult to read... would it help to add some syntactic sugar like

-gcflags={strings=-N}

Obviously these both mean the same, but the second is visually clearer (easier to pick out the meaning of each of the two ='s).

@stevenh

This comment has been minimized.

Show comment
Hide comment
@stevenh

stevenh Nov 7, 2017

Contributor

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/build

I 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?

Contributor

stevenh commented Nov 7, 2017

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/build

I 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?

@cherrymui

This comment has been minimized.

Show comment
Hide comment
@cherrymui

cherrymui Nov 7, 2017

Contributor

-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).

Contributor

cherrymui commented Nov 7, 2017

-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).

@aclements

This comment has been minimized.

Show comment
Hide comment
@aclements

aclements Nov 7, 2017

Member

I find -gcflags=strings=-N difficult to read

I agree that it looks a little weird, but there's a fair amount of precedent for this in other places. E.g., GODEBUG=gctrace=1 follows the same pattern.

It can be made easier to read simply using shell syntax, though: -gcflags="strings=-N". This is probably a good habit anyway since it's necessary when passing multiple flags (or you have to escape the space itself :) Of course, the shell isn't forcing you to put the quotes there, but this seems sufficient to me.

Member

aclements commented Nov 7, 2017

I find -gcflags=strings=-N difficult to read

I agree that it looks a little weird, but there's a fair amount of precedent for this in other places. E.g., GODEBUG=gctrace=1 follows the same pattern.

It can be made easier to read simply using shell syntax, though: -gcflags="strings=-N". This is probably a good habit anyway since it's necessary when passing multiple flags (or you have to escape the space itself :) Of course, the shell isn't forcing you to put the quotes there, but this seems sufficient to me.

@ianlancetaylor

This comment has been minimized.

Show comment
Hide comment
@ianlancetaylor

ianlancetaylor Nov 7, 2017

Contributor

@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 -ldflags option will apply only to the packages listed on the command line. And that seems to be what you want. In particular note that the -X option is indeed applied only at link time; there is no need to recompile the packages mentioned.

Contributor

ianlancetaylor commented Nov 7, 2017

@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 -ldflags option will apply only to the packages listed on the command line. And that seems to be what you want. In particular note that the -X option is indeed applied only at link time; there is no need to recompile the packages mentioned.

@ianlancetaylor

This comment has been minimized.

Show comment
Hide comment
@ianlancetaylor

ianlancetaylor Nov 7, 2017

Contributor

@cherrymui A package specific -ldflags option would apply when running something like go install -ldflags=cmd/compile=-w cmd. I admit I'm having trouble coming up with an example that somebody might actually want to do, but it seems meaningful and consistent.

Contributor

ianlancetaylor commented Nov 7, 2017

@cherrymui A package specific -ldflags option would apply when running something like go install -ldflags=cmd/compile=-w cmd. I admit I'm having trouble coming up with an example that somebody might actually want to do, but it seems meaningful and consistent.

@cherrymui

This comment has been minimized.

Show comment
Hide comment
@cherrymui

cherrymui Nov 7, 2017

Contributor

@ianlancetaylor Yes, it does apply in your example. Thanks.

Contributor

cherrymui commented Nov 7, 2017

@ianlancetaylor Yes, it does apply in your example. Thanks.

@ianlancetaylor

This comment has been minimized.

Show comment
Hide comment
@ianlancetaylor

ianlancetaylor Nov 7, 2017

Contributor

@rsc Your proposal seems problematic for any option that can itself contain an =. Scanning the doc files, I see the following such options:

  • cmd/compile -importmap
  • cmd/asm -D
  • cmd/link -X
  • gccgo quite a few options inherited from GCC

Perhaps we can assume that no package path starts with -, and that therefore the optional package prefix is anything before the first = unless the first character of the option is -. But note that while the language spec does permit prohibiting some characters from the package path, - is not one of those characters.

Or, we could require that these options must always be written with a leading all= prefix.

Contributor

ianlancetaylor commented Nov 7, 2017

@rsc Your proposal seems problematic for any option that can itself contain an =. Scanning the doc files, I see the following such options:

  • cmd/compile -importmap
  • cmd/asm -D
  • cmd/link -X
  • gccgo quite a few options inherited from GCC

Perhaps we can assume that no package path starts with -, and that therefore the optional package prefix is anything before the first = unless the first character of the option is -. But note that while the language spec does permit prohibiting some characters from the package path, - is not one of those characters.

Or, we could require that these options must always be written with a leading all= prefix.

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Nov 8, 2017

Contributor

In the decision between <flags> and <pattern>=<flags>, I believe it is safe to assume that flags always begin with - and patterns do not. I am not worried about packages with import paths beginning with -. I mean, if you try to use -foo as your import path, you're going to run into problems using cmd/go long before you get to trying to set package-specific compilation flags.

Contributor

rsc commented Nov 8, 2017

In the decision between <flags> and <pattern>=<flags>, I believe it is safe to assume that flags always begin with - and patterns do not. I am not worried about packages with import paths beginning with -. I mean, if you try to use -foo as your import path, you're going to run into problems using cmd/go long before you get to trying to set package-specific compilation flags.

@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Nov 8, 2017

Change https://golang.org/cl/76551 mentions this issue: cmd/go: implement per-package asmflags, gcflags, ldflags, gccgoflags

gopherbot commented Nov 8, 2017

Change https://golang.org/cl/76551 mentions this issue: cmd/go: implement per-package asmflags, gcflags, ldflags, gccgoflags

@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Nov 8, 2017

Change https://golang.org/cl/76551 mentions this issue: cmd/go: implement per-package asmflags, gcflags, ldflags, gccgoflags

gopherbot commented Nov 8, 2017

Change https://golang.org/cl/76551 mentions this issue: cmd/go: implement per-package asmflags, gcflags, ldflags, gccgoflags

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Nov 8, 2017

Contributor

@andybons, why did gopherbot just comment twice about this CL? Are there two running when there should only be one?

Contributor

rsc commented Nov 8, 2017

@andybons, why did gopherbot just comment twice about this CL? Are there two running when there should only be one?

@andybons

This comment has been minimized.

Show comment
Hide comment
@andybons

andybons Nov 8, 2017

Member

Only one pod is currently running for GopherBot (including both staging and prod).

/cc @bradfitz

Member

andybons commented Nov 8, 2017

Only one pod is currently running for GopherBot (including both staging and prod).

/cc @bradfitz

@gopherbot gopherbot closed this in 5993251 Nov 9, 2017

@huguesb

This comment has been minimized.

Show comment
Hide comment
@huguesb

huguesb Nov 9, 2017

Contributor

@rsc Tested tip at 48f2a55a. The issue seems to be solved when the input is a package but not when it is a single file.

Contributor

huguesb commented Nov 9, 2017

@rsc Tested tip at 48f2a55a. The issue seems to be solved when the input is a package but not when it is a single file.

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Nov 9, 2017

Contributor

@huguesb, I'm not sure what you mean. This is my output:

$ cat hello.go
package main

import (
	"fmt"
)

func main() {
	fmt.Println("Hello, playground")
}
$ go build -gcflags=-m=2 hello.go
# command-line-arguments
./hello.go:7:6: cannot inline main: non-leaf function
./hello.go:8:14: "Hello, playground" escapes to heap
./hello.go:8:14: 	from ... argument (arg to ...) at ./hello.go:8:13
./hello.go:8:14: 	from *(... argument) (indirection) at ./hello.go:8:13
./hello.go:8:14: 	from ... argument (passed to call[argument content escapes]) at ./hello.go:8:13
./hello.go:8:13: main ... argument does not escape
$

(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?

Contributor

rsc commented Nov 9, 2017

@huguesb, I'm not sure what you mean. This is my output:

$ cat hello.go
package main

import (
	"fmt"
)

func main() {
	fmt.Println("Hello, playground")
}
$ go build -gcflags=-m=2 hello.go
# command-line-arguments
./hello.go:7:6: cannot inline main: non-leaf function
./hello.go:8:14: "Hello, playground" escapes to heap
./hello.go:8:14: 	from ... argument (arg to ...) at ./hello.go:8:13
./hello.go:8:14: 	from *(... argument) (indirection) at ./hello.go:8:13
./hello.go:8:14: 	from ... argument (passed to call[argument content escapes]) at ./hello.go:8:13
./hello.go:8:13: main ... argument does not escape
$

(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?

@huguesb

This comment has been minimized.

Show comment
Hide comment
@huguesb

huguesb Nov 9, 2017

Contributor

Hmm weird. I can't reproduce this anymore. I might have accidentally toolstash restore'd between two tests or something.

Contributor

huguesb commented Nov 9, 2017

Hmm weird. I can't reproduce this anymore. I might have accidentally toolstash restore'd between two tests or something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment