[PATCH 0/3] Refactor some path methods in main.c to use std::string

[PATCH 0/3] Refactor some path methods in main.c to use std::string

[Christian Biesinger via gdb-patches]

Patches 1 and 2 in this series are extracted from my previous patch series
to support a system gdbinit.d directory, but since they are independent from
that and (I hope) less controversial, I moved them here in hopes to get them
committed sooner.
(https://sourceware.org/ml/gdb-patches/2019-08/msg00436.html)

Patch 3 is new.

Christian Biesinger (3):
  Refactor get_init_files to use std::string
  Factor out the code to do the datadir-relocation for gdbinit
  Make relocate_{path,gdb_directory} return std::string

 gdb/auto-load.c     |   2 +-
 gdb/defs.h          |   8 +-
 gdb/guile/guile.c   |   5 +-
 gdb/jit.c           |   4 +-
 gdb/main.c          | 210 ++++++++++++++++++++++----------------------
 gdb/python/python.c |   2 +-
 gdb/top.c           |   2 +-
 gdb/xml-syscall.c   |   7 +-
 8 files changed, 123 insertions(+), 117 deletions(-)

-- 
2.23.0.187.g17f5b7556c-goog

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

[Christian Biesinger via gdb-patches]

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(-)

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);
 
 \f
 /* 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."));
 
-- 
2.23.0.187.g17f5b7556c-goog

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

[Tom Tromey]

>>>>> "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

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

[Christian Biesinger via gdb-patches]

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

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

[Christian Biesinger via gdb-patches]

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          | 89 ++++++++++++++++++++++-----------------------
 gdb/python/python.c |  2 +-
 gdb/top.c           |  2 +-
 gdb/xml-syscall.c   |  7 ++--
 8 files changed, 59 insertions(+), 60 deletions(-)

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);
 
 \f
 /* 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 9e22889590..8c4f906c93 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,71 +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);
+    {
+      gdb::unique_xmalloc_ptr<char> str (make_relative_prefix (progname,
+							       BINDIR,
+							       initial));
+      if (str != nullptr)
+	return str.get ();
+      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);
+      gdb::unique_xmalloc_ptr<char> canon_sysroot (lrealpath (dir.c_str ()));
 
       if (canon_sysroot)
-	{
-	  xfree (dir);
-	  dir = canon_sysroot;
-	}
+	dir = canon_sysroot.get();
     }
 
   return dir;
@@ -220,14 +219,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;
 }
@@ -541,20 +535,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 9c49151db3..9c8c635f72 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -1608,7 +1608,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."));
 
-- 
2.23.0.162.g0b9fbb3734-goog

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

[Tom Tromey]

>>>>> "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*.

Christian> gdb/ChangeLog:

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

Christian> 	* auto-load.c (auto_load_expand_dir_vars): Update.
Christian> 	* defs.h (gdb_datadir): Change to std::string.
Christian> 	(python_libdir): Likewise.
Christian> 	(relocate_gdb_directory): Change return type to std::string.
Christian> 	* guile/guile.c (gdbscm_data_directory): Update.
Christian> 	(initialize_scheme_side): Update.
Christian> 	* jit.c (jit_reader_dir): Change to std::string.
Christian> 	(jit_reader_load_command): Update.
Christian> 	* main.c (gdb_datadir): Change to std::string.
Christian> 	(python_libdir): Likewise.
Christian> 	(set_gdb_data_directory): Update.
Christian> 	(relocate_path): Change to return std::string.
Christian> 	(relocate_gdb_directory): Change to return std::string.
Christian> 	(relocate_gdbinit_path_maybe_in_datadir): Update.
Christian> 	(captured_main_1): Update.
Christian> 	* python/python.c (do_start_initialization): Update.
Christian> 	* top.c (show_gdb_datadir): Update.
Christian> 	* xml-syscall.c (xml_init_syscalls_info): Update.
Christian> 	(init_syscalls_info): Update.

Thanks, this is ok.

Tom

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

[Tom Tromey]

>>>>> "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 2/3] Factor out the code to do the datadir-relocation for gdbinit

[Christian Biesinger via gdb-patches]

This simplifies get_init_files and makes it possible to reuse
this code in an upcoming patch for SYSTEM_GDBINIT_DIR.

gdb/ChangeLog:

2019-08-20  Christian Biesinger  <cbiesinger@google.com>

	* main.c (relocate_gdbinit_path_maybe_in_datadir): Factor this code
	out of get_init_files.
	(get_init_files): Update.
---
 gdb/main.c | 70 ++++++++++++++++++++++++++++++------------------------
 1 file changed, 39 insertions(+), 31 deletions(-)

diff --git a/gdb/main.c b/gdb/main.c
index d27b124fa3..24aad0ca5a 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -191,6 +191,43 @@ relocate_gdb_directory (const char *initial, bool relocatable)
   return dir;
 }
 
+static std::string
+relocate_gdbinit_path_maybe_in_datadir (const std::string& file)
+{
+  size_t datadir_len = strlen (GDB_DATADIR);
+
+  std::string relocated_path;
+
+  /* If SYSTEM_GDBINIT lives in data-directory, and data-directory
+     has been provided, search for SYSTEM_GDBINIT there.  */
+  if (gdb_datadir_provided
+      && datadir_len < file.length ()
+      && filename_ncmp (file.c_str (), GDB_DATADIR, datadir_len) == 0
+      && IS_DIR_SEPARATOR (file[datadir_len]))
+    {
+      /* Append the part of SYSTEM_GDBINIT that follows GDB_DATADIR
+	 to gdb_datadir.  */
+
+      size_t start = datadir_len;
+      for (; IS_DIR_SEPARATOR (file[start]); ++start)
+	continue;
+      relocated_path = std::string (gdb_datadir) + SLASH_STRING +
+	file.substr(start);
+    }
+  else
+    {
+      char *relocated = relocate_path (gdb_program_name,
+				       file.c_str (),
+				       SYSTEM_GDBINIT_RELOCATABLE);
+      if (relocated != nullptr)
+	{
+	  relocated_path = relocated;
+	  xfree (relocated);
+	}
+    }
+    return relocated_path;
+}
+
 /* Compute the locations of init files that GDB should source and
    return them in SYSTEM_GDBINIT, HOME_GDBINIT, LOCAL_GDBINIT.  If
    there is no system gdbinit (resp. home gdbinit and local gdbinit)
@@ -212,37 +249,8 @@ get_init_files (std::string *system_gdbinit,
 
       if (SYSTEM_GDBINIT[0])
 	{
-	  size_t datadir_len = strlen (GDB_DATADIR);
-	  size_t sys_gdbinit_len = strlen (SYSTEM_GDBINIT);
-	  std::string relocated_sysgdbinit;
-
-	  /* If SYSTEM_GDBINIT lives in data-directory, and data-directory
-	     has been provided, search for SYSTEM_GDBINIT there.  */
-	  if (gdb_datadir_provided
-	      && datadir_len < sys_gdbinit_len
-	      && filename_ncmp (SYSTEM_GDBINIT, GDB_DATADIR, datadir_len) == 0
-	      && IS_DIR_SEPARATOR (SYSTEM_GDBINIT[datadir_len]))
-	    {
-	      /* Append the part of SYSTEM_GDBINIT that follows GDB_DATADIR
-		 to gdb_datadir.  */
-
-	      size_t start = datadir_len;
-	      for (; IS_DIR_SEPARATOR (SYSTEM_GDBINIT[start]); ++start)
-		continue;
-	      relocated_sysgdbinit = std::string (gdb_datadir) + SLASH_STRING +
-		&SYSTEM_GDBINIT[start];
-	    }
-	  else
-	    {
-	      char *relocated = relocate_path (gdb_program_name,
-					       SYSTEM_GDBINIT,
-					       SYSTEM_GDBINIT_RELOCATABLE);
-	      if (relocated != nullptr)
-	        {
-		  relocated_sysgdbinit = relocated;
-		  xfree (relocated);
-		}
-	    }
+	  std::string relocated_sysgdbinit =
+	    relocate_gdbinit_path_maybe_in_datadir (SYSTEM_GDBINIT);
 	  if (!relocated_sysgdbinit.empty () &&
 	      stat (relocated_sysgdbinit.c_str (), &s) == 0)
 	    sysgdbinit = relocated_sysgdbinit;
-- 
2.23.0.187.g17f5b7556c-goog

Re: [PATCH 2/3] Factor out the code to do the datadir-relocation for gdbinit

[Tom Tromey]

>>>>> "Christian" == Christian Biesinger via gdb-patches <gdb-patches@sourceware.org> writes:

Christian> This simplifies get_init_files and makes it possible to reuse
Christian> this code in an upcoming patch for SYSTEM_GDBINIT_DIR.

 
Christian> +static std::string
Christian> +relocate_gdbinit_path_maybe_in_datadir (const std::string& file)
Christian> +{

New functions should have an introductory comment.

Christian> +      for (; IS_DIR_SEPARATOR (file[start]); ++start)
Christian> +	continue;

Just omit the "continue" and leave a bare ";".

Christian> +      relocated_path = std::string (gdb_datadir) + SLASH_STRING +
Christian> +	file.substr(start);

This needs parens on the RHS and should be indented differently.
Also, space before the "(" after "substr".

Christian> +	  std::string relocated_sysgdbinit =
Christian> +	    relocate_gdbinit_path_maybe_in_datadir (SYSTEM_GDBINIT);

"=" at start of continuation line.

Tom

Re: [PATCH 2/3] Factor out the code to do the datadir-relocation for gdbinit

[Christian Biesinger via gdb-patches]

On Tue, Sep 10, 2019 at 10:15 AM Tom Tromey <tom@tromey.com> wrote:
>
> >>>>> "Christian" == Christian Biesinger via gdb-patches <gdb-patches@sourceware.org> writes:
>
> Christian> This simplifies get_init_files and makes it possible to reuse
> Christian> this code in an upcoming patch for SYSTEM_GDBINIT_DIR.
>
>
> Christian> +static std::string
> Christian> +relocate_gdbinit_path_maybe_in_datadir (const std::string& file)
> Christian> +{
>
> New functions should have an introductory comment.

Done.

> Christian> +      for (; IS_DIR_SEPARATOR (file[start]); ++start)
> Christian> +    continue;
>
> Just omit the "continue" and leave a bare ";".

Done.

> Christian> +      relocated_path = std::string (gdb_datadir) + SLASH_STRING +
> Christian> +    file.substr(start);
>
> This needs parens on the RHS and should be indented differently.
> Also, space before the "(" after "substr".

Done.

> Christian> +      std::string relocated_sysgdbinit =
> Christian> +        relocate_gdbinit_path_maybe_in_datadir (SYSTEM_GDBINIT);
>
> "=" at start of continuation line.

Done.

[PATCH 2/3 v2] Factor out the code to do the datadir-relocation for gdbinit

[Christian Biesinger via gdb-patches]

This simplifies get_init_files and makes it possible to reuse
this code in an upcoming patch for SYSTEM_GDBINIT_DIR.

gdb/ChangeLog:

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

	* main.c (relocate_gdbinit_path_maybe_in_datadir): Factor this code
	out of get_init_files.
	(get_init_files): Update.
---
 gdb/main.c | 74 +++++++++++++++++++++++++++++++-----------------------
 1 file changed, 43 insertions(+), 31 deletions(-)

diff --git a/gdb/main.c b/gdb/main.c
index e32ed62270..9e22889590 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -191,6 +191,47 @@ relocate_gdb_directory (const char *initial, bool relocatable)
   return dir;
 }
 
+/* Given a gdbinit path in FILE, adjusts it according to the gdb_datadir
+   parameter if it is in the data dir, or passes it through relocate_path
+   otherwise.  */
+
+static std::string
+relocate_gdbinit_path_maybe_in_datadir (const std::string& file)
+{
+  size_t datadir_len = strlen (GDB_DATADIR);
+
+  std::string relocated_path;
+
+  /* If SYSTEM_GDBINIT lives in data-directory, and data-directory
+     has been provided, search for SYSTEM_GDBINIT there.  */
+  if (gdb_datadir_provided
+      && datadir_len < file.length ()
+      && filename_ncmp (file.c_str (), GDB_DATADIR, datadir_len) == 0
+      && IS_DIR_SEPARATOR (file[datadir_len]))
+    {
+      /* Append the part of SYSTEM_GDBINIT that follows GDB_DATADIR
+	 to gdb_datadir.  */
+
+      size_t start = datadir_len;
+      for (; IS_DIR_SEPARATOR (file[start]); ++start)
+	;
+      relocated_path = (std::string (gdb_datadir) + SLASH_STRING
+			+ file.substr (start));
+    }
+  else
+    {
+      char *relocated = relocate_path (gdb_program_name,
+				       file.c_str (),
+				       SYSTEM_GDBINIT_RELOCATABLE);
+      if (relocated != nullptr)
+	{
+	  relocated_path = relocated;
+	  xfree (relocated);
+	}
+    }
+    return relocated_path;
+}
+
 /* Compute the locations of init files that GDB should source and
    return them in SYSTEM_GDBINIT, HOME_GDBINIT, LOCAL_GDBINIT.  If
    there is no system gdbinit (resp. home gdbinit and local gdbinit)
@@ -212,37 +253,8 @@ get_init_files (std::string *system_gdbinit,
 
       if (SYSTEM_GDBINIT[0])
 	{
-	  size_t datadir_len = strlen (GDB_DATADIR);
-	  size_t sys_gdbinit_len = strlen (SYSTEM_GDBINIT);
-	  std::string relocated_sysgdbinit;
-
-	  /* If SYSTEM_GDBINIT lives in data-directory, and data-directory
-	     has been provided, search for SYSTEM_GDBINIT there.  */
-	  if (gdb_datadir_provided
-	      && datadir_len < sys_gdbinit_len
-	      && filename_ncmp (SYSTEM_GDBINIT, GDB_DATADIR, datadir_len) == 0
-	      && IS_DIR_SEPARATOR (SYSTEM_GDBINIT[datadir_len]))
-	    {
-	      /* Append the part of SYSTEM_GDBINIT that follows GDB_DATADIR
-		 to gdb_datadir.  */
-
-	      size_t start = datadir_len;
-	      for (; IS_DIR_SEPARATOR (SYSTEM_GDBINIT[start]); ++start)
-		;
-	      relocated_sysgdbinit = (std::string (gdb_datadir) + SLASH_STRING
-				      + &SYSTEM_GDBINIT[start]);
-	    }
-	  else
-	    {
-	      char *relocated = relocate_path (gdb_program_name,
-					       SYSTEM_GDBINIT,
-					       SYSTEM_GDBINIT_RELOCATABLE);
-	      if (relocated != nullptr)
-	        {
-		  relocated_sysgdbinit = relocated;
-		  xfree (relocated);
-		}
-	    }
+	  std::string relocated_sysgdbinit
+	    = relocate_gdbinit_path_maybe_in_datadir (SYSTEM_GDBINIT);
 	  if (!relocated_sysgdbinit.empty ()
 	      && stat (relocated_sysgdbinit.c_str (), &s) == 0)
 	    sysgdbinit = relocated_sysgdbinit;
-- 
2.23.0.162.g0b9fbb3734-goog

Re: [PATCH 2/3 v2] Factor out the code to do the datadir-relocation for gdbinit

[Tom Tromey]

>>>>> "Christian" == Christian Biesinger via gdb-patches <gdb-patches@sourceware.org> writes:

Christian> This simplifies get_init_files and makes it possible to reuse
Christian> this code in an upcoming patch for SYSTEM_GDBINIT_DIR.

Christian> gdb/ChangeLog:

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

Christian> 	* main.c (relocate_gdbinit_path_maybe_in_datadir): Factor this code
Christian> 	out of get_init_files.
Christian> 	(get_init_files): Update.

Thanks, this is ok.

Tom

[PATCH 1/3] Refactor get_init_files to use std::string

[Christian Biesinger via gdb-patches]

To avoid manual memory management.

Tested on buildbot.

gdb/ChangeLog:

2019-08-20  Christian Biesinger  <cbiesinger@google.com>

	* main.c (get_init_files): Change to use std::string.
	(captured_main_1): Update.
	(print_gdb_help): Update.
---
 gdb/main.c | 102 ++++++++++++++++++++++++++---------------------------
 1 file changed, 50 insertions(+), 52 deletions(-)

diff --git a/gdb/main.c b/gdb/main.c
index 129c57c1d1..d27b124fa3 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -195,27 +195,26 @@ relocate_gdb_directory (const char *initial, bool relocatable)
    return them in SYSTEM_GDBINIT, HOME_GDBINIT, LOCAL_GDBINIT.  If
    there is no system gdbinit (resp. home gdbinit and local gdbinit)
    to be loaded, then SYSTEM_GDBINIT (resp. HOME_GDBINIT and
-   LOCAL_GDBINIT) is set to NULL.  */
+   LOCAL_GDBINIT) is set to the empty string.  */
 static void
-get_init_files (const char **system_gdbinit,
-		const char **home_gdbinit,
-		const char **local_gdbinit)
+get_init_files (std::string *system_gdbinit,
+		std::string *home_gdbinit,
+		std::string *local_gdbinit)
 {
-  static const char *sysgdbinit = NULL;
-  static char *homeinit = NULL;
-  static const char *localinit = NULL;
+  static std::string sysgdbinit;
+  static std::string homeinit;
+  static std::string localinit;
   static int initialized = 0;
 
   if (!initialized)
     {
       struct stat homebuf, cwdbuf, s;
-      const char *homedir;
 
       if (SYSTEM_GDBINIT[0])
 	{
-	  int datadir_len = strlen (GDB_DATADIR);
-	  int sys_gdbinit_len = strlen (SYSTEM_GDBINIT);
-	  char *relocated_sysgdbinit;
+	  size_t datadir_len = strlen (GDB_DATADIR);
+	  size_t sys_gdbinit_len = strlen (SYSTEM_GDBINIT);
+	  std::string relocated_sysgdbinit;
 
 	  /* If SYSTEM_GDBINIT lives in data-directory, and data-directory
 	     has been provided, search for SYSTEM_GDBINIT there.  */
@@ -226,28 +225,30 @@ get_init_files (const char **system_gdbinit,
 	    {
 	      /* Append the part of SYSTEM_GDBINIT that follows GDB_DATADIR
 		 to gdb_datadir.  */
-	      char *tmp_sys_gdbinit = xstrdup (&SYSTEM_GDBINIT[datadir_len]);
-	      char *p;
 
-	      for (p = tmp_sys_gdbinit; IS_DIR_SEPARATOR (*p); ++p)
+	      size_t start = datadir_len;
+	      for (; IS_DIR_SEPARATOR (SYSTEM_GDBINIT[start]); ++start)
 		continue;
-	      relocated_sysgdbinit = concat (gdb_datadir, SLASH_STRING, p,
-					     (char *) NULL);
-	      xfree (tmp_sys_gdbinit);
+	      relocated_sysgdbinit = std::string (gdb_datadir) + SLASH_STRING +
+		&SYSTEM_GDBINIT[start];
 	    }
 	  else
 	    {
-	      relocated_sysgdbinit = relocate_path (gdb_program_name,
-						    SYSTEM_GDBINIT,
-						    SYSTEM_GDBINIT_RELOCATABLE);
+	      char *relocated = relocate_path (gdb_program_name,
+					       SYSTEM_GDBINIT,
+					       SYSTEM_GDBINIT_RELOCATABLE);
+	      if (relocated != nullptr)
+	        {
+		  relocated_sysgdbinit = relocated;
+		  xfree (relocated);
+		}
 	    }
-	  if (relocated_sysgdbinit && stat (relocated_sysgdbinit, &s) == 0)
+	  if (!relocated_sysgdbinit.empty () &&
+	      stat (relocated_sysgdbinit.c_str (), &s) == 0)
 	    sysgdbinit = relocated_sysgdbinit;
-	  else
-	    xfree (relocated_sysgdbinit);
 	}
 
-      homedir = getenv ("HOME");
+      const char *homedir = getenv ("HOME");
 
       /* If the .gdbinit file in the current directory is the same as
 	 the $HOME/.gdbinit file, it should not be sourced.  homebuf
@@ -260,17 +261,16 @@ get_init_files (const char **system_gdbinit,
 
       if (homedir)
 	{
-	  homeinit = xstrprintf ("%s/%s", homedir, GDBINIT);
-	  if (stat (homeinit, &homebuf) != 0)
+	  homeinit = std::string (homedir) + SLASH_STRING + GDBINIT;
+	  if (stat (homeinit.c_str (), &homebuf) != 0)
 	    {
-	      xfree (homeinit);
-	      homeinit = NULL;
+	      homeinit = "";
 	    }
 	}
 
       if (stat (GDBINIT, &cwdbuf) == 0)
 	{
-	  if (!homeinit
+	  if (homeinit.empty ()
 	      || memcmp ((char *) &homebuf, (char *) &cwdbuf,
 			 sizeof (struct stat)))
 	    localinit = GDBINIT;
@@ -470,11 +470,6 @@ captured_main_1 (struct captured_main_args *context)
   /* All arguments of --directory option.  */
   std::vector<char *> dirarg;
 
-  /* gdb init files.  */
-  const char *system_gdbinit;
-  const char *home_gdbinit;
-  const char *local_gdbinit;
-
   int i;
   int save_auto_load;
   int ret = 1;
@@ -908,6 +903,9 @@ captured_main_1 (struct captured_main_args *context)
   /* Lookup gdbinit files.  Note that the gdbinit file name may be
      overriden during file initialization, so get_init_files should be
      called after gdb_init.  */
+  std::string system_gdbinit;
+  std::string home_gdbinit;
+  std::string local_gdbinit;
   get_init_files (&system_gdbinit, &home_gdbinit, &local_gdbinit);
 
   /* Do these (and anything which might call wrap_here or *_filtered)
@@ -984,16 +982,16 @@ captured_main_1 (struct captured_main_args *context)
      This is done *before* all the command line arguments are
      processed; it sets global parameters, which are independent of
      what file you are debugging or what directory you are in.  */
-  if (system_gdbinit && !inhibit_gdbinit)
-    ret = catch_command_errors (source_script, system_gdbinit, 0);
+  if (!system_gdbinit.empty () && !inhibit_gdbinit)
+    ret = catch_command_errors (source_script, system_gdbinit.c_str (), 0);
 
   /* Read and execute $HOME/.gdbinit file, if it exists.  This is done
      *before* all the command line arguments are processed; it sets
      global parameters, which are independent of what file you are
      debugging or what directory you are in.  */
 
-  if (home_gdbinit && !inhibit_gdbinit && !inhibit_home_gdbinit)
-    ret = catch_command_errors (source_script, home_gdbinit, 0);
+  if (!home_gdbinit.empty () && !inhibit_gdbinit && !inhibit_home_gdbinit)
+    ret = catch_command_errors (source_script, home_gdbinit.c_str (), 0);
 
   /* Process '-ix' and '-iex' options early.  */
   for (i = 0; i < cmdarg_vec.size (); i++)
@@ -1096,20 +1094,20 @@ captured_main_1 (struct captured_main_args *context)
 
   /* Read the .gdbinit file in the current directory, *if* it isn't
      the same as the $HOME/.gdbinit file (it should exist, also).  */
-  if (local_gdbinit)
+  if (!local_gdbinit.empty ())
     {
       auto_load_local_gdbinit_pathname
-	= gdb_realpath (local_gdbinit).release ();
+	= gdb_realpath (local_gdbinit.c_str ()).release ();
 
       if (!inhibit_gdbinit && auto_load_local_gdbinit
-	  && file_is_auto_load_safe (local_gdbinit,
+	  && file_is_auto_load_safe (local_gdbinit.c_str (),
 				     _("auto-load: Loading .gdbinit "
 				       "file \"%s\".\n"),
-				     local_gdbinit))
+				     local_gdbinit.c_str ()))
 	{
 	  auto_load_local_gdbinit_loaded = 1;
 
-	  ret = catch_command_errors (source_script, local_gdbinit, 0);
+	  ret = catch_command_errors (source_script, local_gdbinit.c_str (), 0);
 	}
     }
 
@@ -1203,9 +1201,9 @@ gdb_main (struct captured_main_args *args)
 static void
 print_gdb_help (struct ui_file *stream)
 {
-  const char *system_gdbinit;
-  const char *home_gdbinit;
-  const char *local_gdbinit;
+  std::string system_gdbinit;
+  std::string home_gdbinit;
+  std::string local_gdbinit;
 
   get_init_files (&system_gdbinit, &home_gdbinit, &local_gdbinit);
 
@@ -1283,18 +1281,18 @@ Other options:\n\n\
   fputs_unfiltered (_("\n\
 At startup, GDB reads the following init files and executes their commands:\n\
 "), stream);
-  if (system_gdbinit)
+  if (!system_gdbinit.empty ())
     fprintf_unfiltered (stream, _("\
    * system-wide init file: %s\n\
-"), system_gdbinit);
-  if (home_gdbinit)
+"), system_gdbinit.c_str ());
+  if (!home_gdbinit.empty ())
     fprintf_unfiltered (stream, _("\
    * user-specific init file: %s\n\
-"), home_gdbinit);
-  if (local_gdbinit)
+"), home_gdbinit.c_str ());
+  if (!local_gdbinit.empty ())
     fprintf_unfiltered (stream, _("\
    * local init file (see also 'set auto-load local-gdbinit'): ./%s\n\
-"), local_gdbinit);
+"), local_gdbinit.c_str ());
   fputs_unfiltered (_("\n\
 For more information, type \"help\" from within GDB, or consult the\n\
 GDB manual (available as on-line info or a printed manual).\n\
-- 
2.23.0.187.g17f5b7556c-goog

Re: [PATCH 1/3] Refactor get_init_files to use std::string

[Tom Tromey]

>>>>> "Christian" == Christian Biesinger via gdb-patches <gdb-patches@sourceware.org> writes:

Christian> To avoid manual memory management.
Christian> Tested on buildbot.

Thank you for the patch.

Christian> +	      relocated_sysgdbinit = std::string (gdb_datadir) + SLASH_STRING +
Christian> +		&SYSTEM_GDBINIT[start];

This needs parens around the RHS so that the indentation is correct
according to GNU standards.

Christian> +	  if (!relocated_sysgdbinit.empty () &&

&& on the start of a line, not the end.

This is ok with those nits fixed.

Tom

[PATCH 1/3 v2] Refactor get_init_files to use std::string

[Christian Biesinger via gdb-patches]

[Thanks, this is the patch with the nits fixed and a couple of other style
 changes per your comments on patch 2.]

To avoid manual memory management.

Tested on buildbot.

gdb/ChangeLog:

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

	* main.c (get_init_files): Change to use std::string.
	(captured_main_1): Update.
	(print_gdb_help): Update.
---
 gdb/main.c | 104 ++++++++++++++++++++++++++---------------------------
 1 file changed, 51 insertions(+), 53 deletions(-)

diff --git a/gdb/main.c b/gdb/main.c
index 129c57c1d1..e32ed62270 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -195,27 +195,26 @@ relocate_gdb_directory (const char *initial, bool relocatable)
    return them in SYSTEM_GDBINIT, HOME_GDBINIT, LOCAL_GDBINIT.  If
    there is no system gdbinit (resp. home gdbinit and local gdbinit)
    to be loaded, then SYSTEM_GDBINIT (resp. HOME_GDBINIT and
-   LOCAL_GDBINIT) is set to NULL.  */
+   LOCAL_GDBINIT) is set to the empty string.  */
 static void
-get_init_files (const char **system_gdbinit,
-		const char **home_gdbinit,
-		const char **local_gdbinit)
+get_init_files (std::string *system_gdbinit,
+		std::string *home_gdbinit,
+		std::string *local_gdbinit)
 {
-  static const char *sysgdbinit = NULL;
-  static char *homeinit = NULL;
-  static const char *localinit = NULL;
+  static std::string sysgdbinit;
+  static std::string homeinit;
+  static std::string localinit;
   static int initialized = 0;
 
   if (!initialized)
     {
       struct stat homebuf, cwdbuf, s;
-      const char *homedir;
 
       if (SYSTEM_GDBINIT[0])
 	{
-	  int datadir_len = strlen (GDB_DATADIR);
-	  int sys_gdbinit_len = strlen (SYSTEM_GDBINIT);
-	  char *relocated_sysgdbinit;
+	  size_t datadir_len = strlen (GDB_DATADIR);
+	  size_t sys_gdbinit_len = strlen (SYSTEM_GDBINIT);
+	  std::string relocated_sysgdbinit;
 
 	  /* If SYSTEM_GDBINIT lives in data-directory, and data-directory
 	     has been provided, search for SYSTEM_GDBINIT there.  */
@@ -226,28 +225,30 @@ get_init_files (const char **system_gdbinit,
 	    {
 	      /* Append the part of SYSTEM_GDBINIT that follows GDB_DATADIR
 		 to gdb_datadir.  */
-	      char *tmp_sys_gdbinit = xstrdup (&SYSTEM_GDBINIT[datadir_len]);
-	      char *p;
 
-	      for (p = tmp_sys_gdbinit; IS_DIR_SEPARATOR (*p); ++p)
-		continue;
-	      relocated_sysgdbinit = concat (gdb_datadir, SLASH_STRING, p,
-					     (char *) NULL);
-	      xfree (tmp_sys_gdbinit);
+	      size_t start = datadir_len;
+	      for (; IS_DIR_SEPARATOR (SYSTEM_GDBINIT[start]); ++start)
+		;
+	      relocated_sysgdbinit = (std::string (gdb_datadir) + SLASH_STRING
+				      + &SYSTEM_GDBINIT[start]);
 	    }
 	  else
 	    {
-	      relocated_sysgdbinit = relocate_path (gdb_program_name,
-						    SYSTEM_GDBINIT,
-						    SYSTEM_GDBINIT_RELOCATABLE);
+	      char *relocated = relocate_path (gdb_program_name,
+					       SYSTEM_GDBINIT,
+					       SYSTEM_GDBINIT_RELOCATABLE);
+	      if (relocated != nullptr)
+	        {
+		  relocated_sysgdbinit = relocated;
+		  xfree (relocated);
+		}
 	    }
-	  if (relocated_sysgdbinit && stat (relocated_sysgdbinit, &s) == 0)
+	  if (!relocated_sysgdbinit.empty ()
+	      && stat (relocated_sysgdbinit.c_str (), &s) == 0)
 	    sysgdbinit = relocated_sysgdbinit;
-	  else
-	    xfree (relocated_sysgdbinit);
 	}
 
-      homedir = getenv ("HOME");
+      const char *homedir = getenv ("HOME");
 
       /* If the .gdbinit file in the current directory is the same as
 	 the $HOME/.gdbinit file, it should not be sourced.  homebuf
@@ -260,17 +261,16 @@ get_init_files (const char **system_gdbinit,
 
       if (homedir)
 	{
-	  homeinit = xstrprintf ("%s/%s", homedir, GDBINIT);
-	  if (stat (homeinit, &homebuf) != 0)
+	  homeinit = std::string (homedir) + SLASH_STRING + GDBINIT;
+	  if (stat (homeinit.c_str (), &homebuf) != 0)
 	    {
-	      xfree (homeinit);
-	      homeinit = NULL;
+	      homeinit = "";
 	    }
 	}
 
       if (stat (GDBINIT, &cwdbuf) == 0)
 	{
-	  if (!homeinit
+	  if (homeinit.empty ()
 	      || memcmp ((char *) &homebuf, (char *) &cwdbuf,
 			 sizeof (struct stat)))
 	    localinit = GDBINIT;
@@ -470,11 +470,6 @@ captured_main_1 (struct captured_main_args *context)
   /* All arguments of --directory option.  */
   std::vector<char *> dirarg;
 
-  /* gdb init files.  */
-  const char *system_gdbinit;
-  const char *home_gdbinit;
-  const char *local_gdbinit;
-
   int i;
   int save_auto_load;
   int ret = 1;
@@ -908,6 +903,9 @@ captured_main_1 (struct captured_main_args *context)
   /* Lookup gdbinit files.  Note that the gdbinit file name may be
      overriden during file initialization, so get_init_files should be
      called after gdb_init.  */
+  std::string system_gdbinit;
+  std::string home_gdbinit;
+  std::string local_gdbinit;
   get_init_files (&system_gdbinit, &home_gdbinit, &local_gdbinit);
 
   /* Do these (and anything which might call wrap_here or *_filtered)
@@ -984,16 +982,16 @@ captured_main_1 (struct captured_main_args *context)
      This is done *before* all the command line arguments are
      processed; it sets global parameters, which are independent of
      what file you are debugging or what directory you are in.  */
-  if (system_gdbinit && !inhibit_gdbinit)
-    ret = catch_command_errors (source_script, system_gdbinit, 0);
+  if (!system_gdbinit.empty () && !inhibit_gdbinit)
+    ret = catch_command_errors (source_script, system_gdbinit.c_str (), 0);
 
   /* Read and execute $HOME/.gdbinit file, if it exists.  This is done
      *before* all the command line arguments are processed; it sets
      global parameters, which are independent of what file you are
      debugging or what directory you are in.  */
 
-  if (home_gdbinit && !inhibit_gdbinit && !inhibit_home_gdbinit)
-    ret = catch_command_errors (source_script, home_gdbinit, 0);
+  if (!home_gdbinit.empty () && !inhibit_gdbinit && !inhibit_home_gdbinit)
+    ret = catch_command_errors (source_script, home_gdbinit.c_str (), 0);
 
   /* Process '-ix' and '-iex' options early.  */
   for (i = 0; i < cmdarg_vec.size (); i++)
@@ -1096,20 +1094,20 @@ captured_main_1 (struct captured_main_args *context)
 
   /* Read the .gdbinit file in the current directory, *if* it isn't
      the same as the $HOME/.gdbinit file (it should exist, also).  */
-  if (local_gdbinit)
+  if (!local_gdbinit.empty ())
     {
       auto_load_local_gdbinit_pathname
-	= gdb_realpath (local_gdbinit).release ();
+	= gdb_realpath (local_gdbinit.c_str ()).release ();
 
       if (!inhibit_gdbinit && auto_load_local_gdbinit
-	  && file_is_auto_load_safe (local_gdbinit,
+	  && file_is_auto_load_safe (local_gdbinit.c_str (),
 				     _("auto-load: Loading .gdbinit "
 				       "file \"%s\".\n"),
-				     local_gdbinit))
+				     local_gdbinit.c_str ()))
 	{
 	  auto_load_local_gdbinit_loaded = 1;
 
-	  ret = catch_command_errors (source_script, local_gdbinit, 0);
+	  ret = catch_command_errors (source_script, local_gdbinit.c_str (), 0);
 	}
     }
 
@@ -1203,9 +1201,9 @@ gdb_main (struct captured_main_args *args)
 static void
 print_gdb_help (struct ui_file *stream)
 {
-  const char *system_gdbinit;
-  const char *home_gdbinit;
-  const char *local_gdbinit;
+  std::string system_gdbinit;
+  std::string home_gdbinit;
+  std::string local_gdbinit;
 
   get_init_files (&system_gdbinit, &home_gdbinit, &local_gdbinit);
 
@@ -1283,18 +1281,18 @@ Other options:\n\n\
   fputs_unfiltered (_("\n\
 At startup, GDB reads the following init files and executes their commands:\n\
 "), stream);
-  if (system_gdbinit)
+  if (!system_gdbinit.empty ())
     fprintf_unfiltered (stream, _("\
    * system-wide init file: %s\n\
-"), system_gdbinit);
-  if (home_gdbinit)
+"), system_gdbinit.c_str ());
+  if (!home_gdbinit.empty ())
     fprintf_unfiltered (stream, _("\
    * user-specific init file: %s\n\
-"), home_gdbinit);
-  if (local_gdbinit)
+"), home_gdbinit.c_str ());
+  if (!local_gdbinit.empty ())
     fprintf_unfiltered (stream, _("\
    * local init file (see also 'set auto-load local-gdbinit'): ./%s\n\
-"), local_gdbinit);
+"), local_gdbinit.c_str ());
   fputs_unfiltered (_("\n\
 For more information, type \"help\" from within GDB, or consult the\n\
 GDB manual (available as on-line info or a printed manual).\n\
-- 
2.23.0.162.g0b9fbb3734-goog

Re: [PATCH 1/3 v2] Refactor get_init_files to use std::string

[Tom Tromey]

>>>>> "Christian" == Christian Biesinger via gdb-patches <gdb-patches@sourceware.org> writes:

Christian> [Thanks, this is the patch with the nits fixed and a couple of other style
Christian>  changes per your comments on patch 2.]

Christian> To avoid manual memory management.

Christian> Tested on buildbot.

Christian> gdb/ChangeLog:

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

Christian> 	* main.c (get_init_files): Change to use std::string.
Christian> 	(captured_main_1): Update.
Christian> 	(print_gdb_help): Update.

FAOD, this is ok.  Thank you.

Tom

Re: [PATCH 0/3] Refactor some path methods in main.c to use std::string

[Christian Biesinger via gdb-patches]

I pushed these three patches:

Total 24 (delta 21), reused 0 (delta 0)
To ssh://sourceware.org/git/binutils-gdb.git
   9cab7ecda2..f2aec7f6d1  HEAD -> master

Christian

On Mon, Sep 9, 2019 at 1:08 PM Christian Biesinger
<cbiesinger@google.com> wrote:
>
> Patches 1 and 2 in this series are extracted from my previous patch series
> to support a system gdbinit.d directory, but since they are independent from
> that and (I hope) less controversial, I moved them here in hopes to get them
> committed sooner.
> (https://sourceware.org/ml/gdb-patches/2019-08/msg00436.html)
>
> Patch 3 is new.
>
> Christian Biesinger (3):
>   Refactor get_init_files to use std::string
>   Factor out the code to do the datadir-relocation for gdbinit
>   Make relocate_{path,gdb_directory} return std::string
>
>  gdb/auto-load.c     |   2 +-
>  gdb/defs.h          |   8 +-
>  gdb/guile/guile.c   |   5 +-
>  gdb/jit.c           |   4 +-
>  gdb/main.c          | 210 ++++++++++++++++++++++----------------------
>  gdb/python/python.c |   2 +-
>  gdb/top.c           |   2 +-
>  gdb/xml-syscall.c   |   7 +-
>  8 files changed, 123 insertions(+), 117 deletions(-)
>
> --
> 2.23.0.187.g17f5b7556c-goog
>

[PATCH 0/3] [RFC] Load gdbinit files from a directory

[Christian Biesinger via gdb-patches]

This patch series is some refactoring and then a patch to load gdbinit
files from a directory, instead of only allowing a single file.

Fedora ships a system gdbinit file that does something similar; this
does this by default and also works if Python is disabled.

Christian Biesinger (3):
  Refactor get_init_files to use std::string
  Factor out the code to do the datadir-relocation for gdbinit
  Load system gdbinit files from a directory

 gdb/config.in    |   3 +
 gdb/configure    |  77 +++++++++++++++++++--
 gdb/configure.ac |   3 +
 gdb/main.c       | 175 ++++++++++++++++++++++++++++-------------------
 4 files changed, 183 insertions(+), 75 deletions(-)

-- 
2.23.0.rc1.153.gdeed80330f-goog

[PATCH 2/3] Factor out the code to do the datadir-relocation for gdbinit

[Christian Biesinger via gdb-patches]

gdb/ChangeLog:

2019-08-20  Christian Biesinger  <cbiesinger@google.com>

	* main.c (relocate_gdbinit_path_maybe_in_datadir): New function.
	(get_init_files): Update.
---
 gdb/main.c | 68 +++++++++++++++++++++++++++++-------------------------
 1 file changed, 37 insertions(+), 31 deletions(-)

diff --git a/gdb/main.c b/gdb/main.c
index b9e12589ab..a1d1904c9b 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -191,6 +191,41 @@ relocate_gdb_directory (const char *initial, int flag)
   return dir;
 }
 
+static std::string relocate_gdbinit_path_maybe_in_datadir (std::string file)
+{
+  int datadir_len = strlen (GDB_DATADIR);
+
+  /* If SYSTEM_GDBINIT lives in data-directory, and data-directory
+     has been provided, search for SYSTEM_GDBINIT there.  */
+  if (gdb_datadir_provided
+      && datadir_len < file.length ()
+      && filename_ncmp (file.c_str (), GDB_DATADIR, datadir_len) == 0
+      && IS_DIR_SEPARATOR (file[datadir_len]))
+    {
+      /* Append the part of SYSTEM_GDBINIT that follows GDB_DATADIR
+	 to gdb_datadir.  */
+
+      size_t start = datadir_len;
+      for (; IS_DIR_SEPARATOR (file[start]); ++start)
+	continue;
+      return std::string (gdb_datadir) + SLASH_STRING +
+	file.substr(start);
+    }
+  else
+    {
+      char *relocated = relocate_path (gdb_program_name,
+				       file.c_str(),
+				       SYSTEM_GDBINIT_RELOCATABLE);
+      if (relocated != nullptr)
+	{
+	  std::string retval(relocated);
+	  xfree (relocated);
+	  return retval;
+	}
+    }
+    return "";
+}
+
 /* Compute the locations of init files that GDB should source and
    return them in SYSTEM_GDBINIT, HOME_GDBINIT, LOCAL_GDBINIT.  If
    there is no system gdbinit (resp. home gdbinit and local gdbinit)
@@ -212,37 +247,8 @@ get_init_files (std::string *system_gdbinit,
 
       if (SYSTEM_GDBINIT[0])
 	{
-	  int datadir_len = strlen (GDB_DATADIR);
-	  int sys_gdbinit_len = strlen (SYSTEM_GDBINIT);
-	  std::string relocated_sysgdbinit;
-
-	  /* If SYSTEM_GDBINIT lives in data-directory, and data-directory
-	     has been provided, search for SYSTEM_GDBINIT there.  */
-	  if (gdb_datadir_provided
-	      && datadir_len < sys_gdbinit_len
-	      && filename_ncmp (SYSTEM_GDBINIT, GDB_DATADIR, datadir_len) == 0
-	      && IS_DIR_SEPARATOR (SYSTEM_GDBINIT[datadir_len]))
-	    {
-	      /* Append the part of SYSTEM_GDBINIT that follows GDB_DATADIR
-		 to gdb_datadir.  */
-
-	      size_t start = datadir_len;
-	      for (; IS_DIR_SEPARATOR (SYSTEM_GDBINIT[start]); ++start)
-		continue;
-	      relocated_sysgdbinit = std::string (gdb_datadir) + SLASH_STRING +
-		&SYSTEM_GDBINIT[start];
-	    }
-	  else
-	    {
-	      char *relocated = relocate_path (gdb_program_name,
-					       SYSTEM_GDBINIT,
-					       SYSTEM_GDBINIT_RELOCATABLE);
-	      if (relocated != nullptr)
-	        {
-		  relocated_sysgdbinit = relocated;
-		  xfree (relocated);
-		}
-	    }
+	  std::string relocated_sysgdbinit =
+	    relocate_gdbinit_path_maybe_in_datadir (SYSTEM_GDBINIT);
 	  if (!relocated_sysgdbinit.empty () &&
 	      stat (relocated_sysgdbinit.c_str (), &s) == 0)
 	    sysgdbinit = relocated_sysgdbinit;
-- 
2.23.0.rc1.153.gdeed80330f-goog

Re: [PATCH 2/3] Factor out the code to do the datadir-relocation for gdbinit

[Sergio Durigan Junior]

On Tuesday, August 20 2019, Christian Biesinger via gdb-patches wrote:

> gdb/ChangeLog:
>
> 2019-08-20  Christian Biesinger  <cbiesinger@google.com>
>
> 	* main.c (relocate_gdbinit_path_maybe_in_datadir): New function.
> 	(get_init_files): Update.

I'm afraid you'll need a descriptive commit message :-).

> ---
>  gdb/main.c | 68 +++++++++++++++++++++++++++++-------------------------
>  1 file changed, 37 insertions(+), 31 deletions(-)
>
> diff --git a/gdb/main.c b/gdb/main.c
> index b9e12589ab..a1d1904c9b 100644
> --- a/gdb/main.c
> +++ b/gdb/main.c
> @@ -191,6 +191,41 @@ relocate_gdb_directory (const char *initial, int flag)
>    return dir;
>  }
>  
> +static std::string relocate_gdbinit_path_maybe_in_datadir (std::string file)

You should break the line after 'std::string':

  static std::string
  relocate_gdbinit_path_maybe_in_datadir (std::string file)

> +{
> +  int datadir_len = strlen (GDB_DATADIR);

size_t.

Also, you could declare a return variable here and just fill it
inside each 'if', instead of returning early (and then having to return
an empty string at the end (but that's a matter of style, I know).

> +
> +  /* If SYSTEM_GDBINIT lives in data-directory, and data-directory
> +     has been provided, search for SYSTEM_GDBINIT there.  */
> +  if (gdb_datadir_provided
> +      && datadir_len < file.length ()
> +      && filename_ncmp (file.c_str (), GDB_DATADIR, datadir_len) == 0
> +      && IS_DIR_SEPARATOR (file[datadir_len]))
> +    {
> +      /* Append the part of SYSTEM_GDBINIT that follows GDB_DATADIR
> +	 to gdb_datadir.  */
> +
> +      size_t start = datadir_len;
> +      for (; IS_DIR_SEPARATOR (file[start]); ++start)
> +	continue;

Same comment here: this loop seems strange (starting from 'start').

> +      return std::string (gdb_datadir) + SLASH_STRING +
> +	file.substr(start);
> +    }
> +  else
> +    {
> +      char *relocated = relocate_path (gdb_program_name,
> +				       file.c_str(),
> +				       SYSTEM_GDBINIT_RELOCATABLE);
> +      if (relocated != nullptr)
> +	{
> +	  std::string retval(relocated);

Space between variable name and open parenthesis.

> +	  xfree (relocated);
> +	  return retval;
> +	}
> +    }
> +    return "";
> +}
> +
>  /* Compute the locations of init files that GDB should source and
>     return them in SYSTEM_GDBINIT, HOME_GDBINIT, LOCAL_GDBINIT.  If
>     there is no system gdbinit (resp. home gdbinit and local gdbinit)
> @@ -212,37 +247,8 @@ get_init_files (std::string *system_gdbinit,
>  
>        if (SYSTEM_GDBINIT[0])
>  	{
> -	  int datadir_len = strlen (GDB_DATADIR);
> -	  int sys_gdbinit_len = strlen (SYSTEM_GDBINIT);
> -	  std::string relocated_sysgdbinit;
> -
> -	  /* If SYSTEM_GDBINIT lives in data-directory, and data-directory
> -	     has been provided, search for SYSTEM_GDBINIT there.  */
> -	  if (gdb_datadir_provided
> -	      && datadir_len < sys_gdbinit_len
> -	      && filename_ncmp (SYSTEM_GDBINIT, GDB_DATADIR, datadir_len) == 0
> -	      && IS_DIR_SEPARATOR (SYSTEM_GDBINIT[datadir_len]))
> -	    {
> -	      /* Append the part of SYSTEM_GDBINIT that follows GDB_DATADIR
> -		 to gdb_datadir.  */
> -
> -	      size_t start = datadir_len;
> -	      for (; IS_DIR_SEPARATOR (SYSTEM_GDBINIT[start]); ++start)
> -		continue;
> -	      relocated_sysgdbinit = std::string (gdb_datadir) + SLASH_STRING +
> -		&SYSTEM_GDBINIT[start];
> -	    }
> -	  else
> -	    {
> -	      char *relocated = relocate_path (gdb_program_name,
> -					       SYSTEM_GDBINIT,
> -					       SYSTEM_GDBINIT_RELOCATABLE);
> -	      if (relocated != nullptr)
> -	        {
> -		  relocated_sysgdbinit = relocated;
> -		  xfree (relocated);
> -		}
> -	    }
> +	  std::string relocated_sysgdbinit =
> +	    relocate_gdbinit_path_maybe_in_datadir (SYSTEM_GDBINIT);
>  	  if (!relocated_sysgdbinit.empty () &&
>  	      stat (relocated_sysgdbinit.c_str (), &s) == 0)
>  	    sysgdbinit = relocated_sysgdbinit;
> -- 
> 2.23.0.rc1.153.gdeed80330f-goog

Otherwise, LGTM.

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [PATCH 2/3] Factor out the code to do the datadir-relocation for gdbinit

[Christian Biesinger via gdb-patches]

On Wed, Aug 21, 2019 at 12:19 PM Sergio Durigan Junior
<sergiodj@redhat.com> wrote:
>
> On Tuesday, August 20 2019, Christian Biesinger via gdb-patches wrote:
>
> > gdb/ChangeLog:
> >
> > 2019-08-20  Christian Biesinger  <cbiesinger@google.com>
> >
> >       * main.c (relocate_gdbinit_path_maybe_in_datadir): New function.
> >       (get_init_files): Update.
>
> I'm afraid you'll need a descriptive commit message :-).

Changed to:

2019-08-20  Christian Biesinger  <cbiesinger@google.com>

        * main.c (relocate_gdbinit_path_maybe_in_datadir): Factor this code
        out of get_init_files.
        (get_init_files): Update.

(I guess the title of the commit message is not enough?)

> > ---
> >  gdb/main.c | 68 +++++++++++++++++++++++++++++-------------------------
> >  1 file changed, 37 insertions(+), 31 deletions(-)
> >
> > diff --git a/gdb/main.c b/gdb/main.c
> > index b9e12589ab..a1d1904c9b 100644
> > --- a/gdb/main.c
> > +++ b/gdb/main.c
> > @@ -191,6 +191,41 @@ relocate_gdb_directory (const char *initial, int flag)
> >    return dir;
> >  }
> >
> > +static std::string relocate_gdbinit_path_maybe_in_datadir (std::string file)
>
> You should break the line after 'std::string':
>
>   static std::string
>   relocate_gdbinit_path_maybe_in_datadir (std::string file)

Thanks, done and also changed std::string to const std::string&.

> > +{
> > +  int datadir_len = strlen (GDB_DATADIR);
>
> size_t.
>
> Also, you could declare a return variable here and just fill it
> inside each 'if', instead of returning early (and then having to return
> an empty string at the end (but that's a matter of style, I know).

OK, if you prefer, sure. Done.

> > +
> > +  /* If SYSTEM_GDBINIT lives in data-directory, and data-directory
> > +     has been provided, search for SYSTEM_GDBINIT there.  */
> > +  if (gdb_datadir_provided
> > +      && datadir_len < file.length ()
> > +      && filename_ncmp (file.c_str (), GDB_DATADIR, datadir_len) == 0
> > +      && IS_DIR_SEPARATOR (file[datadir_len]))
> > +    {
> > +      /* Append the part of SYSTEM_GDBINIT that follows GDB_DATADIR
> > +      to gdb_datadir.  */
> > +
> > +      size_t start = datadir_len;
> > +      for (; IS_DIR_SEPARATOR (file[start]); ++start)
> > +     continue;
>
> Same comment here: this loop seems strange (starting from 'start').

See my response in the other thread.

> > +      return std::string (gdb_datadir) + SLASH_STRING +
> > +     file.substr(start);
> > +    }
> > +  else
> > +    {
> > +      char *relocated = relocate_path (gdb_program_name,
> > +                                    file.c_str(),
> > +                                    SYSTEM_GDBINIT_RELOCATABLE);
> > +      if (relocated != nullptr)
> > +     {
> > +       std::string retval(relocated);
>
> Space between variable name and open parenthesis.

Thanks! (Though irrelevant with the other change now)

> > +       xfree (relocated);
> > +       return retval;
> > +     }
> > +    }
> > +    return "";
> > +}
> > +
> >  /* Compute the locations of init files that GDB should source and
> >     return them in SYSTEM_GDBINIT, HOME_GDBINIT, LOCAL_GDBINIT.  If
> >     there is no system gdbinit (resp. home gdbinit and local gdbinit)
> > @@ -212,37 +247,8 @@ get_init_files (std::string *system_gdbinit,
> >
> >        if (SYSTEM_GDBINIT[0])
> >       {
> > -       int datadir_len = strlen (GDB_DATADIR);
> > -       int sys_gdbinit_len = strlen (SYSTEM_GDBINIT);
> > -       std::string relocated_sysgdbinit;
> > -
> > -       /* If SYSTEM_GDBINIT lives in data-directory, and data-directory
> > -          has been provided, search for SYSTEM_GDBINIT there.  */
> > -       if (gdb_datadir_provided
> > -           && datadir_len < sys_gdbinit_len
> > -           && filename_ncmp (SYSTEM_GDBINIT, GDB_DATADIR, datadir_len) == 0
> > -           && IS_DIR_SEPARATOR (SYSTEM_GDBINIT[datadir_len]))
> > -         {
> > -           /* Append the part of SYSTEM_GDBINIT that follows GDB_DATADIR
> > -              to gdb_datadir.  */
> > -
> > -           size_t start = datadir_len;
> > -           for (; IS_DIR_SEPARATOR (SYSTEM_GDBINIT[start]); ++start)
> > -             continue;
> > -           relocated_sysgdbinit = std::string (gdb_datadir) + SLASH_STRING +
> > -             &SYSTEM_GDBINIT[start];
> > -         }
> > -       else
> > -         {
> > -           char *relocated = relocate_path (gdb_program_name,
> > -                                            SYSTEM_GDBINIT,
> > -                                            SYSTEM_GDBINIT_RELOCATABLE);
> > -           if (relocated != nullptr)
> > -             {
> > -               relocated_sysgdbinit = relocated;
> > -               xfree (relocated);
> > -             }
> > -         }
> > +       std::string relocated_sysgdbinit =
> > +         relocate_gdbinit_path_maybe_in_datadir (SYSTEM_GDBINIT);
> >         if (!relocated_sysgdbinit.empty () &&
> >             stat (relocated_sysgdbinit.c_str (), &s) == 0)
> >           sysgdbinit = relocated_sysgdbinit;
> > --
> > 2.23.0.rc1.153.gdeed80330f-goog
>
> Otherwise, LGTM.

Thanks.

Christian

Re: [PATCH 2/3] Factor out the code to do the datadir-relocation for gdbinit

[Sergio Durigan Junior]

On Wednesday, August 21 2019, Christian Biesinger wrote:

> On Wed, Aug 21, 2019 at 12:19 PM Sergio Durigan Junior
> <sergiodj@redhat.com> wrote:
>>
>> On Tuesday, August 20 2019, Christian Biesinger via gdb-patches wrote:
>>
>> > gdb/ChangeLog:
>> >
>> > 2019-08-20  Christian Biesinger  <cbiesinger@google.com>
>> >
>> >       * main.c (relocate_gdbinit_path_maybe_in_datadir): New function.
>> >       (get_init_files): Update.
>>
>> I'm afraid you'll need a descriptive commit message :-).
>
> Changed to:
>
> 2019-08-20  Christian Biesinger  <cbiesinger@google.com>
>
>         * main.c (relocate_gdbinit_path_maybe_in_datadir): Factor this code
>         out of get_init_files.
>         (get_init_files): Update.
>
> (I guess the title of the commit message is not enough?)

Sorry, I should have been more clear.

The changelog entry was fine; I was referring to the actual commit
message (the text you write before the changelog).  I think a very
simple text should be enough in this case, since it's a refactorization.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [PATCH 2/3] Factor out the code to do the datadir-relocation for gdbinit

[Christian Biesinger via gdb-patches]

On Wed, Aug 21, 2019 at 12:47 PM Sergio Durigan Junior
<sergiodj@redhat.com> wrote:
>
> On Wednesday, August 21 2019, Christian Biesinger wrote:
>
> > On Wed, Aug 21, 2019 at 12:19 PM Sergio Durigan Junior
> > <sergiodj@redhat.com> wrote:
> >>
> >> On Tuesday, August 20 2019, Christian Biesinger via gdb-patches wrote:
> >>
> >> > gdb/ChangeLog:
> >> >
> >> > 2019-08-20  Christian Biesinger  <cbiesinger@google.com>
> >> >
> >> >       * main.c (relocate_gdbinit_path_maybe_in_datadir): New function.
> >> >       (get_init_files): Update.
> >>
> >> I'm afraid you'll need a descriptive commit message :-).
> >
> > Changed to:
> >
> > 2019-08-20  Christian Biesinger  <cbiesinger@google.com>
> >
> >         * main.c (relocate_gdbinit_path_maybe_in_datadir): Factor this code
> >         out of get_init_files.
> >         (get_init_files): Update.
> >
> > (I guess the title of the commit message is not enough?)
>
> Sorry, I should have been more clear.
>
> The changelog entry was fine; I was referring to the actual commit
> message (the text you write before the changelog).  I think a very
> simple text should be enough in this case, since it's a refactorization.

Oh OK, changed to the following:
    Factor out the code to do the datadir-relocation for gdbinit

    This simplifies get_init_files and makes it possible to reuse
    this code in an upcoming patch for SYSTEM_GDBINIT_DIR.

    gdb/ChangeLog:

    2019-08-20  Christian Biesinger  <cbiesinger@google.com>

            * main.c (relocate_gdbinit_path_maybe_in_datadir): Factor this code
            out of get_init_files.
            (get_init_files): Update.

(I assume it's fine not to resend the full patch, but let me know if
you want me to)

Christian

Re: [PATCH 2/3] Factor out the code to do the datadir-relocation for gdbinit

[Sergio Durigan Junior]

On Wednesday, August 21 2019, Christian Biesinger wrote:

> On Wed, Aug 21, 2019 at 12:47 PM Sergio Durigan Junior
> <sergiodj@redhat.com> wrote:
>>
>> On Wednesday, August 21 2019, Christian Biesinger wrote:
>>
>> > On Wed, Aug 21, 2019 at 12:19 PM Sergio Durigan Junior
>> > <sergiodj@redhat.com> wrote:
>> >>
>> >> On Tuesday, August 20 2019, Christian Biesinger via gdb-patches wrote:
>> >>
>> >> > gdb/ChangeLog:
>> >> >
>> >> > 2019-08-20  Christian Biesinger  <cbiesinger@google.com>
>> >> >
>> >> >       * main.c (relocate_gdbinit_path_maybe_in_datadir): New function.
>> >> >       (get_init_files): Update.
>> >>
>> >> I'm afraid you'll need a descriptive commit message :-).
>> >
>> > Changed to:
>> >
>> > 2019-08-20  Christian Biesinger  <cbiesinger@google.com>
>> >
>> >         * main.c (relocate_gdbinit_path_maybe_in_datadir): Factor this code
>> >         out of get_init_files.
>> >         (get_init_files): Update.
>> >
>> > (I guess the title of the commit message is not enough?)
>>
>> Sorry, I should have been more clear.
>>
>> The changelog entry was fine; I was referring to the actual commit
>> message (the text you write before the changelog).  I think a very
>> simple text should be enough in this case, since it's a refactorization.
>
> Oh OK, changed to the following:
>     Factor out the code to do the datadir-relocation for gdbinit
>
>     This simplifies get_init_files and makes it possible to reuse
>     this code in an upcoming patch for SYSTEM_GDBINIT_DIR.
>
>     gdb/ChangeLog:
>
>     2019-08-20  Christian Biesinger  <cbiesinger@google.com>
>
>             * main.c (relocate_gdbinit_path_maybe_in_datadir): Factor this code
>             out of get_init_files.
>             (get_init_files): Update.
>
> (I assume it's fine not to resend the full patch, but let me know if
> you want me to)

That's great, thanks.

I can't approve it as I'm not a GM, but this LGTM.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/