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

  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

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).

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

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.
  

>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:

Simon> I have come across some uses cases where it would be desirable to treat
Simon> an enum that has duplicate values as a "flag enum".  For example, this
Simon> one here [1]:

Simon>             /* Alias for header backward compatibility. */
Simon>             MEMBARRIER_CMD_SHARED = MEMBARRIER_CMD_GLOBAL,
Simon>     };

Simon> The last enumerator is kept for backwards compatibility.  Without this
Simon> patch, this enumeration wouldn't be considered a flag enum, because two
Simon> enumerators collide.   With this patch, it would be considered a flag
Simon> enum, and the value 3 would be printed as:

Does it always choose the first enumerator?

Simon>  	  if (nbits != 0 && nbits && nbits != 1)
Simon>  	    flag_enum = 0;
Simon> -	  else if ((mask & value) != 0)
Simon> -	    flag_enum = 0;
Simon> -	  else
Simon> -	    mask |= value;

I wonder if this allows too much, though.

Maybe instead it should check for duplicate enumerator values and allow
those, while still disallowing enums with conflicts, like:

enum x {
  one = 0x11,
  two = 0x10,
  three = 0x01
};

... which probably isn't a sensible flag enum.

Tom
  

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

Tom> Maybe instead it should check for duplicate enumerator values and allow
Tom> those, while still disallowing enums with conflicts, like:

Tom> enum x {
Tom>   one = 0x11,
Tom>   two = 0x10,
Tom>   three = 0x01
Tom> };

Tom> ... which probably isn't a sensible flag enum.

You can ignore this, I see it was already mentioned in patch 2.

Tom
  

On 2020-02-18 3:38 p.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:
> 
> Simon> I have come across some uses cases where it would be desirable to treat
> Simon> an enum that has duplicate values as a "flag enum".  For example, this
> Simon> one here [1]:
> 
> Simon>             /* Alias for header backward compatibility. */
> Simon>             MEMBARRIER_CMD_SHARED = MEMBARRIER_CMD_GLOBAL,
> Simon>     };
> 
> Simon> The last enumerator is kept for backwards compatibility.  Without this
> Simon> patch, this enumeration wouldn't be considered a flag enum, because two
> Simon> enumerators collide.   With this patch, it would be considered a flag
> Simon> enum, and the value 3 would be printed as:
> 
> Does it always choose the first enumerator?

Yes.

generic_val_print_enum_1 iterates on the enum type's fields (so,
enumerators) and clears those bits in the value as it goes.  So
if multiple enumerators match, the first one in the enum type's
fields will be printed.  Is this list of fields guaranteed to be
in the same order as what's in the code though?

> 
> Simon>  	  if (nbits != 0 && nbits && nbits != 1)
> Simon>  	    flag_enum = 0;
> Simon> -	  else if ((mask & value) != 0)
> Simon> -	    flag_enum = 0;
> Simon> -	  else
> Simon> -	    mask |= value;
> 
> I wonder if this allows too much, though.
> 
> Maybe instead it should check for duplicate enumerator values and allow
> those, while still disallowing enums with conflicts, like:
> 
> enum x {
>   one = 0x11,
>   two = 0x10,
>   three = 0x01
> };
> 
> ... which probably isn't a sensible flag enum.

Not sure what you mean here.  With the current patch, this enum would not
bne marked as a flag enum, as `one` has multiple bits set.

Simon
  

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> Is this list of fields guaranteed to be
Simon> in the same order as what's in the code though?

Yeah, should be, of course subject to what the compiler decides.

Simon> Not sure what you mean here.  With the current patch, this enum would not
Simon> bne marked as a flag enum, as `one` has multiple bits set.

I read the patches in the wrong order.  FAOD this one also looks good to me.


Tom
  

On 2020-02-18 4:57 p.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
> 
> Simon> Is this list of fields guaranteed to be
> Simon> in the same order as what's in the code though?
> 
> Yeah, should be, of course subject to what the compiler decides.
> 
> Simon> Not sure what you mean here.  With the current patch, this enum would not
> Simon> bne marked as a flag enum, as `one` has multiple bits set.
> 
> I read the patches in the wrong order.  FAOD this one also looks good to me.

Thanks, I'll push the series then.

Simon