cmd/compile: f.NamedValue entry for reg param causes register allocator miscompile

[thanm]

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

$ go version
go version devel go1.17-831573cd21 Fri May 21 03:21:56 2021 +0000 linux/amd64

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.

[thanm]

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:

L26:   func (w *M) walkOp(op *Op) *Op {
L27:	if op == nil {
L28:		return nil
L29:	}
L30:
L31:	orig := op
L32:	cloned := false
L33:	clone := func() {
L34:		if !cloned {
L35:			cloned = true
L36:			op = &Op{}
L37:			*op = *orig
L38:		}
L39:	}

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

  b2: <- b1
    v16 = Copy <mem> v9
    v17 = Load <*Op> v5 v16 (orig[*Op])
    v19 = VarDef <mem> {cloned} v16
    v20 = LocalAddr <*bool> {cloned} v2 v19
    v21 = Store <mem> {bool} v20 v18 v19
    v26 = VarDef <mem> {.autotmp_12} v21
    v27 = LocalAddr <*struct { F uintptr; cloned *bool; op **Op; orig *Op }> {.autotmp_12} v2 v26
    v28 = Store <mem> {struct { F uintptr; cloned *bool; op **Op; orig *Op }} v27 v22 v26
    v29 = LocalAddr <*struct { F uintptr; cloned *bool; op **Op; orig *Op }> {.autotmp_12} v2 v28
    v31 = NilCheck <void> v29 v28
    v32 = OffPtr <*uintptr> [0] v29
    v33 = Store <mem> {uintptr} v32 v30 v28
    v34 = LocalAddr <*bool> {cloned} v2 v33
    v35 = NilCheck <void> v29 v33
    v36 = OffPtr <**bool> [8] v29
    v37 = Store <mem> {*bool} v36 v34 v33
    v38 = NilCheck <void> v29 v37
    v39 = OffPtr <***Op> [16] v29
    v40 = Store <mem> {**Op} v39 v5 v37
    v41 = NilCheck <void> v29 v40
    v42 = OffPtr <**Op> [24] v29
    v43 = Store <mem> {*Op} v42 v17 v40
    v44 = Copy <func()> v29 (clone[func()]) DEAD
    v46 = Load <*Op> v5 v43
    v47 = NilCheck <void> v46 v43
    v48 = OffPtr <*[]P> [200] v46
    v49 = Load <[]P> v48 v43
    v50 = SliceLen <int> v49

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:

(*M).walkOp method(*M) func(*Op) *Op
  b1:
    v137 = ArgIntReg <*Op> {op+0} [1] (op[*Op])
    v138 = ArgIntReg <*M> {w+0} [0] (w[*M])
    v1 = InitMem <mem>
    ...

then in the inlined body of "clone" atht eline 46 callsite we get the following:

v94 = CALLstatic <*Op,mem> {AuxCall{runtime.newobject}} [16] v93 v62
    v96 = SelectN <*Op> [0] v94
    v95 = SelectN <mem> [1] v94
    v99 = VarDef <mem> {op} v95
    v75 = MOVQstore <mem> {op} v2 v96 v99
    v171 = CMPLconstload <flags> {runtime.writeBarrier} [val=0,off=0] v3 v75
    NE v171 -> b16 b10 (unlikely)
  b10: <- b17
    v34 = DUFFCOPY <mem> [224] v96 v137 v75
    Plain -> b21
  b16: <- b17

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:

b2: <- b1
    v29 = StoreReg <*Op> v137 : op[*Op]
    v116 = StoreReg <*M> v138 : w[*M]
    ...

and in the inlined body we now have:

    v94 = CALLstatic <*Op,mem> {AuxCall{runtime.newobject}} [16] v121 v62 : <AX>
    v96 = SelectN <*Op> [0] v94 : AX
    v95 = SelectN <mem> [1] v94
    v99 = VarDef <mem> {op} v95
    v75 = MOVQstore <mem> {op} v2 v96 v99
    v171 = CMPLconstload <flags> {runtime.writeBarrier} [val=0,off=0] v3 v75
    NE v171 -> b16 b10 (unlikely)
  b10: <- b17
    v12 = Copy <*Op> v96 : DI
    v133 = LoadReg <*Op> v29 : SI
    v34 = DUFFCOPY <mem> [224] v12 v133 v75

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.

[gopherbot]

Change https://golang.org/cl/321830 mentions this issue: cmd/compile: revert CL/316890