From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12625 invoked by alias); 16 Nov 2016 18:15:35 -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 12597 invoked by uid 89); 16 Nov 2016 18:15:35 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,SPF_PASS,URIBL_RED autolearn=ham version=3.3.2 spammy=Something, offer, quality, risk X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 16 Nov 2016 18:15:24 +0000 Received: from svr-orw-mbx-03.mgc.mentorg.com ([147.34.90.203]) by relay1.mentorg.com with esmtp id 1c74jx-0007eM-7l from Luis_Gustavo@mentor.com ; Wed, 16 Nov 2016 10:15:21 -0800 Received: from [172.30.4.107] (147.34.91.1) by svr-orw-mbx-03.mgc.mentorg.com (147.34.90.203) with Microsoft SMTP Server (TLS) id 15.0.1210.3; Wed, 16 Nov 2016 10:15:18 -0800 Reply-To: Luis Machado Subject: Re: [PATCH 1/2] gdb: Restore a settings previous value on error during change 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> <20161116180156.GF5975@embecosm.com> To: Andrew Burgess CC: From: Luis Machado Message-ID: <4e138f5b-d611-fef0-3d7d-e26ecb89c9f0@codesourcery.com> Date: Wed, 16 Nov 2016 18:15:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20161116180156.GF5975@embecosm.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-ClientProxiedBy: svr-orw-mbx-03.mgc.mentorg.com (147.34.90.203) To svr-orw-mbx-03.mgc.mentorg.com (147.34.90.203) X-IsSubscribed: yes X-SW-Source: 2016-11/txt/msg00439.txt.bz2 On 11/16/2016 12:01 PM, Andrew Burgess wrote: > * 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 > I don't have any further feedback. Hopefully other's will chime in. :-) Thanks, Luis