From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-x230.google.com (mail-lj1-x230.google.com [IPv6:2a00:1450:4864:20::230]) by sourceware.org (Postfix) with ESMTPS id 984613858D1E for ; Wed, 17 Aug 2022 22:03:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 984613858D1E Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=embecosm.com Received: by mail-lj1-x230.google.com with SMTP id x25so69487ljm.5 for ; Wed, 17 Aug 2022 15:03:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=mime-version:user-agent:references:message-id:in-reply-to:subject :cc:to:from:date:from:to:cc; bh=1zDr6Uq0DwwDh2xQsF8OVH6UvY2PCN1xpqBwEV6LHQs=; b=fMvGqE2raqCfgpjEwW273gOad35mBd8LeIOSpsps3RzJlEOdmgxYUeE3Qs8bJZXze5 5I5UmTHN/WJx011UNTqXTiHblq4ZT4YyDpSIiRrfuKQqHzlZUF3cjx7iCDqm6JL5Ln6k 81hn9XeQGet4HLHoE4b4aFa51BmB2rVbF1y0ms/dHEEOvele4CQFrq0drX9pv2i4//Mq 3Zu6tEHPG/pcTqkVsNOpsFUR9H+wr2XbSiz8M/ld046KsHZv8GhgiUs0peqUvPyjcL85 euZRX1FHHabBL6YW3m2yv+IiPTJpp/C3AQ1U7yzZuI8jHYwzv32PGP9hS2uVSZpsq3cJ GDaA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=mime-version:user-agent:references:message-id:in-reply-to:subject :cc:to:from:date:x-gm-message-state:from:to:cc; bh=1zDr6Uq0DwwDh2xQsF8OVH6UvY2PCN1xpqBwEV6LHQs=; b=l54A0ZQvOzPvztDOKtR7LIbYBDTp3oS8A4UCiPYSZML8Cdx597WzuKaGCkfDVscy5y rBql3dyzXBGLJqHfjGWShqMJk47dhmKVKFnCYMhAbkEyjiFLRfY0iyUb2OQAvjWcxEWz dHzonqzUP0fdXuTfFszNjfGSnYZ1ZVJYZPbLj9rjzsdidtg6osaIW+MtjCYeAhQFVlHV 0qpnTyTC2l++JN5mjnS4Lnn80ynHCZwHNox9fH39P0jzHSVUF7ywjkRiEXPpu6JZcmy+ 88shmpcHjTkRChzSNKe99zAmjdGDfConhqCn7sCWSpHk6BTxpwJQY8I9vSaumxKsJpML CuEg== X-Gm-Message-State: ACgBeo2w5WtUvyOaZ5XhLjXuL2f8ukhnX8VHD/0TknjRGvyvf/W7nOvM FqkgQpzUfOwPDFVNDW6Kabi9Q1aA0APF7zE1 X-Google-Smtp-Source: AA6agR4sNQZ3is97SNMLaZSY5cSNhrGzL7J97b/YDCMBp1vNmToFvpRaLBZJS5a+3P5IedH9ITz9NQ== X-Received: by 2002:a2e:bf16:0:b0:25f:f90a:b856 with SMTP id c22-20020a2ebf16000000b0025ff90ab856mr74557ljr.473.1660773795124; Wed, 17 Aug 2022 15:03:15 -0700 (PDT) Received: from [192.168.219.3] ([78.8.192.131]) by smtp.gmail.com with ESMTPSA id c24-20020ac24158000000b0048af6d4e5fbsm1825085lfi.275.2022.08.17.15.03.12 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Wed, 17 Aug 2022 15:03:14 -0700 (PDT) Date: Wed, 17 Aug 2022 23:03:11 +0100 (BST) From: "Maciej W. Rozycki" To: Andrew Burgess cc: gdb-patches@sourceware.org, Simon Sobisch , Tom Tromey Subject: Re: [PATCH v5 6/8] GDB: Allow arbitrary keywords in integer set commands In-Reply-To: <877d50evew.fsf@redhat.com> Message-ID: References: <877d50evew.fsf@redhat.com> User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, KAM_ASCII_DIVIDERS, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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, 17 Aug 2022 22:03:19 -0000 On Tue, 28 Jun 2022, Andrew Burgess wrote: > > Rather than just `unlimited' allow the integer set commands (or command > > options) to define arbitrary keywords for the user to use, removing > > hardcoded arrangements for the `unlimited' keyword. > > > > Make obsolete the confusingly named `var_zinteger', `var_zuinteger' and > > `var_zuinteger_unlimited' `set'/`show' command variable types redefining > > them in terms of `var_uinteger', `var_integer' and `var_pinteger', which > > have the range of [0;UINT_MAX], [INT_MIN;INT_MAX], and [0;INT_MAX] each. > > > > Following existing practice `var_pinteger' allows extra negative values > > to be used, however unlike `var_zuinteger_unlimited' any number of such > > values can be defined rather than just `-1'. > > > > The "p" in `var_pinteger' stands for "positive", for the lack of a more > > appropriate unambiguous letter, even though 0 obviously is not positive; > > "n" would be confusing as to whether it stands for "non-negative" or > > "negative". > > While looking at this patch I was thinking to myself, I wonder why we > need var_pinteger, can't we achieve the same thing using var_uinteger > and different literals? We probably could. We'd have to forbid values within (INT_MAX;UINT_MAX] however somehow, and it's not clear to me such an arrangement is worth the complication. Mind that `var_pinteger' is a generalisation of `var_zuinteger_unlimited' and whoever came with `var_zuinteger_unlimited' must have had a reason not to define it in terms of an unsigned integer in the first place (and use UINT_MAX for `unlimited'), most likely to avoid values beyond INT_MAX. > So I think I've convinced myself that you don't actually make use of > var_pinteger in this patch, and I'm pretty sure you don't use it in any > of the later patches either. > > Maybe I'm wrong, so could you point out where it's used? See `add_setshow_zuinteger_unlimited_cmd' (and `add_setshow_pinteger_cmd' and `add_setshow_cmds_for_options' obviously as well; the idea here is to gradually move away from `add_setshow_zuinteger_unlimited_cmd' as parts of code are converted to using `add_setshow_cmds_for_options' referring to `zuinteger_unlimited_literals' as extra literals rather than individual calls to `add_setshow_*_cmd' functions). > If it's not used then I think we should remove it. > > If it's only used in a later commit then I think we should split its > addition out into a later commit. > > If its used in this commit, then I'm sorry for missing it! Your concerns however have prompted me to actually make one step further and remove `var_zinteger', `var_zuinteger' and `var_zuinteger_unlimited' enumeration constants altogether, even from the Guile/Scheme and Python interface modules, which should hopefully reduce any confusion that may remain. This has actually made arrangements for extra literals more straightforward in the interface modules and while I have not implemented Scheme or Python syntax for defining new settings with arbitrary extra literals, it should now be pretty straightforward for anyone fluent in these languages. And it made another pair of places where `var_pinteger' is referred from. > > Index: src/gdb/cli/cli-option.h > > =================================================================== > > --- src.orig/gdb/cli/cli-option.h > > +++ src/gdb/cli/cli-option.h [...] > > @@ -207,11 +211,12 @@ struct uinteger_option_def : option_def > > { > > uinteger_option_def (const char *long_option_, > > unsigned int *(*get_var_address_cb_) (Context *), > > + const literal_def *extra_literals_, > > show_value_ftype *show_cmd_cb_, > > const char *set_doc_, > > const char *show_doc_ = nullptr, > > const char *help_doc_ = nullptr) > > - : option_def (long_option_, var_uinteger, > > + : option_def (long_option_, var_uinteger, extra_literals_, > > (erased_get_var_address_ftype *) get_var_address_cb_, > > show_cmd_cb_, > > set_doc_, show_doc_, help_doc_) > > I think we should add a second constructor for uinteger_option_def like > this: > > uinteger_option_def (const char *long_option_, > unsigned int *(*get_var_address_cb_) (Context *), > show_value_ftype *show_cmd_cb_, > const char *set_doc_, > const char *show_doc_ = nullptr, > const char *help_doc_ = nullptr) > : uinteger_option_def (long_option_, get_var_address_cb_, > uinteger_unlimited_literals, show_cmd_cb_, > set_doc_, show_doc_, help_doc_) > { /* Nothing. */ } > > This would keep the existing '0 == unlimited' as the default behaviour, > but still give the option of overriding the literals if needed. I think this would be going against the spirit of my change though where `uinteger' is just a plain unsigned integer with no extra semantic features implied, which is what any software engineer not familiar with GDB's convoluted history would assume. Myself I have had to double-check the description of `var_types' enumeration constants every time while working on this change so as not to get the meanings confused. With your proposed update one would have to pass `nullptr' explicitly as `extra_literals' to get a plain unsigned integer setting and I find it backwards. I find it more natural for a missing initialiser to mean `nullptr' and I have added suitable extra constructors now. NB eventually we'll need an `integer_option_def' class here as well, but I'm leaving it out until there's a user for it; I could perhaps convert say gdb/source.c for demonstrational purposes, but I'm not that inclined. > > @@ -220,18 +225,19 @@ struct uinteger_option_def : option_def > > } > > }; > > > > -/* A var_zuinteger_unlimited command line option. */ > > +/* A var_integer command line option. */ > > > > template > > -struct zuinteger_unlimited_option_def : option_def > > If instead of deleting zuinteger_unlimited_option_def we redefined the > class like this: > > /* A var_zuinteger_unlimited command line option. */ > > template > struct zuinteger_unlimited_option_def : integer_option_def > { > zuinteger_unlimited_option_def (const char *long_option_, > int *(*get_var_address_cb_) (Context *), > show_value_ftype *show_cmd_cb_, > const char *set_doc_, > const char *show_doc_ = nullptr, > const char *help_doc_ = nullptr) > : integer_option_def (long_option_, get_var_address_cb_, > zuinteger_unlimited_literals, show_cmd_cb_, > set_doc_, show_doc_, help_doc_) > { /* Nothing. */ } > }; > > We could then still make use of zuinteger_unlimited_option_def if we > wanted, which would be an integer with zuinteger_unlimited_literals. Like above, I'd prefer to get rid of the convoluted name (and this needs to be called `pinteger_option_def'; so much for my confusion avoidance). Also the more I think about it the more I am convinced all the three variables holding the "unlimited" extra literal simply ought to match the name of the corresponding `var_types' enumeration constant, so `pinteger_unlimited_literals' here and elsewhere. Ultimately we'll need `integer_option_def' too, but there's no user currently (settings of the `var_integer' type are only created via the legacy `add_setshow_integer_cmd' function, which I think should eventually go away). > > Index: src/gdb/cli/cli-setshow.c > > =================================================================== > > --- src.orig/gdb/cli/cli-setshow.c > > +++ src/gdb/cli/cli-setshow.c [...] > > /* See cli-setshow.h. */ > > > > -unsigned int > > -parse_cli_var_uinteger (var_types var_type, const char **arg, > > - bool expression) > > +LONGEST > > +parse_cli_var_integer (var_types var_type, const literal_def *extra_literals, > > + const char **arg, bool expression) > > { > > LONGEST val; > > > > if (*arg == nullptr || **arg == '\0') > > { > > - if (var_type == var_uinteger) > > - error_no_arg (_("integer to set it to, or \"unlimited\"")); > > - else > > + if (extra_literals == nullptr) > > error_no_arg (_("integer to set it to")); > > - } > > - > > - if (var_type == var_uinteger && is_unlimited_literal (arg, expression)) > > - val = 0; > > - else if (expression) > > - val = parse_and_eval_long (*arg); > > - else > > - val = get_ulongest (arg); > > - > > - if (var_type == var_uinteger && val == 0) > > - val = UINT_MAX; > > - else if (val < 0 > > - /* For var_uinteger, don't let the user set the value > > - to UINT_MAX directly, as that exposes an > > - implementation detail to the user interface. */ > > - || (var_type == var_uinteger && val >= UINT_MAX) > > - || (var_type == var_zuinteger && val > UINT_MAX)) > > - error (_("integer %s out of range"), plongest (val)); > > - > > - return val; > > -} > > - > > -/* See cli-setshow.h. */ > > + else > > + { > > + std::string buffer = ""; > > + size_t count = 0; > > > > -int > > -parse_cli_var_zuinteger_unlimited (const char **arg, bool expression) > > -{ > > - LONGEST val; > > + for (const literal_def *l = extra_literals; > > + l->literal != nullptr; > > + l++, count++) > > + { > > + if (count != 0) > > + buffer += ", "; > > + buffer = buffer + '"' + l->literal + '"'; > > + } > > + if (count > 1) > > + error_no_arg > > + (string_printf (_("integer to set it to, or one of: %s"), > > + buffer.c_str ()).c_str ()); > > + else > > + error_no_arg > > + (string_printf (_("integer to set it to, or %s"), > > + buffer.c_str ()).c_str ()); > > + } > > + } > > > > - if (*arg == nullptr || **arg == '\0') > > - error_no_arg (_("integer to set it to, or \"unlimited\"")); > > + if (!get_literal_val (val, extra_literals, arg, expression)) > > + { > > + if (expression) > > + val = parse_and_eval_long (*arg); > > + else > > + val = get_ulongest (arg); > > > > - if (is_unlimited_literal (arg, expression)) > > - val = -1; > > - else if (expression) > > - val = parse_and_eval_long (*arg); > > - else > > - val = get_ulongest (arg); > > + enum tribool allowed = TRIBOOL_UNKNOWN; > > + if (extra_literals != nullptr) > > + { > > + for (const literal_def *l = extra_literals; > > + l->literal != nullptr; > > + l++) > > + if (val == l->val) > > + { > > + allowed = l->allow ? TRIBOOL_TRUE : TRIBOOL_FALSE; > > + val = l->use; > > + break; > > + } > > + else if (val == l->use) > > + { > > + allowed = TRIBOOL_FALSE; > > + break; > > + } > > + } > > I haven't run the testsuite on this, so maybe there's issues, but I'd > like to propose that this loop be rewritten as: > > if (extra_literals != nullptr) > { > for (const literal_def *l = extra_literals; > l->literal != nullptr; > l++) > if (val == l->val && l->allow) > { > allowed = TRIBOOL_TRUE; > val = l->use; > break; > } > else if (val == l->use) > allowed = TRIBOOL_FALSE; > } This has a problem where `l->val' is not allowed with one entry and it doesn't match any other. In that case execution will fall through to TRIBOOL_UNKNOWN below and the value will be incorrectly accepted as long as within the range of the requested `var_type'. > The 'if (val == l->val && l->allow)' would be improved if l->val was > changed to an optional. I like this idea (mind that I'm a C old-timer really and I haven't still soaked with all the C++ features we have at hand) and I have updated code accordingly. > The change to the 'val == l->use' block allows > I think another improvement, this: > > static const literal_def print_characters_literals[] = > { > { "elements", 0, 0, false }, > { "unlimited", 0, UINT_MAX, true }, > { nullptr } > }; > > The change here is that: > > set print characters 0 > > now behaves the same as: > > set print elements 0 > > In both cases, this sets the value to "unlimited". I like the > consistency. You needn't change the loop to achieve this effect, you can just swap the entries in the array or use UINT_MAX as `val' for "elements". And with optional `l->val' you would actually still have to swap the entries as `l->use' would match against 0 first and reject it. The idea with my change was not to expose the internal value for new additions. I guess I'm not that attached to it, so if you think such consistency matters (mind that we have different numeric values for "unlimited" across various settings anyway), then I can allow 0 here. > > Index: src/gdb/command.h > > =================================================================== > > --- src.orig/gdb/command.h > > +++ src/gdb/command.h [...] > > @@ -123,6 +126,22 @@ typedef enum var_types > > } > > var_types; > > > > +/* A structure describing an extra literal accepted and shown in place > > + of a number. */ > > +typedef struct literal_defs > > You don't need the typedef trick as this is C++. We still have them in several places though. I have now removed it from my change. > > + { > > + /* The literal to define, e.g. "unlimited". */ > > + const char *literal; > > + /* A number optionally accepted that stands for the literal. */ > > + LONGEST val; > > + /* The number to substitute internally for LITERAL or VAL; > > + the use of this number is not allowed (unless the same as VAL). */ > > + LONGEST use; > > + /* True if the use of VAL is allowed; otherwise only the literal is. */ > > + bool allow; > > Could we replace VAL and ALLOW with something like: > > gdb::optional .... Done now. > > > + } > > +literal_def; > > + > > /* Return true if a setting of type VAR_TYPE is backed with type T. > > > > This function is left without definition intentionally. This template is > > @@ -153,15 +172,14 @@ inline bool var_type_uses > template<> > > inline bool var_type_uses (var_types t) > > { > > - return (t == var_uinteger || t == var_zinteger || t == var_zuinteger); > > + return t == var_uinteger; > > } > > Should we really remove all these uses of var_zinteger, etc? The enum > values are still around, right? So surely if you passed in that enum > field the true/false response shouldn't change... Not relevant anymore since I have removed `var_zinteger', etc. altogether as the attempt to leave them for Guile/Python was clearly hardly less if not more confusing than the original arrangement. Thank you for your review, I'll be posting v6 shortly. While making this update I have discovered a few further issues, so we're back at 8 patches. Maciej