From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21296 invoked by alias); 17 Feb 2020 10:56:31 -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 21288 invoked by uid 89); 17 Feb 2020 10:56:30 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-21.2 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy=UD:count-one-bits.h, sk:counto, sk:count_o, sk:count-o X-HELO: mail-qk1-f195.google.com Received: from mail-qk1-f195.google.com (HELO mail-qk1-f195.google.com) (209.85.222.195) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 17 Feb 2020 10:56:28 +0000 Received: by mail-qk1-f195.google.com with SMTP id h4so15788178qkm.0 for ; Mon, 17 Feb 2020 02:56:27 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=subject:to:references:from:message-id:date:user-agent:mime-version :in-reply-to:content-language:content-transfer-encoding; bh=MXdoRCnfJINqLdF3EzCAkLC3P/+SEFlb5JR3QBWuPew=; b=BBV8yp9NdSwZ+KHBa48tVqHGOSr0ofkhkqhjapWf8Vb2bkGHc5SncRm2wNr0bOIOO0 5Z9qOhYXwyGAIXQgqPigvV86OGkvkxYQDp6bL/aZDU0I4/jLH5WmFIJ31gBALTgUX5NB q1YRr5JFa+sX0LN9UrPwwZyMU76MFdiILdsEpceEGO9yXclQ7BJwF5KytQPqQbReXL56 f54NjWkHFTehenlZwvFF9lOwQu+LggBTfncBrQ4TKuXTNP/+9xS2TjkNhtDHl2eQ3mYh znEP2ALuuMbB0WMuaQyzoNzFUJ7UFoK8CGvv5g/ETHVsqE8tbQRvEkVf8HxqqfBxkkfA Tv6g== Return-Path: Received: from [192.168.0.185] ([191.34.156.101]) by smtp.gmail.com with ESMTPSA id y27sm8974943qta.50.2020.02.17.02.56.23 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 17 Feb 2020 02:56:24 -0800 (PST) Subject: Re: [PATCH 2/5] gdb: fix printing of flag enums with multi-bit enumerators To: Simon Marchi , gdb-patches@sourceware.org References: <20200213203035.30157-1-simon.marchi@efficios.com> <20200213203035.30157-2-simon.marchi@efficios.com> From: Luis Machado Message-ID: <2e83ba29-99c4-d611-45ec-ee98ac81fc34@linaro.org> Date: Mon, 17 Feb 2020 10:56: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: <20200213203035.30157-2-simon.marchi@efficios.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2020-02/txt/msg00610.txt.bz2 On 2/13/20 5:30 PM, Simon Marchi wrote: > GDB has this feature where if an enum looks like it is meant to > represent binary flags, it will present the values of that type as a > bitwise OR of the flags that are set in the value. > > The original motivation for this patch is to fix this behavior: > > enum hello { AAA = 0x1, BBB = 0xf0 }; > > (gdb) p (enum hello) 0x11 > $1 = (AAA | BBB) > > This is wrong because the bits set in BBB (0xf0) are not all set in the > value 0x11, but GDB presents it as if they all were. > > I think that enumerations with enumerators that have more than one bit > set should simply not qualify as "flag enum", as far as this > heuristic is concerned. I'm not sure what it means to have flags of > more than one bit. So this is what this patch implements. > > I have added an assert in generic_val_print_enum_1 to make sure the flag > enum types respect that, in case they are used by other debug info > readers, in the future. > > I've enhanced the gdb.base/printcmds.exp test to cover this case. I've > also added tests for printing flag enums with value 0, both when the > enumeration has and doesn't have an enumerator for value 0. > > gdb/ChangeLog: > > * dwarf2/read.c: Include "count-one-bits.h". > (update_enumeration_type_from_children): If an enumerator has > multiple bits set, don't treat the enumeration as a "flag enum". > * valprint.c (generic_val_print_enum_1): Assert that enumerators > of flag enums have 0 or 1 bit set. > > gdb/testsuite/ChangeLog: > > * gdb.base/printcmds.c (enum flag_enum): Prefix enumerators with > FE_, add FE_NONE. > (three): Update. > (enum flag_enum_without_zero): New enum. > (flag_enum_without_zero): New variable. > (enum not_flag_enum): New enum. > (three_not_flag): New variable. > * gdb.base/printcmds.exp (test_artificial_arrays): Update. > (test_print_enums): Add more tests for printing flag enums. > --- > gdb/dwarf2/read.c | 14 ++++++++++--- > gdb/testsuite/gdb.base/printcmds.c | 30 ++++++++++++++++++++++++++-- > gdb/testsuite/gdb.base/printcmds.exp | 20 ++++++++++++++++--- > gdb/valprint.c | 8 +++++++- > 4 files changed, 63 insertions(+), 9 deletions(-) > > diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c > index 7edbd9d7dfa4..b866cc2d5747 100644 > --- a/gdb/dwarf2/read.c > +++ b/gdb/dwarf2/read.c > @@ -82,6 +82,7 @@ > #include "gdbsupport/selftest.h" > #include "rust-lang.h" > #include "gdbsupport/pathstuff.h" > +#include "count-one-bits.h" > > /* When == 1, print basic high level tracing messages. > When > 1, be more verbose. > @@ -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? > + 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? > +/* Not a flag enum, an enumerator value has multiple bits sets. */ > +enum not_flag_enum > +{ > + NFE_ONE = 0x01, > + NFE_TWO = 0x02, > + NFE_F0 = 0xf0, > +}; > > -enum flag_enum three = ONE | TWO; > +enum not_flag_enum three_not_flag = NFE_ONE | NFE_TWO; > > /* A structure with an embedded array at an offset > 0. The array has > all elements with the same repeating value, which must not be the > diff --git a/gdb/testsuite/gdb.base/printcmds.exp b/gdb/testsuite/gdb.base/printcmds.exp > index 6e98b7943ba3..6afb965af066 100644 > --- a/gdb/testsuite/gdb.base/printcmds.exp > +++ b/gdb/testsuite/gdb.base/printcmds.exp > @@ -653,9 +653,9 @@ proc test_artificial_arrays {} { > gdb_test_escape_braces "p int1dim\[0\]${ctrlv}@2${ctrlv}@3" \ > "({{0, 1}, {2, 3}, {4, 5}}|\[Cc\]annot.*)" \ > {p int1dim[0]@2@3} > - gdb_test_escape_braces "p int1dim\[0\]${ctrlv}@TWO" " = {0, 1}" \ > + gdb_test_escape_braces "p int1dim\[0\]${ctrlv}@FE_TWO" " = {0, 1}" \ > {p int1dim[0]@TWO} > - gdb_test_escape_braces "p int1dim\[0\]${ctrlv}@TWO${ctrlv}@three" \ > + gdb_test_escape_braces "p int1dim\[0\]${ctrlv}@FE_TWO${ctrlv}@three" \ > "({{0, 1}, {2, 3}, {4, 5}}|\[Cc\]annot.*)" \ > {p int1dim[0]@TWO@three} > gdb_test_escape_braces {p/x (short [])0x12345678} \ > @@ -736,7 +736,21 @@ proc test_print_enums {} { > # Regression test for PR11827. > gdb_test "print some_volatile_enum" "enumvolval1" > > - gdb_test "print three" " = \\\(ONE \\| TWO\\\)" > + # Print a flag enum. > + gdb_test "print three" [string_to_regexp " = (FE_ONE | FE_TWO)"] > + > + # Print a flag enum with value 0, where an enumerator has value 0. > + gdb_test "print (enum flag_enum) 0x0" [string_to_regexp " = FE_NONE"] > + > + # Print a flag enum with value 0, where no enumerator has value 0. > + gdb_test "print flag_enum_without_zero" [string_to_regexp " = (unknown: 0)"] > + > + # Print a flag enum with unknown bits set. > + gdb_test "print (enum flag_enum) 0xf1" [string_to_regexp " = (FE_ONE | unknown: 240)"] > + > + # Test printing an enum not considered a "flag enum" (because one of its > + # enumerators has multiple bits set). > + gdb_test "print three_not_flag" [string_to_regexp " = 3"] > } > > proc test_printf {} { > diff --git a/gdb/valprint.c b/gdb/valprint.c > index f26a87da3bd4..77b9a4993d79 100644 > --- a/gdb/valprint.c > +++ b/gdb/valprint.c > @@ -39,6 +39,7 @@ > #include "cli/cli-option.h" > #include "gdbarch.h" > #include "cli/cli-style.h" > +#include "count-one-bits.h" > > /* Maximum number of wchars returned from wchar_iterate. */ > #define MAX_WCHARS 4 > @@ -638,7 +639,12 @@ generic_val_print_enum_1 (struct type *type, LONGEST val, > { > QUIT; > > - if ((val & TYPE_FIELD_ENUMVAL (type, i)) != 0) > + ULONGEST enumval = TYPE_FIELD_ENUMVAL (type, i); > + int nbits = count_one_bits_ll (enumval); > + > + gdb_assert (nbits == 0 || nbits == 1); > + > + if ((val & enumval) != 0) > { > if (!first) > fputs_filtered (" | ", stream); > Otherwise LGTM.