Re: "set record instruction-history" ?

[Pedro Alves <palves@redhat.com>]

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  <palves@redhat.com>

	* 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;
 }