From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lndn.lancelotsix.com (vps-42846194.vps.ovh.net [IPv6:2001:41d0:801:2000::2400]) by sourceware.org (Postfix) with ESMTPS id C99353848022 for ; Wed, 14 Jul 2021 17:12:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C99353848022 Received: from ubuntu.lan (unknown [IPv6:2a02:390:9086::635]) by lndn.lancelotsix.com (Postfix) with ESMTPSA id AD38D82C19; Wed, 14 Jul 2021 17:12:42 +0000 (UTC) Date: Wed, 14 Jul 2021 17:12:38 +0000 From: Lancelot SIX To: Simon Marchi , gdb-patches@sourceware.org Subject: Re: [PATCH 15/16] gdb: make cmd_list_element var an optional union Message-ID: <20210714171238.vzccwpurh2izbkps@ubuntu.lan> References: <20210714045520.1623120-1-simon.marchi@polymtl.ca> <20210714045520.1623120-16-simon.marchi@polymtl.ca> <20210714120851.3pfew5pgcdp6ezn6@ubuntu.lan> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20210714120851.3pfew5pgcdp6ezn6@ubuntu.lan> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.5.11 (lndn.lancelotsix.com [0.0.0.0]); Wed, 14 Jul 2021 17:12:42 +0000 (UTC) X-Spam-Status: No, score=-5.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 14 Jul 2021 17:12:46 -0000 On Wed, Jul 14, 2021 at 12:08:51PM +0000, Lancelot SIX via Gdb-patches wrote: > On Wed, Jul 14, 2021 at 12:55:19AM -0400, Simon Marchi via Gdb-patches wrote: > > The field cmd_list_element::var is used by set and show commands and > > points to the variable affected by the set command and shown by the show > > command. It is of type `void *` and cast to a pointer of the > > appriopriate type on each use depending on the type of the setting > > (cmd_list_element::var_type). > > > > I propose changing cmd_list_element to be a union of all the possible > > pointer types. Fundamentally, this doesn't change much what is > > happening. But I think this helps understand better how the field is > > used and get a bit of type safety (while it is still possible to use the > > wrong member at some point, we can't cast to something completely > > unrealted). > > Hi, > > I happen to have been working on a different approach to the same > problem. This was triggered when I was reading one of your previous > patches where the surrounding codes had a lot of C style casts all > around. My approach is arguably too complicated for the task, but it > is meant to address (at least partly) the last point you discuss. > > Here is the gist of my approach: I add templated getter / setter to the > var member, the template parameter being the var_type you expect. The > usage looks like this, if you want to access the value of a > cmd_list_element of var_types var_zuinteger: > > cmd_list_element *el; > unsigned int val = el->get_var (); // This type checks. > > The implementation for that requires a bit of boiler plate, but is not > too complex to follow. I add a helper struct I use to describe the > storage type used or each var_types: > > template struct var_types_storage { }; > template<> struct var_types_storage { using t = int; } ; > template<> struct var_types_storage { using t = unsigned int; }; > template<> struct var_types_storage { using t = char *; }; > template<> struct var_types_storage { using t = const char *; }; > // And so on for all the possible var type > > With that available, the getter / setter is quite easy to write: > > template > typename var_types_storage::t get_var () const > { > gdb_assert (T == this->var_type); > return *static_cast::t *> (this->var); > } > > template > void set_var (typename var_types_storage::t v) > { > gdb_assert (T == this->var_type); > *static_cast::t *> (this->var> = v; > } > > template > void set_var_p (typename var_types_storage::t v) > { > this->var_type = T; > this->var = static_cast v; > } > > add_set_or_show_cmd can also be updated in a similar way: > > template static struct cmd_list_element * > add_set_or_show_cmd (const char *name, > enum cmd_types type, > enum command_class theclass, > typename var_types_storage::t *var, > const char *doc, > struct cmd_list_element **list) > { > struct cmd_list_element *c = add_cmd (name, theclass, doc, list); > c->set_var_p (var); > // rest is unchanged > return c; > } > > // similar stuff for add_setshow_cmd_full > > Obviously there are a few differences with your approach. > > First, with my approach, it is possible to add the gdb_assert to check > that when one accesses a value assuming it is of a given type, it > actually is. However, there are few limitations with that, most > notably in gdb/cli/cli-setshow.c, where some processing is common > between cases (var_uinteger and var_zuinteger have quite a lot in > common for example). At some point, the caller wants to call > set_var, but obviously the > assertion will fail half of the time. To go around this, I also use less > type-safe setters / getters: > > template > const T get_var_as() { return *static_cast (this->var); } > template > void set_var_as (T v) { *static_cast (this->var) = v; } > > Second, with the templated approach the caller does not need to know > which member of the union he should access for a given var_types, the > compiler will tel him if an incompatible types are used on the call > site. > > On the other hand, my approach might be more difficult to follow / more > surprising for someone not familiar with this kind of use of template > specialization. It also requires a bit more boiler plate code (i.e. the > var_types_storage struct). > > I still rely on type punning and a underlying void * pointer, which is > not ideal. But at least, this becomes a detail of the implementation > and can be ignored by the caller with all the casting done in just one > place. The void * pointer should (in my approach) eventually be turned > into a private member, which I have not done yet. > > Anyway, your approach is quite an improvement compared to the current > type casting all over the place. I guess that we could, if you want, > have a solution which which is a hybrid of both (i.e. a union instead of > a void*, and templated access functions). > > Obviously I am a bit biased toward my approach. Any thoughts? I’ll > pause my work on my patch for the moment. > > I still need to give a proper read to your patch series, I just skimmed > through to compare our approaches for the moment. > > Best, > Lancelot. Actually, I have been playing around with it a bit more today, and with extra boiler plate, I can remove the get_var_as accessor, and have a more all-rounded get_var. It becomes possible to write something like: // No matter if we are looking at a var_uinteger or a // var_zuinteger, the underlying type we are interested in is // a unsigned int. unsigned int *foo = c->get_var (); In this case, get_var will even figure the common ground for all possibilities the user asks for. The intended use case is when printing the value of a string (ish) variable, and we want to cover multiple scenarios including var_enum and var_string, we can write something like: // c is a const char *, but would be char * if var_enum was not // in the list. auto c = v.get_var (); The code could look like something like this (I am sure it is possible to have a simpler solution to create a similar result): template struct var_types_storage_helper; template struct var_types_storage_helper { static constexpr bool all_same = true; using type = typename var_types_storage::t; static constexpr bool covers_var_type (var_types t) { return t == T; } }; template struct var_types_storage_helper { static constexpr bool all_same = std::is_same::t, typename var_types_storage::t>::value; using type = typename var_types_storage::t; static constexpr bool covers_var_type (var_types t) { return var_types_storage_helper::covers_var_type (t) || var_types_storage_helper::covers_var_type (t); } }; template struct var_types_storage_helper { static constexpr bool all_same = var_types_storage_helper::all_same && var_types_storage_helper::all_same; using type = typename var_types_storage::t; static constexpr bool covers_var_type (var_types t) { return var_types_storage_helper (t) || var_types_storage_helper::covers_var_type (t); } }; and the accessors become: template typename std::common_type::t...>::type get_var() const { assert (var_types_storage_helper::covers_var_type (this->var_type)); return *static_cast::t...>::type *>(this->var); } template void set_var(typename var_types_storage_helper::type v) { assert (var_types_storage_helper::covers_var_type (this->var_type)); static_assert (var_types_storage_helper::all_same); *static_cast::type *> (this->var) = v; } template void set_var_p (typename var_types_storage::t *v) { this->var_type = T; this->var = static_cast (v); } I guess improves the type safety side of things, but the implementation definitely gets more complicated. I am not sure the added checks come at a reasonable price. This should all be valid c++11. Any thoughts? Best, Lancelot. > > > > > > I wrapped the union in an optional, because we need to check in some > > spots whether var is set or not. I think that conceptually, an optional > > makes the most sense. Another option would be to pick an arbitrary > > member of the union and compare it to nullptr, whenever we want to know > > whether var is set, but that seems a bit more hack-ish. > > > > A note on the Guile situation: something that makes our life a little > > bit complicated is that is possible to assign and read the value of a > > Guile-created parameter that is not yet registered (see previous patch > > that adds a test for this). It would have been much more simple to say > > that this is simply not supported anymore, but that could break existing > > scripts, so I don't think it is a good idea. In some cases, for example > > when printing the value of a built-in parameter, we have access to a > > show command and its union setting_variable. When we have an > > un-registered Guile param, we don't have a show command associated to > > it, but we can pass the parameter's pascm_variable union, stored in > > param_smob. Because we have these two kinds of incompatible parameters, > > we need two versions of the pascm_param_value function. > > > > Change-Id: I74a6be2a9188520dbdac4cb123c08e00ebb40a91 > > --- > > gdb/cli/cli-cmds.c | 48 +++++++++----- > > gdb/cli/cli-decode.c | 113 +++++++++++++++++++++++-------- > > gdb/cli/cli-decode.h | 31 ++++++++- > > gdb/cli/cli-setshow.c | 87 +++++++++++++----------- > > gdb/guile/scm-param.c | 125 +++++++++++++++++++++++++++++++---- > > gdb/python/py-param.c | 8 ++- > > gdb/python/python-internal.h | 2 +- > > gdb/python/python.c | 35 +++++++--- > > gdb/remote.c | 2 +- > > 9 files changed, 340 insertions(+), 111 deletions(-)