[PATCH v8 4/6] GDB: Allow arbitrary keywords in integer set commands
Maciej W. Rozycki
macro@embecosm.com
Thu Jan 19 21:17:26 GMT 2023
On Thu, 12 Jan 2023, Simon Marchi wrote:
> This is a nice cleanup I think. I didn't want to delay this further, so
> I did the best review I could given the time I had. I'm fine with you
> merging this (you can add my Approved-By), we can always follow up with
> more fixes. If others would like to review it, it's always appreciated,
> but given the time it's been sitting unreviewed, I kind of doubt that
> will happen.
Given your rationale I chose to commit this change, despite a concern
with not using `setting', as this code is correct regardless and as you
say we can always update it further. See below for details. Thank you
for your review.
> > + if (value == l->use)
> > + {
> > + if (l->val.has_value ())
> > + value = *l->val;
> > + else
> > + return allocate_value (builtin_type (gdbarch)->builtin_void);
>
> I was wondering about this line returning void. As far as I understand,
> this isn't used today. It would only be used if we defined a
> literal_def without a user-visible value. And the consequence would be
> that for this setting, using $_gdb_setting("foo") would yield void for
> that setting value.
It's actually used with the next patch and even covered in the testsuite
(with 6/6):
gdb_test "p \$_gdb_setting(\"print characters\")" " = void" \
"_gdb_setting print characters default"
I didn't think it would make sense to move the implementation of this code
path to the next patch even though this one does not make use of it.
Overall I think it will be best if we do not expose internals and add new
magic numbers now that we have the infrastructure that makes it possible
to avoid. I only made an exception following Andrew's request here:
<https://sourceware.org/pipermail/gdb-patches/2022-June/190389.html> to
make `set print characters' accept "0" for "unlimited" for consistency
with `set print elements'.
> > + if (var.type () == var_uinteger)
> > + return
> > + value_from_ulongest (builtin_type (gdbarch)->builtin_unsigned_int,
> > + static_cast<const unsigned int> (value));
> > + else
> > + return
> > + value_from_longest (builtin_type (gdbarch)->builtin_int,
> > + static_cast<const int> (value));
>
> Unnecessary consts in casts.
Fixed, thanks.
> > if (completion != nullptr)
> > {
> > - if (**args == '\0')
> > - {
> > - /* Convenience to let the user know what the option
> > - can accept. Note there's no common prefix between
> > - the strings on purpose, so that readline doesn't do
> > - a partial match. */
> > - completion->tracker.add_completion
> > - (make_unique_xstrdup ("NUMBER"));
> > - completion->tracker.add_completion
> > - (make_unique_xstrdup ("unlimited"));
> > - return {};
> > - }
> > - else if (startswith ("unlimited", *args))
> > + if (match->extra_literals != nullptr)
>
> The two ifs could be merged, it would help reduce the indentation a bit.
Good catch, thanks! Fixed.
> > -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)
>
> The function here could perhaps take a `const setting &`, instead of var_type
> and extra_literals, which are properties of a setting.
It is used by `parse_option' however, where no setting is involved, so
it's not clear to me how to do that. I could make an artificial setting I
suppose, but I feel like it's kind of creating a problem to fit the
solution. Let me know if I've missed something.
> > + if (!printed)
> > + {
> > + if (var.type () == var_uinteger)
> > + stb.printf ("%u", static_cast<const unsigned int> (value));
> > + else
> > + stb.printf ("%d", static_cast<const int> (value));
>
> The consts are unnecessary here.
Fixed, thanks.
> > @@ -106,22 +108,25 @@ enum var_types
> > var_optional_filename,
> > /* String which stores a filename. (*VAR) is a std::string. */
> > var_filename,
> > - /* ZeroableInteger. *VAR is an int. Like var_integer except
> > - that zero really means zero. */
> > - var_zinteger,
> > - /* ZeroableUnsignedInteger. *VAR is an unsigned int. Zero really
> > - means zero. */
> > - var_zuinteger,
> > - /* ZeroableUnsignedInteger with unlimited value. *VAR is an int,
> > - but its range is [0, INT_MAX]. -1 stands for unlimited and
> > - other negative numbers are not allowed. */
> > - var_zuinteger_unlimited,
> > /* Enumerated type. Can only have one of the specified values.
> > *VAR is a char pointer to the name of the element that we
> > find. */
> > var_enum
> > };
> >
> > +/* A structure describing an extra literal accepted and shown in place
> > + of a number. */
> > +struct literal_def
> > + {
> > + /* The literal to define, e.g. "unlimited". */
> > + const char *literal;
> > + /* The number to substitute internally for LITERAL or VAL;
> > + the use of this number is not allowed (unless the same as VAL). */
> > + LONGEST use;
> > + /* An optional number accepted that stands for the literal. */
> > + gdb::optional<LONGEST> val;
>
> Could you please add an empty line between each field? I find that
> makes it easier to read.
Done (`enum var_types' above seems inconsistent about it).
> > + };
>
> The body of the struct should have one less indentation level (curly
> braces at column 0).
OK (again, just copied `enum var_types' style here).
> > template<typename T>
> > - setting (var_types var_type, T *var)
> > - : m_var_type (var_type), m_var (var)
> > + setting (var_types var_type, T *var, const void *extra_literals = nullptr)
> > + : m_var_type (var_type), m_var (var), m_extra_literals (extra_literals)
>
> Any reason why extra_literals and m_extra_literals are void pointers,
> rather than literal_def pointers?
I guess it's been an artefact from the previous approach which used
`erased_args'. Updated accordingly now.
> > +/* Guile parameter types as in PARAMETER_TYPES later on. */
> > +
> > +typedef enum param_types
> > + {
> > + param_boolean,
> > + param_auto_boolean,
> > + param_zinteger,
> > + param_uinteger,
> > + param_zuinteger,
> > + param_zuinteger_unlimited,
> > + param_string,
> > + param_string_noescape,
> > + param_optional_filename,
> > + param_filename,
> > + param_enum,
> > + }
> > +param_types;
>
> Just "enum param_types, no need for the typedef. Braces at column 0.
Fixed.
> > +static const struct
> > + {
> > + /* The type of the parameter. */
> > + enum var_types type;
> > +
> > + /* Extra literals, such as `unlimited', accepted in lieu of a number. */
> > + const literal_def *extra_literals;
> > + }
>
> Braces at column 0 above.
Ditto.
> > + if (var.type () == var_uinteger)
> > + return scm_from_uint (static_cast<const unsigned int> (value));
> > + else
> > + return scm_from_int (static_cast<const int> (value));
>
> Unnecessary consts in casts.
Fixed.
> > + if (var_type == var_uinteger)
> > + var.set<unsigned int> (static_cast<const unsigned int> (val));
> > + else
> > + var.set<int> (static_cast<const int> (val));
>
> Unnecessary consts in casts.
Likewise.
> > +/* Python parameter types as in PARM_CONSTANTS below. */
> > +
> > +typedef enum param_types
> > + {
> > + param_boolean,
> > + param_auto_boolean,
> > + param_uinteger,
> > + param_integer,
> > + param_string,
> > + param_string_noescape,
> > + param_optional_filename,
> > + param_filename,
> > + param_zinteger,
> > + param_zuinteger,
> > + param_zuinteger_unlimited,
> > + param_enum,
> > + }
> > +param_types;
>
> Just "enum param_types, no need for the typedef. Braces at column 0.
Fixed.
> I think that having two `param_types` enums (here and in the Guile
> support) technically violates the one definition rule. Not sure if that
> matters in practice.
There's no violation as they are defined in separate translation units
each and both have local scope. And so do the respective enumeration
constants.
> > +/* Translation from Python parameters to GDB variable types. Keep in the
> > + same order as PARAM_TYPES due to C++'s lack of designated initializers. */
> > +
> > +static const struct
> > + {
> > + /* The type of the parameter. */
> > + enum var_types type;
> > +
> > + /* Extra literals, such as `unlimited', accepted in lieu of a number. */
> > + const literal_def *extra_literals;
> > + }
>
> Braces at column 0.
Fixed.
> > + if (strcmp (l->literal, "unlimited") == 0)
> > + {
> > + /* Compatibility hack for API brokennes. */
>
> brokenness
Good catch, thanks!
> > + if (var.type () == var_uinteger)
> > + return
> > + gdb_py_object_from_ulongest
> > + (static_cast<const unsigned int> (value)).release ();
> > + else
> > + return
> > + gdb_py_object_from_longest
> > + (static_cast<const int> (value)).release ();
>
> Unnecessary const in casts.
Fixed too.
I'm posting the final version of the patch committed, in a reply to this
message.
Maciej
More information about the Gdb-patches
mailing list