[PATCH 0/4] Some cleanup removal

[PATCH 0/4] Some cleanup removal

[Tom Tromey]

This series removes a few more cleanups.

I tested this on the buildbot; but of course some builders aren't
really working.  Also, I don't think we have a builder that builds
procfs.c, so that patch is totally untested -- perhaps I ought to just
drop it.

Let me know what you think.

Tom

[PATCH 1/4] Remove cleanup from add_path

[Tom Tromey]

This removes a cleanup from add_path, replacing it with a use of
gdb::unique_xmalloc_ptr.  Note that this declaration had to be hoisted
somewhat, to avoid inteference from the "goto"s in this function.

gdb/ChangeLog
2018-09-03  Tom Tromey  <tom@tromey.com>

	* source.c (add_path): Use gdb::unique_xmalloc_ptr.
---
 gdb/ChangeLog |  4 ++++
 gdb/source.c  | 16 +++++++---------
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/gdb/source.c b/gdb/source.c
index cc5c46d0a7b..ec0ea3b81e3 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -479,13 +479,12 @@ add_path (const char *dirname, char **which_path, int parse_separators)
   else
     dir_vec.emplace_back (xstrdup (dirname));
 
-  struct cleanup *back_to = make_cleanup (null_cleanup, NULL);
-
   for (const gdb::unique_xmalloc_ptr<char> &name_up : dir_vec)
     {
       char *name = name_up.get ();
       char *p;
       struct stat st;
+      gdb::unique_xmalloc_ptr<char> new_name_holder;
 
       /* Spaces and tabs will have been removed by buildargv().
          NAME is the start of the directory.
@@ -531,16 +530,17 @@ add_path (const char *dirname, char **which_path, int parse_separators)
 	}
 
       if (name[0] == '~')
-	name = tilde_expand (name);
+	new_name_holder.reset (tilde_expand (name));
 #ifdef HAVE_DOS_BASED_FILE_SYSTEM
       else if (IS_ABSOLUTE_PATH (name) && p == name + 2) /* "d:" => "d:." */
-	name = concat (name, ".", (char *)NULL);
+	new_name_holder.reset (concat (name, ".", (char *) NULL));
 #endif
       else if (!IS_ABSOLUTE_PATH (name) && name[0] != '$')
-	name = concat (current_directory, SLASH_STRING, name, (char *)NULL);
+	new_name_holder.reset (concat (current_directory, SLASH_STRING, name,
+				       (char *) NULL));
       else
-	name = savestring (name, p - name);
-      make_cleanup (xfree, name);
+	new_name_holder.reset (savestring (name, p - name));
+      name = new_name_holder.get ();
 
       /* Unless it's a variable, check existence.  */
       if (name[0] != '$')
@@ -630,8 +630,6 @@ add_path (const char *dirname, char **which_path, int parse_separators)
     skip_dup:
       ;
     }
-
-  do_cleanups (back_to);
 }
 
 
-- 
2.13.6

[PATCH 2/4] Remove cleanup from procfs.c

[Tom Tromey]

This removes the last remaining cleanup from procfs.c, replacing it
with a unique_ptr specialization.

gdb/ChangeLog
2018-09-03  Tom Tromey  <tom@tromey.com>

	* procfs.c (struct procinfo_deleter): New.
	(procinfo_up): New typedef.
	(do_destroy_procinfo_cleanup): Remove.
	(procfs_target::info_proc): Use procinfo_up.  Remove cleanups.
---
 gdb/ChangeLog |  7 +++++++
 gdb/procfs.c  | 21 ++++++++++++---------
 2 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/gdb/procfs.c b/gdb/procfs.c
index ec346503487..7d41907c4aa 100644
--- a/gdb/procfs.c
+++ b/gdb/procfs.c
@@ -549,11 +549,16 @@ destroy_procinfo (procinfo *pi)
     }
 }
 
-static void
-do_destroy_procinfo_cleanup (void *pi)
+/* A deleter that calls destroy_procinfo.  */
+struct procinfo_deleter
 {
-  destroy_procinfo ((procinfo *) pi);
-}
+  void operator() (procinfo *pi) const
+  {
+    destroy_procinfo (pi);
+  }
+};
+
+typedef std::unique_ptr<procinfo, procinfo_deleter> procinfo_up;
 
 enum { NOKILL, KILL };
 
@@ -3545,7 +3550,6 @@ info_proc_mappings (procinfo *pi, int summary)
 bool
 procfs_target::info_proc (const char *args, enum info_proc_what what)
 {
-  struct cleanup *old_chain;
   procinfo *process  = NULL;
   procinfo *thread   = NULL;
   char     *tmp      = NULL;
@@ -3567,7 +3571,6 @@ procfs_target::info_proc (const char *args, enum info_proc_what what)
       error (_("Not supported on this target."));
     }
 
-  old_chain = make_cleanup (null_cleanup, 0);
   gdb_argv built_argv (args);
   for (char *arg : built_argv)
     {
@@ -3582,6 +3585,8 @@ procfs_target::info_proc (const char *args, enum info_proc_what what)
 	  tid = strtoul (arg + 1, NULL, 10);
 	}
     }
+
+  procinfo_up temporary_procinfo;
   if (pid == 0)
     pid = inferior_ptid.pid ();
   if (pid == 0)
@@ -3596,7 +3601,7 @@ procfs_target::info_proc (const char *args, enum info_proc_what what)
 	   /* No.  So open a procinfo for it, but
 	      remember to close it again when finished.  */
 	   process = create_procinfo (pid, 0);
-	   make_cleanup (do_destroy_procinfo_cleanup, process);
+	   temporary_procinfo.reset (process);
 	   if (!open_procinfo_files (process, FD_CTL))
 	     proc_error (process, "info proc, open_procinfo_files", __LINE__);
 	 }
@@ -3627,8 +3632,6 @@ procfs_target::info_proc (const char *args, enum info_proc_what what)
       info_proc_mappings (process, 0);
     }
 
-  do_cleanups (old_chain);
-
   return true;
 }
 
-- 
2.13.6

Re: [PATCH 2/4] Remove cleanup from procfs.c

[Rainer Orth]

Hi Tom,

> This removes the last remaining cleanup from procfs.c, replacing it
> with a unique_ptr specialization.

I've now regtested the patch on amd64-pc-solaris2.11.  After fixing one
compile failure

/vol/src/gnu/gdb/hg/master/dist/gdb/procfs.c:275:13: error: ‘void do_destroy_procinfo_cleanup(void*)’ declared ‘static’ but never defined [-Werror=unused-function]
 static void do_destroy_procinfo_cleanup (void *);
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~

by removing the unnecessary declaration, testing passed without
regressions (as far as I can tell with racy tests etc.).

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH 2/4] Remove cleanup from procfs.c

[Tom Tromey]

>>>>> "Rainer" == Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> writes:

Rainer> I've now regtested the patch on amd64-pc-solaris2.11.

Thank you for doing this.

Rainer> /vol/src/gnu/gdb/hg/master/dist/gdb/procfs.c:275:13: error: ‘void do_destroy_procinfo_cleanup(void*)’ declared ‘static’ but never defined [-Werror=unused-function]
Rainer>  static void do_destroy_procinfo_cleanup (void *);
Rainer>              ^~~~~~~~~~~~~~~~~~~~~~~~~~~

Rainer> by removing the unnecessary declaration, testing passed without
Rainer> regressions (as far as I can tell with racy tests etc.).

I've removed the declaration on my branch.

Tom

[PATCH 3/4] Return std::string from gdb_bfd_errmsg

[Tom Tromey]

This changes gdb_bfd_errmsg to return a std::string, removing a
cleanup.  This approach may be slightly less efficient than the
previous code, but I don't believe this is very important in this
situation.

gdb/ChangeLog
2018-09-03  Tom Tromey  <tom@tromey.com>

	* utils.h (gdb_bfd_errmsg): Return std::string.
	* exec.c (exec_file_attach): Update.
	* compile/compile-object-load.c (compile_object_load): Update.
	* utils.c (gdb_bfd_errmsg): Return std::string.
---
 gdb/ChangeLog                     |  7 +++++++
 gdb/compile/compile-object-load.c |  3 ++-
 gdb/exec.c                        |  2 +-
 gdb/utils.c                       | 27 +++++++--------------------
 gdb/utils.h                       |  2 +-
 5 files changed, 18 insertions(+), 23 deletions(-)

diff --git a/gdb/compile/compile-object-load.c b/gdb/compile/compile-object-load.c
index 873750b9440..40053d281a1 100644
--- a/gdb/compile/compile-object-load.c
+++ b/gdb/compile/compile-object-load.c
@@ -638,7 +638,8 @@ compile_object_load (const compile_file_names &file_names,
 
   if (!bfd_check_format_matches (abfd.get (), bfd_object, &matching))
     error (_("\"%s\": not in loadable format: %s"),
-          filename.get (), gdb_bfd_errmsg (bfd_get_error (), matching));
+	   filename.get (),
+	   gdb_bfd_errmsg (bfd_get_error (), matching).c_str ());
 
   if ((bfd_get_file_flags (abfd.get ()) & (EXEC_P | DYNAMIC)) != 0)
     error (_("\"%s\": not in object format."), filename.get ());
diff --git a/gdb/exec.c b/gdb/exec.c
index 3023ff7e5aa..6e44b0e821b 100644
--- a/gdb/exec.c
+++ b/gdb/exec.c
@@ -362,7 +362,7 @@ exec_file_attach (const char *filename, int from_tty)
 	  exec_close ();
 	  error (_("\"%s\": not in executable format: %s"),
 		 scratch_pathname,
-		 gdb_bfd_errmsg (bfd_get_error (), matching));
+		 gdb_bfd_errmsg (bfd_get_error (), matching).c_str ());
 	}
 
       if (build_section_table (exec_bfd, &sections, &sections_end))
diff --git a/gdb/utils.c b/gdb/utils.c
index 7a8c80c64ed..d7980fe3a18 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -2921,39 +2921,26 @@ compare_positive_ints (const void *ap, const void *bp)
 #define AMBIGUOUS_MESS2	\
   ".\nUse \"set gnutarget format-name\" to specify the format."
 
-const char *
+std::string
 gdb_bfd_errmsg (bfd_error_type error_tag, char **matching)
 {
-  char *ret, *retp;
-  int ret_len;
   char **p;
 
   /* Check if errmsg just need simple return.  */
   if (error_tag != bfd_error_file_ambiguously_recognized || matching == NULL)
     return bfd_errmsg (error_tag);
 
-  ret_len = strlen (bfd_errmsg (error_tag)) + strlen (AMBIGUOUS_MESS1)
-            + strlen (AMBIGUOUS_MESS2);
-  for (p = matching; *p; p++)
-    ret_len += strlen (*p) + 1;
-  ret = (char *) xmalloc (ret_len + 1);
-  retp = ret;
-  make_cleanup (xfree, ret);
-
-  strcpy (retp, bfd_errmsg (error_tag));
-  retp += strlen (retp);
-
-  strcpy (retp, AMBIGUOUS_MESS1);
-  retp += strlen (retp);
+  std::string ret (bfd_errmsg (error_tag));
+  ret += AMBIGUOUS_MESS1;
 
   for (p = matching; *p; p++)
     {
-      sprintf (retp, " %s", *p);
-      retp += strlen (retp);
+      ret += " ";
+      ret += *p;
     }
-  xfree (matching);
+  ret += AMBIGUOUS_MESS2;
 
-  strcpy (retp, AMBIGUOUS_MESS2);
+  xfree (matching);
 
   return ret;
 }
diff --git a/gdb/utils.h b/gdb/utils.h
index 68523994b94..fa9a59087da 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -115,7 +115,7 @@ compare_cstrings (const char *str1, const char *str2)
    MATCHING, if non-NULL, is the corresponding argument to
    bfd_check_format_matches, and will be freed.  */
 
-extern const char *gdb_bfd_errmsg (bfd_error_type error_tag, char **matching);
+extern std::string gdb_bfd_errmsg (bfd_error_type error_tag, char **matching);
 
 /* Reset the prompt_for_continue clock.  */
 void reset_prompt_for_continue_wait_time (void);
-- 
2.13.6

[PATCH 4/4] Remove cleanup from try_open_exec_file

[Tom Tromey]

This removes a cleanup from try_open_exec_file, using std::string to
manage the storage instead.

gdb/ChangeLog
2018-09-03  Tom Tromey  <tom@tromey.com>

	* exec.c (try_open_exec_file): Use std::string.
---
 gdb/ChangeLog |  4 ++++
 gdb/exec.c    | 11 +++++------
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/gdb/exec.c b/gdb/exec.c
index 6e44b0e821b..615fb2b5dbc 100644
--- a/gdb/exec.c
+++ b/gdb/exec.c
@@ -149,11 +149,8 @@ void
 try_open_exec_file (const char *exec_file_host, struct inferior *inf,
 		    symfile_add_flags add_flags)
 {
-  struct cleanup *old_chain;
   struct gdb_exception prev_err = exception_none;
 
-  old_chain = make_cleanup (free_current_contents, &prev_err.message);
-
   /* exec_file_attach and symbol_file_add_main may throw an error if the file
      cannot be opened either locally or remotely.
 
@@ -165,6 +162,7 @@ try_open_exec_file (const char *exec_file_host, struct inferior *inf,
      Even without a symbol file, the remote-based debugging session should
      continue normally instead of ending abruptly.  Hence we catch thrown
      errors/exceptions in the following code.  */
+  std::string saved_message;
   TRY
     {
       /* We must do this step even if exec_file_host is NULL, so that
@@ -180,7 +178,10 @@ try_open_exec_file (const char *exec_file_host, struct inferior *inf,
 
       /* Save message so it doesn't get trashed by the catch below.  */
       if (err.message != NULL)
-	prev_err.message = xstrdup (err.message);
+	{
+	  saved_message = err.message;
+	  prev_err.message = saved_message.c_str ();
+	}
     }
   END_CATCH
 
@@ -197,8 +198,6 @@ try_open_exec_file (const char *exec_file_host, struct inferior *inf,
 	}
       END_CATCH
     }
-
-  do_cleanups (old_chain);
 }
 
 /* See gdbcore.h.  */
-- 
2.13.6

Re: [PATCH 0/4] Some cleanup removal

[Rainer Orth]

Hi Tom,

> This series removes a few more cleanups.
>
> I tested this on the buildbot; but of course some builders aren't
> really working.  Also, I don't think we have a builder that builds
> procfs.c, so that patch is totally untested -- perhaps I ought to just
> drop it.

I thought about providing Solaris 11/SPARC and x86 builders, especially
since I now have appropriate machines running Solaris 11.4.  However,
I'm uncertain how useful that would be given that there are currently
almost 2000 testsuite failures on x86 and 200+ racy tests.  It would
catch build failures though.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH 0/4] Some cleanup removal

[Tom Tromey]

>>>>> "Rainer" == Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> writes:

Rainer> I thought about providing Solaris 11/SPARC and x86 builders, especially
Rainer> since I now have appropriate machines running Solaris 11.4.  However,
Rainer> I'm uncertain how useful that would be given that there are currently
Rainer> almost 2000 testsuite failures on x86 and 200+ racy tests.  It would
Rainer> catch build failures though.

Just catching build failures would be nice.  Maybe we could configure
this builder to just not run the tests, or maybe only gdb.gdb tests.

Because the builders compare against baseline results, I think it is the
large number of racy tests that is more of a problem than the number of
failures.

Tom

Re: [PATCH 0/4] Some cleanup removal

[Rainer Orth]

Hi Tom,

>>>>>> "Rainer" == Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> writes:
>
> Rainer> I thought about providing Solaris 11/SPARC and x86 builders, especially
> Rainer> since I now have appropriate machines running Solaris 11.4.  However,
> Rainer> I'm uncertain how useful that would be given that there are currently
> Rainer> almost 2000 testsuite failures on x86 and 200+ racy tests.  It would
> Rainer> catch build failures though.
>
> Just catching build failures would be nice.  Maybe we could configure
> this builder to just not run the tests, or maybe only gdb.gdb tests.

fine.  I'll give it a try then.

> Because the builders compare against baseline results, I think it is the
> large number of racy tests that is more of a problem than the number of
> failures.

Given that the vast majority of racy tests is in gdb.threads, maybe it's
enough to exclude that.  Besides, I'm pretty certain that the failures
can be reduced to a handful of root causes at most.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH 0/4] Some cleanup removal

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

Tom> This series removes a few more cleanups.
Tom> I tested this on the buildbot; but of course some builders aren't
Tom> really working.  Also, I don't think we have a builder that builds
Tom> procfs.c, so that patch is totally untested -- perhaps I ought to just
Tom> drop it.

I'm checking these in now.
Rainer tested the procfs.c changes and I have put his fix into the series.

Tom