From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 106837 invoked by alias); 21 Aug 2019 17:29:24 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 106829 invoked by uid 89); 21 Aug 2019 17:29:23 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-30.6 required=5.0 tests=AWL,BAYES_00,ENV_AND_HDR_SPF_MATCH,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS,USER_IN_DEF_SPF_WL autolearn=ham version=3.3.1 spammy=HX-HELO:sk:mail-ot, HX-Spam-Relays-External:209.85.210.67, H*RU:209.85.210.67 X-HELO: mail-ot1-f67.google.com Received: from mail-ot1-f67.google.com (HELO mail-ot1-f67.google.com) (209.85.210.67) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 21 Aug 2019 17:29:21 +0000 Received: by mail-ot1-f67.google.com with SMTP id j7so2800196ota.9 for ; Wed, 21 Aug 2019 10:29:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=ZTdt+eEnE36DV+UItd9WJNBlHrwXHVo4FU/n2B1KqHc=; b=WxbMh/WHDtZ+Tp06lIn/Q41/49JMnh2u/ZMWh0xdJQMk4quPPWBUV2/MHoikkCGamP iKFELXUU1jRw5lnr3K1ZMv0c/uxRoV1cjc70AYqXJLjqOYDv2QFe02zn/bbayA12UiDn aKASA80f9gOav1yPeYNKYA7Y0AU8XbF2NJp4xqZxF9iK00zS4OifcIPJ4ZK8D1laFrlt KrK6szAcBeh62n/NBA4w4IKrNECNopEEXmtW+8kml+7ZcxNuuM0Rjg62Mt3Am3jPJ95d oAqrC0ZBB/YHl7QC0ew81YXf0Ws2Oh5sMUZlcOM3NyV7peqo6jYoF12nW4bRSeY6uTjm l/WA== MIME-Version: 1.0 References: <20190820221745.147370-1-cbiesinger@google.com> <20190820221745.147370-2-cbiesinger@google.com> <87v9uqjvav.fsf@redhat.com> In-Reply-To: <87v9uqjvav.fsf@redhat.com> From: "Christian Biesinger via gdb-patches" Reply-To: Christian Biesinger Date: Wed, 21 Aug 2019 17:29:00 -0000 Message-ID: Subject: Re: [PATCH 1/3] Refactor get_init_files to use std::string To: Sergio Durigan Junior Cc: Christian Biesinger via gdb-patches Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2019-08/txt/msg00492.txt.bz2 I will send an updated version in a moment. More comments below. On Wed, Aug 21, 2019 at 12:13 PM Sergio Durigan Junior 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 > > > > * 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 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