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.

Sign up for GitHub

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: line number table incorrect for break statements #14379

Closed
aarzilli opened this issue Feb 18, 2016 · 8 comments
Closed

cmd/compile: line number table incorrect for break statements #14379

aarzilli opened this issue Feb 18, 2016 · 8 comments
Labels
FrozenDueToAge
Milestone
Go1.7

Comments

@aarzilli
Copy link
Contributor

go version go1.6 linux/amd64

If I take this program:

package main

func main() {
    i := 0
    for {
        i++
        if i > 10 {
            break
        }
    }
}

Compile it with go build -gcflags='-N -l' break.go, load its .gosymtab and .gopclntab with debug/gosym and then call

goSymTable.LineToPC("/path/to/break.go", 8)

I get the error: no code at /path/to/break.go:8. Furthermore if I disassemble it I get:

$ go tool objdump -s main.main break
TEXT main.main(SB) /path/to/break.go
    break.go:3  0x401000    4883ec10        SUBQ $0x10, SP
    break.go:4  0x401004    48c7042400000000    MOVQ $0x0, 0(SP)
    break.go:6  0x40100c    488b1c24        MOVQ 0(SP), BX
    break.go:6  0x401010    48895c2408      MOVQ BX, 0x8(SP)
    break.go:6  0x401015    488b5c2408      MOVQ 0x8(SP), BX
    break.go:6  0x40101a    4883c301        ADDQ $0x1, BX
    break.go:6  0x40101e    48891c24        MOVQ BX, 0(SP)
    break.go:7  0x401022    488b1c24        MOVQ 0(SP), BX
    break.go:7  0x401026    4883fb0a        CMPQ $0xa, BX
    break.go:7  0x40102a    7ee0            JLE 0x40100c
    break.go:11 0x40102c    4883c410        ADDQ $0x10, SP
    break.go:11 0x401030    c3          RET

which also doesn't have any address corresponding to break.go:8. I would expect the JLE instruction at 0x40102a to correspond to break.go:8.

For reference this came up while I was reviewing this delve bug.
@ianlancetaylor ianlancetaylor changed the title Line number table incorrect for break statements cmd/compile: line number table incorrect for break statements Feb 18, 2016
@ianlancetaylor ianlancetaylor added this to the Go1.7 milestone Feb 18, 2016
@randall77
Copy link
Contributor

I'm not quite sure this is a bug.
The problem is that the break statement compiles to nothing. The JLE is part of the if. The compiler gets to something like this:

block1: if ... goto block2 else block3
block2: goto block4 (this is the break exiting the loop)

So block1 is assigned line 7 and block2 is assigned line 8 (correctly, so far).
But block2 is empty, so at some point the compiler decides that block1.true should jump directly to block4. To get the result you want, we would have to copy block2's line number into block1. But what if block3 is also empty and goes somewhere else? Which of block2 or block3 gets to impose its line number on block1? I think it makes more sense to let block1 keep its original line number.

A fix would be to not short-circuit the empty block2, it will get a JMP, and the JMP can be line 8. Maybe we can do that for -N, but we probably don't want to do that in general.

I'm sympathetic to trying to set a breakpoint on the break instruction, but I'm not sure we've got a way to make that happen, any more than we do for other source-level constructs that compile to no code.

@aarzilli
Copy link
Contributor Author

Which of block2 or block3 gets to impose its line number on block1?

block1 compiles to multiple instructions which should get different line numbers. I wouldn't want the CMP instruction to get line 8 anyway. If you're saying it can't be done with the way the compiler currently accounts for line numbers, ok, if you're saying it's impossible in principle, no.

@randall77
Copy link
Contributor

By "impose its line number", I mean only on the last instruction of block1 (the JLE), not every instruction.
Who owns the JLE, block1, block2, or block3?

In

if x <= y {
   f()
} else {
   g()
}

Which line should the JLE be assigned to? I would argue line 1, not line 2 or line 4.

I think with -N this should be fixable. I can get the compiler to not eliminate the jumps, but the linker is currently too optimizing for its own good. Gotta bash it on the head...

@minux
Copy link
Member

minux commented Feb 23, 2016 via email

@aarzilli
Copy link
Contributor Author

I see what you are saying now, you are right.

@aarzilli
Copy link
Contributor Author

Gcc adds a nop in similar circumstances but I doubt that's something go should do. I'm closing this. Thank you.

@gopherbot
Copy link

CL https://golang.org/cl/19848 mentions this issue.

@gopherbot
Copy link

CL https://golang.org/cl/19854 mentions this issue.

gopherbot pushed a commit that referenced this issue Feb 24, 2016
@randall77
cmd/compile: keep JMPs around with -N
e360f7c 
When -N, make sure we don't drop every instruction from
a block, even ones which would otherwise be empty.
Helps keep line numbers around for debugging, particularly
for break and continue statements (which often compile
down to nothing).

Fixes #14379

Change-Id: I33722c4f0dcd502f146fa48af262ba3a477c959a
Reviewed-on: https://go-review.googlesource.com/19854
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Minux Ma <minux@golang.org>
gopherbot pushed a commit that referenced this issue Feb 24, 2016
@randall77
[dev.ssa] cmd/compile: leave JMPs in when using -N
8906d2a 
Helps keep line numbers around for debugging, particularly
for break and continue statements (which often compile
down to nothing).

Update #14379

Change-Id: I6ea06aa887b0450d9ba4f11e319e5c263f5a98ba
Reviewed-on: https://go-review.googlesource.com/19848
Reviewed-by: David Chase <drchase@google.com>
@golang golang locked and limited conversation to collaborators Feb 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.7
Development

No branches or pull requests
5 participants
@minux @aarzilli @ianlancetaylor @randall77 @gopherbot