Re: TUI + gdbserver broken?

[Pedro Alves <pedro_alves@portugalmail.pt>]

[-- Attachment #1: Type: text/plain, Size: 6099 bytes --]

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



[-- Attachment #2: tui_frame5.diff --]
[-- Type: text/plain, Size: 1798 bytes --]

---
 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.  */