[PATCH 1/2] gdb: Restore a settings previous value on error during change

[PATCH 1/2] gdb: Restore a settings previous value on error during change

[Andrew Burgess]

When changing the value of a setting in do_set_command, there are three
phases:

 1. The old value is destroyed, and the new value is placed into the
 settings variable.

 2. The set-hook, the 'func' field of the setting is called.

 3. A change notification is issued.

There are two problems that I try to address in this commit.

 1. If the set-hook does not like the new value then either the setting
 is restored to a default value, or we have to have a complex system
 within the set-hook to remember the previous value and restore it
 manually when an invalid change is made.

 2. If the set-hook throws an error then the change notification is
 skipped, even though the setting might still have changed (say, back to
 some default value).

The solution I propose here is to delay destroying the old value of the
setting until after the set-hook has been called.  If the set-hook runs
successfully then the old value will be destroyed when do_set_command
returns, the new value will be left in place, and the change
notification will be sent sent out exactly as before.

However, if the set-hook throws an error this is now caught in
do_set_command, the new value of the setting is removed, and the old
value is restored.

After this change, it is no longer possible in a set-hook to change a
setting to a default value _and_ throw an error.  If you throw an error
you should assume that gdb will restore the previous value of the
setting.  If you want to change a setting in the set-hook and inform the
user, then a warning, or just a standard message should be fine.

gdb/ChangeLog:

	* cli/cli-setshow.c: Add exceptions.h include.
	(do_set_comment): When installing a settings new value, don't
	loose the previous value until the set-hook has been called.  If
	the set-hook throws an error, restore the settings previous value.

gdb/testsuite/ChangeLog:

	* gdb.base/dcache-line-set-error.exp: New file.
---
 gdb/ChangeLog                                    |  7 +++
 gdb/cli/cli-setshow.c                            | 80 ++++++++++++++++++++++--
 gdb/testsuite/ChangeLog                          |  4 ++
 gdb/testsuite/gdb.base/dcache-line-set-error.exp | 67 ++++++++++++++++++++
 4 files changed, 153 insertions(+), 5 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/dcache-line-set-error.exp

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 7fc2b4a..3cb115f 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,10 @@
+2016-11-11  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* cli/cli-setshow.c: Add exceptions.h include.
+	(do_set_comment): When installing a settings new value, don't
+	loose the previous value until the set-hook has been called.  If
+	the set-hook throws an error, restore the settings previous value.
+
 2016-11-11  Yao Qi  <yao.qi@linaro.org>
 
 	* cp-valprint.c (cp_print_value): Remove local base_valaddr.
diff --git a/gdb/cli/cli-setshow.c b/gdb/cli/cli-setshow.c
index d2ec1df..bcfc2ff 100644
--- a/gdb/cli/cli-setshow.c
+++ b/gdb/cli/cli-setshow.c
@@ -21,6 +21,7 @@
 #include <ctype.h>
 #include "arch-utils.h"
 #include "observer.h"
+#include "exceptions.h"
 
 #include "ui-out.h"
 
@@ -151,8 +152,20 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 {
   /* A flag to indicate the option is changed or not.  */
   int option_changed = 0;
+  /* Cleanups created to free the previous value of the setting.  */
+  struct cleanup *cleanups;
+  /* The previous value of the setting.  Restored if the set-hook function
+     throws an error.  */
+  union
+  {
+    enum auto_boolean b;
+    int i;
+    const char *s;
+    unsigned int u;
+  } prev_value;
 
   gdb_assert (c->type == set_cmd);
+  cleanups = make_cleanup (null_cleanup, NULL);
 
   switch (c->var_type)
     {
@@ -200,7 +213,8 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 	if (*(char **) c->var == NULL
 	    || strcmp (*(char **) c->var, newobj) != 0)
 	  {
-	    xfree (*(char **) c->var);
+	    prev_value.s = *(const char **) c->var;
+	    make_cleanup (xfree, *(char **) c->var);
 	    *(char **) c->var = newobj;
 
 	    option_changed = 1;
@@ -215,7 +229,8 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 
       if (*(char **) c->var == NULL || strcmp (*(char **) c->var, arg) != 0)
 	{
-	  xfree (*(char **) c->var);
+	  prev_value.s = *(const char **) c->var;
+	  make_cleanup (xfree, *(char **) c->var);
 	  *(char **) c->var = xstrdup (arg);
 
 	  option_changed = 1;
@@ -248,7 +263,8 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 	if (*(char **) c->var == NULL
 	    || strcmp (*(char **) c->var, val) != 0)
 	  {
-	    xfree (*(char **) c->var);
+	    prev_value.s = *(const char **) c->var;
+	    make_cleanup (xfree, *(char **) c->var);
 	    *(char **) c->var = val;
 
 	    option_changed = 1;
@@ -265,6 +281,7 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 	  error (_("\"on\" or \"off\" expected."));
 	if (val != *(int *) c->var)
 	  {
+	    prev_value.i = *(int *) c->var;
 	    *(int *) c->var = val;
 
 	    option_changed = 1;
@@ -277,6 +294,7 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 
 	if (*(enum auto_boolean *) c->var != val)
 	  {
+	    prev_value.b = *(enum auto_boolean *) c->var;
 	    *(enum auto_boolean *) c->var = val;
 
 	    option_changed = 1;
@@ -313,6 +331,7 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 
 	if (*(unsigned int *) c->var != val)
 	  {
+	    prev_value.u = *(unsigned int *) c->var;
 	    *(unsigned int *) c->var = val;
 
 	    option_changed = 1;
@@ -349,12 +368,13 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 
 	if (*(int *) c->var != val)
 	  {
+	    prev_value.i = *(int *) c->var;
 	    *(int *) c->var = val;
 
 	    option_changed = 1;
 	  }
-	break;
       }
+      break;
     case var_enum:
       {
 	int i;
@@ -419,6 +439,7 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 
 	if (*(const char **) c->var != match)
 	  {
+	    prev_value.s = *(const char **) c->var;
 	    *(const char **) c->var = match;
 
 	    option_changed = 1;
@@ -444,6 +465,7 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 
 	if (*(int *) c->var != val)
 	  {
+	    prev_value.i = *(int *) c->var;
 	    *(int *) c->var = val;
 	    option_changed = 1;
 	  }
@@ -452,7 +474,51 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
     default:
       error (_("gdb internal error: bad var_type in do_setshow_command"));
     }
-  c->func (c, NULL, from_tty);
+
+  TRY
+    {
+      c->func (c, NULL, from_tty);
+    }
+  CATCH (ex, RETURN_MASK_ERROR)
+    {
+      /* The only cleanups in place free the previous value, which we're
+	 about to restore.  */
+      discard_cleanups (cleanups);
+      /* Now restore the previous value, and free the new value.  */
+      if (option_changed)
+	switch (c->var_type)
+	  {
+	  case var_string:
+	  case var_string_noescape:
+	  case var_filename:
+	  case var_optional_filename:
+	    xfree (*(char **) c->var);
+	    *(char **) c->var = (char *) prev_value.s;
+	    break;
+
+	  case var_boolean:
+	  case var_integer:
+	  case var_zinteger:
+	  case var_zuinteger_unlimited:
+	    *(int *) c->var = prev_value.i;
+	    break;
+
+	  case var_auto_boolean:
+	    *(enum auto_boolean *) c->var = prev_value.b;
+	    break;
+
+	  case var_uinteger:
+	  case var_zuinteger:
+	    *(unsigned int *) c->var = prev_value.u;
+	    break;
+
+	  case var_enum:
+	    *(char **) c->var = (char *) prev_value.s;
+	    break;
+	  }
+      throw_exception (ex);
+    }
+  END_CATCH
 
   if (notify_command_param_changed_p (option_changed, c))
     {
@@ -493,6 +559,7 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 	  xfree (cmds);
 	  xfree (name);
 
+	  do_cleanups (cleanups);
 	  return;
 	}
       /* Traverse them in the reversed order, and copy their names into
@@ -557,6 +624,9 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 	}
       xfree (name);
     }
+
+  do_cleanups (cleanups);
+  return;
 }
 
 /* Do a "show" command.  ARG is NULL if no argument, or the
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index b2fc137..01f737e 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,7 @@
+2016-11-11  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* gdb.base/dcache-line-set-error.exp: New file.
+
 2016-11-09  Pedro Alves  <palves@redhat.com>
 
 	* gdb.base/commands.exp (runto_or_return): New procedure.
diff --git a/gdb/testsuite/gdb.base/dcache-line-set-error.exp b/gdb/testsuite/gdb.base/dcache-line-set-error.exp
new file mode 100644
index 0000000..976e07e
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dcache-line-set-error.exp
@@ -0,0 +1,67 @@
+# Test error handling when seting dcache line size
+# Copyright 2011-2016 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+
+# Read the current dcache line-size, check default is still 64.
+set default_line_size 0
+set testname "read default value for dcache line-size"
+send_gdb "show dcache line-size\n"
+gdb_expect {
+    -re "Dcache line size is (\[0-9\]+\)\.\r\n$gdb_prompt $" {
+	set default_line_size $expect_out(1,string)
+	pass $testname
+    }
+    -re ".*$gdb_prompt $"   { fail $testname }
+    timeout	            { fail "$testname (timeout)" }
+}
+
+if { $default_line_size == 0 } then {
+    unsupported "dcache-line-set-error"
+    return
+}
+
+# Try to change to some invalid (non power of 2 value)
+set invalid_line_size [expr $default_line_size - 1]
+gdb_test "set dcache line-size $invalid_line_size" \
+    "Invalid dcache line size: $invalid_line_size .*" \
+    "First attempt to set invalid dcache line size"
+
+# Check the default is still 64
+gdb_test "show dcache line-size" \
+    "Dcache line size is $default_line_size\." \
+    "Check that the default value is still in place"
+
+# Change the line size to some other valid value, and check the value
+# changed.
+set new_line_size [expr $default_line_size * 2]
+gdb_test_no_output "set dcache line-size $new_line_size" \
+    "Set dcache line size to a new value"
+gdb_test "show dcache line-size" \
+    "Dcache line size is $new_line_size\." \
+    "Check that the new value took"
+
+# Try to change to some invalid (non power of 2 value)
+gdb_test "set dcache line-size $invalid_line_size" \
+    "Invalid dcache line size: $invalid_line_size .*" \
+    "Second attempt to set invalid dcache line size"
+
+# Check that the new value is still in place.
+gdb_test "show dcache line-size" \
+    "Dcache line size is $new_line_size\." \
+    "Check that the new value is still in place"
-- 
2.5.1

[PATCH 0/2] Auto roll back on error while changing a setting

[Andrew Burgess]

When we create a user setting within GDB there's an oportunity to
register a callback function that will be called when the value of the
setting is changed.

The callback function can do many things, for example triggering
aditional work as a result of the change in setting, but it can also
error check the new value of the setting.

If the callback is being used to error check the setting then we have
two basic approaches:

  1. If the new value is wrong, install a suitable default and then
  throw an error, or

  2. Maintain a local copy of the setting, then if the new value is
  wrong we can restore the previous value.  If the new vaule is
  acceptable, then we update the local cached copy, after which we
  throw an error.

There are two problems with the above, they are:

  1. Raising an error in a set callback will cause GDB to skip sending
  out a change notification for the setting.  This change notification
  is sent out from do_set_command, but will be skipped if an error is
  thrown.

  2. We end up duplicating the rollback approach, or with inconsistent
  behaviour on errors (some rollback, some install a default).

In this patch series I propose moving the rollback approach up into
do_set_command, and adding a TRY / CATCH to do_set_command.  Now, if
any set callback throws an error then the setting will automatically
be rolled back to the previous value (and no change notification will
be sent out as the value did not change).  This allows us to simplify
a handful of existing set callbacks to remove rollback and default
setting code.

Tested on x86-64 Fedora Linux with no regressions.

---

Andrew Burgess (2):
  gdb: Restore a settings previous value on error during change
  gdb: Simplify variable set hooks

 gdb/ChangeLog                                    | 32 ++++++++++
 gdb/cli/cli-setshow.c                            | 80 ++++++++++++++++++++++--
 gdb/dcache.c                                     | 12 +---
 gdb/record.c                                     | 41 +++---------
 gdb/symtab.c                                     | 23 ++-----
 gdb/testsuite/ChangeLog                          |  8 +++
 gdb/testsuite/gdb.base/dcache-line-set-error.exp | 67 ++++++++++++++++++++
 gdb/testsuite/gdb.base/max-value-size.exp        |  4 +-
 gdb/valprint.c                                   | 31 +++------
 gdb/value.c                                      |  6 +-
 10 files changed, 209 insertions(+), 95 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/dcache-line-set-error.exp

-- 
2.5.1

[PATCH 2/2] gdb: Simplify variable set hooks

[Andrew Burgess]

Now the the variable set-hook mechanism supports automatic rollback of
the variable value if the set-hook throws an error, simplify existing
cases where we manually performed roll-back within the set-hook.

gdb/ChangeLog:

	* dcache.c (set_dcache_size): Don't change value on error path.
	(set_dcache_line_size): Likewise.
	* record.c (record_insn_history_size_setshow_var): Remove.
	(record_call_history_size_setshow_var): Remove.
	(validate_history_size): Simplify to just the error check.
	(set_record_insn_history_size): Update call to
	validate_history_size.
	(set_record_call_history_size): Likewise.
	(_initialize_record): Remove use of *_setshow_var.
	* symtab.c (new_symbol_cache_size): Remove.
	(symbol_cache_size): Update comment.
	(set_symbol_cache_size_handler): Simplify error check.
	(_initialize_symtab): Remove use of new_symbol_cache_size.
	* valprint.c (input_radix_1): Remove.
	(set_input_radix): Remove use of input_radix_1.
	(set_input_radix_1): Likewise.
	(output_radix_1): Remove.
	(set_output_radix): Remove use of output_radix_1.
	(set_output_radix_1): Likewise.
	(_initialize_valprint): Remove use of output_radix_1 and
	input_radix_1.
	* value.c (set_max_value_size): Simplify error case.

gdb/testsuite/ChangeLog:

	* gdb.base/max-value-size.exp: Update expected output.
---
 gdb/ChangeLog                             | 25 +++++++++++++++++++
 gdb/dcache.c                              | 12 +++------
 gdb/record.c                              | 41 ++++++-------------------------
 gdb/symtab.c                              | 23 ++++-------------
 gdb/testsuite/ChangeLog                   |  4 +++
 gdb/testsuite/gdb.base/max-value-size.exp |  4 +--
 gdb/valprint.c                            | 31 ++++++-----------------
 gdb/value.c                               |  6 +----
 8 files changed, 56 insertions(+), 90 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 3cb115f..d142b75 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,30 @@
 2016-11-11  Andrew Burgess  <andrew.burgess@embecosm.com>
 
+	* dcache.c (set_dcache_size): Don't change value on error path.
+	(set_dcache_line_size): Likewise.
+	* record.c (record_insn_history_size_setshow_var): Remove.
+	(record_call_history_size_setshow_var): Remove.
+	(validate_history_size): Simplify to just the error check.
+	(set_record_insn_history_size): Update call to
+	validate_history_size.
+	(set_record_call_history_size): Likewise.
+	(_initialize_record): Remove use of *_setshow_var.
+	* symtab.c (new_symbol_cache_size): Remove.
+	(symbol_cache_size): Update comment.
+	(set_symbol_cache_size_handler): Simplify error check.
+	(_initialize_symtab): Remove use of new_symbol_cache_size.
+	* valprint.c (input_radix_1): Remove.
+	(set_input_radix): Remove use of input_radix_1.
+	(set_input_radix_1): Likewise.
+	(output_radix_1): Remove.
+	(set_output_radix): Remove use of output_radix_1.
+	(set_output_radix_1): Likewise.
+	(_initialize_valprint): Remove use of output_radix_1 and
+	input_radix_1.
+	* value.c (set_max_value_size): Simplify error case.
+
+2016-11-11  Andrew Burgess  <andrew.burgess@embecosm.com>
+
 	* cli/cli-setshow.c: Add exceptions.h include.
 	(do_set_comment): When installing a settings new value, don't
 	loose the previous value until the set-hook has been called.  If
diff --git a/gdb/dcache.c b/gdb/dcache.c
index cb43068..053b1de 100644
--- a/gdb/dcache.c
+++ b/gdb/dcache.c
@@ -652,10 +652,7 @@ set_dcache_size (char *args, int from_tty,
 		 struct cmd_list_element *c)
 {
   if (dcache_size == 0)
-    {
-      dcache_size = DCACHE_DEFAULT_SIZE;
-      error (_("Dcache size must be greater than 0."));
-    }
+    error (_("Dcache size must be greater than 0."));
   target_dcache_invalidate ();
 }
 
@@ -665,11 +662,8 @@ set_dcache_line_size (char *args, int from_tty,
 {
   if (dcache_line_size < 2
       || (dcache_line_size & (dcache_line_size - 1)) != 0)
-    {
-      unsigned d = dcache_line_size;
-      dcache_line_size = DCACHE_DEFAULT_LINE_SIZE;
-      error (_("Invalid dcache line size: %u (must be power of 2)."), d);
-    }
+    error (_("Invalid dcache line size: %u (must be power of 2)."),
+	   dcache_line_size);
   target_dcache_invalidate ();
 }
 
diff --git a/gdb/record.c b/gdb/record.c
index 34ebd1b..6b0811b 100644
--- a/gdb/record.c
+++ b/gdb/record.c
@@ -35,18 +35,9 @@ 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 *record_goto_cmdlist = NULL;
 struct cmd_list_element *set_record_cmdlist = NULL;
@@ -690,23 +681,13 @@ 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.  */
+   variable registered in the command as control variable.  */
 
 static void
-validate_history_size (unsigned int *command_var, unsigned int *setting)
+validate_history_size (unsigned int command_var)
 {
-  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;
+  if (command_var != UINT_MAX && command_var > INT_MAX)
+    error (_("integer %u out of range"), command_var);
 }
 
 /* Called by do_setshow_command.  We only want values in the
@@ -717,8 +698,7 @@ 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);
+  validate_history_size (record_insn_history_size);
 }
 
 /* Called by do_setshow_command.  We only want values in the
@@ -729,8 +709,7 @@ 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);
+  validate_history_size (record_call_history_size);
 }
 
 /* Provide a prototype to silence -Wmissing-prototypes.  */
@@ -750,7 +729,7 @@ _initialize_record (void)
 			     &showdebuglist);
 
   add_setshow_uinteger_cmd ("instruction-history-size", no_class,
-			    &record_insn_history_size_setshow_var, _("\
+			    &record_insn_history_size, _("\
 Set number of instructions to print in \"record instruction-history\"."), _("\
 Show number of instructions to print in \"record instruction-history\"."), _("\
 A size of \"unlimited\" means unlimited instructions.  The default is 10."),
@@ -758,7 +737,7 @@ A size of \"unlimited\" means unlimited instructions.  The default is 10."),
 			    &set_record_cmdlist, &show_record_cmdlist);
 
   add_setshow_uinteger_cmd ("function-call-history-size", no_class,
-			    &record_call_history_size_setshow_var, _("\
+			    &record_call_history_size, _("\
 Set number of function to print in \"record function-call-history\"."), _("\
 Show number of functions to print in \"record function-call-history\"."), _("\
 A size of \"unlimited\" means unlimited lines.  The default is 10."),
@@ -855,8 +834,4 @@ 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;
 }
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 430bc8d..b5352fc 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -212,12 +212,7 @@ unsigned int symtab_create_debug = 0;
 /* When non-zero, print debugging messages related to symbol lookup.  */
 unsigned int symbol_lookup_debug = 0;
 
-/* The size of the cache is staged here.  */
-static unsigned int new_symbol_cache_size = DEFAULT_SYMBOL_CACHE_SIZE;
-
-/* The current value of the symbol cache size.
-   This is saved so that if the user enters a value too big we can restore
-   the original value from here.  */
+/* The current value of the symbol cache size.  */
 static unsigned int symbol_cache_size = DEFAULT_SYMBOL_CACHE_SIZE;
 
 /* Non-zero if a file may be known by two different basenames.
@@ -1285,17 +1280,9 @@ static void
 set_symbol_cache_size_handler (char *args, int from_tty,
 			       struct cmd_list_element *c)
 {
-  if (new_symbol_cache_size > MAX_SYMBOL_CACHE_SIZE)
-    {
-      /* Restore the previous value.
-	 This is the value the "show" command prints.  */
-      new_symbol_cache_size = symbol_cache_size;
-
-      error (_("Symbol cache size is too large, max is %u."),
-	     MAX_SYMBOL_CACHE_SIZE);
-    }
-  symbol_cache_size = new_symbol_cache_size;
-
+  if (symbol_cache_size > MAX_SYMBOL_CACHE_SIZE)
+    error (_("Symbol cache size is too large, max is %u."),
+	   MAX_SYMBOL_CACHE_SIZE);
   set_symbol_cache_size (symbol_cache_size);
 }
 
@@ -6222,7 +6209,7 @@ When enabled (non-zero), symbol lookups are logged."),
 			   &setdebuglist, &showdebuglist);
 
   add_setshow_zuinteger_cmd ("symbol-cache-size", no_class,
-			     &new_symbol_cache_size,
+			     &symbol_cache_size,
 			     _("Set the size of the symbol cache."),
 			     _("Show the size of the symbol cache."), _("\
 The size of the symbol cache.\n\
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 01f737e..14bf74d 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,5 +1,9 @@
 2016-11-11  Andrew Burgess  <andrew.burgess@embecosm.com>
 
+	* gdb.base/max-value-size.exp: Update expected output.
+
+2016-11-11  Andrew Burgess  <andrew.burgess@embecosm.com>
+
 	* gdb.base/dcache-line-set-error.exp: New file.
 
 2016-11-09  Pedro Alves  <palves@redhat.com>
diff --git a/gdb/testsuite/gdb.base/max-value-size.exp b/gdb/testsuite/gdb.base/max-value-size.exp
index 63a97a0..e179f2f 100644
--- a/gdb/testsuite/gdb.base/max-value-size.exp
+++ b/gdb/testsuite/gdb.base/max-value-size.exp
@@ -90,8 +90,8 @@ set_and_check_max_value_size "unlimited"
 
 # Check that we can't set the maximum size stupidly low.
 gdb_test "set max-value-size 1" \
-    "max-value-size set too low, increasing to \[0-9\]+ bytes"
+    "max-value-size 1 bytes is too low"
 gdb_test "set max-value-size 0" \
-    "max-value-size set too low, increasing to \[0-9\]+ bytes"
+    "max-value-size 0 bytes is too low"
 gdb_test "set max-value-size -5" \
     "only -1 is allowed to set as unlimited"
diff --git a/gdb/valprint.c b/gdb/valprint.c
index c0cdb34..9f8b233 100644
--- a/gdb/valprint.c
+++ b/gdb/valprint.c
@@ -2955,11 +2955,6 @@ val_print_string (struct type *elttype, const char *encoding,
 }
 \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.
@@ -2968,7 +2963,7 @@ static unsigned input_radix_1 = 10;
 static void
 set_input_radix (char *args, int from_tty, struct cmd_list_element *c)
 {
-  set_input_radix_1 (from_tty, input_radix_1);
+  set_input_radix_1 (from_tty, input_radix);
 }
 
 static void
@@ -2982,12 +2977,9 @@ set_input_radix_1 (int from_tty, unsigned radix)
      (FIXME).  */
 
   if (radix < 2)
-    {
-      input_radix_1 = input_radix;
-      error (_("Nonsense input radix ``decimal %u''; input radix unchanged."),
-	     radix);
-    }
-  input_radix_1 = input_radix = radix;
+    error (_("Nonsense input radix ``decimal %u''; input radix unchanged."),
+	   radix);
+  input_radix = radix;
   if (from_tty)
     {
       printf_filtered (_("Input radix now set to "
@@ -2996,16 +2988,10 @@ set_input_radix_1 (int from_tty, unsigned radix)
     }
 }
 
-/* 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_1);
+  set_output_radix_1 (from_tty, output_radix);
 }
 
 static void
@@ -3025,12 +3011,11 @@ set_output_radix_1 (int from_tty, unsigned radix)
       user_print_options.output_format = 'o';	/* octal */
       break;
     default:
-      output_radix_1 = output_radix;
       error (_("Unsupported output radix ``decimal %u''; "
 	       "output radix unchanged."),
 	     radix);
     }
-  output_radix_1 = output_radix = radix;
+  output_radix = radix;
   if (from_tty)
     {
       printf_filtered (_("Output radix now set to "
@@ -3208,7 +3193,7 @@ Show printing of symbol names when printing pointers."),
 			   show_symbol_print,
 			   &setprintlist, &showprintlist);
 
-  add_setshow_zuinteger_cmd ("input-radix", class_support, &input_radix_1,
+  add_setshow_zuinteger_cmd ("input-radix", class_support, &input_radix,
 			     _("\
 Set default input radix for entering numbers."), _("\
 Show default input radix for entering numbers."), NULL,
@@ -3216,7 +3201,7 @@ Show default input radix for entering numbers."), NULL,
 			     show_input_radix,
 			     &setlist, &showlist);
 
-  add_setshow_zuinteger_cmd ("output-radix", class_support, &output_radix_1,
+  add_setshow_zuinteger_cmd ("output-radix", class_support, &output_radix,
 			     _("\
 Set default output radix for printing of values."), _("\
 Show default output radix for printing of values."), NULL,
diff --git a/gdb/value.c b/gdb/value.c
index 62c5e37..11ce828 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -987,11 +987,7 @@ set_max_value_size (char *args, int from_tty,
   gdb_assert (max_value_size == -1 || max_value_size >= 0);
 
   if (max_value_size > -1 && max_value_size < MIN_VALUE_FOR_MAX_VALUE_SIZE)
-    {
-      max_value_size = MIN_VALUE_FOR_MAX_VALUE_SIZE;
-      error (_("max-value-size set too low, increasing to %d bytes"),
-	     max_value_size);
-    }
+    error (_("max-value-size %d bytes is too low"), max_value_size);
 }
 
 /* Implement the "show max-value-size" command.  */
-- 
2.5.1

Re: [PATCH 1/2] gdb: Restore a settings previous value on error during change

[Luis Machado]

On 11/11/2016 12:18 PM, Andrew Burgess wrote:
> When changing the value of a setting in do_set_command, there are three
> phases:
>
>  1. The old value is destroyed, and the new value is placed into the
>  settings variable.
>
>  2. The set-hook, the 'func' field of the setting is called.
>
>  3. A change notification is issued.
>
> There are two problems that I try to address in this commit.
>
>  1. If the set-hook does not like the new value then either the setting
>  is restored to a default value, or we have to have a complex system
>  within the set-hook to remember the previous value and restore it
>  manually when an invalid change is made.
>
>  2. If the set-hook throws an error then the change notification is
>  skipped, even though the setting might still have changed (say, back to
>  some default value).
>
> The solution I propose here is to delay destroying the old value of the
> setting until after the set-hook has been called.  If the set-hook runs
> successfully then the old value will be destroyed when do_set_command
> returns, the new value will be left in place, and the change
> notification will be sent sent out exactly as before.
>
> However, if the set-hook throws an error this is now caught in
> do_set_command, the new value of the setting is removed, and the old
> value is restored.
>
> After this change, it is no longer possible in a set-hook to change a
> setting to a default value _and_ throw an error.  If you throw an error
> you should assume that gdb will restore the previous value of the
> setting.  If you want to change a setting in the set-hook and inform the
> user, then a warning, or just a standard message should be fine.
>
> gdb/ChangeLog:
>
> 	* cli/cli-setshow.c: Add exceptions.h include.
> 	(do_set_comment): When installing a settings new value, don't
> 	loose the previous value until the set-hook has been called.  If
> 	the set-hook throws an error, restore the settings previous value.
>
> gdb/testsuite/ChangeLog:
>
> 	* gdb.base/dcache-line-set-error.exp: New file.
> ---
>  gdb/ChangeLog                                    |  7 +++
>  gdb/cli/cli-setshow.c                            | 80 ++++++++++++++++++++++--
>  gdb/testsuite/ChangeLog                          |  4 ++
>  gdb/testsuite/gdb.base/dcache-line-set-error.exp | 67 ++++++++++++++++++++
>  4 files changed, 153 insertions(+), 5 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.base/dcache-line-set-error.exp
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 7fc2b4a..3cb115f 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,10 @@
> +2016-11-11  Andrew Burgess  <andrew.burgess@embecosm.com>
> +
> +	* cli/cli-setshow.c: Add exceptions.h include.
> +	(do_set_comment): When installing a settings new value, don't
> +	loose the previous value until the set-hook has been called.  If
> +	the set-hook throws an error, restore the settings previous value.
> +

"a setting's new ..."
"don't lose ..."
"restore the setting's..."

> @@ -151,8 +152,20 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
>  {
>    /* A flag to indicate the option is changed or not.  */
>    int option_changed = 0;
> +  /* Cleanups created to free the previous value of the setting.  */
> +  struct cleanup *cleanups;
> +  /* The previous value of the setting.  Restored if the set-hook function
> +     throws an error.  */
> +  union
> +  {
> +    enum auto_boolean b;
> +    int i;
> +    const char *s;
> +    unsigned int u;
> +  } prev_value;

I wonder if we can share and potentially simplify the storage mechanism 
for these settings with what struct cmd_list_element already has? As 
opposed to declaring something new just for this particular function?

Since we're already touching this function, it may be worthwhile to get 
it cleaned up a bit if that's not too much effort.

I also see a lot of repetition trying to save the old value. If we could 
get it saved at the function's entry, that would save many lines of 
code, but i think this is a nice-to-have and not required for this patch.

>
>    gdb_assert (c->type == set_cmd);
> +  cleanups = make_cleanup (null_cleanup, NULL);
>
>    switch (c->var_type)
>      {
> @@ -200,7 +213,8 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
>  	if (*(char **) c->var == NULL
>  	    || strcmp (*(char **) c->var, newobj) != 0)
>  	  {
> -	    xfree (*(char **) c->var);
> +	    prev_value.s = *(const char **) c->var;
> +	    make_cleanup (xfree, *(char **) c->var);
>  	    *(char **) c->var = newobj;

For example, this type of procedure of saving the old value repeats 
itself over and...
>
>  	    option_changed = 1;
> @@ -215,7 +229,8 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
>
>        if (*(char **) c->var == NULL || strcmp (*(char **) c->var, arg) != 0)
>  	{
> -	  xfree (*(char **) c->var);
> +	  prev_value.s = *(const char **) c->var;
> +	  make_cleanup (xfree, *(char **) c->var);
>  	  *(char **) c->var = xstrdup (arg);
>

... over and ...
>  	  option_changed = 1;
> @@ -248,7 +263,8 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
>  	if (*(char **) c->var == NULL
>  	    || strcmp (*(char **) c->var, val) != 0)
>  	  {
> -	    xfree (*(char **) c->var);
> +	    prev_value.s = *(const char **) c->var;
> +	    make_cleanup (xfree, *(char **) c->var);
>  	    *(char **) c->var = val;
>

... over again.

> @@ -452,7 +474,51 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
>      default:
>        error (_("gdb internal error: bad var_type in do_setshow_command"));
>      }
> -  c->func (c, NULL, from_tty);
> +
> +  TRY
> +    {
> +      c->func (c, NULL, from_tty);
> +    }
> +  CATCH (ex, RETURN_MASK_ERROR)
> +    {
> +      /* The only cleanups in place free the previous value, which we're
> +	 about to restore.  */

Maybe change this comment a bit to be more to the point?

"Discard the cleanups since we failed to set the new value."

> +      discard_cleanups (cleanups);
> +      /* Now restore the previous value, and free the new value.  */
> +      if (option_changed)
> +	switch (c->var_type)
> +	  {
> +	  case var_string:
> +	  case var_string_noescape:
> +	  case var_filename:
> +	  case var_optional_filename:
> +	    xfree (*(char **) c->var);
> +	    *(char **) c->var = (char *) prev_value.s;
> +	    break;
> +
> +	  case var_boolean:
> +	  case var_integer:
> +	  case var_zinteger:
> +	  case var_zuinteger_unlimited:
> +	    *(int *) c->var = prev_value.i;
> +	    break;
> +
> +	  case var_auto_boolean:
> +	    *(enum auto_boolean *) c->var = prev_value.b;
> +	    break;
> +
> +	  case var_uinteger:
> +	  case var_zuinteger:
> +	    *(unsigned int *) c->var = prev_value.u;
> +	    break;
> +
> +	  case var_enum:
> +	    *(char **) c->var = (char *) prev_value.s;
> +	    break;
> +	  }
> +      throw_exception (ex);
> +    }
> +  END_CATCH

Can we make a cleanup that just restores some old value regardless of 
its type so we can call it once without having to check what kind of 
variable we're dealing with (since we would have checked that before 
getting here already and before putting such cleanup in place?).

It sounds like we could use a couple cleanups? One to free the old value 
due to having a new valid value in place and another to restore the old 
value in case the new value is invalid?

Not sure how much work that adds, but sounds like it would make this 
code a bit more clean?

In any case, just a suggestion.

>  /* Do a "show" command.  ARG is NULL if no argument, or the
> diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
> index b2fc137..01f737e 100644
> --- a/gdb/testsuite/ChangeLog
> +++ b/gdb/testsuite/ChangeLog
> @@ -1,3 +1,7 @@
> +2016-11-11  Andrew Burgess  <andrew.burgess@embecosm.com>
> +
> +	* gdb.base/dcache-line-set-error.exp: New file.
> +
>  2016-11-09  Pedro Alves  <palves@redhat.com>
>
>  	* gdb.base/commands.exp (runto_or_return): New procedure.
> diff --git a/gdb/testsuite/gdb.base/dcache-line-set-error.exp b/gdb/testsuite/gdb.base/dcache-line-set-error.exp
> new file mode 100644
> index 0000000..976e07e
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/dcache-line-set-error.exp
> @@ -0,0 +1,67 @@
> +# Test error handling when seting dcache line size
> +# Copyright 2011-2016 Free Software Foundation, Inc.

Is this based off some other test? If not then 2016 only.

> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +gdb_exit
> +gdb_start
> +gdb_reinitialize_dir $srcdir/$subdir
> +
> +# Read the current dcache line-size, check default is still 64.
> +set default_line_size 0
> +set testname "read default value for dcache line-size"
> +send_gdb "show dcache line-size\n"
> +gdb_expect {
> +    -re "Dcache line size is (\[0-9\]+\)\.\r\n$gdb_prompt $" {
> +	set default_line_size $expect_out(1,string)
> +	pass $testname
> +    }
> +    -re ".*$gdb_prompt $"   { fail $testname }
> +    timeout	            { fail "$testname (timeout)" }
> +}
> +
> +if { $default_line_size == 0 } then {
> +    unsupported "dcache-line-set-error"

We should probably change the testname to something more meaningful. 
Maybe ...

"dcache not support for this target"?

> +    return
> +}
> +
> +# Try to change to some invalid (non power of 2 value)
> +set invalid_line_size [expr $default_line_size - 1]
> +gdb_test "set dcache line-size $invalid_line_size" \
> +    "Invalid dcache line size: $invalid_line_size .*" \
> +    "First attempt to set invalid dcache line size"
> +
> +# Check the default is still 64
> +gdb_test "show dcache line-size" \
> +    "Dcache line size is $default_line_size\." \
> +    "Check that the default value is still in place"
> +
> +# Change the line size to some other valid value, and check the value
> +# changed.
> +set new_line_size [expr $default_line_size * 2]
> +gdb_test_no_output "set dcache line-size $new_line_size" \
> +    "Set dcache line size to a new value"
> +gdb_test "show dcache line-size" \
> +    "Dcache line size is $new_line_size\." \
> +    "Check that the new value took"
> +
> +# Try to change to some invalid (non power of 2 value)
> +gdb_test "set dcache line-size $invalid_line_size" \
> +    "Invalid dcache line size: $invalid_line_size .*" \
> +    "Second attempt to set invalid dcache line size"
> +
> +# Check that the new value is still in place.

s/new value/old value?

> +gdb_test "show dcache line-size" \
> +    "Dcache line size is $new_line_size\." \
> +    "Check that the new value is still in place"

Same here.

Re: [PATCH 2/2] gdb: Simplify variable set hooks

[Luis Machado]

On 11/11/2016 12:18 PM, Andrew Burgess wrote:
> Now the the variable set-hook mechanism supports automatic rollback of
> the variable value if the set-hook throws an error, simplify existing
> cases where we manually performed roll-back within the set-hook.
>
> gdb/ChangeLog:
>
> 	* dcache.c (set_dcache_size): Don't change value on error path.
> 	(set_dcache_line_size): Likewise.
> 	* record.c (record_insn_history_size_setshow_var): Remove.
> 	(record_call_history_size_setshow_var): Remove.
> 	(validate_history_size): Simplify to just the error check.
> 	(set_record_insn_history_size): Update call to
> 	validate_history_size.
> 	(set_record_call_history_size): Likewise.
> 	(_initialize_record): Remove use of *_setshow_var.
> 	* symtab.c (new_symbol_cache_size): Remove.
> 	(symbol_cache_size): Update comment.
> 	(set_symbol_cache_size_handler): Simplify error check.
> 	(_initialize_symtab): Remove use of new_symbol_cache_size.
> 	* valprint.c (input_radix_1): Remove.
> 	(set_input_radix): Remove use of input_radix_1.
> 	(set_input_radix_1): Likewise.
> 	(output_radix_1): Remove.
> 	(set_output_radix): Remove use of output_radix_1.
> 	(set_output_radix_1): Likewise.
> 	(_initialize_valprint): Remove use of output_radix_1 and
> 	input_radix_1.
> 	* value.c (set_max_value_size): Simplify error case.
>
> gdb/testsuite/ChangeLog:
>
> 	* gdb.base/max-value-size.exp: Update expected output.
> ---
>  gdb/ChangeLog                             | 25 +++++++++++++++++++
>  gdb/dcache.c                              | 12 +++------
>  gdb/record.c                              | 41 ++++++-------------------------
>  gdb/symtab.c                              | 23 ++++-------------
>  gdb/testsuite/ChangeLog                   |  4 +++
>  gdb/testsuite/gdb.base/max-value-size.exp |  4 +--
>  gdb/valprint.c                            | 31 ++++++-----------------
>  gdb/value.c                               |  6 +----
>  8 files changed, 56 insertions(+), 90 deletions(-)

I have no comments on this patch. Looks like a nice cleanup of 
convoluted code.

RE: [PATCH 2/2] gdb: Simplify variable set hooks

[Metzger, Markus T]

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Andrew Burgess
> Sent: Friday, November 11, 2016 7:19 PM
> To: gdb-patches@sourceware.org
> Cc: Andrew Burgess <andrew.burgess@embecosm.com>
> Subject: [PATCH 2/2] gdb: Simplify variable set hooks

Hi Andrew,

> Now the the variable set-hook mechanism supports automatic rollback of
> the variable value if the set-hook throws an error, simplify existing
> cases where we manually performed roll-back within the set-hook.
> 
> gdb/ChangeLog:
> 
> 	* dcache.c (set_dcache_size): Don't change value on error path.
> 	(set_dcache_line_size): Likewise.
> 	* record.c (record_insn_history_size_setshow_var): Remove.
> 	(record_call_history_size_setshow_var): Remove.
> 	(validate_history_size): Simplify to just the error check.
> 	(set_record_insn_history_size): Update call to
> 	validate_history_size.
> 	(set_record_call_history_size): Likewise.
> 	(_initialize_record): Remove use of *_setshow_var.
> 	* symtab.c (new_symbol_cache_size): Remove.
> 	(symbol_cache_size): Update comment.
> 	(set_symbol_cache_size_handler): Simplify error check.
> 	(_initialize_symtab): Remove use of new_symbol_cache_size.
> 	* valprint.c (input_radix_1): Remove.
> 	(set_input_radix): Remove use of input_radix_1.
> 	(set_input_radix_1): Likewise.
> 	(output_radix_1): Remove.
> 	(set_output_radix): Remove use of output_radix_1.
> 	(set_output_radix_1): Likewise.
> 	(_initialize_valprint): Remove use of output_radix_1 and
> 	input_radix_1.
> 	* value.c (set_max_value_size): Simplify error case.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.base/max-value-size.exp: Update expected output.
> ---
>  gdb/ChangeLog                             | 25 +++++++++++++++++++
>  gdb/dcache.c                              | 12 +++------
>  gdb/record.c                              | 41 ++++++-------------------------
>  gdb/symtab.c                              | 23 ++++-------------
>  gdb/testsuite/ChangeLog                   |  4 +++
>  gdb/testsuite/gdb.base/max-value-size.exp |  4 +--
>  gdb/valprint.c                            | 31 ++++++-----------------
>  gdb/value.c                               |  6 +----
>  8 files changed, 56 insertions(+), 90 deletions(-)

The record changes look good to me.

Regarding the parent patch, if we passed the to-be-set value to the
set function in addition to the pointer to the set/show variable, we
could leave the actual assignment to the set function.  We wouldn't
need to take care of restoring the previous value in the caller since
we have not updated it, yet.  This might be simpler.

Regards,
Markus.

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH 1/2] gdb: Restore a settings previous value on error during change

[Andrew Burgess]

* Luis Machado <lgustavo@codesourcery.com> [2016-11-11 18:20:24 -0600]:

> On 11/11/2016 12:18 PM, Andrew Burgess wrote:
> > When changing the value of a setting in do_set_command, there are three
> > phases:
> > 
> >  1. The old value is destroyed, and the new value is placed into the
> >  settings variable.
> > 
> >  2. The set-hook, the 'func' field of the setting is called.
> > 
> >  3. A change notification is issued.
> > 
> > There are two problems that I try to address in this commit.
> > 
> >  1. If the set-hook does not like the new value then either the setting
> >  is restored to a default value, or we have to have a complex system
> >  within the set-hook to remember the previous value and restore it
> >  manually when an invalid change is made.
> > 
> >  2. If the set-hook throws an error then the change notification is
> >  skipped, even though the setting might still have changed (say, back to
> >  some default value).
> > 
> > The solution I propose here is to delay destroying the old value of the
> > setting until after the set-hook has been called.  If the set-hook runs
> > successfully then the old value will be destroyed when do_set_command
> > returns, the new value will be left in place, and the change
> > notification will be sent sent out exactly as before.
> > 
> > However, if the set-hook throws an error this is now caught in
> > do_set_command, the new value of the setting is removed, and the old
> > value is restored.
> > 
> > After this change, it is no longer possible in a set-hook to change a
> > setting to a default value _and_ throw an error.  If you throw an error
> > you should assume that gdb will restore the previous value of the
> > setting.  If you want to change a setting in the set-hook and inform the
> > user, then a warning, or just a standard message should be fine.
> > 
> > gdb/ChangeLog:
> > 
> > 	* cli/cli-setshow.c: Add exceptions.h include.
> > 	(do_set_comment): When installing a settings new value, don't
> > 	loose the previous value until the set-hook has been called.  If
> > 	the set-hook throws an error, restore the settings previous value.
> > 
> > gdb/testsuite/ChangeLog:
> > 
> > 	* gdb.base/dcache-line-set-error.exp: New file.
> > ---
> >  gdb/ChangeLog                                    |  7 +++
> >  gdb/cli/cli-setshow.c                            | 80 ++++++++++++++++++++++--
> >  gdb/testsuite/ChangeLog                          |  4 ++
> >  gdb/testsuite/gdb.base/dcache-line-set-error.exp | 67 ++++++++++++++++++++
> >  4 files changed, 153 insertions(+), 5 deletions(-)
> >  create mode 100644 gdb/testsuite/gdb.base/dcache-line-set-error.exp
> > 
> > diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> > index 7fc2b4a..3cb115f 100644
> > --- a/gdb/ChangeLog
> > +++ b/gdb/ChangeLog
> > @@ -1,3 +1,10 @@
> > +2016-11-11  Andrew Burgess  <andrew.burgess@embecosm.com>
> > +
> > +	* cli/cli-setshow.c: Add exceptions.h include.
> > +	(do_set_comment): When installing a settings new value, don't
> > +	loose the previous value until the set-hook has been called.  If
> > +	the set-hook throws an error, restore the settings previous value.
> > +
> 
> "a setting's new ..."
> "don't lose ..."
> "restore the setting's..."
> 
> > @@ -151,8 +152,20 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
> >  {
> >    /* A flag to indicate the option is changed or not.  */
> >    int option_changed = 0;
> > +  /* Cleanups created to free the previous value of the setting.  */
> > +  struct cleanup *cleanups;
> > +  /* The previous value of the setting.  Restored if the set-hook function
> > +     throws an error.  */
> > +  union
> > +  {
> > +    enum auto_boolean b;
> > +    int i;
> > +    const char *s;
> > +    unsigned int u;
> > +  } prev_value;
> 
> I wonder if we can share and potentially simplify the storage mechanism for
> these settings with what struct cmd_list_element already has? As opposed to
> declaring something new just for this particular function?

I'm not sure how we'd do that.  The 'struct cmd_list_element' holds a
'void *' pointer to a variable of _some type_ the interpretation of
that 'void *' depends on the 'var_type' field within the 'struct
cmd_list_element'.  But we never actually hold the _value_ of the
setting in the 'struct cmd_list_element'.

So I think that we will always end up introducing _some_ new structure
to hold the previous value.  I'm open to suggestions though, just
because I can't see the solution doesn't mean it's not there...

> Since we're already touching this function, it may be worthwhile to get it
> cleaned up a bit if that's not too much effort.

I can do some clean up if you'd like, I can even make it part of this
patch series, though obviously it'll be a new patch in this series.
But you'll need to give some more specific indication of what you'd
like to see changed....

> 
> I also see a lot of repetition trying to save the old value. If we could get
> it saved at the function's entry, that would save many lines of code, but i
> think this is a nice-to-have and not required for this patch.

I'm not sure I'd agree with "many", but in the new version I've tried
to reduce the number of new lines added.  As I discuss above, given
that the 'struct cmd_list_element' only has a 'void *' pointer,
backing up the previous value has to be done on a case by case basis.
Again, though, if you have a specific idea in mind that I'm not
seeing, feel free to make a suggestion, I'm happy to change things.

> 
> > 
> >    gdb_assert (c->type == set_cmd);
> > +  cleanups = make_cleanup (null_cleanup, NULL);
> > 
> >    switch (c->var_type)
> >      {
> > @@ -200,7 +213,8 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
> >  	if (*(char **) c->var == NULL
> >  	    || strcmp (*(char **) c->var, newobj) != 0)
> >  	  {
> > -	    xfree (*(char **) c->var);
> > +	    prev_value.s = *(const char **) c->var;
> > +	    make_cleanup (xfree, *(char **) c->var);
> >  	    *(char **) c->var = newobj;
> 
> For example, this type of procedure of saving the old value repeats itself
> over and...
> > 
> >  	    option_changed = 1;
> > @@ -215,7 +229,8 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
> > 
> >        if (*(char **) c->var == NULL || strcmp (*(char **) c->var, arg) != 0)
> >  	{
> > -	  xfree (*(char **) c->var);
> > +	  prev_value.s = *(const char **) c->var;
> > +	  make_cleanup (xfree, *(char **) c->var);
> >  	  *(char **) c->var = xstrdup (arg);
> > 
> 
> ... over and ...
> >  	  option_changed = 1;
> > @@ -248,7 +263,8 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
> >  	if (*(char **) c->var == NULL
> >  	    || strcmp (*(char **) c->var, val) != 0)
> >  	  {
> > -	    xfree (*(char **) c->var);
> > +	    prev_value.s = *(const char **) c->var;
> > +	    make_cleanup (xfree, *(char **) c->var);
> >  	    *(char **) c->var = val;
> > 
> 
> ... over again.
> 
> > @@ -452,7 +474,51 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
> >      default:
> >        error (_("gdb internal error: bad var_type in do_setshow_command"));
> >      }
> > -  c->func (c, NULL, from_tty);
> > +
> > +  TRY
> > +    {
> > +      c->func (c, NULL, from_tty);
> > +    }
> > +  CATCH (ex, RETURN_MASK_ERROR)
> > +    {
> > +      /* The only cleanups in place free the previous value, which we're
> > +	 about to restore.  */
> 
> Maybe change this comment a bit to be more to the point?
> 
> "Discard the cleanups since we failed to set the new value."
> 
> > +      discard_cleanups (cleanups);
> > +      /* Now restore the previous value, and free the new value.  */
> > +      if (option_changed)
> > +	switch (c->var_type)
> > +	  {
> > +	  case var_string:
> > +	  case var_string_noescape:
> > +	  case var_filename:
> > +	  case var_optional_filename:
> > +	    xfree (*(char **) c->var);
> > +	    *(char **) c->var = (char *) prev_value.s;
> > +	    break;
> > +
> > +	  case var_boolean:
> > +	  case var_integer:
> > +	  case var_zinteger:
> > +	  case var_zuinteger_unlimited:
> > +	    *(int *) c->var = prev_value.i;
> > +	    break;
> > +
> > +	  case var_auto_boolean:
> > +	    *(enum auto_boolean *) c->var = prev_value.b;
> > +	    break;
> > +
> > +	  case var_uinteger:
> > +	  case var_zuinteger:
> > +	    *(unsigned int *) c->var = prev_value.u;
> > +	    break;
> > +
> > +	  case var_enum:
> > +	    *(char **) c->var = (char *) prev_value.s;
> > +	    break;
> > +	  }
> > +      throw_exception (ex);
> > +    }
> > +  END_CATCH
> 
> Can we make a cleanup that just restores some old value regardless of its
> type so we can call it once without having to check what kind of variable
> we're dealing with (since we would have checked that before getting here
> already and before putting such cleanup in place?).
> 
> It sounds like we could use a couple cleanups? One to free the old value due
> to having a new valid value in place and another to restore the old value in
> case the new value is invalid?
> 
> Not sure how much work that adds, but sounds like it would make this code a
> bit more clean?

I've folded the whole restore or clean-up the previous value into a
single cleanup function.  I'm not 100% convinced that the new code is
better than the previous, but I don't think it's any worse.

Let me know what you think of the new version.

Thanks,
Andrew

---

gdb: Restore a settings previous value on error during change

When changing the value of a setting in do_set_command, there are three
phases:

 1. The old value is destroyed, and the new value is placed into the
 settings variable.

 2. The set-hook, the 'func' field of the setting is called.

 3. A change notification is issued.

There are two problems that I try to address in this commit.

 1. If the set-hook does not like the new value then either the setting
 is restored to a default value, or we have to have a complex system
 within the set-hook to remember the previous value and restore it
 manually when an invalid change is made.

 2. If the set-hook throws an error then the change notification is
 skipped, even though the setting might still have changed (say, back to
 some default value).

The solution I propose here is to delay destroying the old value of the
setting until after the set-hook has been called.  If the set-hook runs
successfully then the old value will be destroyed when do_set_command
returns, the new value will be left in place, and the change
notification will be sent sent out exactly as before.

However, if the set-hook throws an error this is now caught in
do_set_command, the new value of the setting is removed, and the old
value is restored.

After this change, it is no longer possible in a set-hook to change a
setting to a default value _and_ throw an error.  If you throw an error
you should assume that gdb will restore the previous value of the
setting.  If you want to change a setting in the set-hook and inform the
user, then a warning, or just a standard message should be fine.

gdb/ChangeLog:

	* cli/cli-setshow.c: Add exceptions.h include.
	(struct set_cmd_cleanup_data): New definition.
	(set_cmd_cleanup_or_revert): New function.
	(do_set_comment): When installing a setting's new value, don't
	lose the previous value until the set-hook has been called.  Use a
	cleanup to either delete or restore the previous value.

gdb/testsuite/ChangeLog:

	* gdb.base/dcache-line-set-error.exp: New file.
---
 gdb/ChangeLog                                    |   9 ++
 gdb/cli/cli-setshow.c                            | 142 ++++++++++++++++++++++-
 gdb/testsuite/ChangeLog                          |   4 +
 gdb/testsuite/gdb.base/dcache-line-set-error.exp |  67 +++++++++++
 4 files changed, 217 insertions(+), 5 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/dcache-line-set-error.exp

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 7fc2b4a..bbfefe8 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,12 @@
+2016-11-11  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* cli/cli-setshow.c: Add exceptions.h include.
+	(struct set_cmd_cleanup_data): New definition.
+	(set_cmd_cleanup_or_revert): New function.
+	(do_set_comment): When installing a setting's new value, don't
+	lose the previous value until the set-hook has been called.  Use a
+	cleanup to either delete or restore the previous value.
+
 2016-11-11  Yao Qi  <yao.qi@linaro.org>
 
 	* cp-valprint.c (cp_print_value): Remove local base_valaddr.
diff --git a/gdb/cli/cli-setshow.c b/gdb/cli/cli-setshow.c
index d2ec1df..b5fd75d 100644
--- a/gdb/cli/cli-setshow.c
+++ b/gdb/cli/cli-setshow.c
@@ -21,6 +21,7 @@
 #include <ctype.h>
 #include "arch-utils.h"
 #include "observer.h"
+#include "exceptions.h"
 
 #include "ui-out.h"
 
@@ -140,6 +141,100 @@ is_unlimited_literal (const char *arg)
 	  && (isspace (arg[len]) || arg[len] == '\0'));
 }
 
+/* Used within DO_SET_COMMAND and SET_CMD_CLEANUP_OR_REVERT.  Holds
+   information required to either cleanup the previous value of a setting,
+   or to restore the previous value.  */
+
+struct set_cmd_cleanup_data
+{
+  /* The setting we're working on.  */
+  struct cmd_list_element *c;
+
+  /* Should we revert the setting to the previous value?  If not then we
+     should clean up the previous value.  */
+  int revert_p;
+
+  /* The previous value.  Which field is used depends on c->var_type.  */
+  union
+  {
+    enum auto_boolean b;
+    int i;
+    const char *s;
+    unsigned int u;
+  } prev_value;
+};
+
+/* Cleanup function called from DO_SET_COMMAND.  Either cleans up the
+   previous value of a setting, freeing any associated memory, or,
+   restores the previous value, freeing the new value of a setting.  */
+
+static void
+set_cmd_cleanup_or_revert (void *arg)
+{
+  struct set_cmd_cleanup_data *set_cmd_cleanup_data
+    = (struct set_cmd_cleanup_data *) arg;
+
+  if (set_cmd_cleanup_data->revert_p)
+    {
+      switch (set_cmd_cleanup_data->c->var_type)
+	{
+	case var_string:
+	case var_string_noescape:
+	case var_filename:
+	case var_optional_filename:
+	  xfree (*(char **) set_cmd_cleanup_data->c->var);
+	  *(char **) set_cmd_cleanup_data->c->var
+	    = (char *) set_cmd_cleanup_data->prev_value.s;
+	  break;
+
+	case var_boolean:
+	case var_integer:
+	case var_zinteger:
+	case var_zuinteger_unlimited:
+	  *(int *) set_cmd_cleanup_data->c->var
+	    = set_cmd_cleanup_data->prev_value.i;
+	  break;
+
+	case var_auto_boolean:
+	  *(enum auto_boolean *) set_cmd_cleanup_data->c->var
+	    = set_cmd_cleanup_data->prev_value.b;
+	  break;
+
+	case var_uinteger:
+	case var_zuinteger:
+	  *(unsigned int *) set_cmd_cleanup_data->c->var
+	    = set_cmd_cleanup_data->prev_value.u;
+	  break;
+
+	case var_enum:
+	  *(char **) set_cmd_cleanup_data->c->var
+	    = (char *) set_cmd_cleanup_data->prev_value.s;
+	  break;
+	}
+    }
+  else
+    {
+      switch (set_cmd_cleanup_data->c->var_type)
+	{
+	case var_string:
+	case var_string_noescape:
+	case var_filename:
+	case var_optional_filename:
+	  xfree ((char *) set_cmd_cleanup_data->prev_value.s);
+	  break;
+
+	case var_boolean:
+	case var_integer:
+	case var_zinteger:
+	case var_zuinteger_unlimited:
+	case var_auto_boolean:
+	case var_uinteger:
+	case var_zuinteger:
+	case var_enum:
+	  break;
+	}
+    }
+}
 
 /* Do a "set" command.  ARG is NULL if no argument, or the
    text of the argument, and FROM_TTY is nonzero if this command is
@@ -151,8 +246,19 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 {
   /* A flag to indicate the option is changed or not.  */
   int option_changed = 0;
+  /* Cleanups created to free the previous value of the setting.  */
+  struct cleanup *cleanups;
+  /* Data used to either clean up the previous value of the setting, or
+     restore the previous value, after we have called c->func.  */
+  struct set_cmd_cleanup_data set_cmd_cleanup_data;
+  set_cmd_cleanup_data.c = c;
+  set_cmd_cleanup_data.revert_p = 0;
+  memset (&set_cmd_cleanup_data.prev_value, 0,
+	  sizeof (set_cmd_cleanup_data.prev_value));
 
   gdb_assert (c->type == set_cmd);
+  cleanups = make_cleanup (set_cmd_cleanup_or_revert,
+			   &set_cmd_cleanup_data);
 
   switch (c->var_type)
     {
@@ -200,7 +306,7 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 	if (*(char **) c->var == NULL
 	    || strcmp (*(char **) c->var, newobj) != 0)
 	  {
-	    xfree (*(char **) c->var);
+	    set_cmd_cleanup_data.prev_value.s = *(const char **) c->var;
 	    *(char **) c->var = newobj;
 
 	    option_changed = 1;
@@ -215,7 +321,7 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 
       if (*(char **) c->var == NULL || strcmp (*(char **) c->var, arg) != 0)
 	{
-	  xfree (*(char **) c->var);
+	  set_cmd_cleanup_data.prev_value.s = *(const char **) c->var;
 	  *(char **) c->var = xstrdup (arg);
 
 	  option_changed = 1;
@@ -248,7 +354,7 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 	if (*(char **) c->var == NULL
 	    || strcmp (*(char **) c->var, val) != 0)
 	  {
-	    xfree (*(char **) c->var);
+	    set_cmd_cleanup_data.prev_value.s = *(const char **) c->var;
 	    *(char **) c->var = val;
 
 	    option_changed = 1;
@@ -265,6 +371,7 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 	  error (_("\"on\" or \"off\" expected."));
 	if (val != *(int *) c->var)
 	  {
+	    set_cmd_cleanup_data.prev_value.i = *(int *) c->var;
 	    *(int *) c->var = val;
 
 	    option_changed = 1;
@@ -277,6 +384,7 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 
 	if (*(enum auto_boolean *) c->var != val)
 	  {
+	    set_cmd_cleanup_data.prev_value.b = *(enum auto_boolean *) c->var;
 	    *(enum auto_boolean *) c->var = val;
 
 	    option_changed = 1;
@@ -313,6 +421,7 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 
 	if (*(unsigned int *) c->var != val)
 	  {
+	    set_cmd_cleanup_data.prev_value.u = *(unsigned int *) c->var;
 	    *(unsigned int *) c->var = val;
 
 	    option_changed = 1;
@@ -349,12 +458,13 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 
 	if (*(int *) c->var != val)
 	  {
+	    set_cmd_cleanup_data.prev_value.i = *(int *) c->var;
 	    *(int *) c->var = val;
 
 	    option_changed = 1;
 	  }
-	break;
       }
+      break;
     case var_enum:
       {
 	int i;
@@ -419,6 +529,7 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 
 	if (*(const char **) c->var != match)
 	  {
+	    set_cmd_cleanup_data.prev_value.s = *(const char **) c->var;
 	    *(const char **) c->var = match;
 
 	    option_changed = 1;
@@ -444,6 +555,7 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 
 	if (*(int *) c->var != val)
 	  {
+	    set_cmd_cleanup_data.prev_value.i = *(int *) c->var;
 	    *(int *) c->var = val;
 	    option_changed = 1;
 	  }
@@ -452,7 +564,24 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
     default:
       error (_("gdb internal error: bad var_type in do_setshow_command"));
     }
-  c->func (c, NULL, from_tty);
+
+  TRY
+    {
+      c->func (c, NULL, from_tty);
+    }
+  CATCH (ex, RETURN_MASK_ERROR)
+    {
+      /* Set the REVERT_P flag based on OPTION_CHANGED.  If this leaves
+	 REVERT_P as false then the cleanup SET_CMD_CLEANUP_OR_REVERT will
+	 try to cleanup the previous value, but as OPTION_CHANGED was
+	 false, there is no previous value, and so nothing is done.  If
+	 REVERT_P is changed to TRUE here then there was a previous value,
+	 and so the SET_CMD_CLEANUP_OR_REVERT will restore the previous
+	 value, and cleanup the new value that we no longer need.  */
+      set_cmd_cleanup_data.revert_p = option_changed;
+      throw_exception (ex);
+    }
+  END_CATCH
 
   if (notify_command_param_changed_p (option_changed, c))
     {
@@ -493,6 +622,7 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 	  xfree (cmds);
 	  xfree (name);
 
+	  do_cleanups (cleanups);
 	  return;
 	}
       /* Traverse them in the reversed order, and copy their names into
@@ -557,6 +687,8 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 	}
       xfree (name);
     }
+
+  do_cleanups (cleanups);
 }
 
 /* Do a "show" command.  ARG is NULL if no argument, or the
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index b2fc137..01f737e 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,7 @@
+2016-11-11  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* gdb.base/dcache-line-set-error.exp: New file.
+
 2016-11-09  Pedro Alves  <palves@redhat.com>
 
 	* gdb.base/commands.exp (runto_or_return): New procedure.
diff --git a/gdb/testsuite/gdb.base/dcache-line-set-error.exp b/gdb/testsuite/gdb.base/dcache-line-set-error.exp
new file mode 100644
index 0000000..976e07e
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dcache-line-set-error.exp
@@ -0,0 +1,67 @@
+# Test error handling when seting dcache line size
+# Copyright 2011-2016 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+
+# Read the current dcache line-size, check default is still 64.
+set default_line_size 0
+set testname "read default value for dcache line-size"
+send_gdb "show dcache line-size\n"
+gdb_expect {
+    -re "Dcache line size is (\[0-9\]+\)\.\r\n$gdb_prompt $" {
+	set default_line_size $expect_out(1,string)
+	pass $testname
+    }
+    -re ".*$gdb_prompt $"   { fail $testname }
+    timeout	            { fail "$testname (timeout)" }
+}
+
+if { $default_line_size == 0 } then {
+    unsupported "dcache-line-set-error"
+    return
+}
+
+# Try to change to some invalid (non power of 2 value)
+set invalid_line_size [expr $default_line_size - 1]
+gdb_test "set dcache line-size $invalid_line_size" \
+    "Invalid dcache line size: $invalid_line_size .*" \
+    "First attempt to set invalid dcache line size"
+
+# Check the default is still 64
+gdb_test "show dcache line-size" \
+    "Dcache line size is $default_line_size\." \
+    "Check that the default value is still in place"
+
+# Change the line size to some other valid value, and check the value
+# changed.
+set new_line_size [expr $default_line_size * 2]
+gdb_test_no_output "set dcache line-size $new_line_size" \
+    "Set dcache line size to a new value"
+gdb_test "show dcache line-size" \
+    "Dcache line size is $new_line_size\." \
+    "Check that the new value took"
+
+# Try to change to some invalid (non power of 2 value)
+gdb_test "set dcache line-size $invalid_line_size" \
+    "Invalid dcache line size: $invalid_line_size .*" \
+    "Second attempt to set invalid dcache line size"
+
+# Check that the new value is still in place.
+gdb_test "show dcache line-size" \
+    "Dcache line size is $new_line_size\." \
+    "Check that the new value is still in place"
-- 
2.5.1

Re: [PATCH 1/2] gdb: Restore a settings previous value on error during change

[Luis Machado]

On 11/15/2016 05:02 PM, Andrew Burgess wrote:
> * Luis Machado <lgustavo@codesourcery.com> [2016-11-11 18:20:24 -0600]:
>>> @@ -151,8 +152,20 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
>>>  {
>>>    /* A flag to indicate the option is changed or not.  */
>>>    int option_changed = 0;
>>> +  /* Cleanups created to free the previous value of the setting.  */
>>> +  struct cleanup *cleanups;
>>> +  /* The previous value of the setting.  Restored if the set-hook function
>>> +     throws an error.  */
>>> +  union
>>> +  {
>>> +    enum auto_boolean b;
>>> +    int i;
>>> +    const char *s;
>>> +    unsigned int u;
>>> +  } prev_value;
>>
>> I wonder if we can share and potentially simplify the storage mechanism for
>> these settings with what struct cmd_list_element already has? As opposed to
>> declaring something new just for this particular function?
>
> I'm not sure how we'd do that.  The 'struct cmd_list_element' holds a
> 'void *' pointer to a variable of _some type_ the interpretation of
> that 'void *' depends on the 'var_type' field within the 'struct
> cmd_list_element'.  But we never actually hold the _value_ of the
> setting in the 'struct cmd_list_element'.
>
> So I think that we will always end up introducing _some_ new structure
> to hold the previous value.  I'm open to suggestions though, just
> because I can't see the solution doesn't mean it's not there...
>

What i had in mind was not to declare values with explicit types like 
the above union but reuse 'enum var_types' with a 'void *' pointer that 
is the value.

Something like:

struct prev_value
{
   void *val;
   enum var_type type;
};

That way we would be able to save the previous value as 'void *' at the 
top of the function regardless of its type and register the cleanup only 
once. Only when we get to the cleanup we decide to free it or not. Then 
we can use the 'enum var_type' information to free whatever needs to be 
freed.

That way we can get rid of all the type-specific assignments, like here:

-	  xfree (*(char **) c->var);
+	  prev_value.s = *(const char **) c->var;
+	  make_cleanup (xfree, *(char **) c->var);
  	  *(char **) c->var = xstrdup (arg);

The above gets repeated a few times along the function.

Hopefully i managed to make it a bit more clear?

>> Since we're already touching this function, it may be worthwhile to get it
>> cleaned up a bit if that's not too much effort.
>
> I can do some clean up if you'd like, I can even make it part of this
> patch series, though obviously it'll be a new patch in this series.
> But you'll need to give some more specific indication of what you'd
> like to see changed....
>
>>
>> I also see a lot of repetition trying to save the old value. If we could get
>> it saved at the function's entry, that would save many lines of code, but i
>> think this is a nice-to-have and not required for this patch.
>
> I'm not sure I'd agree with "many", but in the new version I've tried
> to reduce the number of new lines added.  As I discuss above, given
> that the 'struct cmd_list_element' only has a 'void *' pointer,
> backing up the previous value has to be done on a case by case basis.
> Again, though, if you have a specific idea in mind that I'm not
> seeing, feel free to make a suggestion, I'm happy to change things.
>

These two comments i've made had the same scope as the first. That is, 
to use a 'void *' value plus its type and to save the previous value and 
register the cleanup only once at the top of the function. It was not a 
separate cleanup idea.

In any case, this is just a suggestion.

Re: [PATCH 1/2] gdb: Restore a settings previous value on error during change

[Andrew Burgess]

* Luis Machado <lgustavo@codesourcery.com> [2016-11-16 08:17:50 -0600]:

> On 11/15/2016 05:02 PM, Andrew Burgess wrote:
> > * Luis Machado <lgustavo@codesourcery.com> [2016-11-11 18:20:24 -0600]:
> > > > @@ -151,8 +152,20 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
> > > >  {
> > > >    /* A flag to indicate the option is changed or not.  */
> > > >    int option_changed = 0;
> > > > +  /* Cleanups created to free the previous value of the setting.  */
> > > > +  struct cleanup *cleanups;
> > > > +  /* The previous value of the setting.  Restored if the set-hook function
> > > > +     throws an error.  */
> > > > +  union
> > > > +  {
> > > > +    enum auto_boolean b;
> > > > +    int i;
> > > > +    const char *s;
> > > > +    unsigned int u;
> > > > +  } prev_value;
> > > 
> > > I wonder if we can share and potentially simplify the storage mechanism for
> > > these settings with what struct cmd_list_element already has? As opposed to
> > > declaring something new just for this particular function?
> > 
> > I'm not sure how we'd do that.  The 'struct cmd_list_element' holds a
> > 'void *' pointer to a variable of _some type_ the interpretation of
> > that 'void *' depends on the 'var_type' field within the 'struct
> > cmd_list_element'.  But we never actually hold the _value_ of the
> > setting in the 'struct cmd_list_element'.
> > 
> > So I think that we will always end up introducing _some_ new structure
> > to hold the previous value.  I'm open to suggestions though, just
> > because I can't see the solution doesn't mean it's not there...
> > 
> 
> What i had in mind was not to declare values with explicit types like the
> above union but reuse 'enum var_types' with a 'void *' pointer that is the
> value.
> 
> Something like:
> 
> struct prev_value
> {
>   void *val;
>   enum var_type type;
> };
> 
> That way we would be able to save the previous value as 'void *' at the top
> of the function regardless of its type and register the cleanup only
> once.

I think that you're suggesting saving the _value_ of the setting into
a 'void *', so a choice of an 'enum auto_boolean', an int (signed and
unsigned) or a 'char *'.  I guess at the moment they'll all fit, but
that seems like a fairly ugly bit of code.

Plus we'd still have to have a switch as there's no guarantee that all
those types are the same size, so when we dereference through the
cmd_list_element to get the value we'd have to have different code for
each var_type, otherwise we risk accessing outside the object type.

So we'd end up with:

  struct
  {
    void *val;
    enum var_type type;
  }  prev_value;

  switch (c->var_type)
    {
    case var_string:
    case var_string_noescape:
    case var_filename:
    case var_optional_filename:
    case var_enum:
      prev_value.val = (void *)(*(char **) c->var);
      break;

    case var_boolean:
    case var_integer:
    case var_zinteger:
    case var_zuinteger_unlimited:
      prev_value.val = (void *)(*(int *) c->var);
      break;

    case var_auto_boolean:
      prev_value.val = (void *)(*(enum auto_boolean *) c->var);
      break;

    case var_uinteger:
    case var_zuinteger:
      prev_value.val = (void *)(*(unsigned int *) c->var);
      break;
    }

I guess we could collapse the int/unsigned int cases as we know the
sizes will match, and you'll still need similar code to copy the value
back into place.

I'm not seeing any particular code saving, nor any improvement in code
quality there, and any time we use casts to place one item into
another there's always scope for bugs to creep in....

> Only when we get to the cleanup we decide to free it or not. Then we can use
> the 'enum var_type' information to free whatever needs to be freed.
> 
> That way we can get rid of all the type-specific assignments, like here:
> 
> -	  xfree (*(char **) c->var);
> +	  prev_value.s = *(const char **) c->var;
> +	  make_cleanup (xfree, *(char **) c->var);
>  	  *(char **) c->var = xstrdup (arg);
> 
> The above gets repeated a few times along the function.

The latest iteration removes the make_cleanup call here.  We now just
backup the previous value, so that's save a little code.

> 
> Hopefully i managed to make it a bit more clear?
> 
> > > Since we're already touching this function, it may be worthwhile to get it
> > > cleaned up a bit if that's not too much effort.
> > 
> > I can do some clean up if you'd like, I can even make it part of this
> > patch series, though obviously it'll be a new patch in this series.
> > But you'll need to give some more specific indication of what you'd
> > like to see changed....
> > 
> > > 
> > > I also see a lot of repetition trying to save the old value. If we could get
> > > it saved at the function's entry, that would save many lines of code, but i
> > > think this is a nice-to-have and not required for this patch.
> > 
> > I'm not sure I'd agree with "many", but in the new version I've tried
> > to reduce the number of new lines added.  As I discuss above, given
> > that the 'struct cmd_list_element' only has a 'void *' pointer,
> > backing up the previous value has to be done on a case by case basis.
> > Again, though, if you have a specific idea in mind that I'm not
> > seeing, feel free to make a suggestion, I'm happy to change things.
> > 
> 
> These two comments i've made had the same scope as the first. That is, to
> use a 'void *' value plus its type and to save the previous value and
> register the cleanup only once at the top of the function. It was not a
> separate cleanup idea.
> 
> In any case, this is just a suggestion.

I've fixed the typos and other minor items you pointed out in your
initial review (thank you for that), and the second iteration is
possibly a little cleaner it its use of cleanups.

I don't currently see the benefit / improvement that using a 'void *'
to store the previous value would offer (over using an union), so I
have no plan to rewrite in that direction.

I'm always open to being convinced though, if you feel I'm still
missing the point :)

Thanks,
Andrew

Re: [PATCH 1/2] gdb: Restore a settings previous value on error during change

[Luis Machado]

On 11/16/2016 12:01 PM, Andrew Burgess wrote:
> * Luis Machado <lgustavo@codesourcery.com> [2016-11-16 08:17:50 -0600]:
>
>> On 11/15/2016 05:02 PM, Andrew Burgess wrote:
>>> * Luis Machado <lgustavo@codesourcery.com> [2016-11-11 18:20:24 -0600]:
>>>>> @@ -151,8 +152,20 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
>>>>>  {
>>>>>    /* A flag to indicate the option is changed or not.  */
>>>>>    int option_changed = 0;
>>>>> +  /* Cleanups created to free the previous value of the setting.  */
>>>>> +  struct cleanup *cleanups;
>>>>> +  /* The previous value of the setting.  Restored if the set-hook function
>>>>> +     throws an error.  */
>>>>> +  union
>>>>> +  {
>>>>> +    enum auto_boolean b;
>>>>> +    int i;
>>>>> +    const char *s;
>>>>> +    unsigned int u;
>>>>> +  } prev_value;
>>>>
>>>> I wonder if we can share and potentially simplify the storage mechanism for
>>>> these settings with what struct cmd_list_element already has? As opposed to
>>>> declaring something new just for this particular function?
>>>
>>> I'm not sure how we'd do that.  The 'struct cmd_list_element' holds a
>>> 'void *' pointer to a variable of _some type_ the interpretation of
>>> that 'void *' depends on the 'var_type' field within the 'struct
>>> cmd_list_element'.  But we never actually hold the _value_ of the
>>> setting in the 'struct cmd_list_element'.
>>>
>>> So I think that we will always end up introducing _some_ new structure
>>> to hold the previous value.  I'm open to suggestions though, just
>>> because I can't see the solution doesn't mean it's not there...
>>>
>>
>> What i had in mind was not to declare values with explicit types like the
>> above union but reuse 'enum var_types' with a 'void *' pointer that is the
>> value.
>>
>> Something like:
>>
>> struct prev_value
>> {
>>   void *val;
>>   enum var_type type;
>> };
>>
>> That way we would be able to save the previous value as 'void *' at the top
>> of the function regardless of its type and register the cleanup only
>> once.
>
> I think that you're suggesting saving the _value_ of the setting into
> a 'void *', so a choice of an 'enum auto_boolean', an int (signed and
> unsigned) or a 'char *'.  I guess at the moment they'll all fit, but
> that seems like a fairly ugly bit of code.
>
> Plus we'd still have to have a switch as there's no guarantee that all
> those types are the same size, so when we dereference through the
> cmd_list_element to get the value we'd have to have different code for
> each var_type, otherwise we risk accessing outside the object type.
>
> So we'd end up with:
>
>   struct
>   {
>     void *val;
>     enum var_type type;
>   }  prev_value;
>
>   switch (c->var_type)
>     {
>     case var_string:
>     case var_string_noescape:
>     case var_filename:
>     case var_optional_filename:
>     case var_enum:
>       prev_value.val = (void *)(*(char **) c->var);
>       break;
>
>     case var_boolean:
>     case var_integer:
>     case var_zinteger:
>     case var_zuinteger_unlimited:
>       prev_value.val = (void *)(*(int *) c->var);
>       break;
>
>     case var_auto_boolean:
>       prev_value.val = (void *)(*(enum auto_boolean *) c->var);
>       break;
>
>     case var_uinteger:
>     case var_zuinteger:
>       prev_value.val = (void *)(*(unsigned int *) c->var);
>       break;
>     }
>
> I guess we could collapse the int/unsigned int cases as we know the
> sizes will match, and you'll still need similar code to copy the value
> back into place.
>
> I'm not seeing any particular code saving, nor any improvement in code
> quality there, and any time we use casts to place one item into
> another there's always scope for bugs to creep in....
>
>> Only when we get to the cleanup we decide to free it or not. Then we can use
>> the 'enum var_type' information to free whatever needs to be freed.
>>
>> That way we can get rid of all the type-specific assignments, like here:
>>
>> -	  xfree (*(char **) c->var);
>> +	  prev_value.s = *(const char **) c->var;
>> +	  make_cleanup (xfree, *(char **) c->var);
>>  	  *(char **) c->var = xstrdup (arg);
>>
>> The above gets repeated a few times along the function.
>
> The latest iteration removes the make_cleanup call here.  We now just
> backup the previous value, so that's save a little code.
>
>>
>> Hopefully i managed to make it a bit more clear?
>>
>>>> Since we're already touching this function, it may be worthwhile to get it
>>>> cleaned up a bit if that's not too much effort.
>>>
>>> I can do some clean up if you'd like, I can even make it part of this
>>> patch series, though obviously it'll be a new patch in this series.
>>> But you'll need to give some more specific indication of what you'd
>>> like to see changed....
>>>
>>>>
>>>> I also see a lot of repetition trying to save the old value. If we could get
>>>> it saved at the function's entry, that would save many lines of code, but i
>>>> think this is a nice-to-have and not required for this patch.
>>>
>>> I'm not sure I'd agree with "many", but in the new version I've tried
>>> to reduce the number of new lines added.  As I discuss above, given
>>> that the 'struct cmd_list_element' only has a 'void *' pointer,
>>> backing up the previous value has to be done on a case by case basis.
>>> Again, though, if you have a specific idea in mind that I'm not
>>> seeing, feel free to make a suggestion, I'm happy to change things.
>>>
>>
>> These two comments i've made had the same scope as the first. That is, to
>> use a 'void *' value plus its type and to save the previous value and
>> register the cleanup only once at the top of the function. It was not a
>> separate cleanup idea.
>>
>> In any case, this is just a suggestion.
>
> I've fixed the typos and other minor items you pointed out in your
> initial review (thank you for that), and the second iteration is
> possibly a little cleaner it its use of cleanups.
>
> I don't currently see the benefit / improvement that using a 'void *'
> to store the previous value would offer (over using an union), so I
> have no plan to rewrite in that direction.
>
> I'm always open to being convinced though, if you feel I'm still
> missing the point :)
>
> Thanks,
> Andrew
>

I don't have any further feedback. Hopefully other's will chime in. :-)

Thanks,
Luis

Re: [PATCH 1/2] gdb: Restore a settings previous value on error during change

[Pedro Alves]

Hi Andrew,

On 11/15/2016 11:02 PM, Andrew Burgess wrote:

> I've folded the whole restore or clean-up the previous value into a
> single cleanup function.  I'm not 100% convinced that the new code is
> better than the previous, but I don't think it's any worse.

I'm not sure the "revert if set throws" is the best design, but
I do prefer this version with a single cleanup over the first, because
it should be easier to convert the new cleanup to a RAII C++
class (we're trying to get rid of cleanups).

> 
> gdb: Restore a settings previous value on error during change
> 
> When changing the value of a setting in do_set_command, there are three
> phases:
> 
>  1. The old value is destroyed, and the new value is placed into the
>  settings variable.
> 
>  2. The set-hook, the 'func' field of the setting is called.
> 
>  3. A change notification is issued.
> 
> There are two problems that I try to address in this commit.
> 
>  1. If the set-hook does not like the new value then either the setting
>  is restored to a default value, or we have to have a complex system
>  within the set-hook to remember the previous value and restore it
>  manually when an invalid change is made.
> 
>  2. If the set-hook throws an error then the change notification is
>  skipped, even though the setting might still have changed (say, back to
>  some default value).
> 
> The solution I propose here is to delay destroying the old value of the
> setting until after the set-hook has been called.  If the set-hook runs
> successfully then the old value will be destroyed when do_set_command
> returns, the new value will be left in place, and the change
> notification will be sent sent out exactly as before.
> 
> However, if the set-hook throws an error this is now caught in
> do_set_command, the new value of the setting is removed, and the old
> value is restored.
> 
> After this change, it is no longer possible in a set-hook to change a
> setting to a default value _and_ throw an error.  If you throw an error
> you should assume that gdb will restore the previous value of the
> setting.  If you want to change a setting in the set-hook and inform the
> user, then a warning, or just a standard message should be fine.

I have a few questions on this design.

Take a look at solib.c:gdb_sysroot_changed, the "set sysroot"
set hook.  reload_shared_libraries does a lot of work, and it
can easily trip on some error trying to fetch DSOs out of the
new sysroot.  Should the sysroot setting change back to what it was before,
when such an error happens?  What happens to the shared libraries that might
have already been loaded in the new sysroot?  Should "set" hooks
handle cases like this themselves, by wrapping all but command variable
validation with TRY/CATCH?

I'm also concerned about ctrl-c/QUIT while inside the set hook.
E.g., if the user presses ctrl-c while the set hook is working,
and a QUIT is thrown out of the set hook, should the setting's value
be reverted or not?  Maybe the simplest to imagine is if the set hook
prints something that paginates, and the user presses ctrl-c to
abort the pagination.  Maybe whether to revert depends on whether
the setting's new value was already validated or not?

I'm wondering whether a new "validate" hook, called before
the "set" hook is called, would be a better design.  Maybe not.
I'd like to have the design around those points clarified, so
that we have some agreed direction on how to handle such scenarios.

> +
> +  /* Should we revert the setting to the previous value?  If not then we
> +     should clean up the previous value.  */
> +  int revert_p;

"bool".

> -  c->func (c, NULL, from_tty);
> +
> +  TRY
> +    {
> +      c->func (c, NULL, from_tty);
> +    }
> +  CATCH (ex, RETURN_MASK_ERROR)
> +    {
> +      /* Set the REVERT_P flag based on OPTION_CHANGED.  If this leaves
> +	 REVERT_P as false then the cleanup SET_CMD_CLEANUP_OR_REVERT will
> +	 try to cleanup the previous value, but as OPTION_CHANGED was
> +	 false, there is no previous value, and so nothing is done.  If
> +	 REVERT_P is changed to TRUE here then there was a previous value,
> +	 and so the SET_CMD_CLEANUP_OR_REVERT will restore the previous
> +	 value, and cleanup the new value that we no longer need.  */
> +      set_cmd_cleanup_data.revert_p = option_changed;
> +      throw_exception (ex);
> +    }
> +  END_CATCH

Why wrap with TRY/CATCH instead of:

      set_cmd_cleanup_data.revert_p = option_changed;
      c->func (c, NULL, from_tty);
      set_cmd_cleanup_data.revert_p = false;

Note how ctrl-c inside the "set" hook ends up not reverting,
of RETURN_MASK_ERROR instead of RETURN_MASK_ALL.
If that was the reason for the TRY/CATCH, then I'd expect the
comment to mention it.



> +gdb_exit
> +gdb_start
> +gdb_reinitialize_dir $srcdir/$subdir
> +
> +# Read the current dcache line-size, check default is still 64.
> +set default_line_size 0
> +set testname "read default value for dcache line-size"
> +send_gdb "show dcache line-size\n"
> +gdb_expect {

Use gdb_test_multiple.

> +    -re "Dcache line size is (\[0-9\]+\)\.\r\n$gdb_prompt $" {
> +	set default_line_size $expect_out(1,string)
> +	pass $testname
> +    }
> +    -re ".*$gdb_prompt $"   { fail $testname }
> +    timeout	            { fail "$testname (timeout)" }
> +}
> +
> +if { $default_line_size == 0 } then {
> +    unsupported "dcache-line-set-error"

untested.

> +# Try to change to some invalid (non power of 2 value)

Please write complete sentences in comments.  I.e., add a period.

> +set invalid_line_size [expr $default_line_size - 1]
> +gdb_test "set dcache line-size $invalid_line_size" \
> +    "Invalid dcache line size: $invalid_line_size .*" \
> +    "First attempt to set invalid dcache line size"

Lowercase test messages:

    "first attempt to set invalid dcache line size"

(throughout) 

> +
> +# Check the default is still 64

Period.

> +gdb_test "show dcache line-size" \
> +    "Dcache line size is $new_line_size\." \
> +    "Check that the new value took"

"took effect" ?  Or is that idiomatic English perhaps? (I had never
seen it).

Thanks,
Pedro Alves

Re: [PATCH 2/2] gdb: Simplify variable set hooks

[Pedro Alves]

On 11/11/2016 06:18 PM, Andrew Burgess wrote:
> Now the the variable set-hook mechanism supports automatic rollback of
> the variable value if the set-hook throws an error, simplify existing
> cases where we manually performed roll-back within the set-hook.

Looks good, if we assume we end up agreeing on the previous patch.

Off hand, I recall "set non-stop" and "maint set target-async" as
two other commands that could be simplified.

Thanks,
Pedro Alves

Re: [PATCH 1/2] gdb: Restore a settings previous value on error during change

[Andrew Burgess]

* Pedro Alves <palves@redhat.com> [2016-11-24 23:52:03 +0000]:

> Hi Andrew,

Pedro,

Thanks for reviewing this patch series, and apologies that it's taken
so long for me to look at this again.

> 
> On 11/15/2016 11:02 PM, Andrew Burgess wrote:
> 
> > I've folded the whole restore or clean-up the previous value into a
> > single cleanup function.  I'm not 100% convinced that the new code is
> > better than the previous, but I don't think it's any worse.
> 
> I'm not sure the "revert if set throws" is the best design, but
> I do prefer this version with a single cleanup over the first, because
> it should be easier to convert the new cleanup to a RAII C++
> class (we're trying to get rid of cleanups).

I'll take a look through some recent commits and move to an RAII style
for a final revision of this patch, I might as well get it "right
first time".

As far as the design, goes then hopefully we can find something that
you are happy with.

My motivation is best expressed in the second patch (which you already
looked at) trying to unify all of the roll-back code into one place.
I often find myself (in out of tree ports) adding new settings that
all duplicate the roll-back mechanism.

> 
> > 
> > gdb: Restore a settings previous value on error during change
> > 
> > When changing the value of a setting in do_set_command, there are three
> > phases:
> > 
> >  1. The old value is destroyed, and the new value is placed into the
> >  settings variable.
> > 
> >  2. The set-hook, the 'func' field of the setting is called.
> > 
> >  3. A change notification is issued.
> > 
> > There are two problems that I try to address in this commit.
> > 
> >  1. If the set-hook does not like the new value then either the setting
> >  is restored to a default value, or we have to have a complex system
> >  within the set-hook to remember the previous value and restore it
> >  manually when an invalid change is made.
> > 
> >  2. If the set-hook throws an error then the change notification is
> >  skipped, even though the setting might still have changed (say, back to
> >  some default value).
> > 
> > The solution I propose here is to delay destroying the old value of the
> > setting until after the set-hook has been called.  If the set-hook runs
> > successfully then the old value will be destroyed when do_set_command
> > returns, the new value will be left in place, and the change
> > notification will be sent sent out exactly as before.
> > 
> > However, if the set-hook throws an error this is now caught in
> > do_set_command, the new value of the setting is removed, and the old
> > value is restored.
> > 
> > After this change, it is no longer possible in a set-hook to change a
> > setting to a default value _and_ throw an error.  If you throw an error
> > you should assume that gdb will restore the previous value of the
> > setting.  If you want to change a setting in the set-hook and inform the
> > user, then a warning, or just a standard message should be fine.
> 
> I have a few questions on this design.
> 
> Take a look at solib.c:gdb_sysroot_changed, the "set sysroot"
> set hook.  reload_shared_libraries does a lot of work, and it
> can easily trip on some error trying to fetch DSOs out of the
> new sysroot.  Should the sysroot setting change back to what it was before,
> when such an error happens?  What happens to the shared libraries that might
> have already been loaded in the new sysroot?  Should "set" hooks
> handle cases like this themselves, by wrapping all but command variable
> validation with TRY/CATCH?

I've been thinking about this on and off since your review, and I
think that if the set-hook does not already catch and handle thrown
errors (except for "incorrect value" type errors) then we _might_
already have bugs.

If the set-hook is made from 3 steps, and step 2 throws an error, what
does that mean right now?  We'll have done some of the steps to switch
to the new value, but not all, and we'll have not sent out a
notification that the setting changed.  It feels like we'll not be in
a consistent state as it is.... though I agree that switching the
setting back doesn't make this better (and maybe makes it worse).

> I'm also concerned about ctrl-c/QUIT while inside the set hook.
> E.g., if the user presses ctrl-c while the set hook is working,
> and a QUIT is thrown out of the set hook, should the setting's value
> be reverted or not?  Maybe the simplest to imagine is if the set hook
> prints something that paginates, and the user presses ctrl-c to
> abort the pagination.  Maybe whether to revert depends on whether
> the setting's new value was already validated or not?

Again for the same reasons as above this currently (possibly) leaves
GDB in an inconsistent state, right?

> I'm wondering whether a new "validate" hook, called before
> the "set" hook is called, would be a better design.  Maybe not.
> I'd like to have the design around those points clarified, so
> that we have some agreed direction on how to handle such scenarios.

I'm happy to do the work to switch over to a validate-hook/set-hook
split.  And for what I want to achieve this would solve my problems,
while leaving the above issues untouched.

Given the large amount of change this would be I'd ideally like some
level of agreement that it's the right direction before I started.

The current patch simply ignores the issues above; we're currently on
iffy ground if the set-hook throws an error anyway, so after my change
it's not clear if we're really worse off (w.r.t. the above issues) or
if things are just iffy in a different way.... My suspicion is that
resolving that above issues might be something that needs to be done
on a case-by-case basis in the set-hooks.

Again, my goal here is to unify the rollback code we have spread
throughout GDB, and I'm not tied to any implementation.  I'd welcome
any feedback for what might be the best direction to take.

Thanks,
Andrew



> 
> > +
> > +  /* Should we revert the setting to the previous value?  If not then we
> > +     should clean up the previous value.  */
> > +  int revert_p;
> 
> "bool".
> 
> > -  c->func (c, NULL, from_tty);
> > +
> > +  TRY
> > +    {
> > +      c->func (c, NULL, from_tty);
> > +    }
> > +  CATCH (ex, RETURN_MASK_ERROR)
> > +    {
> > +      /* Set the REVERT_P flag based on OPTION_CHANGED.  If this leaves
> > +	 REVERT_P as false then the cleanup SET_CMD_CLEANUP_OR_REVERT will
> > +	 try to cleanup the previous value, but as OPTION_CHANGED was
> > +	 false, there is no previous value, and so nothing is done.  If
> > +	 REVERT_P is changed to TRUE here then there was a previous value,
> > +	 and so the SET_CMD_CLEANUP_OR_REVERT will restore the previous
> > +	 value, and cleanup the new value that we no longer need.  */
> > +      set_cmd_cleanup_data.revert_p = option_changed;
> > +      throw_exception (ex);
> > +    }
> > +  END_CATCH
> 
> Why wrap with TRY/CATCH instead of:
> 
>       set_cmd_cleanup_data.revert_p = option_changed;
>       c->func (c, NULL, from_tty);
>       set_cmd_cleanup_data.revert_p = false;
> 
> Note how ctrl-c inside the "set" hook ends up not reverting,
> of RETURN_MASK_ERROR instead of RETURN_MASK_ALL.
> If that was the reason for the TRY/CATCH, then I'd expect the
> comment to mention it.
> 
> 
> 
> > +gdb_exit
> > +gdb_start
> > +gdb_reinitialize_dir $srcdir/$subdir
> > +
> > +# Read the current dcache line-size, check default is still 64.
> > +set default_line_size 0
> > +set testname "read default value for dcache line-size"
> > +send_gdb "show dcache line-size\n"
> > +gdb_expect {
> 
> Use gdb_test_multiple.
> 
> > +    -re "Dcache line size is (\[0-9\]+\)\.\r\n$gdb_prompt $" {
> > +	set default_line_size $expect_out(1,string)
> > +	pass $testname
> > +    }
> > +    -re ".*$gdb_prompt $"   { fail $testname }
> > +    timeout	            { fail "$testname (timeout)" }
> > +}
> > +
> > +if { $default_line_size == 0 } then {
> > +    unsupported "dcache-line-set-error"
> 
> untested.
> 
> > +# Try to change to some invalid (non power of 2 value)
> 
> Please write complete sentences in comments.  I.e., add a period.
> 
> > +set invalid_line_size [expr $default_line_size - 1]
> > +gdb_test "set dcache line-size $invalid_line_size" \
> > +    "Invalid dcache line size: $invalid_line_size .*" \
> > +    "First attempt to set invalid dcache line size"
> 
> Lowercase test messages:
> 
>     "first attempt to set invalid dcache line size"
> 
> (throughout) 
> 
> > +
> > +# Check the default is still 64
> 
> Period.
> 
> > +gdb_test "show dcache line-size" \
> > +    "Dcache line size is $new_line_size\." \
> > +    "Check that the new value took"
> 
> "took effect" ?  Or is that idiomatic English perhaps? (I had never
> seen it).
> 
> Thanks,
> Pedro Alves
>