From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29108 invoked by alias); 25 Mar 2013 17:46:20 -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 29083 invoked by uid 89); 25 Mar 2013 17:46:13 -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 17:46:12 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r2PHkA06022348 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Mon, 25 Mar 2013 13:46:11 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r2PHk9vq030506; Mon, 25 Mar 2013 13:46:10 -0400 Message-ID: <51508D60.6040906@redhat.com> Date: Mon, 25 Mar 2013 19: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: "Metzger, Markus T" CC: GDB Patches Subject: Re: "set record instruction-history" ? References: <51507403.6030208@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-03/txt/msg00947.txt.bz2 On 03/25/2013 04:13 PM, Metzger, Markus T wrote: >> /* The "record instruction-history" command. */ >> >> static void >> cmd_record_insn_history (char *arg, int from_tty) >> { >> int flags, size; >> >> require_record_target (); >> >> flags = get_insn_history_modifiers (&arg); >> >> /* We use a signed size to also indicate the direction. Make sure that >> unlimited remains unlimited. */ >> size = (int) record_insn_history_size; >> if (size < 0) >> size = INT_MAX; >> >> these last three lines here, though? > > This odd code makes sure that very large values don't wrap around to very small > values when we convert from unsigned int to signed it. This would change the > meaning of those values. ... > Given that the trace can't get bigger than INT_MAX, the user would > still get what he asked for - all the trace. I see, thanks. Something like if (record_call_history_size > INT_MAX) size = INT_MAX; else size = record_call_history_size; would read less surprisingly to me. I'd rather not leave traces of assumptions that a whole range of values maps to the magic "unlimited" though. I'm trying to hide it as implementation detail as much as possible. > I don't really expect such high > numbers. I'd rather expect users to set it to unlimited if they get above > 100 or so. > > How should I correct it? Could you try this patch? So 0 means unlimited, and actually ends up mapping to INT_MAX. The closest we have is an integer command, except that negative values shouldn't be accepted. This patch switches to that, and adds "set" hooks to forbid negative values. We need separate control variables because the "set" hook runs after the command machinery has already changed the value (it'd be nice to have "set validation" hooks that run prior to that, but I'll leave that to a later change; this idiom is already in use in the tree.) (The new var_uinteger_unlimited is close too, but then we'd use -1 for unlimited instead of 0 in the CLI; that's a bigger user interface change I'd rather be done as separate change, if desirable.) gdb/ 2013-03-25 Pedro Alves * record.c (record_insn_history_size): Change type to int. (setshow_record_insn_history_size_var): New global. (record_call_history_size): Change type to int. (setshow_record_call_history_size_var): New global. (cmd_record_insn_history, cmd_record_call_history): Assert instruction history size is positive. Remove cast and mapping of negative size to unlimited size. (validate_positive_integer, set_record_insn_history_size) (set_record_call_history_size): New functions. (_initialize_record): Make the "set record instruction-history-size" and "set record function-call-history-size" integer commands, and install set_record_insn_history_size and set_record_call_history_size as their "set" hooks. --- gdb/record.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 76 insertions(+), 20 deletions(-) diff --git a/gdb/record.c b/gdb/record.c index 6bc1704..cc9db7d 100644 --- a/gdb/record.c +++ b/gdb/record.c @@ -33,10 +33,19 @@ unsigned int record_debug = 0; /* The number of instructions to print in "record instruction-history". */ -static unsigned int record_insn_history_size = 10; +static int record_insn_history_size = 10; + +/* The variable registered as control variable in the "record + instruction-history" command. Necessary for extra input + validation. */ +static int setshow_record_insn_history_size_var; /* The number of functions to print in "record function-call-history". */ -static unsigned int record_call_history_size = 10; +static int record_call_history_size = 10; + +/* The variable registered as control variable in the "record + call-history" command. Necessary for extra input validation. */ +static int setshow_record_call_history_size_var; struct cmd_list_element *record_cmdlist = NULL; struct cmd_list_element *set_record_cmdlist = NULL; @@ -439,11 +448,9 @@ cmd_record_insn_history (char *arg, int from_tty) flags = get_insn_history_modifiers (&arg); - /* We use a signed size to also indicate the direction. Make sure that - unlimited remains unlimited. */ - size = (int) record_insn_history_size; - if (size < 0) - size = INT_MAX; + gdb_assert (record_insn_history_size > 0); + + size = record_insn_history_size; if (arg == NULL || *arg == 0 || strcmp (arg, "+") == 0) target_insn_history (size, flags); @@ -559,11 +566,9 @@ cmd_record_call_history (char *arg, int from_tty) flags = get_call_history_modifiers (&arg); - /* We use a signed size to also indicate the direction. Make sure that - unlimited remains unlimited. */ - size = (int) record_call_history_size; - if (size < 0) - size = INT_MAX; + gdb_assert (record_call_history_size > 0); + + size = record_call_history_size; if (arg == NULL || *arg == 0 || strcmp (arg, "+") == 0) target_call_history (size, flags); @@ -617,6 +622,51 @@ cmd_record_call_history (char *arg, int from_tty) } } +/* Helper for "set record instruction-history-size" and "set record + function-call-history-size" input validation. COMMAND_VAR is the + variable registered in the command as control variable. *SETTING + is the real setting the command allows changing. */ + +static void +validate_positive_integer (int *command_var, int *setting) +{ + if (*command_var < 0) + { + int var = *command_var; + + /* Restore previous value. */ + *command_var = *setting; + error (_("integer %d out of range"), var); + } + + /* Commit new value. */ + *setting = *command_var; +} + +/* Called by do_setshow_command. We only want values in the + (0..INT_MAX) range, while the command's machinery accepts + [INT_MIN..INT_MAX). */ + +static void +set_record_insn_history_size (char *args, int from_tty, + struct cmd_list_element *c) +{ + validate_positive_integer (&setshow_record_insn_history_size_var, + &record_insn_history_size); +} + +/* Called by do_setshow_command. We only want values in the + (0..INT_MAX) range, while the command's machinery accepts + [INT_MIN..INT_MAX). */ + +static void +set_record_call_history_size (char *args, int from_tty, + struct cmd_list_element *c) +{ + validate_positive_integer (&setshow_record_call_history_size_var, + &record_call_history_size); +} + /* Provide a prototype to silence -Wmissing-prototypes. */ extern initialize_file_ftype _initialize_record; @@ -633,19 +683,21 @@ _initialize_record (void) NULL, show_record_debug, &setdebuglist, &showdebuglist); - add_setshow_uinteger_cmd ("instruction-history-size", no_class, - &record_insn_history_size, _("\ + add_setshow_integer_cmd ("instruction-history-size", no_class, + &setshow_record_insn_history_size_var, _("\ Set number of instructions to print in \"record instruction-history\"."), _("\ Show number of instructions to print in \"record instruction-history\"."), - NULL, NULL, NULL, &set_record_cmdlist, - &show_record_cmdlist); + NULL, + set_record_insn_history_size, NULL, + &set_record_cmdlist, &show_record_cmdlist); - add_setshow_uinteger_cmd ("function-call-history-size", no_class, - &record_call_history_size, _("\ + add_setshow_integer_cmd ("function-call-history-size", no_class, + &setshow_record_call_history_size_var, _("\ Set number of function to print in \"record function-call-history\"."), _("\ Show number of functions to print in \"record function-call-history\"."), - NULL, NULL, NULL, &set_record_cmdlist, - &show_record_cmdlist); + NULL, + set_record_call_history_size, NULL, + &set_record_cmdlist, &show_record_cmdlist); c = add_prefix_cmd ("record", class_obscure, cmd_record_start, _("Start recording."), @@ -727,4 +779,8 @@ from the first argument.\n\ The number of functions to print can be defined with \"set record \ function-call-history-size\"."), &record_cmdlist); + + /* Sync command control variables. */ + setshow_record_insn_history_size_var = record_insn_history_size; + setshow_record_call_history_size_var = record_call_history_size; }