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

[Pedro Alves <pedro@palves.net>]

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.