From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.120]) by sourceware.org (Postfix) with ESMTP id D93C93858D35 for ; Sat, 30 May 2020 18:00:10 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org D93C93858D35 Received: from mail-wr1-f70.google.com (mail-wr1-f70.google.com [209.85.221.70]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-231-iFP0CWX5MkqrXTRqHSbJew-1; Sat, 30 May 2020 14:00:08 -0400 X-MC-Unique: iFP0CWX5MkqrXTRqHSbJew-1 Received: by mail-wr1-f70.google.com with SMTP id j16so2428035wre.22 for ; Sat, 30 May 2020 11:00:08 -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=g7UOf8C2UMXCM3qCkuk4aMo2urObNI62feBxRduVsG8=; b=neF1pIjkUrokWAlViX7LxujKNL7SZLju2y4pCeab0IYqqEsW9KvkSpRTARWChy5/uQ gtxKjPHzPaaw+QSApFhjliHiRIEKh72qNcATy3SRh5+Kq8gO7K+KjtSCNYScjidXP3+q CPtZukuqb3FydvTTPyUo5Zk4bB01Ss3XVNXsSrADvrHB1x/R8/JqVaEa9l4Gs9Ecm5s2 tZdTH59s+QFUEcs+X7OYRU0LDTrxhUYI4ay5EBK+fwfcdmnx5aIV4q9q7zBiacILGj7C BceX09j/8PNIhe8o3ua7YoYaZ9jWAGyFeT70N1uAEDkrujRJaO2XHKmvgF5cTIZ/TtJ+ 1rDw== X-Gm-Message-State: AOAM531T23/gOtn+1vdkGlx8hoySK+2HVNMvb3FBApiIJ4zVGhLMu2Hi hUaJKXBnzd81i98YU4WXC8qfeIf88oX++0gvAB7jZUXH12OEk728aOqa4V1nm94iywAsKFUiVMj 7JAk6kEHi9NnE7As74wwtCg== X-Received: by 2002:a05:600c:218b:: with SMTP id e11mr13992645wme.162.1590861606884; Sat, 30 May 2020 11:00:06 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzo2dlZbJ35tH6T/AUVPOflzCLA816IPS4PU2/CbQRKvCmvcnfpzOyeXXkYmY+23nACRHeinQ== X-Received: by 2002:a05:600c:218b:: with SMTP id e11mr13992626wme.162.1590861606603; Sat, 30 May 2020 11:00:06 -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 o10sm11911215wrj.37.2020.05.30.11.00.05 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 30 May 2020 11:00:05 -0700 (PDT) Subject: Re: [PATCH v2][PR gdb/24052] Implement 'set print zero-values on|off' To: Hannes Domani , gdb-patches@sourceware.org References: <20200530161253.61299-1-ssbssa.ref@yahoo.de> <20200530161253.61299-1-ssbssa@yahoo.de> From: Pedro Alves Message-ID: Date: Sat, 30 May 2020 19:00:04 +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: <20200530161253.61299-1-ssbssa@yahoo.de> Content-Language: en-US X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-5.2 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, 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: Sat, 30 May 2020 18:00:12 -0000 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 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. > @@ -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. :-) ) > +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. > +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. Thanks, Pedro Alves