From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 4096 invoked by alias); 17 Feb 2020 17:27:37 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 3480 invoked by uid 89); 17 Feb 2020 17:27:36 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-17.8 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.1 spammy=(unknown), H*M:9901, H*r:TLSv1.3, HContent-Transfer-Encoding:8bit X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 17 Feb 2020 17:27:35 +0000 Received: from [172.16.0.95] (192-222-181-218.qc.cable.ebox.net [192.222.181.218]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id C2B661E05A; Mon, 17 Feb 2020 12:27:32 -0500 (EST) Subject: Re: [PATCH 2/5] gdb: fix printing of flag enums with multi-bit enumerators To: Luis Machado , Simon Marchi , gdb-patches@sourceware.org References: <20200213203035.30157-1-simon.marchi@efficios.com> <20200213203035.30157-2-simon.marchi@efficios.com> <2e83ba29-99c4-d611-45ec-ee98ac81fc34@linaro.org> From: Simon Marchi Message-ID: <78d03fba-9901-01d5-c1ed-9631986e2579@simark.ca> Date: Mon, 17 Feb 2020 17:27:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1 MIME-Version: 1.0 In-Reply-To: <2e83ba29-99c4-d611-45ec-ee98ac81fc34@linaro.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-SW-Source: 2020-02/txt/msg00682.txt.bz2 On 2020-02-17 5:56 a.m., Luis Machado wrote: >> @@ -15526,10 +15527,17 @@ update_enumeration_type_from_children (struct die_info *die, >>         unsigned_enum = 0; >>         flag_enum = 0; >>       } >> -      else if ((mask & value) != 0) >> -    flag_enum = 0; >>         else >> -    mask |= value; >> +    { >> +      int nbits = count_one_bits_ll (value); >> + >> +      if (nbits != 0 && nbits && nbits != 1) > > Isn't this the same as nbits >= 2? popcount shouldn't return a negative number, should it? I think I wrote that because count_one_bits_ll returns a signed int, so I indeed thought "what if it returns a negative number". But if it did, there would be some quite more serious problems, so we probably don't have to think about it here. I'll change it as "nbits >= 2". Oh and there was a spurious "&& nbits" in there. > >> +        flag_enum = 0; >> +      else if ((mask & value) != 0) >> +        flag_enum = 0; >> +      else >> +        mask |= value; >> +    } >>           /* If we already know that the enum type is neither unsigned, nor >>        a flag type, no need to look at the rest of the enumerates.  */ >> diff --git a/gdb/testsuite/gdb.base/printcmds.c b/gdb/testsuite/gdb.base/printcmds.c >> index 57e04e6c01f3..f0b4fa4b86b1 100644 >> --- a/gdb/testsuite/gdb.base/printcmds.c >> +++ b/gdb/testsuite/gdb.base/printcmds.c >> @@ -96,9 +96,35 @@ enum some_volatile_enum { enumvolval1, enumvolval2 }; >>      name.  See PR11827.  */ >>   volatile enum some_volatile_enum some_volatile_enum = enumvolval1; >>   -enum flag_enum { ONE = 1, TWO = 2 }; >> +/* An enum considered as a "flag enum".  */ >> +enum flag_enum >> +{ >> +  FE_NONE = 0x00, >> +  FE_ONE  = 0x01, >> +  FE_TWO  = 0x02, >> +}; >> + >> +enum flag_enum three = FE_ONE | FE_TWO; >> + >> +/* Another enum considered as a "flag enum", but with enumerator with value >> +   0.  */ >> +enum flag_enum_without_zero >> +{ >> +  FEWZ_ONE = 0x01, >> +  FEWZ_TWO = 0x02, >> +}; >> + > > Typo maybe? There is no enum with value 0 in flag_enum_without_zero. Maybe you meant flag_enum to contain a 0 value with FE_NONE? > >> +enum flag_enum_without_zero flag_enum_without_zero = 0; >> + > > Or maybe you were referring to the above? Do you mean a typo in the comment, or the type name? Because there indeed seems to be a typo, it should read "but with no enumerator with value", not "but with enumerator with value". The type name "flag_enum_without_zero" means there is no enumerator that has value zero, is that clear? > Otherwise LGTM. Thanks for your review, I'll likely send a new version. Simon