[PATCH 2/5] gdb: fix printing of flag enums with multi-bit enumerators
Luis Machado
luis.machado@linaro.org
Mon Feb 17 10:56:00 GMT 2020
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.
More information about the Gdb-patches
mailing list