Skip to content
Commit 0726729d authored by Andrew Burgess's avatar Andrew Burgess
Browse files

gdb/testsuite: track if a caching proc calls gdb_exit or not

After a recent patch review I asked myself why can_spawn_for_attach
exists.  This proc currently does some checks, and then calls
can_spawn_for_attach_1 which is an actual caching proc.

The answer is that can_spawn_for_attach exists in order to call
gdb_exit the first time can_spawn_for_attach is called within any test
script.

The reason this is useful is that can_spawn_for_attach_1 calls
gdb_exit.  If the user calls can_spawn_for_attach_1 directly then a
problem might exist.  Imagine a test written like this:

  gdb_start

  if { [can_spawn_for_attach_1] } {
    ... do stuff that assumes GDB is running ...
  }

If this test is NOT the first test run, and if an earlier test calls
can_spawn_for_attach_1, then when the above test is run the
can_spawn_for_attach_1 call will return the cached value and gdb_exit
will not be called.

But, if the above test IS the first test run then
can_spawn_for_attach_1 will not return the cached value, but will
instead compute the cached value, a process that ends up calling
gdb_exit.  When can_spawn_for_attach_1 returns GDB will have exited
and the test might fail if it is written assuming that GDB is
running.

So can_spawn_for_attach was added which ensures that we _always_ call
gdb_exit the first time can_spawn_for_attach is called within a single
test script, this ensures that in the above case, even if the above is
not the first test script run, gdb_exit will still be called.  This
ensures consistent behaviour and avoids some hidden bugs in the
testsuite.

The split between can_spawn_for_attach and can_spawn_for_attach_1 was
introduced in this commit:

  commit 147fe7f9
  Date:   Mon May 6 14:27:09 2024 +0200

      [gdb/testsuite] Handle ptrace operation not permitted in can_spawn_for_attach

However, I observe that can_spawn_for_attach is not the only caching
proc that calls gdb_exit.  Why does can_spawn_for_attach get special
treatment when surely the same issue exists for any other caching proc
that calls gdb_exit?

I think a better solution is to move the logic from
can_spawn_for_attach into cache.exp and generalise it so that it
applies to all caching procs.

This commit does this by:

 1. When the underlying caching proc is executed we track calls to
    gdb_exit.  If a caching proc calls gdb_exit then this information
    is stored in gdb_data_cache (using a ',exit' suffix), and also
    written to the cache file if appropriate.

 2. When a cached value is returned from gdb_do_cache, if the
    underlying proc would have called gdb_exit, and if this is the
    first use of the caching proc in this test script, then we call
    gdb_exit.

When storing the ',exit' value into the on-disk cache file, the flag
value is stored on a second line.  Currently every cached value only
occupies a single line, and a check is added to ensure this remains
true in the future.

To track calls to gdb_exit I eventually settled on using TCL's trace
mechanism.  We already make use of this in lib/gdb.exp so I figure
this is OK to use.  This should be fine, so long as non of the caching
procs use 'with_override' to replace gdb_exit, or do any other proc
replacement to change gdb_exit, however, I think that is pretty
unlikely.

One issue did come up in testing, a FAIL in gdb.base/break-interp.exp,
prior to this commit can_spawn_for_attach would call gdb_exit before
calling the underlying caching proc.  After this call we call gdb_exit
after calling the caching proc.

The underlying caching proc relies on gdb_exit having been called.  To
resolve this issue I just added a call to gdb_exit into
can_spawn_for_attach.

With this done can_spawn_for_attach_1 can be renamed to
can_spawn_for_attach, and the existing can_spawn_for_attach can be
deleted.
parent 8db172ae
Loading
Loading
Loading
Loading
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Please register or to comment