"set record instruction-history" ?

"set record instruction-history" ?

[Pedro Alves]

Hi Markus,

I'm going through all "uinteger" commands in the tree, and
finding several cases of commands that actually shouldn't
be "uinteger", but "zuinteger".

"record instruction-history"'s implementation looks odd enough that
I don't understand what's going on.  The docs don't mention anything
about 0 being special or meaning "unlimited", but I take it that's
the intent?

What's the intent of:

  /* 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?

One can't set this option to negative values:

  (gdb) set record instruction-history-size -2
  integer -2 out of range

The fact that it maps all negatives to INT_MAX is odd,
and needing to set the option to high-enough unsigned 32-bit
integers that trigger that bit of code looks quite
non-user-friendly, even if it didn't always map to INT_MAX:

  (gdb) set record instruction-history-size 0xffffff00
  (gdb) show record instruction-history-size
  Number of instructions to print in "record instruction-history" is 4294967040.

(and exposes 32-bitness to the user, that I'd rather not)

-- 
Pedro Alves

RE: "set record instruction-history" ?

[Metzger, Markus T]

> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com]
> Sent: Monday, March 25, 2013 4:58 PM
> To: GDB Patches; Metzger, Markus T
> Subject: "set record instruction-history" ?
> 
> Hi Markus,
> 
> I'm going through all "uinteger" commands in the tree, and
> finding several cases of commands that actually shouldn't
> be "uinteger", but "zuinteger".
> 
> "record instruction-history"'s implementation looks odd enough that
> I don't understand what's going on.  The docs don't mention anything
> about 0 being special or meaning "unlimited", but I take it that's
> the intent?
> 
> What's the intent of:
> 
>   /* 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.

I do not expect the trace to be near as big as INT_MAX so INT_MAX and anything
bigger than INT_MAX essentially means unlimited.


> One can't set this option to negative values:
> 
>   (gdb) set record instruction-history-size -2
>   integer -2 out of range
> 
> The fact that it maps all negatives to INT_MAX is odd,
> and needing to set the option to high-enough unsigned 32-bit
> integers that trigger that bit of code looks quite
> non-user-friendly, even if it didn't always map to INT_MAX:
> 
>   (gdb) set record instruction-history-size 0xffffff00
>   (gdb) show record instruction-history-size
>   Number of instructions to print in "record instruction-history" is 4294967040.
> 
> (and exposes 32-bitness to the user, that I'd rather not)

Given that the trace can't get bigger than INT_MAX, the user would
still get what he asked for - all the trace.  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?

Regards,
Markus.
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052

Re: "set record instruction-history" ?

[Pedro Alves]

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

RE: "set record instruction-history" ?

[Metzger, Markus T]

> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com]
> Sent: Monday, March 25, 2013 6:46 PM
> To: Metzger, Markus T


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

Shouldn't we map 0 to INT_MAX before committing the new value?


The functionality should be covered by the gdb.btrace test suite.  Do you want
to commit your patch or do you want me to test and then commit it?

Regards,
Markus.
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052

Re: "set record instruction-history" ?

[Pedro Alves]

On 03/26/2013 07:46 AM, Metzger, Markus T wrote:

> Shouldn't we map 0 to INT_MAX before committing the new value?

That's already done for us before this function is reached, by the
set/show machinery.  "commit" here refers to copying the set/show
command's control variable value to the real setting variable
(record_(call|insn)_history_size).

> The functionality should be covered by the gdb.btrace test suite.  Do you want
> to commit your patch or do you want me to test and then commit it?

Could you test and report back please?  I believe my laptop's cpu is
one of those where branch tracing is borked and ended up disabled.

...
(gdb) record btrace
Target does not support branch tracing.
(gdb) testcase ../../../src/gdb/testsuite/gdb.btrace/enable.exp completed in 1 seconds
Running ../../../src/gdb/testsuite/gdb.btrace/function_call_history.exp ...
testcase ../../../src/gdb/testsuite/gdb.btrace/function_call_history.exp completed in 0 seconds
Running ../../../src/gdb/testsuite/gdb.btrace/instruction_history.exp ...
testcase ../../../src/gdb/testsuite/gdb.btrace/instruction_history.exp completed in 0 seconds

                === gdb Summary ===

Thanks,
-- 
Pedro Alves

RE: "set record instruction-history" ?

[Metzger, Markus T]

> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com]
> Sent: Tuesday, March 26, 2013 12:16 PM


> > The functionality should be covered by the gdb.btrace test suite.  Do you want
> > to commit your patch or do you want me to test and then commit it?
> 
> Could you test and report back please?  I believe my laptop's cpu is
> one of those where branch tracing is borked and ended up disabled.

No regressions.

Regards,
Markus.
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052

Re: "set record instruction-history" ?

[Pedro Alves]

On 03/26/2013 11:50 AM, Metzger, Markus T wrote:
>> -----Original Message-----
>> From: Pedro Alves [mailto:palves@redhat.com]
>> Sent: Tuesday, March 26, 2013 12:16 PM
> 
> 
>>> The functionality should be covered by the gdb.btrace test suite.  Do you want
>>> to commit your patch or do you want me to test and then commit it?
>>
>> Could you test and report back please?  I believe my laptop's cpu is
>> one of those where branch tracing is borked and ended up disabled.
> 
> No regressions.

Thanks!  Taking into account the push back in making the
set history size command signed, I've adjusted the patch
to keep this command signed as well and checked it in.
I tried the different numbers that should be affected manually
and I believe it to be an equivalent patch from what you tested.
Let me know, if something broke.

----------------
Subject: [PATCH] "set record instruction-history-size"/"set record function-call-history-size" range validation.

While the commands are uinteger, the target interfaces are limited to
INT_MAX.  Don't let the user request more than we can handle.

gdb/
2013-03-26  Pedro Alves  <palves@redhat.com>

	* record.c (record_insn_history_size_setshow_var)
	(record_call_history_size_setshow_var): New globals.
	(command_size_to_target_size): New function.
	(cmd_record_insn_history, cmd_record_call_history): Use
	command_size_to_target_size instead of cast.
	(validate_history_size, set_record_insn_history_size)
	(set_record_call_history_size): New functions.
	(_initialize_record): Install set_record_insn_history_size and
	set_record_call_history_size as "set" hooks of "set record
	instruction-history-size" and "set record
	function-call-history-size".
---
 gdb/record.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 87 insertions(+), 16 deletions(-)

diff --git a/gdb/record.c b/gdb/record.c
index 6bc1704..b64f3bc 100644
--- a/gdb/record.c
+++ b/gdb/record.c
@@ -35,9 +35,18 @@ unsigned int record_debug = 0;
 /* The number of instructions to print in "record instruction-history".  */
 static unsigned int record_insn_history_size = 10;

+/* The variable registered as control variable in the "record
+   instruction-history" command.  Necessary for extra input
+   validation.  */
+static unsigned int record_insn_history_size_setshow_var;
+
 /* The number of functions to print in "record function-call-history".  */
 static unsigned int record_call_history_size = 10;

+/* The variable registered as control variable in the "record
+   call-history" command.  Necessary for extra input validation.  */
+static unsigned int record_call_history_size_setshow_var;
+
 struct cmd_list_element *record_cmdlist = NULL;
 struct cmd_list_element *set_record_cmdlist = NULL;
 struct cmd_list_element *show_record_cmdlist = NULL;
@@ -428,6 +437,25 @@ get_insn_history_modifiers (char **arg)
   return modifiers;
 }

+/* The "set record instruction-history-size / set record
+   function-call-history-size" commands are unsigned, with UINT_MAX
+   meaning unlimited.  The target interfaces works with signed int
+   though, to indicate direction, so map "unlimited" to INT_MAX, which
+   is about the same as unlimited in practice.  If the user does have
+   a log that huge, she can can fetch it in chunks across several
+   requests, but she'll likely have other problems first...  */
+
+static int
+command_size_to_target_size (unsigned int *command)
+{
+  gdb_assert (*command <= INT_MAX || *command == UINT_MAX);
+
+  if (record_call_history_size == UINT_MAX)
+    return INT_MAX;
+  else
+    return *command;
+}
+
 /* The "record instruction-history" command.  */

 static void
@@ -439,11 +467,7 @@ 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;
+  size = command_size_to_target_size (&record_insn_history_size);

   if (arg == NULL || *arg == 0 || strcmp (arg, "+") == 0)
     target_insn_history (size, flags);
@@ -559,11 +583,7 @@ 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;
+  size = command_size_to_target_size (&record_call_history_size);

   if (arg == NULL || *arg == 0 || strcmp (arg, "+") == 0)
     target_call_history (size, flags);
@@ -617,6 +637,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_history_size (unsigned int *command_var, int *setting)
+{
+  if (*command_var != UINT_MAX && *command_var > INT_MAX)
+    {
+      unsigned int new_value = *command_var;
+
+      /* Restore previous value.  */
+      *command_var = *setting;
+      error (_("integer %u out of range"), new_value);
+    }
+
+  /* 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
+   [0..UINT_MAX].  See command_size_to_target_size.  */
+
+static void
+set_record_insn_history_size (char *args, int from_tty,
+			      struct cmd_list_element *c)
+{
+  validate_history_size (&record_insn_history_size_setshow_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
+   [0..UINT_MAX].  See command_size_to_target_size.  */
+
+static void
+set_record_call_history_size (char *args, int from_tty,
+			      struct cmd_list_element *c)
+{
+  validate_history_size (&record_call_history_size_setshow_var,
+			 &record_call_history_size);
+}
+
 /* Provide a prototype to silence -Wmissing-prototypes.  */
 extern initialize_file_ftype _initialize_record;

@@ -634,18 +699,20 @@ _initialize_record (void)
 			     &showdebuglist);

   add_setshow_uinteger_cmd ("instruction-history-size", no_class,
-			    &record_insn_history_size, _("\
+			    &record_insn_history_size_setshow_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, _("\
+			    &record_call_history_size_setshow_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 +794,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.  */
+  record_insn_history_size_setshow_var = record_insn_history_size;
+  record_call_history_size_setshow_var = record_call_history_size;
 }
-- 
1.7.11.7

RE: "set record instruction-history" ?

[Metzger, Markus T]

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-owner@sourceware.org] On Behalf Of Pedro Alves
> Sent: Tuesday, March 26, 2013 7:07 PM
> To: Metzger, Markus T


> Thanks!  Taking into account the push back in making the
> set history size command signed, I've adjusted the patch
> to keep this command signed as well and checked it in.
> I tried the different numbers that should be affected manually
> and I believe it to be an equivalent patch from what you tested.
> Let me know, if something broke.

There is a single regression...

[...]

> +static int
> +command_size_to_target_size (unsigned int *command)
> +{
> +  gdb_assert (*command <= INT_MAX || *command == UINT_MAX);
> +
> +  if (record_call_history_size == UINT_MAX)

...here. We should have used *command in the comparison.

I also changed the parameter type to "unsigned int" since we're not
updating our argument.

    record: fix instruction-history-size regression
    
    (gdb) PASS: gdb.btrace/instruction_history.exp: set record instruction-history-size 0
    record instruction-history 0
    Bad range.
    (gdb) FAIL: gdb.btrace/instruction_history.exp: record instruction-history - unlimited
    
    2013-03-13  Markus Metzger  <markus.t.metzger@intel.com>
    
    	* record.c (command_size_to_target_size): Fix size comparison.
    	Change parameter type from pointer to integer to integer.
    	Update all users.
    
diff --git a/gdb/record.c b/gdb/record.c
index b64f3bc..2736a1e 100644
--- a/gdb/record.c
+++ b/gdb/record.c
@@ -442,18 +442,18 @@ get_insn_history_modifiers (char **arg)
    meaning unlimited.  The target interfaces works with signed int
    though, to indicate direction, so map "unlimited" to INT_MAX, which
    is about the same as unlimited in practice.  If the user does have
-   a log that huge, she can can fetch it in chunks across several
-   requests, but she'll likely have other problems first...  */
+   a log that huge, she can fetch it in chunks across several requests,
+   but she'll likely have other problems first...  */
 
 static int
-command_size_to_target_size (unsigned int *command)
+command_size_to_target_size (unsigned int size)
 {
-  gdb_assert (*command <= INT_MAX || *command == UINT_MAX);
+  gdb_assert (size <= INT_MAX || size == UINT_MAX);
 
-  if (record_call_history_size == UINT_MAX)
+  if (size == UINT_MAX)
     return INT_MAX;
   else
-    return *command;
+    return size;
 }
 
 /* The "record instruction-history" command.  */
@@ -467,7 +467,7 @@ cmd_record_insn_history (char *arg, int from_tty)
 
   flags = get_insn_history_modifiers (&arg);
 
-  size = command_size_to_target_size (&record_insn_history_size);
+  size = command_size_to_target_size (record_insn_history_size);
 
   if (arg == NULL || *arg == 0 || strcmp (arg, "+") == 0)
     target_insn_history (size, flags);
@@ -583,7 +583,7 @@ cmd_record_call_history (char *arg, int from_tty)
 
   flags = get_call_history_modifiers (&arg);
 
-  size = command_size_to_target_size (&record_call_history_size);
+  size = command_size_to_target_size (record_call_history_size);
 
   if (arg == NULL || *arg == 0 || strcmp (arg, "+") == 0)
     target_call_history (size, flags);


OK to commit?  Does this also go onto the 7.6 branch?

Thanks,
Markus.
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052

Re: "set record instruction-history" ?

[Pedro Alves]

On 03/27/2013 07:53 AM, Metzger, Markus T wrote:

> There is a single regression...

:-(

>     record: fix instruction-history-size regression
>     
>     (gdb) PASS: gdb.btrace/instruction_history.exp: set record instruction-history-size 0
>     record instruction-history 0
>     Bad range.
>     (gdb) FAIL: gdb.btrace/instruction_history.exp: record instruction-history - unlimited
>     
>     2013-03-13  Markus Metzger  <markus.t.metzger@intel.com>
>     
>     	* record.c (command_size_to_target_size): Fix size comparison.
>     	Change parameter type from pointer to integer to integer.
>     	Update all users.

> OK to commit? 

Certainly.  Thanks for fixing this up.

> Does this also go onto the 7.6 branch?

Mine's not on 7.6 yet.  I'll take care of that.

I'm going through all commands (and almost done at that) and will
look over all changes and merge back to 7.6 those that make sense.

-- 
Pedro Alves