Re: [PATCH][gdb/testsuite] Don't abort testrun for invalid command in test-case

Tom de Vries
2020-06-12T07:47:07Z

[-- Attachment #1: Type: text/plain, Size: 2320 bytes --]

On 11-06-2020 18:56, Pedro Alves wrote:
> On 6/11/20 5:25 PM, Tom de Vries wrote:
>> On 11-06-2020 17:31, Pedro Alves wrote:
>
>>> I'd think it would be cleaner to override unknown in gdb_init,
>>> and restore in gdb_finish. No need for filename matching that way.
>>> Like below. Any reason you didn't go for this instead?
>>>
>> I don't think it's cleaner, because we run default_gdb_init, as well the
>> bit of proc runtest between ${tool}_init a ${tool}_finish with the
>> altered ::unknown.
>
> Well, that is code that is exercised by all testscases, it's part of the
> framework. It's not going to ever contain calls to procedures that
> don't exist that go unnoticed for long. The whole testsuite would
> collapse. We can move the overriding until after default_gdb_init
> is called, even, so that even less code runs under gdb's "unknown":
>
> set res [default_gdb_init $test_file_name]
>
> # Skip dejagnu_unknown while running a testcase to prevent it from
> # exiting and aborting the entire test run. Save the unknown proc
> # defined by DejaGnu, and override the unknown proc with a
> # gdb-local version.
> rename ::unknown ::dejagnu_unknown
> proc unknown { args } {
> return [uplevel 1 ::tcl_unknown $args]
> }
>
> return $res
> }
>
>> Alternatively, we could override source to detect the precise point that
>> runtest hands control to the test-case, as attached. But there's still
>> filename matching. [ Note btw, that this approach changes the scope of
>> the fix slightly. ]
>
> Overriding standard procs and filename matching strikes me as uglier than
> using the hooks that dejagnu provides (init/finish), but I can also see your
> point of only overriding unknown for the exact duration of the sourced file
> being cleaner. Thus, I'm not objecting and leave it up to you. I agree
> that it's nice that we don't abort the whole testsuite.
>

I'm convinced now, it's best to use dejagnu hooks (although my questions
related to gdb_init/default_gdb_init remain).

> BTW, isn't "tcl_unknown" an internal DejaGnu name? We should document
> that in a comment, since this could break if DejaGnu changes the renamed
> proc's name.

Done. Committed as attached.

Thanks,
- Tom

> Another reason to ask DejaGnu to handle this for us cleanly.
>

[-- Attachment #2: 0001-gdb-testsuite-Don-t-abort-testrun-for-invalid-command-in-test-case.patch --]
[-- Type: text/x-patch, Size: 2212 bytes --]

[gdb/testsuite] Don't abort testrun for invalid command in test-case

Say we add a call to foobar at the end of a test-case, and run the
test-suite. We'll run into a dejagnu error:
...
ERROR: (DejaGnu) proc "foobar" does not exist.
...
and the test-suite run is aborted.

It's reasonable that the test-case is aborted, but it's not reasonable that
the testsuite run is aborted.

Problems in one test-case should not leak into other test-cases, and they
generally don't. The exception is the "invalid command name" problem due to
an override of ::unknown in dejagnu's framework.exp.

Fix this by reverting dejagnu's ::unknown override for the duration of each
test-case, using the gdb_init/gdb_finish hooks.

Tested on x86_64-linux.

gdb/testsuite/ChangeLog:

2020-06-11 Tom de Vries <tdevries@suse.de>

PR testsuite/26110
* lib/gdb.exp (gdb_init): Revert dejagnu's override of ::unknown.
(gdb_finish): Reinstall dejagnu's override of ::unknown.

---
gdb/testsuite/lib/gdb.exp | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 51f8a05464..64e667c20e 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -5193,7 +5193,19 @@ proc gdb_init { test_file_name } {
global gdb_instances
set gdb_instances 0

- return [default_gdb_init $test_file_name]
+ set res [default_gdb_init $test_file_name]
+
+ # Dejagnu overrides proc unknown. The dejagnu version may trigger in a
+ # test-case but abort the entire test run. To fix this, we install a
+ # local version here, which reverts dejagnu's override, and restore
+ # dejagnu's version in gdb_finish.
+ rename ::unknown ::dejagnu_unknown
+ proc unknown { args } {
+ # Dejagnu saves the original version in ::tcl_unknown, use it.
+ return [uplevel 1 ::tcl_unknown $args]
+ }
+
+ return $res
}

proc gdb_finish { } {
@@ -5201,6 +5213,10 @@ proc gdb_finish { } {
global gdb_prompt
global cleanfiles

+ # Restore dejagnu's version of proc unknown.
+ rename ::unknown ""
+ rename ::dejagnu_unknown ::unknown
+
# Exit first, so that the files are no longer in use.
gdb_exit