cmd/compile: bad line number assigned to instruction

[aarzilli]

Compiling the following with -gcflags='all=-N -l':

package main

type T struct {
	m map[int]int
}

func main() {
	t := T{
		m: make(map[int]int),
	}
	t.Inc(5)
	t.Inc(7)
}

func (s *T) Inc(key int) {
	v := s.m[key] // break, line 16
	v++
	s.m[key] = v // also here
}

Produces a binary for main.(*T).Inc with this:

test.go:16	S	0x45fd3e	8400	TESTB AL, 0(AX)
test.go:16		0x45fd40	488b18	MOVQ 0(AX), BX
test.go:16		0x45fd43	48895c2430	MOVQ BX, 0x30(SP)
test.go:15	S	0x45fd48	488b4c2450	MOVQ 0x50(SP), CX
test.go:16	S	0x45fd4d	488d05cc5c0000	LEAQ main.(*T).Inc+23808(SB), AX
test.go:16		0x45fd54	e8a7f9faff	CALL runtime.mapaccess1_fast64(SB)  >>>

The instruction at 0x45fd48 which passes key to mapaccess1_fast64 is assigned the function's header as its line number instead of the statement it belongs to.

ref. go-delve/delve#3033
cc @dr2chase

[cagedmantis]

/cc @randall77 @griesemer

[gimwu]

My version is go1.18.1 and I had the same problem. I tested between different versions and it seems that this problem was only discovered after go 1.16?

[gopherbot]

Change https://go.dev/cl/417076 mentions this issue: cmd/compile: avoid copying Pos from ONAME when creating converts for maps

[dr2chase]

I plan to wait till 1.20, unless perhaps we backport it into a dot release of 1.19.

[mdempsky]

I think the fix is safe to backport to 1.18/1.19 if we really wanted to, but it doesn't seem to meet the bar for meriting a backport if we haven't found the issue until now. (I suspect it was introduced in Go 1.17, due to the large refactoring that broke up cmd/compile into multiple subpackages.)

[mdempsky]

Looks like the fix is failing on linux-ppc64-buildlet: https://build.golang.org/log/52b5e5dc32bb556730136131f90111ebc7ec9c71

--- FAIL: TestDebugLinesSayHi (8.24s)
    debug_lines_test.go:269: wanted stmts [9 10 11] but got [8 9 10 11]
--- FAIL: TestDebugLines_53456 (4.12s)
    debug_lines_test.go:264: wanted stmts [16 17 18] but got [15 16 17 18] (with repeats still in: [15 16 17 18])
FAIL
FAIL	cmd/compile/internal/ssa	13.063s

[mdempsky]

It looks like it's because hasRegisterAbi is missing "ppc64":

func hasRegisterAbi() bool {
	switch testGoArch() {
	case "amd64", "arm64", "ppc64le", "riscv":
		return true
	}
	return false
}

I think it might be more robust to use go/build and write it as:

func hasRegisterAbi() bool {
        for _, tag := range build.Default.ToolTags {
	        if tag == "goexperiment.regabi" {
                        return true
                }
	}
	return false
}

Edit: Nevermind, I overlooked that testGoArch() could be overridden with a flag. Just updating the switch seems best for now.

[gopherbot]

Change https://go.dev/cl/422957 mentions this issue: cmd/compile/internal/ssa: include "ppc64" in has-regabi arches list