gdb: micro patch for gdb stepping

[glycerine]

Seen on linux/amd64 with golang 1.2rc3, suspected still there with 1.2rc5

Opening a new ticket, related to 6776 [
https://golang.org/issue/6776 ], in case you wish to both consider
long-term issues in 6776, and take care of the immediate gdb-debugging issue now with a
trivial fix, using this ticket.

Proposed: change go1.2rc3/go/src/pkg/runtime/sys_x86.c:36 from
    if(pc[0] == 0xeb) { // jmp 1-byte offset
to
    if(pc[0] == 0xeb || pc[0] == 0xcc) { // jmp 1-byte offset     

As in the comments on 6776, the check for a debugger interrupt 0xcc allows gdb debugging
to continue stepping (or just continue) and not invoke the panic that follows in
sys_x86.c because gdb has set a breakpoint 0xcc.

It's not necessary to fix the gdb debugging experience as soon as possible. But it's so
valuable that I thought it worth considering for inclusion.

I would appreciate any thoughts on correctness. Note however that there are only two
possible acceptable values at pc[0] now (0xeb or 0xe9) in runtime*rewindmorestack() at
the moment, so it is easy to construct a proof that the above patch cannot regress any
existing code.

[ianlancetaylor]

Comment 1:

While I agree that something should be fixed in this area, I don't see how the suggested
fix could be correct.  gdb is going to put that 0xcc there whether the jump offset is 1
byte or 4 bytes.  Simply assuming that it is always 1 byte will not work reliably.
You say that the patch can not regress any existing code, which is true, but it does
something just as bad: it makes it in practice unpredictable whether the code works or
not.

[glycerine]

Comment 2:

"...it makes it in practice unpredictable whether the code works or not." <-- This
statement questionable at best, and is certainly far too broad, since in reality the
patch only effects the experience under GDB, and without the patch GDB usage is
impractical.
Please try stepping in GDB with and without the patch.

[ianlancetaylor]

Comment 3:

My apologies, I should have said that it makes it unpredictable whether gdb works or
not.  What is better: always fails so you find a workaround, or unpredictably fails?
I just built a hello world program in Go, ran it under gdb, and typed "break main.main",
"run", and then typed "step" a bunch of times.  It worked fine.  Perhaps you can
describe more precisely how to recreate the problem.  (I'm using gdb 7.6.)

[glycerine]

Comment 4:

Here is a minimal example that reproduces the situation for me:
package main
import "fmt"
func sub() {
      fmt.Printf("subroutine sub called.\n")
}
func main() {
 sub()     // put gdb breakpoint here and then run; once stopped in gdb: 's', then 'n'                          
}
I see output:
(gdb) n
runtime: pc=0x400c13 0xcc 0xeb 0x48 0x83 0xec
fatal error: runtime: misuse of rewindmorestack
runtime stack:
runtime.throw(0x56425f)
    /usr/cn/go1.2rc3/go/src/pkg/runtime/panic.c:464 +0x69
runtime.rewindmorestack(0xc210001148)
    /usr/cn/go1.2rc3/go/src/pkg/runtime/sys_x86.c:42 +0xb4
runtime.newstack()
    /usr/cn/go1.2rc3/go/src/pkg/runtime/stack.c:230 +0x153
runtime.morestack()
    /usr/cn/go1.2rc3/go/src/pkg/runtime/asm_amd64.s:225 +0x61
goroutine 1 [stack split]:
main.sub()
    /home/jaten/dev/cnet/go/src/cn/hellogdb/hellogdb.go:3 +0x13 fp=0x7ffff7e2ef40
main.main()
    /home/jaten/dev/cnet/go/src/cn/hellogdb/hellogdb.go:7 +0x1a fp=0x7ffff7e2ef48
runtime.main()
    /usr/cn/go1.2rc3/go/src/pkg/runtime/proc.c:220 +0x11f fp=0x7ffff7e2efa0
runtime.goexit()
    /usr/cn/go1.2rc3/go/src/pkg/runtime/proc.c:1394 fp=0x7ffff7e2efa8
[Inferior 1 (process 28238) exited with code 02]
(gdb)
I'm using gdb version: GNU gdb (Ubuntu/Linaro 7.4-2012.04-0ubuntu2.1) 7.4-2012.04
I'm on ubuntu 12.04 linux/amd64

[glycerine]

Comment 5:

Addendum: With patch in place, the minimal example above (hellogdb.go) also shows the
gdb behavior when the the heuristic is wrong and jmp is a 4-byte offset instead of a
1-byte offset: the 'n' next command which sets an implicit breakpoint is just
ineffective, and the 'n' is converted into a 'c', continuing execution.
So I agree it's still not ideal, but still much better than crashing.  
Yeah, it's a hack. It's not perfect, but better than waiting months for perfection.

[ianlancetaylor]

Comment 6:

Thanks for the reproduction instructions.  For me this patch avoids the crash, and I
think this approach is more predictable.  Is the gdb behaviour with this patch as good
as with your proposed patch?
diff -r 65bf677ab8d8 src/pkg/runtime/sys_x86.c
--- a/src/pkg/runtime/sys_x86.c Mon Nov 25 13:36:16 2013 +1100
+++ b/src/pkg/runtime/sys_x86.c Mon Nov 25 17:04:15 2013 -0800
@@ -27,7 +27,8 @@
 runtime·rewindmorestack(Gobuf *gobuf)
 {
    byte *pc;
-   
+   Func *f;
+
    pc = (byte*)gobuf->pc;
    if(pc[0] == 0xe9) { // jmp 4-byte offset
        gobuf->pc = gobuf->pc + 5 + *(int32*)(pc+1);
@@ -37,6 +38,13 @@
        gobuf->pc = gobuf->pc + 2 + *(int8*)(pc+1);
        return;
    }
+   if(pc[0] == 0xcc) { // breakpoint inserted by gdb
+       f = runtime·findfunc(gobuf->pc);
+       if(f != nil) {
+           gobuf->pc = f->entry;
+           return;
+       }
+   }
    runtime·printf("runtime: pc=%p %x %x %x %x %x\n", pc, pc[0], pc[1], pc[2], pc[3], pc[4]);
    runtime·throw("runtime: misuse of rewindmorestack");
 }

[glycerine]

Comment 7:

That works well, for me.  Thanks Ian.
To anyone else reading this: (With either patch) GDB still misses many 'n' steps,
converting them effectively into 'c' commands, continuing execution at full throttle. 
So this lack of being able to step in gdb is still sub-optimal -- but: those bigger
issues can/should be addressed on ticket 6776.  This ticket 6834 is just for the quick
patch.
Here this minor repair allows us to continue, because the program hasn't crashed, and
hence I can just set breakpoints a little further down, catch the runaway, and still get
alot of useful work done in gdb. 
In short, this is a very useful workaround/incremental fix.
Thanks again, Ian.
- Jason

[rsc]

Comment 8:

Labels changed: added go1.3maybe.

[rsc]

Comment 10:

Labels changed: added release-none, removed go1.3maybe.

[rsc]

Comment 11:

Labels changed: added repo-main.

[ianlancetaylor]

Comment 12:

This issue was closed by revision 89c5d17.

Status changed to Fixed.