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: bad line number assigned to instruction #53456

Closed
aarzilli opened this issue Jun 19, 2022 · 8 comments
Closed

cmd/compile: bad line number assigned to instruction #53456

aarzilli opened this issue Jun 19, 2022 · 8 comments
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Debugging FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@aarzilli
Copy link
Contributor

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 cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 23, 2022
@cagedmantis cagedmantis added this to the Backlog milestone Jun 23, 2022
@cagedmantis
Copy link
Contributor

/cc @randall77 @griesemer

@dr2chase dr2chase self-assigned this Jun 24, 2022
@gimwu
Copy link

gimwu commented Jul 13, 2022

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 gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022
@gopherbot
Copy link

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

@dr2chase dr2chase modified the milestones: Backlog, Go1.20 Jul 14, 2022
@dr2chase dr2chase added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jul 14, 2022
@dr2chase
Copy link
Contributor

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

@mdempsky
Copy link
Member

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
Copy link
Member

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 mdempsky reopened this Aug 11, 2022
@mdempsky
Copy link
Member

mdempsky commented Aug 11, 2022

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
Copy link

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

@golang golang locked and limited conversation to collaborators Aug 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Debugging FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

7 participants