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
Labels
Comments
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. |
"...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. |
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.) |
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 |
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. |
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"); } |
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 |
This issue was closed by revision 89c5d17. Status changed to Fixed. |
This issue was closed.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
The text was updated successfully, but these errors were encountered: