From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lndn.lancelotsix.com (lndn.lancelotsix.com [51.195.220.111]) by sourceware.org (Postfix) with ESMTPS id 9519A38515FC for ; Wed, 14 Jul 2021 12:08:57 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 9519A38515FC Received: from ubuntu.lan (unknown [IPv6:2a02:390:9086::635]) by lndn.lancelotsix.com (Postfix) with ESMTPSA id 81F3783773; Wed, 14 Jul 2021 12:08:56 +0000 (UTC) Date: Wed, 14 Jul 2021 12:08:51 +0000 From: Lancelot SIX To: Simon Marchi Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 15/16] gdb: make cmd_list_element var an optional union Message-ID: <20210714120851.3pfew5pgcdp6ezn6@ubuntu.lan> References: <20210714045520.1623120-1-simon.marchi@polymtl.ca> <20210714045520.1623120-16-simon.marchi@polymtl.ca> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20210714045520.1623120-16-simon.marchi@polymtl.ca> 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 12:08:56 +0000 (UTC) X-Spam-Status: No, score=-5.6 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 12:09:00 -0000 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. > > 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(-)