[PATCH] Test the interaction between GDBHISTSIZE and .gdbinit

[PATCH] Test the interaction between GDBHISTSIZE and .gdbinit

[Patrick Palka]

The value inside the GDBHISTSIZE environment variable, only if valid,
should override setting the history size through one's .gdbinit file.

gdb/testsuite/ChangeLog:

	* gdb.base/gdbinit-history.exp: Test the interaction between
	setting GDBHISTSIZE and setting the history size via .gdbinit.
---
 gdb/testsuite/gdb.base/gdbinit-history.exp | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/gdb/testsuite/gdb.base/gdbinit-history.exp b/gdb/testsuite/gdb.base/gdbinit-history.exp
index 7bdce5f..85177f3 100644
--- a/gdb/testsuite/gdb.base/gdbinit-history.exp
+++ b/gdb/testsuite/gdb.base/gdbinit-history.exp
@@ -19,9 +19,10 @@
 
 
 # Check that the history size is properly set to SIZE when reading the .gdbinit
-# file located in HOME.
+# file located in HOME with the environment variable GDBHISTSIZE optionally
+# set to GDBHISTSIZE_VAL.
 
-proc test_gdbinit_history_setting { home size } {
+proc test_gdbinit_history_setting { home size { gdbhistsize_val "-" } } {
     global env
     global INTERNAL_GDBFLAGS
     global srcdir
@@ -36,10 +37,19 @@ proc test_gdbinit_history_setting { home size } {
     # set.
     unset -nocomplain env(GDBHISTSIZE)
 
+    if { $gdbhistsize_val != "-" } {
+	set env(GDBHISTSIZE) $gdbhistsize_val
+    }
+
     set saved_internal_gdbflags $INTERNAL_GDBFLAGS
     set INTERNAL_GDBFLAGS [string map {"-nx" ""} $INTERNAL_GDBFLAGS]
 
-    with_test_prefix "home=$home" {
+    set prefix "home=$home"
+    if { $gdbhistsize_val != "-" } {
+	append prefix " gdbhistsize=$gdbhistsize_val"
+    }
+
+    with_test_prefix $prefix {
 	gdb_exit
 	gdb_start
 
@@ -54,6 +64,7 @@ proc test_gdbinit_history_setting { home size } {
 
     set INTERNAL_GDBFLAGS $saved_internal_gdbflags
 
+    unset -nocomplain env(GDBHISTSIZE)
     array set env [array get old_env]
 }
 
@@ -117,3 +128,9 @@ test_gdbinit_history_setting "gdbinit-history/unlimited" "unlimited"
 test_gdbinit_history_setting "gdbinit-history/zero" "0"
 
 test_no_truncation_of_unlimited_history_file
+
+# A valid GDBHISTSIZE value overrides the setting inside the .gdbinit file; an
+# invalid GDBHISTSIZE value is ignored, falling back on the setting inside the
+# .gdbinit file.
+test_gdbinit_history_setting "gdbinit-history/unlimited" "1000" "1000"
+test_gdbinit_history_setting "gdbinit-history/unlimited" "unlimited" "foo"
-- 
2.4.4.410.g43ed522.dirty

Re: [PATCH] Test the interaction between GDBHISTSIZE and .gdbinit

[Pedro Alves]

On 06/17/2015 09:17 PM, Patrick Palka wrote:
> The value inside the GDBHISTSIZE environment variable, only if valid,
> should override setting the history size through one's .gdbinit file.

Thanks, looks good.

> +    unset -nocomplain env(GDBHISTSIZE)
>      array set env [array get old_env]

Though this unset looks unnecessary, given that the following line
restores the whole array.

Thanks,
Pedro Alves

Re: [PATCH] Test the interaction between GDBHISTSIZE and .gdbinit

[Patrick Palka]

On Thu, Jun 18, 2015 at 5:06 AM, Pedro Alves <palves@redhat.com> wrote:
> On 06/17/2015 09:17 PM, Patrick Palka wrote:
>> The value inside the GDBHISTSIZE environment variable, only if valid,
>> should override setting the history size through one's .gdbinit file.
>
> Thanks, looks good.
>
>> +    unset -nocomplain env(GDBHISTSIZE)
>>      array set env [array get old_env]
>
> Though this unset looks unnecessary, given that the following line
> restores the whole array.

It turns out that

    array set env [array get old_env]

does not completely restore the env array to its original state.  What
it seems to do is to reset each pre-existing environment variable
(existing in the saved env array) to its original value.  New
environment variables that were set inside the env array in the
meantime do not get unset after restoring.  So e.g. after doing

    array set old_env [array get env]
    set env(SOME_NEW_VAR) foo
    array set env [array get old_env]

the environment variable SOME_NEW_VAR=foo will still be in the env
array.  So this "array set env" trick is insufficient.  That is why
the unset of GDBHISTSIZE is necessary there.

To make the pattern of "temporarily altering global variables,
restoring their original value afterwards" more convenient and less
error-prone, I've been thinking about introducing a new tcl proc that
acts as a wrapper for saving/restoring a specified list of variables.
Its use would look something like:

    save_vars { INTERNAL_GDBFLAGS env(GDBHISTSIZE) env(HOME) } {
        append INTERNAL_GDBFLAGS " -nx"
        unset -nocomplain env(GDBHISTSIZE)
        unset -nocomplain env(HOME)
        gdb_test ....
        more_gdb_test ...
    }

which guarantees that after the body has finished executing, the given
list of variables will have their contents restored to their original
values.  What do you think about this?

Re: [PATCH] Test the interaction between GDBHISTSIZE and .gdbinit

[Patrick Palka]

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

On Thu, Jun 18, 2015 at 8:44 AM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> On Thu, Jun 18, 2015 at 5:06 AM, Pedro Alves <palves@redhat.com> wrote:
>> On 06/17/2015 09:17 PM, Patrick Palka wrote:
>>> The value inside the GDBHISTSIZE environment variable, only if valid,
>>> should override setting the history size through one's .gdbinit file.
>>
>> Thanks, looks good.
>>
>>> +    unset -nocomplain env(GDBHISTSIZE)
>>>      array set env [array get old_env]
>>
>> Though this unset looks unnecessary, given that the following line
>> restores the whole array.
>
> It turns out that
>
>     array set env [array get old_env]
>
> does not completely restore the env array to its original state.  What
> it seems to do is to reset each pre-existing environment variable
> (existing in the saved env array) to its original value.  New
> environment variables that were set inside the env array in the
> meantime do not get unset after restoring.  So e.g. after doing
>
>     array set old_env [array get env]
>     set env(SOME_NEW_VAR) foo
>     array set env [array get old_env]
>
> the environment variable SOME_NEW_VAR=foo will still be in the env
> array.  So this "array set env" trick is insufficient.  That is why
> the unset of GDBHISTSIZE is necessary there.
>
> To make the pattern of "temporarily altering global variables,
> restoring their original value afterwards" more convenient and less
> error-prone, I've been thinking about introducing a new tcl proc that
> acts as a wrapper for saving/restoring a specified list of variables.
> Its use would look something like:
>
>     save_vars { INTERNAL_GDBFLAGS env(GDBHISTSIZE) env(HOME) } {
>         append INTERNAL_GDBFLAGS " -nx"
>         unset -nocomplain env(GDBHISTSIZE)
>         unset -nocomplain env(HOME)
>         gdb_test ....
>         more_gdb_test ...
>     }
>
> which guarantees that after the body has finished executing, the given
> list of variables will have their contents restored to their original
> values.  What do you think about this?

Here is an implementation of save_vars (bad name I know) which can
successfully replace the manual saving/restoring of globals done in
gdbinit-history.exp, gdbhistsize-history.exp and readline.exp.  (TCL
sure is cool.)

[-- Attachment #2: tmp (2).diff --]
[-- Type: text/plain, Size: 2220 bytes --]

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index b5928c3..92157a7 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -1830,6 +1830,71 @@ proc with_test_prefix { prefix body } {
   }
 }
 
+# Run BODY in the context of the caller.  After BODY is run, the variables
+# listed in VARS will be reset to the values they had before BODY was run.
+#
+# This is useful for providing a scope in which it is safe to temporarily
+# modify global variables, e.g.
+#
+# global INTERNAL_GDBFLAGS
+# global env(GDBHISTSIZE)
+#
+# save_vars { INTERNAL_GDBFLAGS env(GDBHISTSIZE) } {
+#     append INTERNAL_GDBFLAGS " -nx"
+#     unset -nocomplain env(GDBHISTSIZE)
+#     gdb_start
+#     gdb_test ...
+# }
+#
+# Here, although INTERNAL_GDBFLAGS and env(GDBHISTSIZE) are modified inside the
+# BODY, this proc will guarantee that the modifications will be undone after
+# BODY finishes executing.
+
+proc save_vars { vars body } {
+    array set saved_scalars { }
+    array set saved_arrays { }
+    set unset_vars { }
+
+    foreach var $vars {
+	# First evaluate VAR in the context of the caller in case the variable
+	# name may be a not-yet-interpolated string like env($foo)
+	set var [uplevel 1 list $var]
+
+	if [uplevel 1 [list info exists $var]] {
+	    if [uplevel 1 [list array exists $var]] {
+		set saved_arrays($var) [uplevel 1 [list array get $var]]
+	    } else {
+		set saved_scalars($var) [uplevel 1 [list set $var]]
+	    }
+	} else {
+	    lappend unset_vars $var
+	}
+    }
+
+    set code [catch {uplevel 1 $body} result]
+
+    foreach {var value} [array get saved_scalars] {
+	uplevel 1 [list set $var $value]
+    }
+
+    foreach {var value} [array get saved_arrays] {
+	uplevel 1 [list unset $var]
+	uplevel 1 [list array set $var $value]
+    }
+
+    foreach var $unset_vars {
+	uplevel 1 [list unset -nocomplain $var]
+    }
+
+    if {$code == 1} {
+	global errorInfo errorCode
+	return -code $code -errorinfo $errorInfo -errorcode $errorCode $result
+    } else {
+	return -code $code $result
+    }
+}
+
+
 # Run tests in BODY with GDB prompt and variable $gdb_prompt set to
 # PROMPT.  When BODY is finished, restore GDB prompt and variable
 # $gdb_prompt.

Re: [PATCH] Test the interaction between GDBHISTSIZE and .gdbinit

[Pedro Alves]

On 06/18/2015 03:47 PM, Patrick Palka wrote:

> Here is an implementation of save_vars (bad name I know) which can
> successfully replace the manual saving/restoring of globals done in
> gdbinit-history.exp, gdbhistsize-history.exp and readline.exp.  (TCL
> sure is cool.)
> 

I like it.  :-)

Thanks,
Pedro Alves

Re: [PATCH] Test the interaction between GDBHISTSIZE and .gdbinit

[Doug Evans]

On Thu, Jun 18, 2015 at 7:44 AM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> On Thu, Jun 18, 2015 at 5:06 AM, Pedro Alves <palves@redhat.com> wrote:
>> On 06/17/2015 09:17 PM, Patrick Palka wrote:
>>> The value inside the GDBHISTSIZE environment variable, only if valid,
>>> should override setting the history size through one's .gdbinit file.
>>
>> Thanks, looks good.
>>
>>> +    unset -nocomplain env(GDBHISTSIZE)
>>>      array set env [array get old_env]
>>
>> Though this unset looks unnecessary, given that the following line
>> restores the whole array.
>
> It turns out that
>
>     array set env [array get old_env]
>
> does not completely restore the env array to its original state.  What
> it seems to do is to reset each pre-existing environment variable
> (existing in the saved env array) to its original value.  New
> environment variables that were set inside the env array in the
> meantime do not get unset after restoring.

http://tcl.tk/man/tcl8.5/TclCmd/array.htm

> So e.g. after doing
>
>     array set old_env [array get env]
>     set env(SOME_NEW_VAR) foo
>     array set env [array get old_env]
>
> the environment variable SOME_NEW_VAR=foo will still be in the env
> array.  So this "array set env" trick is insufficient.  That is why
> the unset of GDBHISTSIZE is necessary there.

I haven't read the save_vars patch yet, but how about:

array set old_env [array get env]
...
array unset env ;# <<<<<<<<<<<<<<<
array set env [array get old_env]
array unset old_env

It might be a teensy bit simpler to do:

set old_env [array get env]
...
array set env $old_env
unset old_env

Dunno.

Re: [PATCH] Test the interaction between GDBHISTSIZE and .gdbinit

[Patrick Palka]

On Mon, Jun 22, 2015 at 9:21 AM, Doug Evans <dje@google.com> wrote:
> On Thu, Jun 18, 2015 at 7:44 AM, Patrick Palka <patrick@parcs.ath.cx> wrote:
>> On Thu, Jun 18, 2015 at 5:06 AM, Pedro Alves <palves@redhat.com> wrote:
>>> On 06/17/2015 09:17 PM, Patrick Palka wrote:
>>>> The value inside the GDBHISTSIZE environment variable, only if valid,
>>>> should override setting the history size through one's .gdbinit file.
>>>
>>> Thanks, looks good.
>>>
>>>> +    unset -nocomplain env(GDBHISTSIZE)
>>>>      array set env [array get old_env]
>>>
>>> Though this unset looks unnecessary, given that the following line
>>> restores the whole array.
>>
>> It turns out that
>>
>>     array set env [array get old_env]
>>
>> does not completely restore the env array to its original state.  What
>> it seems to do is to reset each pre-existing environment variable
>> (existing in the saved env array) to its original value.  New
>> environment variables that were set inside the env array in the
>> meantime do not get unset after restoring.
>
> http://tcl.tk/man/tcl8.5/TclCmd/array.htm
>
>> So e.g. after doing
>>
>>     array set old_env [array get env]
>>     set env(SOME_NEW_VAR) foo
>>     array set env [array get old_env]
>>
>> the environment variable SOME_NEW_VAR=foo will still be in the env
>> array.  So this "array set env" trick is insufficient.  That is why
>> the unset of GDBHISTSIZE is necessary there.
>
> I haven't read the save_vars patch yet, but how about:
>
> array set old_env [array get env]
> ...
> array unset env ;# <<<<<<<<<<<<<<<
> array set env [array get old_env]
> array unset old_env
>
> It might be a teensy bit simpler to do:
>
> set old_env [array get env]
> ...
> array set env $old_env
> unset old_env
>
> Dunno.

The env array is "magical" so I'm not sure if these techniques may work on it.

Re: [PATCH] Test the interaction between GDBHISTSIZE and .gdbinit

[Doug Evans]

On Mon, Jun 22, 2015 at 8:46 AM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> On Mon, Jun 22, 2015 at 9:21 AM, Doug Evans <dje@google.com> wrote:
>> On Thu, Jun 18, 2015 at 7:44 AM, Patrick Palka <patrick@parcs.ath.cx> wrote:
>>> On Thu, Jun 18, 2015 at 5:06 AM, Pedro Alves <palves@redhat.com> wrote:
>>>> On 06/17/2015 09:17 PM, Patrick Palka wrote:
>>>>> The value inside the GDBHISTSIZE environment variable, only if valid,
>>>>> should override setting the history size through one's .gdbinit file.
>>>>
>>>> Thanks, looks good.
>>>>
>>>>> +    unset -nocomplain env(GDBHISTSIZE)
>>>>>      array set env [array get old_env]
>>>>
>>>> Though this unset looks unnecessary, given that the following line
>>>> restores the whole array.
>>>
>>> It turns out that
>>>
>>>     array set env [array get old_env]
>>>
>>> does not completely restore the env array to its original state.  What
>>> it seems to do is to reset each pre-existing environment variable
>>> (existing in the saved env array) to its original value.  New
>>> environment variables that were set inside the env array in the
>>> meantime do not get unset after restoring.
>>
>> http://tcl.tk/man/tcl8.5/TclCmd/array.htm
>>
>>> So e.g. after doing
>>>
>>>     array set old_env [array get env]
>>>     set env(SOME_NEW_VAR) foo
>>>     array set env [array get old_env]
>>>
>>> the environment variable SOME_NEW_VAR=foo will still be in the env
>>> array.  So this "array set env" trick is insufficient.  That is why
>>> the unset of GDBHISTSIZE is necessary there.
>>
>> I haven't read the save_vars patch yet, but how about:
>>
>> array set old_env [array get env]
>> ...
>> array unset env ;# <<<<<<<<<<<<<<<
>> array set env [array get old_env]
>> array unset old_env
>>
>> It might be a teensy bit simpler to do:
>>
>> set old_env [array get env]
>> ...
>> array set env $old_env
>> unset old_env
>>
>> Dunno.
>
> The env array is "magical" so I'm not sure if these techniques may work on it.

Well, that's unfortunate.

https://www.tcl.tk/man/tcl8.5/TclCmd/tclvars.htm

"If the entire env array is unset then Tcl will stop monitoring env
accesses and will not update environment variables."

Still, it should be possible to write the equivalent.
I'll look at the save_vars patch.

Re: [PATCH] Test the interaction between GDBHISTSIZE and .gdbinit

[Doug Evans]

On Thu, Jun 18, 2015 at 9:47 AM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> Here is an implementation of save_vars (bad name I know) which can
> successfully replace the manual saving/restoring of globals done in
> gdbinit-history.exp, gdbhistsize-history.exp and readline.exp.  (TCL
> sure is cool.)

Emacs has save-excursion so there is some precedent for the naming choice.

LGTM

Re: [PATCH] Test the interaction between GDBHISTSIZE and .gdbinit

[Pedro Alves]

Hi Patrick,

On 06/18/2015 04:29 PM, Pedro Alves wrote:
> On 06/18/2015 03:47 PM, Patrick Palka wrote:
> 
>> Here is an implementation of save_vars (bad name I know) which can
>> successfully replace the manual saving/restoring of globals done in
>> gdbinit-history.exp, gdbhistsize-history.exp and readline.exp.  (TCL
>> sure is cool.)
>>
> 
> I like it.  :-)

I just found another use for this.  AFAICS, Doug was OK with it
as well.

Mind if we put this in?

Thanks,
Pedro Alves

Re: [PATCH] Test the interaction between GDBHISTSIZE and .gdbinit

[Patrick Palka]

On Tue, Aug 11, 2015 at 6:11 PM, Pedro Alves <palves@redhat.com> wrote:
> Hi Patrick,
>
> On 06/18/2015 04:29 PM, Pedro Alves wrote:
>> On 06/18/2015 03:47 PM, Patrick Palka wrote:
>>
>>> Here is an implementation of save_vars (bad name I know) which can
>>> successfully replace the manual saving/restoring of globals done in
>>> gdbinit-history.exp, gdbhistsize-history.exp and readline.exp.  (TCL
>>> sure is cool.)
>>>
>>
>> I like it.  :-)
>
> I just found another use for this.  AFAICS, Doug was OK with it
> as well.
>
> Mind if we put this in?

Done.