PR 7536 - ``set input-radix 1'' changes radix

PR 7536 - ``set input-radix 1'' changes radix

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 513 bytes --]

PR 7536 is about the fact that GDB lets the user set the input radix to
0 or 1, although claims it doesn't:

  (gdb) set input-radix 1
  Nonsense input radix ``decimal 1''; input radix unchanged.
  (gdb) show input-radix 
  Default input radix for entering numbers is 1.
  (gdb) set output-radix 1
  Invalid number "1".
  (gdb)

output-radix has similar problems.  This patch fixes both, and adds a
new test to radix.exp.

Checked in, after making sure there were no regressions
on x86_64-linux.

-- 
Pedro Alves

[-- Attachment #2: pr7536.diff --]
[-- Type: text/x-diff, Size: 5849 bytes --]

2008-12-29  Pedro Alves  <pedro@codesourcery.com>

	PR gdb/7536:
	* valprint.c (input_radix_1): New static global.
	(set_input_radix): Use it instead of "input_radix".
	(set_input_radix_1): Always leave input_radix_1 set to
	input_radix.
	(output_radix_1): New static global.
	(set_output_radix): Use it instead of "output_radix".
	(set_output_radix_1): Always leave output_radix_1 set to
	output_radix.
	(_initialize_valprint): Use "input_radix_1" instead of
	"input_radix" with the "input-radix" command.  Use
	"output_radix_1" instead of "output_radix" with the "output-radix"
	command.

2008-12-29  Pedro Alves  <pedro@codesourcery.com>

	PR gdb/7536:
	* gdb.base/radix.exp: Add tests to ensure invalid input radices
	and unsupported output radices are really rejected.

---
 gdb/testsuite/gdb.base/radix.exp |   28 ++++++++++++++++++++++++++++
 gdb/valprint.c                   |   32 ++++++++++++++++++++++----------
 2 files changed, 50 insertions(+), 10 deletions(-)

Index: src/gdb/valprint.c
===================================================================
--- src.orig/gdb/valprint.c	2008-12-24 16:09:13.000000000 +0000
+++ src/gdb/valprint.c	2008-12-29 01:49:51.000000000 +0000
@@ -1359,6 +1359,12 @@ val_print_string (CORE_ADDR addr, int le
 }
 \f
 
+/* The 'set input-radix' command writes to this auxiliary variable.
+   If the requested radix is valid, INPUT_RADIX is updated; otherwise,
+   it is left unchanged.  */
+
+static unsigned input_radix_1 = 10;
+
 /* Validate an input or output radix setting, and make sure the user
    knows what they really did here.  Radix setting is confusing, e.g.
    setting the input radix to "10" never changes it!  */
@@ -1366,7 +1372,7 @@ val_print_string (CORE_ADDR addr, int le
 static void
 set_input_radix (char *args, int from_tty, struct cmd_list_element *c)
 {
-  set_input_radix_1 (from_tty, input_radix);
+  set_input_radix_1 (from_tty, input_radix_1);
 }
 
 static void
@@ -1381,12 +1387,11 @@ set_input_radix_1 (int from_tty, unsigne
 
   if (radix < 2)
     {
-      /* FIXME: cagney/2002-03-17: This needs to revert the bad radix
-         value.  */
+      input_radix_1 = input_radix;
       error (_("Nonsense input radix ``decimal %u''; input radix unchanged."),
 	     radix);
     }
-  input_radix = radix;
+  input_radix_1 = input_radix = radix;
   if (from_tty)
     {
       printf_filtered (_("Input radix now set to decimal %u, hex %x, octal %o.\n"),
@@ -1394,10 +1399,16 @@ set_input_radix_1 (int from_tty, unsigne
     }
 }
 
+/* The 'set output-radix' command writes to this auxiliary variable.
+   If the requested radix is valid, OUTPUT_RADIX is updated,
+   otherwise, it is left unchanged.  */
+
+static unsigned output_radix_1 = 10;
+
 static void
 set_output_radix (char *args, int from_tty, struct cmd_list_element *c)
 {
-  set_output_radix_1 (from_tty, output_radix);
+  set_output_radix_1 (from_tty, output_radix_1);
 }
 
 static void
@@ -1417,12 +1428,11 @@ set_output_radix_1 (int from_tty, unsign
       user_print_options.output_format = 'o';	/* octal */
       break;
     default:
-      /* FIXME: cagney/2002-03-17: This needs to revert the bad radix
-         value.  */
+      output_radix_1 = output_radix;
       error (_("Unsupported output radix ``decimal %u''; output radix unchanged."),
 	     radix);
     }
-  output_radix = radix;
+  output_radix_1 = output_radix = radix;
   if (from_tty)
     {
       printf_filtered (_("Output radix now set to decimal %u, hex %x, octal %o.\n"),
@@ -1566,14 +1576,16 @@ Show printing of addresses."), NULL,
 			   show_addressprint,
 			   &setprintlist, &showprintlist);
 
-  add_setshow_uinteger_cmd ("input-radix", class_support, &input_radix, _("\
+  add_setshow_uinteger_cmd ("input-radix", class_support, &input_radix_1,
+			    _("\
 Set default input radix for entering numbers."), _("\
 Show default input radix for entering numbers."), NULL,
 			    set_input_radix,
 			    show_input_radix,
 			    &setlist, &showlist);
 
-  add_setshow_uinteger_cmd ("output-radix", class_support, &output_radix, _("\
+  add_setshow_uinteger_cmd ("output-radix", class_support, &output_radix_1,
+			    _("\
 Set default output radix for printing of values."), _("\
 Show default output radix for printing of values."), NULL,
 			    set_output_radix,
Index: src/gdb/testsuite/gdb.base/radix.exp
===================================================================
--- src.orig/gdb/testsuite/gdb.base/radix.exp	2008-12-29 00:47:46.000000000 +0000
+++ src/gdb/testsuite/gdb.base/radix.exp	2008-12-29 02:14:59.000000000 +0000
@@ -154,3 +154,31 @@ test_output_radix 16 "10" "20"
     setup_kfail *-*-* "gdb/1715"
     test_one_output 16 "20."        "14"
     test_one_output 16 "(int) 20."  "14"
+
+# Test rejecting invalid input radices and unsupported output radices
+# really rejects the radices, instead of just claiming so (PR 7536).
+
+gdb_test "set radix" \
+    "Input and output radices now set to decimal 10, hex a, octal 12\." \
+    "Reset radices"
+
+gdb_test "set input-radix 1" \
+    "Nonsense input radix ``decimal 1''; input radix unchanged\\." \
+    "Reject input-radix 1"
+gdb_test "show input-radix" \
+    "Default input radix for entering numbers is 10\\." \
+    "Input radix unchanged after rejection"
+
+gdb_test "set output-radix 1" \
+    "Unsupported output radix ``decimal 1''; output radix unchanged\\." \
+    "Reject output-radix 1"
+gdb_test "show output-radix" \
+    "Default output radix for printing of values is 10\\." \
+    "Output radix unchanged after rejection"
+
+gdb_test "set radix 7" \
+    "Unsupported output radix ``decimal 7''; output radix unchanged\\." \
+    "set radix 7 rejected"
+gdb_test "show output-radix" \
+    "Default output radix for printing of values is 10\\." \
+    "Output radix unchanged after rejection through set radix command"

Re: PR 7536 - ``set input-radix 1'' changes radix

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Pedro> PR 7536 is about the fact that GDB lets the user set the input
Pedro> radix to 0 or 1, although claims it doesn't:

Pedro> Checked in, after making sure there were no regressions
Pedro> on x86_64-linux.

Thanks.

I also looked at this bug a little, and I was wondering whether we
should fix this generically, for all parameters, by saving the
parameter's value and then restoring it if the setter function raises
an error.  What do you think of that?

Tom

Re: PR 7536 - ``set input-radix 1'' changes radix

[Pedro Alves]

On Monday 29 December 2008 19:11:17, Tom Tromey wrote:

> I also looked at this bug a little, and I was wondering whether we
> should fix this generically, for all parameters, by saving the
> parameter's value and then restoring it if the setter function raises
> an error.  What do you think of that?

That's actually something that crosses my mind everytime I
touch these set/validate functions.

There are a couple of corner/tricky case that you would have to consider
like QUIT (pagination makes that easy-ish to trigger), and the fact that
it can happen before or after the setter having already done
side effects; or, an error not meaning the setting didn't take effect.
A codebase audit would probably be required to make sure side effects
are unwound correctly, but it's probably doable.  Another option would
be to pass to the setter the new alreayd parsed value, and let it validate
and set the final variable itself --- the setter code always knows what
it it setting.  The issue is that you either pass a variant, or have
a different setter function type for each type of variable.

-- 
Pedro Alves

Re: PR 7536 - ``set input-radix 1'' changes radix

[Joel Brobecker]

> I also looked at this bug a little, and I was wondering whether we
> should fix this generically, for all parameters, by saving the
> parameter's value and then restoring it if the setter function raises
> an error.  What do you think of that?

It crossed my mind many times too. I didn't realize the various issues
that Pedro raised, but it still seems doable.

-- 
Joel