This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 1/3] Refactor get_init_files to use std::string
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