[RFA] add selected-frame-level-changed events

[RFA] add selected-frame-level-changed events

[Keith Seitz]

Hi,

Here's the follow-up to the gdb-event.sh change which added the 
"selected-frame-level-changed" event, which adds notifications for this 
event in the right places.

It would have been nice to just put one notification in select_frame, but 
as we all know (or should), that function is called WAY to many times 
internally to be of any use for this purpose. Maybe a fixme is needed?

Keith

ChangeLog
2002-08-27  Keith Seitz  <keiths@redhat.com>

        * stack.c (select_frame_command): Send selected-frame-level-changed
        event notification, but only if the level actually changed.
        (up_silently_base): Add selected-frame-level-changed event
        notification.
        (down_silently_base): Likewise.

Patch
Index: stack.c
===================================================================
RCS file: /cvs/src/src/gdb/stack.c,v
retrieving revision 1.40
diff -p -r1.40 stack.c
*** stack.c	11 Jul 2002 19:29:08 -0000	1.40
--- stack.c	27 Aug 2002 20:33:38 -0000
*************** select_frame_command_wrapper (char *leve
*** 1620,1627 ****
  static void
  select_frame_command (char *level_exp, int from_tty)
  {
!   register struct frame_info *frame, *frame1;
!   unsigned int level = 0;
  
    if (!target_has_stack)
      error ("No stack.");
--- 1620,1627 ----
  static void
  select_frame_command (char *level_exp, int from_tty)
  {
!   struct frame_info *frame;
!   int level = frame_relative_level (selected_frame);
  
    if (!target_has_stack)
      error ("No stack.");
*************** select_frame_command (char *level_exp, i
*** 1629,1634 ****
--- 1629,1636 ----
    frame = parse_frame_specification (level_exp);
  
    select_frame (frame);
+   if (level != frame_relative_level (selected_frame))
+     selected_frame_level_changed_event (frame_relative_level (selected_frame));
  }
  
  /* The "frame" command.  With no arg, print selected frame briefly.
*************** up_silently_base (char *count_exp)
*** 1674,1679 ****
--- 1676,1682 ----
    if (count1 != 0 && count_exp == 0)
      error ("Initial frame selected; you cannot go up.");
    select_frame (fi);
+   selected_frame_level_changed_event (frame_relative_level (selected_frame));
  }
  
  static void
*************** down_silently_base (char *count_exp)
*** 1719,1724 ****
--- 1722,1728 ----
      }
  
    select_frame (frame);
+   selected_frame_level_changed_event (frame_relative_level (selected_frame));
  }
  
  /* ARGSUSED */

Re: [RFA] add selected-frame-level-changed events

[Andrew Cagney]

> Hi,
> 
> Here's the follow-up to the gdb-event.sh change which added the 
> "selected-frame-level-changed" event, which adds notifications for this 
> event in the right places.
> 
> It would have been nice to just put one notification in select_frame, but 
> as we all know (or should), that function is called WAY to many times 
> internally to be of any use for this purpose. Maybe a fixme is needed?

Why not put it in there anyway?

I think, not putting it in select_frame() is the part that needs the fixme.

Andrew

Re: [RFA] add selected-frame-level-changed events

[Keith Seitz]

On Tue, 27 Aug 2002, Andrew Cagney wrote:

> Why not put it in there anyway?

You mean add a notification in select_frame? Because UIs don't care when 
GDB is temporarily switching frames for whatever reason. What matters is 
when something happens to cause the current frame to be changed and cause 
gdb and the MI client to get out of sync. As far as I know, this only 
happens via the command line frame commands (up, down, frame). There might 
be others, but I've never seen them.

> I think, not putting it in select_frame() is the part that needs the fixme.

Right, that's what I meant. Sorry if I was vague. Ideally, we would like 
it in select_frame, but we cannot do that right now because the UI would 
be informed of billions of frame changes per step, especially if varobj is 
operating.

Keith

Re: [RFA] add selected-frame-level-changed events

[Andrew Cagney]

> On Tue, 27 Aug 2002, Andrew Cagney wrote:
> 
> 
>> Why not put it in there anyway?
> 
> 
> You mean add a notification in select_frame? Because UIs don't care when 
> GDB is temporarily switching frames for whatever reason. What matters is 
> when something happens to cause the current frame to be changed and cause 
> gdb and the MI client to get out of sync. As far as I know, this only 
> happens via the command line frame commands (up, down, frame). There might 
> be others, but I've never seen them.

Wonder what the TUI gets up to :-)

>> I think, not putting it in select_frame() is the part that needs the fixme.
> 
> 
> Right, that's what I meant. Sorry if I was vague. Ideally, we would like 
> it in select_frame, but we cannot do that right now because the UI would 
> be informed of billions of frame changes per step, especially if varobj is 
> operating.

Ok.  Can you just add comments explaining the above, especially in 
select_frame(), so that people know why the call isn't yet there.

As for the unnecessary select frame calls, I guess ``we'' are working on 
it.  I suspect that varobj could do with an audit --- use FRAME_ID() and 
frame parameterized versions of various functions.

Andrew

Re: [RFA] add selected-frame-level-changed events

[Keith Seitz]

On Tue, 27 Aug 2002, Andrew Cagney wrote:

> Ok.  Can you just add comments explaining the above, especially in 
> select_frame(), so that people know why the call isn't yet there.

I've appended a revised patch for your review.

> As for the unnecessary select frame calls, I guess ``we'' are working on 
> it.  I suspect that varobj could do with an audit --- use FRAME_ID() and 
> frame parameterized versions of various functions.

Hmmm... Patch coming.

Keith

Revised patch:

Index: stack.c
===================================================================
RCS file: /cvs/src/src/gdb/stack.c,v
retrieving revision 1.40
diff -p -r1.40 stack.c
*** stack.c	11 Jul 2002 19:29:08 -0000	1.40
--- stack.c	28 Aug 2002 15:36:14 -0000
*************** select_frame (struct frame_info *fi)
*** 1516,1521 ****
--- 1516,1527 ----
       frame is being invalidated.  */
    if (selected_frame_level_changed_hook)
      selected_frame_level_changed_hook (frame_relative_level (fi));
+   /* NOTE: kseitz/2002-08-28: It would be nice to call
+      selected_frame_level_changed_event right here, but we would end up
+      flooding the UI with events because select_frame is used extensively
+      internally.  Instead, event notifications are scattered through
+      the "UI" code, select_frame_command, up_silently_base, and
+      down_silently_base. */
  
    /* Ensure that symbols for this frame are read in.  Also, determine the
       source language of this frame, and switch to it if desired.  */
*************** select_frame_command_wrapper (char *leve
*** 1620,1627 ****
  static void
  select_frame_command (char *level_exp, int from_tty)
  {
!   register struct frame_info *frame, *frame1;
!   unsigned int level = 0;
  
    if (!target_has_stack)
      error ("No stack.");
--- 1626,1633 ----
  static void
  select_frame_command (char *level_exp, int from_tty)
  {
!   struct frame_info *frame;
!   int level = frame_relative_level (selected_frame);
  
    if (!target_has_stack)
      error ("No stack.");
*************** select_frame_command (char *level_exp, i
*** 1629,1634 ****
--- 1635,1642 ----
    frame = parse_frame_specification (level_exp);
  
    select_frame (frame);
+   if (level != frame_relative_level (selected_frame))
+     selected_frame_level_changed_event (frame_relative_level (selected_frame));
  }
  
  /* The "frame" command.  With no arg, print selected frame briefly.
*************** up_silently_base (char *count_exp)
*** 1674,1679 ****
--- 1682,1688 ----
    if (count1 != 0 && count_exp == 0)
      error ("Initial frame selected; you cannot go up.");
    select_frame (fi);
+   selected_frame_level_changed_event (frame_relative_level (selected_frame));
  }
  
  static void
*************** down_silently_base (char *count_exp)
*** 1719,1724 ****
--- 1728,1734 ----
      }
  
    select_frame (frame);
+   selected_frame_level_changed_event (frame_relative_level (selected_frame));
  }
  
  /* ARGSUSED */

Re: [RFA] add selected-frame-level-changed events

[Keith Seitz]

On Wed, 28 Aug 2002, Keith Seitz wrote:

> > As for the unnecessary select frame calls, I guess ``we'' are working on 
> > it.  I suspect that varobj could do with an audit --- use FRAME_ID() and 
> > frame parameterized versions of various functions.
> 
> Hmmm... Patch coming.

Sigh, varobj seems to already use frame_ids, but not any 
frame-parameterized functions. Ahh, well, that's because there are none 
(yet?)!

I guess a patch won't be coming any time soon, then! :-(

Keith

Re: [RFA] add selected-frame-level-changed events

[Andrew Cagney]

> +   /* NOTE: kseitz/2002-08-28: It would be nice to call

``FIXME''.  The call can't be here due to limitations in the current 
implementation :-)

> +      selected_frame_level_changed_event right here, but we would end up
> +      flooding the UI with events because select_frame is used extensively
> +      internally.  Instead, event notifications are scattered through
> +      the "UI" code, select_frame_command, up_silently_base, and
> +      down_silently_base. */
>   


Suggest adding something like.

``Once everything is parameterized with a frame, the hook can be moved 
here since at that point, this function will be called when the users 
selected frame is being changed.''.

So that the change has an ``exit clause''.  that way it is clear when 
the fixme can be deleted/fixed.

Otherwize, yes, just commit
Andrew

Re: [RFA] add selected-frame-level-changed events

[Keith Seitz]

On Wed, 28 Aug 2002, Andrew Cagney wrote:

> So that the change has an ``exit clause''.  that way it is clear when 
> the fixme can be deleted/fixed.

Noted.

> Otherwize, yes, just commit

For the record, here's what I've committed:

ChangeLog
2002-08-28  Keith Seitz  <keiths@redhat.com>

        * stack.c (select_frame): Add FIXME concerning selected-frame
        events.
        (select_frame_command): Send selected-frame-level-changed
        event notification, but only if the level actually changed.
        (up_silently_base): Add selected-frame-level-changed event
        notification.
        (down_silently_base): Likewise.

Patch
Index: stack.c
===================================================================
RCS file: /cvs/src/src/gdb/stack.c,v
retrieving revision 1.40
diff -p -r1.40 stack.c
*** stack.c	11 Jul 2002 19:29:08 -0000	1.40
--- stack.c	28 Aug 2002 17:26:22 -0000
*************** select_frame (struct frame_info *fi)
*** 1517,1522 ****
--- 1517,1531 ----
    if (selected_frame_level_changed_hook)
      selected_frame_level_changed_hook (frame_relative_level (fi));
  
+   /* 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
+      because select_frame is used extensively internally.
+ 
+      Once we have frame-parameterized frame (and frame-related) commands,
+      the event notification can be moved here, since this function will only
+      be called when the users selected frame is being changed. */
+ 
    /* 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)
*************** select_frame_command_wrapper (char *leve
*** 1620,1627 ****
  static void
  select_frame_command (char *level_exp, int from_tty)
  {
!   register struct frame_info *frame, *frame1;
!   unsigned int level = 0;
  
    if (!target_has_stack)
      error ("No stack.");
--- 1629,1636 ----
  static void
  select_frame_command (char *level_exp, int from_tty)
  {
!   struct frame_info *frame;
!   int level = frame_relative_level (selected_frame);
  
    if (!target_has_stack)
      error ("No stack.");
*************** select_frame_command (char *level_exp, i
*** 1629,1634 ****
--- 1638,1645 ----
    frame = parse_frame_specification (level_exp);
  
    select_frame (frame);
+   if (level != frame_relative_level (selected_frame))
+     selected_frame_level_changed_event (frame_relative_level (selected_frame));
  }
  
  /* The "frame" command.  With no arg, print selected frame briefly.
*************** up_silently_base (char *count_exp)
*** 1674,1679 ****
--- 1685,1691 ----
    if (count1 != 0 && count_exp == 0)
      error ("Initial frame selected; you cannot go up.");
    select_frame (fi);
+   selected_frame_level_changed_event (frame_relative_level (selected_frame));
  }
  
  static void
*************** down_silently_base (char *count_exp)
*** 1719,1724 ****
--- 1731,1737 ----
      }
  
    select_frame (frame);
+   selected_frame_level_changed_event (frame_relative_level (selected_frame));
  }
  
  /* ARGSUSED */