[3/3] Make relocate_{path,gdb_directory} return std::string

Message ID 20190909180830.215313-4-cbiesinger@google.com
State New, archived
Headers

Commit Message

Terekhov, Mikhail via Gdb-patches Sept. 9, 2019, 6:08 p.m. UTC
  This simplifies memory management. I've also changed some global variables
to std::string accordingly (which store the result of these functions),
but not all because some are used with add_setshow_optional_filename_cmd
which requires a char*.

gdb/ChangeLog:

2019-09-09  Christian Biesinger  <cbiesinger@google.com>

	* auto-load.c (auto_load_expand_dir_vars): Update.
	* defs.h (gdb_datadir): Change to std::string.
	(python_libdir): Likewise.
	(relocate_gdb_directory): Change return type to std::string.
	* guile/guile.c (gdbscm_data_directory): Update.
	(initialize_scheme_side): Update.
	* jit.c (jit_reader_dir): Change to std::string.
	(jit_reader_load_command): Update.
	* main.c (gdb_datadir): Change to std::string.
	(python_libdir): Likewise.
	(set_gdb_data_directory): Update.
	(relocate_path): Change to return std::string.
	(relocate_gdb_directory): Change to return std::string.
	(relocate_gdbinit_path_maybe_in_datadir): Update.
	(captured_main_1): Update.
	* python/python.c (do_start_initialization): Update.
	* top.c (show_gdb_datadir): Update.
	* xml-syscall.c (xml_init_syscalls_info): Update.
	(init_syscalls_info): Update.
---
 gdb/auto-load.c     |  2 +-
 gdb/defs.h          |  8 ++---
 gdb/guile/guile.c   |  5 +--
 gdb/jit.c           |  4 +--
 gdb/main.c          | 84 ++++++++++++++++++++++-----------------------
 gdb/python/python.c |  2 +-
 gdb/top.c           |  2 +-
 gdb/xml-syscall.c   |  7 ++--
 8 files changed, 57 insertions(+), 57 deletions(-)
  

Comments

Tom Tromey Sept. 10, 2019, 3:26 p.m. UTC | #1
>>>>> "Christian" == Christian Biesinger via gdb-patches <gdb-patches@sourceware.org> writes:

Christian> This simplifies memory management. I've also changed some global variables
Christian> to std::string accordingly (which store the result of these functions),
Christian> but not all because some are used with add_setshow_optional_filename_cmd
Christian> which requires a char*.

Thank you.

Christian> -  xfree (gdb_datadir);
Christian> -  gdb_datadir = gdb_realpath (new_datadir).release ();
Christian> +  gdb_datadir = gdb_realpath (new_datadir).get ();

I wonder if using a unique_xmalloc_ptr would be better.
I suppose it doesn't matter hugely, but it would eliminate some of these
double allocations.

Christian> +      gdb::unique_xmalloc_ptr<char> abs_datadir =
Christian> +	gdb_abspath (gdb_datadir.c_str ());

"=" on continuation line.

Christian> -static char *
Christian> +static std::string
Christian>  relocate_path (const char *progname, const char *initial, bool relocatable)
Christian>  {
Christian>    if (relocatable)
Christian> -    return make_relative_prefix (progname, BINDIR, initial);
Christian> -  return xstrdup (initial);
Christian> +    {
Christian> +      char *str = make_relative_prefix (progname, BINDIR, initial);
Christian> +      if (str != nullptr)
Christian> +        return str;

This seems to leak "str".

Christian> +      char *canon_sysroot = lrealpath (dir.c_str ());
 
Changing this to a unique_xmalloc_ptr would mean one less call to
xfree...

Christian> +  debug_file_directory =

"=" on the next line, there is one more of these in this spot.

Tom
  
Terekhov, Mikhail via Gdb-patches Sept. 10, 2019, 7:55 p.m. UTC | #2
On Tue, Sep 10, 2019 at 10:27 AM Tom Tromey <tom@tromey.com> wrote:
>
> >>>>> "Christian" == Christian Biesinger via gdb-patches <gdb-patches@sourceware.org> writes:
>
> Christian> This simplifies memory management. I've also changed some global variables
> Christian> to std::string accordingly (which store the result of these functions),
> Christian> but not all because some are used with add_setshow_optional_filename_cmd
> Christian> which requires a char*.
>
> Thank you.

Thanks for the review!

> Christian> -  xfree (gdb_datadir);
> Christian> -  gdb_datadir = gdb_realpath (new_datadir).release ();
> Christian> +  gdb_datadir = gdb_realpath (new_datadir).get ();
>
> I wonder if using a unique_xmalloc_ptr would be better.
> I suppose it doesn't matter hugely, but it would eliminate some of these
> double allocations.

Do you mean for the return type of relocate_path and
relocate_gdb_directory? Hm.. I can change it if you want, but I'd
rather not -- not all codepaths go through an xmalloc (when
relocatable=false, or relocate_path returns null in
relocate_gdb_directory). So that would add some xstrdup calls that
would not otherwise be necessary.

> Christian> +      gdb::unique_xmalloc_ptr<char> abs_datadir =
> Christian> +    gdb_abspath (gdb_datadir.c_str ());
>
> "=" on continuation line.

Done.

> Christian> -static char *
> Christian> +static std::string
> Christian>  relocate_path (const char *progname, const char *initial, bool relocatable)
> Christian>  {
> Christian>    if (relocatable)
> Christian> -    return make_relative_prefix (progname, BINDIR, initial);
> Christian> -  return xstrdup (initial);
> Christian> +    {
> Christian> +      char *str = make_relative_prefix (progname, BINDIR, initial);
> Christian> +      if (str != nullptr)
> Christian> +        return str;
>
> This seems to leak "str".

Oh, thanks! Fixed using a unique_xmalloc_ptr.

> Christian> +      char *canon_sysroot = lrealpath (dir.c_str ());
>
> Changing this to a unique_xmalloc_ptr would mean one less call to
> xfree...

Thanks, done.

> Christian> +  debug_file_directory =
>
> "=" on the next line, there is one more of these in this spot.

Done.

I'll send a new version with those comments fixed in a moment.

Christian
  
Tom Tromey Sept. 11, 2019, 8:22 p.m. UTC | #3
>>>>> "Christian" == Christian Biesinger via gdb-patches <gdb-patches@sourceware.org> writes:

>> I wonder if using a unique_xmalloc_ptr would be better.
>> I suppose it doesn't matter hugely, but it would eliminate some of these
>> double allocations.

Christian> Do you mean for the return type of relocate_path and
Christian> relocate_gdb_directory? Hm.. I can change it if you want, but I'd
Christian> rather not -- not all codepaths go through an xmalloc (when
Christian> relocatable=false, or relocate_path returns null in
Christian> relocate_gdb_directory). So that would add some xstrdup calls that
Christian> would not otherwise be necessary.

Yeah.  What you did sounds fine.

Tom
  

Patch

diff --git a/gdb/auto-load.c b/gdb/auto-load.c
index 616aeb6fc9..115d5c10e8 100644
--- a/gdb/auto-load.c
+++ b/gdb/auto-load.c
@@ -178,7 +178,7 @@  static std::vector<gdb::unique_xmalloc_ptr<char>>
 auto_load_expand_dir_vars (const char *string)
 {
   char *s = xstrdup (string);
-  substitute_path_component (&s, "$datadir", gdb_datadir);
+  substitute_path_component (&s, "$datadir", gdb_datadir.c_str ());
   substitute_path_component (&s, "$debugdir", debug_file_directory);
 
   if (debug_auto_load && strcmp (s, string) != 0)
diff --git a/gdb/defs.h b/gdb/defs.h
index 14e0a3e1d1..c9a38b60a6 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -115,11 +115,11 @@  extern int dbx_commands;
 extern char *gdb_sysroot;
 
 /* * GDB datadir, used to store data files.  */
-extern char *gdb_datadir;
+extern std::string gdb_datadir;
 
-/* * If non-NULL, the possibly relocated path to python's "lib" directory
+/* * If not empty, the possibly relocated path to python's "lib" directory
    specified with --with-python.  */
-extern char *python_libdir;
+extern std::string python_libdir;
 
 /* * Search path for separate debug files.  */
 extern char *debug_file_directory;
@@ -282,7 +282,7 @@  struct value;
 
 /* This really belong in utils.c (path-utils.c?), but it references some
    globals that are currently only available to main.c.  */
-extern char *relocate_gdb_directory (const char *initial, bool relocatable);
+extern std::string relocate_gdb_directory (const char *initial, bool relocatable);
 
 
 /* Annotation stuff.  */
diff --git a/gdb/guile/guile.c b/gdb/guile/guile.c
index 39bec8724f..defe554f76 100644
--- a/gdb/guile/guile.c
+++ b/gdb/guile/guile.c
@@ -325,7 +325,7 @@  gdbscm_execute_gdb_command (SCM command_scm, SCM rest)
 static SCM
 gdbscm_data_directory (void)
 {
-  return gdbscm_scm_from_c_string (gdb_datadir);
+  return gdbscm_scm_from_c_string (gdb_datadir.c_str ());
 }
 
 /* (guile-data-directory) -> string */
@@ -582,7 +582,8 @@  initialize_scheme_side (void)
 {
   char *boot_scm_path;
 
-  guile_datadir = concat (gdb_datadir, SLASH_STRING, "guile", (char *) NULL);
+  guile_datadir = concat (gdb_datadir.c_str (), SLASH_STRING, "guile",
+			  (char *) NULL);
   boot_scm_path = concat (guile_datadir, SLASH_STRING, "gdb",
 			  SLASH_STRING, boot_scm_filename, (char *) NULL);
 
diff --git a/gdb/jit.c b/gdb/jit.c
index 5fef03700c..4722d6c6ce 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -42,7 +42,7 @@ 
 #include "readline/tilde.h"
 #include "completer.h"
 
-static const char *jit_reader_dir = NULL;
+static std::string jit_reader_dir;
 
 static const struct objfile_data *jit_objfile_data;
 
@@ -216,7 +216,7 @@  jit_reader_load_command (const char *args, int from_tty)
     error (_("JIT reader already loaded.  Run jit-reader-unload first."));
 
   if (!IS_ABSOLUTE_PATH (file.get ()))
-    file.reset (xstrprintf ("%s%s%s", jit_reader_dir, SLASH_STRING,
+    file.reset (xstrprintf ("%s%s%s", jit_reader_dir.c_str (), SLASH_STRING,
 			    file.get ()));
 
   loaded_jit_reader = jit_reader_load (file.get ());
diff --git a/gdb/main.c b/gdb/main.c
index 24aad0ca5a..f9126c7299 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -61,7 +61,7 @@  int dbx_commands = 0;
 char *gdb_sysroot = 0;
 
 /* GDB datadir, used to store data files.  */
-char *gdb_datadir = 0;
+std::string gdb_datadir;
 
 /* Non-zero if GDB_DATADIR was provided on the command line.
    This doesn't track whether data-directory is set later from the
@@ -70,7 +70,7 @@  static int gdb_datadir_provided = 0;
 
 /* If gdb was configured with --with-python=/path,
    the possibly relocated path to python's lib directory.  */
-char *python_libdir = 0;
+std::string python_libdir;
 
 /* Target IO streams.  */
 struct ui_file *gdb_stdtargin;
@@ -121,70 +121,70 @@  set_gdb_data_directory (const char *new_datadir)
   else if (!S_ISDIR (st.st_mode))
     warning (_("%s is not a directory."), new_datadir);
 
-  xfree (gdb_datadir);
-  gdb_datadir = gdb_realpath (new_datadir).release ();
+  gdb_datadir = gdb_realpath (new_datadir).get ();
 
   /* gdb_realpath won't return an absolute path if the path doesn't exist,
      but we still want to record an absolute path here.  If the user entered
      "../foo" and "../foo" doesn't exist then we'll record $(pwd)/../foo which
      isn't canonical, but that's ok.  */
-  if (!IS_ABSOLUTE_PATH (gdb_datadir))
+  if (!IS_ABSOLUTE_PATH (gdb_datadir.c_str ()))
     {
-      gdb::unique_xmalloc_ptr<char> abs_datadir = gdb_abspath (gdb_datadir);
+      gdb::unique_xmalloc_ptr<char> abs_datadir =
+	gdb_abspath (gdb_datadir.c_str ());
 
-      xfree (gdb_datadir);
-      gdb_datadir = abs_datadir.release ();
+      gdb_datadir = abs_datadir.get ();
     }
 }
 
 /* Relocate a file or directory.  PROGNAME is the name by which gdb
    was invoked (i.e., argv[0]).  INITIAL is the default value for the
    file or directory.  RELOCATABLE is true if the value is relocatable,
-   false otherwise.  Returns a newly allocated string; this may return
-   NULL under the same conditions as make_relative_prefix.  */
+   false otherwise.  This may return an empty string under the same
+   conditions as make_relative_prefix returning NULL.  */
 
-static char *
+static std::string
 relocate_path (const char *progname, const char *initial, bool relocatable)
 {
   if (relocatable)
-    return make_relative_prefix (progname, BINDIR, initial);
-  return xstrdup (initial);
+    {
+      char *str = make_relative_prefix (progname, BINDIR, initial);
+      if (str != nullptr)
+        return str;
+      return std::string ();
+    }
+  return initial;
 }
 
 /* Like relocate_path, but specifically checks for a directory.
    INITIAL is relocated according to the rules of relocate_path.  If
    the result is a directory, it is used; otherwise, INITIAL is used.
-   The chosen directory is then canonicalized using lrealpath.  This
-   function always returns a newly-allocated string.  */
+   The chosen directory is then canonicalized using lrealpath.  */
 
-char *
+std::string
 relocate_gdb_directory (const char *initial, bool relocatable)
 {
-  char *dir;
-
-  dir = relocate_path (gdb_program_name, initial, relocatable);
-  if (dir)
+  std::string dir = relocate_path (gdb_program_name, initial, relocatable);
+  if (!dir.empty ())
     {
       struct stat s;
 
-      if (*dir == '\0' || stat (dir, &s) != 0 || !S_ISDIR (s.st_mode))
+      if (stat (dir.c_str (), &s) != 0 || !S_ISDIR (s.st_mode))
 	{
-	  xfree (dir);
-	  dir = NULL;
+	  dir.clear ();
 	}
     }
-  if (!dir)
-    dir = xstrdup (initial);
+  if (dir.empty ())
+    dir = initial;
 
   /* Canonicalize the directory.  */
-  if (*dir)
+  if (!dir.empty ())
     {
-      char *canon_sysroot = lrealpath (dir);
+      char *canon_sysroot = lrealpath (dir.c_str ());
 
       if (canon_sysroot)
 	{
-	  xfree (dir);
 	  dir = canon_sysroot;
+	  xfree (canon_sysroot);
 	}
     }
 
@@ -216,14 +216,9 @@  relocate_gdbinit_path_maybe_in_datadir (const std::string& file)
     }
   else
     {
-      char *relocated = relocate_path (gdb_program_name,
-				       file.c_str (),
-				       SYSTEM_GDBINIT_RELOCATABLE);
-      if (relocated != nullptr)
-	{
-	  relocated_path = relocated;
-	  xfree (relocated);
-	}
+      relocated_path = relocate_path (gdb_program_name,
+				      file.c_str (),
+				      SYSTEM_GDBINIT_RELOCATABLE);
     }
     return relocated_path;
 }
@@ -537,20 +532,23 @@  captured_main_1 (struct captured_main_args *context)
     perror_warning_with_name (_("error finding working directory"));
 
   /* Set the sysroot path.  */
-  gdb_sysroot = relocate_gdb_directory (TARGET_SYSTEM_ROOT,
-					TARGET_SYSTEM_ROOT_RELOCATABLE);
+  gdb_sysroot =
+    xstrdup (relocate_gdb_directory (TARGET_SYSTEM_ROOT,
+				     TARGET_SYSTEM_ROOT_RELOCATABLE).c_str ());
 
-  if (gdb_sysroot == NULL || *gdb_sysroot == '\0')
+  if (*gdb_sysroot == '\0')
     {
       xfree (gdb_sysroot);
       gdb_sysroot = xstrdup (TARGET_SYSROOT_PREFIX);
     }
 
-  debug_file_directory = relocate_gdb_directory (DEBUGDIR,
-						 DEBUGDIR_RELOCATABLE);
+  debug_file_directory =
+    xstrdup (relocate_gdb_directory (DEBUGDIR,
+				     DEBUGDIR_RELOCATABLE).c_str ());
 
-  gdb_datadir = relocate_gdb_directory (GDB_DATADIR,
-					GDB_DATADIR_RELOCATABLE);
+  gdb_datadir =
+    xstrdup (relocate_gdb_directory (GDB_DATADIR,
+				     GDB_DATADIR_RELOCATABLE).c_str ());
 
 #ifdef WITH_PYTHON_PATH
   {
diff --git a/gdb/python/python.c b/gdb/python/python.c
index b309ae91ba..ed7d51b6ef 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -1601,7 +1601,7 @@  do_start_initialization ()
      /foo/lib/pythonX.Y/...
      This must be done before calling Py_Initialize.  */
   gdb::unique_xmalloc_ptr<char> progname
-    (concat (ldirname (python_libdir).c_str (), SLASH_STRING, "bin",
+    (concat (ldirname (python_libdir.c_str ()).c_str (), SLASH_STRING, "bin",
 	      SLASH_STRING, "python", (char *) NULL));
 #ifdef IS_PY3K
   std::string oldloc = setlocale (LC_ALL, NULL);
diff --git a/gdb/top.c b/gdb/top.c
index 9d4ce1fa3b..2b53640af0 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -2038,7 +2038,7 @@  show_gdb_datadir (struct ui_file *file, int from_tty,
 		  struct cmd_list_element *c, const char *value)
 {
   fprintf_filtered (file, _("GDB's data directory is \"%s\".\n"),
-		    gdb_datadir);
+		    gdb_datadir.c_str ());
 }
 
 static void
diff --git a/gdb/xml-syscall.c b/gdb/xml-syscall.c
index d144f82fbf..dc988dfae8 100644
--- a/gdb/xml-syscall.c
+++ b/gdb/xml-syscall.c
@@ -316,7 +316,8 @@  static struct syscalls_info *
 xml_init_syscalls_info (const char *filename)
 {
   gdb::optional<gdb::char_vector> full_file
-    = xml_fetch_content_from_file (filename, gdb_datadir);
+    = xml_fetch_content_from_file (filename,
+				   const_cast<char *>(gdb_datadir.c_str ()));
   if (!full_file)
     return NULL;
 
@@ -336,7 +337,7 @@  init_syscalls_info (struct gdbarch *gdbarch)
   /* Should we re-read the XML info for this target?  */
   if (syscalls_info != NULL && !syscalls_info->my_gdb_datadir.empty ()
       && filename_cmp (syscalls_info->my_gdb_datadir.c_str (),
-		       gdb_datadir) != 0)
+		       gdb_datadir.c_str ()) != 0)
     {
       /* The data-directory changed from the last time we used it.
 	 It means that we have to re-read the XML info.  */
@@ -361,7 +362,7 @@  init_syscalls_info (struct gdbarch *gdbarch)
     {
       if (xml_syscall_file != NULL)
 	warning (_("Could not load the syscall XML file `%s/%s'."),
-		 gdb_datadir, xml_syscall_file);
+		 gdb_datadir.c_str (), xml_syscall_file);
       else
 	warning (_("There is no XML file to open."));