Re: [PATCH 3/5] gdb: allow duplicate enumerators in flag enums

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

On 2/13/20 5:30 PM, Simon Marchi wrote:
> I have come across some uses cases where it would be desirable to treat
> an enum that has duplicate values as a "flag enum".  For example, this
> one here [1]:
> 
>      enum membarrier_cmd {
>              MEMBARRIER_CMD_QUERY                                = 0,
>              MEMBARRIER_CMD_GLOBAL                               = (1 << 0),
>              MEMBARRIER_CMD_GLOBAL_EXPEDITED                     = (1 << 1),
>              MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED            = (1 << 2),
>              MEMBARRIER_CMD_PRIVATE_EXPEDITED                    = (1 << 3),
>              MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED           = (1 << 4),
>              MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE          = (1 << 5),
>              MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE = (1 << 6),
> 
>              /* Alias for header backward compatibility. */
>              MEMBARRIER_CMD_SHARED = MEMBARRIER_CMD_GLOBAL,
>      };
> 
> The last enumerator is kept for backwards compatibility.  Without this
> patch, this enumeration wouldn't be considered a flag enum, because two
> enumerators collide.   With this patch, it would be considered a flag
> enum, and the value 3 would be printed as:
> 
>    MEMBARRIER_CMD_GLOBAL | MEMBARRIER_CMD_GLOBAL_EXPEDITED
> 

Sounds reasonable to me.

> Although if people prefer, we could display both MEMBARRIER_CMD_GLOBAL
> and MEMBARRIER_CMD_SHARED in the result.  It wouldn't be wrong, and
> could perhaps be useful in case a bit may have multiple meanings
> (depending on some other bit value).

It would be fine either way for me. Since it is a backwards 
compatibility value, i guess it will be less and less used as time goes 
by. Not sure if it would be great to keep displaying it when it is no 
longer so useful.

I'd keep just one value. If others want it, maybe we could enable it via 
a separate display option.

> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/membarrier.h?id=0bf999f9c5e74c7ecf9dafb527146601e5c848b9#n125
> ---
>   gdb/dwarf2/read.c                  | 5 -----
>   gdb/testsuite/gdb.base/printcmds.c | 7 ++++---
>   2 files changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index b866cc2d5747..5c34667ef36d 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -15495,7 +15495,6 @@ update_enumeration_type_from_children (struct die_info *die,
>     struct die_info *child_die;
>     int unsigned_enum = 1;
>     int flag_enum = 1;
> -  ULONGEST mask = 0;
>   
>     auto_obstack obstack;
>   
> @@ -15533,10 +15532,6 @@ update_enumeration_type_from_children (struct die_info *die,
>   
>   	  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
> diff --git a/gdb/testsuite/gdb.base/printcmds.c b/gdb/testsuite/gdb.base/printcmds.c
> index f0b4fa4b86b1..81d2efe1a037 100644
> --- a/gdb/testsuite/gdb.base/printcmds.c
> +++ b/gdb/testsuite/gdb.base/printcmds.c
> @@ -99,9 +99,10 @@ volatile enum some_volatile_enum some_volatile_enum = enumvolval1;
>   /* An enum considered as a "flag enum".  */
>   enum flag_enum
>   {
> -  FE_NONE = 0x00,
> -  FE_ONE  = 0x01,
> -  FE_TWO  = 0x02,
> +  FE_NONE       = 0x00,
> +  FE_ONE        = 0x01,
> +  FE_TWO        = 0x02,
> +  FE_TWO_LEGACY = 0x02,
>   };
>   
>   enum flag_enum three = FE_ONE | FE_TWO;
> 

LGTM.