=frame-selected MI notification

=frame-selected MI notification

[Dmitry Dzhus]

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

This patch adds =frame-selected notification to GDB/MI. This
notification is important, because it provides front-ends a way to find
out that user has asked for frame change via frontend console using CLI
command `up`, `down` or `frame N`.

I had to move `frame_info` structure description to `frame.h` so its
fields may be accessed from mi-main.c. I'm not sure if this is the
correct approach.

Nick Roberts once posted a patch which implements both =thread-selected
and =frame-selected via observers:
http://sourceware.org/ml/gdb-patches/2008-04/msg00377.html. Current
implementation of =thread-selected in mi-main.c uses a different
approach, comparing current thread ID before and after executing a
command in `mi_execute_command` function. My patch does the same for
frame level. I don't know how is it technically better. I think it would
be nice if somebody reviewed Nick's patch once again or at least
accepted the one proposed by me.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gdb-frame-notification.patch --]
[-- Type: text/x-patch, Size: 6554 bytes --]

diff --git a/gdb/frame.c b/gdb/frame.c
index 67e0607..8ad349b 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -57,71 +57,6 @@ static struct frame_info *get_prev_frame_raw (struct frame_info *this_frame);
    frames (e.g. "set heuristic-fence-post" in mips-tdep.c, or anything
    which reads new symbols)), we should call reinit_frame_cache.  */
 
-struct frame_info
-{
-  /* Level of this frame.  The inner-most (youngest) frame is at level
-     0.  As you move towards the outer-most (oldest) frame, the level
-     increases.  This is a cached value.  It could just as easily be
-     computed by counting back from the selected frame to the inner
-     most frame.  */
-  /* NOTE: cagney/2002-04-05: Perhaps a level of ``-1'' should be
-     reserved to indicate a bogus frame - one that has been created
-     just to keep GDB happy (GDB always needs a frame).  For the
-     moment leave this as speculation.  */
-  int level;
-
-  /* The frame's low-level unwinder and corresponding cache.  The
-     low-level unwinder is responsible for unwinding register values
-     for the previous frame.  The low-level unwind methods are
-     selected based on the presence, or otherwise, of register unwind
-     information such as CFI.  */
-  void *prologue_cache;
-  const struct frame_unwind *unwind;
-
-  /* Cached copy of the previous frame's architecture.  */
-  struct
-  {
-    int p;
-    struct gdbarch *arch;
-  } prev_arch;
-
-  /* Cached copy of the previous frame's resume address.  */
-  struct {
-    int p;
-    CORE_ADDR value;
-  } prev_pc;
-  
-  /* Cached copy of the previous frame's function address.  */
-  struct
-  {
-    CORE_ADDR addr;
-    int p;
-  } prev_func;
-  
-  /* This frame's ID.  */
-  struct
-  {
-    int p;
-    struct frame_id value;
-  } this_id;
-  
-  /* The frame's high-level base methods, and corresponding cache.
-     The high level base methods are selected based on the frame's
-     debug info.  */
-  const struct frame_base *base;
-  void *base_cache;
-
-  /* Pointers to the next (down, inner, younger) and previous (up,
-     outer, older) frame_info's in the frame cache.  */
-  struct frame_info *next; /* down, inner, younger */
-  int prev_p;
-  struct frame_info *prev; /* up, outer, older */
-
-  /* The reason why we could not set PREV, or UNWIND_NO_REASON if we
-     could.  Only valid when PREV_P is set.  */
-  enum unwind_stop_reason stop_reason;
-};
-
 /* Flag to control debugging.  */
 
 int frame_debug;
diff --git a/gdb/frame.h b/gdb/frame.h
index febef5c..900d263 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -80,8 +80,6 @@ struct ui_file;
 
 /* The frame object.  */
 
-struct frame_info;
-
 /* The frame object's ID.  This provides a per-frame unique identifier
    that can be used to relocate a `struct frame_info' after a target
    resume or a frame cache destruct.  It of course assumes that the
@@ -440,6 +438,72 @@ enum unwind_stop_reason
     UNWIND_NO_SAVED_PC,
   };
 
+
+struct frame_info
+{
+  /* Level of this frame.  The inner-most (youngest) frame is at level
+     0.  As you move towards the outer-most (oldest) frame, the level
+     increases.  This is a cached value.  It could just as easily be
+     computed by counting back from the selected frame to the inner
+     most frame.  */
+  /* NOTE: cagney/2002-04-05: Perhaps a level of ``-1'' should be
+     reserved to indicate a bogus frame - one that has been created
+     just to keep GDB happy (GDB always needs a frame).  For the
+     moment leave this as speculation.  */
+  int level;
+
+  /* The frame's low-level unwinder and corresponding cache.  The
+     low-level unwinder is responsible for unwinding register values
+     for the previous frame.  The low-level unwind methods are
+     selected based on the presence, or otherwise, of register unwind
+     information such as CFI.  */
+  void *prologue_cache;
+  const struct frame_unwind *unwind;
+
+  /* Cached copy of the previous frame's architecture.  */
+  struct
+  {
+    int p;
+    struct gdbarch *arch;
+  } prev_arch;
+
+  /* Cached copy of the previous frame's resume address.  */
+  struct {
+    int p;
+    CORE_ADDR value;
+  } prev_pc;
+  
+  /* Cached copy of the previous frame's function address.  */
+  struct
+  {
+    CORE_ADDR addr;
+    int p;
+  } prev_func;
+  
+  /* This frame's ID.  */
+  struct
+  {
+    int p;
+    struct frame_id value;
+  } this_id;
+  
+  /* The frame's high-level base methods, and corresponding cache.
+     The high level base methods are selected based on the frame's
+     debug info.  */
+  const struct frame_base *base;
+  void *base_cache;
+
+  /* Pointers to the next (down, inner, younger) and previous (up,
+     outer, older) frame_info's in the frame cache.  */
+  struct frame_info *next; /* down, inner, younger */
+  int prev_p;
+  struct frame_info *prev; /* up, outer, older */
+
+  /* The reason why we could not set PREV, or UNWIND_NO_REASON if we
+     could.  Only valid when PREV_P is set.  */
+  enum unwind_stop_reason stop_reason;
+};
+
 /* Return the reason why we can't unwind past this frame.  */
 
 enum unwind_stop_reason get_frame_unwind_stop_reason (struct frame_info *);
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 6aa1d08..7239700 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -1277,6 +1277,11 @@ mi_execute_command (char *cmd, int from_tty)
     {
       struct gdb_exception result;
       ptid_t previous_ptid = inferior_ptid;
+      struct frame_info *previous_frame;
+      if (has_stack_frames())
+          previous_frame = get_selected_frame (NULL);
+      else
+          previous_frame = NULL;
 
       if (do_timings)
 	{
@@ -1338,6 +1343,29 @@ mi_execute_command (char *cmd, int from_tty)
 	    }
 	}
 
+      /* =frame-selected notification */
+      if (ui_out_is_mi_like_p (interp_ui_out (top_level_interpreter ()))
+	  && previous_frame
+	  && strcmp (command->command, "stack-select-frame") != 0)
+	{
+	  struct mi_interp *mi = top_level_interpreter_data ();
+          struct frame_info *fi = get_selected_frame (NULL);
+	  int report_change = 0;
+
+	  if (fi->level != previous_frame->level)
+              report_change = ((command->frame == -1)
+                               || (fi->level != command->frame));
+
+	  if (report_change)
+	    {     
+	      target_terminal_ours ();
+	      fprintf_unfiltered (mi->event_channel, 
+				  "frame-selected,id=\"%d\"",
+				  get_selected_frame(NULL)->level);
+	      gdb_flush (mi->event_channel);
+	    }
+	}
+
       mi_parse_free (command);
     }
 

[-- Attachment #3: Type: text/plain, Size: 48 bytes --]

-- 
Happy Hacking.

http://sphinx.net.ru
ã‚€

Re: =frame-selected MI notification

[Tom Tromey]

>>>>> "Dmitry" == Dmitry Dzhus <dima@sphinx.net.ru> writes:

Dmitry> Nick Roberts once posted a patch which implements both =thread-selected
Dmitry> and =frame-selected via observers:
Dmitry> http://sourceware.org/ml/gdb-patches/2008-04/msg00377.html.

I didn't see any discussion of this patch in the archives.
Was it just forgotten?

In general I think that it makes sense to have MI notifications for
state changes caused by CLI commands.

Dmitry> Current implementation of =thread-selected in mi-main.c uses a
Dmitry> different approach, comparing current thread ID before and after
Dmitry> executing a command in `mi_execute_command` function. My patch
Dmitry> does the same for frame level. I don't know how is it
Dmitry> technically better. I think it would be nice if somebody
Dmitry> reviewed Nick's patch once again or at least accepted the one
Dmitry> proposed by me.

I tend to prefer Nick's approach.  I like the use of observers.

I would want to hear Vladimir's opinion as MI maintainer before
approving anything though.

Dmitry> -struct frame_info
Dmitry> -{
Dmitry> -  /* Level of this frame.  The inner-most (youngest) frame is at level

Moving struct frame_info from frame.c to frame.h is definitely not ok.

The point of the current approach is that frame_info is opaque and
cannot be directly manipulated by other modules.  I think this is a good
quality worth preserving.

But ...

Dmitry> +	  if (fi->level != previous_frame->level)

You should probably just get the frame_id for the previous frame, before
the command, and then compare it to the frame_id after the command
(using frame_id_eq).

Tom

Re: =frame-selected MI notification

[Vladimir Prus]

Tom Tromey wrote:

>>>>>> "Dmitry" == Dmitry Dzhus <dima@sphinx.net.ru> writes:
> 
> Dmitry> Nick Roberts once posted a patch which implements both =thread-selected
> Dmitry> and =frame-selected via observers:
> Dmitry> http://sourceware.org/ml/gdb-patches/2008-04/msg00377.html.
> 
> I didn't see any discussion of this patch in the archives.
> Was it just forgotten?
> 
> In general I think that it makes sense to have MI notifications for
> state changes caused by CLI commands.
> 
> Dmitry> Current implementation of =thread-selected in mi-main.c uses a
> Dmitry> different approach, comparing current thread ID before and after
> Dmitry> executing a command in `mi_execute_command` function. My patch
> Dmitry> does the same for frame level. I don't know how is it
> Dmitry> technically better. I think it would be nice if somebody
> Dmitry> reviewed Nick's patch once again or at least accepted the one
> Dmitry> proposed by me.
> 
> I tend to prefer Nick's approach.  I like the use of observers.

I'll review this patch later, but I wanted to clarify this point. The current
codebase emits =thread-selected already, and it does not use observers,
by design. The reason is that MI frontend is not actually interested in
changes in GDB's current thread. The design I am pushing for is that
frontend maintains the current thread and frame (possibly, different for different
main window, even) and uses --thread and --frame options to MI commands to
obtain necessary information in the context of the desired thread.

The only thing frontend might want to know from GDB is that a CLI command
has explicitly asked to change thread or frame -- in which case frontend
might want to switch its own current thread or frame. To reiterate,
frontend does not care about thread or frame switches that GDB might
do internally. 

It was decided that the easiest way to detect such explicit request
from the user is to note that thread has changed after an MI command
was executed. It seems reasonable to apply the same logic to frame.

Furthermore, I do not think that observers for either thread or frame
changes is good idea, design wise. The fact that gdb has current thread
or current frame at the code level, is, IMNSHO, a design bug. Global
variables are generally bad, and those two are certainly not exception.
It's better not to make situation worse by adding new mechanism based
on that design bug.

- Volodya

Re: =frame-selected MI notification

[Nick Roberts]

 > I'll review this patch later, but I wanted to clarify this point. The current
 > codebase emits =thread-selected already, and it does not use observers,
 > by design. The reason is that MI frontend is not actually interested in
 > changes in GDB's current thread. 

I think Emacs, and other frontends that have a console, will always be
interested in the current thread/selected frame.  The behaviour of CLI
commands like up, down, print etc depend on it.

Of course, if GDB emits a notification then the frontend can always choose to
ignore it.  If GDB doesn't emit a notification then the frontend has to send
an extra command to get the information it needs after after every user
command.

It's not only thread and frame changes that could be emitted as notifications.
Setting, deleting, disabling breakpoints with CLI commands could also emit
notifications.  This would save having to send -break-list after every user
command.

 > ...
 > Furthermore, I do not think that observers for either thread or frame
 > changes is good idea, design wise. The fact that gdb has current thread
 > or current frame at the code level, is, IMNSHO, a design bug. Global
 > variables are generally bad, and those two are certainly not exception.
 > It's better not to make situation worse by adding new mechanism based
 > on that design bug.

I think it would be hard to write a large C program with no global variables
but, in any case, the concept of current thread/selected frame seems to be a
great convenience when using GDB from the command line.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: =frame-selected MI notification

[Vladimir Prus]

On Saturday 29 August 2009 Nick Roberts wrote:

>  > I'll review this patch later, but I wanted to clarify this point. The current
>  > codebase emits =thread-selected already, and it does not use observers,
>  > by design. The reason is that MI frontend is not actually interested in
>  > changes in GDB's current thread. 
> 
> I think Emacs, and other frontends that have a console, will always be
> interested in the current thread/selected frame.  The behaviour of CLI
> commands like up, down, print etc depend on it.

It seems to me that if frontend has thread 3, frame 2 presently selected
in UI, then "print" in CLI should print the value of expression in thread
3, frame 3. To that end, frontend should emit the following command:

	-interpreter-exec --thread 3 --frame 3 console "print whatever"

It does not seem like frontend need to know the value of the 'inferior_ptid' variable
in GDB source code to do that?

As I understand from talks with Dmitry on IRC, emacs already uses --thread and
--frame options as appropriate, so everything should just work. And, I also
assume that Emacs works just fine with the patch he posted.

> Of course, if GDB emits a notification then the frontend can always choose to
> ignore it.

How? GDB does not say if notification is emitted as result of internal thread
switch, or not, so frontend would need to consult an oracle to ignore notification.

> If GDB doesn't emit a notification then the frontend has to send
> an extra command to get the information it needs after after every user
> command.
> 
> It's not only thread and frame changes that could be emitted as notifications.
> Setting, deleting, disabling breakpoints with CLI commands could also emit
> notifications.  This would save having to send -break-list after every user
> command.

I think breakpoints are very different matter. Breakpoint table is an inherent
global property of debug session. You cannot sensibly have different breakpoint
tables in different IDE windows, or use different breakpoint tables for
different -exec-continue commands. Therefore, notifying a frontend about any
change to breakpoint table is uncontroversial change. We even have an
observer already -- breakpoints_changed -- that can be hooked to MI.

>  > Furthermore, I do not think that observers for either thread or frame
>  > changes is good idea, design wise. The fact that gdb has current thread
>  > or current frame at the code level, is, IMNSHO, a design bug. Global
>  > variables are generally bad, and those two are certainly not exception.
>  > It's better not to make situation worse by adding new mechanism based
>  > on that design bug.
> 
> I think it would be hard to write a large C program with no global variables
> but, in any case, the concept of current thread/selected frame seems to be a
> great convenience when using GDB from the command line.

It is convenient for command line GDB usage. This, however, does not mean that
global variable that affects all of gdb makes sense.

- Volodya

Re: =frame-selected MI notification

[Tom Tromey]

Nick> I think it would be hard to write a large C program with no global
Nick> variables but, in any case, the concept of current thread/selected
Nick> frame seems to be a great convenience when using GDB from the
Nick> command line.

Volodya> It is convenient for command line GDB usage. This, however,
Volodya> does not mean that global variable that affects all of gdb
Volodya> makes sense.

As long as Emacs and GDB can be in sync, I suppose I don't care much
whether the notification is done by the MI command wrapper or deep in
the guts of GDB.

So, I think we either need Dmitry's patch or Nick's.

I assume we'll find more little holes like this, too.

Tom

Re: =frame-selected MI notification

[Tom Tromey]

>>>>> "Volodya" == Vladimir Prus <vladimir@codesourcery.com> writes:

Volodya> Furthermore, I do not think that observers for either thread or
Volodya> frame changes is good idea, design wise. The fact that gdb has
Volodya> current thread or current frame at the code level, is, IMNSHO,
Volodya> a design bug.

I agree, for the core.  But, the problem in question concerns
notification about the changes to the user's view.

E.g., it wouldn't be unreasonable to want to write a Python script that
reacts to changes in the CLI's notion of frame.  In this case, such an
observer would be useful and appropriate.

Tom

Re: =frame-selected MI notification

[Vladimir Prus]

On Tuesday 01 September 2009 Tom Tromey wrote:

> >>>>> "Volodya" == Vladimir Prus <vladimir@codesourcery.com> writes:
> 
> Volodya> Furthermore, I do not think that observers for either thread or
> Volodya> frame changes is good idea, design wise. The fact that gdb has
> Volodya> current thread or current frame at the code level, is, IMNSHO,
> Volodya> a design bug.
> 
> I agree, for the core.  But, the problem in question concerns
> notification about the changes to the user's view.
> 
> E.g., it wouldn't be unreasonable to want to write a Python script that
> reacts to changes in the CLI's notion of frame.  In this case, such an
> observer would be useful and appropriate.

Why would you want this? To implement some 'plugin' to CLI interface?
Then, since current_ptid is essentially "CLI current thread", such a 
plugin naturally need observer to notice changes of current_ptid.
However, this does not say MI should use the same observer ;-)

- Volodya

Re: =frame-selected MI notification

[Dmitry Dzhus]

Vladimir Prus wrote:

> As I understand from talks with Dmitry on IRC, emacs already uses
> --thread and --frame options as appropriate

Yes.

> And, I also assume that Emacs works just fine with the patch he
> posted.

Not at the moment, but with =frame-selected notification being really
implemented it will be easy to write code which will let Emacs now about
user's intent to switch «current frame» from console.
-- 
Happy Hacking.

http://sphinx.net.ru
ã‚€

Re: =frame-selected MI notification

[Dmitry Dzhus]

Tom Tromey wrote:

> You should probably just get the frame_id for the previous frame, before
> the command, and then compare it to the frame_id after the command
> (using frame_id_eq).

Thank you for your comments, I'll send the updated patch.
-- 
Happy Hacking.

http://sphinx.net.ru
ã‚€

Re: =frame-selected MI notification

[Tom Tromey]

>>>>> "Volodya" == Vladimir Prus <vladimir@codesourcery.com> writes:

Tom> E.g., it wouldn't be unreasonable to want to write a Python script that
Tom> reacts to changes in the CLI's notion of frame.  In this case, such an
Tom> observer would be useful and appropriate.

Volodya> Why would you want this? To implement some 'plugin' to CLI interface?

Yeah.

Tom