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: top-level var does not respect , rule #7320

Open
adonovan opened this issue Feb 13, 2014 · 9 comments
Assignees
Milestone

Comments

@adonovan
Copy link

@adonovan adonovan commented Feb 13, 2014

The following program fails with gc but succeeds with 'ssadump -run', because gc is
computing the initialization order incorrectly.

---------------

package main

// The package-level var initialization dependency graph contains the           
// following edges:                                                             
//      order     -> makeOrder                                                  
//      makeOrder -> b                                                          
//      makeOrder -> a                                                          
// Since a and b are incomparable in this partial order, we initialize          
// them in lexical order, a before b.  The total order of                       
// initialization is thus: [a, b, order].                                       
//                                                                              
// ssadump -run succeeds but go run fails.                                      
//                                                                              
// Interestingly, reversing the order of {b, a} in makeOrder (and {1,           
// 0} in the assertion) causes it to succeed with both tools, meaning           
// that gc uses the lexical order of the references within makeOrder,           
// not the lexical order of the declarations of a and b, to resolve             
// incomparability.                                                             

var counter int

func next() int {
        c := counter
        counter++
        return c
}

func makeOrder() [2]int {
        return [2]int{b, a}
}

var order = makeOrder()

func main() {
        if order != [2]int{1, 0} {
                panic(order) // gc panics (got {0, 1})                          
        }
}

var a, b = next(), next() // 0, 1                                               

---------------


(Again I feel a vague sense of deja vu, but couldn't find a duplicate report.)
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Feb 13, 2014

Comment 1:

Yes, r or gri reported the same thing last week I think, but I cannot find that one. The
issue is that the compiler treats all top-level variables as separate for the purposes
of computing an order, and so the "a before b" you are relying on is not there in the
compiler. However, it is also unclear to me whether it is really there in practice. What
about:
var a, b = f(), g()
func f() int {
    return b
}
func g() int {
    return 5
}
Today the compiler just initializes b - which means calling g - before calling f, as it
must. We could make this an init loop error, and perhaps we should, but I think that's a
language change at this point.
If I remember correctly, the earlier report (which I also cannot find) contained a
similar example. There's a spec question here before there's a compiler bug.
It would be nice to answer the spec question at least before Go 1.3.

Labels changed: added release-go1.3.

Status changed to Accepted.

@adonovan

This comment has been minimized.

Copy link
Author

@adonovan adonovan commented Feb 13, 2014

Comment 2:

I think the spec on this point is actually complete and unambiguous, if a little hard to
read.
"Within a package, package-level variables are initialized, and constant values are
determined, according to order of reference: if the initializer of A depends on B, A
will be set after B. Dependency analysis does not depend on the actual values of the
items being initialized, only on their appearance in the source. A depends on B if the
value of A contains a mention of B, contains a value whose initializer mentions B, or
mentions a function that mentions B, recursively. It is an error if such dependencies
form a cycle. If two items are not interdependent, they will be initialized in the order
they appear in the source, possibly in multiple files, as presented to the compiler."
(The word "mention" needs a proper definition, but its meaning is clear enough as it
relates to this issue.)
I think the compiler is correct to treat var x, y = f(), g() as equivalent to var x =
f(); var y = g(), i.e. all 1:1 initializations of top-level vars should be treated as if
declared separately.  (NB: var x, y = z() is quite different.)
In your example, the init graph contains these edges:
  a->f->b->g
which already gives us a total order.  This is a degenerate case since there are no
incomparable pairs, so we don't even need to look at the lexical order of declaration. 
Does anyone really think this should be an error?
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Feb 13, 2014

Comment 3:

I see, so you are only reporting that the tie-breaker is wrong, not that a, b = f(), g()
should always call f before g. The other issue was pointing out that the possibility of
calling g before f conflicted with the non-top-level semantics for var a, b = f(), g().
@adonovan

This comment has been minimized.

Copy link
Author

@adonovan adonovan commented Feb 13, 2014

Comment 4:

Yes, that's right.
For the record, I found the other bug you mentioned:
"evaluation order and initialization order specs are in contradiction"
https://golang.org/issue/7137
I don't have an opinion on that one, other than that the spec and implementations be
made to agree.
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Apr 3, 2014

Comment 5:

Simpler:
package main
var counter int
func next() int {
        c := counter
        counter++
        return c
}
var order = [2]int{b, a}
var a, b = next(), next() // 0, 1           
func main() {
    if order[0] != 1 || order[1] != 2 {
        println(order[0], order[1])
        panic("order")
    }
}
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Apr 3, 2014

Comment 6:

Issue #7134 has been merged into this issue.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented May 19, 2014

Comment 7:

Better simpler program:
package main
var order = [2]int{b, a}
var a, b = next(), next() // 0, 1
func main() {
    if a != 0 || b != 1 {
        println(a, b)
        panic("order")
    }
}
var counter int
func next() int {
    c := counter
    counter++
    return c
}
Not going to fix this for Go 1.3. It's too subtle and too unimportant.

Labels changed: added release-go1.4, removed release-go1.3.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Sep 15, 2014

Comment 8:

Labels changed: added release-go1.5, removed release-go1.4.

@griesemer

This comment has been minimized.

Copy link
Contributor

@griesemer griesemer commented Oct 1, 2014

Comment 9:

Labels changed: added repo-main.

@bradfitz bradfitz modified the milestone: Go1.5 Dec 16, 2014
@bradfitz bradfitz removed the release-go1.5 label Dec 16, 2014
@rsc rsc removed accepted labels Apr 14, 2015
@rsc rsc modified the milestones: Go1.6, Go1.5 May 19, 2015
@rsc rsc changed the title cmd/gc: top-level var does not respect , rule cmd/compile: top-level var does not respect , rule Jun 8, 2015
@rsc rsc modified the milestones: Unplanned, Go1.6 Dec 5, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.