* TUI + gdbserver broken?
@ 2007-03-19 1:53 Pedro Alves
2007-03-19 2:11 ` Daniel Jacobowitz
0 siblings, 1 reply; 19+ messages in thread
From: Pedro Alves @ 2007-03-19 1:53 UTC (permalink / raw)
To: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 2623 bytes --]
Hi all,
TUI mode when debugging a remote target got broken after the recent
selected frame changes. After connecting to the target (target remote ...)
Issuing a continue results in:
(gdb) c
Continuing.
Remote communication error: Software caused connection abort.
And a snippet of a set debug remote session:
(...)
Packet vCont (verbose-resume) is supported
Sending packet: $vCont;c#a8...Ack
Packet received: W00
Sending packet: $g#67...Remote communication error: Software caused connection
abort.
(gdb) c
Continuing.
gdb is sending a g packet after a W. Something in gdb didn't
realize that the the target already exited.
The problem is that we now call deprecated_safe_get_selected_frame
in tui_selected_frame_level_changed_hook... :
static void
tui_selected_frame_level_changed_hook (int level)
{
struct frame_info *fi;
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. */
if (fi)
{
struct symtab *s;
s = find_pc_symtab (get_frame_pc (fi));
(...)
... and the deprecated_safe_get_selected_frame is not realizing the
target is not running anymore, so calls get_selected_frame, which
ensures that there is always a frame selected. Then in
tui_selected_frame_level_changed_hook, the get_frame_pc is called,
which ends up trying to talk to the remote target, but at this point
the stub is not reachable anymore, hence the Remote
communication error.
The way deprecated_safe_get_selected_frame checks to see if there
is a selectable frame is:
struct frame_info *
deprecated_safe_get_selected_frame (void)
{
if (!target_has_registers || !target_has_stack || !target_has_memory)
return NULL;
return get_selected_frame (NULL);
}
When using a remote target the following test is not true on when the 'W'
packet arrives:
if (!target_has_registers || !target_has_stack || !target_has_memory)
The attached patch fixes it by calling target_mark_running, and
target_mark_exited in remote.c. These functions set those
target_has_* to 0 and 1 appropriately. They are currently only
used on remote-sim.c. Let me know if there is a better way to
know if the target is running.
tui_registers_changed_hook then has the problem that is is calling
get_selected_frame, when target_has_registers is false. Fixed by
using deprecated_safe_get_selected_frame here too.
Note that I still see a TUI slow repainting problem introduced by
the selected frame changes. I haven't investigated that yet.
Cheers,
Pedro Alves
[-- Attachment #2: tui_frame.diff --]
[-- Type: text/plain, Size: 2974 bytes --]
gdb/ChangeLog
* remote.c (remote_open_1): Call target_mark_running.
(remote_wait): Call target_mark_exited.
(remote_async_wait): Likewise.
(remote_mourn_1): Likewise.
(extended_remote_create_inferior): Call target_mark_running.
(extended_remote_async_create_inferior): Likewise.
* tui/tui-hooks.c (tui_registers_changed_hook): Get selected
frame using deprecated_safe_get_selected_frame.
Index: src/gdb/remote.c
===================================================================
--- src.orig/gdb/remote.c 2007-03-18 21:43:06.000000000 +0000
+++ src/gdb/remote.c 2007-03-18 22:18:46.000000000 +0000
@@ -2583,6 +2583,8 @@ remote_open_1 (char *name, int from_tty,
if (exec_bfd) /* No use without an exec file. */
remote_check_symbols (symfile_objfile);
+
+ target_mark_running (target);
}
/* This takes a program previously attached to and detaches it. After
@@ -3240,6 +3242,7 @@ Packet: '%s'\n"),
case 'W': /* Target exited. */
{
/* The remote process exited. */
+ target_mark_exited (&remote_ops);
status->kind = TARGET_WAITKIND_EXITED;
status->value.integer = (fromhex (buf[1]) << 4) + fromhex (buf[2]);
goto got_status;
@@ -3436,6 +3439,7 @@ Packet: '%s'\n"),
case 'W': /* Target exited. */
{
/* The remote process exited. */
+ target_mark_exited (&async_remote_ops);
status->kind = TARGET_WAITKIND_EXITED;
status->value.integer = (fromhex (buf[1]) << 4) + fromhex (buf[2]);
goto got_status;
@@ -5036,6 +5040,7 @@ extended_remote_mourn (void)
static void
remote_mourn_1 (struct target_ops *target)
{
+ target_mark_exited (target);
unpush_target (target);
generic_mourn_inferior ();
}
@@ -5066,6 +5071,8 @@ extended_remote_create_inferior (char *e
will have a legacy FPA coprocessor emulated and others may have
access to a hardware VFP unit. */
+ target_mark_running (&extended_remote_ops);
+
/* Now put the breakpoints back in. This way we're safe if the
restart function works via a unix fork on the remote side. */
insert_breakpoints ();
@@ -5091,6 +5098,8 @@ extended_remote_async_create_inferior (c
/* Now restart the remote server. */
extended_remote_restart ();
+ target_mark_running (&extended_async_remote_ops);
+
/* NOTE: We don't need to recheck for a target description here; but
if we gain the ability to switch the remote executable we may
need to, if for instance we are running a process which requested
Index: src/gdb/tui/tui-hooks.c
===================================================================
--- src.orig/gdb/tui/tui-hooks.c 2007-03-18 21:42:30.000000000 +0000
+++ src/gdb/tui/tui-hooks.c 2007-03-18 21:43:36.000000000 +0000
@@ -132,7 +132,7 @@ tui_registers_changed_hook (void)
{
struct frame_info *fi;
- fi = get_selected_frame (NULL);
+ fi = deprecated_safe_get_selected_frame ();
if (tui_refreshing_registers == 0)
{
tui_refreshing_registers = 1;
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: TUI + gdbserver broken? 2007-03-19 1:53 TUI + gdbserver broken? Pedro Alves @ 2007-03-19 2:11 ` Daniel Jacobowitz 2007-03-19 15:58 ` Denis PILAT 2007-03-19 21:43 ` TUI + gdbserver broken? Pedro Alves 0 siblings, 2 replies; 19+ messages in thread From: Daniel Jacobowitz @ 2007-03-19 2:11 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches Thanks for all your detective work on this. I'm sorry I apparently broke TUI so badly - I wish we had test coverage. On Mon, Mar 19, 2007 at 01:51:47AM +0000, Pedro Alves wrote: > The problem is that we now call deprecated_safe_get_selected_frame > in tui_selected_frame_level_changed_hook... : Before I look at your patch, could you check one more thing for me: what's the backtrace look like when we get here? > The attached patch fixes it by calling target_mark_running, and > target_mark_exited in remote.c. These functions set those > target_has_* to 0 and 1 appropriately. They are currently only > used on remote-sim.c. Let me know if there is a better way to > know if the target is running. I think it is more likely that we shouldn't be doing whatever we're doing until after we've finished cleaning up the target's state. It's stuck between wait and mourn. > tui_registers_changed_hook then has the problem that is is calling > get_selected_frame, when target_has_registers is false. Fixed by > using deprecated_safe_get_selected_frame here too. This bit makes sense; you can commit it separately if you want. -- Daniel Jacobowitz CodeSourcery ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: TUI + gdbserver broken? 2007-03-19 2:11 ` Daniel Jacobowitz @ 2007-03-19 15:58 ` Denis PILAT 2007-03-27 20:09 ` Daniel Jacobowitz 2007-03-19 21:43 ` TUI + gdbserver broken? Pedro Alves 1 sibling, 1 reply; 19+ messages in thread From: Denis PILAT @ 2007-03-19 15:58 UTC (permalink / raw) To: Pedro Alves, gdb-patches Daniel Jacobowitz wrote: > Thanks for all your detective work on this. I'm sorry I apparently > broke TUI so badly - I wish we had test coverage. About TUI for Solaris, Fred and I have found where the problem comes from, but we are not sure about the fix. A "new" call to solib_add in solib-svr4.c has been added 2006-10-18 (yes 5 months ago!). This call leads to a problem about the owner of the target_terminal, it seems that the TUI tries to write in the terminal without beein owner. We tried just to add a call to "target_terminal_ours ()" at the beginning of infcmd.c (post_create_inferior), that fixes the problem. But I guess it's not the good place to do that. -- Denis --- infcmd.c 27 Feb 2007 19:46:04 -0000 1.150 +++ infcmd.c 19 Mar 2007 15:57:34 -0000 @@ -406,6 +406,8 @@ tty_command (char *file, int from_tty) void post_create_inferior (struct target_ops *target, int from_tty) { + target_terminal_ours (); + /* If the target hasn't taken care of this already, do it now. Targets which need to access registers during to_open, to_create_inferior, or to_attach should do it earlier; but many ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: TUI + gdbserver broken? 2007-03-19 15:58 ` Denis PILAT @ 2007-03-27 20:09 ` Daniel Jacobowitz 2007-04-17 15:48 ` [RFA] TUI is broken under Solaris Denis PILAT 0 siblings, 1 reply; 19+ messages in thread From: Daniel Jacobowitz @ 2007-03-27 20:09 UTC (permalink / raw) To: Denis PILAT; +Cc: Pedro Alves, gdb-patches On Mon, Mar 19, 2007 at 04:58:23PM +0100, Denis PILAT wrote: > Daniel Jacobowitz wrote: > >Thanks for all your detective work on this. I'm sorry I apparently > >broke TUI so badly - I wish we had test coverage. > About TUI for Solaris, Fred and I have found where the problem comes from, but > we are not sure about the fix. > A "new" call to solib_add in solib-svr4.c has been added 2006-10-18 (yes 5 > months ago!). > This call leads to a problem about the owner of the target_terminal, it seems > that the TUI tries to write in the terminal without beein owner. > We tried just to add a call to "target_terminal_ours ()" at the beginning of > infcmd.c (post_create_inferior), that fixes the problem. > But I guess it's not the good place to do that. I don't know. Maybe? It should happen somewhere central, either central to TUI or central to GDB, so that we have the terminal before TUI ever attempts to refresh. post_create_inferior might be the right place. -- Daniel Jacobowitz CodeSourcery ^ permalink raw reply [flat|nested] 19+ messages in thread
* [RFA] TUI is broken under Solaris 2007-03-27 20:09 ` Daniel Jacobowitz @ 2007-04-17 15:48 ` Denis PILAT 2007-04-17 15:51 ` Daniel Jacobowitz 0 siblings, 1 reply; 19+ messages in thread From: Denis PILAT @ 2007-04-17 15:48 UTC (permalink / raw) To: gdb-patches Daniel Jacobowitz wrote: > On Mon, Mar 19, 2007 at 04:58:23PM +0100, Denis PILAT wrote: > >> Daniel Jacobowitz wrote: >> >>> Thanks for all your detective work on this. I'm sorry I apparently >>> broke TUI so badly - I wish we had test coverage. >>> >> About TUI for Solaris, Fred and I have found where the problem comes from, but >> we are not sure about the fix. >> A "new" call to solib_add in solib-svr4.c has been added 2006-10-18 (yes 5 >> months ago!). >> This call leads to a problem about the owner of the target_terminal, it seems >> that the TUI tries to write in the terminal without beein owner. >> > > >> We tried just to add a call to "target_terminal_ours ()" at the beginning of >> infcmd.c (post_create_inferior), that fixes the problem. >> But I guess it's not the good place to do that. >> > > I don't know. Maybe? It should happen somewhere central, either > central to TUI or central to GDB, so that we have the terminal before > TUI ever attempts to refresh. > > post_create_inferior might be the right place. > > Here is the proposed patch: It fixes the tui problem under Solaris. TUI hosted under Solaris has been broken for monthes. No way to break on main and run a simple program. Here we avoid any written to the terminal without beeing owner of it. Don't know if it the best solution but it works No testsuite regression for Solaris and Linux native debuggers. -- Denis 2007-04-17 Denis Pilat <denis.pilat@st.com> * infcmd.c (post_create_inferior): Start with a call to target_terminal_ours(). Index: infcmd.c =================================================================== RCS file: /cvs/src/src/gdb/infcmd.c,v retrieving revision 1.151 diff -u -p -r1.151 infcmd.c --- infcmd.c 27 Mar 2007 23:01:00 -0000 1.151 +++ infcmd.c 17 Apr 2007 15:04:03 -0000 @@ -406,6 +406,9 @@ tty_command (char *file, int from_tty) void post_create_inferior (struct target_ops *target, int from_tty) { + /* Be sure we own the terminal in case write operations are performed. */ + target_terminal_ours (); + /* If the target hasn't taken care of this already, do it now. Targets which need to access registers during to_open, to_create_inferior, or to_attach should do it earlier; but many ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFA] TUI is broken under Solaris 2007-04-17 15:48 ` [RFA] TUI is broken under Solaris Denis PILAT @ 2007-04-17 15:51 ` Daniel Jacobowitz 2007-04-18 8:24 ` Denis PILAT 0 siblings, 1 reply; 19+ messages in thread From: Daniel Jacobowitz @ 2007-04-17 15:51 UTC (permalink / raw) To: Denis PILAT; +Cc: gdb-patches On Tue, Apr 17, 2007 at 05:42:20PM +0200, Denis PILAT wrote: > Here is the proposed patch: > It fixes the tui problem under Solaris. TUI hosted under Solaris has been > broken for monthes. No way to break on main and run a simple program. > Here we avoid any written to the terminal without beeing owner of it. Don't > know if it the best solution but it works > No testsuite regression for Solaris and Linux native debuggers. This is OK. -- Daniel Jacobowitz CodeSourcery ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFA] TUI is broken under Solaris 2007-04-17 15:51 ` Daniel Jacobowitz @ 2007-04-18 8:24 ` Denis PILAT 0 siblings, 0 replies; 19+ messages in thread From: Denis PILAT @ 2007-04-18 8:24 UTC (permalink / raw) To: gdb-patches Daniel Jacobowitz wrote: > On Tue, Apr 17, 2007 at 05:42:20PM +0200, Denis PILAT wrote: > >> Here is the proposed patch: >> It fixes the tui problem under Solaris. TUI hosted under Solaris has been >> broken for monthes. No way to break on main and run a simple program. >> Here we avoid any written to the terminal without beeing owner of it. Don't >> know if it the best solution but it works >> No testsuite regression for Solaris and Linux native debuggers. >> > > This is OK. > > Just committed. -- Denis ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: TUI + gdbserver broken? 2007-03-19 2:11 ` Daniel Jacobowitz 2007-03-19 15:58 ` Denis PILAT @ 2007-03-19 21:43 ` Pedro Alves 2007-03-19 22:14 ` Daniel Jacobowitz 1 sibling, 1 reply; 19+ messages in thread From: Pedro Alves @ 2007-03-19 21:43 UTC (permalink / raw) To: gdb-patches [-- Attachment #1: Type: text/plain, Size: 5477 bytes --] Daniel Jacobowitz wrote: > Thanks for all your detective work on this. I'm sorry I apparently > broke TUI so badly - I wish we had test coverage. > No problem. I wonder: Is TUI testable? Since TUI is sitting on top of deprecated hooks, it TUI itself deprecated? > On Mon, Mar 19, 2007 at 01:51:47AM +0000, Pedro Alves wrote: > >> The problem is that we now call deprecated_safe_get_selected_frame >> in tui_selected_frame_level_changed_hook... : >> > > Before I look at your patch, could you check one more thing for me: > what's the backtrace look like when we get here? > > Here it is. I've stopped in tui_selected_frame_level_changed_hook at the point is is obviously going to do something very wrong: /* The selected frame has changed. This is happens after a target stop or when the user explicitly changes the frame (up/down/thread/...). */ static void tui_selected_frame_level_changed_hook (int level) { struct frame_info *fi; 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. */ if (fi) { struct symtab *s; s = find_pc_symtab (get_frame_pc (fi)); <<<<< here. (tui-hooks.c:240) #0 tui_selected_frame_level_changed_hook (level=0) at ../../gdb-server_search/src/gdb/tui/tui-hooks.c:240 #1 0x0047c359 in select_frame (fi=0x1005b6f8) at ../../gdb-server_search/src/gdb/frame.c:995 #2 0x0047c2ab in get_selected_frame (message=0x0) at ../../gdb-server_search/src/gdb/frame.c:965 #3 0x0047c325 in deprecated_safe_get_selected_frame () at ../../gdb-server_search/src/gdb/frame.c:981 #4 0x004a3191 in tui_selected_frame_level_changed_hook (level=-1) at ../../gdb-server_search/src/gdb/tui/tui-hooks.c:233 #5 0x0047c359 in select_frame (fi=0x0) at ../../gdb-server_search/src/gdb/frame.c:995 #6 0x0047c572 in reinit_frame_cache () at ../../gdb-server_search/src/gdb/frame.c:1095 #7 0x004262f7 in handle_inferior_event (ecs=0x22c690) at ../../gdb-server_search/src/gdb/infrun.c:1302 #8 0x00425b87 in wait_for_inferior () at ../../gdb-server_search/src/gdb/infrun.c:1003 #9 0x004259d8 in proceed (addr=4294967295, siggnal=TARGET_SIGNAL_DEFAULT, step=0) at ../../gdb-server_search/src/gdb/infrun.c:825 #10 0x004138ab in continue_command (proc_count_exp=0x0, from_tty=1) at ../../gdb-server_search/src/gdb/infcmd.c:649 #11 0x0042ca1b in do_cfunc (c=0x10050258, args=0x0, from_tty=1) at ../../gdb-server_search/src/gdb/cli/cli-decode.c:62 #12 0x0042ef0d in cmd_func (cmd=0x10050258, args=0x0, from_tty=1) at ../../gdb-server_search/src/gdb/cli/cli-decode.c:1666 #13 0x004025b8 in execute_command (p=0x10033721 "", from_tty=1) at ../../gdb-server_search/src/gdb/top.c:456 #14 0x004243c2 in command_handler (command=0x10033720 "c") at ../../gdb-server_search/src/gdb/event-top.c:519 #15 0x00424b84 in command_line_handler (rl=0x101778c8 "\020hj") at ../../gdb-server_search/src/gdb/event-top.c:804 #16 0x00424cd8 in gdb_readline2 (client_data=0x0) at ../../gdb-server_search/src/gdb/event-top.c:884 #17 0x0042426c in stdin_event_handler (error=0, client_data=0x0) at ../../gdb-server_search/src/gdb/event-top.c:432 #18 0x0043944e in handle_file_event (event_file_desc=0) at ../../gdb-server_search/src/gdb/event-loop.c:730 #19 0x00438cd2 in process_event () at ../../gdb-server_search/src/gdb/event-loop.c:343 #20 0x00438d1b in gdb_do_one_event (data=0x0) at ../../gdb-server_search/src/gdb/event-loop.c:380 #21 0x00412f7b in catch_errors (func=0x438ce7 <gdb_do_one_event>, func_args=0x0, errstring=0x649101 "", mask=6) at ../../gdb-server_search/src/gdb/exceptions.c:515 #22 0x004a2e1b in tui_command_loop (data=0x0) at ../../gdb-server_search/src/gdb/tui/tui-interp.c:151 #23 0x0041c92a in current_interp_command_loop () at ../../gdb-server_search/src/gdb/interps.c:278 #24 0x004010cb in captured_command_loop (data=0x0) at ../../gdb-server_search/src/gdb/main.c:101 #25 0x00412f7b in catch_errors (func=0x4010c0 <captured_command_loop>, func_args=0x0, errstring=0x61a139 "", mask=6) at ../../gdb-server_search/src/gdb/exceptions.c:515 #26 0x00402110 in captured_main (data=0x22cc90) at ../../gdb-server_search/src/gdb/main.c:867 #27 0x00412f7b in catch_errors (func=0x401102 <captured_main>, func_args=0x22cc90, errstring=0x61a139 "", mask=6) at ../../gdb-server_search/src/gdb/exceptions.c:515 #28 0x00402146 in gdb_main (args=0x22cc90) at ../../gdb-server_search/src/gdb/main.c:876 #29 0x004010ba in main (argc=2, argv=0x10033438) at ../../gdb-server_search/src/gdb/tui/tui-main.c:36 > I think it is more likely that we shouldn't be doing whatever we're > doing until after we've finished cleaning up the target's state. > It's stuck between wait and mourn. > > I should have realized it too 8) Here is an updated patch that also fixes the problem. The comments in the patch should make it obvious, and I think it does what you hinted at. >> tui_registers_changed_hook then has the problem that is is calling >> get_selected_frame, when target_has_registers is false. Fixed by >> using deprecated_safe_get_selected_frame here too. >> > > This bit makes sense; you can commit it separately if you want. > I don't need this anymore with this new patch, so I've dropped it. It is easy to add if needed - error is called in that case. Cheers, Pedro Alves [-- Attachment #2: tui_frame2.diff --] [-- Type: text/plain, Size: 2532 bytes --] gdb/ChangeLog * infrun.c (handle_inferior_event): Delay calling reinit_frame_cache to after calling target_mourn_inferior when target has exited or was signalled. --- gdb/infrun.c | 30 +++++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-) Index: src/gdb/infrun.c =================================================================== --- src.orig/gdb/infrun.c 2007-03-18 23:22:38.000000000 +0000 +++ src/gdb/infrun.c 2007-03-19 20:36:40.000000000 +0000 @@ -1299,8 +1299,6 @@ handle_inferior_event (struct execution_ } ecs->infwait_state = infwait_normal_state; - reinit_frame_cache (); - /* If it's a new process, add it to the thread database */ ecs->new_thread_event = (!ptid_equal (ecs->ptid, inferior_ptid) @@ -1308,13 +1306,22 @@ handle_inferior_event (struct execution_ && !in_thread_list (ecs->ptid)); if (ecs->ws.kind != TARGET_WAITKIND_EXITED - && ecs->ws.kind != TARGET_WAITKIND_SIGNALLED && ecs->new_thread_event) + && ecs->ws.kind != TARGET_WAITKIND_SIGNALLED) { - add_thread (ecs->ptid); + /* When the inferior exited, postpone frame related operations + until the new target state is recorded. There may be + hooks installed that may try to fetch data from the + inferior without realizing it is gone. */ + reinit_frame_cache (); + + if (ecs->new_thread_event) + { + add_thread (ecs->ptid); - ui_out_text (uiout, "[New "); - ui_out_text (uiout, target_pid_or_tid_to_str (ecs->ptid)); - ui_out_text (uiout, "]\n"); + ui_out_text (uiout, "[New "); + ui_out_text (uiout, target_pid_or_tid_to_str (ecs->ptid)); + ui_out_text (uiout, "]\n"); + } } switch (ecs->ws.kind) @@ -1387,6 +1394,11 @@ handle_inferior_event (struct execution_ target_mourn_inferior (); singlestep_breakpoints_inserted_p = 0; /*SOFTWARE_SINGLE_STEP_P() */ stop_print_frame = 0; + + /* Now that the target state is recorded we can safelly + do frame related operations. */ + reinit_frame_cache (); + stop_stepping (ecs); return; @@ -1404,6 +1416,10 @@ handle_inferior_event (struct execution_ may be needed. */ target_mourn_inferior (); + /* Now that the target state is recorded we can safelly + do frame related operations. */ + reinit_frame_cache (); + print_stop_reason (SIGNAL_EXITED, stop_signal); singlestep_breakpoints_inserted_p = 0; /*SOFTWARE_SINGLE_STEP_P() */ stop_stepping (ecs); ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: TUI + gdbserver broken? 2007-03-19 21:43 ` TUI + gdbserver broken? Pedro Alves @ 2007-03-19 22:14 ` Daniel Jacobowitz 2007-03-19 23:31 ` Pedro Alves 0 siblings, 1 reply; 19+ messages in thread From: Daniel Jacobowitz @ 2007-03-19 22:14 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches On Mon, Mar 19, 2007 at 08:50:56PM +0000, Pedro Alves wrote: > Is TUI testable? Probably, but it would require someone comfortable with expect to set it up, I suspect. > Since TUI is sitting on top of deprecated hooks, it TUI itself deprecated? No. > static void > tui_selected_frame_level_changed_hook (int level) > { > struct frame_info *fi; > 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. */ > if (fi) > { > struct symtab *s; How about this: if (level >= 0) { struct frame_info *fi = get_selected_frame (NULL); Insight wants to be notified even if there's no frame, apparently (gross, I hate the way Insight is still attached at the hip to GDB - I honestly believe that in a few years we'll have no choice but to break Insight). But TUI doesn't. > Here is an updated patch that also fixes the problem. > The comments in the patch should make it obvious, and I think it does > what you hinted at. No, I meant at a higher level (i.e. why we were calling the hook). The frame cache is supposed to be a cache - we should be able to discard it at any and every time. -- Daniel Jacobowitz CodeSourcery ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: TUI + gdbserver broken? 2007-03-19 22:14 ` Daniel Jacobowitz @ 2007-03-19 23:31 ` Pedro Alves 2007-03-19 23:41 ` Daniel Jacobowitz 0 siblings, 1 reply; 19+ messages in thread From: Pedro Alves @ 2007-03-19 23:31 UTC (permalink / raw) To: gdb-patches Daniel Jacobowitz wrote: > >> static void >> tui_selected_frame_level_changed_hook (int level) >> { >> struct frame_info *fi; >> > > >> 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. */ >> if (fi) >> { >> struct symtab *s; >> > > How about this: > > if (level >= 0) > { > struct frame_info *fi = get_selected_frame (NULL); > > > That has exactly the same effect as it was before the your select frame changes. That is, only try to get the selected frame when there is one. Maybe there should still be a function like this in frame.c ? struct frame_info * deprecated_get_selected_frame () { return selected_frame; } (or perhaps call it get_selected_frame_if_any) I don't think that the callbacks should ever change the selected_frame state, that is, they should be pure observers - that is how it worked before, and your suggestion would make it so too. (I'll go try both versions) Cheers, Pedro Alves ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: TUI + gdbserver broken? 2007-03-19 23:31 ` Pedro Alves @ 2007-03-19 23:41 ` Daniel Jacobowitz 2007-03-20 0:39 ` Pedro Alves 0 siblings, 1 reply; 19+ messages in thread From: Daniel Jacobowitz @ 2007-03-19 23:41 UTC (permalink / raw) To: gdb-patches On Mon, Mar 19, 2007 at 10:57:53PM +0000, Pedro Alves wrote: > That has exactly the same effect as it was before the your select frame > changes. > That is, only try to get the selected frame when there is one. Maybe there > should > still be a function like this in frame.c ? > struct frame_info * > deprecated_get_selected_frame () > { > return selected_frame; > } > (or perhaps call it get_selected_frame_if_any) The problem is that when we reinitialize the frame cache, the selected frame is indeterminate - the selected_frame global may randomly be initialized or NULL depending on what else has been called since we last did reinit_frame_cache. -- Daniel Jacobowitz CodeSourcery ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: TUI + gdbserver broken? 2007-03-19 23:41 ` Daniel Jacobowitz @ 2007-03-20 0:39 ` Pedro Alves 2007-03-20 2:56 ` Daniel Jacobowitz 0 siblings, 1 reply; 19+ messages in thread From: Pedro Alves @ 2007-03-20 0:39 UTC (permalink / raw) To: gdb-patches [-- Attachment #1: Type: text/plain, Size: 1382 bytes --] Daniel Jacobowitz escreveu: > On Mon, Mar 19, 2007 at 10:57:53PM +0000, Pedro Alves wrote: > >> That has exactly the same effect as it was before the your select frame >> changes. >> That is, only try to get the selected frame when there is one. Maybe there >> should >> still be a function like this in frame.c ? >> > > >> struct frame_info * >> deprecated_get_selected_frame () >> { >> return selected_frame; >> } >> > > Neither this, or your (level >= 0), worked - they both make tui enter some kind of loop, that I can't get a trace. Perhaps in some other host it would be easier (I'm on Cygwin). Bummer. >> (or perhaps call it get_selected_frame_if_any) >> > > The problem is that when we reinitialize the frame cache, the selected > frame is indeterminate - the selected_frame global may randomly be > initialized or NULL depending on what else has been called since we > last did reinit_frame_cache. > Maybe that has something to do with the hangs I see. How about the attached? Move the selected_frame creation logic to select_frame. It fixes both the remote debugging problem, and the repainting problems - probably caused by the get_selected_frame calling select_frame generating extra spurious events. It would need a testsuite run and a bit of cleanup, since the get_selected_frame would lose the parameter. Cheers, Pedro Alves [-- Attachment #2: tui_frame3.diff --] [-- Type: text/plain, Size: 1911 bytes --] gdb/ChangeLog * frame.c (get_selected_frame): Move selected frame to current frame mapping to ... (select_frame): ... here. --- gdb/frame.c | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) Index: src/gdb/frame.c =================================================================== --- src.orig/gdb/frame.c 2007-03-18 23:22:10.000000000 +0000 +++ src/gdb/frame.c 2007-03-20 00:29:42.000000000 +0000 @@ -953,19 +953,6 @@ static struct frame_info *selected_frame struct frame_info * get_selected_frame (const char *message) { - if (selected_frame == NULL) - { - if (message != NULL && (!target_has_registers - || !target_has_stack - || !target_has_memory)) - error (("%s"), message); - /* Hey! Don't trust this. It should really be re-finding the - last selected frame of the currently selected thread. This, - though, is better than nothing. */ - select_frame (get_current_frame ()); - } - /* There is always a frame. */ - gdb_assert (selected_frame != NULL); return selected_frame; } @@ -994,6 +981,21 @@ select_frame (struct frame_info *fi) if (deprecated_selected_frame_level_changed_hook) deprecated_selected_frame_level_changed_hook (frame_relative_level (fi)); + if (fi == NULL) + { + if (!target_has_registers + || !target_has_stack + || !target_has_memory) + return; + + /* Hey! Don't trust this. It should really be re-finding the + last selected frame of the currently selected thread. This, + though, is better than nothing. */ + selected_frame = get_current_frame (); + } + /* There is always a frame. */ + gdb_assert (selected_frame != NULL); + /* FIXME: kseitz/2002-08-28: It would be nice to call selected_frame_level_changed_event() right here, but due to limitations in the current interfaces, we would end up flooding UIs with events ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: TUI + gdbserver broken? 2007-03-20 0:39 ` Pedro Alves @ 2007-03-20 2:56 ` Daniel Jacobowitz 2007-03-20 9:05 ` Pedro Alves 0 siblings, 1 reply; 19+ messages in thread From: Daniel Jacobowitz @ 2007-03-20 2:56 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches On Tue, Mar 20, 2007 at 12:38:46AM +0000, Pedro Alves wrote: > How about the attached? Move the selected_frame creation logic > to select_frame. > It fixes both the remote debugging problem, and the repainting > problems - probably caused by the get_selected_frame calling > select_frame generating extra spurious events. > It would need a testsuite run and a bit of cleanup, since the > get_selected_frame would lose the parameter. Please don't. The comment you're deleting in get_selected_frame is very important: - /* There is always a frame. */ Andrew went to a lot of work making this true, and we now assume it all over the place. Every caller of get_selected_frame now assumes it returns a valid frame. If you're stuck tracking down what TUI is doing on Cygwin, I will compare this with Denis's findings and see what happens on Linux. -- Daniel Jacobowitz CodeSourcery ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: TUI + gdbserver broken? 2007-03-20 2:56 ` Daniel Jacobowitz @ 2007-03-20 9:05 ` Pedro Alves 2007-03-20 23:21 ` Pedro Alves 0 siblings, 1 reply; 19+ messages in thread From: Pedro Alves @ 2007-03-20 9:05 UTC (permalink / raw) To: gdb-patches On 3/20/07, Daniel Jacobowitz <drow@false.org> wrote: > If you're stuck tracking down what TUI is doing on Cygwin, I will > compare this with Denis's findings and see what happens on Linux. > OK, thanks. Sorry I can be of more help here. Debugging TUI on Cygwin really is a pain. Cheers, Pedro Alves ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: TUI + gdbserver broken? 2007-03-20 9:05 ` Pedro Alves @ 2007-03-20 23:21 ` Pedro Alves 2007-03-22 2:51 ` Pedro Alves 0 siblings, 1 reply; 19+ messages in thread From: Pedro Alves @ 2007-03-20 23:21 UTC (permalink / raw) To: gdb-patches [-- Attachment #1: Type: text/plain, Size: 6464 bytes --] Pedro Alves wrote: > > OK, thanks. Sorry I can be of more help here. Debugging TUI on > Cygwin really is a pain. > Gulp, I should stop replying like that first thing in the morning. What a lamer. :/ Humm, debugging a gdb from cvs just after that selected frame patch that broke TUI, I got some things different from what I see from current head. When I first stepped in a native debug session, I saw that deprecated_safe_get_selected_frame returned NULL, because one of target_has_memory, target_has_stack or target_has_registers was 0, but with current CVS they are all still 1. Maybe one of the follow up patches changed it, I don't know. I've just done a careful double debugging session, stepping simultaneously in a native debugging session, and a remote gdbserver debugging session, and surprisingly, I now see that the native version also ends up calling target_fetch_registers - that is, the trace I sent also happens on native cygwin, and what I stated as "obviously wrong" might not be so wrong after all. So, in frame.c, we reinit the frame cache, which selects a NULL frame, which calls the tui hook, that calls deprecated_safe_get_selected_frame (with level -1), which since it doesn't know that the target exited already, it calls get_selected_frame, which ensures there is a frame, but by doing it calls select_frame, that calls again the same tui hook (now with level 0), which calls deprecated_safe_get_selected_frame, returns a non null frame, and then goes into ... : static void tui_selected_frame_level_changed_hook (int level) { struct frame_info *fi; 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. */ if (fi) { struct symtab *s; s = find_pc_symtab (get_frame_pc (fi)); <<<<< ... here. (tui-hooks.c:240) The backtrace is : #0 tui_selected_frame_level_changed_hook (level=0) at ../../gdb-server_search/src/gdb/tui/tui-hooks.c:240 #1 0x0047c359 in select_frame (fi=0x1005b6f8) at ../../gdb-server_search/src/gdb/frame.c:995 #2 0x0047c2ab in get_selected_frame (message=0x0) at ../../gdb-server_search/src/gdb/frame.c:965 #3 0x0047c325 in deprecated_safe_get_selected_frame () at ../../gdb-server_search/src/gdb/frame.c:981 #4 0x004a3191 in tui_selected_frame_level_changed_hook (level=-1) at ../../gdb-server_search/src/gdb/tui/tui-hooks.c:233 #5 0x0047c359 in select_frame (fi=0x0) at ../../gdb-server_search/src/gdb/frame.c:995 #6 0x0047c572 in reinit_frame_cache () at ../../gdb-server_search/src/gdb/frame.c:1095 #7 0x004262f7 in handle_inferior_event (ecs=0x22c690) at ../../gdb-server_search/src/gdb/infrun.c:1302 (...) And then, stepping a bit more, target_fetch_register will eventually be called to fetch the pc. Here is a trace on native Cygwin. #0 do_win32_fetch_inferior_registers (r=8) at ../../gdb-server_search/src/gdb/win32-nat.c:375 #1 0x004723c0 in win32_fetch_inferior_registers (r=8) at ../../gdb-server_search/src/gdb/win32-nat.c:413 #2 0x0047719e in regcache_raw_read (regcache=0x1008cf48, regnum=8, buf=0x22c3b0 "û\005\020ð") at ../../gdb-server_search/src/gdb/regcache.c:510 #3 0x00477769 in regcache_cooked_read (regcache=0x1008cf48, regnum=8, buf=0x22c3b0 "û\005\020ð") at ../../gdb-server_search/src/gdb/regcache.c:588 #4 0x00523565 in sentinel_frame_prev_register (next_frame=0x1005bb88, this_prologue_cache=0x1005bb8c, regnum=8, optimized=0x22c394, lvalp=0x22c388, addrp=0x22c390, realnum=0x22c38c, bufferp=0x22c3b0 "û\005\020ð") at ../../gdb-server_search/src/gdb/sentinel-frame.c:69 #5 0x0047b794 in frame_register_unwind (frame=0x1005bb88, regnum=8, optimizedp=0x22c394, lvalp=0x22c388, addrp=0x22c390, realnump=0x22c38c, bufferp=0x22c3b0 "û\005\020ð") at ../../gdb-server_search/src/gdb/frame.c:589 #6 0x0047ba84 in frame_unwind_register (frame=0x1005bb88, regnum=8, buf=0x22c3b0 "û\005\020ð") at ../../gdb-server_search/src/gdb/frame.c:641 #7 0x0051db80 in i386_unwind_pc (gdbarch=0x1008a3d8, next_frame=0x1005bb88) at ../../gdb-server_search/src/gdb/i386-tdep.c:915 #8 0x0045bfa5 in gdbarch_unwind_pc (gdbarch=0x1008a3d8, next_frame=0x1005bb88) at ../../gdb-server_search/src/gdb/gdbarch.c:3071 #9 0x005235bb in sentinel_frame_prev_pc (next_frame=0x1005bb88, this_prologue_cache=0x1005bb8c) at ../../gdb-server_search/src/gdb/sentinel-frame.c:89 #10 0x0047b37f in frame_pc_unwind (this_frame=0x1005bb88) at ../../gdb-server_search/src/gdb/frame.c:438 #11 0x0047cec4 in get_frame_pc (frame=0x1005bbd8) at ../../gdb-server_search/src/gdb/frame.c:1491 #12 0x004a31a5 in tui_selected_frame_level_changed_hook (level=0) at ../../gdb-server_search/src/gdb/tui/tui-hooks.c:240 #13 0x0047c359 in select_frame (fi=0x1005bbd8) at ../../gdb-server_search/src/gdb/frame.c:995 #14 0x0047c2ab in get_selected_frame (message=0x0) at ../../gdb-server_search/src/gdb/frame.c:965 #15 0x0047c325 in deprecated_safe_get_selected_frame () at ../../gdb-server_search/src/gdb/frame.c:981 #16 0x004a3191 in tui_selected_frame_level_changed_hook (level=-1) at ../../gdb-server_search/src/gdb/tui/tui-hooks.c:233 #17 0x0047c359 in select_frame (fi=0x0) at ../../gdb-server_search/src/gdb/frame.c:995 #18 0x0047c572 in reinit_frame_cache () at ../../gdb-server_search/src/gdb/frame.c:1095 #19 0x004262f7 in handle_inferior_event (ecs=0x22c660) at ../../gdb-server_search/src/gdb/infrun.c:1302 #20 0x00425b87 in wait_for_inferior () at ../../gdb-server_search/src/gdb/infrun.c:1003 (...) Since when native debugging there is no problem (at least on Windows) to read the registers from an already exited process, TUI doesn't brake. But when remote debugging, if the target stub has exited, we can't of course read registers (or whatever) from it. The attached patch implements a possible fix. We can have the target support (in this case remote.c) take care of knowing when it is possible to attend a request from upper layers. That is, ... : (...) Packet vCont (verbose-resume) is supported Sending packet: $vCont;c#a8...Ack Packet received: W00 Sending packet: $g#67...Remote communication error: Software caused connection abort. - ... stop it from trying to to fetch registers when the target already exited (got a 'W' packed). (let me know if I reached the wrong patches patience level. :( ) Cheers, Pedro Alves [-- Attachment #2: tui_frame4.diff --] [-- Type: text/plain, Size: 2554 bytes --] gdb/ChangeLog * remote.c (remote_fetch_registers): Don't try to fetch registers from an already exited inferior. --- gdb/remote.c | 42 +++++++++++++++++++++++++----------------- 1 file changed, 25 insertions(+), 17 deletions(-) Index: src/gdb/remote.c =================================================================== --- src.orig/gdb/remote.c 2007-03-19 19:52:28.000000000 +0000 +++ src/gdb/remote.c 2007-03-20 22:19:58.000000000 +0000 @@ -3684,28 +3684,34 @@ remote_fetch_registers (int regnum) struct remote_state *rs = get_remote_state (); struct remote_arch_state *rsa = get_remote_arch_state (); int i; + int exited; set_thread (PIDGET (inferior_ptid), 1); + exited = (rs->buf[0] == 'W'); + if (regnum >= 0) { struct packet_reg *reg = packet_reg_from_regnum (rsa, regnum); gdb_assert (reg != NULL); - /* If this register might be in the 'g' packet, try that first - - we are likely to read more than one register. If this is the - first 'g' packet, we might be overly optimistic about its - contents, so fall back to 'p'. */ - if (reg->in_g_packet) + if (!exited) { - fetch_registers_using_g (); + /* If this register might be in the 'g' packet, try that first - + we are likely to read more than one register. If this is the + first 'g' packet, we might be overly optimistic about its + contents, so fall back to 'p'. */ if (reg->in_g_packet) + { + fetch_registers_using_g (); + if (reg->in_g_packet) + return; + } + + if (fetch_register_using_p (reg)) return; } - if (fetch_register_using_p (reg)) - return; - /* This register is not available. */ regcache_raw_supply (current_regcache, reg->regnum, NULL); set_register_cached (reg->regnum, -1); @@ -3713,16 +3719,18 @@ remote_fetch_registers (int regnum) return; } - fetch_registers_using_g (); + if (!exited) + fetch_registers_using_g (); for (i = 0; i < NUM_REGS; i++) - if (!rsa->regs[i].in_g_packet) - if (!fetch_register_using_p (&rsa->regs[i])) - { - /* This register is not available. */ - regcache_raw_supply (current_regcache, i, NULL); - set_register_cached (i, -1); - } + if (exited + || (!rsa->regs[i].in_g_packet + && !fetch_register_using_p (&rsa->regs[i]))) + { + /* This register is not available. */ + regcache_raw_supply (current_regcache, i, NULL); + set_register_cached (i, -1); + } } /* Prepare to store registers. Since we may send them all (using a ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: TUI + gdbserver broken? 2007-03-20 23:21 ` Pedro Alves @ 2007-03-22 2:51 ` Pedro Alves 2007-03-27 20:15 ` Daniel Jacobowitz 0 siblings, 1 reply; 19+ messages in thread From: Pedro Alves @ 2007-03-22 2:51 UTC (permalink / raw) To: gdb-patches [-- 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. */ ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: TUI + gdbserver broken? 2007-03-22 2:51 ` Pedro Alves @ 2007-03-27 20:15 ` Daniel Jacobowitz 2007-03-29 2:26 ` Pedro Alves 0 siblings, 1 reply; 19+ messages in thread From: Daniel Jacobowitz @ 2007-03-27 20:15 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches On Thu, Mar 22, 2007 at 02:51:00AM +0000, Pedro Alves wrote: > 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. The problem, really, is that these are silly places for hooks when GDB's internal state is inconsistent. The registers_changed hook is getting you in trouble because it is called when we invalidate registers - having it immediately fetch new registers is very bad! See the comment in infrun.c right before this call to registers_changed for more. Fortunately TUI's registers_changed hook hasn't done anything in a long time. Insight doesn't even use it. We can delete it. Could you see if the attached works for you? -- Daniel Jacobowitz CodeSourcery 2007-03-27 Daniel Jacobowitz <dan@codesourcery.com> * defs.h (deprecated_registers_changed_hook): Delete declaration. * interps.c (clear_interpreter_hooks): Do not clear deprecated_registers_changed_hook. * regcache.c (registers_changed): Do not call it. * top.c (deprecated_registers_changed_hook): Do not define it. * mi/mi-interp.c (mi_command_loop): Do not clear it. * tui/tui-hooks.c (tui_install_hooks): Do not install it. (tui_remove_hooks): Do not remove it. (tui_selected_frame_level_changed_hook): Check for negative level. Use get_selected_frame. (tui_registers_changed_hook): Deleted. Index: defs.h =================================================================== RCS file: /cvs/src/src/gdb/defs.h,v retrieving revision 1.202 diff -u -p -r1.202 defs.h --- defs.h 8 Feb 2007 16:18:56 -0000 1.202 +++ defs.h 27 Mar 2007 20:12:13 -0000 @@ -1135,7 +1135,6 @@ extern void (*deprecated_create_breakpoi extern void (*deprecated_delete_breakpoint_hook) (struct breakpoint * bpt); extern void (*deprecated_modify_breakpoint_hook) (struct breakpoint * bpt); extern void (*deprecated_interactive_hook) (void); -extern void (*deprecated_registers_changed_hook) (void); extern void (*deprecated_readline_begin_hook) (char *, ...) ATTRIBUTE_FPTR_PRINTF_1; extern char *(*deprecated_readline_hook) (char *); Index: interps.c =================================================================== RCS file: /cvs/src/src/gdb/interps.c,v retrieving revision 1.19 diff -u -p -r1.19 interps.c --- interps.c 9 Jan 2007 17:58:51 -0000 1.19 +++ interps.c 27 Mar 2007 20:12:13 -0000 @@ -329,7 +329,6 @@ clear_interpreter_hooks (void) deprecated_delete_breakpoint_hook = 0; deprecated_modify_breakpoint_hook = 0; deprecated_interactive_hook = 0; - deprecated_registers_changed_hook = 0; deprecated_readline_begin_hook = 0; deprecated_readline_hook = 0; deprecated_readline_end_hook = 0; Index: regcache.c =================================================================== RCS file: /cvs/src/src/gdb/regcache.c,v retrieving revision 1.142 diff -u -p -r1.142 regcache.c --- regcache.c 13 Jan 2007 23:24:43 -0000 1.142 +++ regcache.c 27 Mar 2007 20:12:14 -0000 @@ -464,9 +464,6 @@ registers_changed (void) for (i = 0; i < current_regcache->descr->nr_raw_registers; i++) set_register_cached (i, 0); - - if (deprecated_registers_changed_hook) - deprecated_registers_changed_hook (); } /* DEPRECATED_REGISTERS_FETCHED () Index: top.c =================================================================== RCS file: /cvs/src/src/gdb/top.c,v retrieving revision 1.119 diff -u -p -r1.119 top.c --- top.c 28 Feb 2007 15:55:54 -0000 1.119 +++ top.c 27 Mar 2007 20:12:14 -0000 @@ -271,11 +271,6 @@ void (*deprecated_detach_hook) (void); void (*deprecated_interactive_hook) (void); -/* Called when the registers have changed, as a hint to a GUI - to minimize window update. */ - -void (*deprecated_registers_changed_hook) (void); - /* Tell the GUI someone changed the register REGNO. -1 means that the caller does not know which register changed or that several registers have changed (see value_assign). */ Index: mi/mi-interp.c =================================================================== RCS file: /cvs/src/src/gdb/mi/mi-interp.c,v retrieving revision 1.18 diff -u -p -r1.18 mi-interp.c --- mi/mi-interp.c 9 Jan 2007 17:59:08 -0000 1.18 +++ mi/mi-interp.c 27 Mar 2007 20:12:14 -0000 @@ -349,7 +349,6 @@ mi_command_loop (int mi_version) deprecated_delete_breakpoint_hook = 0; deprecated_modify_breakpoint_hook = 0; deprecated_interactive_hook = 0; - deprecated_registers_changed_hook = 0; deprecated_readline_begin_hook = 0; deprecated_readline_hook = 0; deprecated_readline_end_hook = 0; Index: tui/tui-hooks.c =================================================================== RCS file: /cvs/src/src/gdb/tui/tui-hooks.c,v retrieving revision 1.30 diff -u -p -r1.30 tui-hooks.c --- tui/tui-hooks.c 7 Mar 2007 10:29:47 -0000 1.30 +++ tui/tui-hooks.c 27 Mar 2007 20:12:14 -0000 @@ -124,26 +124,10 @@ tui_query_hook (const char * msg, va_lis return retval; } -/* Prevent recursion of deprecated_registers_changed_hook(). */ +/* Prevent recursion of deprecated_register_changed_hook(). */ static int tui_refreshing_registers = 0; static void -tui_registers_changed_hook (void) -{ - struct frame_info *fi; - - fi = get_selected_frame (NULL); - if (tui_refreshing_registers == 0) - { - tui_refreshing_registers = 1; -#if 0 - tui_check_data_values (fi); -#endif - tui_refreshing_registers = 0; - } -} - -static void tui_register_changed_hook (int regno) { struct frame_info *fi; @@ -230,7 +214,11 @@ tui_selected_frame_level_changed_hook (i { struct frame_info *fi; - fi = deprecated_safe_get_selected_frame (); + /* Negative level means that the selected frame was cleared. */ + if (level < 0) + return; + + fi = get_selected_frame (NULL); /* Ensure that symbols for this frame are read in. Also, determine the source language of this frame, and switch to it if desired. */ if (fi) @@ -289,7 +277,6 @@ tui_install_hooks (void) /* Install the event hooks. */ tui_old_event_hooks = deprecated_set_gdb_event_hooks (&tui_event_hooks); - deprecated_registers_changed_hook = tui_registers_changed_hook; deprecated_register_changed_hook = tui_register_changed_hook; deprecated_detach_hook = tui_detach_hook; } @@ -302,7 +289,6 @@ tui_remove_hooks (void) deprecated_selected_frame_level_changed_hook = 0; deprecated_print_frame_info_listing_hook = 0; deprecated_query_hook = 0; - deprecated_registers_changed_hook = 0; deprecated_register_changed_hook = 0; deprecated_detach_hook = 0; ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: TUI + gdbserver broken? 2007-03-27 20:15 ` Daniel Jacobowitz @ 2007-03-29 2:26 ` Pedro Alves 2007-03-29 18:55 ` Daniel Jacobowitz 0 siblings, 1 reply; 19+ messages in thread From: Pedro Alves @ 2007-03-29 2:26 UTC (permalink / raw) To: gdb-patches Daniel Jacobowitz wrote: > Could you see if the attached works for you? Perfectly - fixes both the "Remote communication error", and the extra refreshes. Thanks! Cheers, Pedro Alves ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: TUI + gdbserver broken? 2007-03-29 2:26 ` Pedro Alves @ 2007-03-29 18:55 ` Daniel Jacobowitz 0 siblings, 0 replies; 19+ messages in thread From: Daniel Jacobowitz @ 2007-03-29 18:55 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches On Wed, Mar 28, 2007 at 10:12:11PM +0100, Pedro Alves wrote: > Daniel Jacobowitz wrote: > >Could you see if the attached works for you? > Perfectly - fixes both the "Remote communication error", and > the extra refreshes. Great. I checked it in. -- Daniel Jacobowitz CodeSourcery ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2007-04-18 6:57 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2007-03-19 1:53 TUI + gdbserver broken? Pedro Alves 2007-03-19 2:11 ` Daniel Jacobowitz 2007-03-19 15:58 ` Denis PILAT 2007-03-27 20:09 ` Daniel Jacobowitz 2007-04-17 15:48 ` [RFA] TUI is broken under Solaris Denis PILAT 2007-04-17 15:51 ` Daniel Jacobowitz 2007-04-18 8:24 ` Denis PILAT 2007-03-19 21:43 ` TUI + gdbserver broken? Pedro Alves 2007-03-19 22:14 ` Daniel Jacobowitz 2007-03-19 23:31 ` Pedro Alves 2007-03-19 23:41 ` Daniel Jacobowitz 2007-03-20 0:39 ` Pedro Alves 2007-03-20 2:56 ` Daniel Jacobowitz 2007-03-20 9:05 ` Pedro Alves 2007-03-20 23:21 ` Pedro Alves 2007-03-22 2:51 ` Pedro Alves 2007-03-27 20:15 ` Daniel Jacobowitz 2007-03-29 2:26 ` Pedro Alves 2007-03-29 18:55 ` Daniel Jacobowitz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox