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

gdb: micro patch for gdb stepping #6834

Closed
glycerine opened this issue Nov 25, 2013 · 11 comments
Closed

gdb: micro patch for gdb stepping #6834

glycerine opened this issue Nov 25, 2013 · 11 comments

Comments

@glycerine
Copy link

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

rsc commented Nov 27, 2013

Comment 8:

Labels changed: added go1.3maybe.

@rsc
Copy link
Contributor

rsc commented Dec 4, 2013

Comment 10:

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

@rsc
Copy link
Contributor

rsc commented Dec 4, 2013

Comment 11:

Labels changed: added repo-main.

@ianlancetaylor
Copy link
Contributor

Comment 12:

This issue was closed by revision 89c5d17.

Status changed to Fixed.

@golang golang locked and limited conversation to collaborators Jun 25, 2016
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants