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

Jump to bottom

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
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@aarzilli
Copy link
Contributor 

$ 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 the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 24, 2021
@toothrot toothrot added this to the Backlog milestone Aug 24, 2021
@dr2chase
Copy link
Contributor

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.

@mdempsky
Copy link
Member

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.

@aarzilli
Copy link
Contributor Author

Also happens with GOEXPERIMENT=unified.

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

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

@mdempsky
Copy link
Member

Bisect identified 57668b8.

/cc @randall77 @danscales

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

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

@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Sep 20, 2021
@dr2chase
cmd/compile: extend dump-to-file to handle "genssa" (asm) case.
af72ddf 
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

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

@bcmills bcmills mentioned this issue Sep 22, 2021
Closed
@golang golang locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
6 participants
@toothrot @mdempsky @aarzilli @dr2chase @randall77 @gopherbot