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: degraded pcln quality on 1.18 #47880

Closed
aarzilli opened this issue Aug 22, 2021 · 8 comments
Closed

cmd/compile: degraded pcln quality on 1.18 #47880

aarzilli opened this issue Aug 22, 2021 · 8 comments
Assignees
Milestone

Comments

@aarzilli
Copy link
Contributor

@aarzilli aarzilli commented Aug 22, 2021

$ go version
go version devel go1.18-6e50991d2a Sat Aug 21 18:23:58 2021 +0000 linux/amd64

Compile https://github.com/go-delve/delve/blob/master/_fixtures/parallel_next.go with -gcflags='all=-N -l', the instruction MOVQ 0x98(SP), AX at 0x493f0c and 0x493faf is assigned to line :8 (the header of main.sayhi) and has the stmt flag set, causing delve to move back and forth from the body of the function to the header.

On 1.17 the same instruction is generated but it is assigned to the surrounding line of code and does not have the stmt flag set.

cc @dr2chase

@toothrot toothrot added this to the Backlog milestone Aug 24, 2021
@dr2chase
Copy link
Contributor

@dr2chase dr2chase commented Aug 24, 2021

For reference, input source code is, function in question is sayhi


package main

import (
	"fmt"
	"sync"
)

func sayhi(n int, wg *sync.WaitGroup) {
	fmt.Println("hi", n)
	fmt.Println("hi", n)
	wg.Done()
}

func main() {
	var wg sync.WaitGroup
	wg.Add(10)
	for i := 0; i < 10; i++ {
		go sayhi(i, &wg)
	}
	wg.Wait()
}

The change is in the AST input to the back-end (look at the second-to-the-last line):

buildssa-body
. BLOCK # main.go:10:13
. BLOCK-List
. . AS tc(1) # main.go:10:13
. . . NAME-main..autotmp_2 esc(N) tc(1) Class:PAUTO Offset:0 Addrtaken AutoTemp OnStack Used ARRAY-[2]interface {} # main.go:10:13
. . AS tc(1) # main.go:10:13
. . . NAME-main..autotmp_4 esc(N) tc(1) Class:PAUTO Offset:0 AutoTemp OnStack Used PTR-*[2]interface {} # main.go:10:13
. . . ADDR tc(1) PTR-*[2]interface {} # main.go:10:13 PTR-*[2]interface {}
. . . . NAME-main..autotmp_2 esc(N) tc(1) Class:PAUTO Offset:0 Addrtaken AutoTemp OnStack Used ARRAY-[2]interface {} # main.go:10:13
. . BLOCK # main.go:10:14
. . BLOCK-List
. . . AS tc(1) # main.go:10:14
. . . . INDEX tc(1) Bounded INTER-interface {} # main.go:10:13 INTER-interface {}
. . . . . DEREF tc(1) Implicit ARRAY-[2]interface {} # main.go:10:13 ARRAY-[2]interface {}
. . . . . . NAME-main..autotmp_4 esc(N) tc(1) Class:PAUTO Offset:0 AutoTemp OnStack Used PTR-*[2]interface {} # main.go:10:13
. . . . . LITERAL-0 tc(1) int # main.go:10:13
. . . . EFACE tc(1) INTER-interface {} # main.go:10:14 INTER-interface {}
. . . . . ADDR tc(1) PTR-*uint8 # main.go:10:14 PTR-*uint8
. . . . . . LINKSYMOFFSET tc(1) Offset:0 uint8 uint8
. . . . . ADDR tc(1) PTR-*string # main.go:10:14 PTR-*string
. . . . . . NAME-main..stmp_0 tc(1) Class:PEXTERN Offset:0 Addrtaken Readonly Used string # main.go:10:14
. . BLOCK # main.go:10:14
. . BLOCK-List
. . . AS-init
. . . . AS tc(1) # main.go:10:14
. . . . . NAME-main..autotmp_5 esc(N) tc(1) Class:PAUTO Offset:0 AutoTemp OnStack Used UNSAFEPTR-unsafe.Pointer # main.go:10:14
. . . . . CALLFUNC tc(1) Walked UNSAFEPTR-unsafe.Pointer # main.go:10:14 UNSAFEPTR-unsafe.Pointer
. . . . . . NAME-runtime.convT64 tc(1) Class:PFUNC Offset:0 Used FUNC-func(uint64) unsafe.Pointer
. . . . . CALLFUNC-Args
. . . . . . CONV tc(1) uint64 # main.go:9:12 uint64
. . . . . . . NAME-main.n esc(no) tc(1) Class:PPARAM Offset:0 OnStack Used int # main.go:9:12

versus the earlier

buildssa-body
. BLOCK # main.go:10
. BLOCK-List
. . AS tc(1) # main.go:10
. . . NAME-main..autotmp_2 esc(N) tc(1) Class:PAUTO Offset:0 Addrtaken AutoTemp OnStack Used ARRAY-[2]interface {} # main.go:10
. . AS tc(1) # main.go:10
. . . NAME-main..autotmp_4 esc(N) tc(1) Class:PAUTO Offset:0 AutoTemp OnStack Used PTR-*[2]interface {} # main.go:10
. . . ADDR tc(1) PTR-*[2]interface {} # main.go:10 PTR-*[2]interface {}
. . . . NAME-main..autotmp_2 esc(N) tc(1) Class:PAUTO Offset:0 Addrtaken AutoTemp OnStack Used ARRAY-[2]interface {} # main.go:10
. . BLOCK # main.go:10
. . BLOCK-List
. . . AS tc(1) # main.go:10
. . . . INDEX tc(1) Bounded INTER-interface {} # main.go:10 INTER-interface {}
. . . . . DEREF tc(1) Implicit ARRAY-[2]interface {} # main.go:10 ARRAY-[2]interface {}
. . . . . . NAME-main..autotmp_4 esc(N) tc(1) Class:PAUTO Offset:0 AutoTemp OnStack Used PTR-*[2]interface {} # main.go:10
. . . . . LITERAL-0 tc(1) int # main.go:10
. . . . EFACE tc(1) INTER-interface {} # main.go:10 INTER-interface {}
. . . . . ADDR tc(1) PTR-*uint8 # main.go:10 PTR-*uint8
. . . . . . LINKSYMOFFSET tc(1) Offset:0 uint8 uint8
. . . . . ADDR tc(1) PTR-*string # main.go:10 PTR-*string
. . . . . . NAME-main..stmp_0 tc(1) Class:PEXTERN Offset:0 Addrtaken Readonly Used string # main.go:10
. . BLOCK # main.go:10
. . BLOCK-List
. . . AS-init
. . . . AS tc(1) # main.go:10
. . . . . NAME-main..autotmp_5 esc(N) tc(1) Class:PAUTO Offset:0 AutoTemp OnStack Used UNSAFEPTR-unsafe.Pointer # main.go:10
. . . . . CALLFUNC tc(1) Use:1 Walked UNSAFEPTR-unsafe.Pointer # main.go:10 UNSAFEPTR-unsafe.Pointer
. . . . . . NAME-runtime.convT64 tc(1) Class:PFUNC Offset:0 Used FUNC-func(uint64) unsafe.Pointer
. . . . . CALLFUNC-Args
. . . . . . CONV tc(1) uint64 # main.go:10 uint64
. . . . . . . NAME-main.n esc(no) tc(1) Class:PPARAM Offset:0 OnStack Used int # main.go:9

@mdempsky does anything here leap out at you? To me it looks like the CONV picked up the pos of its input's declaration, instead of its location in the source code.

Loading

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Aug 24, 2021

Does the issue still reproduce with GOEXPERIMENT=unified? -G=3 calculates position information differently than -G=0 did; but unified IR should more closely match the position information generated by -G=0.

Loading

@aarzilli
Copy link
Contributor Author

@aarzilli aarzilli commented Aug 25, 2021

Also happens with GOEXPERIMENT=unified.

Loading

@mdempsky mdempsky self-assigned this Aug 25, 2021
@mdempsky
Copy link
Member

@mdempsky mdempsky commented Aug 25, 2021

Oh, it looks like it happens with -G=0 too. Seems like a change in walk then. Bisecting.

Loading

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Aug 25, 2021

Bisect identified 57668b8.

/cc @randall77 @danscales

Loading

@mdempsky mdempsky assigned randall77 and unassigned mdempsky Aug 25, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Aug 25, 2021

Change https://golang.org/cl/345095 mentions this issue: cmd/compile: use right line number for conversion expression

Loading

@gopherbot gopherbot closed this in bb0b511 Aug 31, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Sep 9, 2021

Change https://golang.org/cl/348970 mentions this issue: cmd/compile: extend dump-to-file to handle "genssa" (asm) case.

Loading

gopherbot pushed a commit that referenced this issue Sep 20, 2021
Extend the existing dump-to-file to also do assembly output
to make it easier to write debug-information tests that check
for line-numbering in particular orders.

Includes POC test (which is silent w/o -v):
go test  -v -run TestDebugLines cmd/compile/internal/ssa
=== RUN   TestDebugLines
Preserving temporary directory /var/folders/v6/xyzzy/T/debug_lines_test321
About to run (cd /var/folders/v6/xyzzy/T/debug_lines_test321; \
    GOSSADIR=/var/folders/v6/xyzzy/T/debug_lines_test321 \
    /Users/drchase/work/go/bin/go build -o foo.o \
    '-gcflags=-N -l -d=ssa/genssa/dump=sayhi' \
    /Users/drchase/work/go/src/cmd/compile/internal/ssa/testdata/sayhi.go )
Saw stmt# 8 for submatch '8' on dump line #7 = ' v107   00005 (+8)  MOVQ    AX, "".n(SP)'
Saw stmt# 9 for submatch '9' on dump line #9 = ' v87    00007 (+9)  MOVUPS  X15, ""..autotmp_2-32(SP)'
Saw stmt# 10 for submatch '10' on dump line #46 = ' v65     00044 (+10)     MOVUPS  X15, ""..autotmp_2-32(SP)'
Saw stmt# 11 for submatch '11' on dump line #83 = ' v131    00081 (+11)     MOVQ    "".wg+8(SP), AX'
--- PASS: TestDebugLines (4.95s)
PASS
ok  	cmd/compile/internal/ssa	5.685s

Includes a test to ensure that inlining information is printed correctly.

Updates #47880.

Change-Id: I83b596476a88687d71d5b65dbb94641a576d747e
Reviewed-on: https://go-review.googlesource.com/c/go/+/348970
Trust: David Chase <drchase@google.com>
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Sep 21, 2021

Change https://golang.org/cl/351229 mentions this issue: cmd/compile: add test skip for plan9 because it lacks $HOME

Loading

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

Successfully merging a pull request may close this issue.

None yet
6 participants