TUI + gdbserver broken?

TUI + gdbserver broken?

[Pedro Alves]

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

Re: TUI + gdbserver broken?

[Daniel Jacobowitz]

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

Re: TUI + gdbserver broken?

[Denis PILAT]

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

Re: TUI + gdbserver broken?

[Pedro Alves]

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

Re: TUI + gdbserver broken?

[Daniel Jacobowitz]

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

Re: TUI + gdbserver broken?

[Pedro Alves]

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

Re: TUI + gdbserver broken?

[Daniel Jacobowitz]

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

Re: TUI + gdbserver broken?

[Pedro Alves]

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

Re: TUI + gdbserver broken?

[Daniel Jacobowitz]

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

Re: TUI + gdbserver broken?

[Pedro Alves]

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

Re: TUI + gdbserver broken?

[Pedro Alves]

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

Re: TUI + gdbserver broken?

[Pedro Alves]

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

Re: TUI + gdbserver broken?

[Daniel Jacobowitz]

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

Re: TUI + gdbserver broken?

[Daniel Jacobowitz]

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;
 

Re: TUI + gdbserver broken?

[Pedro Alves]

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

Re: TUI + gdbserver broken?

[Daniel Jacobowitz]

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

[RFA] TUI is broken under Solaris

[Denis PILAT]

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

Re: [RFA] TUI is broken under Solaris

[Daniel Jacobowitz]

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

Re: [RFA] TUI is broken under Solaris

[Denis PILAT]

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

Re: TUI + gdbserver broken?

[Denis PILAT]

Pedro Alves wrote:
> Hi,
>
> TUI + gdbserver seems to be broken on head.  Can't reproduce when
> TUI + native debugging.
>
As far as the TUI is concerned:
It's broken with Solaris Native debugging.
I've spent a few hours debugging that problem but I have not solution yet !!
I also tried using deprecated_get_selected_frame without any better 
results :(

This problem is present in the CVS HEAD but also in the 6.6 released 
version, so I don't think it comes from recent changes.

-- 
Denis

P.S. here is the backtrace I got in the native solaris TUI
Program received signal SIGTTIN, Stopped (tty 
input).                                                                 
Line: ??   PC: 0xff3a3958
0xff19e880 in _read () from /usr/lib/libc.so.1
(gdb) btto change it and/or distribute copies of it under certain 
conditions.
#0  0xff19e880 in _read () from /usr/lib/libc.so.1
#1  0xff33da4c in _chkinput () from /usr/lib/libcurses.so.1ty" for details.
#2  0xff34e680 in wrefresh () from /usr/lib/libcurses.so.1
#3  0x00095628 in tui_refresh_win (win_info=0x2e73c4)
    at 
/project/flexdbug/user/pd10/gdb/sts-gdb-repository/vendor/cvs/head/src/gdb/tui/tui-wingeneral.c:63testsuite/gdb.base/break.c, 
line 90.
#4  0x00092b18 in tui_show_locator_content () at 
/project/flexdbug/user/pd10/gdb/sts-gdb-repository/vendor/cvs/head/src/gdb/tui/tui-stack.c:261
#5  0x00092db8 in tui_show_frame_info 
(fi=0x317890)uild-sts-gdb-repository/cvshead-sol8/gdb/testsuite/gdb.base/break
    at 
/project/flexdbug/user/pd10/gdb/sts-gdb-repository/vendor/cvs/head/src/gdb/tui/tui-stack.c:341
#6  0x0008e6bc in tui_selected_frame_level_changed_hook (level=0)
    at 
/project/flexdbug/user/pd10/gdb/sts-gdb-repository/vendor/cvs/head/src/gdb/tui/tui-hooks.c:249
#7  0x0016a4f8 in select_frame (fi=0x317890) at 
/project/flexdbug/user/pd10/gdb/sts-gdb-repository/vendor/cvs/head/src/gdb/frame.c:986
#8  0x0016a3bc in get_selected_frame (message=0x0) at 
/project/flexdbug/user/pd10/gdb/sts-gdb-repository/vendor/cvs/head/src/gdb/frame.c:956
#9  0x0016a48c in deprecated_safe_get_selected_frame ()
    at 
/project/flexdbug/user/pd10/gdb/sts-gdb-repository/vendor/cvs/head/src/gdb/frame.c:972
#10 0x0008e684 in tui_selected_frame_level_changed_hook (level=-1)
    at 
/project/flexdbug/user/pd10/gdb/sts-gdb-repository/vendor/cvs/head/src/gdb/tui/tui-hooks.c:233
#11 0x0016a4f8 in select_frame (fi=0x0) at 
/project/flexdbug/user/pd10/gdb/sts-gdb-repository/vendor/cvs/head/src/gdb/frame.c:986
#12 0x0016a80c in reinit_frame_cache () at 
/project/flexdbug/user/pd10/gdb/sts-gdb-repository/vendor/cvs/head/src/gdb/frame.c:1086
#13 0x000d66e8 in handle_inferior_event (ecs=0xffbee0a0)
    at 
/project/flexdbug/user/pd10/gdb/sts-gdb-repository/vendor/cvs/head/src/gdb/infrun.c:1306
#14 0x000d5f38 in wait_for_inferior () at 
/project/flexdbug/user/pd10/gdb/sts-gdb-repository/vendor/cvs/head/src/gdb/infrun.c:1007
#15 0x00177020 in startup_inferior (ntraps=2) at 
/project/flexdbug/user/pd10/gdb/sts-gdb-repository/vendor/cvs/head/src/gdb/fork-child.c:435
#16 0x00063bc0 in procfs_init_inferior (pid=3577048)
    at 
/project/flexdbug/user/pd10/gdb/sts-gdb-repository/vendor/cvs/head/src/gdb/procfs.c:4872
#17 0x00176fa4 in fork_inferior (
    exec_file_arg=0x340418 
"/project/flexdbug/user/pd10/gdb/build-sts-gdb-repository/cvshead-sol8/gdb/testsuite/gdb.base/break",
    allargs=0x327008 "", env=0x30bdd8, traceme_fun=0x63bcc 
<procfs_set_exec_trap>, init_trace_fun=0x63940 <procfs_init_inferior>,
    pre_trace_fun=0, shell_file_arg=0xffbef892 "/bin/tcsh")
    at 
/project/flexdbug/user/pd10/gdb/sts-gdb-repository/vendor/cvs/head/src/gdb/fork-child.c:396
#18 0x00063ebc in procfs_create_inferior (
    exec_file=0x340418 
"/project/flexdbug/user/pd10/gdb/build-sts-gdb-repository/cvshead-sol8/gdb/testsuite/gdb.base/break",
   allargs=0x327008 ""Quit
) at 
/project/flexdbug/user/pd10/gdb/sts-gdb-repository/vendor/cvs/head/src/gdb/procfs.c:5101
#19 0x0008b640 in sol_thread_create_inferior (
    exec_file=0x340418 
"/project/flexdbug/user/pd10/gdb/build-sts-gdb-repository/cvshead-sol8/gdb/testsuite/gdb.base/break",
    allargs=0x327008 "", env=0x30bdd8, from_tty=1)
    at 
/project/flexdbug/user/pd10/gdb/sts-gdb-repository/vendor/cvs/head/src/gdb/sol-thread.c:758
#20 0x000fdaec in find_default_create_inferior (
    exec_file=0x340418 
"/project/flexdbug/user/pd10/gdb/build-sts-gdb-repository/cvshead-sol8/gdb/testsuite/gdb.base/break",
    allargs=0x327008 "", env=0x30bdd8, from_tty=1) at 
/project/flexdbug/user/pd10/gdb/sts-gdb-repository/vendor/cvs/head/src/gdb/target.c:1768
#21 0x000d2724 in run_command_1 (args=0x0, from_tty=1, tbreak_at_main=0)
    at 
/project/flexdbug/user/pd10/gdb/sts-gdb-repository/vendor/cvs/head/src/gdb/infcmd.c:558
#22 0x0007cb74 in do_cfunc (c=0x30b978,
    args=0x340418 
"/project/flexdbug/user/pd10/gdb/build-sts-gdb-repository/cvshead-sol8/gdb/testsuite/gdb.base/break", 
from_tty=1)
    at 
/project/flexdbug/user/pd10/gdb/sts-gdb-repository/vendor/cvs/head/src/gdb/cli/cli-decode.c:62