Re: [PATCH] gdb/testsuite: Installed-GDB testing & data-directory

[Andrew Burgess <aburgess@redhat.com>]

* Pedro Alves <pedro@palves.net> [2022-03-18 16:10:36 +0000]:

> In testsuite/README, we suggest that you can run the testsuite against
> some other GDB binary by using:
> 
>     make check RUNTESTFLAGS=GDB=/usr/bin/gdb
> 
> However, that example isn't fully correct, because with that command
> line, the testsuite will still pass
> 
>   -data-directory=[pwd]/../data-directory
> 
> to /usr/bin/gdb, like e.g.:
> 
>   ...
>   builtin_spawn /usr/bin/gdb -nw -nx -data-directory /home/pedro/gdb/build/gdb/testsuite/../data-directory -iex set height 0 -iex set width 0
>   ...
> 
> while if you're testing an installed GDB (the system GDB being the
> most usual scenario), then you should normally let it use its own
> configured directory, not the just-built GDB's data directory.
> 
> This commit improves the status quo with the following two changes:
> 
>  - if the user specifies GDB on the command line, then by default,
>    don't start GDB with the -data-directory command line option.
>    I.e., let the tested GDB use its own configured data directory.

Not sure I like that.  First, I'm normally not pointing at a fully
installed GDB, so a --data-directory is needed anyway.  I usually have
~10 different gdb development trees in use, 1 of which is an upstream
build.  Normally I use this option to point at a GDB built (but not
installed) in one of these other build trees.

Second, mostly it usually doesn't matter which data-directory is used,
but if I've changed anything in a python module then I'm definitely
going to want to pick up the data-directory from the local build tree
rather than anything else.

I get that with the feature below I can still achieve the same end
result, but (for me) the most common use case (using the local
data-directory) now requires more typing.

I guess you hit a case where using the local data-directory was a bad
thing?  I'm guessing maybe a change to something in the installed
Python code maybe?  I'm just trying to understand the motivation for
this change.

Thanks,
Andrew



> 
>  - let the user override the data directory, via a new
>    GDB_DATA_DIRECTORY global.  This replaces the existing
>    BUILD_DATA_DIRECTORY variable in testsuite/lib/gdb.exp, which
>    wasn't overridable, and was a bit misnamed for the new purpose.
> 
> So after this, the following commands I believe behave intuitively:
> 
>  # Test the non-installed GDB in some build dir:
> 
>     make check \
>       RUNTESTFLAGS="GDB=/path/to/other/build/gdb \
>                     GDB_DATA_DIRECTORY=/path/to/other/build/gdb/data-directory"
> 
>  # Test the GDB installed in some prefix:
> 
>     make check \
>       RUNTESTFLAGS="GDB=/opt/gdb/bin/gdb"
> 
>  # Test the built GDB with some alternative data directory, e.g., the
>    system GDB's data directory:
> 
>     make check \
>       RUNTESTFLAGS="GDB_DATA_DIRECTORY=/usr/share/gdb"
> 
> Change-Id: Icdc21c85219155d9564a9900961997e6624b78fb
> ---
>  gdb/testsuite/README      | 13 ++++++++++++
>  gdb/testsuite/lib/gdb.exp | 44 +++++++++++++++++++++++++++++++++------
>  2 files changed, 51 insertions(+), 6 deletions(-)
> 
> diff --git a/gdb/testsuite/README b/gdb/testsuite/README
> index c2f659a7188..3a34dcdd154 100644
> --- a/gdb/testsuite/README
> +++ b/gdb/testsuite/README
> @@ -164,6 +164,19 @@ instance
>  
>  checks both the installed GDB and GDBserver.
>  
> +GDB_DATA_DIRECTORY
> +
> +If you set GDB, then by default the testsuite assumes you are
> +exercising an installed GDB, and thus the testsuite lets GDB use its
> +configured data directory.  Otherwise, if you don't set GDB, then by
> +default the tested GDB uses the data directory found under the GDB
> +build directory.  You can override this by setting GDB_DATA_DIRECTORY.
> +For instance:
> +
> +    make check \
> +      RUNTESTFLAGS="GDB=/path/to/other/build/gdb \
> +                    GDB_DATA_DIRECTORY=/path/to/other/build/gdb/data-directory"
> +
>  INTERNAL_GDBFLAGS
>  
>  Command line options passed to all GDB invocations.
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index 08726f78563..729bded2950 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -92,8 +92,13 @@ load_lib gdb-utils.exp
>  load_lib memory.exp
>  load_lib check-test-names.exp
>  
> +# The path to the GDB binary to test.
>  global GDB
>  
> +# The data directory to use for testing.  If this is the empty string,
> +# then we let GDB use its own configured data directory.
> +global GDB_DATA_DIRECTORY
> +
>  # The spawn ID used for I/O interaction with the inferior.  For native
>  # targets, or remote targets that can do I/O through GDB
>  # (semi-hosting) this will be the same as the host/GDB's spawn ID.
> @@ -111,9 +116,24 @@ if ![info exists GDB] {
>      } else {
>  	set GDB [transform gdb]
>      }
> +} else {
> +    # If the user specifies GDB on the command line, and doesn't
> +    # specify GDB_DATA_DIRECTORY, then assume we're testing an
> +    # installed GDB, and let it use its own configured data directory.
> +    if ![info exists GDB_DATA_DIRECTORY] {
> +	set GDB_DATA_DIRECTORY ""
> +    }
>  }
>  verbose "using GDB = $GDB" 2
>  
> +# The data directory the testing GDB will use.  By default, assume
> +# we're testing a non-installed GDB in the build directory.  Users may
> +# also explictly override the -data-directory from the command line.
> +if ![info exists GDB_DATA_DIRECTORY] {
> +    set GDB_DATA_DIRECTORY "[pwd]/../data-directory"
> +}
> +verbose "using GDB_DATA_DIRECTORY = $GDB_DATA_DIRECTORY" 2
> +
>  # GDBFLAGS is available for the user to set on the command line.
>  # E.g. make check RUNTESTFLAGS=GDBFLAGS=mumble
>  # Testcases may use it to add additional flags, but they must:
> @@ -125,23 +145,35 @@ if ![info exists GDBFLAGS] {
>  }
>  verbose "using GDBFLAGS = $GDBFLAGS" 2
>  
> -# Make the build data directory available to tests.
> -set BUILD_DATA_DIRECTORY "[pwd]/../data-directory"
> +# Append the -data-directory option to pass to GDB to CMDLINE and
> +# return the resulting string.  If GDB_DATA_DIRECTORY is empty,
> +# nothing is appended.
> +proc append_gdb_data_directory_option {cmdline} {
> +    global GDB_DATA_DIRECTORY
> +
> +    if { $GDB_DATA_DIRECTORY != "" } {
> +	return "$cmdline -data-directory $GDB_DATA_DIRECTORY"
> +    } else {
> +	return $cmdline
> +    }
> +}
>  
>  # INTERNAL_GDBFLAGS contains flags that the testsuite requires.
>  # `-nw' disables any of the windowed interfaces.
>  # `-nx' disables ~/.gdbinit, so that it doesn't interfere with the tests.
> -# `-data-directory' points to the data directory in the build directory.
>  # `-iex "set {height,width} 0"' disables pagination.
> +# `-data-directory' points to the data directory, usually in the build
> +# directory.
>  global INTERNAL_GDBFLAGS
>  if ![info exists INTERNAL_GDBFLAGS] {
>      set INTERNAL_GDBFLAGS \
>  	[join [list \
>  		   "-nw" \
>  		   "-nx" \
> -		   "-data-directory $BUILD_DATA_DIRECTORY" \
>  		   {-iex "set height 0"} \
>  		   {-iex "set width 0"}]]
> +
> +    set INTERNAL_GDBFLAGS [append_gdb_data_directory_option $INTERNAL_GDBFLAGS]
>  }
>  
>  # The variable gdb_prompt is a regexp which matches the gdb prompt.
> @@ -8021,9 +8053,9 @@ proc verify_psymtab_expanded { filename readin } {
>  # string (the default) means .gdb_index; "-dwarf-5" means .debug_names.
>  
>  proc add_gdb_index { program {style ""} } {
> -    global srcdir GDB env BUILD_DATA_DIRECTORY
> +    global srcdir GDB env
>      set contrib_dir "$srcdir/../contrib"
> -    set env(GDB) "$GDB --data-directory=$BUILD_DATA_DIRECTORY"
> +    set env(GDB) [append_gdb_data_directory_option $GDB]
>      set result [catch "exec $contrib_dir/gdb-add-index.sh $style $program" output]
>      if { $result != 0 } {
>  	verbose -log "result is $result"
> 
> base-commit: 0a30596cfad9cd221a81eea984b6fe3fabb20b95
> -- 
> 2.26.2
>