This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH v2 5/5] Eliminate make_cleanup_ui_file_delete / make ui_file a class hierarchy


On 01/25/2017 05:52 PM, Pedro Alves wrote:
> 
>> > 
>>> >> +  if (!log->open (logging_filename, logging_overwrite ? "w" : "a"))
>>> >>      perror_with_name (_("set logging"));
>>> >> -  cleanups = make_cleanup_ui_file_delete (output);
>>> >> +
>>> >> +  output = log.get ();
>>> >>
>>> >>    /* Redirects everything to gdb_stdout while this is running.  */
>>> >>    if (!logging_redirect)
>>> >>      {
>>> >>        no_redirect_file = output;
>>> >>
>>> >> -      output = tee_file_new (gdb_stdout, 0, no_redirect_file, 0);
>>> >> -      if (output == NULL)
>>> >> -    perror_with_name (_("set logging"));
>>> >> -      make_cleanup_ui_file_delete (output);
>>> >> +      output = new tee_file (gdb_stdout, 0, no_redirect_file, 0);
>> > 
>> > Is there anything that replaces this cleanup?  IOW, if the
>> > fprinf_unfiltered just below did throw an exception for some reason,
>> > would this tee_file leak?
> Looks like there isn't.  I'll fix it.  Bah, this is going to
> mess up a follow-up patch to push down the tee to the interpreter
> callback that I had prepared meanwhile.  :-)
> 

Here's the diff to the incoming v3.  I think this addresses all
your comments.  The only other change is that I realized that
ui_file should be abstract, so that people don't try to allocate
one to build a "/dev/null" stream.  I've checked the AIX build
on gcc119.  I'll send v3 shortly.

The "tee" patch I was mentioning will make
current_interp_set_logging take a ui_file_up and a bool
instead of two ui_file * pointers, so some of the ".get()s"
will end up disappearing (if that goes in).

>From 4e444042b03c07f8387c872f9ded53bb81082a90 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 25 Jan 2017 18:52:31 +0000
Subject: [PATCH] v2-review

---
 gdb/aix-thread.c      |  3 +--
 gdb/arm-tdep.c        | 11 ++++-------
 gdb/cli/cli-logging.c | 34 +++++++++++++++++-----------------
 gdb/mi/mi-console.c   |  6 ------
 gdb/mi/mi-console.h   |  2 --
 gdb/serial.c          |  2 +-
 gdb/ui-file.h         | 18 ++++++++++++++----
 gdb/utils.h           |  3 ---
 8 files changed, 37 insertions(+), 42 deletions(-)

diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
index cc14d9d..cf1a462 100644
--- a/gdb/aix-thread.c
+++ b/gdb/aix-thread.c
@@ -1749,7 +1749,6 @@ static char *
 aix_thread_extra_thread_info (struct target_ops *self,
 			      struct thread_info *thread)
 {
-  struct ui_file *buf;
   int status;
   pthdb_pthread_t pdtid;
   pthdb_tid_t tid;
@@ -1797,7 +1796,7 @@ aix_thread_extra_thread_info (struct target_ops *self,
 
   xfree (ret);			/* Free old buffer.  */
 
-  ret = xstrdup (buf.string ());
+  ret = xstrdup (buf.c_str ());
 
   return ret;
 }
diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index bc170b1..efce97b 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -9578,7 +9578,6 @@ _initialize_arm_tdep (void)
   const char *setdesc;
   const char *const *regnames;
   int i;
-  static std::string helptext;
   char regdesc[1024], *rdptr = regdesc;
   size_t rest = sizeof (regdesc);
 
@@ -9644,12 +9643,10 @@ _initialize_arm_tdep (void)
   valid_disassembly_styles[num_disassembly_options] = NULL;
 
   /* Create the help text.  */
-  string_file stb;
-  stb.printf ("%s%s%s",
-	      _("The valid values are:\n"),
-	      regdesc,
-	      _("The default is \"std\"."));
-  helptext = std::move (stb.string ());
+  std::string helptext = string_printf ("%s%s%s",
+					_("The valid values are:\n"),
+					regdesc,
+					_("The default is \"std\"."));
 
   add_setshow_enum_cmd("disassembler", no_class,
 		       valid_disassembly_styles, &disassembly_style,
diff --git a/gdb/cli/cli-logging.c b/gdb/cli/cli-logging.c
index f9a845e..f165896 100644
--- a/gdb/cli/cli-logging.c
+++ b/gdb/cli/cli-logging.c
@@ -178,8 +178,8 @@ pop_output_files (void)
 static void
 handle_redirections (int from_tty)
 {
-  struct ui_file *output;
-  struct ui_file *no_redirect_file = NULL;
+  ui_file_up output;
+  ui_file_up no_redirect_file;
 
   if (saved_filename != NULL)
     {
@@ -188,34 +188,30 @@ handle_redirections (int from_tty)
       return;
     }
 
-  std::unique_ptr<stdio_file> log (new stdio_file ());
+  stdio_file_up log (new stdio_file ());
   if (!log->open (logging_filename, logging_overwrite ? "w" : "a"))
     perror_with_name (_("set logging"));
 
-  output = log.get ();
-
   /* Redirects everything to gdb_stdout while this is running.  */
   if (!logging_redirect)
     {
-      no_redirect_file = output;
+      no_redirect_file = std::move (log);
+      output.reset (new tee_file (gdb_stdout, 0, no_redirect_file.get (), 0));
 
-      output = new tee_file (gdb_stdout, 0, no_redirect_file, 0);
       if (from_tty)
 	fprintf_unfiltered (gdb_stdout, "Copying output to %s.\n",
 			    logging_filename);
-      logging_no_redirect_file = no_redirect_file;
     }
   else
     {
       gdb_assert (logging_no_redirect_file == NULL);
+      output = std::move (log);
 
       if (from_tty)
 	fprintf_unfiltered (gdb_stdout, "Redirecting output to %s.\n",
 			    logging_filename);
     }
 
-  log.release ();
-
   saved_filename = xstrdup (logging_filename);
   saved_output.out = gdb_stdout;
   saved_output.err = gdb_stderr;
@@ -224,18 +220,22 @@ handle_redirections (int from_tty)
   saved_output.targerr = gdb_stdtargerr;
 
   /* Let the interpreter do anything it needs.  */
-  if (current_interp_set_logging (1, output, no_redirect_file) == 0)
+  if (current_interp_set_logging (1, output.get (),
+				  no_redirect_file.get ()) == 0)
     {
-      gdb_stdout = output;
-      gdb_stdlog = output;
-      gdb_stderr = output;
-      gdb_stdtarg = output;
-      gdb_stdtargerr = output;
+      gdb_stdout = output.get ();
+      gdb_stdlog = output.get ();
+      gdb_stderr = output.get ();
+      gdb_stdtarg = output.get ();
+      gdb_stdtargerr = output.get ();
     }
 
+  output.release ();
+  logging_no_redirect_file = no_redirect_file.release ();
+
   /* Don't do the redirect for MI, it confuses MI's ui-out scheme.  */
   if (!current_uiout->is_mi_like_p ())
-    current_uiout->redirect (output);
+    current_uiout->redirect (gdb_stdout);
 }
 
 static void
diff --git a/gdb/mi/mi-console.c b/gdb/mi/mi-console.c
index 218ffbc..7c1c2ac 100644
--- a/gdb/mi/mi-console.c
+++ b/gdb/mi/mi-console.c
@@ -37,12 +37,6 @@ mi_console_file::mi_console_file (ui_file *raw, const char *prefix, char quote)
 {}
 
 void
-mi_console_file::puts (const char *buf)
-{
-  this->write (buf, strlen (buf));
-}
-
-void
 mi_console_file::write (const char *buf, long length_buf)
 {
   size_t prev_size = m_buffer.size ();
diff --git a/gdb/mi/mi-console.h b/gdb/mi/mi-console.h
index a7962a8..289013f 100644
--- a/gdb/mi/mi-console.h
+++ b/gdb/mi/mi-console.h
@@ -39,8 +39,6 @@ public:
 
   void write (const char *buf, long length_buf) override;
 
-  void puts (const char *) override;
-
 private:
   /* The wrapped raw output stream.  */
   ui_file *m_raw;
diff --git a/gdb/serial.c b/gdb/serial.c
index 831d2eb..b48b977 100644
--- a/gdb/serial.c
+++ b/gdb/serial.c
@@ -251,7 +251,7 @@ serial_open_ops_1 (const struct serial_ops *ops, const char *open_name)
 
   if (serial_logfile != NULL)
     {
-      std::unique_ptr<stdio_file> file (new stdio_file ());
+      stdio_file_up file (new stdio_file ());
 
       if (!file->open (serial_logfile, "w"))
 	perror_with_name (serial_logfile);
diff --git a/gdb/ui-file.h b/gdb/ui-file.h
index 3972d50..1cb9693 100644
--- a/gdb/ui-file.h
+++ b/gdb/ui-file.h
@@ -21,13 +21,13 @@
 
 #include <string>
 
-/* The ui_file base class.  */
+/* The abstract ui_file base class.  */
 
 class ui_file
 {
 public:
   ui_file ();
-  virtual ~ui_file ();
+  virtual ~ui_file () = 0;
 
   /* Public non-virtual API.  */
 
@@ -117,8 +117,16 @@ public:
   /* string_file-specific public API.  */
 
   /* Accesses the std::string containing the entire output collected
-     so far.  Returns a non-const reference so that it's easy to move
-     the string contents out of the string_file.  */
+     so far.
+
+     Returns a non-const reference so that it's easy to move the
+     string contents out of the string_file.  E.g.:
+
+      string_file buf;
+      buf.printf (....);
+      buf.printf (....);
+      return std::move (buf.string ());
+  */
   std::string &string () { return m_string; }
 
   /* Provide a few convenience methods with the same API as the
@@ -187,6 +195,8 @@ private:
   bool m_close_p;
 };
 
+typedef std::unique_ptr<stdio_file> stdio_file_up;
+
 /* Like stdio_file, but specifically for stderr.
 
    This exists because there is no real line-buffering on Windows, see
diff --git a/gdb/utils.h b/gdb/utils.h
index aada0a4..f138702 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -66,9 +66,6 @@ char **gdb_buildargv (const char *);
 
 extern struct cleanup *make_cleanup_freeargv (char **);
 
-struct ui_file;
-extern struct cleanup *make_cleanup_ui_file_delete (struct ui_file *);
-
 struct ui_out;
 extern struct cleanup *
   make_cleanup_ui_out_redirect_pop (struct ui_out *uiout);
-- 
2.5.5



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]