From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from sonic311-30.consmr.mail.ir2.yahoo.com (sonic311-30.consmr.mail.ir2.yahoo.com [77.238.176.162]) by sourceware.org (Postfix) with ESMTPS id 27F073851C34 for ; Sun, 31 May 2020 14:58:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 27F073851C34 X-YMail-OSG: I6fytNYVM1n21u.diyG_7BdscQPt79qnGr.xwVBuzHXCV9JFg4jOt6f.9jJihJY 38fp0ycd.fNXwPNmP2onVmogPK3uyvmvL_CQBqm1nsgTdGcQfgVOGyYgRAS9oPcr_SkzGWJwEY6h LRnI7Jp.1rfEhG3IQZqyadMgLhC7oVkVsJl39x..4X.Ex.txgLylxhtieOdh54J.HnRk8ZsDRj6A GJiGPg8vZ9MJ96PG6747ANWChgqJZyRsIXQZ0yushLlE4t6KxFM5COIFbnv46dkhhix0wMLxfYlN gdtPE8tTFsJ44xOdiHpfsk6oBvCKKz0N1JQCv8RaiKq48.0c3oumBDIeH5VLiDRbNvlGVSbWiGPQ LlxBpx6.utcB1l3tpnhMC3Z2B5dMWm0l1KVZw.uuJm0NFH7qCPioMnvw0Ruj9AMQxwtRSDjoy3p_ GCOguernWfMhwGd4_WP974WsBEXNUaElK6ur4ZwU3pRP63VMWXey8rfRecgRgnqt35ePjtUguSfq 1bgozrguSV8lafopz_V3H0XmfE4ggcINylY_yNFDAGtQySg5nSqrLIBZ047fxf6tIZVg0PWmLuqu DTFMlmH5XQqw.xSqmp_Z_yGgoOiHehwW5KaPbfonXkSneUvD9D2tPvAnKltjaB3VbQR3Ds1.nVCt J442uLTVBAWO.Uxy3moTHMw_.dV9MTb36Z0Obm.9q8GSR41Abzl3mU2AVNczXZnWwIcB5JhRd_dC .rjeR16kqn2vojcq8sXxYQeTGre2tDLy8EsWTCvVTe31iwAabPjbRercMgg6iYQHCnRbJwhP8Zo3 5SevFbBD3byRkcr6J9dNR2_16BCZyYG3E9NbfDgX4kaFJDWGZTJSjoyfSpk7YyxlWsSF190rBSiP .hqiEqCdGdiQvU_Y2xJDMQsMmOX0fhtVfpqE8Uo5zdwu0f4.YEoeOhw4OpnWBsY3KIv7r0sUauHG TbQZkmjPtjoKQCzCdp7woJb9vZl4lLmhZJx5Zcx5XmCuFEE0UgwUJkoFX3uvNOtG8ytH6tQ2D2cr K2aumoefKdmOC5AeNatWXLSprTOwU0arpI6a0zRTdjLbYCtOkwOzTWMpAeOUJoU3TceOzOT3mQOk 1h.sO.7zd3qbTX80YLYfQDuVEjch0tDBu53_OnWQIwr4Sj51uxmrURCVuah9uwpy77XkElUSfkXy vk9cMUnW6L.IbtTDfj.y9dWXkp0JJuCHzaiLR6isR66ETxkwL0b7HKTwovNMVcjWPeC3hP_T7TaN aK5i7SkutYz7xK636f3ME_qH1msnInfHMY5YI0NAJezSC8mkaX6s4hCMpaO.GbnrKjrWOPEv5oVS 59WCi9l8unE1Juc8- Received: from sonic.gate.mail.ne1.yahoo.com by sonic311.consmr.mail.ir2.yahoo.com with HTTP; Sun, 31 May 2020 14:58:49 +0000 Date: Sun, 31 May 2020 14:58:42 +0000 (UTC) From: Hannes Domani To: Gdb-patches Message-ID: <235118837.781199.1590937122739@mail.yahoo.com> In-Reply-To: References: <20200530161253.61299-1-ssbssa.ref@yahoo.de> <20200530161253.61299-1-ssbssa@yahoo.de> <1980167901.498897.1590883592361@mail.yahoo.com> Subject: Re: [PATCH v2][PR gdb/24052] Implement 'set print zero-values on|off' MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Mailer: WebService/1.1.16037 YMailNorrin Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:76.0) Gecko/20100101 Firefox/76.0 X-Spam-Status: No, score=-6.6 required=5.0 tests=BAYES_00, BODY_8BITS, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, 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 14:58:53 -0000 Am Sonntag, 31. Mai 2020, 15:39:05 MESZ hat Pedro Alves Folgendes geschrieben: > On 5/31/20 1:06 AM, Hannes Domani via Gdb-patches wrote: > >=C2=A0 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.=C2=A0 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 =3D -1, BAR =3D 0, QUX =3D 1 }; > >> > >> From looking at the implementation it seems to me like zero-values off > >> makes us not print a BAR value.=C2=A0 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.=C2=A0 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.=C2=A0 This is used for >=C2=A0=C2=A0=C2=A0=C2=A0 set/show commands in which the options are on/off= /automatic.=C2=A0 */ > enum auto_boolean > { >=C2=A0=C2=A0 AUTO_BOOLEAN_TRUE, >=C2=A0=C2=A0 AUTO_BOOLEAN_FALSE, >=C2=A0=C2=A0 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 >=C2=A0=C2=A0 { >=C2=A0=C2=A0=C2=A0=C2=A0 language_mode_auto, language_mode_manual >=C2=A0=C2=A0 } > language_mode; > > What's the significance of hiding auto but not manual? > > Here: > > /* alignment enum */ > enum ui_align >=C2=A0=C2=A0 { >=C2=A0=C2=A0=C2=A0=C2=A0 ui_left =3D -1, >=C2=A0=C2=A0=C2=A0=C2=A0 ui_center, >=C2=A0=C2=A0=C2=A0=C2=A0 ui_right, >=C2=A0=C2=A0=C2=A0=C2=A0 ui_noalign >=C2=A0=C2=A0 }; > > 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 =3D 1, BAR_BIT =3D 2, QUX_BIT =3D 4 }; > >> > >> Here's where you can check whether you have a flag enum: > >> > >> /* * True if this type is a "flag" enum.=C2=A0 A flag enum is one wher= e all > >>=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 the values are pairwise disjoint when "a= nd"ed together.=C2=A0 This > >>=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 affects how enum values are printed.=C2= =A0 */ > >> > >> #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 > > { > >=C2=A0=C2=A0 int x, y; > > }; > > struct line > > { > >=C2=A0=C2=A0 point start; > >=C2=A0=C2=A0 point end; > > }; > > line l =3D { {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.bas= e/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 @@ >=C2=A0=C2=A0=C2=A0=C2=A0 You should have received a copy of the GNU Genera= l Public License >=C2=A0=C2=A0=C2=A0=C2=A0 along with this program.=C2=A0 If not, see .=C2=A0 */ > > +#include > + > int ix; > struct two > { > @@ -33,8 +35,29 @@ struct t >=C2=A0=C2=A0=C2=A0=C2=A0 {0, 1, 2, 0, 0, 3, 4, 5, 0, 6}, {0, 0, 0}, >=C2=A0=C2=A0=C2=A0=C2=A0 {0, &ix, 0, 0, &t1.i1}}; > > +struct padding > +{ > +=C2=A0 char c; > +=C2=A0 int i; > +}; > + > +struct outer > +{ > +=C2=A0 struct padding pad1; > +=C2=A0 struct padding pad2; > +}; > + > +struct outer g_out; > + > int > main () > { > +=C2=A0 struct outer out; > +=C2=A0 memset (&out, 0xff, sizeof (out)); > +=C2=A0 out.pad1.c =3D 0; > +=C2=A0 out.pad1.i =3D 0; > +=C2=A0 out.pad2.c =3D 0; > +=C2=A0 out.pad2.i =3D 0; > + >=C2=A0=C2=A0 return 0; > } > > -- > 2.14.5 > > Now run to the "return 0;" line, and see: > > (gdb) p g_out > $1 =3D {pad1 =3D {c =3D 0 '\000', i =3D 0}, pad2 =3D {c =3D 0 '\000', i = =3D 0}} > (gdb) p out > $2 =3D {pad1 =3D {c =3D 0 '\000', i =3D 0}, pad2 =3D {c =3D 0 '\000', i = =3D 0}} > > (gdb) p -zero-values off -- g_out > $3 =3D {} > (gdb) p -zero-values off -- out > $4 =3D {pad1 =3D {}, pad2 =3D {}} > > 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".=C2=A0 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, > >>>=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 && field_is_static (&type->field (i))) > >>>=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 continue; > >>> > >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* If requested, skip printing of zer= o value fields.=C2=A0 */ > >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (!options->zero_value_print > >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 && !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.=C2=A0 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.=C2=A0 :-) ) > > > > It doesn't skip static fields, they are just not checked for a zero-val= ue. > > And that was intentional, mainly for the same reason as above. > > I don't get it.=C2=A0 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=C2=A0 | 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.bas= e/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 >=C2=A0=C2=A0 int ia[10]; >=C2=A0=C2=A0 int ea[3]; >=C2=A0=C2=A0 int *ipa[5]; > + > +=C2=A0 static int static_field; > + > } t1 =3D {0, 0, 1, 0, 2.5, 0, &ix, 0, 0, {0, 0}, {3, 0}, {4, 5}, >=C2=A0=C2=A0=C2=A0=C2=A0 {0, 1, 2, 0, 0, 3, 4, 5, 0, 6}, {0, 0, 0}, >=C2=A0=C2=A0=C2=A0=C2=A0 {0, &ix, 0, 0, &t1.i1}}; > > +int t::static_field =3D 0; > + > struct padding > { >=C2=A0=C2=A0 char c; > diff --git a/gdb/testsuite/gdb.base/zero-values.exp b/gdb/testsuite/gdb.b= ase/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++}]} = { >=C2=A0=C2=A0=C2=A0=C2=A0 untested $testfile.exp >=C2=A0=C2=A0=C2=A0=C2=A0 return -1 > } > > -- > 2.14.5 > > And now observe: > > (gdb) p t1 > $1 =3D {i1 =3D 0, i2 =3D 0, i3 =3D 1, d1 =3D 0, d2 =3D 2.5, d3 =3D 0, p1 = =3D 0x601110 , p2 =3D 0x0, p3 =3D 0x0, t1 =3D {v1 =3D 0, v2 =3D 0}, t2 = =3D {v1 =3D 3, v2 =3D 0}, t3 =3D {v1 =3D 4, v2 =3D 5}, ia =3D {0, 1, 2, 0, = 0, 3, 4, 5, 0, 6}, ea =3D {0, 0, 0}, ipa =3D {0x0, 0x601110 , 0x0, 0x0,= 0x601040 }, static static_field =3D 0} > > (gdb) p -zero-values off -- t1 > $2 =3D {i3 =3D 1, d2 =3D 2.5, p1 =3D 0x601110 , t2 =3D {v1 =3D 3}, t3= =3D {v1 =3D 4, v2 =3D 5}, ia =3D {1, 2, 3, 4, 5, 6}, ipa =3D {0x601110 , 0x601040 }, static static_field =3D 0} > > Why print "static_field =3D 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 k= ind 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, > >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0 struct cmd_list_element *c, > >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0 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 > >=C2=A0=C2=A0 This controls whether GDB prints zero value members of stru= ctures and > >=C2=A0=C2=A0 arrays.=C2=A0 The default is 'on'. > Yes, something like that.=C2=A0 I guess also for unions.=C2=A0 You should= add a > test for unions too, btw. OK. > Note that the documentation bits will be reviewed/approved > by Eli. Hannes