Skip to content
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

Closed
ianlancetaylor opened this issue Oct 18, 2022 · 12 comments
Closed
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@ianlancetaylor
Copy link
Contributor

m.go:

package main

import (
	"flag"
)

var (
	fooFlag = flag.String("foo", "", "this should be ok")
	foo     = flag.Lookup("foo")

	barFlag = flag.String("bar", "", "this should be also ok, but is "+notOK()+".")
	bar     = flag.Lookup("bar")
)

func notOK() string {
	return "not OK"
}

m_test.go:

package main

import (
	"testing"
)

func TestFoo(t *testing.T) {
	if foo == nil {
		t.Fatal()
	}
}

func TestBar(t *testing.T) {
	if bar == nil {
		t.Fatal()
	}
}

When run with go test, this passes. When run with go test -cover it fails:

--- FAIL: TestBar (0.00s)
    m_test.go:15: 
FAIL
	main	coverage: 100.0% of statements
exit status 1
FAIL	example.com	0.003s

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 with go test -cover.

This test passed with go test -cover in versions of Go before Go 1.13.

CC @golang/runtime @thanm

@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 18, 2022
@ianlancetaylor ianlancetaylor added this to the Go1.20 milestone Oct 18, 2022
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Oct 18, 2022
@thanm
Copy link
Contributor

thanm commented Oct 18, 2022

Thanks. I'll take a look.

@cher11
Copy link

cher11 commented Oct 18, 2022

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
//line go-flags-coverage-problem/pkg.go:1
package main

import (
"google3/base/go/flag"
)

var (
fooFlag = flag.String("foo", "", "this should be ok")
foo = flag.Lookup("foo")

    barFlag = flag.String("bar", "", "this should be also ok, but is "+notOK()+".")
    bar     = flag.Lookup("bar")

)

func notOK() string {GoCover.Count[0] = 1;
return "not OK"
}

func main() {GoCover.Count[1] = 1;

}

var GoCover = struct {
Count [2]uint32
Pos [3 * 2]uint32
NumStmt [2]uint16
} {
Pos: [3 * 2]uint32{
15, 17, 0x20015, // [0]
19, 21, 0x2000e, // [1]
},
NumStmt: [2]uint16{
1, // 0
0, // 1
},
}

@cher11
Copy link

cher11 commented Oct 18, 2022

(I also commented in the internal tool)

@thanm
Copy link
Contributor

thanm commented Oct 18, 2022

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

@thanm
Copy link
Contributor

thanm commented Oct 18, 2022

This test passed with go test -cover in versions of Go before Go 1.13.

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.

@thanm
Copy link
Contributor

thanm commented Oct 18, 2022

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.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/443715 mentions this issue: cmd/compile: special case coverage vars in pkg init order

romaindoumenc pushed a commit to TroutSoftware/go that referenced this issue Nov 3, 2022
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>
@mdempsky
Copy link
Contributor

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?

@thanm
Copy link
Contributor

thanm commented May 30, 2023

Yes, it does seem as though changing cmd/cover would be possible, although I think a little help from the Go command will be needed as well. I'll send a CL later this morning.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/499215 mentions this issue: cmd/{cover,go}: revise fix for pkg init order change with -cover

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/499216 mentions this issue: cmd/compile: remove special treatment for cover vars in initorder

gopherbot pushed a commit that referenced this issue May 30, 2023
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>
gopherbot pushed a commit that referenced this issue May 30, 2023
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>
@mdempsky
Copy link
Contributor

@thanm Thanks!

@golang golang locked and limited conversation to collaborators May 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants