cmd/compile, runtime: initialization order within a file appears to have changed

[cosnicolaou]

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

go version go1.17.2 darwin/amd64

Does this issue reproduce with the latest release?

Yes, with 1.17.1 and .2. It has not been a problem prior to 1.17.

What operating system and processor architecture are you using (go env)?

go env Output

GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/cnicolaou/Library/Caches/go-build"
GOENV="/Users/cnicolaou/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/cnicolaou/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/cnicolaou/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.16.7"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/cnicolaou/go-bug-report/bug/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/1h/54rm9zd51ddcs0pt8j_l7d2m0000gn/T/go-build3841401253=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Nothing new! The code that encountered the problem has been in use since 2014/15.

It is my understanding that for the code snippet below, the initializeMe function will always
be called first and before any other initialization. This has been been the case prior to
go 1.17

package foo
// no other code here.
var _ = initializeMe()
// other code here, which really should not matter

However, with go 1.17, this behavior appears to have changed, and initializeMe is no longer
called before other initialization and in my case is not called before initialization that depends
on it. I understand that there is no dependency analysis going on but there shouldn't need
since by my reading of the spec initializeMe should always be called first. I have attached
code snippets to repro the problem.

The code that seems to defeat the expected initialization involves an additional variable initialization
via an other package.

var breaksIfThisExists = struct {
        vals    []int
} {
        []int{indirect.GetVal(d)},
}

where indirect.GetVal(d) calls a method on d whose implementation accesses a variable that
is initialized by initializeMe.

What did you expect to see?

no panic.

What did you see instead?

a panic

bug.tar.gz

[randall77]

The way I read your code, and the spec, is that

var _ = initializeMe()

cannot be initialized immediately, because the body of initializeMe depends on globalX. Only breakIfThisExists and globalX are initializeable immediately, and the first one in program order is breakIfThisExists.

So my reading is that 1.17 is correct, and earlier versions are broken. Not sure what changed, though.

[cosnicolaou]

isn't it the case that the body of initializeMe initializes globalX rather than depending on it?

Another thing to consider is that go1.17 behaves like earlier versions if you remove the use of the indirect package, that is, the code below works as per prior versions and that it's the indirection that triggers the change in behavior. Which is likely a saving grace since if prior versions were out-of-spec then go1.17 is correct ie. go1.17 is also out of spec for the common case!

package bug

import (
        "fmt"
)

var _ = initializeMe()

type dummy string

func (d dummy) Get() int {
        if globalX == nil {
                panic("initializeMe was not called\n")
        }
        return *globalX
}

const d = dummy("A")
var worksWithThis = d.Get()

var globalX *int

func initializeMe() struct{} {
        fmt.Printf("initMe....\n")
        globalX = new(int)
        *globalX = 33
        return struct{}{}
}

[randall77]

isn't it the case that the body of initializeMe initializes globalX rather than depending on it?

Not according to the spec. Initialization order only depends on lexical references. It doesn't look at, for instance, if a reference to a variable is a read or write. (Except, of course, for the assignment in the top-level declaration itself.)

I'm not sure I understand your point in the latest example.
It is true that you can hide dependencies in other packages, which is what your original example did. This is very similar in that regard to the example in the spec, where it hides a dependency using an interface call (the 3rd code block in https://golang.org/ref/spec#Package_initialization).

[cosnicolaou]

I had misinterpreted the lexical reference (as appearing anywhere on the rhs), but, yes, the spec is quite clear, my apologies.

fd22df9 - old behaviour
68e6fa4 - new behaviour - this seems to be when the change was introduced, but I haven't read the code to see what actually changed, the commit message is suggestive of your suspicion though (ie. 17 is correct, earlier versions not).

My admittedly unclear point from the previous message is, that given the spec, wouldn't you expect my example above (without the other package) to behave the same way as the one with the indirection? My probably wrong reasoning being, for the example above:

• d depends on globalX (via the Get method)
• initializeMe depends on globalX
• I presume initializeMe gets called first because it's referenced first in the file

With the other package in play.

const d = dummy("A")

var breaksIfThisExists = struct {
        vals    []int
} {
        []int{indirect.GetVal(d)},
}

• d depends on globalX
• initializeMe depends on globalX
• but d.GetVal() gets called before initializeMe?

Maybe I'm just confused! I'd be just be happy with a work around but haven't found yet. If 17.x is the correct behavior then 16 versions of the incorrect behaviour will have stored up quite a few latent bugs!

[randall77]

d depends on globalX

That isn't the case here. d is a constant, and even if it wasn't, it doesn't depend on globalX, because Get is not called on it.

[cosnicolaou]

I also tried it with a var, and how is Get not called on it?

const d = dummy("A")
var worksWithThis = d.Get()

[randall77]

worksWithThis depends on globalX, through the Get call. But d itself doesn't depend on globalX.

[cosnicolaou]

ok, so just so that I understand it:

1. working

• initializeMe -> globalX
• worksWithThis -> globalX
  chose initializeMe because it's first in the file

2. not working

• initializeMe -> globalX
• breaksIfThisExists (no dependency)
  choose breaksIfThisExists because it's good to go in the first iteration.

Is that the correct way to read this?

Now, having taking a quick look at #43444 I can see a workaround, namely, declaring globalX before
initializeMe, or explicitly initializing it before its indirectly depended on allows initializeMe to run.

var globalX *int
var _ = initializeMe()

I think you should highlight this difference in the release notes for go 1.17, it all seems pretty subtle to me. The fourth example below, I think is arguable since it currently breaks, but maybe it shouldn't.

// works
var globalX *int
var _ = initializeMe()

// fails
var _ = initializeMe()
var globalX *int

// works
var _ = initializeMe()
var globalX = new(int)

var breaksIfThisExists = struct {
....

// breaks
var _ = initializeMe()

var breaksIfThisExists = struct {
....

var globalX = new(int)

[randall77]

Your understanding is correct.

We could update the release notes. What would you want them to say?

[cosnicolaou]

Anything that explains the difference between the old and new behavior with a warning to watch out for such problems and suggested work arounds. Do you want me to draft something?

[randall77]

That would help. I'm not sure I exactly understand how to characterize what changed. Other than "it now better adheres to the spec".

[cosnicolaou]

well, I'm similarly uncertain, but I'll draft something and you can run it by Matthew before publishing.

[cosnicolaou]

can you assign the following to mdempsky to review?

Please add this, or something similar, to the compiler section:

go1.17 correctly implements the language specification with respect to package initialization order, earlier versions failed to do so; consequently code that depended on the previous incorrect initialization order may no longer work as expected under 1.17 and future versions. Prior versions failed to detect dependencies on declared but not initialized variables and dependencies in 'dead' code that the compiler removed before determining dependencies. A consequence of these changes is that functions called by initialization expressions that reference other initialization variables will run later than under previous versions since the initialization functions are not considered ready to be initialized until after (rather than immediately) their dependent variables are initialized. The following example illustrates the differences:

package  #main

var _ = myInit()

var sp = ""
func f(name string, _ ...interface{}) int {
        print(sp, name)
        sp = " "
        return 0
}

var a = f("a", x)
var b = f("b", y)
var c = f("c", z)
var d = func() int {
        if false {
                _ = z
        }
        return f("d")
}()
var e = f("e")

var x int
var y int = 42
var z int = func() int { return 42 }()
var _ = *i0
var i0 *int

func myInit() struct{} {
        print(sp+"myInit")
        sp = " "
        i0 = new(int)
        *i0 = 10
        return struct{}{}
}

func main() { println() }

Prior to 1.17 the output would be myInit a d e b c but with 1.17 it becomes e a b c d myInit.

1. a's dependency on x is now correctly detected.
2. d's dependency on z is now correctly detected since the dependency is considered before the dead code if false {... is eliminated.
3. myInit has to wait for i0 to be initialized before it can run, whereas prior to 1.17 i0 was assumed to initialized before the dependency chain was determined and hence would run first.