Bug 627506 - python: gdb.execute([...], to_string=True) partly prints to stdout/stderr
python: gdb.execute([...], to_string=True) partly prints to stdout/stderr
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: gdb (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Jan Kratochvil
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2010-08-26 05:04 EDT by Paul Bolle
Modified: 2010-09-14 00:59 EDT (History)
2 users (show)

See Also:
Fixed In Version: gdb-7.2-4.fc14
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-09-14 00:59:13 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
make uiout do what gdb.execute([...]), to_string=True) expects (775 bytes, patch)
2010-08-26 15:37 EDT, Paul Bolle
no flags Details | Diff
updated patch, taken into account comments from comment #8 (6.64 KB, patch)
2010-09-01 07:34 EDT, Paul Bolle
no flags Details | Diff
include _all_ needed changes (7.71 KB, patch)
2010-09-01 07:41 EDT, Paul Bolle
no flags Details | Diff

  None (edit)
Description Paul Bolle 2010-08-26 05:04:04 EDT
Description of problem:
gdb.execute([...], to_string=True) should not print to stdout/stderr. It does print something, somehow to stdout/stderr.

Version-Release number of selected component (if applicable):
gdb-7.1.90.20100806-10.fc14.i686

How reproducible:
Always.

Steps to Reproduce:
(gdb) python
>help_out = gdb.execute("help", False, to_string=True)
>print "help_out:"
>print help_out
>end
  
Actual results:
Aliases of other commandsMaking program stop at certain pointsExamining dataSpecifying and examining filesMaintenance commandsObscure featuresRunning the programExamining the stackStatus inquiriesSupport facilitiesTracing of program execution without stopping the programUser-defined commandshelp_out:
List of classes of commands:

aliases -- 
breakpoints -- 
data -- 
files -- 
internals -- 
obscure -- 
running -- 
stack -- 
status -- 
support -- 
tracepoints -- 
user-defined -- 

Type "help" followed by a class name for a list of commands in that class.
Type "help all" for the list of all commands.
Type "help" followed by command name for full documentation.
Type "apropos word" to search for commands related to "word".
Command name abbreviations are allowed if unambiguous.

(gdb)

Expected results:
help_out:
List of classes of commands:

aliases -- Aliases of other commands
breakpoints -- Making program stop at certain points
data -- Examining data
files -- Specifying and examining files
internals -- Maintenance commands
obscure -- Obscure features
running -- Running the program
stack -- Examining the stack
status -- Status inquiries
support -- Support facilities
tracepoints -- Tracing of program execution without stopping the program
user-defined -- User-defined commands

Type "help" followed by a class name for a list of commands in that class.
Type "help all" for the list of all commands.
Type "help" followed by command name for full documentation.
Type "apropos word" to search for commands related to "word".
Command name abbreviations are allowed if unambiguous.

(gdb)

Additional info:
Note how only a part of the output (everything following " -- " in the output) is not printed "to_string" bot to stdout/stderr. 

I have no idea what's going on yet, so I can't suggest a patch.
Comment 1 Paul Bolle 2010-08-26 12:27:12 EDT
0) For what it's worth, it seems gdb/cli/cli-decode.c:print_help_for_command() is relevant here.

static void
print_help_for_command (struct cmd_list_element *c, char *prefix, int recurse,
			struct ui_file *stream)
{
  fprintf_filtered (stream, "%s%s -- ", prefix, c->name);
  print_doc_line (stream, c->doc);
  fputs_filtered ("\n", stream);
  
  [...];
}

Note that the lines still printed by gdb here, and not sent "to_string", are generated with print_doc_line().

1) gdb/cli/cli-decode.c:print_doc_line() doesn't actually use the stream argument and apparently prints using the (new for me) ui_out stuff:
    ui_out_text (uiout, line_buffer);

My first guess is the ui_out stuff ignores the python command tricks with stdout/stderr. Sadly, the ui_out stuff is opaque to me, so I couldn't pinpoint this further.
Comment 2 Jan Kratochvil 2010-08-26 12:57:32 EDT
uiout should be also redirected.  Why it isn't is a question/bug.
Comment 3 Paul Bolle 2010-08-26 15:37:44 EDT
Created attachment 441315 [details]
make uiout do what gdb.execute([...]), to_string=True) expects

This patch is in the "hit the engine until it starts running" category, but it seems to do the right thing. Let's just call it a RFC.
Comment 4 Jan Kratochvil 2010-08-26 15:45:54 EDT
There is rather ui_out_redirect(), see handle_redirections().
Comment 5 Paul Bolle 2010-08-26 16:20:56 EDT
(In reply to comment #4)
> There is rather ui_out_redirect(), see handle_redirections().

That was a bit terse. I didn't understand the reference to handle_redirections(). Anyhow, this seems to work too (please note that it's very lightly tested). Is this what was hinted at?

diff --git a/gdb/top.c b/gdb/top.c
index b29e68d..038c916 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -481,11 +481,13 @@ execute_command_to_string (char *p, int from_tty)
 
   gdb_stdout = str_file;
   gdb_stderr = str_file;
+  ui_out_redirect (uiout, str_file);
 
   execute_command (p, from_tty);
 
   retval = ui_file_xstrdup (str_file, NULL);
 
+  ui_out_redirect (uiout, NULL);
   do_cleanups (cleanup);
 
   return retval;
Comment 6 Jan Kratochvil 2010-08-26 16:29:19 EDT
(In reply to comment #5)
> Is this what was hinted at?

Generally yes.

> @@ -481,11 +481,13 @@ execute_command_to_string (char *p, int from_tty)
> 
>    gdb_stdout = str_file;
>    gdb_stderr = str_file;
> +  ui_out_redirect (uiout, str_file);
> 
>    execute_command (p, from_tty);
> 
>    retval = ui_file_xstrdup (str_file, NULL);
> 
> +  ui_out_redirect (uiout, NULL);
>    do_cleanups (cleanup);

There is a problem.  If execute_command() throws an exception uiout remains incorrectly redirected.  There should be registered a cleanup function that gets executed (either during an exception or during do_cleanups (cleanup);).

These exceptions are specific to GDB (not C++) which is a bit inconvenient.
Comment 7 Paul Bolle 2010-08-27 07:47:25 EDT
(In reply to comment #6)
> There is a problem.  If execute_command() throws an exception uiout remains
> incorrectly redirected.  There should be registered a cleanup function that
> gets executed (either during an exception or during do_cleanups (cleanup);).

You mean something like this:

diff --git a/gdb/top.c b/gdb/top.c
index b29e68d..d94c662 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -458,6 +458,12 @@ execute_command (char *p, int from_tty)
     }
 }
 
+static void
+do_ui_out_redirect (void *dummy)
+{
+  ui_out_redirect (uiout, NULL);
+};
+
 /* Run execute_command for P and FROM_TTY.  Capture its output into the
    returned string, do not display it to the screen.  BATCH_FLAG will be
    temporarily set to true.  */
@@ -477,10 +483,14 @@ execute_command_to_string (char *p, int from_tty)
 
   make_cleanup_restore_ui_file (&gdb_stdout);
   make_cleanup_restore_ui_file (&gdb_stderr);
+  make_cleanup_restore_ui_file (&gdb_stdtarg);
   make_cleanup_ui_file_delete (str_file);
 
   gdb_stdout = str_file;
   gdb_stderr = str_file;
+  gdb_stdtarg = str_file;
+  ui_out_redirect (uiout, str_file);
+  make_cleanup (do_ui_out_redirect, 0);
 
   execute_command (p, from_tty);
 

(Note that this also redirects gdb_stdtarg to str_file, which helps with the monitor command. Trying to send the output of the monitor command to a python string, while debugging a remote target, actually is what triggered me to do all this.)
Comment 8 Jan Kratochvil 2010-08-27 12:22:13 EDT
(In reply to comment #7)
> +static void
> +do_ui_out_redirect (void *dummy)
> +{
> +  ui_out_redirect (uiout, NULL);
> +};

The original redirection may not be to NULL but to some other file
when `set logging' is active.  You should restore the original value, not to just set it to NULL.

Also this function should be present in utils.c where all the cleanup stubs are present.

>    make_cleanup_restore_ui_file (&gdb_stdout);
>    make_cleanup_restore_ui_file (&gdb_stderr);
> +  make_cleanup_restore_ui_file (&gdb_stdtarg);
+
> (Note that this also redirects gdb_stdtarg to str_file, which helps with the
> monitor command. Trying to send the output of the monitor command to a python
> string, while debugging a remote target, actually is what triggered me to do
> all this.)

Shouldn't be also gdb_stdlog redirected?
(I am not sure now but I ask, at least it could be commented why not).

This is not a requirement for the patch, just if you would like it complete.


> +  make_cleanup (do_ui_out_redirect, 0);

This should be also moved to the stub in utils.c


BTW this patch is getting larger than trivial, do you have a signed FSF GDB copyright assignment?  If not please contact Tom Tromey <tromey at redhat.com>.

The patch will need to be posted to <gdb-patches@sourceware.org>.

I can check it more during weekend otherwise.
Comment 9 Paul Bolle 2010-08-27 12:36:03 EDT
(In reply to comment #8)
> BTW this patch is getting larger than trivial, do you have a signed FSF GDB
> copyright assignment?  If not please contact Tom Tromey <tromey at redhat.com>.

No, I don't. I'm happy to do that, but I'm also happy to see a patch submitted on your name (as I'm just following the directions you gave me).

> The patch will need to be posted to <gdb-patches@sourceware.org>.
> 
> I can check it more during weekend otherwise.

I suppose we finalize the patch here, before sending it to that list. Is that right?
Comment 10 Jan Kratochvil 2010-08-27 12:44:32 EDT
(In reply to comment #9)
> I suppose we finalize the patch here, before sending it to that list. Is that
> right?

I do not mind, I usually post an upstream post link to the RH Bug anyway.
Comment 11 Paul Bolle 2010-08-28 06:48:15 EDT
(In reply to comment #8)
> The original redirection may not be to NULL but to some other file
> when `set logging' is active.  You should restore the original value, not to
> just set it to NULL.

0) There seem to be three places where a pair of complementing calls to ui_out_redirect() is used:
    gdb/breakpoint.c:
        save_breakpoints()
    gdb/python/py-breakpoint.c:
        bppy_get_commands()
    gdb/cli/cli-logging.c:
        set_logging_on()
            handle_redirections()
        set_logging_off()
            pop_output_files()

Doesn't the same problem occur with the interaction of save_breakpoints(), bppy_get_commands(), and set_logging_on and set_logging_off?

1) I'd think gdb/cli-out.c:cli_redirect() could manipulate a (limited) stack of streams in ui_out->data instead of the current pair of ui_out->data-stream and ui_out->data->original_stream to handle this problem. But how many levels of redirection are actually possible?
Comment 12 Paul Bolle 2010-08-28 07:03:12 EDT
(In reply to comment #8)
> Shouldn't be also gdb_stdlog redirected?
> (I am not sure now but I ask, at least it could be commented why not).
> 
> This is not a requirement for the patch, just if you would like it complete.

I wouldn't know. I know next to nothing about the logging system. I never ran into a problem with the python command and gdb_stdlog so I haven't looked at it yet. 

By the way, there's also gdb_stdtargerr. But that's new for me too.
Comment 13 Paul Bolle 2010-09-01 07:34:04 EDT
Created attachment 442390 [details]
updated patch, taken into account comments from comment #8

0) Another try, with a rather large patch.

1) Patch highlights:
- allows for a trivial stack of four outstreams (ie, three redirects). Four is an arbitrary cutoff;
- that stack is implemented with cli_stream_push(), cli_stream_pop(), and changes to struct cli_ui_out_data;
- adds a new cleanup: make_cleanup_redirect_ui_out(). That cleanup doesn't blindly pop off the last outstream, but has a simple test to see if an outstream needs to be popped off (eg, it tries to do the right thing even if invoked before the outstream actually was redirected in the first place);
- now only four lines are changed in gdb/top.c:execute_command_to_string(), but rather a lot had to be changed in related areas to get there.

2) Comments appreciated.
Comment 14 Paul Bolle 2010-09-01 07:41:33 EDT
Created attachment 442392 [details]
include _all_ needed changes

0) Yet another try, with an even larger patch (which should actually be complete).

1) Patch highlights:
- allows for a trivial stack of four outstreams (ie, three redirects). Four is
an arbitrary cutoff;
- that stack is implemented with cli_stream_push(), cli_stream_pop(), and
changes to struct cli_ui_out_data;
- adds ui_out_can_redirect(), a small helper function;
- adds a new cleanup: make_cleanup_redirect_ui_out(). That cleanup doesn't
blindly pop off the last outstream, but has a simple test to see if an
outstream needs to be popped off (eg, it tries to do the right thing even if
invoked before the outstream actually was redirected in the first place);
- now only four lines are changed in gdb/top.c:execute_command_to_string(), but
rather a lot had to be changed in related areas to get there.

2) Comments appreciated.
Comment 15 Jan Kratochvil 2010-09-02 13:25:08 EDT
(In reply to comment #14)
> - allows for a trivial stack of four outstreams (ie, three redirects). Four is
> an arbitrary cutoff;

I do not find such limitation needed/useful/good.

> 2) Comments appreciated.

Bugzilla is not a place for patches discussion.  gdb-patches@sourceware.org is.
Going to post there something.
Comment 16 Paul Bolle 2010-09-02 13:37:30 EDT
(In reply to comment #15)
> Going to post there something.

If you mean to say that you're going to post a solution on that list, please feel free to CC me.
Comment 17 Jan Kratochvil 2010-09-03 07:22:24 EDT
pre-requisite fix:
[patch] Fix nesting of ui_out_redirect
http://sourceware.org/ml/gdb-patches/2010-09/msg00120.html
Comment 18 Jan Kratochvil 2010-09-03 14:08:04 EDT
The fix:
[patch] Fix uiout for execute_command_to_string
http://sourceware.org/ml/gdb-patches/2010-09/msg00131.html

I haven't seen a real world reason for ui_out_can_redirect().  If you think it should be there follow up upstream, please.
Comment 19 Jan Kratochvil 2010-09-11 12:02:25 EDT
Upstream check-in:
http://sourceware.org/ml/gdb-cvs/2010-09/msg00080.html
Comment 20 Fedora Update System 2010-09-11 15:17:25 EDT
gdb-7.2-4.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/gdb-7.2-4.fc14
Comment 21 Fedora Update System 2010-09-14 00:59:05 EDT
gdb-7.2-4.fc14 has been pushed to the Fedora 14 stable repository.  If problems still persist, please make note of it in this bug report.

Note You need to log in before you can comment on or make changes to this bug.