gdb/testsuite: better handle failures in simavr board, reap simavr process
authorSimon Marchi <simon.marchi@efficios.com>
Mon, 29 Jun 2020 14:19:43 +0000 (10:19 -0400)
committerSimon Marchi <simon.marchi@efficios.com>
Mon, 29 Jun 2020 14:19:43 +0000 (10:19 -0400)
commitdf5b8876083ec8c7bfb44ecb91b516c864edebfd
tree4ba39db53e67407c1fe9f0a84e332aa50fe69856
parent8c190ce03837391885bbba9055bea04d36077f9c
gdb/testsuite: better handle failures in simavr board, reap simavr process

This patch makes a few adjustments to the simavr.exp to handle tests
that error out more gracefully.  I put all the changes in the same patch
because right now it's in a very bad shape, so it's very painful to do
small incremental adjustements.  I found that with these changes, it
becomes reasonably stable.

For example, the gdb.base/step-over-syscall.exp test is a bit buggy
(stuff for another patch...), in that it calls gdb_load (through
clean_restart) with a file that doesn't exist.  The gdb_load
implementation in simavr.exp gets called with a file that doesn't exist,
and simavr (expectedly) fails to start.

When this happens, we currently leave the $simavr_spawn_id variable set.
However, because the simavr process has terminated, its spawn id is
closed.  When the next test tries to close the previous connection to
simavr, it fails to, and this error is thrown:

    ERROR: close: spawn id exp86 not open
        while executing
    "close -i $simavr_spawn_id"
        (procedure "gdb_load" line 18)
        invoked from within

In other words, any test ran after step-over-syscall.exp will have
simavr.exp's gdb_load fail on it.

This patch tries to make sure that simavr.exp's gdb_load only leaves
simavr_spawn_id set if everything went fine.  If we couldn't start
simavr, don't see the expected output, or fail to connect to it, we
close the spawn id (if needed) and unset simavr_spawn_id.

As an additional precaution, I added a catch around the "close the
previous connection" code.  Ideally, this shouldn't be necessary, but I
bet there are other similar scenarios where we might try to close an
already close spawn id.  So I think displaying a warning is better than
messing up all following tests.

Also, the board never wait'ed for the simavr process, resulting in tons
of zombie simavr processes hanging around.  This patch adds some calls
to "wait" whenever we close the connection (or realize it is already
closed), which seems to fix the problem.

Finally I switched a `gdb_expect` to bare `expect`, where we wait for
the "listening" message from simavr.  I found it necessary because
gdb_expect (through remote_expect) adds a `-i <gdb spawn id> -timeout
10`.  So if for some reason the GDB process has crashed in the mean
time, this expect will unexpectedly error out with a `spawn id <gdb
spawn id> not open`.  Therefore, change `gdb_expect` to `expect` so that
we only deal with simavr's spawn id here.

Here are the results on TESTS="gdb.base/*.exp" before:

    # of expected passes 4816
    # of unexpected failures 4433
    # of known failures 2
    # of unresolved testcases 300
    # of untested testcases 143
    # of unsupported tests 7
    # of paths in test names 2
    # of duplicate test names 10

and after:

    # of expected passes 21957
    # of unexpected failures 1564
    # of expected failures 8
    # of unknown successes 1
    # of known failures 31
    # of unresolved testcases 532
    # of untested testcases 153
    # of unsupported tests 28
    # of paths in test names 2
    # of duplicate test names 32

I tested using simavr's current master (7c254e2081b5).

gdb/testsuite/ChangeLog:

* boards/simavr.exp (gdb_load): Catch errors when closing
previous connection.  Close connection, wait for process and
unset simavr_spawn_id on failure.

Change-Id: I695fc765e1c22e1d1dc0b08e0f5141244986b868
gdb/testsuite/ChangeLog
gdb/testsuite/boards/simavr.exp