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

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

[Tom de Vries]

Hi,

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 limiting the scope of dejagnu's ::unknown override.

Tested on x86_64-linux.

Any comments?

Thanks,
- Tom

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

gdb/testsuite/ChangeLog:

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

	PR testsuite/26110
	* lib/gdb.exp (::dejagnu_unknown): Rename from ::unknown.
	(unknown): New proc.

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

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 51f8a05464..e25f2e74f7 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -7243,5 +7243,32 @@ proc with_override { name override body } {
     return $result
 }
 
+# Save the unknown proc defined by dejagnu.
+rename ::unknown ::dejagnu_unknown
+
+# Override the unknown proc with a gdb-local version.
+proc unknown { args } {
+    set script [info script]
+
+    set script [file dirname $script]
+    set subdir3 [file tail $script]
+
+    set script [file dirname $script]
+    set subdir2 [file tail $script]
+
+    set script [file dirname $script]
+    set subdir1 [file tail $script]
+
+    if { $subdir1 == "gdb"
+	 && $subdir2 == "testsuite"
+	 && [regexp {^gdb[.]} $subdir3] } {
+	# If we're executing a gdb test-case, skip dejagnu_unknown to prevent
+	# it from exiting and aborting the entire test run.
+	return [uplevel 1 ::tcl_unknown $args]
+    }
+
+    return [uplevel 1 ::dejagnu_unknown $args]
+}
+
 # Always load compatibility stuff.
 load_lib future.exp

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

[Pedro Alves]

On 6/11/20 3:35 PM, Tom de Vries wrote:
> Hi,
> 
> 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 limiting the scope of dejagnu's ::unknown override.
> 
> Tested on x86_64-linux.
> 
> Any comments?

We should ask the DejaGnu folks to make the exiting on error
optional, so that in a future version we can drop the hack.

What about, say, an error due to calling a tcl proc with the wrong
arguments?  "unknown" isn't called for that.  Any idea how to
catch those?

> +# Save the unknown proc defined by dejagnu.
> +rename ::unknown ::dejagnu_unknown
> +
> +# Override the unknown proc with a gdb-local version.
> +proc unknown { args } {
> +    set script [info script]
> +
> +    set script [file dirname $script]
> +    set subdir3 [file tail $script]
> +
> +    set script [file dirname $script]
> +    set subdir2 [file tail $script]
> +
> +    set script [file dirname $script]
> +    set subdir1 [file tail $script]
> +
> +    if { $subdir1 == "gdb"
> +	 && $subdir2 == "testsuite"
> +	 && [regexp {^gdb[.]} $subdir3] } {
> +	# If we're executing a gdb test-case, skip dejagnu_unknown to prevent
> +	# it from exiting and aborting the entire test run.
> +	return [uplevel 1 ::tcl_unknown $args]
> +    }
> +

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?

From dc4d41908280b26224dc190ff9e03a07b7357b2b Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Thu, 11 Jun 2020 16:11:32 +0100
Subject: [PATCH] unknown

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

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 9a0620a2bf1..312ff1fb366 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -5193,6 +5193,15 @@ proc gdb_init { test_file_name } {
     global gdb_instances
     set gdb_instances 0
 
+    # 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 [default_gdb_init $test_file_name]
 }
 
@@ -5201,6 +5210,10 @@ proc gdb_finish { } {
     global gdb_prompt
     global cleanfiles
 
+    # Restore DejaGnu's 'unknown' version.
+    rename ::unknown ""
+    rename ::dejagnu_unknown ::unknown
+
     # Exit first, so that the files are no longer in use.
     gdb_exit
 

-- 
2.14.5

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

[Tom de Vries]

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

On 11-06-2020 17:31, Pedro Alves wrote:
> On 6/11/20 3:35 PM, Tom de Vries wrote:
>> Hi,
>>
>> 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 limiting the scope of dejagnu's ::unknown override.
>>
>> Tested on x86_64-linux.
>>
>> Any comments?
> 
> We should ask the DejaGnu folks to make the exiting on error
> optional, so that in a future version we can drop the hack.
> 

Agreed.

> What about, say, an error due to calling a tcl proc with the wrong
> arguments?  "unknown" isn't called for that.  Any idea how to
> catch those?
> 

If I do:
...
diff --git a/gdb/testsuite/gdb.ada/O2_float_param.exp
b/gdb/testsuite/gdb.ada/O2_float_param.exp
index 09ebeec405..afbf6c5ace 100644
--- a/gdb/testsuite/gdb.ada/O2_float_param.exp
+++ b/gdb/testsuite/gdb.ada/O2_float_param.exp
@@ -29,3 +29,9 @@ runto "increment"

 gdb_test "frame" \
          "#0\\s+callee\\.increment \\(val(=val@entry)?=99\\.0,
msg=\\.\\.\\.\\).*"
+
+proc foo { a b c } {
+
+}
+
+foo
...
and run the test-suite, I get:
...
Running
/data/gdb_versions/devel/src/gdb/testsuite/gdb.ada/O2_float_param.exp ...
ERROR: tcl error sourcing
/data/gdb_versions/devel/src/gdb/testsuite/gdb.ada/O2_float_param.exp.
ERROR: wrong # args: should be "foo a b c"
    while executing
"foo"
    (file
"/data/gdb_versions/devel/src/gdb/testsuite/gdb.ada/O2_float_param.exp"
line 37)
    invoked from within
"source
/data/gdb_versions/devel/src/gdb/testsuite/gdb.ada/O2_float_param.exp"
    ("uplevel" body line 1)
    invoked from within
"uplevel #0 source
/data/gdb_versions/devel/src/gdb/testsuite/gdb.ada/O2_float_param.exp"
    invoked from within
"catch "uplevel #0 source $test_file_name""
Running
/data/gdb_versions/devel/src/gdb/testsuite/gdb.ada/access_tagged_param.exp
...
Running
/data/gdb_versions/devel/src/gdb/testsuite/gdb.ada/access_to_packed_array.exp
...
  ...
...

So, the error is caught in proc runtest:
...
        if { [catch "uplevel #0 source $test_file_name"] == 1 } {
...

>> +# Save the unknown proc defined by dejagnu.
>> +rename ::unknown ::dejagnu_unknown
>> +
>> +# Override the unknown proc with a gdb-local version.
>> +proc unknown { args } {
>> +    set script [info script]
>> +
>> +    set script [file dirname $script]
>> +    set subdir3 [file tail $script]
>> +
>> +    set script [file dirname $script]
>> +    set subdir2 [file tail $script]
>> +
>> +    set script [file dirname $script]
>> +    set subdir1 [file tail $script]
>> +
>> +    if { $subdir1 == "gdb"
>> +	 && $subdir2 == "testsuite"
>> +	 && [regexp {^gdb[.]} $subdir3] } {
>> +	# If we're executing a gdb test-case, skip dejagnu_unknown to prevent
>> +	# it from exiting and aborting the entire test run.
>> +	return [uplevel 1 ::tcl_unknown $args]
>> +    }
>> +
> 
> 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.

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. ]

Thanks,
- Tom

[-- Attachment #2: 0001-gdb-testsuite-Don-t-abort-testrun-for-invalid-command-in-test-case.patch --]
[-- Type: text/x-patch, Size: 2035 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 limiting the scope of dejagnu's ::unknown override.

Tested on x86_64-linux.

gdb/testsuite/ChangeLog:

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

	PR testsuite/26110
	* lib/gdb.exp (gdb_unknown): New proc.
	(::tcl_source): Rename from ::source.
	(source): New proc.

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

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 51f8a05464..ae6d561a10 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -7243,5 +7243,35 @@ proc with_override { name override body } {
     return $result
 }
 
+proc gdb_unknown { args } {
+    # Skip dejagnu_unknown to prevent it from exiting and aborting the entire
+    # test run.
+    return [uplevel 1 ::tcl_unknown $args]
+}
+
+rename ::source ::tcl_source
+
+proc source { arg } {
+    set script $arg
+
+    set script [file dirname $script]
+    set subdir3 [file tail $script]
+
+    set script [file dirname $script]
+    set subdir2 [file tail $script]
+
+    set script [file dirname $script]
+    set subdir1 [file tail $script]
+
+    if { $subdir1 == "gdb"
+        && $subdir2 == "testsuite"
+        && [regexp {^gdb[.]} $subdir3] } {
+       return \
+           [uplevel 1 with_override ::unknown gdb_unknown \"::tcl_source $arg\"]
+    }
+
+    return [uplevel 1 ::tcl_source $arg]
+}
+
 # Always load compatibility stuff.
 load_lib future.exp

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

[Pedro Alves]

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.

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.  Another reason to ask DejaGnu to handle this for us cleanly.

Thanks,
Pedro Alves

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

[Tom de Vries]

On 11-06-2020 18:56, Pedro Alves wrote:
>> 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
>   }
> 

Hmm, what is the distinction between gdb_init and default_gdb_init?

All the other uses in gdb.exp of pattern foo/default_foo have an
implementation:
...
proc foo {} {
  [default_foo]
}
...
but gdb_init is much more than that. Why is that different?

The code introducing default_gdb_init mentions that gdb_init can be
overridden.  If so, then the unknown override won't be active. Or should
all gdb_init overrides copy this code?

Thanks,
- Tom

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

[Tom de Vries]

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

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

[Tom de Vries]

On 11-06-2020 18:56, Pedro Alves wrote:
> Another reason to ask DejaGnu to handle this for us cleanly.

Done: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=41824 .

Thanks,
- Tom

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

[Pedro Alves]

On 6/11/20 11:55 PM, Tom de Vries wrote:

> Hmm, what is the distinction between gdb_init and default_gdb_init?
> 
> All the other uses in gdb.exp of pattern foo/default_foo have an
> implementation:
> ...
> proc foo {} {
>   [default_foo]
> }
> ...
> but gdb_init is much more than that. Why is that different?
> 

I don't know.  I guess it shouldn't.  I guess people (including me) added to
gdb_init over time without realizing they were breaking the pattern.  Maybe nobody
notices it because whatever is overriding gdb_init renames the original one and
then calls it.

Thanks,
Pedro Alves

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

[Tom de Vries]

On 6/16/20 2:47 PM, Pedro Alves wrote:
> On 6/11/20 11:55 PM, Tom de Vries wrote:
> 
>> Hmm, what is the distinction between gdb_init and default_gdb_init?
>>
>> All the other uses in gdb.exp of pattern foo/default_foo have an
>> implementation:
>> ...
>> proc foo {} {
>>   [default_foo]
>> }
>> ...
>> but gdb_init is much more than that. Why is that different?
>>
> 
> I don't know.  I guess it shouldn't.  I guess people (including me) added to
> gdb_init over time without realizing they were breaking the pattern.  Maybe nobody
> notices it because whatever is overriding gdb_init renames the original one and
> then calls it.

Hmm, thanks for the explanation.  I feel we need to improve this
situation somehow, but I'm not sure yet how.

Thanks,
- Tom

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

[Pedro Alves]

On 6/17/20 3:14 PM, Tom de Vries wrote:
> On 6/16/20 2:47 PM, Pedro Alves wrote:
>> On 6/11/20 11:55 PM, Tom de Vries wrote:
>>
>>> Hmm, what is the distinction between gdb_init and default_gdb_init?
>>>
>>> All the other uses in gdb.exp of pattern foo/default_foo have an
>>> implementation:
>>> ...
>>> proc foo {} {
>>>   [default_foo]
>>> }
>>> ...
>>> but gdb_init is much more than that. Why is that different?
>>>
>>
>> I don't know.  I guess it shouldn't.  I guess people (including me) added to
>> gdb_init over time without realizing they were breaking the pattern.  Maybe nobody
>> notices it because whatever is overriding gdb_init renames the original one and
>> then calls it.
> 
> Hmm, thanks for the explanation.  I feel we need to improve this
> situation somehow, but I'm not sure yet how.

Move stuff in gdb_init to default_gdb_init, and add a comment to these
functions to not add stuff in there?  That seems like the obvious.
I'm not sure why we have that pattern in the first place though, given
that you can rename/override functions in tcl anyhow.  This all predates me.

Thanks,
Pedro Alves

[PATCH][gdb/testsuite] Move code from gdb_init to default_gdb_init

[Tom de Vries]

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

[ was: Re: [PATCH][gdb/testsuite] Don't abort testrun for invalid
command in test-case ]
On 6/17/20 4:19 PM, Pedro Alves wrote:
> On 6/17/20 3:14 PM, Tom de Vries wrote:
>> On 6/16/20 2:47 PM, Pedro Alves wrote:
>>> On 6/11/20 11:55 PM, Tom de Vries wrote:
>>>
>>>> Hmm, what is the distinction between gdb_init and default_gdb_init?
>>>>
>>>> All the other uses in gdb.exp of pattern foo/default_foo have an
>>>> implementation:
>>>> ...
>>>> proc foo {} {
>>>>   [default_foo]
>>>> }
>>>> ...
>>>> but gdb_init is much more than that. Why is that different?
>>>>
>>>
>>> I don't know.  I guess it shouldn't.  I guess people (including me) added to
>>> gdb_init over time without realizing they were breaking the pattern.  Maybe nobody
>>> notices it because whatever is overriding gdb_init renames the original one and
>>> then calls it.
>>
>> Hmm, thanks for the explanation.  I feel we need to improve this
>> situation somehow, but I'm not sure yet how.
> 
> Move stuff in gdb_init to default_gdb_init, and add a comment to these
> functions to not add stuff in there?  That seems like the obvious.
> I'm not sure why we have that pattern in the first place though, given
> that you can rename/override functions in tcl anyhow.  This all predates me.

How about this patch then?

Thanks,
- Tom

[-- Attachment #2: 0001-gdb-testsuite-Move-code-from-gdb_init-to-default_gdb_init.patch --]
[-- Type: text/x-patch, Size: 11031 bytes --]

[gdb/testsuite] Move code from gdb_init to default_gdb_init

If a baseboard file wants to override a proc foo, but also use the original
proc, it'll have to do something like:
...
rename foo save_foo
proc foo { } {
    ...
    set res [save_foo]
    ...
    return res
}
...
This adds a new proc named save_foo, which introduces the risk of clashing with
an existing proc.

There's a pattern in the gdb testsuite procs, that facilitates this override:
...
proc default_foo { } {
  ...
}

proc foo { } {
    return [default_foo]
}
...
such that in a baseboard file we don't need the rename:
...
proc foo { } {
    ...
    set res [default_foo]
    ...
    return res
}
...

The exception to the pattern though is gdb_init, which has a default_gdb_init
counterpart, but contains much more code than just the call to
default_gdb_init.

Fix this by moving all but the call to default_gdb_init to default_gdb_init.

Tested on x86_64-linux.

gdb/testsuite/ChangeLog:

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

	* lib/gdb.exp (gdb_init): Move all but call to default_gdb_init to ...
	(default_gdb_init): ... here.

---
 gdb/testsuite/lib/gdb.exp | 246 ++++++++++++++++++++++++----------------------
 1 file changed, 127 insertions(+), 119 deletions(-)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 02867fb5bd..7b7484c031 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -4902,6 +4902,7 @@ proc gdb_continue { function } {
     return [gdb_test "continue" ".*Breakpoint $decimal, $function .*" "continue to $function"]
 }
 
+# Default implementation of gdb_init.
 proc default_gdb_init { test_file_name } {
     global gdb_wrapper_initialized
     global gdb_wrapper_target
@@ -4909,6 +4910,107 @@ proc default_gdb_init { test_file_name } {
     global cleanfiles
     global pf_prefix
     
+    # Reset the timeout value to the default.  This way, any testcase
+    # that changes the timeout value without resetting it cannot affect
+    # the timeout used in subsequent testcases.
+    global gdb_test_timeout
+    global timeout
+    set timeout $gdb_test_timeout
+
+    if { [regexp ".*gdb\.reverse\/.*" $test_file_name]
+	 && [target_info exists gdb_reverse_timeout] } {
+	set timeout [target_info gdb_reverse_timeout]
+    }
+
+    # If GDB_INOTIFY is given, check for writes to '.'.  This is a
+    # debugging tool to help confirm that the test suite is
+    # parallel-safe.  You need "inotifywait" from the
+    # inotify-tools package to use this.
+    global GDB_INOTIFY inotify_pid
+    if {[info exists GDB_INOTIFY] && ![info exists inotify_pid]} {
+	global outdir tool inotify_log_file
+
+	set exclusions {outputs temp gdb[.](log|sum) cache}
+	set exclusion_re ([join $exclusions |])
+
+	set inotify_log_file [standard_temp_file inotify.out]
+	set inotify_pid [exec inotifywait -r -m -e move,create,delete . \
+			     --exclude $exclusion_re \
+			     |& tee -a $outdir/$tool.log $inotify_log_file &]
+
+	# Wait for the watches; hopefully this is long enough.
+	sleep 2
+
+	# Clear the log so that we don't emit a warning the first time
+	# we check it.
+	set fd [open $inotify_log_file w]
+	close $fd
+    }
+
+    # Block writes to all banned variables, and invocation of all
+    # banned procedures...
+    global banned_variables
+    global banned_procedures
+    global banned_traced
+    if (!$banned_traced) {
+	foreach banned_var $banned_variables {
+            global "$banned_var"
+            trace add variable "$banned_var" write error
+	}
+	foreach banned_proc $banned_procedures {
+	    global "$banned_proc"
+	    trace add execution "$banned_proc" enter error
+	}
+	set banned_traced 1
+    }
+
+    # We set LC_ALL, LC_CTYPE, and LANG to C so that we get the same
+    # messages as expected.
+    setenv LC_ALL C
+    setenv LC_CTYPE C
+    setenv LANG C
+
+    # Don't let a .inputrc file or an existing setting of INPUTRC mess up
+    # the test results.  Even if /dev/null doesn't exist on the particular
+    # platform, the readline library will use the default setting just by
+    # failing to open the file.  OTOH, opening /dev/null successfully will
+    # also result in the default settings being used since nothing will be
+    # read from this file.
+    setenv INPUTRC "/dev/null"
+
+    # This disables style output, which would interfere with many
+    # tests.
+    setenv TERM "dumb"
+
+    # Ensure that GDBHISTFILE and GDBHISTSIZE are removed from the
+    # environment, we don't want these modifications to the history
+    # settings.
+    unset -nocomplain ::env(GDBHISTFILE)
+    unset -nocomplain ::env(GDBHISTSIZE)
+
+    # Initialize GDB's pty with a fixed size, to make sure we avoid pagination
+    # during startup.  See "man expect" for details about stty_init.
+    global stty_init
+    set stty_init "rows 25 cols 80"
+
+    # Some tests (for example gdb.base/maint.exp) shell out from gdb to use
+    # grep.  Clear GREP_OPTIONS to make the behavior predictable,
+    # especially having color output turned on can cause tests to fail.
+    setenv GREP_OPTIONS ""
+
+    # Clear $gdbserver_reconnect_p.
+    global gdbserver_reconnect_p
+    set gdbserver_reconnect_p 1
+    unset gdbserver_reconnect_p
+
+    # Clear $last_loaded_file
+    global last_loaded_file
+    unset -nocomplain last_loaded_file
+
+    # Reset GDB number of instances
+    global gdb_instances
+    set gdb_instances 0
+
     set cleanfiles {}
 
     gdb_clear_suppressed
@@ -4942,6 +5044,20 @@ proc default_gdb_init { test_file_name } {
     if [info exists use_gdb_stub] {
 	unset use_gdb_stub
     }
+
+    gdb_setup_known_globals
+
+    if { [info procs ::gdb_tcl_unknown] != "" } {
+	# 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 } {
+	    # Use tcl's unknown.
+	    return [uplevel 1 ::gdb_tcl_unknown $args]
+	}
+    }
 }
 
 # Return a path using GDB_PARALLEL.
@@ -5188,127 +5304,19 @@ if { [interp eval $temp "info procs ::unknown"] != "" } {
 interp delete $temp
 unset temp
 
-proc gdb_init { test_file_name } {
-    # Reset the timeout value to the default.  This way, any testcase
-    # that changes the timeout value without resetting it cannot affect
-    # the timeout used in subsequent testcases.
-    global gdb_test_timeout
-    global timeout
-    set timeout $gdb_test_timeout
-
-    if { [regexp ".*gdb\.reverse\/.*" $test_file_name]
-	 && [target_info exists gdb_reverse_timeout] } {
-	set timeout [target_info gdb_reverse_timeout]
-    }
-
-    # If GDB_INOTIFY is given, check for writes to '.'.  This is a
-    # debugging tool to help confirm that the test suite is
-    # parallel-safe.  You need "inotifywait" from the
-    # inotify-tools package to use this.
-    global GDB_INOTIFY inotify_pid
-    if {[info exists GDB_INOTIFY] && ![info exists inotify_pid]} {
-	global outdir tool inotify_log_file
-
-	set exclusions {outputs temp gdb[.](log|sum) cache}
-	set exclusion_re ([join $exclusions |])
-
-	set inotify_log_file [standard_temp_file inotify.out]
-	set inotify_pid [exec inotifywait -r -m -e move,create,delete . \
-			     --exclude $exclusion_re \
-			     |& tee -a $outdir/$tool.log $inotify_log_file &]
-
-	# Wait for the watches; hopefully this is long enough.
-	sleep 2
-
-	# Clear the log so that we don't emit a warning the first time
-	# we check it.
-	set fd [open $inotify_log_file w]
-	close $fd
-    }
-
-    # Block writes to all banned variables, and invocation of all
-    # banned procedures...
-    global banned_variables
-    global banned_procedures
-    global banned_traced
-    if (!$banned_traced) {
-    	foreach banned_var $banned_variables {
-            global "$banned_var"
-            trace add variable "$banned_var" write error
-	}
-	foreach banned_proc $banned_procedures {
-	    global "$banned_proc"
-	    trace add execution "$banned_proc" enter error
-	}
-	set banned_traced 1
-    }
-
-    # We set LC_ALL, LC_CTYPE, and LANG to C so that we get the same
-    # messages as expected.
-    setenv LC_ALL C
-    setenv LC_CTYPE C
-    setenv LANG C
-
-    # Don't let a .inputrc file or an existing setting of INPUTRC mess up
-    # the test results.  Even if /dev/null doesn't exist on the particular
-    # platform, the readline library will use the default setting just by
-    # failing to open the file.  OTOH, opening /dev/null successfully will
-    # also result in the default settings being used since nothing will be
-    # read from this file.
-    setenv INPUTRC "/dev/null"
-
-    # This disables style output, which would interfere with many
-    # tests.
-    setenv TERM "dumb"
-
-    # Ensure that GDBHISTFILE and GDBHISTSIZE are removed from the
-    # environment, we don't want these modifications to the history
-    # settings.
-    unset -nocomplain ::env(GDBHISTFILE)
-    unset -nocomplain ::env(GDBHISTSIZE)
-
-    # Initialize GDB's pty with a fixed size, to make sure we avoid pagination
-    # during startup.  See "man expect" for details about stty_init.
-    global stty_init
-    set stty_init "rows 25 cols 80"
-
-    # Some tests (for example gdb.base/maint.exp) shell out from gdb to use
-    # grep.  Clear GREP_OPTIONS to make the behavior predictable,
-    # especially having color output turned on can cause tests to fail.
-    setenv GREP_OPTIONS ""
-
-    # Clear $gdbserver_reconnect_p.
-    global gdbserver_reconnect_p
-    set gdbserver_reconnect_p 1
-    unset gdbserver_reconnect_p
-
-    # Clear $last_loaded_file
-    global last_loaded_file
-    unset -nocomplain last_loaded_file
-
-    # Reset GDB number of instances
-    global gdb_instances
-    set gdb_instances 0
-
-    set res [default_gdb_init $test_file_name]
-
-    gdb_setup_known_globals
-
-    if { [info procs ::gdb_tcl_unknown] != "" } {
-	# 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 } {
-	    # Use tcl's unknown.
-	    return [uplevel 1 ::gdb_tcl_unknown $args]
-	}
-    }
-
-    return $res
+# Gdb implementation of ${tool}_init.  Called right before executing the
+# test-case.
+# Overridable function -- you can override this function in your
+# baseboard file.
+proc gdb_init { args } {
+    # A baseboard file overriding this proc and calling the default version
+    # should behave the same as this proc.  So, don't add code here, but to
+    # the default version instead.
+    return [default_gdb_init {*}$args]
 }
 
+# Gdb implementation of ${tool}_finish.  Called right after executing the
+# test-case.
 proc gdb_finish { } {
     global gdbserver_reconnect_p
     global gdb_prompt

Re: [PATCH][gdb/testsuite] Move code from gdb_init to default_gdb_init

[Pedro Alves]

On 6/18/20 11:16 AM, Tom de Vries wrote:
> [ was: Re: [PATCH][gdb/testsuite] Don't abort testrun for invalid
> command in test-case ]
> On 6/17/20 4:19 PM, Pedro Alves wrote:
>> On 6/17/20 3:14 PM, Tom de Vries wrote:
>>> On 6/16/20 2:47 PM, Pedro Alves wrote:
>>>> On 6/11/20 11:55 PM, Tom de Vries wrote:
>>>>
>>>>> Hmm, what is the distinction between gdb_init and default_gdb_init?
>>>>>
>>>>> All the other uses in gdb.exp of pattern foo/default_foo have an
>>>>> implementation:
>>>>> ...
>>>>> proc foo {} {
>>>>>   [default_foo]
>>>>> }
>>>>> ...
>>>>> but gdb_init is much more than that. Why is that different?
>>>>>
>>>>
>>>> I don't know.  I guess it shouldn't.  I guess people (including me) added to
>>>> gdb_init over time without realizing they were breaking the pattern.  Maybe nobody
>>>> notices it because whatever is overriding gdb_init renames the original one and
>>>> then calls it.
>>>
>>> Hmm, thanks for the explanation.  I feel we need to improve this
>>> situation somehow, but I'm not sure yet how.
>>
>> Move stuff in gdb_init to default_gdb_init, and add a comment to these
>> functions to not add stuff in there?  That seems like the obvious.
>> I'm not sure why we have that pattern in the first place though, given
>> that you can rename/override functions in tcl anyhow.  This all predates me.
> 
> How about this patch then?

LGTM.

(Comments should spell "GDB" instead of "Gdb" though.)

Thanks,
Pedro Alves