Message ID | 20200213203035.30157-2-simon.marchi@efficios.com |
---|---|
State | New |
Headers | show |
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.
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
On 2/17/20 2:27 PM, Simon Marchi wrote: > 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". I did some research and did not see a clear reason why popcount returns an int. There was a mention of popcount returning negative for some obscure implementation if it was passed a negative number. GCC's documentation doesn't make it clear either. > > 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". Sorry, i should've been more clear. I meant the comment saying "but with enumerator with value". You guessed it right.
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) + 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, +}; + +enum flag_enum_without_zero flag_enum_without_zero = 0; + +/* 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);