[PATCH v2][PR gdb/24052] Implement 'set print zero-values on|off'
Hannes Domani
ssbssa@yahoo.de
Sun May 31 14:58:42 GMT 2020
Am Sonntag, 31. Mai 2020, 15:39:05 MESZ hat Pedro Alves <palves@redhat.com> Folgendes geschrieben:
> On 5/31/20 1:06 AM, Hannes Domani via Gdb-patches wrote:
> > 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.
>
> I now see that the original reporter also mentioned enums.
>
> The thing is that an enum does not measure a quantity or offset.
> "0" has no usual particular significance compared to
> other enumerators. While with pointers and integrals, usually
> "0" has significance, meaning no quantity, no offset or no object,
> or in general absence of the property being measured by the variable.
>
> For example, here's GDB's auto_boolean:
>
> /* * A generic, not quite boolean, enumeration. This is used for
> set/show commands in which the options are on/off/automatic. */
> enum auto_boolean
> {
> AUTO_BOOLEAN_TRUE,
> AUTO_BOOLEAN_FALSE,
> AUTO_BOOLEAN_AUTO
> };
>
> I'd think it confusing that "zero-values off" would hide
> AUTO_BOOLEAN_TRUE, but not AUTO_BOOLEAN_FALSE.
>
> Here:
>
> extern enum language_mode
> {
> language_mode_auto, language_mode_manual
> }
> language_mode;
>
> What's the significance of hiding auto but not manual?
>
> Here:
>
> /* alignment enum */
> enum ui_align
> {
> ui_left = -1,
> ui_center,
> ui_right,
> ui_noalign
> };
>
> Why hide ui_center, instead of the other enumerators?
>
> Etc.
It seems we have very different views about this.
I don't think it's confusing at all to hide AUTO_BOOLEAN_TRUE/
language_mode_auto/ui_center in these cases.
(For me it's more confusing that AUTO_BOOLEAN_TRUE is first in this enum.)
If you don't want to hide it, just don't use -zero-values off.
> >
> >
> >> 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.)
>
> Not sure about wrong but at least it will require tweaking, even if
> you ignore printers and enums, for the fact that it ignores
> structure padding.
>
> For example, apply this on top of your patch:
>
> From c9210531b1a57b05bce7b7da46575050cda15bf8 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Sun, 31 May 2020 14:08:17 +0100
> Subject: [PATCH] padding
>
> ---
> gdb/testsuite/gdb.base/zero-values.c | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/gdb/testsuite/gdb.base/zero-values.c b/gdb/testsuite/gdb.base/zero-values.c
> index 0a281d9671b..b2309398108 100644
> --- a/gdb/testsuite/gdb.base/zero-values.c
> +++ b/gdb/testsuite/gdb.base/zero-values.c
> @@ -15,6 +15,8 @@
> You should have received a copy of the GNU General Public License
> along with this program. If not, see <http://www.gnu.org/licenses/>. */
>
> +#include <string.h>
> +
> int ix;
> struct two
> {
> @@ -33,8 +35,29 @@ struct t
> {0, 1, 2, 0, 0, 3, 4, 5, 0, 6}, {0, 0, 0},
> {0, &ix, 0, 0, &t1.i1}};
>
> +struct padding
> +{
> + char c;
> + int i;
> +};
> +
> +struct outer
> +{
> + struct padding pad1;
> + struct padding pad2;
> +};
> +
> +struct outer g_out;
> +
> int
> main ()
> {
> + struct outer out;
> + memset (&out, 0xff, sizeof (out));
> + out.pad1.c = 0;
> + out.pad1.i = 0;
> + out.pad2.c = 0;
> + out.pad2.i = 0;
> +
> return 0;
> }
>
> --
> 2.14.5
>
> Now run to the "return 0;" line, and see:
>
> (gdb) p g_out
> $1 = {pad1 = {c = 0 '\000', i = 0}, pad2 = {c = 0 '\000', i = 0}}
> (gdb) p out
> $2 = {pad1 = {c = 0 '\000', i = 0}, pad2 = {c = 0 '\000', i = 0}}
>
> (gdb) p -zero-values off -- g_out
> $3 = {}
> (gdb) p -zero-values off -- out
> $4 = {pad1 = {}, pad2 = {}}
>
> As you see, $3 and $4 gave different outputs, due to the padding.
I agree that this might be weird, but I kinda see this as a feature.
> There's still the question about pretty printing open, though.
>
> (BTW, while quickly playing with it, I wondered whether we could come
> up with an option name that would make it so that "on" would
> hide the zeroes, so that you could get away without having to
> write "off" by default, like "p -z -- out". That may conflict
> with the potential desire to expand on/off into an enumeration
> with other variants other than on/off, though.)
>
>
> >>> @@ -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.
>
> I don't get it. If they are not skipped, why wouldn't
> "print -zero-value off" hide zero values in static fields?
>
> Apply this further patch on top of the other one:
>
> From d3e174a53712c1c0a797d201e10d186b35174509 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Sun, 31 May 2020 14:29:53 +0100
> Subject: [PATCH] static
>
> ---
> gdb/testsuite/gdb.base/zero-values.c | 5 +++++
> gdb/testsuite/gdb.base/zero-values.exp | 2 +-
> 2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/testsuite/gdb.base/zero-values.c b/gdb/testsuite/gdb.base/zero-values.c
> index b2309398108..57ce6ba71db 100644
> --- a/gdb/testsuite/gdb.base/zero-values.c
> +++ b/gdb/testsuite/gdb.base/zero-values.c
> @@ -31,10 +31,15 @@ struct t
> int ia[10];
> int ea[3];
> int *ipa[5];
> +
> + static int static_field;
> +
> } t1 = {0, 0, 1, 0, 2.5, 0, &ix, 0, 0, {0, 0}, {3, 0}, {4, 5},
> {0, 1, 2, 0, 0, 3, 4, 5, 0, 6}, {0, 0, 0},
> {0, &ix, 0, 0, &t1.i1}};
>
> +int t::static_field = 0;
> +
> struct padding
> {
> char c;
> diff --git a/gdb/testsuite/gdb.base/zero-values.exp b/gdb/testsuite/gdb.base/zero-values.exp
> index a8f377b8f7e..8f3c2cca3cb 100644
> --- a/gdb/testsuite/gdb.base/zero-values.exp
> +++ b/gdb/testsuite/gdb.base/zero-values.exp
> @@ -17,7 +17,7 @@
>
> standard_testfile
>
> -if {[prepare_for_testing $testfile.exp $testfile $srcfile debug]} {
> +if {[prepare_for_testing $testfile.exp $testfile $srcfile {debug c++}]} {
> untested $testfile.exp
> return -1
> }
>
> --
> 2.14.5
>
> And now observe:
>
> (gdb) p t1
> $1 = {i1 = 0, i2 = 0, i3 = 1, d1 = 0, d2 = 2.5, d3 = 0, p1 = 0x601110 <ix>, p2 = 0x0, p3 = 0x0, t1 = {v1 = 0, v2 = 0}, t2 = {v1 = 3, v2 = 0}, t3 = {v1 = 4, v2 = 5}, ia = {0, 1, 2, 0, 0, 3, 4, 5, 0, 6}, ea = {0, 0, 0}, ipa = {0x0, 0x601110 <ix>, 0x0, 0x0, 0x601040 <t1>}, static static_field = 0}
>
> (gdb) p -zero-values off -- t1
> $2 = {i3 = 1, d2 = 2.5, p1 = 0x601110 <ix>, t2 = {v1 = 3}, t3 = {v1 = 4, v2 = 5}, ia = {1, 2, 3, 4, 5, 6}, ipa = {0x601110 <ix>, 0x601040 <t1>}, static static_field = 0}
>
> Why print "static_field = 0" when zero-values is off?
When printing structures, I usually don't care about the static members.
And with -zero-values off it should just display the parts that have some kind
of value.
So now I kinda want to hide all static members when -zero-values off, no
matter what their real value is.
> >>> +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'.
> Yes, something like that. I guess also for unions. You should add a
> test for unions too, btw.
OK.
> Note that the documentation bits will be reviewed/approved
> by Eli.
Hannes
More information about the Gdb-patches
mailing list