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: f.NamedValue entry for reg param causes register allocator miscompile #46304
Comments
The CL in question (which I wrote) was intended to improve the debugging experience: in cases where you have an incoming parameter that is passed in a single register, it adds the corresponding SSA value to the function's NamedValues table. This table is used later on in the compile to generate DWARF location expressions; by recording the association between the name + the SSA value, we get much better DWARF. At the time I wrote the CL, I had been under the impression that f.NamedValues was used only for debug/DWARF generation. Turns out this is actually not the case; the register allocator uses f.NamedValues as a way to pick the spill location for an SSA value if it needs to be written to memory. So, a pretty major oversight on my part there. Here's a portion of the code from the repro testcase:
The compiler emits code for the closure at line 33 that copies the values of "&op" and "orig" into the closure object. At an early stage in the compile this code looks something like
All of this code winds up getting optimized away however (since the function literal "clone" is inlined), and instead at a later point in the compile what we have is the following (this is before register allocation). In the entry block:
then in the inlined body of "clone" atht eline 46 callsite we get the following:
Note the reference to v137 in the call to duffcopy. This is a direct reference to the ArgIntReg defined in the entry; it corresponds to the original user variable "orig" (which has been optimized away). During register allocation, regalloc decides that it is going to spill v137 (a very logical spill candidate, since in b2 all of its uses are far in the future). As a result we get the following in b2:
and in the inlined body we now have:
Because there is an entry in the f.NamedValues map for v137, we store the value to its home slot. The problem here is that "op" is redefined -- the store v75 writes the new value of op to its home location, meaning that the result of the call to newObject overwrites the saved (spilled) value of 137. This means that the LoadReg at v133 is loading up not the saved value, but the new value just assigned to "op". This means that the "src" argument when invoking duffcopy is the same as the destination, so the effect is that instead of copying "*op" we wind up with a zero value object instead. This is what triggers the crash. I think it is pretty clear from this that https://go-review.googlesource.com/c/go/+/316890 needs to be reverted. The more interesting question is what we should do to restore a better debugging experience for functions with register parameters-- it seems a shame that we can't find some way to report to the user that "op" is in RBX at the start of this function. |
Change https://golang.org/cl/321830 mentions this issue: |
What version of Go are you using (
go version
)?Does this issue reproduce with the latest release?
This is a problem on tip, not in previous releases.
What operating system and processor architecture are you using (
go env
)?linux/amd64
What did you do?
Run this program:
https://play.golang.org/p/spT4IJFC6_n
What did you expect to see?
Program should print "anew" and terminate.
What did you see instead?
panic: runtime error: index out of range [0] with length 0
goroutine 1 [running]:
main.(*M).walkOp(0xc0000f0e08, 0xc0000f4000)
/tmp/repro.go:50 +0x29c
main.main()
/tmp/repro.go:63 +0x145
exit status 2
Bisection identifies this commit as the problem:
https://go.googlesource.com/go/+/f62739b8611a0f1c96e59eb6574422562bb46233
which corresponds to the CL https://go-review.googlesource.com/c/go/+/316890.
The text was updated successfully, but these errors were encountered: