[PATCH 1/3][gdb/testsuite] Fix global array cp_class_table_history leak

[PATCH 1/3][gdb/testsuite] Fix global array cp_class_table_history leak

[Tom de Vries]

Hi,

Test-cases using proc cp_test_ptype_class set a global array
cp_class_table_history, which remains set after the test-case has run.  This
has the potential to:
- cause tcl errors, as well as
- reuse results from one test-case in another test-case when that is not
  appropriate

Fix this by:
- moving the variable into a namespace, and
- only using the variable in the test-cases that need it, and
- resetting the variable at the end of those test-cases.

Any comments?

Thanks,
- Tom

[gdb/testsuite] Fix global array cp_class_table_history leak

gdb/testsuite/ChangeLog:

2020-05-19  Tom de Vries  <tdevries@suse.de>

	PR testsuite/25996
	* lib/cp-support.exp (init_cp_test_ptype_class_cache)
	(finish_cp_test_ptype_class_cache): New proc.
	(cp_test_ptype_class): Only use use_cp_class_table_history if
	init_cp_test_ptype_class_cache was called.
	* gdb.cp/inherit.exp: Call init_cp_test_ptype_class_cache and
	finish_cp_test_ptype_class_cache.
	* gdb.cp/virtfunc.exp: Same.

---
 gdb/testsuite/gdb.cp/inherit.exp  |  6 +++++
 gdb/testsuite/gdb.cp/virtfunc.exp |  6 +++++
 gdb/testsuite/lib/cp-support.exp  | 53 +++++++++++++++++++++++++++++++--------
 3 files changed, 55 insertions(+), 10 deletions(-)

diff --git a/gdb/testsuite/gdb.cp/inherit.exp b/gdb/testsuite/gdb.cp/inherit.exp
index 9616015709..29d4880999 100644
--- a/gdb/testsuite/gdb.cp/inherit.exp
+++ b/gdb/testsuite/gdb.cp/inherit.exp
@@ -711,4 +711,10 @@ proc do_tests { } {
     test_print_mvi_classes
 }
 
+# This test-case uses the cp_test_ptype_class cache (by means of the ibid
+# argument), so initialize and finalize the cache.
+init_cp_test_ptype_class_cache
+
 do_tests
+
+finish_cp_test_ptype_class_cache
diff --git a/gdb/testsuite/gdb.cp/virtfunc.exp b/gdb/testsuite/gdb.cp/virtfunc.exp
index 2b046c5759..214ab1ec22 100644
--- a/gdb/testsuite/gdb.cp/virtfunc.exp
+++ b/gdb/testsuite/gdb.cp/virtfunc.exp
@@ -289,4 +289,10 @@ proc do_tests {} {
     gdb_test "step" ".*E::vg.*" "step through thunk into E::vg"
 }
 
+# This test-case uses the cp_test_ptype_class cache (by means of the ibid
+# argument), so initialize and finalize the cache.
+init_cp_test_ptype_class_cache
+
 do_tests
+
+finish_cp_test_ptype_class_cache
diff --git a/gdb/testsuite/lib/cp-support.exp b/gdb/testsuite/lib/cp-support.exp
index b4a5582742..4142b00d8e 100644
--- a/gdb/testsuite/lib/cp-support.exp
+++ b/gdb/testsuite/lib/cp-support.exp
@@ -255,17 +255,22 @@ proc cp_test_ptype_class { in_exp in_testname in_key in_tag in_class_table
 	set in_command "ptype${in_ptype_arg} $in_exp"
     }
 
-    # Save class tables in a history array for reuse.
-
-    global cp_class_table_history
-    if { $in_class_table == "ibid" } then {
-	if { ! [info exists cp_class_table_history("$in_key,$in_tag") ] } then {
-	    fail "$in_testname // bad ibid"
-	    return false
+    namespace upvar ::cp_support_internal:: use_cp_class_table_history \
+	use_cp_class_table_history
+    if { [info exists use_cp_class_table_history ] } {
+	# Save class tables in a history array for reuse.
+
+	namespace upvar ::cp_support_internal:: cp_class_table_history \
+	    cp_class_table_history
+	if { $in_class_table == "ibid" } then {
+	    if { ! [info exists cp_class_table_history("$in_key,$in_tag") ] } then {
+		fail "$in_testname // bad ibid"
+		return false
+	    }
+	    set in_class_table $cp_class_table_history("$in_key,$in_tag")
+	} else {
+	    set cp_class_table_history("$in_key,$in_tag") $in_class_table
 	}
-	set in_class_table $cp_class_table_history("$in_key,$in_tag")
-    } else {
-	set cp_class_table_history("$in_key,$in_tag") $in_class_table
     }
 
     # Split the class table into separate tables.
@@ -764,3 +769,31 @@ proc cp_test_ptype_class { in_exp in_testname in_key in_tag in_class_table
 
     return true
 }
+
+# Initialize the cp_test_ptype_class cache.
+
+proc init_cp_test_ptype_class_cache { } {
+    namespace upvar ::cp_support_internal:: use_cp_class_table_history \
+	use_cp_class_table_history
+    set use_cp_class_table_history 1
+
+    namespace upvar ::cp_support_internal:: cp_class_table_history \
+	cp_class_table_history
+    if { [info exists cp_class_table_history] } {
+	unset cp_class_table_history
+    }
+}
+
+# Finalize the cp_test_ptype_class cache.
+
+proc finish_cp_test_ptype_class_cache { } {
+    namespace upvar ::cp_support_internal:: use_cp_class_table_history \
+	use_cp_class_table_history
+    unset use_cp_class_table_history
+
+    namespace upvar ::cp_support_internal:: cp_class_table_history \
+	cp_class_table_history
+    if { [info exists cp_class_table_history] } {
+	unset cp_class_table_history
+    }
+}

Re: [PATCH 1/3][gdb/testsuite] Fix global array cp_class_table_history leak

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> +# This test-case uses the cp_test_ptype_class cache (by means of the ibid
Tom> +# argument), so initialize and finalize the cache.
Tom> +init_cp_test_ptype_class_cache
Tom> +
Tom>  do_tests
Tom> +
Tom> +finish_cp_test_ptype_class_cache

If the users always have to do this init/test/finish thing, then maybe
some kind of wrapper proc would be more appropriate.

Also, it's more robust to use 'catch' to ensure that the cleanup is
always run.

However, is the finish bit needed?  If the init proc cleans up all the
state, then it seems unnecessary.

Tom

Re: [PATCH 1/3][gdb/testsuite] Fix global array cp_class_table_history leak

[Tom de Vries]

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

On 22-05-2020 22:12, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> +# This test-case uses the cp_test_ptype_class cache (by means of the ibid
> Tom> +# argument), so initialize and finalize the cache.
> Tom> +init_cp_test_ptype_class_cache
> Tom> +
> Tom>  do_tests
> Tom> +
> Tom> +finish_cp_test_ptype_class_cache
> 
> If the users always have to do this init/test/finish thing, then maybe
> some kind of wrapper proc would be more appropriate.
> 

Done.

> Also, it's more robust to use 'catch' to ensure that the cleanup is
> always run.
> 

Done.

> However, is the finish bit needed?  If the init proc cleans up all the
> state, then it seems unnecessary.

Agreed, strictly speaking it's unnecessary, it's just a cleanup.

Thanks,
- Tom



[-- Attachment #2: 0001-gdb-testsuite-Fix-global-array-cp_class_table_history-leak.patch --]
[-- Type: text/x-patch, Size: 4943 bytes --]

[gdb/testsuite] Fix global array cp_class_table_history leak

Test-cases using proc cp_test_ptype_class set a global array
cp_class_table_history, which remains set after the test-case has run.  This
has the potential to:
- cause tcl errors, as well as
- reuse results from one test-case in another test-case when that is not
  appropriate

Fix this by:
- moving the variable into a namespace, and
- only using the variable in the test-cases that need it, and
- resetting the variable at the end of those test-cases.

gdb/testsuite/ChangeLog:

2020-05-19  Tom de Vries  <tdevries@suse.de>

	PR testsuite/25996
	* lib/cp-support.exp (init_cp_test_ptype_class_cache)
	(finish_cp_test_ptype_class_cache, use_cp_test_ptype_class_cache):
	New proc.
	(cp_test_ptype_class): Only use use_cp_class_table_history if
	init_cp_test_ptype_class_cache was called.
	* gdb.cp/inherit.exp: Call use_cp_test_ptype_class_cache.
	* gdb.cp/virtfunc.exp: Same.

---
 gdb/testsuite/gdb.cp/inherit.exp  |  6 +++-
 gdb/testsuite/gdb.cp/virtfunc.exp |  6 +++-
 gdb/testsuite/lib/cp-support.exp  | 68 +++++++++++++++++++++++++++++++++------
 3 files changed, 68 insertions(+), 12 deletions(-)

diff --git a/gdb/testsuite/gdb.cp/inherit.exp b/gdb/testsuite/gdb.cp/inherit.exp
index 2d4635c96a..67a11533dd 100644
--- a/gdb/testsuite/gdb.cp/inherit.exp
+++ b/gdb/testsuite/gdb.cp/inherit.exp
@@ -719,4 +719,8 @@ proc do_tests { } {
     test_print_mvi_classes
 }
 
-do_tests
+# This test-case uses the cp_test_ptype_class cache (by means of the ibid
+# argument), so mark it as such.
+use_cp_test_ptype_class_cache {
+    do_tests
+}
diff --git a/gdb/testsuite/gdb.cp/virtfunc.exp b/gdb/testsuite/gdb.cp/virtfunc.exp
index 2b046c5759..dc83091692 100644
--- a/gdb/testsuite/gdb.cp/virtfunc.exp
+++ b/gdb/testsuite/gdb.cp/virtfunc.exp
@@ -289,4 +289,8 @@ proc do_tests {} {
     gdb_test "step" ".*E::vg.*" "step through thunk into E::vg"
 }
 
-do_tests
+# This test-case uses the cp_test_ptype_class cache (by means of the ibid
+# argument), so mark it as such.
+use_cp_test_ptype_class_cache {
+    do_tests
+}
diff --git a/gdb/testsuite/lib/cp-support.exp b/gdb/testsuite/lib/cp-support.exp
index b4a5582742..ba97fbbd87 100644
--- a/gdb/testsuite/lib/cp-support.exp
+++ b/gdb/testsuite/lib/cp-support.exp
@@ -255,17 +255,22 @@ proc cp_test_ptype_class { in_exp in_testname in_key in_tag in_class_table
 	set in_command "ptype${in_ptype_arg} $in_exp"
     }
 
-    # Save class tables in a history array for reuse.
-
-    global cp_class_table_history
-    if { $in_class_table == "ibid" } then {
-	if { ! [info exists cp_class_table_history("$in_key,$in_tag") ] } then {
-	    fail "$in_testname // bad ibid"
-	    return false
+    namespace upvar ::cp_support_internal:: use_cp_class_table_history \
+	use_cp_class_table_history
+    if { [info exists use_cp_class_table_history ] } {
+	# Save class tables in a history array for reuse.
+
+	namespace upvar ::cp_support_internal:: cp_class_table_history \
+	    cp_class_table_history
+	if { $in_class_table == "ibid" } then {
+	    if { ! [info exists cp_class_table_history("$in_key,$in_tag") ] } then {
+		fail "$in_testname // bad ibid"
+		return false
+	    }
+	    set in_class_table $cp_class_table_history("$in_key,$in_tag")
+	} else {
+	    set cp_class_table_history("$in_key,$in_tag") $in_class_table
 	}
-	set in_class_table $cp_class_table_history("$in_key,$in_tag")
-    } else {
-	set cp_class_table_history("$in_key,$in_tag") $in_class_table
     }
 
     # Split the class table into separate tables.
@@ -764,3 +769,46 @@ proc cp_test_ptype_class { in_exp in_testname in_key in_tag in_class_table
 
     return true
 }
+
+# Initialize the cp_test_ptype_class cache.
+
+proc init_cp_test_ptype_class_cache { } {
+    namespace upvar ::cp_support_internal:: use_cp_class_table_history \
+	use_cp_class_table_history
+    set use_cp_class_table_history 1
+
+    namespace upvar ::cp_support_internal:: cp_class_table_history \
+	cp_class_table_history
+    if { [info exists cp_class_table_history] } {
+	unset cp_class_table_history
+    }
+}
+
+# Finalize the cp_test_ptype_class cache.
+
+proc finish_cp_test_ptype_class_cache { } {
+    namespace upvar ::cp_support_internal:: use_cp_class_table_history \
+	use_cp_class_table_history
+    unset use_cp_class_table_history
+
+    namespace upvar ::cp_support_internal:: cp_class_table_history \
+	cp_class_table_history
+    if { [info exists cp_class_table_history] } {
+	unset cp_class_table_history
+    }
+}
+
+# Run WRAPPED_BODY using the cp_test_ptype_class_cache.
+
+proc use_cp_test_ptype_class_cache { wrapped_body } {
+    init_cp_test_ptype_class_cache
+    set code [catch {uplevel 1 $wrapped_body} result]
+    finish_cp_test_ptype_class_cache
+
+    if {$code == 1} {
+	global errorInfo errorCode
+	return -code error -errorinfo $errorInfo -errorcode $errorCode $result
+    } elseif {$code > 1} {
+	return -code $code $result
+    }
+}