Re: TUI + gdbserver broken?

[Pedro Alves <pedro_alves@portugalmail.pt>]

[-- 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);