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/compile: change in initialization order when using go test -cover #56293
Comments
Thanks. I'll take a look. |
It looks that it is not a bug really: Below is the instrumented source code. barFlag now depends on the newly added variable GoCover. According to the Go spec at https://go.dev/ref/spec#Package_initialization, it changed the initialization order: First variables with no dependencies are initialized, in particular the 'bar' one (and GoCover). And then barFlag is initialized only. Go compiler knows nothing about the dependencies through the flag library. go tool cover --mode=set go-flags-coverage-problem/pkg.go import ( var (
) func notOK() string {GoCover.Count[0] = 1; func main() {GoCover.Count[1] = 1; } var GoCover = struct { |
(I also commented in the internal tool) |
@cher11 thanks, yes. looking at it from the perspective of the compiler being fed the instrumented source code, it is doing the "right" thing with respect to initialization. To the average user however (e.g. folks who are not looking "under the hood" at the instrumented source code) it will seem that the compiler is behaving incorrectly, so I think it is important that we handle this case better. |
Interesting here-- it looks as though Matthew completely rewrote the initorder code in the compiler for 1.13, so I suspect that prior to that point the test was working essentially by accident. |
OK, I will send a CL shortly with a fix. This fix is specific to the new coverage implementation however (e.g. GOEXPERIMENT=coverageredesign) -- with the old implementation this would be a good deal more difficult to work around. |
Change https://go.dev/cl/443715 mentions this issue: |
When computing package initialization order, special case the counter variables inserted by "cmd/cover" for coverage instrumentation, since their presence can perturb the order in which variables are initialized in ways that are user-visible and incorrect with respect to the original (uninstrumented) program. Fixes golang#56293. Change-Id: Ieec9239ded4f8e2503ff9bbe0fe171afb771b335 Reviewed-on: https://go-review.googlesource.com/c/go/+/443715 Run-TryBot: Than McIntosh <thanm@google.com> Reviewed-by: Cherry Mui <cherryyz@google.com> Reviewed-by: David Chase <drchase@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
The solution taken here (adding special cases to pkginit/initorder.go) means we can't easily switch to using types2's Info.InitOrder, unless we add the same special case logic to go/types and types2. Can we instead change cmd/cover to avoid instrumenting the user code in ways that affects initialization order? E.g., couldn't cmd/cover just put the GoCover variable(s) first so its initialization doesn't delay initializing other variables? Perhaps in a new source file that cmd/go arranges to pass to cmd/compile first on the command line? |
Yes, it does seem as though changing |
Change https://go.dev/cl/499215 mentions this issue: |
Change https://go.dev/cl/499216 mentions this issue: |
This patch contains a revised fix for issue #56293, switching to a scheme in which coverage counter variables and meta-data variables are written to a separate output file as opposed to being tacked onto the end of an existing rewritten source file. The advantage of writing counter vars to a separate file is that the Go command can then present that file as the first source file to the compiler when the package is built; this will ensure that counter variable are treated as lexically "before" any other variable that might call an instrumented function as part of its initializer. Updates #56293. Change-Id: Iccb8a6532b976d36ccbd5a2a339882d1f5d19477 Reviewed-on: https://go-review.googlesource.com/c/go/+/499215 Reviewed-by: Matthew Dempsky <mdempsky@google.com> Run-TryBot: Than McIntosh <thanm@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Bryan Mills <bcmills@google.com>
This patch reverts a portion of the changes in CL 443715, specifically the code in initorder that treats coverage counter variables as special with respect to init order. The special casing is no longer needed now after a change to the way coverage instrumention is done (the go and cover cmds now make sure that coverage variables appear first in the compilation order). Updates #56293. Change-Id: Idf803ff4c1a095e88d455a6adcd63991687eb288 Reviewed-on: https://go-review.googlesource.com/c/go/+/499216 Run-TryBot: Than McIntosh <thanm@google.com> Reviewed-by: Bryan Mills <bcmills@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@thanm Thanks! |
This patch contains a revised fix for issue golang#56293, switching to a scheme in which coverage counter variables and meta-data variables are written to a separate output file as opposed to being tacked onto the end of an existing rewritten source file. The advantage of writing counter vars to a separate file is that the Go command can then present that file as the first source file to the compiler when the package is built; this will ensure that counter variable are treated as lexically "before" any other variable that might call an instrumented function as part of its initializer. Updates golang#56293. Change-Id: Iccb8a6532b976d36ccbd5a2a339882d1f5d19477 Reviewed-on: https://go-review.googlesource.com/c/go/+/499215 Reviewed-by: Matthew Dempsky <mdempsky@google.com> Run-TryBot: Than McIntosh <thanm@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Bryan Mills <bcmills@google.com>
This patch reverts a portion of the changes in CL 443715, specifically the code in initorder that treats coverage counter variables as special with respect to init order. The special casing is no longer needed now after a change to the way coverage instrumention is done (the go and cover cmds now make sure that coverage variables appear first in the compilation order). Updates golang#56293. Change-Id: Idf803ff4c1a095e88d455a6adcd63991687eb288 Reviewed-on: https://go-review.googlesource.com/c/go/+/499216 Run-TryBot: Than McIntosh <thanm@google.com> Reviewed-by: Bryan Mills <bcmills@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Matthew Dempsky <mdempsky@google.com>
m.go:
m_test.go:
When run with
go test
, this passes. When run withgo test -cover
it fails:The variables have no initialization dependencies visible to the compiler, so the order of initialization rules say that they should be initialized in declaration order. That is what happens with
go test
, but it appears to not happen withgo test -cover
.This test passed with
go test -cover
in versions of Go before Go 1.13.CC @golang/runtime @thanm
The text was updated successfully, but these errors were encountered: