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

["Christian Biesinger via gdb-patches" <gdb-patches@sourceware.org>]

On Wed, Aug 21, 2019 at 1:32 PM Sergio Durigan Junior
<sergiodj@redhat.com> wrote:
>
> On Tuesday, August 20 2019, Christian Biesinger via gdb-patches wrote:
>
> > gdb/ChangeLog:
> >
> > 2019-08-20  Christian Biesinger  <cbiesinger@google.com>
> >
> >       * config.in: Add SYSTEM_GDBINIT_DIR.
>
> config.in is also regenerated.

Done.

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

Done.

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

Can you elaborate? I think it is correct, but perhaps the diff makes
it look weird because the + is messing with the tabs.

> > +       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!).

Done.

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

Done.

> > +             {
> > +               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).

Done.

> > +                 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.

Done, here and a couple lines below.

> > +        {
> > +       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.

Added NEWS and documentation, but how should I write a testcase for
it? It depends on a configure option... (we don't seem to have a test
for --with-system-gdbinit either, unless I missed it)

Will send a new patch shortly, also with a fixed configure script.

Christian