From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from sonic308-19.consmr.mail.ir2.yahoo.com (sonic308-19.consmr.mail.ir2.yahoo.com [77.238.178.147]) by sourceware.org (Postfix) with ESMTPS id 8D3B83851C04 for ; Sun, 31 May 2020 00:06:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 8D3B83851C04 X-YMail-OSG: SSJbs4gVM1kjLn_3POYrv7L39zvHuTe0S91Cga1yYEPOs9YeZO9Orqp.GKOPrSZ fuQr9hxgnsm1PnodqaxhjI8xjnYdc5K7SMvQBfe2jTr0hdTw9CMsHw.dChg0cE8KJ24gvXsTHcSo UM6SYkWvmMeoj1v6g5YsvNgM0IP.qYlMGPurx.zjBPo5VCuE8epqqz543KaZr.oMTiqJOU6MDDte PH5vWnn6yDLUANnvIvGjv7V2rEEDrq0nLT0RPQsdXHUjeVEpZDhJWv0u1iVAcWhPGSwalN0FhJjm js1.JF9CA8avqwpVfXRX8RdguCw893rtfdfAkfe15t3PC50bB5xWwXgmQ8i.LRvtRSjUq_67np74 fr8aE0QaRUUmN9QrEzi9J98lP4PYrEotu7D2tEdIEnqrCnzBGiwstE5qHgSUUChLvoy6rtnSRG4J zn4hKgvqApf9RS_oG8blxpjSEkV7.RX_9IyGGDYolq.3Yw_XnqXLzw_yEsHXVfNbeY9HZO1TGeBW w01TuNnoB8CqeX0jeUInZ7WbTNA5pXVs6C48j4sINAHwbCtydZ65c4jzPV2ss_h7R0shiBrXNayv ZEeS_wxpCUc9wUsO1kzCI3Ullsu_Tat1osB9G0gZRuVRFryrGBHYhGVhAZS3YCMaNJ6eRBBIBO3M bUDn8GyI_asPgsksJ7grXESWISxpfRra70jcC_iFiBf84Dt0m4Rs33TefYob08E65EPGKPL8a7b5 2va2NO.dsyeuAXgz_X2vkpl0lwyFsUa5OAjTkiav1W36XGQ3kM3NQSaADeKsWwgVKZON4g5SQ8g6 cSvNYDtbIMdIQyh5gVPrOil.L5P_eh3.MJDF0hHN0HdwQf47EbZH46NHTukVIQ2JexfTEqlq2iFw iWzUfx0g1Kb6to0TMX.N3B2loE49YR8LOidfvSgT7Jkq6xnSkPw.nA8w1mKfHO8FrK85Kw4Q0LAo M3btNlvAbeL6OX9OmZpXo272xcOsZqf_m16WwIzsq8Lnt_LmolAwH4JLC7ZIPxvfdzc3DOyCGeq6 uqyOKohgfIkxsq9b9ZHnunQB7La1U9cj1HaGpeECceFCqxcwr0kawmFm_owQAqWxG4_6CUN4OLOO DkcG2pdUcvHDMwkXbjLvA1Wo94dk5zpZv6uUKWjPkpP9xgaJ_1NIpPLgdmhqbe_KsDd_5FQhK4C7 ul_OvOSTud0a3G34wmomSHOP3ajePdR7kjsflCYSixbskJEhdAdrrd2I0wr3beYgzdfAigCUBq7e _MY9hCP3JzfBp5m1bWfa7npQ8A_M4OVpl_QwatPL0VCo2PfQMEoMF2Jd1TYv0zCakpXEnQ6VOM6J bPPOnmcORNRng5DhyEg-- Received: from sonic.gate.mail.ne1.yahoo.com by sonic308.consmr.mail.ir2.yahoo.com with HTTP; Sun, 31 May 2020 00:06:33 +0000 Date: Sun, 31 May 2020 00:06:32 +0000 (UTC) From: Hannes Domani To: Gdb-patches Message-ID: <1980167901.498897.1590883592361@mail.yahoo.com> In-Reply-To: References: <20200530161253.61299-1-ssbssa.ref@yahoo.de> <20200530161253.61299-1-ssbssa@yahoo.de> 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=-3.5 required=5.0 tests=BAYES_00, BODY_8BITS, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, 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 00:06:36 -0000 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. > 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 where a= ll >=C2=A0=C2=A0=C2=A0=C2=A0 the values are pairwise disjoint when "and"ed tog= ether.=C2=A0 This >=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 int x, y; }; struct line { =C2=A0 point start; =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.) > > @@ -194,6 +194,18 @@ cp_print_value_fields (struct value *val, struct u= i_file *stream, > >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 && fi= eld_is_static (&type->field (i))) > >=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 zero = 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_st= atic (&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-value. And that was intentional, mainly for the same reason as above. > > +bool > > +value_is_zero (struct value *v) > > +{ > > +=C2=A0 struct type *type =3D check_typedef (value_type (v)); > > +=C2=A0 const gdb_byte *addr =3D value_contents_for_printing (v); > > +=C2=A0 unsigned int len =3D TYPE_LENGTH (type); > > +=C2=A0 unsigned int zeros; > > +=C2=A0 for (zeros =3D 0; zeros < len; zeros++) > > Write: > >=C2=A0=C2=A0 for (unsigned int zeros =3D 0; zeros < len; zeros++) > > But I have to say that I find it weird to name the iterator > variable "zeros".=C2=A0 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. > > > +=C2=A0=C2=A0=C2=A0 { > > You can drop this level of braces. > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (addr[zeros] !=3D 0) > > +=C2=A0=C2=A0=C2=A0 return false; > > +=C2=A0=C2=A0=C2=A0 } > > +=C2=A0 return true; > > +/* Controls printing of zero value members.=C2=A0 */ > > +static void > > There should be an empty line after the comment. OK. > > +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 This controls whether GDB prints zero value members of structures an= d =C2=A0 arrays.=C2=A0 The default is 'on'. Hannes