Re: How to Read Program Architecture from GDB/MI?

[Andrew Burgess <andrew dot burgess at embecosm dot com>]

* Paramjot Oberoi <paramjot@gmail.com> [2016-07-26 14:36:35 -0400]:

> Andrew,
> 
> Thank you for your patch, it's a huge improvement over the hack I was
> considering. For the time being I will use "interpreter-exec mi "show
> architecture."", and hook the console output. This has some drawbacks,
> but it will save me from having to distribute a custom GDB patch if I
> want to distribute my code.
> 
> I'm still familiarizing myself with the code base, but long term I'm
> divided on whether it makes sense to use tuples here for the output.
> Wouldn't that break existing compatibility with all GDB/MI frontends
> that use -gdb-show?
> 
> "show architecture" currently returns two pieces of information,
> whether the architecture is set to "auto" ("target architecture is set
> automatically") and what the currently executing architecture is
> ("currently i386"). Let's simply add another show command that always
> returns the currently executing architecture. "show
> current-architecture". Now you can query if the architecture is set to
> auto (not particularly useful to me) or if you don't care, you can
> simply query the currently executing architecture directly.

I don't think that's a great idea.  The problem is you're focusing too
much on the architecture example that you've hit, when in reality this
problem extends to all -gdb-show VAR cases (or could do).

Switching to tuples could be a problem, but given the current pretty
poor state of the MI show support I think that a case could be made to
fix this properly once, causing a little pain now, but resulting in a
much better situation moving forward.

If we took your suggestion, adding a new variable
'current-architecture' then things start to get pretty messy.  I guess
you're thinking that this variable would never be set.  Would it make
sense to be able to view this variable from the CLI?  I guess it
doesn't hurt, but then you have the confusion of having the same
information (apparently) stored in two variables....

Not to mention that that are lots of variables that follow the auto
model, so you'd want to add current-* versions for all of these too.

If backwards compatibility was a real issue then we could make the MI
return the current non-tuple style answer by default, and have a
config switch that changes the output to the tuple style fuller
answer.  Clients that expect or want this behaviour can then select it
... or maybe we just add a new MI command -gdb-show-full VAR that
returns the tuple style output.

In summary I still think tuples are the way to go, there are ways to
solve the backwards compatibility issues, and adding a new variable
would not, I think, scale well over all of the "auto" variables.

Hope that helps,

Thanks,
Andrew





> 
> -gdb-show architecture -> maps directly to the set architecture
> command. Returns null if the architecture hasn't been set (which means
> it's auto), "auto" if the user set it to auto, or the architecture
> type that the user set.
> -gdb-show current-architecture -> always returns the current executing
> architecture.
> 
> Let me know what your thoughts are. Thanks in advance.
> 
> 
> On Tue, Jul 26, 2016 at 4:09 AM, Andrew Burgess
> <andrew.burgess@embecosm.com> wrote:
> > * Paramjot Oberoi <paramjot@gmail.com> [2016-07-26 02:27:31 -0400]:
> >
> >> Thank you for the quick response, it got me down the right track.
> >> Unfortunately simply commenting out that if() won't work for me as the
> >> output will no longer have the GDB/MI request IDs. It would be the
> >> same as if I did "interpreter-exec mi "show architecture." I believe
> >> the correct fix will require some proper planning. I've spent a few
> >> hours looking at GDB's source code and I can't think of a clean way to
> >> implement it.
> >>
> >> The core issue is the output from "show architecture" and
> >> "interpreter-exec mi "-gdb-show architecture"" do not match in the
> >> case that the architecture is not set or is set to auto. If you
> >> manually set the architecture ("set architecture i386") the GDB/MI
> >> output is correct. As you mentioned the c->show_value_func
> >> (show_architecture) only gets called for the console output case, and
> >> not for the GDB/MI. It is the source of the difference.
> >>
> >> Console output:
> >> (gdb) show architecture
> >> The target architecture is set automatically (currently i386)  --->
> >> set_architecture_string is NULL, but output correctly says the target
> >> architecture is auto, and prints out the current architecture
> >> (gdb) set architecture auto
> >> The target architecture is set automatically (currently i386)
> >> (gdb) show architecture
> >> The target architecture is set automatically (currently i386) --->
> >> set_architecture_string is "auto", and prints out the current
> >> architecture
> >>
> >> GDB/MI output:
> >> (gdb) interpreter-exec mi "-gdb-show architecture"
> >> ^done ---> set_architecture_string is NULL so nothing is output, does
> >> not print out the current architecture
> >> (gdb) set architecture auto
> >> The target architecture is set automatically (currently i386)
> >> (gdb) interpreter-exec mi "-gdb-show architecture"
> >> ^done,value="auto" ---> set_architecture_string is "auto", does not
> >> print out the current architecture
> >>
> >> c->show_value_func (show_architecture) is what handles the special
> >> logic for having "auto" or uninitialized architectures:
> >>
> >> static void
> >> show_architecture (struct ui_file *file, int from_tty,
> >>    struct cmd_list_element *c, const char *value)
> >> {
> >>   if (target_architecture_user == NULL)
> >>     fprintf_filtered (file, _("The target architecture is set "
> >>       "automatically (currently %s)\n"),
> >>       gdbarch_bfd_arch_info (get_current_arch ())->printable_name);
> >>    else
> >>     fprintf_filtered (file, _("The target architecture is assumed to be %s\n"),
> >>       set_architecture_string);
> >> }
> >>
> >> There is no such equivalent callback for the GDB/MI case. I can't
> >> think of a way to do this that wouldn't be hackish. One thought was to
> >> modify the if() else to specifically look for this case:
> >>
> >> if (ui_out_is_mi_like_p (uiout))
> >> {
> >> if(c->show_value_func == show_architecture)
> >> {
> >> // reimplement the logic of show_architecture() here, but for MI
> >> // we would need to wipe the existing stb because it might already
> >> have the word "auto" in there
> >>
> >> }
> >> ui_out_field_stream (uiout, "value", stb);
> >> }
> >> else
> >> {
> >> ...
> >> ...
> >> }
> >
> > As you point out I don't think there's a quick fix to your problem,
> > and the comment in 'do_show_command' acknowledges that this area is
> > broken when it comes to MI.
> >
> > As a quick fix how about the patch below.  It's not ideal, but it
> > might be enough for you.
> >
> > The basic idea is to wrap the MI return from -gdb-show into a tuple,
> > then add an extra field 'message', which contains the raw output of a
> > CLI 'show' command.
> >
> > [ I think that long term we'd probably switch to a tuple anyway, when
> >   we correctly handle things like a variable being 'auto' we'd
> >   probably want a reply that looked something like: {value="auto",
> >   current="i386"}, so switching to a tuple is probably the way to
> >   go. ]
> >
> > For now however, the display of value is not fixed, so the 'auto'
> > value does not get displayed at all, but you do always get the message
> > string, the default reply now looks like this:
> >
> >     (gdb) interpreter-exec mi "-gdb-show architecture"
> >     ^done,{message="The target architecture is set automatically (currently i386)\n"}
> >
> > You would then have to parse the message string yourself.
> >
> > Hope this helps,
> > Andrew
> >
> > ----
> >
> > diff --git a/gdb/cli/cli-setshow.c b/gdb/cli/cli-setshow.c
> > index eb17158..f7597cf 100644
> > --- a/gdb/cli/cli-setshow.c
> > +++ b/gdb/cli/cli-setshow.c
> > @@ -650,7 +650,23 @@ do_show_command (const char *arg, int from_tty, struct cmd_list_element *c)
> >       MI and CLI specific versions.  */
> >
> >    if (ui_out_is_mi_like_p (uiout))
> > -    ui_out_field_stream (uiout, "value", stb);
> > +    {
> > +      struct ui_file *msg_file;
> > +      char *value;
> > +
> > +      make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
> > +      ui_out_field_stream (uiout, "value", stb);
> > +
> > +      value = ui_file_xstrdup (stb, NULL);
> > +      make_cleanup (xfree, value);
> > +      msg_file = mem_fileopen ();
> > +      if (c->show_value_func != NULL)
> > +       c->show_value_func (msg_file, from_tty, c, value);
> > +      else
> > +       deprecated_show_value_hack (msg_file, from_tty, c, value);
> > +
> > +      ui_out_field_stream (uiout, "message", msg_file);
> > +    }
> >    else
> >      {
> >        char *value = ui_file_xstrdup (stb, NULL);