From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18914 invoked by alias); 26 Mar 2013 16:16:54 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 18862 invoked by uid 89); 26 Mar 2013 16:16:42 -0000 X-Spam-SWARE-Status: No, score=-8.0 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_XF autolearn=ham version=3.3.1 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Tue, 26 Mar 2013 16:16:38 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r2QGGX6b029376 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 26 Mar 2013 12:16:33 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id r2QGGVOF016068; Tue, 26 Mar 2013 12:16:32 -0400 Message-ID: <5151C9DF.6030303@redhat.com> Date: Tue, 26 Mar 2013 17:48:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130311 Thunderbird/17.0.4 MIME-Version: 1.0 To: Yao Qi CC: Pedro Alves , dje@google.com, gdb-patches@sourceware.org Subject: Re: Change "set history size" back to signed (Re: [committed]: [PATCH 1/3] var_integer -> var_uinteger) References: <7A6A55B4-0293-4AD6-AB1F-B3169F8ADCC1@cs.umd.edu> <1344871663-915-1-git-send-email-yao@codesourcery.com> <1344871663-915-2-git-send-email-yao@codesourcery.com> <20534.29766.629459.204573@ruffy2.mtv.corp.google.com> <50372591.7080404@codesourcery.com> <20535.46196.619465.922388@ruffy2.mtv.corp.google.com> <503B476F.50805@codesourcery.com> <20539.61738.119545.634687@ruffy2.mtv.corp.google.com> <503CD0F7.20906@codesourcery.com> <5150A09D.3090202@redhat.com> <515176BB.4000702@codesourcery.com> In-Reply-To: <515176BB.4000702@codesourcery.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-03/txt/msg00969.txt.bz2 On 03/26/2013 10:21 AM, Yao Qi wrote: > Pedro, > I understand that your point of choosing signed command is readline > interface is signed. There's more than one point. - the whole readline interface is signed, and works with the 0..INT_MAX range. - using an unsigned variable for the user setting adds the need for an extra level of care, looking out for the surprising signed+unsigned comparisons causing unsigned conversions / overflows. - using an unsigned variable led to mapping (INT_MAX..UINT_MAX) "unlimited". This might make sense at first sight, but it really exposes an implementation detail to the user interface that we should not expose. We don't allow using setting the size to UINT_MAX directly. The user visible interface is "use 0 for unlimited". The UINT_MAX representation is an implementation detail we could change, e.g., by keeping a separate flag for "unlimited", which is actually what the readline interface does (stifled vs non stifled). Generically speaking, exposing this detail to clients of the interface may make our lives complicated when we find the need to extend the range of some command in the future, and it's better if users (frontends/scripts) aren't relying on anything but what we tell them to use for "unlimited". Making values other than 0 error out is the way to prevent users from using those ranges inappropriately. Quite related, note: (gdb) set history size 0xffffffff integer 4294967295 out of range But, (gdb) set history size 0xfffffffe (gdb) show history size The size of the command history is unlimited. (gdb) set history size 0x100000000 integer 4294967296 out of range If values over INT_MAX are accepted as unlimited, then there's no good argument for only accepting [INT_MAX..UINT_MAX) as valid "unlimited" magic numbers, while not accepting [UINT_MAX..inf). We ended up only accepting that range because we shoehorned "unsigned" in, and we ended up exposing a worse implementation detail than the detail of "the implementation limits the history size to INT_MAX, assuming infinite available memory". I hope it's obvious that I'm more worried of these kinds of things propagating to new commands over time through copy/paste than supporting such a large history file... > However, my point of choosing unsigned command > is the characteristic of command "set history size" is unsigned. I > don't know why readline interface is signed, but I don't want this > affect or restrict the design of the command. Making the implementation of the command signed does not affect or restrict the design of the command. At least, not today. We still don't accept negative numbers. But, making the command signed usually makes the code that _uses_ the control variable simpler, without dangers of the unsigned/signed comparisons and implicit casts, etc.. @@ -1388,7 +1388,10 @@ show_commands (char *args, int from_tty) hist_len = history_size; for (offset = 0; offset < history_size; offset++) { - if (!history_get (history_base + offset)) + /* Avoid overflow when passing 'history_base + offset' to + history_get. */ + if ((offset + history_base > INT_MAX) + || !history_get (history_base + offset)) { hist_len = offset; break; This shows one of the points I'm making. Making the setting's control variable of different type of the rest of the code adds the need to recall that one variable among all these is unsigned, and that one need to think about whether these comparisons are signed or unsigned, along with the promotion/conversion rules ("hmm? isn't that that comparison always false? hmm, no, the left size is is converted to unsigned first. hmmm, what about wraparound in that addition then?") as evidenced in this and the "set listsize" changes, the need to consider this detail is easy to forget. Anyway, I care more about the exported user interface than the implementation. If we're staying with unsigned, then let's rename the variable to at least make it more obvious that this variable is not one of GNU history's public variables. Looking deeper, I don't think we need that loop at all. We can just read readline's history_length: ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Variable: int history_length The number of entries currently stored in the history list. ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /* Return the history entry which is logically at OFFSET in the history array. OFFSET is relative to history_base. */ HIST_ENTRY * history_get (offset) int offset; { int local_index; local_index = offset - history_base; return (local_index >= history_length || local_index < 0 || the_history == 0) ? (HIST_ENTRY *)NULL : the_history[local_index]; } ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ At the time this code was added (gdb 4.13 ~1994), 'history_length' was extern, but not documented in readline's GNU history documents, so I guess it wasn't considered public then and the loop was the workaround. One of the warts of GDB choosing 0 to mean unlimited is that "set history size 0" behaves differently from 'HISTSIZE=0 gdb'. The latter leaves GDB with no history, while the former means "unlimited"... $ HISTSIZE=0 ./gdb ... (gdb) show history size The size of the command history is 0. We shouldn't really change what HISTSIZE=0 means, as bash, etc. also handle 0 as real zero, and zero it's what really makes sense. I'm hoping that "set foo unlimited" might enable us to reconsider these magic 0s later at some point without causing much disturbance... We actually don't need to keep two variables in order to be able to revert to previous value on error -- we can just query readline. gdb/ 2013-03-26 Pedro Alves * top.c (history_size): Rename to ... (history_size_setshow_var): ... this. Add comment. (show_commands): Change local variable 'hist_len' back to 'int'. Set it from readline's 'history_length' instead of calling history_get in a loop. (set_history_size_command): Error out for sizes over INT_MAX. Restore previous history size on invalid size. (init_history): If HISTSIZE is negative, leave the history size as zero. Add comments. (init_main): Adjust. --- gdb/top.c | 71 ++++++++++++++++++++++++++++++++++++++++++--------------------- 1 file changed, 47 insertions(+), 24 deletions(-) diff --git a/gdb/top.c b/gdb/top.c index 7905b51..08419cb 100644 --- a/gdb/top.c +++ b/gdb/top.c @@ -711,7 +711,10 @@ show_write_history_p (struct ui_file *file, int from_tty, value); } -static unsigned int history_size; +/* The variable associated with the "set/show history size" + command. */ +static unsigned int history_size_setshow_var; + static void show_history_size (struct ui_file *file, int from_tty, struct cmd_list_element *c, const char *value) @@ -1381,19 +1384,9 @@ show_commands (char *args, int from_tty) /* The first command in the history which doesn't exist (i.e. one more than the number of the last command). Relative to history_base. */ - unsigned int hist_len; + int hist_len = history_length; /* Print out some of the commands from the command history. */ - /* First determine the length of the history list. */ - hist_len = history_size; - for (offset = 0; offset < history_size; offset++) - { - if (!history_get (history_base + offset)) - { - hist_len = offset; - break; - } - } if (args) { @@ -1446,16 +1439,30 @@ show_commands (char *args, int from_tty) static void set_history_size_command (char *args, int from_tty, struct cmd_list_element *c) { - /* The type of parameter in stifle_history is int, so values from INT_MAX up - mean 'unlimited'. */ - if (history_size >= INT_MAX) + /* 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 " + throws out-of-range. */ + if (history_size_setshow_var > INT_MAX + && history_size_setshow_var != UINT_MAX) { - /* Ensure that 'show history size' prints 'unlimited'. */ - history_size = UINT_MAX; - unstifle_history (); + 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); + stifle_history (history_size_setshow_var); } void @@ -1508,11 +1515,27 @@ init_history (void) tmpenv = getenv ("HISTSIZE"); if (tmpenv) - history_size = atoi (tmpenv); - else if (!history_size) - history_size = 256; + { + int var; + + 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; + } + + 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) + history_size_setshow_var = 256; - stifle_history (history_size); + /* Note that unlike "set history size 0", "HISTSIZE=0" really sets + the history size to 0... */ + stifle_history (history_size_setshow_var); tmpenv = getenv ("GDBHISTFILE"); if (tmpenv) @@ -1635,7 +1658,7 @@ Without an argument, saving is enabled."), show_write_history_p, &sethistlist, &showhistlist); - add_setshow_uinteger_cmd ("size", no_class, &history_size, _("\ + add_setshow_uinteger_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."), -- 1.7.11.7