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

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

I will send an updated version in a moment. More comments below.

On Wed, Aug 21, 2019 at 12:13 PM Sergio Durigan Junior
<sergiodj@redhat.com> 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  <cbiesinger@google.com>
> >
> >       * 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