From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-1.mimecast.com (us-smtp-2.mimecast.com [205.139.110.61]) by sourceware.org (Postfix) with ESMTP id 0E9A6385E830 for ; Sun, 31 May 2020 13:39:03 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 0E9A6385E830 Received: from mail-wr1-f71.google.com (mail-wr1-f71.google.com [209.85.221.71]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-84-vnWExmulMNaM2ZApN9Nb2g-1; Sun, 31 May 2020 09:38:59 -0400 X-MC-Unique: vnWExmulMNaM2ZApN9Nb2g-1 Received: by mail-wr1-f71.google.com with SMTP id w16so3457556wru.18 for ; Sun, 31 May 2020 06:38:59 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=Hl2R9fAeQsAATWz8o7itsmePuHJsTW9XJnqMnIIYBME=; b=c6GnjlMlH3GcV0DW9JL0f+XNSzYPMOxvYBd0kuamDYYq7bP74l1yarGL6VwJmAkq5m 8yawGcN52iTtWLFbRLGRCPHJlAGRlYmqI6MzbSSZJSwpOIEX0rEwD/oEPag3+MqMM4sa jJUwjCATku68HT89eIuN2Y6M4yn8zW4aRTbtlMAPBpjuXHA4QbG5v5D8KAXVSmaInOGM MerAeWjOQPN2hFTpB98Pbu37xrt0EAxaMdddigTq8tVgeUGxOvGuyXd0cDj81WUlCl6P 31uWtnusUgTUxRtrlPPnoMkS6aO83V2NEdcC68huHFO+fuNi46nHMLhFm1IRHzAEFsBr 5fmQ== X-Gm-Message-State: AOAM530/hrRfPdIMeWN/mHAj8Ce0/fqHmlFVl/Qcwia9Pu1pvYWUgrUs WFGZcgvUN5CJAwr9FIJpre/tOCkQviZQAm9L1sWwj5nCSPGb8ydKDjIkuDaXZiurwuqywcl+1Sf 84hvGRL+57ShZQ7zdWZ8qkQ== X-Received: by 2002:a05:6000:1150:: with SMTP id d16mr17398510wrx.197.1590932337884; Sun, 31 May 2020 06:38:57 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzaQ7bcrrvHRjOUJ5fMVY0T1rLJpCGU0XJ5RUKMWElYSVXe/2fBGxT9tgk2kN6M8Ur9eOzqgg== X-Received: by 2002:a05:6000:1150:: with SMTP id d16mr17398494wrx.197.1590932337616; Sun, 31 May 2020 06:38:57 -0700 (PDT) Received: from ?IPv6:2001:8a0:f909:7b00:56ee:75ff:fe8d:232b? ([2001:8a0:f909:7b00:56ee:75ff:fe8d:232b]) by smtp.gmail.com with ESMTPSA id b9sm17435098wrt.39.2020.05.31.06.38.56 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 31 May 2020 06:38:57 -0700 (PDT) Subject: Re: [PATCH v2][PR gdb/24052] Implement 'set print zero-values on|off' To: Hannes Domani , Gdb-patches References: <20200530161253.61299-1-ssbssa.ref@yahoo.de> <20200530161253.61299-1-ssbssa@yahoo.de> <1980167901.498897.1590883592361@mail.yahoo.com> From: Pedro Alves Message-ID: Date: Sun, 31 May 2020 14:38:55 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <1980167901.498897.1590883592361@mail.yahoo.com> Content-Language: en-US X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-7.7 required=5.0 tests=BAYES_00, BODY_8BITS, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SCC_10_SHORT_WORD_LINES, SCC_5_SHORT_WORD_LINES, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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: Sun, 31 May 2020 13:39:05 -0000 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 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 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 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 . */ +#include + 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. 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 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 , 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 , 0x0, 0x0, 0x601040 }, static static_field = 0} (gdb) p -zero-values off -- t1 $2 = {i3 = 1, d2 = 2.5, p1 = 0x601110 , t2 = {v1 = 3}, t3 = {v1 = 4, v2 = 5}, ia = {1, 2, 3, 4, 5, 6}, ipa = {0x601110 , 0x601040 }, static static_field = 0} Why print "static_field = 0" when zero-values is off? >>> +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. Note that the documentation bits will be reviewed/approved by Eli. Thanks, Pedro Alves