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

Message ID 20190820221745.147370-2-cbiesinger@google.com
State Superseded
Headers show
Series
  • Load gdbinit files from a directory
Related show

Commit Message

H.J. Lu via Gdb-patches Aug. 20, 2019, 10:17 p.m.
To avoid manual memory management.

Tested on buildbot.

gdb/ChangeLog:

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

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

-- 
2.23.0.rc1.153.gdeed80330f-goog

Comments

Sergio Durigan Junior Aug. 21, 2019, 5:13 p.m. | #1
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' :-).

> -	  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/
H.J. Lu via Gdb-patches Aug. 21, 2019, 5:28 p.m. | #2
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
Sergio Durigan Junior Aug. 21, 2019, 5:33 p.m. | #3
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/

Patch

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\