From patchwork Fri Nov 11 18:18:44 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 17416 Received: (qmail 105237 invoked by alias); 11 Nov 2016 18:19:11 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 105130 invoked by uid 89); 11 Nov 2016 18:19:10 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=loose, gdb_assert, CATCH, 4657 X-HELO: mail-wm0-f43.google.com Received: from mail-wm0-f43.google.com (HELO mail-wm0-f43.google.com) (74.125.82.43) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 11 Nov 2016 18:19:00 +0000 Received: by mail-wm0-f43.google.com with SMTP id t79so106083752wmt.0 for ; Fri, 11 Nov 2016 10:18:59 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=ud9MUCF0PEbUgQdE8E9hr8T2FaF6S4WXoCxK0qy6h6U=; b=IN0It4U+iwgopSzTuhsUWuXsxoaLJc1huaKZTgQViEPmIgDP/dLwKOD9WvRlMoWAZW 4Tmwx0KPQTYTx5iYyxdbk5EuiHgJCPuttUUfUpiCCic6eTAVwueUUUoa2D79VQIjmlP0 2Eb8MwzXBFm9/nA9ymDFj0M0Gv0/Hl0iF40h2jxDD2C+val5kSNGVCwDDC7pF+eJzydM iTERaMMyF0RmzALIvz6I7jOvqFJHAeqysAiBLea7J1FwUryNdA0kBYFiRXa+ZtBSWpis dL+t5RGLkyAuR7F6sJTnbww3dMvPlq8OvtFhmmE8wSBuLBVIgQo/ihWfOhW5SF9fyDhK 7gWw== X-Gm-Message-State: ABUngvfD1SevZd7NIUm9IlTckUeH8E1aWgA7aZlpLeLRX3wXBsRu3zus9HAg+O08M2on9Q== X-Received: by 10.194.66.101 with SMTP id e5mr9852272wjt.172.1478888337586; Fri, 11 Nov 2016 10:18:57 -0800 (PST) Received: from localhost (host86-164-199-110.range86-164.btcentralplus.com. [86.164.199.110]) by smtp.gmail.com with ESMTPSA id js10sm12750396wjb.19.2016.11.11.10.18.56 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 11 Nov 2016 10:18:56 -0800 (PST) From: Andrew Burgess To: gdb-patches@sourceware.org Cc: Andrew Burgess Subject: [PATCH 1/2] gdb: Restore a settings previous value on error during change Date: Fri, 11 Nov 2016 18:18:44 +0000 Message-Id: <1478888325-32039-2-git-send-email-andrew.burgess@embecosm.com> In-Reply-To: <1478888325-32039-1-git-send-email-andrew.burgess@embecosm.com> References: <1478888325-32039-1-git-send-email-andrew.burgess@embecosm.com> X-IsSubscribed: yes 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 + + * 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 * 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 #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 + + * gdb.base/dcache-line-set-error.exp: New file. + 2016-11-09 Pedro Alves * 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 . + +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"