Fix behaviour of 'show' commands in hook functions in MI mode

  Hi,

This is my first patch, so apologies in advance for anything that I 
haven't done quite right.

In console mode, if I add a 'hook-run' which executes (for example) 
'show inferior-tty', the result is printed before the run, and if I add 
a 'hookpost-run' with the same command, the result is printed after the 
run.  This behaviour matches the documentation (and my expectations).

In MI mode, 'show' commands in post-execution hooks seem to be ignored, 
and the result of a 'show' in a pre-execution hook is printed AFTER the 
hooked command has terminated, for example in an exec-async-output 
record as follows:

*stopped,value="<result of show command>",reason="exited-normally"

The following patch modifies the behaviour of a 'show' command executed 
in a hook function in MI mode, so that it will print the result using 
the console-mode behaviour, wrapped up as an MI console-stream-output 
record.

I hope this is reasonable -- please get in touch if it warrants discussion.

To reproduce:

echo "int main() { }" | gcc -x c -
echo -e "set inferior-tty /dev/null\ndefine hook-run\nshow \
inferior-tty\nend\nrun\nquit" > x
gdb -i=mi -x x ./a.out

Patch:

        * cli/cli-setshow.c: Print the results of 'show' commands in hook
        functions in MI mode using the console-mode behaviour.

      {
  

Ping.

On 14/11/14 10:40, Thomas Perry wrote:
> Hi,
>
> This is my first patch, so apologies in advance for anything that I
> haven't done quite right.
>
> In console mode, if I add a 'hook-run' which executes (for example)
> 'show inferior-tty', the result is printed before the run, and if I add
> a 'hookpost-run' with the same command, the result is printed after the
> run.  This behaviour matches the documentation (and my expectations).
>
> In MI mode, 'show' commands in post-execution hooks seem to be ignored,
> and the result of a 'show' in a pre-execution hook is printed AFTER the
> hooked command has terminated, for example in an exec-async-output
> record as follows:
>
> *stopped,value="<result of show command>",reason="exited-normally"
>
> The following patch modifies the behaviour of a 'show' command executed
> in a hook function in MI mode, so that it will print the result using
> the console-mode behaviour, wrapped up as an MI console-stream-output
> record.
>
> I hope this is reasonable -- please get in touch if it warrants discussion.
>
> To reproduce:
>
> echo "int main() { }" | gcc -x c -
> echo -e "set inferior-tty /dev/null\ndefine hook-run\nshow \
> inferior-tty\nend\nrun\nquit" > x
> gdb -i=mi -x x ./a.out
>
> Patch:
>
>         * cli/cli-setshow.c: Print the results of 'show' commands in hook
>         functions in MI mode using the console-mode behaviour.
>
> diff --git a/gdb/cli/cli-setshow.c b/gdb/cli/cli-setshow.c
> index a7d0728..1659fca 100644
> --- a/gdb/cli/cli-setshow.c
> +++ b/gdb/cli/cli-setshow.c
> @@ -21,6 +21,7 @@
>   #include <ctype.h>
>   #include "arch-utils.h"
>   #include "observer.h"
> +#include "top.h"
>
>   #include "ui-out.h"
>
> @@ -649,7 +650,7 @@ do_show_command (const char *arg, int from_tty,
> struct cmd_list_element *c)
>        code to print the value out.  For the latter there should be
>        MI and CLI specific versions.  */
>
> -  if (ui_out_is_mi_like_p (uiout))
> +  if (ui_out_is_mi_like_p (uiout) && !in_user_command)
>       ui_out_field_stream (uiout, "value", stb);
>     else
>       {
  

Ping again.  It would be great if someone could have a brief look over this.

On 01/12/14 13:31, Thomas Perry wrote:
> Ping.
>
> On 14/11/14 10:40, Thomas Perry wrote:
>> Hi,
>>
>> This is my first patch, so apologies in advance for anything that I
>> haven't done quite right.
>>
>> In console mode, if I add a 'hook-run' which executes (for example)
>> 'show inferior-tty', the result is printed before the run, and if I add
>> a 'hookpost-run' with the same command, the result is printed after the
>> run.  This behaviour matches the documentation (and my expectations).
>>
>> In MI mode, 'show' commands in post-execution hooks seem to be ignored,
>> and the result of a 'show' in a pre-execution hook is printed AFTER the
>> hooked command has terminated, for example in an exec-async-output
>> record as follows:
>>
>> *stopped,value="<result of show command>",reason="exited-normally"
>>
>> The following patch modifies the behaviour of a 'show' command executed
>> in a hook function in MI mode, so that it will print the result using
>> the console-mode behaviour, wrapped up as an MI console-stream-output
>> record.
>>
>> I hope this is reasonable -- please get in touch if it warrants
>> discussion.
>>
>> To reproduce:
>>
>> echo "int main() { }" | gcc -x c -
>> echo -e "set inferior-tty /dev/null\ndefine hook-run\nshow \
>> inferior-tty\nend\nrun\nquit" > x
>> gdb -i=mi -x x ./a.out
>>
>> Patch:
>>
>>         * cli/cli-setshow.c: Print the results of 'show' commands in hook
>>         functions in MI mode using the console-mode behaviour.
>>
>> diff --git a/gdb/cli/cli-setshow.c b/gdb/cli/cli-setshow.c
>> index a7d0728..1659fca 100644
>> --- a/gdb/cli/cli-setshow.c
>> +++ b/gdb/cli/cli-setshow.c
>> @@ -21,6 +21,7 @@
>>   #include <ctype.h>
>>   #include "arch-utils.h"
>>   #include "observer.h"
>> +#include "top.h"
>>
>>   #include "ui-out.h"
>>
>> @@ -649,7 +650,7 @@ do_show_command (const char *arg, int from_tty,
>> struct cmd_list_element *c)
>>        code to print the value out.  For the latter there should be
>>        MI and CLI specific versions.  */
>>
>> -  if (ui_out_is_mi_like_p (uiout))
>> +  if (ui_out_is_mi_like_p (uiout) && !in_user_command)
>>       ui_out_field_stream (uiout, "value", stb);
>>     else
>>       {
>
  

Thomas,

I apologize for the late review. And thank you for pinging us.

> This is my first patch, so apologies in advance for anything that I
> haven't done quite right.

Quite good, actually, for a first patch :-).

> In console mode, if I add a 'hook-run' which executes (for example)
> 'show inferior-tty', the result is printed before the run, and if I
> add a 'hookpost-run' with the same command, the result is printed
> after the run.  This behaviour matches the documentation (and my
> expectations).
> 
> In MI mode, 'show' commands in post-execution hooks seem to be
> ignored, and the result of a 'show' in a pre-execution hook is
> printed AFTER the hooked command has terminated, for example in an
> exec-async-output record as follows:
> 
> *stopped,value="<result of show command>",reason="exited-normally"
> 
> The following patch modifies the behaviour of a 'show' command
> executed in a hook function in MI mode, so that it will print the
> result using the console-mode behaviour, wrapped up as an MI
> console-stream-output record.
> 
> I hope this is reasonable -- please get in touch if it warrants discussion.
> 
> To reproduce:
> 
> echo "int main() { }" | gcc -x c -
> echo -e "set inferior-tty /dev/null\ndefine hook-run\nshow \
> inferior-tty\nend\nrun\nquit" > x
> gdb -i=mi -x x ./a.out
> 
> Patch:

Replace the above by the name of the ChangeLog you're changing. Eg:

| gdb/ChangeLog:

> 
>        * cli/cli-setshow.c: Print the results of 'show' commands in hook
>        functions in MI mode using the console-mode behaviour.

The patch looks reasonable to me.

Can you create a testcase for this? I don't have much experience with
writing GDB/MI testcases, but that should be fairly straightforward
to do, and we'll need that in order to avoid regressing.

Also, have you exercised your patch against the testsuite? If you did,
then it is important to explicitly say so and include the platform on
which it was done.
  

On 11/14/2014 10:40 AM, Thomas Perry wrote:

> The following patch modifies the behaviour of a 'show' command executed 
> in a hook function in MI mode, so that it will print the result using 
> the console-mode behaviour, wrapped up as an MI console-stream-output 
> record.
> 
> I hope this is reasonable -- please get in touch if it warrants discussion.

Sorry, I don't think special-casing "show" is the right solution.

See e.g.:

 $ echo -e "define hook-run\ninfo threads\nend\nrun\nquit" > x
 $ ./gdb -i=mi -x x /usr/bin/true
 ...
 *stopped,threads=[],reason="exited-normally"
         ^^^^^^^^^^

vs:

 $ echo -e "define hook-run\nend\nrun\nquit" > x
 *stopped,reason="exited-normally"

But simpler, even without a hook:

 $ echo -e "info threads" > x
 $ gdb -q -i=mi -x x /usr/bin/true
 ...
 (gdb)
 p 1
 &"p 1\n"
 ~"$1 = 1"
 ~"\n"
 ^done,threads=[]
      ^^^^^^^^^^^
 (gdb)
 p 1
 &"p 1\n"
 ~"$2 = 1"
 ~"\n"
 ^done    // correct now
 (gdb)


Or even without a command file:

 $ gdb -q -i=mi -ex "set inferior-tty /dev/null" -ex "show inferior-tty" /usr/bin/true
 =thread-group-added,id="i1"
...
 =cmd-param-changed,param="inferior-tty",value="/dev/null"
 (gdb)
 p 1
 &"p 1\n"
 ~"$1 = 1"
 ~"\n"
 ^done,value="/dev/null"
       ^^^^^^^^^^^^^^^^^
 (gdb)

Thanks,
Pedro Alves
  

On 15/12/14 13:58, Pedro Alves wrote:
> On 11/14/2014 10:40 AM, Thomas Perry wrote:
>
>> The following patch modifies the behaviour of a 'show' command executed
>> in a hook function in MI mode, so that it will print the result using
>> the console-mode behaviour, wrapped up as an MI console-stream-output
>> record.
>>
>> I hope this is reasonable -- please get in touch if it warrants discussion.
>
> Sorry, I don't think special-casing "show" is the right solution.
>
> See e.g.:
>
>   $ echo -e "define hook-run\ninfo threads\nend\nrun\nquit" > x
>   $ ./gdb -i=mi -x x /usr/bin/true
>   ...
>   *stopped,threads=[],reason="exited-normally"
>           ^^^^^^^^^^
>
> vs:
>
>   $ echo -e "define hook-run\nend\nrun\nquit" > x
>   *stopped,reason="exited-normally"
>
> But simpler, even without a hook:
>
>   $ echo -e "info threads" > x
>   $ gdb -q -i=mi -x x /usr/bin/true
>   ...
>   (gdb)
>   p 1
>   &"p 1\n"
>   ~"$1 = 1"
>   ~"\n"
>   ^done,threads=[]
>        ^^^^^^^^^^^
>   (gdb)
>   p 1
>   &"p 1\n"
>   ~"$2 = 1"
>   ~"\n"
>   ^done    // correct now
>   (gdb)
>
>
> Or even without a command file:
>
>   $ gdb -q -i=mi -ex "set inferior-tty /dev/null" -ex "show inferior-tty" /usr/bin/true
>   =thread-group-added,id="i1"
> ...
>   =cmd-param-changed,param="inferior-tty",value="/dev/null"
>   (gdb)
>   p 1
>   &"p 1\n"
>   ~"$1 = 1"
>   ~"\n"
>   ^done,value="/dev/null"
>         ^^^^^^^^^^^^^^^^^
>   (gdb)
>
> Thanks,
> Pedro Alves

Hi Pedro (and Joel),

Thanks very much for reviewing the patch.  I agree with you that there 
are other aspects of the MI behaviour that don't seem to work as we 
might expect.  However, would the patch be acceptable on the grounds 
that it improves the current behaviour (subject to passing tests), even 
if its scope is limited to "show" commands?

One alternative approach would be for GDB/MI to output a distinct 
result-record for commands like "show" and "info threads" rather than 
adding their output to records for other commands, but this will be a 
more difficult (and risky) change.  What is your opinion on this?  Is 
this likely to be an acceptable modification to the current behaviour?

Thanks,
Tom Perry
  

On 12/18/2014 11:00 AM, Thomas Perry wrote:
> On 15/12/14 13:58, Pedro Alves wrote:
>> On 11/14/2014 10:40 AM, Thomas Perry wrote:
>>
>>> The following patch modifies the behaviour of a 'show' command executed
>>> in a hook function in MI mode, so that it will print the result using
>>> the console-mode behaviour, wrapped up as an MI console-stream-output
>>> record.
>>>
>>> I hope this is reasonable -- please get in touch if it warrants discussion.
>>
>> Sorry, I don't think special-casing "show" is the right solution.
>>
>> See e.g.:
>>
>>   $ echo -e "define hook-run\ninfo threads\nend\nrun\nquit" > x
>>   $ ./gdb -i=mi -x x /usr/bin/true
>>   ...
>>   *stopped,threads=[],reason="exited-normally"
>>           ^^^^^^^^^^
>>
>> vs:
>>
>>   $ echo -e "define hook-run\nend\nrun\nquit" > x
>>   *stopped,reason="exited-normally"
>>
>> But simpler, even without a hook:
>>
>>   $ echo -e "info threads" > x
>>   $ gdb -q -i=mi -x x /usr/bin/true
>>   ...
>>   (gdb)
>>   p 1
>>   &"p 1\n"
>>   ~"$1 = 1"
>>   ~"\n"
>>   ^done,threads=[]
>>        ^^^^^^^^^^^
>>   (gdb)
>>   p 1
>>   &"p 1\n"
>>   ~"$2 = 1"
>>   ~"\n"
>>   ^done    // correct now
>>   (gdb)
>>
>>
>> Or even without a command file:
>>
>>   $ gdb -q -i=mi -ex "set inferior-tty /dev/null" -ex "show inferior-tty" /usr/bin/true
>>   =thread-group-added,id="i1"
>> ...
>>   =cmd-param-changed,param="inferior-tty",value="/dev/null"
>>   (gdb)
>>   p 1
>>   &"p 1\n"
>>   ~"$1 = 1"
>>   ~"\n"
>>   ^done,value="/dev/null"
>>         ^^^^^^^^^^^^^^^^^
>>   (gdb)
>>
>> Thanks,
>> Pedro Alves
> 
> Hi Pedro (and Joel),
> 
> Thanks very much for reviewing the patch.  I agree with you that there 
> are other aspects of the MI behaviour that don't seem to work as we 
> might expect.  

These aren't other aspects, but rather more manifestations of the
same issue.

> However, would the patch be acceptable on the grounds 
> that it improves the current behaviour (subject to passing tests), even 
> if its scope is limited to "show" commands?

I think the root cause should be identified instead.

> One alternative approach would be for GDB/MI to output a distinct 
> result-record for commands like "show" and "info threads" rather than 
> adding their output to records for other commands, but this will be a 
> more difficult (and risky) change.  What is your opinion on this?  Is 
> this likely to be an acceptable modification to the current behaviour?

No, not acceptable, sorry.  "info threads" is not special either.  It was
just an example.  The issue will manifest with any command that
uses ui_out for structured output.  Another example:

$ gdb -q -i=mi -ex "info break" /usr/bin/true
(gdb)
p 1
&"p 1\n"
~"$1 = 1"
~"\n"
^done,BreakpointTable={nr_rows="0",nr_cols="6",hdr=[{width="7",alignment="-1",col_name="number",colhdr="Num"},{width="14",alignment="-1",col_name="type",colhdr="Type"},{width="4",alignment="-1",col_name="disp",colhdr="Disp"},{width="3",alignment="-1",col_name="enabled",colhdr="Enb"},{width="10",alignment="-1",col_name="addr",colhdr="Address"},{width="40",alignment="2",col_name="what",colhdr="What"}],body=[]}
(gdb)

I'd suspect that MI's cli uiout isn't being properly
wired up in these cases.

Thanks,
Pedro Alves