cmd/compile: misleading PC <> line number with "for {}"

[pwaller]

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

$ go version
go1.12beta2

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output

$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/pwaller/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/pwaller/.local"
GOPROXY=""
GORACE=""
GOROOT="/home/pwaller/.local/src/go"
GOTMPDIR=""
GOTOOLDIR="/home/pwaller/.local/src/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build478432202=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Compiled the following program:

package main

func main() {
	go func() {}()

	for {}
}

What did you expect to see?

I expected the infinite for loop in the binary to be correctly mapped to the source.

What did you see instead?

The for loop is on line 6 in the source, but in the binary at 0x44f0d5, where there is a "jmp-to-self", it is shown by go tool objdump at main.go:4.

$ go1.12beta2 build && go tool objdump -s main.main misleading-debuginfo

TEXT main.main(SB) .../misleading-debuginfo/main.go
  main.go:3		0x44f0a0		64488b0c25f8ffffff	MOVQ FS:0xfffffff8, CX			
  main.go:3		0x44f0a9		483b6110		CMPQ 0x10(CX), SP			
  main.go:3		0x44f0ad		7628			JBE 0x44f0d7				
  main.go:3		0x44f0af		4883ec18		SUBQ $0x18, SP				
  main.go:3		0x44f0b3		48896c2410		MOVQ BP, 0x10(SP)			
  main.go:3		0x44f0b8		488d6c2410		LEAQ 0x10(SP), BP			
  main.go:4		0x44f0bd		c7042400000000		MOVL $0x0, 0(SP)			
  main.go:4		0x44f0c4		488d05fd420200		LEAQ 0x242fd(IP), AX			
  main.go:4		0x44f0cb		4889442408		MOVQ AX, 0x8(SP)			
  main.go:4		0x44f0d0		e82bd1fdff		CALL runtime.newproc(SB)		
  main.go:4		0x44f0d5		ebfe			JMP 0x44f0d5				
  main.go:3		0x44f0d7		e8d47effff		CALL runtime.morestack_noctxt(SB)	
  main.go:3		0x44f0dc		ebc2			JMP main.main(SB)			

Additional thinking

I encountered this while diagnosing golang/tools#74 - an issue causing 100% CPU usage unnecessarily. Because the mapping was incorrect, I did not immediately discover the for {} (I wasn't expecting it).

In the example above, the "infinite for" is not far away from its correct location, but it is easy to imagine them being quite distant (for example if the goroutine had a substantial chunk of code in it).

I realize that a precise PC <> line number in general might be too much to ask for. But it seems worth asking, and if it is too much, at least this issue serves to document some discussion.

@gopherbot please add label Debugging.

/cc @ianlancetaylor who likely knows about these things.

[pwaller]

$ go build -gcflags=-K
# .../faulty-debuginfo
./main.go:6:2: prog: unknown position (line 0)

Interesting. I'm taking a closer look. Added some tracing to the compiler at it seems like the correct position is known, and then later lost.

[pwaller]

I had a look, and learned a little, but haven't solved it.

A question on my mind is where is the right place to give a pos to a basic block?

1. The line number shown in the unknown position warning (which is correct) is:

   go/src/cmd/compile/internal/gc/lex.go

   Line 17 in e1b49ad

   var lineno src.XPos

   If I do b.pp.pos = lineno in the "unknown position if-block" (source immediately below), then I get what I expected in the binary. I'm guessing there is a good reason this value isn't used, but I'm not sure why not. I guess lineno is meant to go away eventually.

   go/src/cmd/compile/internal/gc/gsubr.go

   Lines 133 to 135 in e1b49ad

   	if !pp.pos.IsKnown() && Debug['K'] != 0 {
   	Warn("prog: unknown position (line 0)")
   	}

2. I looked into the numberlines pass. The block in question isn't being assigned to. I guess the problem is that since the block doesn't have any values, there isn't an obvious place to grab a pos from during numberlines. So the for block falls through this continue here:

   go/src/cmd/compile/internal/ssa/numberlines.go

   Line 134 in e1b49ad

   continue

3. If I assign the for loop body pos to the node pos (bBody.Pos = n.Pos) in ssa.go func (s *state) stmt(n *Node), I get the right result in the output. Is this a reasonable thing to do?

   go/src/cmd/compile/internal/gc/ssa.go

   Line 1069 in e1b49ad

   bBody := s.f.NewBlock(ssa.BlockPlain)

/cc @dr2chase from the git log. Is option (3) a reasonable thing to do? If so, I'll send a CL. If not, if you point me in the right direction I'd attempt to fix it.

[dr2chase]

Option (3) is worth a look.
To see how stuff proceeds through the sausage factory, set environment variable GOSSAFUNC=main before building, and look at ssa.html in a (Javascript-enabled) browser.

Our current and inadequate test for line assignment is in cmd/compile/internal/ssa/debug_test.go

gdb: go test -v debug_test.go -args -f
dlv: go test -v debug_test.go -args -f -d

(If you're on a Mac, you need -f, assuming you have gdb or ggdb on you path).

It's fragile, sometimes it hangs, I'm supposed to be working on that.
The "golden" files are more like brass, so you need to review any differences, they might be better.

[pwaller]

I considered testing along the lines of the existing tests, but of course we can't step a debugger through an infinite for loop until it completes. I'd be tempted to inspect the output of go tool objdump in this case, but I have no idea whether that is appropriate as part of the compiler test suite, given that it would be relatively expensive.

The following patch fixes the issue. I'll leave it here for now and I'll submit it once the tree opens. (Edit: submitted as https://golang.org/cl/163019 with hashtag wait-release).

Patch

commit 5cfec30c9b2e1f52321ce94b93a111a0b25ee23c (HEAD -> issue-30167)
Author: Peter Waller <p@pwaller.net>
Date:   Mon Feb 11 15:09:24 2019 +0000

    cmd/compile/internal/ssa: set OFOR bBody.Pos to AST Pos
    
    Assign SSA OFOR's bBody.Pos to AST (*Node).Pos as it is created.
    
    An empty for loop has no other information which may be used to give
    correct position information in the resulting executable. Such a for
    loop may compile to a single `JMP *self` and it is important that the
    location of this is in the right place.
    
    Fixes #30167.

diff --git a/src/cmd/compile/internal/gc/ssa.go b/src/cmd/compile/internal/gc/ssa.go
index 6ddc9fba7a..d399752782 100644
--- a/src/cmd/compile/internal/gc/ssa.go
+++ b/src/cmd/compile/internal/gc/ssa.go
@@ -1071,6 +1071,9 @@ func (s *state) stmt(n *Node) {
                bIncr := s.f.NewBlock(ssa.BlockPlain)
                bEnd := s.f.NewBlock(ssa.BlockPlain)
 
+               // ensure empty for loops have correct position; issue #30167
+               bBody.Pos = n.Pos
+
                // first, jump to condition test (OFOR) or body (OFORUNTIL)
                b := s.endBlock()
                if n.Op == OFOR {

[gopherbot]

Change https://golang.org/cl/163019 mentions this issue: cmd/compile/internal/ssa: set OFOR bBody.Pos to AST Pos