This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 1/2] gdb: Restore a settings previous value on error during change
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.