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