| Message ID | 1432248648-7402-1-git-send-email-patrick@parcs.ath.cx |
|---|---|
| State | New, archived |
| Headers |
Received: (qmail 12632 invoked by alias); 21 May 2015 22:51:03 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 12623 invoked by uid 89); 21 May 2015 22:51:02 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.2 required=5.0 tests=AWL, BAYES_05, KAM_LAZY_DOMAIN_SECURITY, RCVD_IN_DNSWL_LOW autolearn=no version=3.3.2 X-HELO: mail-qk0-f171.google.com Received: from mail-qk0-f171.google.com (HELO mail-qk0-f171.google.com) (209.85.220.171) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Thu, 21 May 2015 22:51:01 +0000 Received: by qkdn188 with SMTP id n188so1259047qkd.2 for <gdb-patches@sourceware.org>; Thu, 21 May 2015 15:50:58 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=PJIY+e1qPiO+HPYdPBBTrrLpaTyE0D2KwAq5KzFicbw=; b=YKT7uXT7Oy7B78Wr//ZEZGa48bgDhRmB1ZUvpS54iOGYhaxykXc/1ZvEQtGos5em5c 8gUgB9XSQfFWwKQbm2hqQnt+DSQNqIyA68sAI9pood7lSPOGPQkCCZR/U0WbmcnYmq+C XCK5eZN5KozR9CsfRS/TATE/ZQDN4n6/OZ5SJlUNhpklEqX64vWQbvI1TrL8fGIi83T8 +Gt8EulY6HjifMCR12v+htckzVLvd8R4o8urpkrWJaB26KXw59lsKkQaUn6NXClGyRAc pEBFzHZIel4xC/xJd1HyiyCufEe21aVsfOG/uHMNQcPFXbbfowg1dvxmmS4rOjw1Ic4o 2vdw== X-Gm-Message-State: ALoCoQktZLGZKqWXEHvWhZY3pQpad783vN6/xH47Gr70H5DlEMj7MjKqFIk4WB1bauzGetfvk1mA X-Received: by 10.140.16.173 with SMTP id 42mr7250924qgb.31.1432248658725; Thu, 21 May 2015 15:50:58 -0700 (PDT) Received: from localhost.localdomain (ool-4353acd8.dyn.optonline.net. [67.83.172.216]) by mx.google.com with ESMTPSA id 17sm178815qky.29.2015.05.21.15.50.57 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Thu, 21 May 2015 15:50:57 -0700 (PDT) From: Patrick Palka <patrick@parcs.ath.cx> To: gdb-patches@sourceware.org Cc: Patrick Palka <patrick@parcs.ath.cx> Subject: [PATCH] Tweak the handling of $HISTSIZE edge cases [PR gdb/16999] Date: Thu, 21 May 2015 18:50:48 -0400 Message-Id: <1432248648-7402-1-git-send-email-patrick@parcs.ath.cx> |
Commit Message
Patrick Palka
May 21, 2015, 10:50 p.m. UTC
When GDB reads a nonsensical value for the HISTSIZE environment variable, i.e. one that is non-numeric or negative, GDB then sets its history size to 0. This behavior is annoying and also inconsistent with the behavior of bash. This patch makes the behavior of invalid HISTSIZE mostly match that of bash. When we encounter an invalid or null HISTSIZE we now set the history size to unlimited instead of 0. Whereas bash ignores a non-numeric HISTSIZE, we set the history to unlimited in that case so that an accidental typo will not potentially truncate the user's history. gdb/ChangeLog: PR gdb/16999 * top.c (init_history): For invalid HISTSIZE, set history size to unlimited. gdb/testsuite/ChangeLog: PR gdb/16999 * gdb.base/histsize-history.exp: New test. --- gdb/testsuite/gdb.base/histsize-history.exp | 60 +++++++++++++++++++++++++++++ gdb/top.c | 22 ++++++----- 2 files changed, 73 insertions(+), 9 deletions(-) create mode 100644 gdb/testsuite/gdb.base/histsize-history.exp
Comments
On 05/21/2015 11:50 PM, Patrick Palka wrote: The test is OK. Good use of with_test_prefix. Minor nit: I found no other use of with_test_prefix with spaces around the '='. > diff --git a/gdb/top.c b/gdb/top.c > index 74e1e07..38b4e5d 100644 > --- a/gdb/top.c > +++ b/gdb/top.c > @@ -1684,17 +1684,21 @@ init_history (void) > if (tmpenv) > { > int var; > + char *endptr; > > - var = atoi (tmpenv); > - if (var < 0) > - { > - /* Prefer ending up with no history rather than overflowing > - readline's history interface, which uses signed 'int' > - everywhere. */ > - var = 0; > - } > + var = strtol (tmpenv, &endptr, 10); > > - history_size_setshow_var = var; > + /* If HISTSIZE is the empty string, negative, or non-numeric then set the > + history size to unlimited. This behavior is mostly consistent with > + that of bash. Whereas bash ignores a non-numeric HISTSIZE, we set the > + history to unlimited in that case to avoid potentially truncating the > + user's history. */ > + if (strlen (tmpenv) == 0 > + || var < 0 > + || *endptr != '\0') If I'm reading correctly, this treats HISTSIZE=" " as "disable history". Is that intended? Also, a nit: I find it a bit odd to see strlen to check empty string in one case, and != '\0' in another, instead of: if (*tmpenv == '\0' || var < 0 || *endptr != '\0') > + history_size_setshow_var = -1; > + else > + history_size_setshow_var = var; > } > /* If the init file hasn't set a size yet, pick the default. */ > else if (history_size_setshow_var == -2) Thanks, Pedro Alves
On Thu, May 21, 2015 at 7:33 PM, Pedro Alves <palves@redhat.com> wrote: > On 05/21/2015 11:50 PM, Patrick Palka wrote: > > The test is OK. Good use of with_test_prefix. > > Minor nit: I found no other use of with_test_prefix with > spaces around the '='. > >> diff --git a/gdb/top.c b/gdb/top.c >> index 74e1e07..38b4e5d 100644 >> --- a/gdb/top.c >> +++ b/gdb/top.c >> @@ -1684,17 +1684,21 @@ init_history (void) >> if (tmpenv) >> { >> int var; >> + char *endptr; >> >> - var = atoi (tmpenv); >> - if (var < 0) >> - { >> - /* Prefer ending up with no history rather than overflowing >> - readline's history interface, which uses signed 'int' >> - everywhere. */ >> - var = 0; >> - } >> + var = strtol (tmpenv, &endptr, 10); >> >> - history_size_setshow_var = var; >> + /* If HISTSIZE is the empty string, negative, or non-numeric then set the >> + history size to unlimited. This behavior is mostly consistent with >> + that of bash. Whereas bash ignores a non-numeric HISTSIZE, we set the >> + history to unlimited in that case to avoid potentially truncating the >> + user's history. */ >> + if (strlen (tmpenv) == 0 >> + || var < 0 >> + || *endptr != '\0') > > If I'm reading correctly, this treats HISTSIZE=" " as "disable history". > Is that intended? It's not really intended. The motivation was to make sure that an obvious typo in HISTSIZE (e.g. HISTSIZE="1000-") will not truncate the history size, but HISTSIZE=" " is not really a typo. But IMO adding a more intelligent typo heuristic (one to replace *endptr != '\0') is not worth it -- it's just a history file after all. But that reminds me that the strings " 10" and "10 " should not be considered non-numeric. That could easily be achieved via a couple of calls to isspace(). > > Also, a nit: I find it a bit odd to see strlen to check empty string > in one case, and != '\0' in another, instead of: > > if (*tmpenv == '\0' > || var < 0 > || *endptr != '\0') > >> + history_size_setshow_var = -1; >> + else >> + history_size_setshow_var = var; >> } >> /* If the init file hasn't set a size yet, pick the default. */ >> else if (history_size_setshow_var == -2) Well semantically endptr is less of a string and more of a pointer to a char within a string -- at least that's how I view it. But I will change the first condition to check for '\0'. On a related note, I wonder whether it is a good idea for GDB to look at HISTSIZE at all. As the buildbots and you have shown, some distros export HISTSIZE by default and by doing so it renders useless GDB's internal "history size" setting (as far as .gdbinit is concerned). I think people expect HISTSIZE to only affect shells, not e.g. readline applications. (Otherwise, I would expect the readline library to already extract the default history size from HISTSIZE or from another environment variable, something it currently has no support for.) So I wonder whether it would be better to stop reading HISTSIZE, to instead read GDBHISTSIZE or something. > > Thanks, > Pedro Alves >
On 05/22/2015 01:26 AM, Patrick Palka wrote: > On Thu, May 21, 2015 at 7:33 PM, Pedro Alves <palves@redhat.com> wrote: >> If I'm reading correctly, this treats HISTSIZE=" " as "disable history". >> Is that intended? > > It's not really intended. The motivation was to make sure that an > obvious typo in HISTSIZE (e.g. HISTSIZE="1000-") will not truncate the > history size, but HISTSIZE=" " is not really a typo. But IMO adding a > more intelligent typo heuristic (one to replace *endptr != '\0') is > not worth it -- it's just a history file after all. I haven't gone back to recheck what bash does, but, I can see that happening in scripts, like: if whatever; then mysize=1000 fi HISTSIZE="$mysize " HISTFILESIZE="$mysize" and then mysize ends up unset. > > But that reminds me that the strings " 10" and "10 " should not be > considered non-numeric. That could easily be achieved via a couple of > calls to isspace(). Exactly, I was thinking of those too, but I didn't want to call out what the behavior should be. :-) > >> >> Also, a nit: I find it a bit odd to see strlen to check empty string >> in one case, and != '\0' in another, instead of: >> >> if (*tmpenv == '\0' >> || var < 0 >> || *endptr != '\0') >> >>> + history_size_setshow_var = -1; >>> + else >>> + history_size_setshow_var = var; >>> } >>> /* If the init file hasn't set a size yet, pick the default. */ >>> else if (history_size_setshow_var == -2) > > Well semantically endptr is less of a string and more of a pointer to > a char within a string -- at least that's how I view it. But I will > change the first condition to check for '\0'. Ah. Good point. I'm fine either way then. > > On a related note, I wonder whether it is a good idea for GDB to look > at HISTSIZE at all. As the buildbots and you have shown, some distros > export HISTSIZE by default and by doing so it renders useless GDB's > internal "history size" setting (as far as .gdbinit is concerned). I > think people expect HISTSIZE to only affect shells, not e.g. readline > applications. (Otherwise, I would expect the readline library to > already extract the default history size from HISTSIZE or from another > environment variable, something it currently has no support for.) So > I wonder whether it would be better to stop reading HISTSIZE, to > instead read GDBHISTSIZE or something. Yeah, I'm inclined to agree. Thanks, Pedro Alves
On Thu, May 21, 2015 at 8:41 PM, Pedro Alves <palves@redhat.com> wrote: > On 05/22/2015 01:26 AM, Patrick Palka wrote: >> On Thu, May 21, 2015 at 7:33 PM, Pedro Alves <palves@redhat.com> wrote: > >>> If I'm reading correctly, this treats HISTSIZE=" " as "disable history". >>> Is that intended? >> >> It's not really intended. The motivation was to make sure that an >> obvious typo in HISTSIZE (e.g. HISTSIZE="1000-") will not truncate the >> history size, but HISTSIZE=" " is not really a typo. But IMO adding a >> more intelligent typo heuristic (one to replace *endptr != '\0') is >> not worth it -- it's just a history file after all. > > I haven't gone back to recheck what bash does, but, I can see > that happening in scripts, like: > > if whatever; then > mysize=1000 > fi > HISTSIZE="$mysize " HISTFILESIZE="$mysize" > > and then mysize ends up unset. bash would treat HISTSIZE=" " as non-numeric and thus do nothing. This patch on the other hand treats non-numeric values as if they are typos and thus sets the history size to unlimited to avoid truncation. But now I'm starting to question whether this is a good idea... sigh... > >> >> But that reminds me that the strings " 10" and "10 " should not be >> considered non-numeric. That could easily be achieved via a couple of >> calls to isspace(). > > Exactly, I was thinking of those too, but I didn't want to call > out what the behavior should be. :-) > >> >>> >>> Also, a nit: I find it a bit odd to see strlen to check empty string >>> in one case, and != '\0' in another, instead of: >>> >>> if (*tmpenv == '\0' >>> || var < 0 >>> || *endptr != '\0') >>> >>>> + history_size_setshow_var = -1; >>>> + else >>>> + history_size_setshow_var = var; >>>> } >>>> /* If the init file hasn't set a size yet, pick the default. */ >>>> else if (history_size_setshow_var == -2) >> >> Well semantically endptr is less of a string and more of a pointer to >> a char within a string -- at least that's how I view it. But I will >> change the first condition to check for '\0'. > > Ah. Good point. I'm fine either way then. > >> >> On a related note, I wonder whether it is a good idea for GDB to look >> at HISTSIZE at all. As the buildbots and you have shown, some distros >> export HISTSIZE by default and by doing so it renders useless GDB's >> internal "history size" setting (as far as .gdbinit is concerned). I >> think people expect HISTSIZE to only affect shells, not e.g. readline >> applications. (Otherwise, I would expect the readline library to >> already extract the default history size from HISTSIZE or from another >> environment variable, something it currently has no support for.) So >> I wonder whether it would be better to stop reading HISTSIZE, to >> instead read GDBHISTSIZE or something. > > Yeah, I'm inclined to agree. I will make a small patch series that does this then (which will include this patch). > > Thanks, > Pedro Alves >
On Thu, May 21, 2015 at 8:56 PM, Patrick Palka <patrick@parcs.ath.cx> wrote: > On Thu, May 21, 2015 at 8:41 PM, Pedro Alves <palves@redhat.com> wrote: >> On 05/22/2015 01:26 AM, Patrick Palka wrote: >>> On Thu, May 21, 2015 at 7:33 PM, Pedro Alves <palves@redhat.com> wrote: >> >>>> If I'm reading correctly, this treats HISTSIZE=" " as "disable history". >>>> Is that intended? >>> >>> It's not really intended. The motivation was to make sure that an >>> obvious typo in HISTSIZE (e.g. HISTSIZE="1000-") will not truncate the >>> history size, but HISTSIZE=" " is not really a typo. But IMO adding a >>> more intelligent typo heuristic (one to replace *endptr != '\0') is >>> not worth it -- it's just a history file after all. >> >> I haven't gone back to recheck what bash does, but, I can see >> that happening in scripts, like: >> >> if whatever; then >> mysize=1000 >> fi >> HISTSIZE="$mysize " HISTFILESIZE="$mysize" >> >> and then mysize ends up unset. > > bash would treat HISTSIZE=" " as non-numeric and thus do nothing. > This patch on the other hand treats non-numeric values as if they are > typos and thus sets the history size to unlimited to avoid truncation. > But now I'm starting to question whether this is a good idea... > sigh... > >> >>> >>> But that reminds me that the strings " 10" and "10 " should not be >>> considered non-numeric. That could easily be achieved via a couple of >>> calls to isspace(). >> >> Exactly, I was thinking of those too, but I didn't want to call >> out what the behavior should be. :-) >> >>> >>>> >>>> Also, a nit: I find it a bit odd to see strlen to check empty string >>>> in one case, and != '\0' in another, instead of: >>>> >>>> if (*tmpenv == '\0' >>>> || var < 0 >>>> || *endptr != '\0') >>>> >>>>> + history_size_setshow_var = -1; >>>>> + else >>>>> + history_size_setshow_var = var; >>>>> } >>>>> /* If the init file hasn't set a size yet, pick the default. */ >>>>> else if (history_size_setshow_var == -2) >>> >>> Well semantically endptr is less of a string and more of a pointer to >>> a char within a string -- at least that's how I view it. But I will >>> change the first condition to check for '\0'. >> >> Ah. Good point. I'm fine either way then. >> >>> >>> On a related note, I wonder whether it is a good idea for GDB to look >>> at HISTSIZE at all. As the buildbots and you have shown, some distros >>> export HISTSIZE by default and by doing so it renders useless GDB's >>> internal "history size" setting (as far as .gdbinit is concerned). I >>> think people expect HISTSIZE to only affect shells, not e.g. readline >>> applications. (Otherwise, I would expect the readline library to >>> already extract the default history size from HISTSIZE or from another >>> environment variable, something it currently has no support for.) So >>> I wonder whether it would be better to stop reading HISTSIZE, to >>> instead read GDBHISTSIZE or something. >> >> Yeah, I'm inclined to agree. > > I will make a small patch series that does this then (which will > include this patch). What do you think about removing HISTSIZE/GDBHISTSIZE support altogether? It is awfully redundant (we can already automatically set the history size via .gdbinit or via -ex "set history size foo") and thus not really useful. Even if we go along with replacing HISTSIZE with GDBHISTSIZE I just can't see much use for it. > >> >> Thanks, >> Pedro Alves >>
Changing title to call for attention. Maybe we should ask on gdb@. Background here: https://sourceware.org/ml/gdb-patches/2015-05/msg00349.html https://sourceware.org/ml/gdb-patches/2015-05/msg00563.html > What do you think about removing HISTSIZE/GDBHISTSIZE support > altogether? It is awfully redundant (we can already automatically set > the history size via .gdbinit or via -ex "set history size foo") and > thus not really useful. Even if we go along with replacing HISTSIZE > with GDBHISTSIZE I just can't see much use for it. What about GDBHISTFILE? I think that the rationale for the existence of one should apply to both. (with the HISTSIZE vs GDBHISTSIZE distinction being a separate matter.) I'm really not sure. Trying to play devil's advocate: #1 - An env var can be set once, for all users. But that can be done with --with-system-gdbinit=FILE as well. #2 - Along with GDBHISTFILE, it survives -nx. Does it really matter? I don't know. #3 - Seems friendly to allow at least GDBHISTFILE be an env var so it can easily be toggled per host. Though that can be done through Python inside .gdbinit nowadays. Though^2, Python isn't always available. OTOH, I'm getting more convinced that we should at least rename HISTSIZE -> GDBHISTSIZE. The cost of keeping that doesn't seem to be much. Thanks, Pedro Alves
On Fri, May 22, 2015 at 6:00 AM, Pedro Alves <palves@redhat.com> wrote: > Changing title to call for attention. Maybe we should ask > on gdb@. Background here: > > https://sourceware.org/ml/gdb-patches/2015-05/msg00349.html > https://sourceware.org/ml/gdb-patches/2015-05/msg00563.html > >> What do you think about removing HISTSIZE/GDBHISTSIZE support >> altogether? It is awfully redundant (we can already automatically set >> the history size via .gdbinit or via -ex "set history size foo") and >> thus not really useful. Even if we go along with replacing HISTSIZE >> with GDBHISTSIZE I just can't see much use for it. > > What about GDBHISTFILE? I think that the rationale for the existence > of one should apply to both. (with the HISTSIZE vs GDBHISTSIZE distinction > being a separate matter.) GDBHISTFILE is less useless than HISTSIZE I think. I can imagine unique use cases for GDBHISTFILE (e.g. to have separate per-project history files) whereas for HISTSIZE, not so much. So I don't think their usefulness can be conflated. > > I'm really not sure. Trying to play devil's advocate: > > #1 - An env var can be set once, for all users. But that can be > done with --with-system-gdbinit=FILE as well. > > #2 - Along with GDBHISTFILE, it survives -nx. Does it really matter? > I don't know. > > #3 - Seems friendly to allow at least GDBHISTFILE be an env var so it > can easily be toggled per host. Though that can be done through > Python inside .gdbinit nowadays. Though^2, Python isn't always > available. > > OTOH, I'm getting more convinced that we should at least > rename HISTSIZE -> GDBHISTSIZE. The cost of keeping that > doesn't seem to be much. I just don't see any utility in this environment variable. I imagine most users stumble upon this feature by realizing that their global HISTSIZE variable is being read by GDB. Once we rename it to GDBHISTSIZE we will no longer have this coincidence and the variable will be forever ignored. > > Thanks, > Pedro Alves >
On Fri, May 22, 2015 at 7:57 AM, Patrick Palka <patrick@parcs.ath.cx> wrote: > On Fri, May 22, 2015 at 6:00 AM, Pedro Alves <palves@redhat.com> wrote: >> Changing title to call for attention. Maybe we should ask >> on gdb@. Background here: >> >> https://sourceware.org/ml/gdb-patches/2015-05/msg00349.html >> https://sourceware.org/ml/gdb-patches/2015-05/msg00563.html >> >>> What do you think about removing HISTSIZE/GDBHISTSIZE support >>> altogether? It is awfully redundant (we can already automatically set >>> the history size via .gdbinit or via -ex "set history size foo") and >>> thus not really useful. Even if we go along with replacing HISTSIZE >>> with GDBHISTSIZE I just can't see much use for it. >> >> What about GDBHISTFILE? I think that the rationale for the existence >> of one should apply to both. (with the HISTSIZE vs GDBHISTSIZE distinction >> being a separate matter.) > > GDBHISTFILE is less useless than HISTSIZE I think. I can imagine > unique use cases for GDBHISTFILE (e.g. to have separate per-project > history files) whereas for HISTSIZE, not so much. So I don't think > their usefulness can be conflated. > >> >> I'm really not sure. Trying to play devil's advocate: >> >> #1 - An env var can be set once, for all users. But that can be >> done with --with-system-gdbinit=FILE as well. >> >> #2 - Along with GDBHISTFILE, it survives -nx. Does it really matter? >> I don't know. >> >> #3 - Seems friendly to allow at least GDBHISTFILE be an env var so it >> can easily be toggled per host. Though that can be done through >> Python inside .gdbinit nowadays. Though^2, Python isn't always >> available. >> >> OTOH, I'm getting more convinced that we should at least >> rename HISTSIZE -> GDBHISTSIZE. The cost of keeping that >> doesn't seem to be much. > > I just don't see any utility in this environment variable. I imagine > most users stumble upon this feature by realizing that their global > HISTSIZE variable is being read by GDB. Once we rename it to > GDBHISTSIZE we will no longer have this coincidence and the variable > will be forever ignored. ... but since the work to rename it to GDBHISTSIZE has already been done, we might as well keep it :) > >> >> Thanks, >> Pedro Alves >>
diff --git a/gdb/testsuite/gdb.base/histsize-history.exp b/gdb/testsuite/gdb.base/histsize-history.exp new file mode 100644 index 0000000..10fc453 --- /dev/null +++ b/gdb/testsuite/gdb.base/histsize-history.exp @@ -0,0 +1,60 @@ +# Copyright 2015 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +# This file is part of the gdb testsuite. + +# Test the setting of "history size" via the HISTSIZE environment variable + + +# Check that the history size is properly set to SIZE when env(HISTSIZE) is set +# to HISTSIZE. + +proc test_histsize_history_setting { histsize size } { + global env + + set have_old_histsize 0 + if [info exists env(HISTSIZE)] { + set have_old_histsize 1 + set old_histsize $env(HISTSIZE) + } + set env(HISTSIZE) $histsize + + with_test_prefix "histsize = $histsize" { + gdb_exit + gdb_start + + gdb_test "show history size" "The size of the command history is $size." + + if { $size == "0" } { + gdb_test_no_output "show commands" + } elseif { $size != "1" } { + gdb_test "show commands" \ + " . show history size\r\n . show commands" + } + + if { $have_old_histsize } { + set env(HISTSIZE) $old_histsize + } else { + unset env(HISTSIZE) + } + } +} + +test_histsize_history_setting "" "unlimited" +test_histsize_history_setting "0" "0" +test_histsize_history_setting "20" "20" +test_histsize_history_setting "-5" "unlimited" +test_histsize_history_setting "not_an_integer" "unlimited" +test_histsize_history_setting "10zab" "unlimited" diff --git a/gdb/top.c b/gdb/top.c index 74e1e07..38b4e5d 100644 --- a/gdb/top.c +++ b/gdb/top.c @@ -1684,17 +1684,21 @@ init_history (void) if (tmpenv) { int var; + char *endptr; - var = atoi (tmpenv); - if (var < 0) - { - /* Prefer ending up with no history rather than overflowing - readline's history interface, which uses signed 'int' - everywhere. */ - var = 0; - } + var = strtol (tmpenv, &endptr, 10); - history_size_setshow_var = var; + /* If HISTSIZE is the empty string, negative, or non-numeric then set the + history size to unlimited. This behavior is mostly consistent with + that of bash. Whereas bash ignores a non-numeric HISTSIZE, we set the + history to unlimited in that case to avoid potentially truncating the + user's history. */ + if (strlen (tmpenv) == 0 + || var < 0 + || *endptr != '\0') + history_size_setshow_var = -1; + else + history_size_setshow_var = var; } /* If the init file hasn't set a size yet, pick the default. */ else if (history_size_setshow_var == -2)