[COMMITTED] Fix PR gdb/17820

  Hi Patrick,

I noticed that the buildbots are showing that this new test is failing:

 https://sourceware.org/ml/gdb-testers/2015-q2/msg04164.html

~~~
  

On Fri, May 15, 2015 at 12:05 PM, Pedro Alves <palves@redhat.com> wrote:
> Hi Patrick,
>
> I noticed that the buildbots are showing that this new test is failing:
>
>  https://sourceware.org/ml/gdb-testers/2015-q2/msg04164.html
>
> ~~~
> ============================
> new FAIL: gdb.base/gdbinit-history.exp: show commands <<2>>
> new FAIL: gdb.base/gdbinit-history.exp: show history size
> new FAIL: gdb.base/gdbinit-history.exp: show history size <<2>>
> ~~~
>
> Logs at:
>
>  http://gdb-build.sergiodj.net/cgit/Fedora-x86_64-m64/.git/tree/?h=master&id=aa0e3e6cdb70d805ec9a3c689861d936ab9c5c90
>
>  (follow the "plain" link on the right right)
>
> Then I noticed that the test is also failing on my
> machine (F20).  :-)  I went ahead and wrote a fix.  What do
> you think?

Interesting.  I obviously did not encounter this failure even though I
too have HISTSIZE set (in my local rc file however).

>
> -------
> From d75f038583bfa6265253e389c1012dc29eabd208 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Fri, 15 May 2015 16:47:23 +0100
> Subject: [PATCH] Fix gdb.base/gdbinit-history.exp when HISTSIZE is set in the
>  environment
>
> Some buildslaves are showing that this test is failing.  E.g.,:
>
>  https://sourceware.org/ml/gdb-testers/2015-q2/msg04164.html
>
> The issue is that HISTSIZE is set to 1000 in the environment that runs
> the tests (that's the default in Fedora, set in /etc/profile).
>
> We can trivially reproduce it with:
>
>  $ HISTSIZE=1000 make check RUNTESTFLAGS="gdbinit-history.exp"
>  (...)
>  Running /home/pedro/gdb/mygit/src/gdb/testsuite/gdb.base/gdbinit-history.exp ...
>  FAIL: gdb.base/gdbinit-history.exp: show history size
>  FAIL: gdb.base/gdbinit-history.exp: show history size
>  FAIL: gdb.base/gdbinit-history.exp: show commands
>
> gdb.log shows:
>  ...
>  (gdb) set height 0
>  (gdb) set width 0
>  (gdb) show history size
>  The size of the command history is 1000.
>  (gdb) FAIL: gdb.base/gdbinit-history.exp: show history size
>
> gdb/testsuite/ChangeLog:
> 2015-05-15  Pedro Alves  <palves@redhat.com>
>
>         * gdb.base/gdbinit-history.exp (test_gdbinit_history_setting):
>         Unset HISTSIZE in the environment while testing and restore
>         afterwards.
> ---
>  gdb/testsuite/gdb.base/gdbinit-history.exp | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/gdb/testsuite/gdb.base/gdbinit-history.exp b/gdb/testsuite/gdb.base/gdbinit-history.exp
> index 474680a..7513934 100644
> --- a/gdb/testsuite/gdb.base/gdbinit-history.exp
> +++ b/gdb/testsuite/gdb.base/gdbinit-history.exp
> @@ -29,6 +29,17 @@ proc test_gdbinit_history_setting { home size } {
>
>      set old_home $env(HOME)
>      set env(HOME) "$srcdir/$subdir/$home"
> +
> +    # The HISTSIZE environment variable takes precedence over whatever
> +    # history size is set in .gdbinit.  Make sure the former is not
> +    # set.
> +    set have_old_histsize 0
> +    if [info exists env(HISTSIZE)] {
> +       set old_histsize $env(HISTSIZE)
> +       unset env(HISTSIZE)
> +       set have_old_histsize 1
> +    }
> +
>      set saved_internal_gdbflags $INTERNAL_GDBFLAGS
>      set INTERNAL_GDBFLAGS [string map {"-nx" ""} $INTERNAL_GDBFLAGS]
>
> @@ -44,6 +55,11 @@ proc test_gdbinit_history_setting { home size } {
>      }
>
>      set INTERNAL_GDBFLAGS $saved_internal_gdbflags
> +
> +    if {$have_old_histsize} {
> +       set env(HISTSIZE) $old_histsize
> +    }

Why not change this predicate to

     if [info exists old_histsize]

to obviate the need for $have_old_histsize altogether?

> +
>      set $env(HOME) $old_home
>  }
>
> --
> 1.9.3
>
>
  

On Fri, May 15, 2015 at 12:05 PM, Pedro Alves <palves@redhat.com> wrote:
> Hi Patrick,
>
> I noticed that the buildbots are showing that this new test is failing:
>
>  https://sourceware.org/ml/gdb-testers/2015-q2/msg04164.html
>
> ~~~
> ============================
> new FAIL: gdb.base/gdbinit-history.exp: show commands <<2>>
> new FAIL: gdb.base/gdbinit-history.exp: show history size
> new FAIL: gdb.base/gdbinit-history.exp: show history size <<2>>

Also the tests in this file have duplicate names.. That's undesirable
right?  If so I could make the names unique.  Would such a change fall
under the "obvious" rule?
  

On 05/15/2015 06:09 PM, Patrick Palka wrote:
> On Fri, May 15, 2015 at 12:05 PM, Pedro Alves <palves@redhat.com> wrote:
>> Hi Patrick,
>>
>> I noticed that the buildbots are showing that this new test is failing:
>>
>>  https://sourceware.org/ml/gdb-testers/2015-q2/msg04164.html
>>
>> ~~~
>> ============================
>> new FAIL: gdb.base/gdbinit-history.exp: show commands <<2>>
>> new FAIL: gdb.base/gdbinit-history.exp: show history size
>> new FAIL: gdb.base/gdbinit-history.exp: show history size <<2>>
> 
> Also the tests in this file have duplicate names.. That's undesirable
> right?  If so I could make the names unique.  

Yes.  I should have spotted that earlier.

> Would such a change fall under the "obvious" rule?

Not sure, depends on how you would fix it. :-)  Apply the test
described in MAINTAINERS.  :-)

There are a couple ways to address that.  In cases like
this test, where we have a function that called multiple
times, the modern way is to use with_test_prefix to wrap the
function call or the function body, which then also covers
FAILs issued from within gdb_start, etc.

Thanks,
Pedro Alves
  

On Fri, May 15, 2015 at 1:17 PM, Pedro Alves <palves@redhat.com> wrote:
> On 05/15/2015 06:09 PM, Patrick Palka wrote:
>> On Fri, May 15, 2015 at 12:05 PM, Pedro Alves <palves@redhat.com> wrote:
>>> Hi Patrick,
>>>
>>> I noticed that the buildbots are showing that this new test is failing:
>>>
>>>  https://sourceware.org/ml/gdb-testers/2015-q2/msg04164.html
>>>
>>> ~~~
>>> ============================
>>> new FAIL: gdb.base/gdbinit-history.exp: show commands <<2>>
>>> new FAIL: gdb.base/gdbinit-history.exp: show history size
>>> new FAIL: gdb.base/gdbinit-history.exp: show history size <<2>>
>>
>> Also the tests in this file have duplicate names.. That's undesirable
>> right?  If so I could make the names unique.
>
> Yes.  I should have spotted that earlier.
>
>> Would such a change fall under the "obvious" rule?
>
> Not sure, depends on how you would fix it. :-)  Apply the test
> described in MAINTAINERS.  :-)

So it will probably not be obvious because of naming preferences.

>
> There are a couple ways to address that.  In cases like
> this test, where we have a function that called multiple
> times, the modern way is to use with_test_prefix to wrap the
> function call or the function body, which then also covers
> FAILs issued from within gdb_start, etc.

Cool.. I will do something like that.
  

On 05/15/2015 06:26 PM, Patrick Palka wrote:
> On Fri, May 15, 2015 at 1:17 PM, Pedro Alves <palves@redhat.com> wrote:
>> On 05/15/2015 06:09 PM, Patrick Palka wrote:
>>> On Fri, May 15, 2015 at 12:05 PM, Pedro Alves <palves@redhat.com> wrote:
>>>> Hi Patrick,
>>>>
>>>> I noticed that the buildbots are showing that this new test is failing:
>>>>
>>>>  https://sourceware.org/ml/gdb-testers/2015-q2/msg04164.html
>>>>
>>>> ~~~
>>>> ============================
>>>> new FAIL: gdb.base/gdbinit-history.exp: show commands <<2>>
>>>> new FAIL: gdb.base/gdbinit-history.exp: show history size
>>>> new FAIL: gdb.base/gdbinit-history.exp: show history size <<2>>
>>>
>>> Also the tests in this file have duplicate names.. That's undesirable
>>> right?  If so I could make the names unique.
>>
>> Yes.  I should have spotted that earlier.
>>
>>> Would such a change fall under the "obvious" rule?
>>
>> Not sure, depends on how you would fix it. :-)  Apply the test
>> described in MAINTAINERS.  :-)
> 
> So it will probably not be obvious because of naming preferences.

Sorry for the trigger-happy pun.  I didn't mean to sound
rude or put you off.  I certainly do not hate your work.  :-)

I was just thinking that someone not familiar with the
testsuite's history might consider obvious to change the test
names one by one, while we avoid that nowadays in some cases
(like described below).

> 
>>
>> There are a couple ways to address that.  In cases like
>> this test, where we have a function that called multiple
>> times, the modern way is to use with_test_prefix to wrap the
>> function call or the function body, which then also covers
>> FAILs issued from within gdb_start, etc.
> 
> Cool.. I will do something like that.

Excellent, thanks.
  

On Fri, May 15, 2015 at 1:48 PM, Pedro Alves <palves@redhat.com> wrote:
> On 05/15/2015 06:26 PM, Patrick Palka wrote:
>> On Fri, May 15, 2015 at 1:17 PM, Pedro Alves <palves@redhat.com> wrote:
>>> On 05/15/2015 06:09 PM, Patrick Palka wrote:
>>>> On Fri, May 15, 2015 at 12:05 PM, Pedro Alves <palves@redhat.com> wrote:
>>>>> Hi Patrick,
>>>>>
>>>>> I noticed that the buildbots are showing that this new test is failing:
>>>>>
>>>>>  https://sourceware.org/ml/gdb-testers/2015-q2/msg04164.html
>>>>>
>>>>> ~~~
>>>>> ============================
>>>>> new FAIL: gdb.base/gdbinit-history.exp: show commands <<2>>
>>>>> new FAIL: gdb.base/gdbinit-history.exp: show history size
>>>>> new FAIL: gdb.base/gdbinit-history.exp: show history size <<2>>
>>>>
>>>> Also the tests in this file have duplicate names.. That's undesirable
>>>> right?  If so I could make the names unique.
>>>
>>> Yes.  I should have spotted that earlier.
>>>
>>>> Would such a change fall under the "obvious" rule?
>>>
>>> Not sure, depends on how you would fix it. :-)  Apply the test
>>> described in MAINTAINERS.  :-)
>>
>> So it will probably not be obvious because of naming preferences.
>
> Sorry for the trigger-happy pun.  I didn't mean to sound
> rude or put you off.  I certainly do not hate your work.  :-)

No problem, I had not inferred any rude intentions.

>
> I was just thinking that someone not familiar with the
> testsuite's history might consider obvious to change the test
> names one by one, while we avoid that nowadays in some cases
> (like described below).

Yeah, I was planning on exactly that!

>
>>
>>>
>>> There are a couple ways to address that.  In cases like
>>> this test, where we have a function that called multiple
>>> times, the modern way is to use with_test_prefix to wrap the
>>> function call or the function body, which then also covers
>>> FAILs issued from within gdb_start, etc.
>>
>> Cool.. I will do something like that.
>
> Excellent, thanks.
>
> --
> Pedro Alves
>