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

classic Classic list List threaded Threaded
30 messages Options
12
Reply | Threaded
Open this post in threaded view
|

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

Sourceware - gdb-patches mailing list
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

Reply | Threaded
Open this post in threaded view
|

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

Sourceware - gdb-patches mailing list
To avoid manual memory management.

Tested on buildbot.

gdb/ChangeLog:

2019-08-20  Christian Biesinger  <[hidden email]>

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

diff --git a/gdb/main.c b/gdb/main.c
index 678c413021..b9e12589ab 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -195,27 +195,26 @@ relocate_gdb_directory (const char *initial, int flag)
    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;
+  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) + "/" + 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,7 @@ 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, home_gdbinit, local_gdbinit;
   get_init_files (&system_gdbinit, &home_gdbinit, &local_gdbinit);
 
   /* Do these (and anything which might call wrap_here or *_filtered)
@@ -984,16 +980,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 +1092,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 +1199,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 +1279,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.rc1.153.gdeed80330f-goog

Reply | Threaded
Open this post in threaded view
|

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

Sourceware - gdb-patches mailing list
In reply to this post by Sourceware - gdb-patches mailing list
gdb/ChangeLog:

2019-08-20  Christian Biesinger  <[hidden email]>

        * 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

Reply | Threaded
Open this post in threaded view
|

[PATCH 3/3] Load system gdbinit files from a directory

Sourceware - gdb-patches mailing list
In reply to this post by Sourceware - gdb-patches mailing list
gdb/ChangeLog:

2019-08-20  Christian Biesinger  <[hidden email]>

        * config.in: Add SYSTEM_GDBINIT_DIR.
        * configure: Regenerate.
        * configure.ac: Add new option --with-system-gdbinit-dir.
        * main.c (get_init_files): Change system_gdbinit argument to
        a vector and return the files in SYSTEM_GDBINIT_DIR in
        addition to SYSTEM_GDBINIT.
        (captured_main_1): Update.
        (print_gdb_help): Update.
---
 gdb/config.in    |  3 ++
 gdb/configure    | 77 ++++++++++++++++++++++++++++++++++++++++++++----
 gdb/configure.ac |  3 ++
 gdb/main.c       | 53 +++++++++++++++++++++++++++------
 4 files changed, 121 insertions(+), 15 deletions(-)

diff --git a/gdb/config.in b/gdb/config.in
index 26ca02f6a3..ca523fe4ab 100644
--- a/gdb/config.in
+++ b/gdb/config.in
@@ -684,6 +684,9 @@
 /* automatically load a system-wide gdbinit file */
 #undef SYSTEM_GDBINIT
 
+/* automatically load system-wide gdbinit files from this dir */
+#undef SYSTEM_GDBINIT_DIR
+
 /* Define if the system-gdbinit directory should be relocated when GDB is
    moved. */
 #undef SYSTEM_GDBINIT_RELOCATABLE
diff --git a/gdb/configure b/gdb/configure
index cb71bbf057..e5aa2e6b3b 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -693,6 +693,7 @@ WIN32LIBS
 SER_HARDWIRE
 WERROR_CFLAGS
 WARN_CFLAGS
+SYSTEM_GDBINIT_DIR
 SYSTEM_GDBINIT
 TARGET_SYSTEM_ROOT
 CONFIG_LDFLAGS
@@ -824,6 +825,7 @@ infodir
 docdir
 oldincludedir
 includedir
+runstatedir
 localstatedir
 sharedstatedir
 sysconfdir
@@ -884,6 +886,7 @@ with_libipt_prefix
 with_included_regex
 with_sysroot
 with_system_gdbinit
+with_system_gdbinit_dir
 enable_werror
 enable_build_warnings
 enable_gdb_build_warnings
@@ -956,6 +959,7 @@ datadir='${datarootdir}'
 sysconfdir='${prefix}/etc'
 sharedstatedir='${prefix}/com'
 localstatedir='${prefix}/var'
+runstatedir='${localstatedir}/run'
 includedir='${prefix}/include'
 oldincludedir='/usr/include'
 docdir='${datarootdir}/doc/${PACKAGE}'
@@ -1208,6 +1212,15 @@ do
   | -silent | --silent | --silen | --sile | --sil)
     silent=yes ;;
 
+  -runstatedir | --runstatedir | --runstatedi | --runstated \
+  | --runstate | --runstat | --runsta | --runst | --runs \
+  | --run | --ru | --r)
+    ac_prev=runstatedir ;;
+  -runstatedir=* | --runstatedir=* | --runstatedi=* | --runstated=* \
+  | --runstate=* | --runstat=* | --runsta=* | --runst=* | --runs=* \
+  | --run=* | --ru=* | --r=*)
+    runstatedir=$ac_optarg ;;
+
   -sbindir | --sbindir | --sbindi | --sbind | --sbin | --sbi | --sb)
     ac_prev=sbindir ;;
   -sbindir=* | --sbindir=* | --sbindi=* | --sbind=* | --sbin=* \
@@ -1345,7 +1358,7 @@ fi
 for ac_var in exec_prefix prefix bindir sbindir libexecdir datarootdir \
  datadir sysconfdir sharedstatedir localstatedir includedir \
  oldincludedir docdir infodir htmldir dvidir pdfdir psdir \
- libdir localedir mandir
+ libdir localedir mandir runstatedir
 do
   eval ac_val=\$$ac_var
   # Remove trailing slashes.
@@ -1498,6 +1511,7 @@ Fine tuning of the installation directories:
   --sysconfdir=DIR        read-only single-machine data [PREFIX/etc]
   --sharedstatedir=DIR    modifiable architecture-independent data [PREFIX/com]
   --localstatedir=DIR     modifiable single-machine data [PREFIX/var]
+  --runstatedir=DIR       modifiable per-process data [LOCALSTATEDIR/run]
   --libdir=DIR            object code libraries [EPREFIX/lib]
   --includedir=DIR        C header files [PREFIX/include]
   --oldincludedir=DIR     C header files for non-gcc [/usr/include]
@@ -1618,6 +1632,9 @@ Optional Packages:
   --with-sysroot[=DIR]    search for usr/lib et al within DIR
   --with-system-gdbinit=PATH
                           automatically load a system-wide gdbinit file
+  --with-system-gdbinit-dir=PATH
+                          automatically load system-wide gdbinit files from
+                          this directory
   --with-lzma             support lzma compression (auto/yes/no)
   --with-liblzma-prefix[=DIR]  search for liblzma in DIR/include and DIR/lib
   --without-liblzma-prefix     don't search for liblzma in includedir and libdir
@@ -4683,7 +4700,7 @@ else
     We can't simply define LARGE_OFF_T to be 9223372036854775807,
     since some C++ compilers masquerading as C compilers
     incorrectly reject 9223372036854775807.  */
-#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
+#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31) << 31))
   int off_t_is_large[(LARGE_OFF_T % 2147483629 == 721
        && LARGE_OFF_T % 2147483647 == 1)
       ? 1 : -1];
@@ -4729,7 +4746,7 @@ else
     We can't simply define LARGE_OFF_T to be 9223372036854775807,
     since some C++ compilers masquerading as C compilers
     incorrectly reject 9223372036854775807.  */
-#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
+#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31) << 31))
   int off_t_is_large[(LARGE_OFF_T % 2147483629 == 721
        && LARGE_OFF_T % 2147483647 == 1)
       ? 1 : -1];
@@ -4753,7 +4770,7 @@ rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
     We can't simply define LARGE_OFF_T to be 9223372036854775807,
     since some C++ compilers masquerading as C compilers
     incorrectly reject 9223372036854775807.  */
-#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
+#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31) << 31))
   int off_t_is_large[(LARGE_OFF_T % 2147483629 == 721
        && LARGE_OFF_T % 2147483647 == 1)
       ? 1 : -1];
@@ -4798,7 +4815,7 @@ else
     We can't simply define LARGE_OFF_T to be 9223372036854775807,
     since some C++ compilers masquerading as C compilers
     incorrectly reject 9223372036854775807.  */
-#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
+#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31) << 31))
   int off_t_is_large[(LARGE_OFF_T % 2147483629 == 721
        && LARGE_OFF_T % 2147483647 == 1)
       ? 1 : -1];
@@ -4822,7 +4839,7 @@ rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
     We can't simply define LARGE_OFF_T to be 9223372036854775807,
     since some C++ compilers masquerading as C compilers
     incorrectly reject 9223372036854775807.  */
-#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
+#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31) << 31))
   int off_t_is_large[(LARGE_OFF_T % 2147483629 == 721
        && LARGE_OFF_T % 2147483647 == 1)
       ? 1 : -1];
@@ -12872,6 +12889,8 @@ main ()
     if (*(data + i) != *(data3 + i))
       return 14;
   close (fd);
+  free (data);
+  free (data3);
   return 0;
 }
 _ACEOF
@@ -15239,6 +15258,52 @@ _ACEOF
 
 
 
+# Check whether --with-system-gdbinit-dir was given.
+if test "${with_system_gdbinit_dir+set}" = set; then :
+  withval=$with_system_gdbinit_dir;
+    SYSTEM_GDBINIT_DIR=$withval
+else
+  SYSTEM_GDBINIT_DIR=
+fi
+
+
+  test "x$prefix" = xNONE && prefix="$ac_default_prefix"
+  test "x$exec_prefix" = xNONE && exec_prefix='${prefix}'
+  ac_define_dir=`eval echo $SYSTEM_GDBINIT_DIR`
+  ac_define_dir=`eval echo $ac_define_dir`
+
+cat >>confdefs.h <<_ACEOF
+#define SYSTEM_GDBINIT_DIR "$ac_define_dir"
+_ACEOF
+
+
+
+
+  if test "x$exec_prefix" = xNONE || test "x$exec_prefix" = 'x${prefix}'; then
+     if test "x$prefix" = xNONE; then
+     test_prefix=/usr/local
+     else
+ test_prefix=$prefix
+     fi
+  else
+     test_prefix=$exec_prefix
+  fi
+  value=0
+  case ${ac_define_dir} in
+     "${test_prefix}"|"${test_prefix}/"*|\
+ '${exec_prefix}'|'${exec_prefix}/'*)
+     value=1
+     ;;
+  esac
+
+cat >>confdefs.h <<_ACEOF
+#define SYSTEM_GDBINIT_DIR_RELOCATABLE $value
+_ACEOF
+
+
+
+
+
 # Check whether --enable-werror was given.
 if test "${enable_werror+set}" = set; then :
   enableval=$enable_werror; case "${enableval}" in
diff --git a/gdb/configure.ac b/gdb/configure.ac
index 5a18c16405..27d67ead3e 100644
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -1844,6 +1844,9 @@ GDB_AC_DEFINE_RELOCATABLE(TARGET_SYSTEM_ROOT, sysroot, ${ac_define_dir})
 GDB_AC_WITH_DIR(SYSTEM_GDBINIT, system-gdbinit,
     [automatically load a system-wide gdbinit file],
     [])
+GDB_AC_WITH_DIR(SYSTEM_GDBINIT_DIR, system-gdbinit-dir,
+    [automatically load system-wide gdbinit files from this directory],
+    [])
 
 AM_GDB_WARNINGS
 AM_GDB_UBSAN
diff --git a/gdb/main.c b/gdb/main.c
index a1d1904c9b..39957db4bd 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -45,6 +45,7 @@
 #include "event-top.h"
 #include "infrun.h"
 #include "gdbsupport/signals-state-save-restore.h"
+#include <algorithm>
 #include <vector>
 #include "gdbsupport/pathstuff.h"
 #include "cli/cli-style.h"
@@ -232,11 +233,11 @@ static std::string relocate_gdbinit_path_maybe_in_datadir (std::string file)
    to be loaded, then SYSTEM_GDBINIT (resp. HOME_GDBINIT and
    LOCAL_GDBINIT) is set to the empty string.  */
 static void
-get_init_files (std::string *system_gdbinit,
+get_init_files (std::vector<std::string> *system_gdbinit,
  std::string *home_gdbinit,
  std::string *local_gdbinit)
 {
-  static std::string sysgdbinit;
+  static std::vector<std::string> sysgdbinit;
   static std::string homeinit;
   static std::string localinit;
   static int initialized = 0;
@@ -251,7 +252,28 @@ get_init_files (std::string *system_gdbinit,
     relocate_gdbinit_path_maybe_in_datadir (SYSTEM_GDBINIT);
   if (!relocated_sysgdbinit.empty () &&
       stat (relocated_sysgdbinit.c_str (), &s) == 0)
-    sysgdbinit = relocated_sysgdbinit;
+    sysgdbinit.push_back(relocated_sysgdbinit);
+ }
+      if (SYSTEM_GDBINIT_DIR[0])
+        {
+  std::string relocated_gdbinit_dir =
+    relocate_gdbinit_path_maybe_in_datadir (SYSTEM_GDBINIT_DIR);
+  DIR* dir;
+  if (!relocated_gdbinit_dir.empty () &&
+    (dir = opendir (relocated_gdbinit_dir.c_str ())) != nullptr)
+    {
+      std::vector<std::string> files;
+      struct dirent* ent;
+      while ((ent = readdir (dir)) != nullptr)
+        {
+  if (ent->d_name[0] != '.')
+    files.push_back (relocated_gdbinit_dir + SLASH_STRING +
+     ent->d_name);
+ }
+      closedir (dir);
+      std::sort (files.begin (), files.end ());
+      sysgdbinit.insert (sysgdbinit.end (), files.begin (), files.end ());
+    }
  }
 
       const char *homedir = getenv ("HOME");
@@ -909,7 +931,8 @@ 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, home_gdbinit, local_gdbinit;
+  std::vector<std::string> system_gdbinit;
+  std::string home_gdbinit, local_gdbinit;
   get_init_files (&system_gdbinit, &home_gdbinit, &local_gdbinit);
 
   /* Do these (and anything which might call wrap_here or *_filtered)
@@ -987,7 +1010,10 @@ captured_main_1 (struct captured_main_args *context)
      processed; it sets global parameters, which are independent of
      what file you are debugging or what directory you are in.  */
   if (!system_gdbinit.empty () && !inhibit_gdbinit)
-    ret = catch_command_errors (source_script, system_gdbinit.c_str (), 0);
+    {
+      for (const std::string& file : system_gdbinit)
+ ret = catch_command_errors (source_script, file.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
@@ -1205,7 +1231,7 @@ gdb_main (struct captured_main_args *args)
 static void
 print_gdb_help (struct ui_file *stream)
 {
-  std::string system_gdbinit;
+  std::vector<std::string> system_gdbinit;
   std::string home_gdbinit;
   std::string local_gdbinit;
 
@@ -1286,9 +1312,18 @@ Other options:\n\n\
 At startup, GDB reads the following init files and executes their commands:\n\
 "), stream);
   if (!system_gdbinit.empty ())
-    fprintf_unfiltered (stream, _("\
-   * system-wide init file: %s\n\
-"), system_gdbinit.c_str ());
+    {
+      std::string output;
+      for (size_t idx = 0; idx < system_gdbinit.size(); ++idx)
+        {
+  output += system_gdbinit[idx];
+  if (idx < system_gdbinit.size() - 1)
+    output += ", ";
+ }
+      fprintf_unfiltered (stream, _("\
+   * system-wide init files: %s\n\
+"), output.c_str ());
+    }
   if (!home_gdbinit.empty ())
     fprintf_unfiltered (stream, _("\
    * user-specific init file: %s\n\
--
2.23.0.rc1.153.gdeed80330f-goog

Reply | Threaded
Open this post in threaded view
|

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

Sergio Durigan Junior
In reply to this post by Sourceware - gdb-patches mailing list
On Tuesday, August 20 2019, Christian Biesinger via gdb-patches wrote:

> To avoid manual memory management.

Thanks!  Comments below.

> Tested on buildbot.
>
> gdb/ChangeLog:
>
> 2019-08-20  Christian Biesinger  <[hidden email]>
>
> * main.c (get_init_files): Change to use std::string.
> (captured_main_1): Update.
> (print_gdb_help): Update.
> ---
>  gdb/main.c | 96 ++++++++++++++++++++++++++----------------------------
>  1 file changed, 46 insertions(+), 50 deletions(-)
>
> diff --git a/gdb/main.c b/gdb/main.c
> index 678c413021..b9e12589ab 100644
> --- a/gdb/main.c
> +++ b/gdb/main.c
> @@ -195,27 +195,26 @@ relocate_gdb_directory (const char *initial, int flag)
>     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);

Since you're cleaning things up, I wouldn't mind converting these two
variables to 'size_t' :-).

> -  char *relocated_sysgdbinit;
> +  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;

This seems wrong; you're starting the iteration from
'SYSTEM_GDBINIT[start]', but 'start' is 'datadir_len'.

> -      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) + "/" + GDBINIT;

I know the old code did that already, and I also know that SLASH_STRING
is always defined as "/", but I think we should use it here, instead of
hard coding the "/".

> +  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,7 @@ 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, home_gdbinit, local_gdbinit;

I'd prefer if you kept each variable declared separately, like it was
before:

  std::string system_gdbinit;
  std::string home_gdbinit;
  std::string local_gdbinit;

IIRC, we discourage declaring many variables in the same line.

>    get_init_files (&system_gdbinit, &home_gdbinit, &local_gdbinit);
>  
>    /* Do these (and anything which might call wrap_here or *_filtered)
> @@ -984,16 +980,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 +1092,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 +1199,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 +1279,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.rc1.153.gdeed80330f-goog

Otherwise, LGTM.  Thanks for doing this!

--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/
Reply | Threaded
Open this post in threaded view
|

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

Sergio Durigan Junior
In reply to this post by Sourceware - gdb-patches mailing list
On Tuesday, August 20 2019, Christian Biesinger via gdb-patches wrote:

> gdb/ChangeLog:
>
> 2019-08-20  Christian Biesinger  <[hidden email]>
>
> * 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/
Reply | Threaded
Open this post in threaded view
|

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

Sourceware - gdb-patches mailing list
In reply to this post by Sergio Durigan Junior
I will send an updated version in a moment. More comments below.

On Wed, Aug 21, 2019 at 12:13 PM Sergio Durigan Junior
<[hidden email]> wrote:

>
> On Tuesday, August 20 2019, Christian Biesinger via gdb-patches wrote:
>
> > To avoid manual memory management.
>
> Thanks!  Comments below.
>
> > Tested on buildbot.
> >
> > gdb/ChangeLog:
> >
> > 2019-08-20  Christian Biesinger  <[hidden email]>
> >
> >       * main.c (get_init_files): Change to use std::string.
> >       (captured_main_1): Update.
> >       (print_gdb_help): Update.
> > ---
> >  gdb/main.c | 96 ++++++++++++++++++++++++++----------------------------
> >  1 file changed, 46 insertions(+), 50 deletions(-)
> >
> > diff --git a/gdb/main.c b/gdb/main.c
> > index 678c413021..b9e12589ab 100644
> > --- a/gdb/main.c
> > +++ b/gdb/main.c
> > @@ -195,27 +195,26 @@ relocate_gdb_directory (const char *initial, int flag)
> >     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);
>
> Since you're cleaning things up, I wouldn't mind converting these two
> variables to 'size_t' :-).

Will do.

> > -       char *relocated_sysgdbinit;
> > +       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;
>
> This seems wrong; you're starting the iteration from
> 'SYSTEM_GDBINIT[start]', but 'start' is 'datadir_len'.

The previous version first initialized tmp_sys_gdbinit to start from
&SYSTEM_GDBINIT[datadir_len], so I think my change is correct (and
simpler)?

> > -           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) + "/" + GDBINIT;
>
> I know the old code did that already, and I also know that SLASH_STRING
> is always defined as "/", but I think we should use it here, instead of
> hard coding the "/".

Will do. (I didn't know that SLASH_STRING is always "/", I assumed it
would be "\" on Windows....)

> > +       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,7 @@ 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, home_gdbinit, local_gdbinit;
>
> I'd prefer if you kept each variable declared separately, like it was
> before:
>
>   std::string system_gdbinit;
>   std::string home_gdbinit;
>   std::string local_gdbinit;
>
> IIRC, we discourage declaring many variables in the same line.

Will do.

> >    get_init_files (&system_gdbinit, &home_gdbinit, &local_gdbinit);
> >
> >    /* Do these (and anything which might call wrap_here or *_filtered)
> > @@ -984,16 +980,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 +1092,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 +1199,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 +1279,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.rc1.153.gdeed80330f-goog
>
> Otherwise, LGTM.  Thanks for doing this!

Thanks for the review!

Christian
Reply | Threaded
Open this post in threaded view
|

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

Sourceware - gdb-patches mailing list
To avoid manual memory management.

Tested on buildbot.

gdb/ChangeLog:

2019-08-20  Christian Biesinger  <[hidden email]>

        * 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 678c413021..724b2928a7 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -195,27 +195,26 @@ relocate_gdb_directory (const char *initial, int flag)
    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.rc1.153.gdeed80330f-goog

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 3/3] Load system gdbinit files from a directory

Sergio Durigan Junior
In reply to this post by Sourceware - gdb-patches mailing list
On Tuesday, August 20 2019, Christian Biesinger via gdb-patches wrote:

> gdb/ChangeLog:
>
> 2019-08-20  Christian Biesinger  <[hidden email]>
>
> * config.in: Add SYSTEM_GDBINIT_DIR.

config.in is also regenerated.

> * configure: Regenerate.
> * configure.ac: Add new option --with-system-gdbinit-dir.
> * main.c (get_init_files): Change system_gdbinit argument to
> a vector and return the files in SYSTEM_GDBINIT_DIR in
> addition to SYSTEM_GDBINIT.
> (captured_main_1): Update.
> (print_gdb_help): Update.

You'll need a descriptive commit log.

> ---
>  gdb/config.in    |  3 ++
>  gdb/configure    | 77 ++++++++++++++++++++++++++++++++++++++++++++----
>  gdb/configure.ac |  3 ++
>  gdb/main.c       | 53 +++++++++++++++++++++++++++------
>  4 files changed, 121 insertions(+), 15 deletions(-)
>
> diff --git a/gdb/config.in b/gdb/config.in
> index 26ca02f6a3..ca523fe4ab 100644
> --- a/gdb/config.in
> +++ b/gdb/config.in
> @@ -684,6 +684,9 @@
>  /* automatically load a system-wide gdbinit file */
>  #undef SYSTEM_GDBINIT
>  
> +/* automatically load system-wide gdbinit files from this dir */
> +#undef SYSTEM_GDBINIT_DIR
> +
>  /* Define if the system-gdbinit directory should be relocated when GDB is
>     moved. */
>  #undef SYSTEM_GDBINIT_RELOCATABLE
> diff --git a/gdb/configure b/gdb/configure
> index cb71bbf057..e5aa2e6b3b 100755
> --- a/gdb/configure
> +++ b/gdb/configure
> @@ -693,6 +693,7 @@ WIN32LIBS
>  SER_HARDWIRE
>  WERROR_CFLAGS
>  WARN_CFLAGS
> +SYSTEM_GDBINIT_DIR
>  SYSTEM_GDBINIT
>  TARGET_SYSTEM_ROOT
>  CONFIG_LDFLAGS
> @@ -824,6 +825,7 @@ infodir
>  docdir
>  oldincludedir
>  includedir
> +runstatedir
>  localstatedir
>  sharedstatedir
>  sysconfdir
> @@ -884,6 +886,7 @@ with_libipt_prefix
>  with_included_regex
>  with_sysroot
>  with_system_gdbinit
> +with_system_gdbinit_dir
>  enable_werror
>  enable_build_warnings
>  enable_gdb_build_warnings
> @@ -956,6 +959,7 @@ datadir='${datarootdir}'
>  sysconfdir='${prefix}/etc'
>  sharedstatedir='${prefix}/com'
>  localstatedir='${prefix}/var'
> +runstatedir='${localstatedir}/run'
>  includedir='${prefix}/include'
>  oldincludedir='/usr/include'
>  docdir='${datarootdir}/doc/${PACKAGE}'
> @@ -1208,6 +1212,15 @@ do
>    | -silent | --silent | --silen | --sile | --sil)
>      silent=yes ;;
>  
> +  -runstatedir | --runstatedir | --runstatedi | --runstated \
> +  | --runstate | --runstat | --runsta | --runst | --runs \
> +  | --run | --ru | --r)
> +    ac_prev=runstatedir ;;
> +  -runstatedir=* | --runstatedir=* | --runstatedi=* | --runstated=* \
> +  | --runstate=* | --runstat=* | --runsta=* | --runst=* | --runs=* \
> +  | --run=* | --ru=* | --r=*)
> +    runstatedir=$ac_optarg ;;
> +
>    -sbindir | --sbindir | --sbindi | --sbind | --sbin | --sbi | --sb)
>      ac_prev=sbindir ;;
>    -sbindir=* | --sbindir=* | --sbindi=* | --sbind=* | --sbin=* \
> @@ -1345,7 +1358,7 @@ fi
>  for ac_var in exec_prefix prefix bindir sbindir libexecdir datarootdir \
>   datadir sysconfdir sharedstatedir localstatedir includedir \
>   oldincludedir docdir infodir htmldir dvidir pdfdir psdir \
> - libdir localedir mandir
> + libdir localedir mandir runstatedir
>  do
>    eval ac_val=\$$ac_var
>    # Remove trailing slashes.
> @@ -1498,6 +1511,7 @@ Fine tuning of the installation directories:
>    --sysconfdir=DIR        read-only single-machine data [PREFIX/etc]
>    --sharedstatedir=DIR    modifiable architecture-independent data [PREFIX/com]
>    --localstatedir=DIR     modifiable single-machine data [PREFIX/var]
> +  --runstatedir=DIR       modifiable per-process data [LOCALSTATEDIR/run]
>    --libdir=DIR            object code libraries [EPREFIX/lib]
>    --includedir=DIR        C header files [PREFIX/include]
>    --oldincludedir=DIR     C header files for non-gcc [/usr/include]
> @@ -1618,6 +1632,9 @@ Optional Packages:
>    --with-sysroot[=DIR]    search for usr/lib et al within DIR
>    --with-system-gdbinit=PATH
>                            automatically load a system-wide gdbinit file
> +  --with-system-gdbinit-dir=PATH
> +                          automatically load system-wide gdbinit files from
> +                          this directory
>    --with-lzma             support lzma compression (auto/yes/no)
>    --with-liblzma-prefix[=DIR]  search for liblzma in DIR/include and DIR/lib
>    --without-liblzma-prefix     don't search for liblzma in includedir and libdir
> @@ -4683,7 +4700,7 @@ else
>      We can't simply define LARGE_OFF_T to be 9223372036854775807,
>      since some C++ compilers masquerading as C compilers
>      incorrectly reject 9223372036854775807.  */
> -#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
> +#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31) << 31))
>    int off_t_is_large[(LARGE_OFF_T % 2147483629 == 721
>         && LARGE_OFF_T % 2147483647 == 1)
>        ? 1 : -1];
> @@ -4729,7 +4746,7 @@ else
>      We can't simply define LARGE_OFF_T to be 9223372036854775807,
>      since some C++ compilers masquerading as C compilers
>      incorrectly reject 9223372036854775807.  */
> -#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
> +#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31) << 31))
>    int off_t_is_large[(LARGE_OFF_T % 2147483629 == 721
>         && LARGE_OFF_T % 2147483647 == 1)
>        ? 1 : -1];
> @@ -4753,7 +4770,7 @@ rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
>      We can't simply define LARGE_OFF_T to be 9223372036854775807,
>      since some C++ compilers masquerading as C compilers
>      incorrectly reject 9223372036854775807.  */
> -#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
> +#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31) << 31))
>    int off_t_is_large[(LARGE_OFF_T % 2147483629 == 721
>         && LARGE_OFF_T % 2147483647 == 1)
>        ? 1 : -1];
> @@ -4798,7 +4815,7 @@ else
>      We can't simply define LARGE_OFF_T to be 9223372036854775807,
>      since some C++ compilers masquerading as C compilers
>      incorrectly reject 9223372036854775807.  */
> -#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
> +#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31) << 31))
>    int off_t_is_large[(LARGE_OFF_T % 2147483629 == 721
>         && LARGE_OFF_T % 2147483647 == 1)
>        ? 1 : -1];
> @@ -4822,7 +4839,7 @@ rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
>      We can't simply define LARGE_OFF_T to be 9223372036854775807,
>      since some C++ compilers masquerading as C compilers
>      incorrectly reject 9223372036854775807.  */
> -#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
> +#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31) << 31))
>    int off_t_is_large[(LARGE_OFF_T % 2147483629 == 721
>         && LARGE_OFF_T % 2147483647 == 1)
>        ? 1 : -1];
> @@ -12872,6 +12889,8 @@ main ()
>      if (*(data + i) != *(data3 + i))
>        return 14;
>    close (fd);
> +  free (data);
> +  free (data3);
>    return 0;
>  }
>  _ACEOF
> @@ -15239,6 +15258,52 @@ _ACEOF
>  
>  
>  
> +# Check whether --with-system-gdbinit-dir was given.
> +if test "${with_system_gdbinit_dir+set}" = set; then :
> +  withval=$with_system_gdbinit_dir;
> +    SYSTEM_GDBINIT_DIR=$withval
> +else
> +  SYSTEM_GDBINIT_DIR=
> +fi
> +
> +
> +  test "x$prefix" = xNONE && prefix="$ac_default_prefix"
> +  test "x$exec_prefix" = xNONE && exec_prefix='${prefix}'
> +  ac_define_dir=`eval echo $SYSTEM_GDBINIT_DIR`
> +  ac_define_dir=`eval echo $ac_define_dir`
> +
> +cat >>confdefs.h <<_ACEOF
> +#define SYSTEM_GDBINIT_DIR "$ac_define_dir"
> +_ACEOF
> +
> +
> +
> +
> +  if test "x$exec_prefix" = xNONE || test "x$exec_prefix" = 'x${prefix}'; then
> +     if test "x$prefix" = xNONE; then
> +     test_prefix=/usr/local
> +     else
> + test_prefix=$prefix
> +     fi
> +  else
> +     test_prefix=$exec_prefix
> +  fi
> +  value=0
> +  case ${ac_define_dir} in
> +     "${test_prefix}"|"${test_prefix}/"*|\
> + '${exec_prefix}'|'${exec_prefix}/'*)
> +     value=1
> +     ;;
> +  esac
> +
> +cat >>confdefs.h <<_ACEOF
> +#define SYSTEM_GDBINIT_DIR_RELOCATABLE $value
> +_ACEOF
> +
> +
> +
> +
> +
>  # Check whether --enable-werror was given.
>  if test "${enable_werror+set}" = set; then :
>    enableval=$enable_werror; case "${enableval}" in
> diff --git a/gdb/configure.ac b/gdb/configure.ac
> index 5a18c16405..27d67ead3e 100644
> --- a/gdb/configure.ac
> +++ b/gdb/configure.ac
> @@ -1844,6 +1844,9 @@ GDB_AC_DEFINE_RELOCATABLE(TARGET_SYSTEM_ROOT, sysroot, ${ac_define_dir})
>  GDB_AC_WITH_DIR(SYSTEM_GDBINIT, system-gdbinit,
>      [automatically load a system-wide gdbinit file],
>      [])
> +GDB_AC_WITH_DIR(SYSTEM_GDBINIT_DIR, system-gdbinit-dir,
> +    [automatically load system-wide gdbinit files from this directory],
> +    [])
>  
>  AM_GDB_WARNINGS
>  AM_GDB_UBSAN
> diff --git a/gdb/main.c b/gdb/main.c
> index a1d1904c9b..39957db4bd 100644
> --- a/gdb/main.c
> +++ b/gdb/main.c
> @@ -45,6 +45,7 @@
>  #include "event-top.h"
>  #include "infrun.h"
>  #include "gdbsupport/signals-state-save-restore.h"
> +#include <algorithm>
>  #include <vector>
>  #include "gdbsupport/pathstuff.h"
>  #include "cli/cli-style.h"
> @@ -232,11 +233,11 @@ static std::string relocate_gdbinit_path_maybe_in_datadir (std::string file)
>     to be loaded, then SYSTEM_GDBINIT (resp. HOME_GDBINIT and
>     LOCAL_GDBINIT) is set to the empty string.  */
>  static void
> -get_init_files (std::string *system_gdbinit,
> +get_init_files (std::vector<std::string> *system_gdbinit,
>   std::string *home_gdbinit,
>   std::string *local_gdbinit)
>  {
> -  static std::string sysgdbinit;
> +  static std::vector<std::string> sysgdbinit;
>    static std::string homeinit;
>    static std::string localinit;
>    static int initialized = 0;
> @@ -251,7 +252,28 @@ get_init_files (std::string *system_gdbinit,
>      relocate_gdbinit_path_maybe_in_datadir (SYSTEM_GDBINIT);
>    if (!relocated_sysgdbinit.empty () &&
>        stat (relocated_sysgdbinit.c_str (), &s) == 0)
> -    sysgdbinit = relocated_sysgdbinit;
> +    sysgdbinit.push_back(relocated_sysgdbinit);
> + }
> +      if (SYSTEM_GDBINIT_DIR[0])
> +        {

The indentation looks funny here.

> +  std::string relocated_gdbinit_dir =
> +    relocate_gdbinit_path_maybe_in_datadir (SYSTEM_GDBINIT_DIR);
> +  DIR* dir;
> +  if (!relocated_gdbinit_dir.empty () &&
> +    (dir = opendir (relocated_gdbinit_dir.c_str ())) != nullptr)

I may be wrong, but I think we discourage variable assignment inside
conditions (but things are changing so fast with C++ that I don't know
anymore!).

> +    {
> +      std::vector<std::string> files;
> +      struct dirent* ent;
> +      while ((ent = readdir (dir)) != nullptr)

Likewise.

> +        {
> +  if (ent->d_name[0] != '.')

I think we should also check 'ent->d_type' and make sure it's DT_REG (in
which case, the 'ent->d_name' check above could be removed).  It's
questionable whether we should also deal DT_LINK, but I don't think so
(for now).

> +    files.push_back (relocated_gdbinit_dir + SLASH_STRING +
> +     ent->d_name);
> + }
> +      closedir (dir);
> +      std::sort (files.begin (), files.end ());
> +      sysgdbinit.insert (sysgdbinit.end (), files.begin (), files.end ());
> +    }
>   }
>  
>        const char *homedir = getenv ("HOME");
> @@ -909,7 +931,8 @@ 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, home_gdbinit, local_gdbinit;
> +  std::vector<std::string> system_gdbinit;
> +  std::string home_gdbinit, local_gdbinit;
>    get_init_files (&system_gdbinit, &home_gdbinit, &local_gdbinit);
>  
>    /* Do these (and anything which might call wrap_here or *_filtered)
> @@ -987,7 +1010,10 @@ captured_main_1 (struct captured_main_args *context)
>       processed; it sets global parameters, which are independent of
>       what file you are debugging or what directory you are in.  */
>    if (!system_gdbinit.empty () && !inhibit_gdbinit)
> -    ret = catch_command_errors (source_script, system_gdbinit.c_str (), 0);
> +    {
> +      for (const std::string& file : system_gdbinit)
> + ret = catch_command_errors (source_script, file.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
> @@ -1205,7 +1231,7 @@ gdb_main (struct captured_main_args *args)
>  static void
>  print_gdb_help (struct ui_file *stream)
>  {
> -  std::string system_gdbinit;
> +  std::vector<std::string> system_gdbinit;
>    std::string home_gdbinit;
>    std::string local_gdbinit;
>  
> @@ -1286,9 +1312,18 @@ Other options:\n\n\
>  At startup, GDB reads the following init files and executes their commands:\n\
>  "), stream);
>    if (!system_gdbinit.empty ())
> -    fprintf_unfiltered (stream, _("\
> -   * system-wide init file: %s\n\
> -"), system_gdbinit.c_str ());
> +    {
> +      std::string output;
> +      for (size_t idx = 0; idx < system_gdbinit.size(); ++idx)

Space between variable name and parens.

> +        {
> +  output += system_gdbinit[idx];
> +  if (idx < system_gdbinit.size() - 1)
> +    output += ", ";
> + }
> +      fprintf_unfiltered (stream, _("\
> +   * system-wide init files: %s\n\
> +"), output.c_str ());
> +    }
>    if (!home_gdbinit.empty ())
>      fprintf_unfiltered (stream, _("\
>     * user-specific init file: %s\n\
> --
> 2.23.0.rc1.153.gdeed80330f-goog

This looks good, but it needs a testcase, documentation updates and a
NEWS entry.

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/
Reply | Threaded
Open this post in threaded view
|

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

Sergio Durigan Junior
In reply to this post by Sourceware - gdb-patches mailing list
On Wednesday, August 21 2019, Christian Biesinger wrote:

>> > -       char *relocated_sysgdbinit;
>> > +       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;
>>
>> This seems wrong; you're starting the iteration from
>> 'SYSTEM_GDBINIT[start]', but 'start' is 'datadir_len'.
>
> The previous version first initialized tmp_sys_gdbinit to start from
> &SYSTEM_GDBINIT[datadir_len], so I think my change is correct (and
> simpler)?

Ah, right, now I see the '&SYSTEM_GDBINIT[datadir_len]' there.  That
looks fine, then.

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/
Reply | Threaded
Open this post in threaded view
|

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

Sourceware - gdb-patches mailing list
In reply to this post by Sergio Durigan Junior
On Wed, Aug 21, 2019 at 12:19 PM Sergio Durigan Junior
<[hidden email]> wrote:

>
> On Tuesday, August 20 2019, Christian Biesinger via gdb-patches wrote:
>
> > gdb/ChangeLog:
> >
> > 2019-08-20  Christian Biesinger  <[hidden email]>
> >
> >       * 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  <[hidden email]>

        * 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
Reply | Threaded
Open this post in threaded view
|

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

Sourceware - gdb-patches mailing list
gdb/ChangeLog:

2019-08-20  Christian Biesinger  <[hidden email]>

        * 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 724b2928a7..089a7628e5 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -191,6 +191,43 @@ relocate_gdb_directory (const char *initial, int flag)
   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.rc1.153.gdeed80330f-goog

Reply | Threaded
Open this post in threaded view
|

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

Sergio Durigan Junior
In reply to this post by Sourceware - gdb-patches mailing list
On Wednesday, August 21 2019, Christian Biesinger wrote:

> On Wed, Aug 21, 2019 at 12:19 PM Sergio Durigan Junior
> <[hidden email]> wrote:
>>
>> On Tuesday, August 20 2019, Christian Biesinger via gdb-patches wrote:
>>
>> > gdb/ChangeLog:
>> >
>> > 2019-08-20  Christian Biesinger  <[hidden email]>
>> >
>> >       * 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  <[hidden email]>
>
>         * 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/
Reply | Threaded
Open this post in threaded view
|

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

Sourceware - gdb-patches mailing list
On Wed, Aug 21, 2019 at 12:47 PM Sergio Durigan Junior
<[hidden email]> wrote:

>
> On Wednesday, August 21 2019, Christian Biesinger wrote:
>
> > On Wed, Aug 21, 2019 at 12:19 PM Sergio Durigan Junior
> > <[hidden email]> wrote:
> >>
> >> On Tuesday, August 20 2019, Christian Biesinger via gdb-patches wrote:
> >>
> >> > gdb/ChangeLog:
> >> >
> >> > 2019-08-20  Christian Biesinger  <[hidden email]>
> >> >
> >> >       * 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  <[hidden email]>
> >
> >         * 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  <[hidden email]>

            * 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
Reply | Threaded
Open this post in threaded view
|

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
> <[hidden email]> wrote:
>>
>> On Wednesday, August 21 2019, Christian Biesinger wrote:
>>
>> > On Wed, Aug 21, 2019 at 12:19 PM Sergio Durigan Junior
>> > <[hidden email]> wrote:
>> >>
>> >> On Tuesday, August 20 2019, Christian Biesinger via gdb-patches wrote:
>> >>
>> >> > gdb/ChangeLog:
>> >> >
>> >> > 2019-08-20  Christian Biesinger  <[hidden email]>
>> >> >
>> >> >       * 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  <[hidden email]>
>> >
>> >         * 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  <[hidden email]>
>
>             * 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/
Reply | Threaded
Open this post in threaded view
|

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

Pedro Alves-7
In reply to this post by Sourceware - gdb-patches mailing list
On 8/20/19 11:17 PM, Christian Biesinger via gdb-patches wrote:
> 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.

Note that Fedora won't be able to replace the current mechanism with
this, because it also loads Python files from the dir:

 python
 import glob
 # glob.iglob is not available in python-2.4 (RHEL-5).
 for f in glob.glob('/etc/gdbinit.d/*.gdb'):
   gdb.execute('source %s' % f)
 for f in glob.glob('/etc/gdbinit.d/*.py'):
   gdb.execute('source %s' % f)
 end

So we'd need an additional "--with-system-python-scripts-dir"
for Python scripts or some such.

The advantage of Fedora's method IMO is that it's more flexible:
A distro or packager can decide to load gdb scripts from more than
one dir by default, e.g., from "~/gdbinit.d/", or to load gdb scripts and
python scripts from different dirs, etc.  It's similar to
/etc/bashrc loading scripts from /etc/profile.d/, etc. instead of bash
loading the scripts from a dir itself.  Of course the difference
here is that you can't walk directories with gdb's cli scripting.

Speaking of Python scripts, I guess Fedora's script should be
loading Guile scripts as well.

That isn't to say that I object to your patchset, TBC.  I just
see it a bit under the "why do it in C when you can script" light.
Of course the answer can reasonably be "I need this without Python".

Thanks,
Pedro Alves
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 3/3] Load system gdbinit files from a directory

Sergio Durigan Junior
In reply to this post by Sourceware - gdb-patches mailing list
On Tuesday, August 20 2019, Christian Biesinger via gdb-patches wrote:

> diff --git a/gdb/configure b/gdb/configure
> index cb71bbf057..e5aa2e6b3b 100755
> --- a/gdb/configure
> +++ b/gdb/configure
> @@ -693,6 +693,7 @@ WIN32LIBS
>  SER_HARDWIRE
>  WERROR_CFLAGS
>  WARN_CFLAGS
> +SYSTEM_GDBINIT_DIR
>  SYSTEM_GDBINIT
>  TARGET_SYSTEM_ROOT
>  CONFIG_LDFLAGS
> @@ -824,6 +825,7 @@ infodir
>  docdir
>  oldincludedir
>  includedir
> +runstatedir
>  localstatedir
>  sharedstatedir
>  sysconfdir
> @@ -884,6 +886,7 @@ with_libipt_prefix
>  with_included_regex
>  with_sysroot
>  with_system_gdbinit
> +with_system_gdbinit_dir
>  enable_werror
>  enable_build_warnings
>  enable_gdb_build_warnings
> @@ -956,6 +959,7 @@ datadir='${datarootdir}'
>  sysconfdir='${prefix}/etc'
>  sharedstatedir='${prefix}/com'
>  localstatedir='${prefix}/var'
> +runstatedir='${localstatedir}/run'
>  includedir='${prefix}/include'
>  oldincludedir='/usr/include'
>  docdir='${datarootdir}/doc/${PACKAGE}'
> @@ -1208,6 +1212,15 @@ do
>    | -silent | --silent | --silen | --sile | --sil)
>      silent=yes ;;
>  
> +  -runstatedir | --runstatedir | --runstatedi | --runstated \
> +  | --runstate | --runstat | --runsta | --runst | --runs \
> +  | --run | --ru | --r)
> +    ac_prev=runstatedir ;;
> +  -runstatedir=* | --runstatedir=* | --runstatedi=* | --runstated=* \
> +  | --runstate=* | --runstat=* | --runsta=* | --runst=* | --runs=* \
> +  | --run=* | --ru=* | --r=*)
> +    runstatedir=$ac_optarg ;;
> +

Something else I forgot to comment: this 'runstatedir' addition seems
unrelated to the patch; it's probably due to the autoconf version you're
using to regenerate these files.

Make sure you have the correct versions installed:

  https://sourceware.org/gdb/wiki/DeveloperTips#Editing_configure.ac

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/
Reply | Threaded
Open this post in threaded view
|

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

Sourceware - gdb-patches mailing list
In reply to this post by Pedro Alves-7
On Wed, Aug 21, 2019 at 1:13 PM Pedro Alves <[hidden email]> wrote:

> On 8/20/19 11:17 PM, Christian Biesinger via gdb-patches wrote:
> > 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.
>
> Note that Fedora won't be able to replace the current mechanism with
> this, because it also loads Python files from the dir:
>
>  python
>  import glob
>  # glob.iglob is not available in python-2.4 (RHEL-5).
>  for f in glob.glob('/etc/gdbinit.d/*.gdb'):
>    gdb.execute('source %s' % f)
>  for f in glob.glob('/etc/gdbinit.d/*.py'):
>    gdb.execute('source %s' % f)
>  end
>
> So we'd need an additional "--with-system-python-scripts-dir"
> for Python scripts or some such.

That's a good point. I don't think -with-system-python-scripts-dir
would really work for Fedora either since the directory there is
currently the same but I could change the patch to use
get_ext_lang_of_file/ext_lang_script_sourcer (which is effectively
pretty similar to the current Fedora gdbinit)

> The advantage of Fedora's method IMO is that it's more flexible:
> A distro or packager can decide to load gdb scripts from more than
> one dir by default, e.g., from "~/gdbinit.d/", or to load gdb scripts and
> python scripts from different dirs, etc.  It's similar to
> /etc/bashrc loading scripts from /etc/profile.d/, etc. instead of bash
> loading the scripts from a dir itself.  Of course the difference
> here is that you can't walk directories with gdb's cli scripting.

I guess it may be possible to add glob support to the source command.

> Speaking of Python scripts, I guess Fedora's script should be
> loading Guile scripts as well.

I agree. (Certainly if GDB shipped a default gdbinit like that, it
should do that)

> That isn't to say that I object to your patchset, TBC.  I just
> see it a bit under the "why do it in C when you can script" light.
> Of course the answer can reasonably be "I need this without Python".

So there were two reasons why I liked this approach:
- This ships something with GDB, that will work on all Linux
distributions the same way, as long as they agree on a directory
location
- It works without Python enabled

I'd also be happy with shipping a default gdbinit file that does a
similar thing, especially if we were to add glob support to the source
command.

I dunno, anyone else have thoughts on this?

Christian
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 3/3] Load system gdbinit files from a directory

Sourceware - gdb-patches mailing list
In reply to this post by Sergio Durigan Junior
On Wed, Aug 21, 2019 at 1:15 PM Sergio Durigan Junior
<[hidden email]> wrote:

>
> On Tuesday, August 20 2019, Christian Biesinger via gdb-patches wrote:
>
> > diff --git a/gdb/configure b/gdb/configure
> > index cb71bbf057..e5aa2e6b3b 100755
> > --- a/gdb/configure
> > +++ b/gdb/configure
> > @@ -693,6 +693,7 @@ WIN32LIBS
> >  SER_HARDWIRE
> >  WERROR_CFLAGS
> >  WARN_CFLAGS
> > +SYSTEM_GDBINIT_DIR
> >  SYSTEM_GDBINIT
> >  TARGET_SYSTEM_ROOT
> >  CONFIG_LDFLAGS
> > @@ -824,6 +825,7 @@ infodir
> >  docdir
> >  oldincludedir
> >  includedir
> > +runstatedir
> >  localstatedir
> >  sharedstatedir
> >  sysconfdir
> > @@ -884,6 +886,7 @@ with_libipt_prefix
> >  with_included_regex
> >  with_sysroot
> >  with_system_gdbinit
> > +with_system_gdbinit_dir
> >  enable_werror
> >  enable_build_warnings
> >  enable_gdb_build_warnings
> > @@ -956,6 +959,7 @@ datadir='${datarootdir}'
> >  sysconfdir='${prefix}/etc'
> >  sharedstatedir='${prefix}/com'
> >  localstatedir='${prefix}/var'
> > +runstatedir='${localstatedir}/run'
> >  includedir='${prefix}/include'
> >  oldincludedir='/usr/include'
> >  docdir='${datarootdir}/doc/${PACKAGE}'
> > @@ -1208,6 +1212,15 @@ do
> >    | -silent | --silent | --silen | --sile | --sil)
> >      silent=yes ;;
> >
> > +  -runstatedir | --runstatedir | --runstatedi | --runstated \
> > +  | --runstate | --runstat | --runsta | --runst | --runs \
> > +  | --run | --ru | --r)
> > +    ac_prev=runstatedir ;;
> > +  -runstatedir=* | --runstatedir=* | --runstatedi=* | --runstated=* \
> > +  | --runstate=* | --runstat=* | --runsta=* | --runst=* | --runs=* \
> > +  | --run=* | --ru=* | --r=*)
> > +    runstatedir=$ac_optarg ;;
> > +
>
> Something else I forgot to comment: this 'runstatedir' addition seems
> unrelated to the patch; it's probably due to the autoconf version you're
> using to regenerate these files.
>
> Make sure you have the correct versions installed:
>
>   https://sourceware.org/gdb/wiki/DeveloperTips#Editing_configure.ac

Turns out the debian version of autoconf has a local patch to do this
runstate thing :(
https://sources.debian.org/src/autoconf/2.69-11/debian/patches/add-runstatedir.patch/

Anyway, I'll hold off on updating this patch pending the discussion
about the approach in the other thread. (But patches 1 and 2 may still
be good to get committed)

Christian
Reply | Threaded
Open this post in threaded view
|

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

Sergio Durigan Junior
In reply to this post by Sourceware - gdb-patches mailing list
On Wednesday, August 21 2019, Christian Biesinger via gdb-patches wrote:

> On Wed, Aug 21, 2019 at 1:13 PM Pedro Alves <[hidden email]> wrote:
>> On 8/20/19 11:17 PM, Christian Biesinger via gdb-patches wrote:
>> > 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.
>>
>> Note that Fedora won't be able to replace the current mechanism with
>> this, because it also loads Python files from the dir:
>>
>>  python
>>  import glob
>>  # glob.iglob is not available in python-2.4 (RHEL-5).
>>  for f in glob.glob('/etc/gdbinit.d/*.gdb'):
>>    gdb.execute('source %s' % f)
>>  for f in glob.glob('/etc/gdbinit.d/*.py'):
>>    gdb.execute('source %s' % f)
>>  end
>>
>> So we'd need an additional "--with-system-python-scripts-dir"
>> for Python scripts or some such.
>
> That's a good point. I don't think -with-system-python-scripts-dir
> would really work for Fedora either since the directory there is
> currently the same but I could change the patch to use
> get_ext_lang_of_file/ext_lang_script_sourcer (which is effectively
> pretty similar to the current Fedora gdbinit)

I don't think we need to have separate directories for
Python/Guile/"native" GDB scripts.  I think we should consolidate all of
these scripts inside one directory.

>> Speaking of Python scripts, I guess Fedora's script should be
>> loading Guile scripts as well.

Guile support has been (temporarily?) disabled in Fedora GDB.

> I agree. (Certainly if GDB shipped a default gdbinit like that, it
> should do that)


>> That isn't to say that I object to your patchset, TBC.  I just
>> see it a bit under the "why do it in C when you can script" light.
>> Of course the answer can reasonably be "I need this without Python".
>
> So there were two reasons why I liked this approach:
> - This ships something with GDB, that will work on all Linux
> distributions the same way, as long as they agree on a directory
> location

I think /etc/gdbinit.d/ is a nice location, and I intend to keep using
it for Fedora GDB and to make Debian GDB use it as well.

I agree that supporting other directories, like ~/.gdbinit.d/, would be
great.

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