From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 102538 invoked by alias); 26 Aug 2019 00:25:13 -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 102511 invoked by uid 89); 26 Aug 2019 00:25:10 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-30.5 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=installation, Fine, regenerated, ent X-HELO: mail-oi1-f196.google.com Received: from mail-oi1-f196.google.com (HELO mail-oi1-f196.google.com) (209.85.167.196) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 26 Aug 2019 00:25:04 +0000 Received: by mail-oi1-f196.google.com with SMTP id o6so10843030oic.9 for ; Sun, 25 Aug 2019 17:25:03 -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=me31xrJ5q6lLrgynAfr2Rve2On5V+RQhxqr1oFU/vvE=; b=lmiLrWZpCEaOhrFyMchtq8mP0Wko4yhbHgbbspm/sZe3jKMLY+eOR1m+qntHE0dWH2 J23oM+2Q+jE5FX+YvuDdvPEll4kZ9l+iYKgpnMSYPGleP2e6HBEr4paftI0+UkNUjpdP 1aXVX4paeAppGiAWZYWAkD9mxgUh3ueMk/pOHfyHv2Hj7TUa7Fb+I3NMjSpUcD5KH68m l5IU8/76m9q+IYBqSroM/QSqPiprZ1FpPovEKeT7j9uXgpED/38ofXQEqIPfMf6PZh6w FwZactFTJqTqsdoy+hzSikH5z7T9WY+DSpTtLyy/ZobUgQRXh7o9yq7YxM0bh3ppHsJ1 +0Gg== MIME-Version: 1.0 References: <20190820221745.147370-1-cbiesinger@google.com> <20190820221745.147370-4-cbiesinger@google.com> <87mug2jugb.fsf@redhat.com> In-Reply-To: <87mug2jugb.fsf@redhat.com> From: "Christian Biesinger via gdb-patches" Reply-To: Christian Biesinger Date: Mon, 26 Aug 2019 00:25:00 -0000 Message-ID: Subject: Re: [PATCH 3/3] Load system gdbinit files from a directory 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/msg00578.txt.bz2 On Wed, Aug 21, 2019 at 1:32 PM Sergio Durigan Junior wrote: > > On Tuesday, August 20 2019, Christian Biesinger via gdb-patches wrote: > > > gdb/ChangeLog: > > > > 2019-08-20 Christian Biesinger > > > > * config.in: Add SYSTEM_GDBINIT_DIR. > > config.in is also regenerated. Done. > > * configure: Regenerate. > > * configure.ac: Add new option --with-system-gdbinit-dir. > > * main.c (get_init_files): Change system_gdbinit argument to > > a vector and return the files in SYSTEM_GDBINIT_DIR in > > addition to SYSTEM_GDBINIT. > > (captured_main_1): Update. > > (print_gdb_help): Update. > > You'll need a descriptive commit log. Done. > > --- > > gdb/config.in | 3 ++ > > gdb/configure | 77 ++++++++++++++++++++++++++++++++++++++++++++---- > > gdb/configure.ac | 3 ++ > > gdb/main.c | 53 +++++++++++++++++++++++++++------ > > 4 files changed, 121 insertions(+), 15 deletions(-) > > > > diff --git a/gdb/config.in b/gdb/config.in > > index 26ca02f6a3..ca523fe4ab 100644 > > --- a/gdb/config.in > > +++ b/gdb/config.in > > @@ -684,6 +684,9 @@ > > /* automatically load a system-wide gdbinit file */ > > #undef SYSTEM_GDBINIT > > > > +/* automatically load system-wide gdbinit files from this dir */ > > +#undef SYSTEM_GDBINIT_DIR > > + > > /* Define if the system-gdbinit directory should be relocated when GDB is > > moved. */ > > #undef SYSTEM_GDBINIT_RELOCATABLE > > diff --git a/gdb/configure b/gdb/configure > > index cb71bbf057..e5aa2e6b3b 100755 > > --- a/gdb/configure > > +++ b/gdb/configure > > @@ -693,6 +693,7 @@ WIN32LIBS > > SER_HARDWIRE > > WERROR_CFLAGS > > WARN_CFLAGS > > +SYSTEM_GDBINIT_DIR > > SYSTEM_GDBINIT > > TARGET_SYSTEM_ROOT > > CONFIG_LDFLAGS > > @@ -824,6 +825,7 @@ infodir > > docdir > > oldincludedir > > includedir > > +runstatedir > > localstatedir > > sharedstatedir > > sysconfdir > > @@ -884,6 +886,7 @@ with_libipt_prefix > > with_included_regex > > with_sysroot > > with_system_gdbinit > > +with_system_gdbinit_dir > > enable_werror > > enable_build_warnings > > enable_gdb_build_warnings > > @@ -956,6 +959,7 @@ datadir='${datarootdir}' > > sysconfdir='${prefix}/etc' > > sharedstatedir='${prefix}/com' > > localstatedir='${prefix}/var' > > +runstatedir='${localstatedir}/run' > > includedir='${prefix}/include' > > oldincludedir='/usr/include' > > docdir='${datarootdir}/doc/${PACKAGE}' > > @@ -1208,6 +1212,15 @@ do > > | -silent | --silent | --silen | --sile | --sil) > > silent=yes ;; > > > > + -runstatedir | --runstatedir | --runstatedi | --runstated \ > > + | --runstate | --runstat | --runsta | --runst | --runs \ > > + | --run | --ru | --r) > > + ac_prev=runstatedir ;; > > + -runstatedir=* | --runstatedir=* | --runstatedi=* | --runstated=* \ > > + | --runstate=* | --runstat=* | --runsta=* | --runst=* | --runs=* \ > > + | --run=* | --ru=* | --r=*) > > + runstatedir=$ac_optarg ;; > > + > > -sbindir | --sbindir | --sbindi | --sbind | --sbin | --sbi | --sb) > > ac_prev=sbindir ;; > > -sbindir=* | --sbindir=* | --sbindi=* | --sbind=* | --sbin=* \ > > @@ -1345,7 +1358,7 @@ fi > > for ac_var in exec_prefix prefix bindir sbindir libexecdir datarootdir \ > > datadir sysconfdir sharedstatedir localstatedir includedir \ > > oldincludedir docdir infodir htmldir dvidir pdfdir psdir \ > > - libdir localedir mandir > > + libdir localedir mandir runstatedir > > do > > eval ac_val=\$$ac_var > > # Remove trailing slashes. > > @@ -1498,6 +1511,7 @@ Fine tuning of the installation directories: > > --sysconfdir=DIR read-only single-machine data [PREFIX/etc] > > --sharedstatedir=DIR modifiable architecture-independent data [PREFIX/com] > > --localstatedir=DIR modifiable single-machine data [PREFIX/var] > > + --runstatedir=DIR modifiable per-process data [LOCALSTATEDIR/run] > > --libdir=DIR object code libraries [EPREFIX/lib] > > --includedir=DIR C header files [PREFIX/include] > > --oldincludedir=DIR C header files for non-gcc [/usr/include] > > @@ -1618,6 +1632,9 @@ Optional Packages: > > --with-sysroot[=DIR] search for usr/lib et al within DIR > > --with-system-gdbinit=PATH > > automatically load a system-wide gdbinit file > > + --with-system-gdbinit-dir=PATH > > + automatically load system-wide gdbinit files from > > + this directory > > --with-lzma support lzma compression (auto/yes/no) > > --with-liblzma-prefix[=DIR] search for liblzma in DIR/include and DIR/lib > > --without-liblzma-prefix don't search for liblzma in includedir and libdir > > @@ -4683,7 +4700,7 @@ else > > We can't simply define LARGE_OFF_T to be 9223372036854775807, > > since some C++ compilers masquerading as C compilers > > incorrectly reject 9223372036854775807. */ > > -#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62)) > > +#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31) << 31)) > > int off_t_is_large[(LARGE_OFF_T % 2147483629 == 721 > > && LARGE_OFF_T % 2147483647 == 1) > > ? 1 : -1]; > > @@ -4729,7 +4746,7 @@ else > > We can't simply define LARGE_OFF_T to be 9223372036854775807, > > since some C++ compilers masquerading as C compilers > > incorrectly reject 9223372036854775807. */ > > -#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62)) > > +#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31) << 31)) > > int off_t_is_large[(LARGE_OFF_T % 2147483629 == 721 > > && LARGE_OFF_T % 2147483647 == 1) > > ? 1 : -1]; > > @@ -4753,7 +4770,7 @@ rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext > > We can't simply define LARGE_OFF_T to be 9223372036854775807, > > since some C++ compilers masquerading as C compilers > > incorrectly reject 9223372036854775807. */ > > -#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62)) > > +#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31) << 31)) > > int off_t_is_large[(LARGE_OFF_T % 2147483629 == 721 > > && LARGE_OFF_T % 2147483647 == 1) > > ? 1 : -1]; > > @@ -4798,7 +4815,7 @@ else > > We can't simply define LARGE_OFF_T to be 9223372036854775807, > > since some C++ compilers masquerading as C compilers > > incorrectly reject 9223372036854775807. */ > > -#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62)) > > +#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31) << 31)) > > int off_t_is_large[(LARGE_OFF_T % 2147483629 == 721 > > && LARGE_OFF_T % 2147483647 == 1) > > ? 1 : -1]; > > @@ -4822,7 +4839,7 @@ rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext > > We can't simply define LARGE_OFF_T to be 9223372036854775807, > > since some C++ compilers masquerading as C compilers > > incorrectly reject 9223372036854775807. */ > > -#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62)) > > +#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31) << 31)) > > int off_t_is_large[(LARGE_OFF_T % 2147483629 == 721 > > && LARGE_OFF_T % 2147483647 == 1) > > ? 1 : -1]; > > @@ -12872,6 +12889,8 @@ main () > > if (*(data + i) != *(data3 + i)) > > return 14; > > close (fd); > > + free (data); > > + free (data3); > > return 0; > > } > > _ACEOF > > @@ -15239,6 +15258,52 @@ _ACEOF > > > > > > > > +# Check whether --with-system-gdbinit-dir was given. > > +if test "${with_system_gdbinit_dir+set}" = set; then : > > + withval=$with_system_gdbinit_dir; > > + SYSTEM_GDBINIT_DIR=$withval > > +else > > + SYSTEM_GDBINIT_DIR= > > +fi > > + > > + > > + test "x$prefix" = xNONE && prefix="$ac_default_prefix" > > + test "x$exec_prefix" = xNONE && exec_prefix='${prefix}' > > + ac_define_dir=`eval echo $SYSTEM_GDBINIT_DIR` > > + ac_define_dir=`eval echo $ac_define_dir` > > + > > +cat >>confdefs.h <<_ACEOF > > +#define SYSTEM_GDBINIT_DIR "$ac_define_dir" > > +_ACEOF > > + > > + > > + > > + > > + if test "x$exec_prefix" = xNONE || test "x$exec_prefix" = 'x${prefix}'; then > > + if test "x$prefix" = xNONE; then > > + test_prefix=/usr/local > > + else > > + test_prefix=$prefix > > + fi > > + else > > + test_prefix=$exec_prefix > > + fi > > + value=0 > > + case ${ac_define_dir} in > > + "${test_prefix}"|"${test_prefix}/"*|\ > > + '${exec_prefix}'|'${exec_prefix}/'*) > > + value=1 > > + ;; > > + esac > > + > > +cat >>confdefs.h <<_ACEOF > > +#define SYSTEM_GDBINIT_DIR_RELOCATABLE $value > > +_ACEOF > > + > > + > > + > > + > > + > > # Check whether --enable-werror was given. > > if test "${enable_werror+set}" = set; then : > > enableval=$enable_werror; case "${enableval}" in > > diff --git a/gdb/configure.ac b/gdb/configure.ac > > index 5a18c16405..27d67ead3e 100644 > > --- a/gdb/configure.ac > > +++ b/gdb/configure.ac > > @@ -1844,6 +1844,9 @@ GDB_AC_DEFINE_RELOCATABLE(TARGET_SYSTEM_ROOT, sysroot, ${ac_define_dir}) > > GDB_AC_WITH_DIR(SYSTEM_GDBINIT, system-gdbinit, > > [automatically load a system-wide gdbinit file], > > []) > > +GDB_AC_WITH_DIR(SYSTEM_GDBINIT_DIR, system-gdbinit-dir, > > + [automatically load system-wide gdbinit files from this directory], > > + []) > > > > AM_GDB_WARNINGS > > AM_GDB_UBSAN > > diff --git a/gdb/main.c b/gdb/main.c > > index a1d1904c9b..39957db4bd 100644 > > --- a/gdb/main.c > > +++ b/gdb/main.c > > @@ -45,6 +45,7 @@ > > #include "event-top.h" > > #include "infrun.h" > > #include "gdbsupport/signals-state-save-restore.h" > > +#include > > #include > > #include "gdbsupport/pathstuff.h" > > #include "cli/cli-style.h" > > @@ -232,11 +233,11 @@ static std::string relocate_gdbinit_path_maybe_in_datadir (std::string file) > > to be loaded, then SYSTEM_GDBINIT (resp. HOME_GDBINIT and > > LOCAL_GDBINIT) is set to the empty string. */ > > static void > > -get_init_files (std::string *system_gdbinit, > > +get_init_files (std::vector *system_gdbinit, > > std::string *home_gdbinit, > > std::string *local_gdbinit) > > { > > - static std::string sysgdbinit; > > + static std::vector sysgdbinit; > > static std::string homeinit; > > static std::string localinit; > > static int initialized = 0; > > @@ -251,7 +252,28 @@ get_init_files (std::string *system_gdbinit, > > relocate_gdbinit_path_maybe_in_datadir (SYSTEM_GDBINIT); > > if (!relocated_sysgdbinit.empty () && > > stat (relocated_sysgdbinit.c_str (), &s) == 0) > > - sysgdbinit = relocated_sysgdbinit; > > + sysgdbinit.push_back(relocated_sysgdbinit); > > + } > > + if (SYSTEM_GDBINIT_DIR[0]) > > + { > > The indentation looks funny here. Can you elaborate? I think it is correct, but perhaps the diff makes it look weird because the + is messing with the tabs. > > + std::string relocated_gdbinit_dir = > > + relocate_gdbinit_path_maybe_in_datadir (SYSTEM_GDBINIT_DIR); > > + DIR* dir; > > + if (!relocated_gdbinit_dir.empty () && > > + (dir = opendir (relocated_gdbinit_dir.c_str ())) != nullptr) > > I may be wrong, but I think we discourage variable assignment inside > conditions (but things are changing so fast with C++ that I don't know > anymore!). Done. > > + { > > + std::vector files; > > + struct dirent* ent; > > + while ((ent = readdir (dir)) != nullptr) > > Likewise. Done. > > + { > > + if (ent->d_name[0] != '.') > > I think we should also check 'ent->d_type' and make sure it's DT_REG (in > which case, the 'ent->d_name' check above could be removed). It's > questionable whether we should also deal DT_LINK, but I don't think so > (for now). Done. > > + files.push_back (relocated_gdbinit_dir + SLASH_STRING + > > + ent->d_name); > > + } > > + closedir (dir); > > + std::sort (files.begin (), files.end ()); > > + sysgdbinit.insert (sysgdbinit.end (), files.begin (), files.end ()); > > + } > > } > > > > const char *homedir = getenv ("HOME"); > > @@ -909,7 +931,8 @@ 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; > > + std::vector system_gdbinit; > > + std::string home_gdbinit, local_gdbinit; > > get_init_files (&system_gdbinit, &home_gdbinit, &local_gdbinit); > > > > /* Do these (and anything which might call wrap_here or *_filtered) > > @@ -987,7 +1010,10 @@ captured_main_1 (struct captured_main_args *context) > > processed; it sets global parameters, which are independent of > > what file you are debugging or what directory you are in. */ > > if (!system_gdbinit.empty () && !inhibit_gdbinit) > > - ret = catch_command_errors (source_script, system_gdbinit.c_str (), 0); > > + { > > + for (const std::string& file : system_gdbinit) > > + ret = catch_command_errors (source_script, file.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 > > @@ -1205,7 +1231,7 @@ gdb_main (struct captured_main_args *args) > > static void > > print_gdb_help (struct ui_file *stream) > > { > > - std::string system_gdbinit; > > + std::vector system_gdbinit; > > std::string home_gdbinit; > > std::string local_gdbinit; > > > > @@ -1286,9 +1312,18 @@ Other options:\n\n\ > > At startup, GDB reads the following init files and executes their commands:\n\ > > "), stream); > > if (!system_gdbinit.empty ()) > > - fprintf_unfiltered (stream, _("\ > > - * system-wide init file: %s\n\ > > -"), system_gdbinit.c_str ()); > > + { > > + std::string output; > > + for (size_t idx = 0; idx < system_gdbinit.size(); ++idx) > > Space between variable name and parens. Done, here and a couple lines below. > > + { > > + output += system_gdbinit[idx]; > > + if (idx < system_gdbinit.size() - 1) > > + output += ", "; > > + } > > + fprintf_unfiltered (stream, _("\ > > + * system-wide init files: %s\n\ > > +"), output.c_str ()); > > + } > > if (!home_gdbinit.empty ()) > > fprintf_unfiltered (stream, _("\ > > * user-specific init file: %s\n\ > > -- > > 2.23.0.rc1.153.gdeed80330f-goog > > This looks good, but it needs a testcase, documentation updates and a > NEWS entry. Added NEWS and documentation, but how should I write a testcase for it? It depends on a configure option... (we don't seem to have a test for --with-system-gdbinit either, unless I missed it) Will send a new patch shortly, also with a fixed configure script. Christian