| 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);
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(-)