Re: [PATCH v2][PR gdb/24052] Implement 'set print zero-values on|off'

[Hannes Domani <ssbssa@yahoo.de>]

 Am Samstag, 30. Mai 2020, 20:00:12 MESZ hat Pedro Alves <palves@redhat.com> Folgendes geschrieben:

> Hi,
>
> I take it that false booleans are also considered
> zero, and not shown.  I guess that might be reasonable, though
> I could also see someone arguing about it.
>
> Enums with zero value are more dubious, like:
>
> enum foo { FOO = -1, BAR = 0, QUX = 1 };
>
> From looking at the implementation it seems to me like zero-values off
> makes us not print a BAR value.  Not sure that's really desirable.

It's what I would expect, but that might just be my opinion.


> It seems more clear to me that it'd be desirable to suppress a
> zero with flag enums than with regular enums, like with this:
>
> enum bitmask { FOO_BIT = 1, BAR_BIT = 2, QUX_BIT = 4 };
>
> Here's where you can check whether you have a flag enum:
>
> /* * True if this type is a "flag" enum.  A flag enum is one where all
>     the values are pairwise disjoint when "and"ed together.  This
>     affects how enum values are printed.  */
>
> #define TYPE_FLAG_ENUM(t) (TYPE_MAIN_TYPE (t)->flag_flag_enum)
>
> I think we should have tests for these cases.
>
> There's also the question of how this interacts with Python
> pretty printers:
>
> - If there's a printer for a type or typedef that makes it so that a
> zero is actually printed as some string other than "0", e.g., "JackPot!",
> do we really want to suppress it?
>
> - OTOH, if a printer decides to print a non-zero value as literal "0",
> do we want to show it?
>
> Whatever we decide, I think we should document expected behavior.
>
> Also, it would be good to test function and method pointers too.

The way this is implemented, if I have e.g. this kinda situation:

struct point
{
  int x, y;
};
struct line
{
  point start;
  point end;
};
line l = { {0, 0}, {2, 3} };

Then it would check first if the whole l.start was just zero bytes, and
in that case, not print it at all.

But if I have to check the individual struct members for special cases,
like enums or pretty printers, this whole approach will not work.

(Maybe that means my approach is wrong.)


> > @@ -194,6 +194,18 @@ cp_print_value_fields (struct value *val, struct ui_file *stream,
> >            && field_is_static (&type->field (i)))
> >          continue;
> >
> > +      /* If requested, skip printing of zero value fields.  */
> > +      if (!options->zero_value_print
> > +          && !field_is_static (&type->field (i)))
>
> Not sure whether you copied this by mistake from the code above, but
> skipping static fields seems wrong, since there's an option
> for that.  I think we should keep the the options orthogonal.
>
> ( I could now argue that this calls for a testcase making sure
> that they stay orthogonal.  :-) )

It doesn't skip static fields, they are just not checked for a zero-value.
And that was intentional, mainly for the same reason as above.


> > +bool
> > +value_is_zero (struct value *v)
> > +{
> > +  struct type *type = check_typedef (value_type (v));
> > +  const gdb_byte *addr = value_contents_for_printing (v);
> > +  unsigned int len = TYPE_LENGTH (type);
> > +  unsigned int zeros;
> > +  for (zeros = 0; zeros < len; zeros++)
>
> Write:
>
>   for (unsigned int zeros = 0; zeros < len; zeros++)
>
> But I have to say that I find it weird to name the iterator
> variable "zeros".  Standard "i" would be better, IMO.
>
> TYPE_LENGTH(type) is cheap, so you could also get rid of
> the len variable if you'd like.
>
> > +    {
>
> You can drop this level of braces.
>
> > +      if (addr[zeros] != 0)
> > +    return false;
> > +    }
> > +  return true;
> > +/* Controls printing of zero value members.  */
> > +static void
>
> There should be an empty line after the comment.

OK.


> > +show_zero_value_print (struct ui_file *file, int from_tty,
> > +              struct cmd_list_element *c,
> > +              const char *value)
>
> Finally, this needs a gdb/NEWS entry.

Like this, added to the "New commands" section?:

set print zero-values [on|off]
show print zero-values
  This controls whether GDB prints zero value members of structures and
  arrays.  The default is 'on'.


Hannes