From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 53655 invoked by alias); 20 Sep 2017 13:14:49 -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 53341 invoked by uid 89); 20 Sep 2017 13:14:49 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=Hx-languages-length:5131 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 20 Sep 2017 13:14:47 +0000 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 621C0883C6 for ; Wed, 20 Sep 2017 13:14:46 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 621C0883C6 Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=palves@redhat.com Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id 60F6D5D6A3; Wed, 20 Sep 2017 13:14:40 +0000 (UTC) Subject: Re: [PATCH v2 3/5] Introduce gdb_chdir To: Sergio Durigan Junior , GDB Patches References: <20170912042325.14927-1-sergiodj@redhat.com> <20170919042842.9210-1-sergiodj@redhat.com> <20170919042842.9210-4-sergiodj@redhat.com> From: Pedro Alves Message-ID: <0ef70723-ac23-747a-636e-5fc3f9214531@redhat.com> Date: Wed, 20 Sep 2017 13:14:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20170919042842.9210-4-sergiodj@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-09/txt/msg00485.txt.bz2 On 09/19/2017 05:28 AM, Sergio Durigan Junior wrote: > In order to be able to change the inferior's directory before its > execution, it is necessary to perform a tilde expansion of the > directory provided by the user and then chdir into the resulting dir. > This is what gdb_chdir does. > > Unfortunately it is not possible to use "tilde_expand" from readline > because this is common code and gdbserver doesn't use readline. For > that reason I decided to go with "glob" and its GNU extension, > GLOB_TILDE. With the import of the "glob" module from gnulib, this is > a no-brainer. > > gdb/ChangeLog: > yyyy-mm-dd Sergio Durigan Junior > > * Makefile.in (SFILES): Add gdb_chdir.c. > (HFILES_NO_SRCDIR): Add gdb_chdir.h. > (COMMON_OBS): Add gdb_chdir.o. > * cli/cli-cmds.c: Include "gdb_chdir.h". > (cd_command): Use "gdb_chdir" instead of "tilde_expand > plus chdir". > * common/gdb_chdir.c: New file. > * common/gdb_chdir.h: Likewise. > > gdb/gdbserver/ChangeLog: > yyyy-mm-dd Sergio Durigan Junior > > * Makefile.in (SFILES): Add $(srcdir)/common/gdb_chdir.c. > (OBS): Add gdb_chdir.o. > --- > gdb/Makefile.in | 3 ++ > gdb/cli/cli-cmds.c | 20 +++++----- > gdb/common/gdb_chdir.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++ > gdb/common/gdb_chdir.h | 26 +++++++++++++ > gdb/gdbserver/Makefile.in | 2 + > 5 files changed, 136 insertions(+), 11 deletions(-) > create mode 100644 gdb/common/gdb_chdir.c > create mode 100644 gdb/common/gdb_chdir.h > > diff --git a/gdb/Makefile.in b/gdb/Makefile.in > index 9dfc117b2f..1f093e6c0f 100644 > --- a/gdb/Makefile.in > +++ b/gdb/Makefile.in > @@ -1245,6 +1245,7 @@ SFILES = \ > common/filestuff.c \ > common/format.c \ > common/job-control.c \ > + common/gdb_chdir.c \ > common/gdb_vecs.c \ > common/new-op.c \ > common/print-utils.c \ > @@ -1529,6 +1530,7 @@ HFILES_NO_SRCDIR = \ > common/fileio.h \ > common/format.h \ > common/gdb_assert.h \ > + common/gdb_chdir.h \ > common/gdb_locale.h \ > common/gdb_setjmp.h \ > common/gdb_signals.h \ > @@ -1734,6 +1736,7 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \ > frame-unwind.o \ > gcore.o \ > gdb_bfd.o \ > + gdb_chdir.o \ > gdb-dlfcn.o \ > gdb_obstack.o \ > gdb_regex.o \ > diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c > index 85d6d21113..653dd56a64 100644 > --- a/gdb/cli/cli-cmds.c > +++ b/gdb/cli/cli-cmds.c > @@ -54,6 +54,8 @@ > #include "tui/tui.h" /* For tui_active et.al. */ > #endif > > +#include "gdb_chdir.h" > + > #include > #include > #include > @@ -408,12 +410,7 @@ cd_command (char *dir, int from_tty) > repeat might be useful but is more likely to be a mistake. */ > dont_repeat (); > > - gdb::unique_xmalloc_ptr dir_holder > - (tilde_expand (dir != NULL ? dir : "~")); > - dir = dir_holder.get (); > - > - if (chdir (dir) < 0) > - perror_with_name (dir); > + gdb_chdir (dir != NULL ? dir : "~"); > > #ifdef HAVE_DOS_BASED_FILE_SYSTEM > /* There's too much mess with DOSish names like "d:", "d:.", > @@ -436,20 +433,21 @@ cd_command (char *dir, int from_tty) > len--; > } > > - dir_holder.reset (savestring (dir, len)); > - if (IS_ABSOLUTE_PATH (dir_holder.get ())) > + std::string newdir = std::string (dir, len); > + const char *newdir_str = newdir.c_str (); > + if (IS_ABSOLUTE_PATH (newdir_str)) > { > xfree (current_directory); > - current_directory = dir_holder.release (); > + current_directory = xstrdup (newdir_str); This introduces one extra deep string dup. One to construct the std::string, and another here in this xstrdup. How about simply, above: - dir_holder.reset (savestring (dir, len)); + gdb::unique_xmalloc_ptr dir_holder (savestring (dir, len)) But more importantly, isn't there a behavior change here? Before, current_directory would be a copy of the the expand path, while after the patch, it's a copy of the input string, before expansion. Right? > } > else > { > if (IS_DIR_SEPARATOR (current_directory[strlen (current_directory) - 1])) > - current_directory = concat (current_directory, dir_holder.get (), > + current_directory = concat (current_directory, newdir_str, > (char *) NULL); > else > current_directory = concat (current_directory, SLASH_STRING, > - dir_holder.get (), (char *) NULL); > + newdir_str, (char *) NULL); > } > + /* Destroy the object and free M_GLOB. */ > + ~gdb_glob () > + { > + globfree (&m_glob); > + } > + > + /* Return the GL_PATHC component of M_GLOB. */ > + int > + pathc () const > + { > + return m_glob.gl_pathc; > + } We've been putting type and function name in the same line in inline member functions. That's the de facto standard in GCC as well, AFAICS. > + > + /* Return the GL_PATHV component of M_GLOB. */ > + char ** > + pathv () const > + { > + return m_glob.gl_pathv; > + } Ditto. > +#ifndef HAVE_GDB_CHDIR_H > +#define HAVE_GDB_CHDIR_H > + > +/* Perform a "chdir" to DIR, doing the proper tilde expansion before. */ > +extern void gdb_chdir (const char *dir); Nit, I'd drop "the proper", as it doesn't see to add value. I.e., what's proper and what's not proper? Thanks, Pedro Alves