Re: [PATCH 2/5] gdb: fix printing of flag enums with multi-bit enumerators

[Luis Machado <luis.machado@linaro.org>]

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.