* [PATCH] gdb/testsuite: Installed-GDB testing & data-directory
@ 2022-03-18 16:10 Pedro Alves
2022-03-18 16:23 ` Andrew Burgess
0 siblings, 1 reply; 6+ messages in thread
From: Pedro Alves @ 2022-03-18 16:10 UTC (permalink / raw)
To: gdb-patches
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.
- 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gdb/testsuite: Installed-GDB testing & data-directory
2022-03-18 16:10 [PATCH] gdb/testsuite: Installed-GDB testing & data-directory Pedro Alves
@ 2022-03-18 16:23 ` Andrew Burgess
2022-03-18 16:43 ` Pedro Alves
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Burgess @ 2022-03-18 16:23 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
* 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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gdb/testsuite: Installed-GDB testing & data-directory
2022-03-18 16:23 ` Andrew Burgess
@ 2022-03-18 16:43 ` Pedro Alves
2022-03-18 17:37 ` [PATCH v2] " Pedro Alves
0 siblings, 1 reply; 6+ messages in thread
From: Pedro Alves @ 2022-03-18 16:43 UTC (permalink / raw)
To: Andrew Burgess; +Cc: gdb-patches
Hi Andrew,
On 2022-03-18 16:23, Andrew Burgess wrote:
> * 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.
As you know, there's no guarantee that the data directory from a gdb built against
commit A works correctly with a gdb built against commit B. It should all
be treated as a single unit. I would say that we consider that scenario
unsupported/undefined-behavior. You know it works most of the time if the builds
are of similar vintage, and that's OK if you really know what you're doing, but
we naturally don't want to say that that's supported. I'm not saying I think
you're claiming that it should. I'm instead trying to build the case for
saying that it's better to make this behave correctly by default.
Do you disagree that what we suggest in testsuite/README:
make check RUNTESTFLAGS=GDB=/usr/bin/gdb
behaves incorrectly (in the sense that it does unsupportable things),
and we should instead suggest something that works correctly by default?
My first approach was to just let users override GDB_DATA_DIRECTORY, and
thus tweak the README to read:
make check RUNTESTFLAGS="GDB=/usr/bin/gdb GDB_DATA_DIRECTORY="
but then extending the README text to explain this felt pretty awkward and I
realized that we should probably default GDB_DATA_DIRECTORY to "" if
GDB was overridden, and then things would work for 99% of the users.
I admit I didn't think people were using this the way you are using it.
>
> 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.
>
Interesting.
> 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.
The motivation is that we (AMD) build packages that are meant to be installed
by users, and then we have a separate testing step that installs the packages
and validates them using the testsuite. This testing step doesn't want to build
the whole of GDB, as we want to "test what we ship" -- it just configures using
gdb/testsuite/configure directly, and then runs:
make check RUNTESTFLAGS="GDB=/path/to/installed/gdb".
The supposedly just-built data-directory that the testsuite passes to gdb with
"-data-directory [pwd]/../data-directory" doesn't even exist, which
exposes the problem.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] gdb/testsuite: Installed-GDB testing & data-directory
2022-03-18 16:43 ` Pedro Alves
@ 2022-03-18 17:37 ` Pedro Alves
2022-03-18 23:06 ` Andrew Burgess
0 siblings, 1 reply; 6+ messages in thread
From: Pedro Alves @ 2022-03-18 17:37 UTC (permalink / raw)
To: Andrew Burgess; +Cc: gdb-patches
On 2022-03-18 16:43, Pedro Alves wrote:
> Do you disagree that what we suggest in testsuite/README:
>
> make check RUNTESTFLAGS=GDB=/usr/bin/gdb
>
> behaves incorrectly (in the sense that it does unsupportable things),
> and we should instead suggest something that works correctly by default?
>
> My first approach was to just let users override GDB_DATA_DIRECTORY, and
> thus tweak the README to read:
>
> make check RUNTESTFLAGS="GDB=/usr/bin/gdb GDB_DATA_DIRECTORY="
>
> but then extending the README text to explain this felt pretty awkward and I
> realized that we should probably default GDB_DATA_DIRECTORY to "" if
> GDB was overridden, and then things would work for 99% of the users.
>
> I admit I didn't think people were using this the way you are using it.
Below's what such an approach looks like. This works for us as well,
we just have to pass the GDB_DATA_DIRECTORY= explicitly, which isn't as
nice, but isn't a big deal.
Let me know what you think.
From e701a7ea7812cf60a0a146c705f4ba27dd31ad11 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Fri, 18 Mar 2022 12:39:55 +0000
Subject: [[PATCH v2]] gdb/testsuite: Installed-GDB testing & data-directory
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 by letting 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.
Here are some use case examples:
# 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 GDB_DATA_DIRECTORY="
# 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 | 29 +++++++++++++++++++++++++----
gdb/testsuite/lib/gdb.exp | 37 +++++++++++++++++++++++++++++++------
2 files changed, 56 insertions(+), 10 deletions(-)
diff --git a/gdb/testsuite/README b/gdb/testsuite/README
index c2f659a7188..cae78fe7946 100644
--- a/gdb/testsuite/README
+++ b/gdb/testsuite/README
@@ -13,14 +13,17 @@ There are two ways to run the testsuite and pass additional parameters
to DejaGnu. The first is to do `make check' in the main build
directory and specifying the makefile variable `RUNTESTFLAGS':
- make check RUNTESTFLAGS='GDB=/usr/bin/gdb gdb.base/a2-run.exp'
+ make check \
+ RUNTESTFLAGS='GDB=/usr/bin/gdb \
+ GDB_DATA_DIRECTORY= \
+ gdb.base/a2-run.exp'
The second is to cd to the testsuite directory and invoke the DejaGnu
`runtest' command directly.
cd testsuite
make site.exp
- runtest GDB=/usr/bin/gdb
+ runtest GDB=/usr/bin/gdb GDB_DATA_DIRECTORY=
(The `site.exp' file contains a handful of useful variables like host
and target triplets, and pathnames.)
@@ -151,9 +154,10 @@ By default, the testsuite exercises the GDB in the build directory,
but you can set GDB to be a pathname to a different version. For
instance,
- make check RUNTESTFLAGS=GDB=/usr/bin/gdb
+ make check RUNTESTFLAGS="GDB=/usr/bin/gdb GDB_DATA_DIRECTORY="
-runs the testsuite on the GDB in /usr/bin.
+runs the testsuite on the GDB installed in /usr/bin. See below for
+the description of GDB_DATA_DIRECTORY.
GDBSERVER
@@ -164,6 +168,23 @@ instance
checks both the installed GDB and GDBserver.
+GDB_DATA_DIRECTORY
+
+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"
+
+If GDB_DATA_DIRECTORY is set to the empty string, then the testsuite
+lets GDB use its configured data directory. This is useful to test an
+installed GDB. For example:
+
+ make check \
+ RUNTESTFLAGS="GDB=/usr/bin/gdb 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..89011336b6d 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.
@@ -114,6 +119,14 @@ if ![info exists GDB] {
}
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 +138,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 +8046,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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] gdb/testsuite: Installed-GDB testing & data-directory
2022-03-18 17:37 ` [PATCH v2] " Pedro Alves
@ 2022-03-18 23:06 ` Andrew Burgess
2022-03-21 12:02 ` Pedro Alves
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Burgess @ 2022-03-18 23:06 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
Pedro Alves <pedro@palves.net> writes:
> On 2022-03-18 16:43, Pedro Alves wrote:
>
>> Do you disagree that what we suggest in testsuite/README:
>>
>> make check RUNTESTFLAGS=GDB=/usr/bin/gdb
>>
>> behaves incorrectly (in the sense that it does unsupportable things),
>> and we should instead suggest something that works correctly by default?
>>
>> My first approach was to just let users override GDB_DATA_DIRECTORY, and
>> thus tweak the README to read:
>>
>> make check RUNTESTFLAGS="GDB=/usr/bin/gdb GDB_DATA_DIRECTORY="
>>
>> but then extending the README text to explain this felt pretty awkward and I
>> realized that we should probably default GDB_DATA_DIRECTORY to "" if
>> GDB was overridden, and then things would work for 99% of the users.
>>
>> I admit I didn't think people were using this the way you are using it.
>
> Below's what such an approach looks like. This works for us as well,
> we just have to pass the GDB_DATA_DIRECTORY= explicitly, which isn't as
> nice, but isn't a big deal.
>
> Let me know what you think.
Sure, the default behaviour works for me just fine. But I suspect I
might be being unreasonable asking you to change this. Like you point
out, really, it's me who's relying on undefined behaviour, so, it's
probably me being unreasonable asking you to change approach.
Feel free to go with whatever approach you think is best, I'm sure I'll
adapt just fine.
Thanks,
Andrew
>
> From e701a7ea7812cf60a0a146c705f4ba27dd31ad11 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <pedro@palves.net>
> Date: Fri, 18 Mar 2022 12:39:55 +0000
> Subject: [[PATCH v2]] gdb/testsuite: Installed-GDB testing & data-directory
>
> 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 by letting 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.
>
> Here are some use case examples:
>
> # 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 GDB_DATA_DIRECTORY="
>
> # 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 | 29 +++++++++++++++++++++++++----
> gdb/testsuite/lib/gdb.exp | 37 +++++++++++++++++++++++++++++++------
> 2 files changed, 56 insertions(+), 10 deletions(-)
>
> diff --git a/gdb/testsuite/README b/gdb/testsuite/README
> index c2f659a7188..cae78fe7946 100644
> --- a/gdb/testsuite/README
> +++ b/gdb/testsuite/README
> @@ -13,14 +13,17 @@ There are two ways to run the testsuite and pass additional parameters
> to DejaGnu. The first is to do `make check' in the main build
> directory and specifying the makefile variable `RUNTESTFLAGS':
>
> - make check RUNTESTFLAGS='GDB=/usr/bin/gdb gdb.base/a2-run.exp'
> + make check \
> + RUNTESTFLAGS='GDB=/usr/bin/gdb \
> + GDB_DATA_DIRECTORY= \
> + gdb.base/a2-run.exp'
>
> The second is to cd to the testsuite directory and invoke the DejaGnu
> `runtest' command directly.
>
> cd testsuite
> make site.exp
> - runtest GDB=/usr/bin/gdb
> + runtest GDB=/usr/bin/gdb GDB_DATA_DIRECTORY=
>
> (The `site.exp' file contains a handful of useful variables like host
> and target triplets, and pathnames.)
> @@ -151,9 +154,10 @@ By default, the testsuite exercises the GDB in the build directory,
> but you can set GDB to be a pathname to a different version. For
> instance,
>
> - make check RUNTESTFLAGS=GDB=/usr/bin/gdb
> + make check RUNTESTFLAGS="GDB=/usr/bin/gdb GDB_DATA_DIRECTORY="
>
> -runs the testsuite on the GDB in /usr/bin.
> +runs the testsuite on the GDB installed in /usr/bin. See below for
> +the description of GDB_DATA_DIRECTORY.
>
> GDBSERVER
>
> @@ -164,6 +168,23 @@ instance
>
> checks both the installed GDB and GDBserver.
>
> +GDB_DATA_DIRECTORY
> +
> +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"
> +
> +If GDB_DATA_DIRECTORY is set to the empty string, then the testsuite
> +lets GDB use its configured data directory. This is useful to test an
> +installed GDB. For example:
> +
> + make check \
> + RUNTESTFLAGS="GDB=/usr/bin/gdb 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..89011336b6d 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.
> @@ -114,6 +119,14 @@ if ![info exists GDB] {
> }
> 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 +138,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 +8046,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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] gdb/testsuite: Installed-GDB testing & data-directory
2022-03-18 23:06 ` Andrew Burgess
@ 2022-03-21 12:02 ` Pedro Alves
0 siblings, 0 replies; 6+ messages in thread
From: Pedro Alves @ 2022-03-21 12:02 UTC (permalink / raw)
To: Andrew Burgess; +Cc: gdb-patches
On 2022-03-18 23:06, Andrew Burgess wrote:
> Pedro Alves <pedro@palves.net> writes:
>
>> On 2022-03-18 16:43, Pedro Alves wrote:
>>
>>> Do you disagree that what we suggest in testsuite/README:
>>>
>>> make check RUNTESTFLAGS=GDB=/usr/bin/gdb
>>>
>>> behaves incorrectly (in the sense that it does unsupportable things),
>>> and we should instead suggest something that works correctly by default?
>>>
>>> My first approach was to just let users override GDB_DATA_DIRECTORY, and
>>> thus tweak the README to read:
>>>
>>> make check RUNTESTFLAGS="GDB=/usr/bin/gdb GDB_DATA_DIRECTORY="
>>>
>>> but then extending the README text to explain this felt pretty awkward and I
>>> realized that we should probably default GDB_DATA_DIRECTORY to "" if
>>> GDB was overridden, and then things would work for 99% of the users.
>>>
>>> I admit I didn't think people were using this the way you are using it.
>>
>> Below's what such an approach looks like. This works for us as well,
>> we just have to pass the GDB_DATA_DIRECTORY= explicitly, which isn't as
>> nice, but isn't a big deal.
>>
>> Let me know what you think.
>
> Sure, the default behaviour works for me just fine. But I suspect I
> might be being unreasonable asking you to change this. Like you point
> out, really, it's me who's relying on undefined behaviour, so, it's
> probably me being unreasonable asking you to change approach.
>
> Feel free to go with whatever approach you think is best, I'm sure I'll
> adapt just fine.
Thanks Andrew, I appreciate it. I still prefer v1. With either master or v2, if you
specify GDB, but not GDB_DATA_DIRECTORY, then the data directory used is
_always_ wrong, while with v1, it is correct for installed GDBs.
I'll push v1 to master in a bit.
Pedro Alves
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-03-21 12:02 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-18 16:10 [PATCH] gdb/testsuite: Installed-GDB testing & data-directory Pedro Alves
2022-03-18 16:23 ` Andrew Burgess
2022-03-18 16:43 ` Pedro Alves
2022-03-18 17:37 ` [PATCH v2] " Pedro Alves
2022-03-18 23:06 ` Andrew Burgess
2022-03-21 12:02 ` Pedro Alves
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).