public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Andrew Burgess <aburgess@redhat.com>
To: Pedro Alves <pedro@palves.net>
Cc: gdb-patches@sourceware.org
Subject: Re: [PATCH v2] gdb/testsuite: Installed-GDB testing & data-directory
Date: Fri, 18 Mar 2022 23:06:35 +0000	[thread overview]
Message-ID: <87a6dmuah0.fsf@redhat.com> (raw)
In-Reply-To: <bba276a7-cacf-b8fd-bbc1-69483c0a9302@palves.net>

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


  reply	other threads:[~2022-03-18 23:06 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-03-18 16:10 [PATCH] " 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 [this message]
2022-03-21 12:02         ` Pedro Alves

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87a6dmuah0.fsf@redhat.com \
    --to=aburgess@redhat.com \
    --cc=gdb-patches@sourceware.org \
    --cc=pedro@palves.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).