This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: TUI + gdbserver broken?
You know what they say. "A patch per day, ..." :)
I wish I new before about the set new-console 1 command. It makes
debugging TUI on cygwin much easier and it would save me from
holding the Guinness record of bad patches in a row. :)
I've been trying to understand why doing as you suggested doesn't
work, and what I see makes me believe that the current frame hook
handling is not safe - that is, as I said before, I believe that
calling:
get_selected_frame -> select_frame ->
deprecated_selected_frame_level_changed_hook
inside a deprecated_selected_frame_level_changed_hook is a bad
idea. I believe those hooks should be pure observers.
Take a look at the stack trace that I get with the following
pseudo-patch applied.
static void
tui_selected_frame_level_changed_hook (int level)
{
struct frame_info *fi;
+ if (level < 0)
+ return;
fi = deprecated_safe_get_selected_frame ();
remote_fetch_registers (regnum=8) at
../../gdb-server_search/src/gdb/remote.c:3688
#1 0x0047719e in regcache_raw_read (regcache=0x10042670, regnum=8,
buf=0x22c440 "ð")
at ../../gdb-server_search/src/gdb/regcache.c:510
#2 0x00477769 in regcache_cooked_read (regcache=0x10042670, regnum=8,
buf=0x22c440 "ð")
at ../../gdb-server_search/src/gdb/regcache.c:588
#3 0x00523575 in sentinel_frame_prev_register (next_frame=0x1005bb68,
this_prologue_cache=0x1005bb6c,
regnum=8, optimized=0x22c424, lvalp=0x22c418, addrp=0x22c420,
realnum=0x22c41c, bufferp=0x22c440 "ð")
at ../../gdb-server_search/src/gdb/sentinel-frame.c:69
#4 0x0047b794 in frame_register_unwind (frame=0x1005bb68, regnum=8,
optimizedp=0x22c424, lvalp=0x22c418,
addrp=0x22c420, realnump=0x22c41c, bufferp=0x22c440 "ð") at
../../gdb-server_search/src/gdb/frame.c:589
#5 0x0047ba84 in frame_unwind_register (frame=0x1005bb68, regnum=8,
buf=0x22c440 "ð")
at ../../gdb-server_search/src/gdb/frame.c:641
#6 0x0051db90 in i386_unwind_pc (gdbarch=0x101cdef8, next_frame=0x1005bb68)
at ../../gdb-server_search/src/gdb/i386-tdep.c:915
#7 0x0045bfa5 in gdbarch_unwind_pc (gdbarch=0x101cdef8,
next_frame=0x1005bb68)
at ../../gdb-server_search/src/gdb/gdbarch.c:3071
#8 0x005235cb in sentinel_frame_prev_pc (next_frame=0x1005bb68,
this_prologue_cache=0x1005bb6c)
at ../../gdb-server_search/src/gdb/sentinel-frame.c:89
#9 0x0047b37f in frame_pc_unwind (this_frame=0x1005bb68) at
../../gdb-server_search/src/gdb/frame.c:438
#10 0x0047ced7 in frame_unwind_address_in_block (next_frame=0x1005bb68,
this_type=NORMAL_FRAME)
at ../../gdb-server_search/src/gdb/frame.c:1502
#11 0x004de9ca in dwarf2_frame_sniffer (next_frame=0x1005bb68)
at ../../gdb-server_search/src/gdb/dwarf2-frame.c:1210
#12 0x004b1b6f in frame_unwind_find_by_frame (next_frame=0x1005bb68,
this_cache=0x1005bbbc)
at ../../gdb-server_search/src/gdb/frame-unwind.c:98
#13 0x0047d1c7 in get_frame_type (frame=0x1005bbb8) at
../../gdb-server_search/src/gdb/frame.c:1634
#14 0x0047cf20 in get_frame_address_in_block (this_frame=0x1005bbb8)
at ../../gdb-server_search/src/gdb/frame.c:1529
#15 0x0047c36a in select_frame (fi=0x1005bbb8) at
../../gdb-server_search/src/gdb/frame.c:1016
#16 0x0047c2ab in get_selected_frame (message=0x0) at
../../gdb-server_search/src/gdb/frame.c:965
#17 0x0047c325 in deprecated_safe_get_selected_frame () at
../../gdb-server_search/src/gdb/frame.c:981
#18 0x004a3098 in tui_registers_changed_hook () at
../../gdb-server_search/src/gdb/tui/tui-hooks.c:135
#19 0x00477008 in registers_changed () at
../../gdb-server_search/src/gdb/regcache.c:469
#20 0x00425af2 in wait_for_inferior () at
../../gdb-server_search/src/gdb/infrun.c:993
#21 0x00425a08 in start_remote (from_tty=1) at
../../gdb-server_search/src/gdb/infrun.c:855
#22 0x005133bb in remote_start_remote (uiout=0x10069220,
from_tty_p=0x22c764)
at ../../gdb-server_search/src/gdb/remote.c:2109
#23 0x00412d5c in catch_exception (uiout=0x10069220, func=0x513329
<remote_start_remote>, func_args=0x22c764,
mask=6) at ../../gdb-server_search/src/gdb/exceptions.c:469
#24 0x00513c98 in remote_open_1 (name=0x10033bc8 ":9999", from_tty=1,
target=0x6b8df0, extended_p=0, async_p=0)
at ../../gdb-server_search/src/gdb/remote.c:2563
#25 0x005133ed in remote_open (name=0x10033bc8 ":9999", from_tty=1)
at ../../gdb-server_search/src/gdb/remote.c:2118
#26 0x0042ca1b in do_cfunc (c=0x10042728, args=0x10033bc8 ":9999",
from_tty=1)
By bad luck, we ended up calling remote_fetch_registers after calling
putpkt ("?") in remote_start_remote, which calls set_thread (*), followed
by a fetch registers put/recv pkt. By the time we get to remote_wait,
waiting for the resume reply, there is nothing left waiting to
be processed, so we just sit there in an infinite loop waiting
for a packet that never comes.
(*) - It takes a few more millis than in the normal unpatched case.
Probably a timeout?
Now, why doesn't this happen without the patch ( current cvs ) ?
Pure luck.
Without that patch, luckily there is a
tui_selected_frame_level_changed_hook called during gdbarch_update,
before registers_changed (#19) is called, which has the side effect
of calling target_fetch_registers before "?" is sent. By the time
we reach #19 there is already a frame selected in the cache. This
makes get_selected_frame (#16) return immediately without calling
select_frame. This then means target_fetch_registers is not
called again, and nothing is wrongly injected in the stream like
in the patched version.
As you can see, changing the selected_frame state from inside a
selected_frame_changed hook is dangerous. I maintain my view
that the hooks should be pure observers, or that either
deprecated_safe_get_selected_frame should be taught some
more to know if the target is running, or instead a patch like
the first one I sent (the one that called
target_mark_running/target_mark_exited) should be installed.
If you agree that the former is the way to go, the attached
patch works for me.
Maybe we should also copy (not move) the
'if (selected == NULL) -> select (current)' mapping
to select_frame.
Cheers,
Pedro Alves
---
gdb/frame.c | 6 ++++++
gdb/tui/tui-hooks.c | 11 ++++++++++-
2 files changed, 16 insertions(+), 1 deletion(-)
Index: src/gdb/frame.c
===================================================================
--- src.orig/gdb/frame.c 2007-03-20 20:00:04.000000000 +0000
+++ src/gdb/frame.c 2007-03-22 02:33:54.000000000 +0000
@@ -946,6 +946,12 @@ get_current_frame (void)
static struct frame_info *selected_frame;
+int
+selected_frame_is_valid (void)
+{
+ return (selected_frame != NULL);
+}
+
/* Return the selected frame. Always non-NULL (unless there isn't an
inferior sufficient for creating a frame) in which case an error is
thrown. */
Index: src/gdb/tui/tui-hooks.c
===================================================================
--- src.orig/gdb/tui/tui-hooks.c 2007-03-21 22:48:08.000000000 +0000
+++ src/gdb/tui/tui-hooks.c 2007-03-22 02:46:56.000000000 +0000
@@ -127,12 +127,18 @@ tui_query_hook (const char * msg, va_lis
/* Prevent recursion of deprecated_registers_changed_hook(). */
static int tui_refreshing_registers = 0;
+/* Move to frame.h if patch is approved. */
+extern int selected_frame_is_valid (void);
+
static void
tui_registers_changed_hook (void)
{
struct frame_info *fi;
- fi = get_selected_frame (NULL);
+ if (!selected_frame_is_valid ())
+ return;
+
+ fi = deprecated_safe_get_selected_frame ();
if (tui_refreshing_registers == 0)
{
tui_refreshing_registers = 1;
@@ -230,6 +236,9 @@ tui_selected_frame_level_changed_hook (i
{
struct frame_info *fi;
+ if (!selected_frame_is_valid ())
+ return;
+
fi = deprecated_safe_get_selected_frame ();
/* Ensure that symbols for this frame are read in. Also, determine the
source language of this frame, and switch to it if desired. */