-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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: misleading PC <> line number with "for {}" #30167
Comments
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. |
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?
/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. |
Option (3) is worth a look. 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 (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. |
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 The following patch fixes the issue. Patchcommit 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 { |
Change https://golang.org/cl/163019 mentions this issue: |
What version of Go are you using (
go version
)?Does this issue reproduce with the latest release?
Yes.
What operating system and processor architecture are you using (
go env
)?go env
OutputWhat did you do?
Compiled the following program:
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 bygo tool objdump
atmain.go:4
.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.
The text was updated successfully, but these errors were encountered: