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

[Luis Machado]

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.