From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17851 invoked by alias); 25 Mar 2013 19:08:27 -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 17806 invoked by uid 89); 25 Mar 2013 19:08:19 -0000 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; Mon, 25 Mar 2013 19:08:19 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r2PJ8FfV021118 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Mon, 25 Mar 2013 15:08:15 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r2PJ8DA7020977; Mon, 25 Mar 2013 15:08:14 -0400 Message-ID: <5150A09D.3090202@redhat.com> Date: Mon, 25 Mar 2013 22:55: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: dje@google.com, gdb-patches@sourceware.org Subject: 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> In-Reply-To: <503CD0F7.20906@codesourcery.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-03/txt/msg00949.txt.bz2 > 2012-08-28 Yao Qi > > * top.c (history_size): Add 'unsigned'. > (show_commands): Change local variables to 'unsigned'. > (set_history_size_command): Don't check history_size is negative. > Adjust the condition to call unstifle_history and set history_size > to UNIT_MAX. I'd like to revert "set history size" back to signed. http://sourceware.org/ml/gdb-patches/2012-08/msg00832.html All the variables associated with the command are int, and they need to be, because that's how the readline interfaces are defined. As it stands, this introduced signed/unsigned comparisons and undefined overflows, like in: void show_commands (char *args, int from_tty) { /* Index for history commands. Relative to history_base. */ int offset; ... /* 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)) ... } With history_size set to UINT_MAX, with a (very) large history, that will overflow the signed "offset". Making "offset" itself unsigned is useless, as then we'd overflow on the call to history_get. The fact that all the code that uses these interfaces and variables is "signed", and that "history_size" ends up trimmed to INT_MAX anyway: /* Called by do_setshow_command. */ 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) { /* Ensure that 'show history size' prints 'unlimited'. */ history_size = UINT_MAX; unstifle_history (); } else stifle_history (history_size); } very much reads to me that making this "unsigned" is not justified. This patch changes the command back to var_integer. It's not a reversion -- I'm doing things differently from what was done before. Namely, if a negative value is specified, the user sees the exact same error the uinteger command throws. Also, in that case, the command reverts back to the previous setting, like current code does implicitly, but unlike the original, that would change the setting to the default on range error. IOW, there's no user visible change compared to the current code. gdb/ 2013-03-25 Pedro Alves * top.c (history_size): Change type to back int. (setshow_history_size_var): New global. (show_commands): Change local variables back to 'int'. (set_history_size_command): Revert to previous setting and error out if user set the setting to negative. (init_history): Set setshow_history_size_var. (init_main): Change back "set/show history size" to integer command. --- gdb/top.c | 43 ++++++++++++++++++++++++++++++------------- 1 file changed, 30 insertions(+), 13 deletions(-) diff --git a/gdb/top.c b/gdb/top.c index 7905b51..bc83496 100644 --- a/gdb/top.c +++ b/gdb/top.c @@ -711,7 +711,15 @@ show_write_history_p (struct ui_file *file, int from_tty, value); } -static unsigned int history_size; +/* The history size, as set with the "set/show history size" command. + This is not the variable registered with the set/show commands + though. */ +static int history_size; + +/* Variable associated with "set/show history size" command, as + control variable. Needed for extra "set" validation. */ +static int setshow_history_size_var; + static void show_history_size (struct ui_file *file, int from_tty, struct cmd_list_element *c, const char *value) @@ -1381,7 +1389,7 @@ 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; /* Print out some of the commands from the command history. */ /* First determine the length of the history list. */ @@ -1446,15 +1454,21 @@ 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) + if (setshow_history_size_var < 0) { - /* Ensure that 'show history size' prints 'unlimited'. */ - history_size = UINT_MAX; - unstifle_history (); + int var = setshow_history_size_var; + + /* Restore. */ + setshow_history_size_var = history_size; + error (_("integer %d out of range"), var); } - else + + /* Commit. */ + history_size = setshow_history_size_var; + + if (history_size == INT_MAX) + unstifle_history (); + else if (history_size >= 0) stifle_history (history_size); } @@ -1512,6 +1526,9 @@ init_history (void) else if (!history_size) history_size = 256; + /* Sync the set/show control variable. */ + setshow_history_size_var = history_size; + stifle_history (history_size); tmpenv = getenv ("GDBHISTFILE"); @@ -1635,13 +1652,13 @@ Without an argument, saving is enabled."), show_write_history_p, &sethistlist, &showhistlist); - add_setshow_uinteger_cmd ("size", no_class, &history_size, _("\ + add_setshow_integer_cmd ("size", no_class, &setshow_history_size_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."), - set_history_size_command, - show_history_size, - &sethistlist, &showhistlist); + set_history_size_command, + show_history_size, + &sethistlist, &showhistlist); add_setshow_filename_cmd ("filename", no_class, &history_filename, _("\ Set the filename in which to record the command history"), _("\ -- Pedro Alves