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

[Tom de Vries <tdevries@suse.de>]

[-- 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