From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 119324 invoked by alias); 16 Nov 2016 18:02:24 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 119307 invoked by uid 89); 16 Nov 2016 18:02:22 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.1 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM,SPF_PASS,URIBL_RED autolearn=no version=3.3.2 spammy=Something, H*RU:sk:host86-, Hx-spam-relays-external:sk:host86-, H*r:sk:host86- X-HELO: mail-wm0-f54.google.com Received: from mail-wm0-f54.google.com (HELO mail-wm0-f54.google.com) (74.125.82.54) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 16 Nov 2016 18:02:12 +0000 Received: by mail-wm0-f54.google.com with SMTP id f82so89988706wmf.1 for ; Wed, 16 Nov 2016 10:02:11 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=+Ez0fkTRz1arT/2vnI3fg5h09jHD/lbmkixQH6Rs80I=; b=IttZd1dTL+pVHqWCPB6J9/yD6JNlSKN/n7i3uz00eRamp3A85P2paBBLZL99t0mm4B oZc2tG6EWBIvrDljO8DhN5GjNg5ceubAhveM2ok4iOqOHfR0gDx+yrocLIkpuTJApUat PnRsX0aVcwHY/dN3Wj4PHXFI60fwsnVWhJL8AyTLyyBRSD+YVYEvEhF9+oY/qC3qjuAw P/sMGjLmbBnuTfnZ0owQfQuUwAWNv5sh69qhKMFkYWtdYqyEITnozGp6uhgz0d7ymXpI UE3DVxo+lHvu5fXswx3C1XlrLRVAmEu295SYDhxMSkIO0VC+Q0n9gmlyq/HKsKK00WrP fg/Q== X-Gm-Message-State: ABUngve+QPDvZzp9rvfcM0ujUISkM/dYxUqr6JvxZqlRHIGLysA66aUoY0Gz1+lo8tLu1w== X-Received: by 10.28.14.66 with SMTP id 63mr10531811wmo.127.1479319330027; Wed, 16 Nov 2016 10:02:10 -0800 (PST) Received: from localhost (host86-135-139-198.range86-135.btcentralplus.com. [86.135.139.198]) by smtp.gmail.com with ESMTPSA id wg8sm36942335wjb.42.2016.11.16.10.02.08 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 16 Nov 2016 10:02:09 -0800 (PST) Date: Wed, 16 Nov 2016 18:02:00 -0000 From: Andrew Burgess To: Luis Machado Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 1/2] gdb: Restore a settings previous value on error during change Message-ID: <20161116180156.GF5975@embecosm.com> References: <1478888325-32039-1-git-send-email-andrew.burgess@embecosm.com> <1478888325-32039-2-git-send-email-andrew.burgess@embecosm.com> <8ef10d71-c977-d0ee-c6c5-f4b1aac3f55a@codesourcery.com> <20161115230218.GC5975@embecosm.com> <94e1d60f-3e69-bdbf-b549-d96d5be8dfdd@codesourcery.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <94e1d60f-3e69-bdbf-b549-d96d5be8dfdd@codesourcery.com> X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.6.1 (2016-04-27) X-IsSubscribed: yes X-SW-Source: 2016-11/txt/msg00438.txt.bz2 * Luis Machado [2016-11-16 08:17:50 -0600]: > On 11/15/2016 05:02 PM, Andrew Burgess wrote: > > * Luis Machado [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