Skip to content

[-v] cmd/compile: incorrect package initialization order for spec example #2

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

Closed
wants to merge 1 commit into from

Conversation

chadwhitacre
Copy link
Owner

@chadwhitacre chadwhitacre commented Nov 28, 2018

Picking up from #1 ...

Todo

  • determine state needed in walk in order to generate lout properly
  • populate the state!
  • generate lout properly
  • clean up any code elsewhere in Go that depends on this buggy behavior

Then

  • find a nicer way to initialize Deps
  • consider bool instead of struct{} for deps
  • unit test Deps
  • I guess static variables are added directly so shouldn't be included?
  • ensure all sorts of assignments are covered, expand test suite
  • make sure pointer usage is correct
  • protect against infinite looping
  • check performance
  • sort dcls by number of deps?
  • test traversing across multiple functions
  • test different GOARCH/GOOS?
  • run whole test suite

Meanwhile

Finally

Further

  • investigate OAS2 and OASOP—what happens if I x,y = 1,2 or x = 1; x += 1 in xtop?
  • investigate Ninit—does initreorder ever actually recurse? Can we get some test coverage reporting?
  • investigate nf—what if we have init functions but no statements? really return early?

@chadwhitacre
Copy link
Owner Author

Okay! Failing test. Now what? 😬

@chadwhitacre
Copy link
Owner Author

Are there some side-effects to initreorder (beyond sinit.go)? If not then I am thinking about maybe starting a rewrite there. Can we store state in some intermediate data structure (a la initplans), which we update while walking and then use to generate the final lout?

@chadwhitacre
Copy link
Owner Author

Here are the functions in sinit.go:

addMapEntries
addvalue
anylit
fixedlit
foundinitloop
genAsStatic
getdyn
getlit
init1
init2
init2list
initfix
initplan
initreorder
isLiteral
isStaticCompositeLiteral
isZero
isvaluelit
litas
maplit
oaslit
slicelit
stataddr
staticassign
staticcopy
staticinit
staticname

@chadwhitacre
Copy link
Owner Author

chadwhitacre commented Nov 28, 2018

Just looking at the requirements, my first thought is to keep a) a deque of variable names in declaration order, and b) a map of variable names to stacks of requirements for each variable. When we see a LHS we push the variable name onto (a) and file an empty stack under the variable name in (b). During RHS processing we push dependencies onto the stack in (b). After processing a complete declaration we flatten (a) and (b) by recursing through them and appending to the out list when we find an empty stack in (b).

#1 (comment)

@chadwhitacre
Copy link
Owner Author

chadwhitacre commented Nov 29, 2018

Here is global state:

initlist
initplans
inittemps

And here is the call graph ( indicates a nested recursion):

initfix
  initreorder
    initreorder
    init1
      init1
      init2
        init1 ↺
          init2list
            init2 ↺
      init2list
        init2
      foundinitloop
      staticinit
        staticassign
          staticassign
          initplan
            addvalue
              initplan ↺
              isZero
                isZero
              isvaluelit
          isZero
            isZero
          stataddr
            getlit
          staticcopy
            staticcopy
            isZero
              isZero
            staticassign ↺
          staticname

Aaaand here is a call graph coming in from walk.go(!) ( indicates a nested recursion):

oaslit
  anylit
    anylit
    slicelit
      fixedlit
        fixedlit
        genAsStatic
          stataddr
        isLiteral
        slicelit ↺
      getdyn
        getdyn
        isLiteral
      isLiteral
      stataddr
        stataddr
        getlit
      staticname
    fixedlit
    staticname
  maplit
    addMapEntries
    isStaticCompositeLiteral
      isStaticCompositeLiteral
    litas
    fixedlit
    staticname

@chadwhitacre
Copy link
Owner Author

Yeah, okay, getting back into it: right now declarations are sorted by a) reverse dependency and b) RHS appearance (i.e., usage or reference), but they need to be sorted by a) reverse dependency and b) LHS appearance (i.e., declaration).

That is, b and c still need to come before a, but b needs to come before c.

@chadwhitacre
Copy link
Owner Author

With 5cb3111:

$ pwd
/Users/whit537/workbench/go/src/github.com/golang/go/src
$ ./make.bash && ../bin/go run ../test/run.go -- ../test/fixedbugs/issue22326.go
[...]
# go run run.go -- ../test/fixedbugs/issue22326.go
exit status 1
# command-line-arguments
declaring a
declaring c
declaring d
declaring b
panic: b is 5 not 4

goroutine 1 [running]:
main.expect(...)
        /Users/whit537/workbench/go/src/github.com/golang/go/test/fixedbugs/issue22326.go:21
main.main()
        /Users/whit537/workbench/go/src/github.com/golang/go/test/fixedbugs/issue22326.go:26 +0x267
exit status 2

FAIL    ../test/fixedbugs/issue22326.go 2.070s
exit status 1
$

a c d b is not the order I expected here! I expected a b c d. 🤔

@chadwhitacre
Copy link
Owner Author

With 9e14150:

# go run run.go -- ../test/fixedbugs/issue22326.go
exit status 1
# command-line-arguments
seeing a
declaring a
declaring c
declaring d
declaring b
seeing b
seeing c
seeing d
panic: b is 5 not 4

goroutine 1 [running]:
main.expect(...)
        /Users/whit537/workbench/go/src/github.com/golang/go/test/fixedbugs/issue22326.go:21
main.main()
        /Users/whit537/workbench/go/src/github.com/golang/go/test/fixedbugs/issue22326.go:26 +0x267
exit status 2

FAIL    ../test/fixedbugs/issue22326.go 1.083s
exit status 1
$

@chadwhitacre
Copy link
Owner Author

36d9e34

0 declaring a
first time using a
first time using c
first time using d
first time using b
0 declaring b
0 declaring c
0 declaring d

@chadwhitacre
Copy link
Owner Author

Here's a sketch of data structures and an algorithm to add to initfix:

dcls: [a,b,c,d]

deps:
	a: [c,b]
	b: [d]
	c: [d]
	d: []

depsback:
	d: [c,b]
	c: [a]
	b: [a]
	a: []

while dcls:
	for n in dcls:
		if deps[n]:
			continue

		lout.append(n)
		dcls.remove(n)

		for o in depsback[n]:
			deps[o].remove(n)

@chadwhitacre
Copy link
Owner Author

c2cebfe:

declaring a at depth 0
first time using a
first time using c
first time using d
first time using b
declaring b at depth 0
declaring c at depth 0
declaring d at depth 0
 out: a, b, c, d
lout: c, b, a

@chadwhitacre
Copy link
Owner Author

chadwhitacre commented Dec 3, 2018

Bringing back append logging in 91b85e4 .

declaring a at depth 0
first time using a
first time using c
first time using d
appending (1) c
first time using b
appending (1) b
appending (1) a
declaring b at depth 0
declaring c at depth 0
declaring d at depth 0
dcls: a, b, c, d
 out: a, b, c, d
lout: c, b, a

@chadwhitacre
Copy link
Owner Author

Here's logging that includes the d = 3 assignment: 02a1c00.

declaring a at depth 0
expecting a
expecting c
expecting d
s'initing d
appending c at point (1)
expecting b
appending b at point (1)
appending a at point (1)
declaring b at depth 0
declaring c at depth 0
declaring d at depth 0
dcls: a, b, c, d
 out: a, b, c, d
lout: c, b, a

@chadwhitacre
Copy link
Owner Author

Alright, sooo ... I'm pretty sure expecting always comes before appending/s'initing. I think that's the time to populate keys in dep/depsback. But ... values?

@chadwhitacre
Copy link
Owner Author

Has to be append/s'init time. What does that look like?

When I am appending c what can I say about the dependencies that c is a part of? I want to say one or both of:

deps:
	a: [c]
	c: [d]

depsback:
	c: [a]
	d: [c]

(It's trivial to populate both deps and depsback at the same time.)

@chadwhitacre
Copy link
Owner Author

Progress! Now how about b expecting d? That's the one we need!

4e75238

declaring a at depth 0
root is expecting a
a is expecting c
c is expecting d
s'initing d
appending c at point (1)
a is expecting b
appending b at point (1)
appending a at point (1)
declaring b at depth 0
declaring c at depth 0
declaring d at depth 0
dcls: a, b, c, d
 out: a, b, c, d
lout: c, b, a

@chadwhitacre
Copy link
Owner Author

I'm referring to golang#22326 (comment) in another tab through all of this, btw.

@chadwhitacre
Copy link
Owner Author

ea56f22 🤔

declaring a at depth 0
root is expecting a
a is expecting c
c is expecting d
s'initing d
already done: d
already done: d
already done: d
already done: f
appending c at point (1)
a is expecting b
already done: f
appending b at point (1)
already done: c
already done: b
appending a at point (1)
declaring b at depth 0
declaring c at depth 0
declaring d at depth 0
dcls: a, b, c, d
 out: a, b, c, d
lout: c, b, a

@chadwhitacre
Copy link
Owner Author

7c1eca0 💃

declaring a at depth 0
root is expecting a
a is expecting c
c is expecting function f
f is expecting d
s'initing d
already done: d
already done: d
already done: d
already done: f
appending c at point (1)
a is expecting b
already done: f
appending b at point (1)
already done: c
already done: b
appending a at point (1)
declaring b at depth 0
declaring c at depth 0
declaring d at depth 0
dcls: a, b, c, d
 out: a, b, c, d
lout: c, b, a

@chadwhitacre
Copy link
Owner Author

Alright, so we can get from c to d through f, but still where is b is expecting function f?

@chadwhitacre
Copy link
Owner Author

Got it! 😍 💃

6504037

declaring a at depth 0
root is expecting a
a is expecting c
c is expecting f
f is expecting d
s'initing d
f is expecting d
f is expecting d
f is expecting d
f is expecting d
appending c at point (1)
a is expecting b
b is expecting f
appending b at point (1)
appending a at point (1)
root is expecting c
root is expecting b
declaring b at depth 0
root is expecting b
root is expecting f
declaring c at depth 0
root is expecting c
root is expecting f
declaring d at depth 0
root is expecting d
dcls: a, b, c, d
 out: a, b, c, d
lout: c, b, a

@chadwhitacre
Copy link
Owner Author

💃💃💃💃💃💃

81f01ab

fwd:
        a: [c, b]
        c: [f]
        f: [d]
        b: [f]
bck:
        f: [c, b]
        d: [f]
        b: [a]
        a: []
        c: [a]

@chadwhitacre
Copy link
Owner Author

cb6308f

dcls: a, b, c, d
fwd:
        a: [c, b]
        c: [f]
        f: [d]
        b: [f]
bck:
        a: []
        c: [a]
        f: [b, c]
        d: [f]
        b: [a]
out: 
processing a (=) ... skipped!
processing b (=) ... skipped!
processing c (=) ... skipped!
processing d (=) ... go!
dcls: a, b, c, d
fwd:
        a: [b, c]
        c: [f]
        f: [d]
        b: [f]
bck:
        a: []
        c: [a]
        f: [c, b]
        d: [f]
        b: [a]
 out: d
lout: c, b, a
panic: b is 5 not 4

@chadwhitacre
Copy link
Owner Author

f5cdd46

dcls: a, b, c, d
fwd:
        a: [c, b]
        c: [f]
        f: [d]
        b: [f]
bck:
        a: []
        c: [a]
        f: [c, b]
        d: [f]
        b: [a]
out:
f is a NAME and Func is &{<S>    [] [] [] 0 map[] <nil> <N> 0 <N> <N> <nil> 0xc000086bc0 0 {0 0} {0 0} 0 128 <nil>}
dcls: a, b, c
fwd:
        a: [c, b]
        c: [f]
        f: [d]
        b: [f]
bck:
        a: []
        c: [a]
        f: [b, c]
        d: [f]
        b: [a]
 out: d
lout: c, b, a
panic: b is 5 not 4

@chadwhitacre
Copy link
Owner Author

!!!!!!

removing c
removing c from fwd[a]
removing a from bck[c]
 dcls: a
  fwd:
        a: []
        c: []
        f: []
        b: []
  bck:
        a: []
        c: []
        f: []
        d: []
        b: []
  out: d, b, c
a ready? true
removing a
 dcls: 
  fwd:
        a: []
        c: []
        f: []
        b: []
  bck:
        d: []
        b: []
        a: []
        c: []
        f: []
  out: d, b, c, a
 lout: c, b, a
panic: b is 5 not 4

@chadwhitacre
Copy link
Owner Author

😍 😍 😍 😍 😍 😍 😍 😍 😍 😍 😍

  out: b (=), c (=), a (=)
 lout: c (=), b (=), a (=)

@chadwhitacre
Copy link
Owner Author

That's acbf979. Still a ways to go, apparently a) lout != out for other cases, and b) there are some infinite loops.

@chadwhitacre
Copy link
Owner Author

Okay, trying to button together out and lout. Level-set with master 048c916:

$ make.bash
Building Go cmd/dist using /usr/local/Cellar/go/1.10.1/libexec.
Building Go toolchain1 using /usr/local/Cellar/go/1.10.1/libexec.
Building Go bootstrap cmd/go (go_bootstrap) using Go toolchain1.
Building Go toolchain2 using go_bootstrap and Go toolchain1.
Building Go toolchain3 using go_bootstrap and Go toolchain2.
Building packages and commands for darwin/amd64.
---
Installed Go for darwin/amd64 in /Users/whit537/workbench/go/src/github.com/golang/go
Installed commands in /Users/whit537/workbench/go/src/github.com/golang/go/bin

@chadwhitacre
Copy link
Owner Author

DWARFRegisters is an empty map because it's declared thus:

// LinkArch is the definition of a single architecture.
type LinkArch struct {
*sys.Arch
Init func(*Link)
Preprocess func(*Link, *LSym, ProgAlloc)
Assemble func(*Link, *LSym, ProgAlloc)
Progedit func(*Link, *Prog, ProgAlloc)
UnaryDst map[As]bool // Instruction takes one operand, a destination.
DWARFRegisters map[int16]int16
}

Soooo ... the default value is what we're seeing, we're not seeing the result of ... setting DWARFRegisters, which happens in ... obj6, ya?

var Linkamd64 = obj.LinkArch{
Arch: sys.ArchAMD64,
Init: instinit,
Preprocess: preprocess,
Assemble: span6,
Progedit: progedit,
UnaryDst: unaryDst,
DWARFRegisters: AMD64DWARFRegisters,
}

Wasn't that not showing up earlier? Yes. 🤔

@chadwhitacre
Copy link
Owner Author

Actually I think that's a lie.

@chadwhitacre
Copy link
Owner Author

(Regarding the default value.)

@chadwhitacre
Copy link
Owner Author

Okay maybe it's not a lie:

https://play.golang.org/p/NJrROy3SkSk

package main

import (
	"fmt"
)

type LinkArch struct {
	DWARFRegisters map[int16]int16
}

func main() {
	var arch = new(LinkArch)
	println(fmt.Sprintf("%v", arch.DWARFRegisters))
}

Outputs:

map[]

@chadwhitacre
Copy link
Owner Author

chadwhitacre commented Dec 27, 2018

Alright, so we have something like this:

https://play.golang.org/p/XDuMFzoSM2m

package main

import (
	"fmt"
)

var AMD64DWARFRegisters = map[int16]int16{ 
	0: 42,
}

type LinkArch struct {
	DWARFRegisters map[int16]int16
}

func main() {
	var arch = LinkArch{
		DWARFRegisters: AMD64DWARFRegisters,
	}
	println(fmt.Sprintf("%v", arch.DWARFRegisters))
}
map[0:42]

Except there is package traversal in there. And it breaks.

@chadwhitacre
Copy link
Owner Author

71fbcb8 ☺️

$ ./foo 
$

@chadwhitacre
Copy link
Owner Author

Okay! So why does that work? 🤔

Tune in next time! 😬

@chadwhitacre
Copy link
Owner Author

Test passing in 81daac1. 👍

/Volumes/workbench/go/src/github.com/golang/go
total 1384
-rw-r--r--    1 whit537  wheel    55297 Nov 25 08:12 AUTHORS
-rw-r--r--    1 whit537  wheel     1339 Nov 25 08:12 CONTRIBUTING.md
-rw-r--r--    1 whit537  wheel    71104 Nov 25 08:12 CONTRIBUTORS
-rw-r--r--    1 whit537  wheel     1479 Nov 25 08:10 LICENSE
-rw-r--r--    1 whit537  wheel     1303 Nov 25 08:10 PATENTS
-rw-r--r--    1 whit537  wheel     1607 Nov 25 08:12 README.md
-rw-r--r--    1 whit537  wheel       45 Dec 27 12:02 VERSION.cache
drwxr-xr-x   17 whit537  wheel      544 Nov 25 08:12 api/
drwxr-xr-x    4 whit537  wheel      128 Dec 27 12:07 bin/
drwxr-xr-x   49 whit537  wheel     1568 Nov 25 08:12 doc/
-rw-r--r--    1 whit537  wheel     5686 Nov 25 08:10 favicon.ico
drwxr-xr-x    3 whit537  wheel       96 Nov 25 08:10 lib/
drwxr-xr-x   16 whit537  wheel      512 Nov 25 08:12 misc/
drwxr-xr-x    7 whit537  wheel      224 Dec 27 12:02 pkg/
-rw-r--r--    1 whit537  wheel       26 Nov 25 08:10 robots.txt
drwxr-xr-x   69 whit537  wheel     2208 Dec 27 12:08 src/
-rw-r--r--    1 whit537  wheel  1249708 Dec 27 12:07 tags
drwxr-xr-x  306 whit537  wheel     9792 Nov 25 08:12 test/
$ which go
./bin/go
$ go run test/run.go -- test/fixedbugs/issue22326.go 
$

@chadwhitacre chadwhitacre force-pushed the 22326 branch 2 times, most recently from dcf1787 to bc55a9a Compare December 27, 2018 17:22
@chadwhitacre
Copy link
Owner Author

68db6f2

$ time (./make.bash -v > out 2>&1 && ../bin/go run ../test/run.go -- ../test/fixedbugs/issue22326.go >> out 2>&1); echo 'final: done!' >> out && say 'yeah'

real    5m13.443s
user    9m4.120s
sys     1m17.023s
$

@chadwhitacre
Copy link
Owner Author

Significant slowdown relative to 048c916:

$ time (./make.bash -v > out 2>&1 && ../bin/go run ../test/run.go -- ../test/fixedbugs/issue22326.go >> out 2>&1); echo 'final: done!' >> out && say 'yeah'

real    3m40.285s
user    7m41.508s
sys     1m14.226s
$

@chadwhitacre
Copy link
Owner Author

Squashed! Getting ready to contribute!

@chadwhitacre
Copy link
Owner Author

Rebased on latest master (af43203).

$ time (./make.bash -v > out 2>&1 && ../bin/go run ../test/run.go -- ../test/fixedbugs/issue22326.go >> out 2>&1); echo 'final: done!' >> out && say 'yeah'

real    5m37.722s
user    10m2.047s
sys     1m18.599s
$

@chadwhitacre
Copy link
Owner Author

Alright, I'm going to give this a once-over before submitting. Not planning to address any of the big issues in the issue description (out of time! :( ) but just basic reorganization and gofmting ...

@chadwhitacre
Copy link
Owner Author

Baseline. 👍

$ time (./all.bash -v > out 2>&1); say 'yeah'                                                                  
real    26m59.555s
user    51m9.112s
sys     20m42.971s
$

@chadwhitacre
Copy link
Owner Author

Okay! I have a commit! e041f1e

Now, how do I get that into Gerrit? 🤔

@chadwhitacre
Copy link
Owner Author

I mean, besides letting Gopherbot do it for me. 😆

The spec advertises the sort order for variable initialization as
dependency and then declaration, but the compiler currently orders
initialization by dependency and then use. This CL introduces NodeSet
and Deps structs in sinit.go, uses instances of these structs in the
call chain under initreorder to track dependency information, and uses
this dependency information back in initfix to properly order its
output.

Fixing the order of the output of initfix ran afoul of a complex bug in
the build toolchain that I did not have time to fully comprehend. This
CL works around it by constructing obj.LinkArches at run time instead of
compile time.

Unfortunately, this CL results in a performance penalty for make.bash:
it takes half-again as long to run with as without.

Fixes golang#22326
@chadwhitacre
Copy link
Owner Author

I've figured out how to convince git codereview change to do something good. Now rerunning the test suite ...

@chadwhitacre
Copy link
Owner Author

$ time (./all.bash -v > out 2>&1); say 'yeah'

real    16m26.151s
user    30m46.513s
sys     8m16.435s
$
$ tail out
ok      cmd/vendor/github.com/google/pprof/profile      0.165s
ok      cmd/vendor/github.com/ianlancetaylor/demangle   0.051s
ok      cmd/vendor/golang.org/x/arch/arm/armasm 0.039s
ok      cmd/vendor/golang.org/x/arch/arm64/arm64asm     0.334s
ok      cmd/vendor/golang.org/x/arch/ppc64/ppc64asm     0.027s
ok      cmd/vendor/golang.org/x/arch/x86/x86asm 0.179s
ok      cmd/vendor/golang.org/x/crypto/ssh/terminal     0.020s
ok      cmd/vendor/golang.org/x/sys/unix        3.573s
ok      cmd/vet 20.944s
2019/01/03 18:47:00 Failed: exit status 2
$

🤔

@chadwhitacre
Copy link
Owner Author

Okay the test suite is totally hosed. 😞

@chadwhitacre
Copy link
Owner Author

💃

$ git codereview mail
remote: Processing changes: refs: 1, new: 1, done    
remote: 
remote: SUCCESS
remote: 
remote: New Changes:
remote:   https://go-review.googlesource.com/c/go/+/156328 cmd/compile: initialize variables in the right order
$ 

Moving to https://go-review.googlesource.com/c/go/+/156328/! 😍

@chadwhitacre chadwhitacre deleted the 22326 branch January 5, 2019 00:01
@chadwhitacre
Copy link
Owner Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant