From patchwork Thu Feb 2 22:12:46 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 19098 Received: (qmail 13300 invoked by alias); 2 Feb 2017 22:12:53 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 13271 invoked by uid 89); 2 Feb 2017 22:12:52 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=LGTM!, lgtm!, H*f:sk:eea863e, H*MI:sk:3047e09 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 02 Feb 2017 22:12:49 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 64D5B64A7E; Thu, 2 Feb 2017 22:12:49 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.phx2.redhat.com [10.5.9.4]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v12MCmCD021776; Thu, 2 Feb 2017 17:12:48 -0500 Subject: Re: [PATCH] Move "tee" building down to interpreter::set_logging_proc To: Simon Marchi References: <1486045694-22866-1-git-send-email-palves@redhat.com> <1fc904d1eef0f26d27539a2b2b4da5ab@polymtl.ca> <3047e09325f39e0acf745009c17d75d1@polymtl.ca> Cc: gdb-patches@sourceware.org From: Pedro Alves Message-ID: <4f170dc7-bf82-ea0f-8c2d-6fc1c09eb794@redhat.com> Date: Thu, 2 Feb 2017 22:12:46 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <3047e09325f39e0acf745009c17d75d1@polymtl.ca> On 02/02/2017 08:42 PM, Simon Marchi wrote: > On 2017-02-02 13:04, Pedro Alves wrote: >> How about this? > LGTM! Many thanks for the review! I've pushed it in now, as below. (Added a missing "the".) From 616268b639780e0819b51053c794037bcde3de16 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Thu, 2 Feb 2017 22:00:43 +0000 Subject: [PATCH] Move "tee" building down to interpreter::set_logging_proc This patch gets rid of this hack in mi_set_logging: /* The tee created already is based on gdb_stdout, which for MI is a console and so we end up in an infinite loop of console writing to ui_file writing to console etc. So discard the existing tee (it hasn't been used yet, and MI won't ever use it), and create one based on raw_stdout instead. */ By pushing down responsibility for the tee creation to the interpreter. I.e., pushing the CLI bits out of handle_redirections down to the CLI interpreter's set_logging_proc method. This fixes a few leaks that I spotted, and then confirmed with "valgrind --leak-check=full": [...] ==21429== 56 (32 direct, 24 indirect) bytes in 1 blocks are definitely lost in loss record 30,243 of 34,980 ==21429== at 0x4C29216: operator new(unsigned long) (vg_replace_malloc.c:334) ==21429== by 0x62D9A9: mi_set_logging(interp*, int, ui_file*, ui_file*) (mi-interp.c:1395) ==21429== by 0x810B8A: current_interp_set_logging(int, ui_file*, ui_file*) (interps.c:360) ==21429== by 0x61C537: handle_redirections(int) (cli-logging.c:162) ==21429== by 0x61C6EC: set_logging_on(char*, int) (cli-logging.c:190) ==21429== by 0x6163BE: do_cfunc(cmd_list_element*, char*, int) (cli-decode.c:105) ==21429== by 0x6193C1: cmd_func(cmd_list_element*, char*, int) (cli-decode.c:1913) ==21429== by 0x8DB790: execute_command(char*, int) (top.c:674) ==21429== by 0x632AE6: mi_execute_cli_command(char const*, int, char const*) (mi-main.c:2343) ==21429== by 0x6329BA: mi_cmd_execute(mi_parse*) (mi-main.c:2306) ==21429== by 0x631E19: captured_mi_execute_command(ui_out*, mi_parse*) (mi-main.c:1998) ==21429== by 0x632389: mi_execute_command(char const*, int) (mi-main.c:2163) ==21429== [...] ==26635== 24 bytes in 1 blocks are definitely lost in loss record 20,740 of 34,995 ==26635== at 0x4C29216: operator new(unsigned long) (vg_replace_malloc.c:334) ==26635== by 0x61C355: handle_redirections(int) (cli-logging.c:131) ==26635== by 0x61C6EC: set_logging_on(char*, int) (cli-logging.c:190) ==26635== by 0x6163BE: do_cfunc(cmd_list_element*, char*, int) (cli-decode.c:105) ==26635== by 0x6193C1: cmd_func(cmd_list_element*, char*, int) (cli-decode.c:1913) ==26635== by 0x8DB7BC: execute_command(char*, int) (top.c:674) ==26635== by 0x7B9132: command_handler(char*) (event-top.c:590) ==26635== by 0x7B94F7: command_line_handler(char*) (event-top.c:780) ==26635== by 0x7B8ABB: gdb_rl_callback_handler(char*) (event-top.c:213) ==26635== by 0x933CE9: rl_callback_read_char (callback.c:220) ==26635== by 0x7B89ED: gdb_rl_callback_read_char_wrapper_noexcept() (event-top.c:175) ==26635== by 0x7B8A49: gdb_rl_callback_read_char_wrapper(void*) (event-top.c:192) One is fixed by transfering ownership of the log file to the tee. In pseudo-patch, since the code was moved at the same time: - out = new tee_file (curr_output, false, logfile.get (), false); + out = new tee_file (curr_output, false, logfile.get (), true); The other is this bit in mi_set_logging: else { + delete mi->raw_stdout; I tried to split the leak fixes to a smaller preparatory patch, but that was difficult exactly because of the tee hack in handle_redirections -> mi_set_logging. gdb/ChangeLog: 2017-02-02 Pedro Alves * cli/cli-interp.c (struct saved_output_files, saved_output): Moved from cli/cli-logging.c. (cli_set_logging): New function. (cli_interp_procs): Install cli_set_logging. * cli/cli-interp.h (make_logging_output, cli_set_logging): Declare. * cli/cli-logging.c (struct saved_output_files, saved_output): Moved to cli/cli-interp.c. (pop_output_files): Don't save outputs here. (make_logging_output): New function. (handle_redirections): Don't build tee nor save previous outputs here. * interps.c (current_interp_set_logging): Change prototype. Assume there's always a set_logging_proc method installed. * interps.h (interp_set_logging_ftype): Change prototype. (current_interp_set_logging): Change prototype and adjust comment. * mi/mi-interp.c (mi_set_logging): Change protototype. Adjust to use make_logging_output. * tui/tui-interp.c (tui_interp_procs): Install cli_set_logging. --- gdb/ChangeLog | 21 ++++++++++++ gdb/cli/cli-interp.c | 59 ++++++++++++++++++++++++++++++++- gdb/cli/cli-interp.h | 18 ++++++++++ gdb/cli/cli-logging.c | 92 +++++++++++++++++---------------------------------- gdb/interps.c | 13 +++----- gdb/interps.h | 22 ++++++------ gdb/mi/mi-interp.c | 30 ++++++----------- gdb/tui/tui-interp.c | 2 +- 8 files changed, 155 insertions(+), 102 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index df5238c..fbbcc07 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,5 +1,26 @@ 2017-02-02 Pedro Alves + * cli/cli-interp.c (struct saved_output_files, saved_output): + Moved from cli/cli-logging.c. + (cli_set_logging): New function. + (cli_interp_procs): Install cli_set_logging. + * cli/cli-interp.h (make_logging_output, cli_set_logging): + Declare. + * cli/cli-logging.c (struct saved_output_files, saved_output): + Moved to cli/cli-interp.c. + (pop_output_files): Don't save outputs here. + (make_logging_output): New function. + (handle_redirections): Don't build tee nor save previous outputs + here. + * interps.c (current_interp_set_logging): Change prototype. + Assume there's always a set_logging_proc method installed. + * interps.h (interp_set_logging_ftype): Change prototype. + (current_interp_set_logging): Change prototype and adjust comment. + * mi/mi-interp.c (mi_set_logging): Change protototype. Adjust to + use make_logging_output. + * tui/tui-interp.c (tui_interp_procs): Install cli_set_logging. +2017-02-02 Pedro Alves + * cli/cli-logging.c (maybe_warn_already_logging): New factored out from ... (set_logging_overwrite): ... here. diff --git a/gdb/cli/cli-interp.c b/gdb/cli/cli-interp.c index 8802a5a..e0327f6 100644 --- a/gdb/cli/cli-interp.c +++ b/gdb/cli/cli-interp.c @@ -373,6 +373,63 @@ cli_ui_out (struct interp *self) return cli->cli_uiout; } +/* These hold the pushed copies of the gdb output files. + If NULL then nothing has yet been pushed. */ +struct saved_output_files +{ + ui_file *out; + ui_file *err; + ui_file *log; + ui_file *targ; + ui_file *targerr; +}; +static saved_output_files saved_output; + +/* See cli-interp.h. */ + +void +cli_set_logging (struct interp *interp, + ui_file_up logfile, bool logging_redirect) +{ + if (logfile != NULL) + { + saved_output.out = gdb_stdout; + saved_output.err = gdb_stderr; + saved_output.log = gdb_stdlog; + saved_output.targ = gdb_stdtarg; + saved_output.targerr = gdb_stdtargerr; + + /* A raw pointer since ownership is transferred to + gdb_stdout. */ + ui_file *output = make_logging_output (gdb_stdout, + std::move (logfile), + logging_redirect); + gdb_stdout = output; + gdb_stdlog = output; + gdb_stderr = output; + gdb_stdtarg = output; + gdb_stdtargerr = output; + } + else + { + /* Only delete one of the files -- they are all set to the same + value. */ + delete gdb_stdout; + + gdb_stdout = saved_output.out; + gdb_stderr = saved_output.err; + gdb_stdlog = saved_output.log; + gdb_stdtarg = saved_output.targ; + gdb_stdtargerr = saved_output.targerr; + + saved_output.out = NULL; + saved_output.err = NULL; + saved_output.log = NULL; + saved_output.targ = NULL; + saved_output.targerr = NULL; + } +} + /* The CLI interpreter's vtable. */ static const struct interp_procs cli_interp_procs = { @@ -381,7 +438,7 @@ static const struct interp_procs cli_interp_procs = { cli_interpreter_suspend, /* suspend_proc */ cli_interpreter_exec, /* exec_proc */ cli_ui_out, /* ui_out_proc */ - NULL, /* set_logging_proc */ + cli_set_logging, /* set_logging_proc */ cli_interpreter_pre_command_loop, /* pre_command_loop_proc */ cli_interpreter_supports_command_editing, /* supports_command_editing_proc */ }; diff --git a/gdb/cli/cli-interp.h b/gdb/cli/cli-interp.h index ef86372..f93548c 100644 --- a/gdb/cli/cli-interp.h +++ b/gdb/cli/cli-interp.h @@ -20,6 +20,24 @@ struct interp; +/* Make the output ui_file to use when logging is enabled. + CURR_OUTPUT is the stream where output is currently being sent to + (e.g., gdb_stdout for the CLI, raw output stream for the MI). + LOGFILE is the log file already opened by the caller. + LOGGING_REDIRECT is the value of the "set logging redirect" + setting. If true, the resulting output is the logfile. If false, + the output stream is a tee, with the log file as one of the + outputs. Ownership of LOGFILE is transferred to the returned + output file, which is an owning pointer. */ +extern ui_file *make_logging_output (ui_file *curr_output, + ui_file_up logfile, + bool logging_redirect); + +/* The CLI interpreter's set_logging_proc method. Exported so other + interpreters can reuse it. */ +extern void cli_set_logging (struct interp *interp, + ui_file_up logfile, bool logging_redirect); + extern int cli_interpreter_supports_command_editing (struct interp *interp); extern void cli_interpreter_pre_command_loop (struct interp *self); diff --git a/gdb/cli/cli-logging.c b/gdb/cli/cli-logging.c index edb8313..e8ec444 100644 --- a/gdb/cli/cli-logging.c +++ b/gdb/cli/cli-logging.c @@ -22,17 +22,6 @@ #include "ui-out.h" #include "interps.h" -/* These hold the pushed copies of the gdb output files. - If NULL then nothing has yet been pushed. */ -struct saved_output_files -{ - struct ui_file *out; - struct ui_file *err; - struct ui_file *log; - struct ui_file *targ; - struct ui_file *targerr; -}; -static struct saved_output_files saved_output; static char *saved_filename; static char *logging_filename; @@ -90,37 +79,35 @@ show_logging_redirect (struct ui_file *file, int from_tty, static void pop_output_files (void) { - if (current_interp_set_logging (0, NULL, NULL) == 0) - { - /* Only delete one of the files -- they are all set to the same - value. */ - delete gdb_stdout; - - gdb_stdout = saved_output.out; - gdb_stderr = saved_output.err; - gdb_stdlog = saved_output.log; - gdb_stdtarg = saved_output.targ; - gdb_stdtargerr = saved_output.targerr; - } - - saved_output.out = NULL; - saved_output.err = NULL; - saved_output.log = NULL; - saved_output.targ = NULL; - saved_output.targerr = NULL; + current_interp_set_logging (NULL, false); /* Stay consistent with handle_redirections. */ if (!current_uiout->is_mi_like_p ()) current_uiout->redirect (NULL); } +/* See cli-interp.h. */ + +ui_file * +make_logging_output (ui_file *curr_output, ui_file_up logfile, + bool logging_redirect) +{ + if (logging_redirect) + return logfile.release (); + else + { + /* Note that the "tee" takes ownership of the log file. */ + ui_file *out = new tee_file (curr_output, false, + logfile.get (), true); + logfile.release (); + return out; + } +} + /* This is a helper for the `set logging' command. */ static void handle_redirections (int from_tty) { - ui_file_up output; - ui_file_up no_redirect_file; - if (saved_filename != NULL) { fprintf_unfiltered (gdb_stdout, "Already logging to %s.\n", @@ -133,46 +120,27 @@ handle_redirections (int from_tty) perror_with_name (_("set logging")); /* Redirects everything to gdb_stdout while this is running. */ - if (!logging_redirect) + if (from_tty) { - no_redirect_file = std::move (log); - output.reset (new tee_file (gdb_stdout, 0, no_redirect_file.get (), 0)); - - if (from_tty) + if (!logging_redirect) fprintf_unfiltered (gdb_stdout, "Copying output to %s.\n", logging_filename); - } - else - { - output = std::move (log); - - if (from_tty) + else fprintf_unfiltered (gdb_stdout, "Redirecting output to %s.\n", logging_filename); } saved_filename = xstrdup (logging_filename); - saved_output.out = gdb_stdout; - saved_output.err = gdb_stderr; - saved_output.log = gdb_stdlog; - saved_output.targ = gdb_stdtarg; - saved_output.targerr = gdb_stdtargerr; /* Let the interpreter do anything it needs. */ - if (current_interp_set_logging (1, output.get (), - no_redirect_file.get ()) == 0) - { - gdb_stdout = output.get (); - gdb_stdlog = output.get (); - gdb_stderr = output.get (); - gdb_stdtarg = output.get (); - gdb_stdtargerr = output.get (); - } - - output.release (); - no_redirect_file.release (); - - /* Don't do the redirect for MI, it confuses MI's ui-out scheme. */ + current_interp_set_logging (std::move (log), logging_redirect); + + /* Redirect the current ui-out object's output to the log. Use + gdb_stdout, not log, since the interpreter may have created a tee + that wraps the log. Don't do the redirect for MI, it confuses + MI's ui-out scheme. Note that we may get here with MI as current + interpreter, but with the current ui_out as a CLI ui_out, with + '-interpreter-exec console "set logging on"'. */ if (!current_uiout->is_mi_like_p ()) current_uiout->redirect (gdb_stdout); } diff --git a/gdb/interps.c b/gdb/interps.c index a2b7ffe..06e7ccf 100644 --- a/gdb/interps.c +++ b/gdb/interps.c @@ -346,18 +346,15 @@ interp_ui_out (struct interp *interp) return interp->procs->ui_out_proc (interp); } -int -current_interp_set_logging (int start_log, struct ui_file *out, - struct ui_file *logfile) +void +current_interp_set_logging (ui_file_up logfile, + bool logging_redirect) { struct ui_interp_info *ui_interp = get_current_interp_info (); struct interp *interp = ui_interp->current_interpreter; - if (interp == NULL - || interp->procs->set_logging_proc == NULL) - return 0; - - return interp->procs->set_logging_proc (interp, start_log, out, logfile); + return interp->procs->set_logging_proc (interp, std::move (logfile), + logging_redirect); } /* Temporarily overrides the current interpreter. */ diff --git a/gdb/interps.h b/gdb/interps.h index 5ec2b05..ef2ceeb 100644 --- a/gdb/interps.h +++ b/gdb/interps.h @@ -49,9 +49,9 @@ typedef struct gdb_exception (interp_exec_ftype) (void *data, typedef void (interp_pre_command_loop_ftype) (struct interp *self); typedef struct ui_out *(interp_ui_out_ftype) (struct interp *self); -typedef int (interp_set_logging_ftype) (struct interp *self, int start_log, - struct ui_file *out, - struct ui_file *logfile); +typedef void (interp_set_logging_ftype) (struct interp *self, + ui_file_up logfile, + bool logging_redirect); typedef int (interp_supports_command_editing_ftype) (struct interp *self); @@ -109,13 +109,15 @@ extern int current_interp_named_p (const char *name); /* Call this function to give the current interpreter an opportunity to do any special handling of streams when logging is enabled or - disabled. START_LOG is 1 when logging is starting, 0 when it ends, - and OUT is the stream for the log file; it will be NULL when - logging is ending. LOGFILE is non-NULL if the output streams - are to be tees, with the log file as one of the outputs. */ - -extern int current_interp_set_logging (int start_log, struct ui_file *out, - struct ui_file *logfile); + disabled. LOGFILE is the stream for the log file when logging is + starting and is NULL when logging is ending. LOGGING_REDIRECT is + the value of the "set logging redirect" setting. If true, the + interpreter should configure the output streams to send output only + to the logfile. If false, the interpreter should configure the + output streams to send output to both the current output stream + (i.e., the terminal) and the log file. */ +extern void current_interp_set_logging (ui_file_up logfile, + bool logging_redirect); /* Returns opaque data associated with the top-level interpreter. */ extern void *top_level_interpreter_data (void); diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c index f167a53..aa76989 100644 --- a/gdb/mi/mi-interp.c +++ b/gdb/mi/mi-interp.c @@ -1373,33 +1373,25 @@ mi_ui_out (struct interp *interp) /* Do MI-specific logging actions; save raw_stdout, and change all the consoles to use the supplied ui-file(s). */ -static int -mi_set_logging (struct interp *interp, int start_log, - struct ui_file *out, struct ui_file *logfile) +static void +mi_set_logging (struct interp *interp, + ui_file_up logfile, bool logging_redirect) { struct mi_interp *mi = (struct mi_interp *) interp_data (interp); - if (!mi) - return 0; + gdb_assert (mi != NULL); - if (start_log) + if (logfile != NULL) { - /* The tee created already is based on gdb_stdout, which for MI - is a console and so we end up in an infinite loop of console - writing to ui_file writing to console etc. So discard the - existing tee (it hasn't been used yet, and MI won't ever use - it), and create one based on raw_stdout instead. */ - if (logfile) - { - delete out; - out = new tee_file (mi->raw_stdout, false, logfile, false); - } - mi->saved_raw_stdout = mi->raw_stdout; - mi->raw_stdout = out; + mi->raw_stdout = make_logging_output (mi->raw_stdout, + std::move (logfile), + logging_redirect); + } else { + delete mi->raw_stdout; mi->raw_stdout = mi->saved_raw_stdout; mi->saved_raw_stdout = NULL; } @@ -1409,8 +1401,6 @@ mi_set_logging (struct interp *interp, int start_log, mi->log->set_raw (mi->raw_stdout); mi->targ->set_raw (mi->raw_stdout); mi->event_channel->set_raw (mi->raw_stdout); - - return 1; } /* The MI interpreter's vtable. */ diff --git a/gdb/tui/tui-interp.c b/gdb/tui/tui-interp.c index 7893904..e2c0605 100644 --- a/gdb/tui/tui-interp.c +++ b/gdb/tui/tui-interp.c @@ -302,7 +302,7 @@ static const struct interp_procs tui_interp_procs = { tui_suspend, tui_exec, tui_ui_out, - NULL, + cli_set_logging, cli_interpreter_pre_command_loop, cli_interpreter_supports_command_editing, };