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

[Andrew Burgess <andrew dot burgess at embecosm dot com>]

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