Message ID | 1430073669-31059-1-git-send-email-patrick@parcs.ath.cx |
---|---|
State | New, archived |
Headers |
Received: (qmail 84808 invoked by alias); 26 Apr 2015 18:42:05 -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 84797 invoked by uid 89); 26 Apr 2015 18:42:04 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL, BAYES_00, KAM_LAZY_DOMAIN_SECURITY, RCVD_IN_DNSWL_LOW autolearn=no version=3.3.2 X-HELO: mail-qg0-f51.google.com Received: from mail-qg0-f51.google.com (HELO mail-qg0-f51.google.com) (209.85.192.51) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Sun, 26 Apr 2015 18:42:03 +0000 Received: by qgdy78 with SMTP id y78so42018884qgd.0 for <gdb-patches@sourceware.org>; Sun, 26 Apr 2015 11:42:01 -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=bWsPUC4yYueLK9SkXfOi/Y+10wxIPVf1Wvlg2DroUug=; b=ZUvLrFRhUDVY0rVEt/Si1jG9raY2uKpTwQZanvZsNnSpfVS+nNGEAVXF1uYhPLG9da 1HDXQiTTMNoaK9sUFsJWTCvPjqYg4E813bgWS9WI4vFqD82GFvTuiRwEYNF8BMfKkWug KnLcEdBsn+WlXdRh/UWT1hoVpvjUTLh1JOFxQhwGypqqy50Sw4UQ+YsYonCtNgTobGMd +TY+WuP5FnehPPu7EMEe9+a8+Twz1OmPo07hKHRHRul3vCGpFdYlnBf1Wa7fA4CY1V9K DPez8ZhfcpiyDlUJMAGybKu8GRza8RGHgIdZqQPDJxRxT4EGXGCcoWLmpwZ5qTv+4vgS ZKCQ== X-Gm-Message-State: ALoCoQnaIDRi+YHWPBP8FGYAdlF91iCTSIA71X6WEktROY+4DtmG8KPa4/j2EcfjrozSJl5nKlZi X-Received: by 10.55.31.32 with SMTP id f32mr8834682qkf.81.1430073720828; Sun, 26 Apr 2015 11:42:00 -0700 (PDT) Received: from localhost.localdomain (ool-4353acd8.dyn.optonline.net. [67.83.172.216]) by mx.google.com with ESMTPSA id s91sm10734306qge.44.2015.04.26.11.41.59 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Sun, 26 Apr 2015 11:42:00 -0700 (PDT) From: Patrick Palka <patrick@parcs.ath.cx> To: gdb-patches@sourceware.org Cc: Patrick Palka <patrick@parcs.ath.cx> Subject: [PATCH] Fix PR gdb/17820 Date: Sun, 26 Apr 2015 14:41:09 -0400 Message-Id: <1430073669-31059-1-git-send-email-patrick@parcs.ath.cx> |
Commit Message
Patrick Palka
April 26, 2015, 6:41 p.m. UTC
This patch is a comprehensive fix for PR 17820 which reports that using "set history size unlimited" inside one's gdbinit file doesn't really work. There are three small changes in this patch. The most important change this patch makes is to decode the argument of the "size" subcommand using add_setshow_zuinteger_unlimited_cmd() instead of using add_setshow_uinteger_cmd(). The new decoder takes an int * and maps unlimited to -1 whereas the old decoder takes an unsigned int * and maps unlimited to UINT_MAX. Using the new decoder simplifies our handling of unlimited and makes it easier to interface with readline which itself expects a signed-int history size. The second change is the factoring of the [stifle|unstifle]_history logic into a common function which is now used by both init_history() and set_history_size_command(). This is technically the change that fixes the PR itself. Thirdly, this patch initializes history_size_setshow_var to -2 to mean that the variable has not been set yet. Now init_history() tests for -2 instead of 0 to determine whether to give the variable a default value. This means that having "set history size 0" in one's gdbinit file will actually keep the history size at 0 and not reset it to 256. [Alternatively I can just initialize the variable to 256 in the first place. Would that be better?] gdb/ChangeLog: PR gdb/17820 * top.c (history_size_setshow_var): Change type to signed. Initialize to -2. Update documentation. (set_readline_history_size): Define. (set_history_size_command): Use it. Remove logic for handling out-of-range sizes. (init_history): Use set_readline_history_size(). Test for a value of -2 instead of 0 when determining whether to set a default history size. (init_main): Decode the argument of the "size" command as a zuinteger_unlimited. --- gdb/top.c | 53 ++++++++++++++++++++++------------------------------- 1 file changed, 22 insertions(+), 31 deletions(-)
Comments
On 04/26/2015 07:41 PM, Patrick Palka wrote: > This patch is a comprehensive fix for PR 17820 which reports that > using "set history size unlimited" inside one's gdbinit file doesn't > really work. > > There are three small changes in this patch. The most important change > this patch makes is to decode the argument of the "size" subcommand > using add_setshow_zuinteger_unlimited_cmd() instead of using > add_setshow_uinteger_cmd(). The new decoder takes an int * and maps > unlimited to -1 whereas the old decoder takes an unsigned int * and maps > unlimited to UINT_MAX. Using the new decoder simplifies our handling of > unlimited and makes it easier to interface with readline which itself > expects a signed-int history size. > > The second change is the factoring of the [stifle|unstifle]_history logic > into a common function which is now used by both init_history() and > set_history_size_command(). This is technically the change that fixes > the PR itself. > > Thirdly, this patch initializes history_size_setshow_var to -2 to mean > that the variable has not been set yet. Now init_history() tests for -2 > instead of 0 to determine whether to give the variable a default value. > This means that having "set history size 0" in one's gdbinit file will > actually keep the history size at 0 and not reset it to 256. Please see also: https://sourceware.org/ml/gdb-patches/2013-03/msg00962.html for background. Darn. So around that time, we added support for explicit "unlimited" to a bunch of commands. Since "set history size 0" already meant unlimited before, and since this is the sort of command that users put in .gdbinit instead of issuing manually, it was reasonable to assume if we changed the "unlimited" representation, we'd break user scripts. So the idea was that we'd let a few years/releases go by, and then we'd change "size=0" to really mean 0, and we'd tell users to use "set history size unlimited" instead. But now we see that "set history size unlimited" in .gdbinit never really worked. Bummer. So this means that users must have kept using "set history size 0" instead... So if we change this now, there's no way to have a single "set history size FOO" setting that means unlimited with both gdb <= 7.9 and the next gdb release. :-/ Oh well. Maybe we should just tell users to do "set history size BIGINT" instead? I'd definitely like to make "set history size 0" really disable history. So I think that if goes forward, it'd be good to have a NEWS entry. What do you (and others) think? > [Alternatively I can just initialize the variable to 256 in the first > place. Would that be better?] -2 is fine with me. > > gdb/ChangeLog: > > PR gdb/17820 > * top.c (history_size_setshow_var): Change type to signed. > Initialize to -2. Update documentation. > (set_readline_history_size): Define. > (set_history_size_command): Use it. Remove logic for handling > out-of-range sizes. > (init_history): Use set_readline_history_size(). Test for a > value of -2 instead of 0 when determining whether to set a > default history size. > (init_main): Decode the argument of the "size" command as a > zuinteger_unlimited. Looks good to me. Adding a testcase would be ideal, but I'll not make it a requirement. I think we should be able to write one making use of GDBFLAGSs. (and IWBN to test the GDBHISTSIZE/HISTSIZE environment variables too, which we can do with "set env(HISTSIZE)", etc.) Thanks, Pedro Alves
On Mon, Apr 27, 2015 at 2:39 PM, Pedro Alves <palves@redhat.com> wrote: > On 04/26/2015 07:41 PM, Patrick Palka wrote: >> This patch is a comprehensive fix for PR 17820 which reports that >> using "set history size unlimited" inside one's gdbinit file doesn't >> really work. >> >> There are three small changes in this patch. The most important change >> this patch makes is to decode the argument of the "size" subcommand >> using add_setshow_zuinteger_unlimited_cmd() instead of using >> add_setshow_uinteger_cmd(). The new decoder takes an int * and maps >> unlimited to -1 whereas the old decoder takes an unsigned int * and maps >> unlimited to UINT_MAX. Using the new decoder simplifies our handling of >> unlimited and makes it easier to interface with readline which itself >> expects a signed-int history size. >> >> The second change is the factoring of the [stifle|unstifle]_history logic >> into a common function which is now used by both init_history() and >> set_history_size_command(). This is technically the change that fixes >> the PR itself. >> >> Thirdly, this patch initializes history_size_setshow_var to -2 to mean >> that the variable has not been set yet. Now init_history() tests for -2 >> instead of 0 to determine whether to give the variable a default value. >> This means that having "set history size 0" in one's gdbinit file will >> actually keep the history size at 0 and not reset it to 256. > > Please see also: > > https://sourceware.org/ml/gdb-patches/2013-03/msg00962.html > > for background. I too think the variable should be signed if only because it simplifies the code. > > Darn. So around that time, we added support for explicit "unlimited" > to a bunch of commands. Since "set history size 0" already meant > unlimited before, and since this is the sort of command that users > put in .gdbinit instead of issuing manually, it was reasonable > to assume if we changed the "unlimited" representation, we'd > break user scripts. So the idea was that we'd let a few years/releases > go by, and then we'd change "size=0" to really mean 0, and we'd > tell users to use "set history size unlimited" instead. > > But now we see that "set history size unlimited" in .gdbinit never > really worked. Bummer. So this means that users must have kept > using "set history size 0" instead... "set history size 0" (in the .gdbinit) doesn't set history to unlimited either -- it sets it to 256! So users currently have no choice but to use "set history size BIGINT" for approximately-unlimited history. > > So if we change this now, there's no way to have a single > "set history size FOO" setting that means unlimited with > both gdb <= 7.9 and the next gdb release. :-/ Oh well. Maybe we should > just tell users to do "set history size BIGINT" instead? But currently, neither "set history size 0" nor "set history size unlimited" mean unlimited (in the .gdbinit). So users today cannot set an unlimited history size at all. Changing this now will now at least make "set history size unlimited". So whether or not we change this now, there will be no way to have a single "set history size FOO" setting that means unlimited across current and future GDB versions. Am I misunderstanding? > > I'd definitely like to make "set history size 0" really > disable history. > > So I think that if goes forward, it'd be good to have a NEWS entry. I can think of two things to mention. One is that "set history size 0" now really disables history. The other is that "set history size unlimited" now really does not truncate history. Do you have anything else in mind? > > What do you (and others) think? > >> [Alternatively I can just initialize the variable to 256 in the first >> place. Would that be better?] > > -2 is fine with me. > >> >> gdb/ChangeLog: >> >> PR gdb/17820 >> * top.c (history_size_setshow_var): Change type to signed. >> Initialize to -2. Update documentation. >> (set_readline_history_size): Define. >> (set_history_size_command): Use it. Remove logic for handling >> out-of-range sizes. >> (init_history): Use set_readline_history_size(). Test for a >> value of -2 instead of 0 when determining whether to set a >> default history size. >> (init_main): Decode the argument of the "size" command as a >> zuinteger_unlimited. > > Looks good to me. > > Adding a testcase would be ideal, but I'll not make it a requirement. > I think we should be able to write one making use of GDBFLAGSs. (and > IWBN to test the GDBHISTSIZE/HISTSIZE environment variables too, which > we can do with "set env(HISTSIZE)", etc.) Do you have in mind a test that creates a dummy .gdbinit file to be read by GDB? Or is there another way to test this code path? > > Thanks, > Pedro Alves >
On 04/28/2015 02:05 AM, Patrick Palka wrote: > On Mon, Apr 27, 2015 at 2:39 PM, Pedro Alves <palves@redhat.com> wrote: >> But now we see that "set history size unlimited" in .gdbinit never >> really worked. Bummer. So this means that users must have kept >> using "set history size 0" instead... > > "set history size 0" (in the .gdbinit) doesn't set history to > unlimited either -- it sets it to 256! So users currently have no > choice but to use "set history size BIGINT" for > approximately-unlimited history. Indeed, looks like that has already been the case... >> So if we change this now, there's no way to have a single >> "set history size FOO" setting that means unlimited with >> both gdb <= 7.9 and the next gdb release. :-/ Oh well. Maybe we should >> just tell users to do "set history size BIGINT" instead? > > But currently, neither "set history size 0" nor "set history size > unlimited" mean unlimited (in the .gdbinit). So users today cannot > set an unlimited history size at all. Changing this now will now at > least make "set history size unlimited". So whether or not we change > this now, there will be no way to have a single "set history size FOO" > setting that means unlimited across current and future GDB versions. > Am I misunderstanding? > >> >> I'd definitely like to make "set history size 0" really >> disable history. >> >> So I think that if goes forward, it'd be good to have a NEWS entry. > > I can think of two things to mention. One is that "set history size > 0" now really disables history. The other is that "set history size > unlimited" now really does not truncate history. Do you have anything > else in mind? That's good. Maybe add the suggestion to use "set history size BIGINT" in .gdbinit, if compatibility with previous releases is desired. >>> PR gdb/17820 >>> * top.c (history_size_setshow_var): Change type to signed. >>> Initialize to -2. Update documentation. >>> (set_readline_history_size): Define. >>> (set_history_size_command): Use it. Remove logic for handling >>> out-of-range sizes. >>> (init_history): Use set_readline_history_size(). Test for a >>> value of -2 instead of 0 when determining whether to set a >>> default history size. >>> (init_main): Decode the argument of the "size" command as a >>> zuinteger_unlimited. >> >> Looks good to me. >> >> Adding a testcase would be ideal, but I'll not make it a requirement. >> I think we should be able to write one making use of GDBFLAGSs. (and >> IWBN to test the GDBHISTSIZE/HISTSIZE environment variables too, which >> we can do with "set env(HISTSIZE)", etc.) > > Do you have in mind a test that creates a dummy .gdbinit file to be > read by GDB? Or is there another way to test this code path? It may be testable with -x or -ix on the command line too, not sure, gdb.base/bp-cmds-execution-x-script.exp is an example, though given "set history size" already behaves different today depending on when it is called, an alternate way to test the issue that happens to use the same path today may change in the future, and we may (re)introducing unnoticed bugs. So I think we should test that path exactly. I was thinking of creating a dir, put a test .gdbinit file there, and point HOME at that dir. We'd just skip the test on remote host testing. Thanks, Pedro Alves
On Wed, Apr 29, 2015 at 6:44 AM, Pedro Alves <palves@redhat.com> wrote: > On 04/28/2015 02:05 AM, Patrick Palka wrote: >> On Mon, Apr 27, 2015 at 2:39 PM, Pedro Alves <palves@redhat.com> wrote: > >>> But now we see that "set history size unlimited" in .gdbinit never >>> really worked. Bummer. So this means that users must have kept >>> using "set history size 0" instead... >> >> "set history size 0" (in the .gdbinit) doesn't set history to >> unlimited either -- it sets it to 256! So users currently have no >> choice but to use "set history size BIGINT" for >> approximately-unlimited history. > > Indeed, looks like that has already been the case... > >>> So if we change this now, there's no way to have a single >>> "set history size FOO" setting that means unlimited with >>> both gdb <= 7.9 and the next gdb release. :-/ Oh well. Maybe we should >>> just tell users to do "set history size BIGINT" instead? >> >> But currently, neither "set history size 0" nor "set history size >> unlimited" mean unlimited (in the .gdbinit). So users today cannot >> set an unlimited history size at all. Changing this now will now at >> least make "set history size unlimited". So whether or not we change >> this now, there will be no way to have a single "set history size FOO" >> setting that means unlimited across current and future GDB versions. >> Am I misunderstanding? >> >>> >>> I'd definitely like to make "set history size 0" really >>> disable history. >>> >>> So I think that if goes forward, it'd be good to have a NEWS entry. >> >> I can think of two things to mention. One is that "set history size >> 0" now really disables history. The other is that "set history size >> unlimited" now really does not truncate history. Do you have anything >> else in mind? > > That's good. Maybe add the suggestion to > use "set history size BIGINT" in .gdbinit, if compatibility > with previous releases is desired. > >>>> PR gdb/17820 >>>> * top.c (history_size_setshow_var): Change type to signed. >>>> Initialize to -2. Update documentation. >>>> (set_readline_history_size): Define. >>>> (set_history_size_command): Use it. Remove logic for handling >>>> out-of-range sizes. >>>> (init_history): Use set_readline_history_size(). Test for a >>>> value of -2 instead of 0 when determining whether to set a >>>> default history size. >>>> (init_main): Decode the argument of the "size" command as a >>>> zuinteger_unlimited. >>> >>> Looks good to me. >>> >>> Adding a testcase would be ideal, but I'll not make it a requirement. >>> I think we should be able to write one making use of GDBFLAGSs. (and >>> IWBN to test the GDBHISTSIZE/HISTSIZE environment variables too, which >>> we can do with "set env(HISTSIZE)", etc.) >> >> Do you have in mind a test that creates a dummy .gdbinit file to be >> read by GDB? Or is there another way to test this code path? > > It may be testable with -x or -ix on the command line too, not > sure, gdb.base/bp-cmds-execution-x-script.exp is an example, though > given "set history size" already behaves different today depending on when > it is called, an alternate way to test the issue that happens to use > the same path today may change in the future, and we may (re)introducing > unnoticed bugs. So I think we should test that path exactly. I was > thinking of creating a dir, put a test .gdbinit file there, and point > HOME at that dir. We'd just skip the test on remote host testing. I tried this but the problem is that the testsuite seems to always pass -nx to invocations of GDB meaning that .gdbinit files are not read. Would you know how to work around this? > > Thanks, > Pedro Alves >
On 05/12/2015 12:30 PM, Patrick Palka wrote: > On Wed, Apr 29, 2015 at 6:44 AM, Pedro Alves <palves@redhat.com> wrote: >> On 04/28/2015 02:05 AM, Patrick Palka wrote: >>>> Adding a testcase would be ideal, but I'll not make it a requirement. >>>> I think we should be able to write one making use of GDBFLAGSs. (and >>>> IWBN to test the GDBHISTSIZE/HISTSIZE environment variables too, which >>>> we can do with "set env(HISTSIZE)", etc.) >>> >>> Do you have in mind a test that creates a dummy .gdbinit file to be >>> read by GDB? Or is there another way to test this code path? >> >> It may be testable with -x or -ix on the command line too, not >> sure, gdb.base/bp-cmds-execution-x-script.exp is an example, though >> given "set history size" already behaves different today depending on when >> it is called, an alternate way to test the issue that happens to use >> the same path today may change in the future, and we may (re)introducing >> unnoticed bugs. So I think we should test that path exactly. I was >> thinking of creating a dir, put a test .gdbinit file there, and point >> HOME at that dir. We'd just skip the test on remote host testing. > > I tried this but the problem is that the testsuite seems to always > pass -nx to invocations of GDB meaning that .gdbinit files are not > read. Would you know how to work around this? > Thanks for working on this. Maybe strip -nx out of INTERNAL_GDBFLAGS temporarily. E.g.,: set saved_internal_gdbflags $INTERNAL_GDBFLAGS set INTERNAL_GDBFLAGS [string map {"-nx " ""} $INTERNAL_GDBFLAGS] ... start gdb here ... set INTERNAL_GDBFLAGS $saved_internal_gdbflags Thanks, Pedro Alves
diff --git a/gdb/top.c b/gdb/top.c index ddf5415..fb8e9e6 100644 --- a/gdb/top.c +++ b/gdb/top.c @@ -695,8 +695,8 @@ show_write_history_p (struct ui_file *file, int from_tty, } /* The variable associated with the "set/show history size" - command. */ -static unsigned int history_size_setshow_var; + command. The value -1 means unlimited, and -2 means undefined. */ +static int history_size_setshow_var = -2; static void show_history_size (struct ui_file *file, int from_tty, @@ -1618,34 +1618,26 @@ show_commands (char *args, int from_tty) } } +/* Update the size of our command history file to HISTORY_SIZE. + + A HISTORY_SIZE of -1 stands for unlimited. */ + +static void +set_readline_history_size (int history_size) +{ + gdb_assert (history_size >= -1); + + if (history_size == -1) + unstifle_history (); + else + stifle_history (history_size); +} + /* Called by do_setshow_command. */ static void set_history_size_command (char *args, int from_tty, struct cmd_list_element *c) { - /* Readline's history interface works with 'int', so it can only - handle history sizes up to INT_MAX. The command itself is - uinteger, so UINT_MAX means "unlimited", but we only get that if - the user does "set history size 0" -- "set history size <UINT_MAX>" - throws out-of-range. */ - if (history_size_setshow_var > INT_MAX - && history_size_setshow_var != UINT_MAX) - { - unsigned int new_value = history_size_setshow_var; - - /* Restore previous value before throwing. */ - if (history_is_stifled ()) - history_size_setshow_var = history_max_entries; - else - history_size_setshow_var = UINT_MAX; - - error (_("integer %u out of range"), new_value); - } - - /* Commit the new value to readline's history. */ - if (history_size_setshow_var == UINT_MAX) - unstifle_history (); - else - stifle_history (history_size_setshow_var); + set_readline_history_size (history_size_setshow_var); } void @@ -1713,12 +1705,10 @@ init_history (void) history_size_setshow_var = var; } /* If the init file hasn't set a size yet, pick the default. */ - else if (history_size_setshow_var == 0) + else if (history_size_setshow_var == -2) history_size_setshow_var = 256; - /* Note that unlike "set history size 0", "HISTSIZE=0" really sets - the history size to 0... */ - stifle_history (history_size_setshow_var); + set_readline_history_size (history_size_setshow_var); tmpenv = getenv ("GDBHISTFILE"); if (tmpenv) @@ -1867,7 +1857,8 @@ Without an argument, saving is enabled."), show_write_history_p, &sethistlist, &showhistlist); - add_setshow_uinteger_cmd ("size", no_class, &history_size_setshow_var, _("\ + add_setshow_zuinteger_unlimited_cmd ("size", no_class, + &history_size_setshow_var, _("\ Set the size of the command history,"), _("\ Show the size of the command history,"), _("\ ie. the number of previous commands to keep a record of.\n\