[5/5] gdb: change print format of flag enums with value 0
Commit Message
If a flag enum has value 0 and the enumeration type does not have an
enumerator with value 0, we currently print:
$1 = (unknown: 0x0)
I don't like the display of "unknown" here, since for flags, 0 is a
an expected value. It just means that no flags are set. This patch
makes it so that we print it as a simple 0 in this situation:
$1 = 0
If there is an enumerator with value 0, it is still printed using that
enumerator, for example (from the test):
$1 = FE_NONE
gdb/ChangeLog:
* valprint.c (generic_val_print_enum_1): When printing a flag
enum with value 0 and there is no enumerator with value 0, print
just "0" instead of "(unknown: 0x0)".
gdb/testsuite/ChangeLog:
* gdb.base/printcmds.exp (test_print_enums): Update expected
output.
---
gdb/testsuite/gdb.base/printcmds.exp | 2 +-
gdb/valprint.c | 31 +++++++++++++++++++++-------
2 files changed, 25 insertions(+), 8 deletions(-)
Comments
On 2/13/20 5:30 PM, Simon Marchi wrote:
> If a flag enum has value 0 and the enumeration type does not have an
> enumerator with value 0, we currently print:
>
> $1 = (unknown: 0x0)
>
> I don't like the display of "unknown" here, since for flags, 0 is a
> an expected value. It just means that no flags are set. This patch
> makes it so that we print it as a simple 0 in this situation:
Should we print "no flags set" alongside the 0 for this case then?
>
> $1 = 0
>
> If there is an enumerator with value 0, it is still printed using that
> enumerator, for example (from the test):
>
> $1 = FE_NONE
>
> gdb/ChangeLog:
>
> * valprint.c (generic_val_print_enum_1): When printing a flag
> enum with value 0 and there is no enumerator with value 0, print
> just "0" instead of "(unknown: 0x0)".
>
> gdb/testsuite/ChangeLog:
>
> * gdb.base/printcmds.exp (test_print_enums): Update expected
> output.
> ---
> gdb/testsuite/gdb.base/printcmds.exp | 2 +-
> gdb/valprint.c | 31 +++++++++++++++++++++-------
> 2 files changed, 25 insertions(+), 8 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.base/printcmds.exp b/gdb/testsuite/gdb.base/printcmds.exp
> index d6f5c75650bf..bd2afc8696f0 100644
> --- a/gdb/testsuite/gdb.base/printcmds.exp
> +++ b/gdb/testsuite/gdb.base/printcmds.exp
> @@ -743,7 +743,7 @@ proc test_print_enums {} {
> 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: 0x0)"]
> + gdb_test "print flag_enum_without_zero" [string_to_regexp " = 0"]
>
> # Print a flag enum with unknown bits set.
> gdb_test "print (enum flag_enum) 0xf1" [string_to_regexp " = (FE_ONE | unknown: 0xf0)"]
> diff --git a/gdb/valprint.c b/gdb/valprint.c
> index bd21be69e1bf..6e62d420c0f4 100644
> --- a/gdb/valprint.c
> +++ b/gdb/valprint.c
> @@ -634,7 +634,6 @@ generic_val_print_enum_1 (struct type *type, LONGEST val,
> /* We have a "flag" enum, so we try to decompose it into
> pieces as appropriate. A flag enum has disjoint
> constants by definition. */
> - fputs_filtered ("(", stream);
> for (i = 0; i < len; ++i)
> {
> QUIT;
> @@ -646,24 +645,42 @@ generic_val_print_enum_1 (struct type *type, LONGEST val,
>
> if ((val & enumval) != 0)
> {
> - if (!first)
> + if (first)
> + {
> + fputs_filtered ("(", stream);
> + first = 0;
> + }
> + else
> fputs_filtered (" | ", stream);
> - first = 0;
>
> val &= ~TYPE_FIELD_ENUMVAL (type, i);
> fputs_filtered (TYPE_FIELD_NAME (type, i), stream);
> }
> }
>
> - if (first || val != 0)
> + if (val != 0)
> {
> - if (!first)
> + /* There are leftover bits, print them. */
> + if (first)
> + fputs_filtered ("(", stream);
> + else
> fputs_filtered (" | ", stream);
> +
> fputs_filtered ("unknown: 0x", stream);
> print_longest (stream, 'x', 0, val);
> + fputs_filtered (")", stream);
> + }
> + else if (first)
> + {
> + /* Nothing has been printed and the value is 0, the enum value must
> + have been 0. */
> + fputs_filtered ("0", stream);
> + }
> + else
> + {
> + /* Something has been printed, close the parenthesis. */
> + fputs_filtered (")", stream);
> }
> -
> - fputs_filtered (")", stream);
> }
> else
> print_longest (stream, 'd', 0, val);
>
Otherwise LGTM.
On 2020-02-17 7:08 a.m., Luis Machado wrote:
> On 2/13/20 5:30 PM, Simon Marchi wrote:
>> If a flag enum has value 0 and the enumeration type does not have an
>> enumerator with value 0, we currently print:
>>
>> $1 = (unknown: 0x0)
>>
>> I don't like the display of "unknown" here, since for flags, 0 is a
>> an expected value. It just means that no flags are set. This patch
>> makes it so that we print it as a simple 0 in this situation:
>
> Should we print "no flags set" alongside the 0 for this case then?
I don't know, I think that 0 is the natural and standard thing to print
here. If you had a flags variable in your code, you'd initialize it with
my_flags = 0;
So if GDB prints that, I think it's clear.
Simon
>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:
Simon> gdb/ChangeLog:
Simon> * valprint.c (generic_val_print_enum_1): When printing a flag
Simon> enum with value 0 and there is no enumerator with value 0, print
Simon> just "0" instead of "(unknown: 0x0)".
This looks reasonable. Thanks.
Simon> /* We have a "flag" enum, so we try to decompose it into
Simon> pieces as appropriate. A flag enum has disjoint
Simon> constants by definition. */
I guess this comment is stale now, because an earlier patch changed it
so that identical constants are allowed.
Tom
On 2020-02-18 3:45 p.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:
>
> Simon> gdb/ChangeLog:
>
> Simon> * valprint.c (generic_val_print_enum_1): When printing a flag
> Simon> enum with value 0 and there is no enumerator with value 0, print
> Simon> just "0" instead of "(unknown: 0x0)".
>
> This looks reasonable. Thanks.
>
> Simon> /* We have a "flag" enum, so we try to decompose it into
> Simon> pieces as appropriate. A flag enum has disjoint
> Simon> constants by definition. */
>
> I guess this comment is stale now, because an earlier patch changed it
> so that identical constants are allowed.
Right, I would then change this comment to this in patch 3/5:
634 /* We have a "flag" enum, so we try to decompose it into pieces as
635 appropriate. The enum may have multiple enumerators representing
636 the same bit, in which case we choose to only print the first one
637 we find. */
Simon
@@ -743,7 +743,7 @@ proc test_print_enums {} {
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: 0x0)"]
+ gdb_test "print flag_enum_without_zero" [string_to_regexp " = 0"]
# Print a flag enum with unknown bits set.
gdb_test "print (enum flag_enum) 0xf1" [string_to_regexp " = (FE_ONE | unknown: 0xf0)"]
@@ -634,7 +634,6 @@ generic_val_print_enum_1 (struct type *type, LONGEST val,
/* We have a "flag" enum, so we try to decompose it into
pieces as appropriate. A flag enum has disjoint
constants by definition. */
- fputs_filtered ("(", stream);
for (i = 0; i < len; ++i)
{
QUIT;
@@ -646,24 +645,42 @@ generic_val_print_enum_1 (struct type *type, LONGEST val,
if ((val & enumval) != 0)
{
- if (!first)
+ if (first)
+ {
+ fputs_filtered ("(", stream);
+ first = 0;
+ }
+ else
fputs_filtered (" | ", stream);
- first = 0;
val &= ~TYPE_FIELD_ENUMVAL (type, i);
fputs_filtered (TYPE_FIELD_NAME (type, i), stream);
}
}
- if (first || val != 0)
+ if (val != 0)
{
- if (!first)
+ /* There are leftover bits, print them. */
+ if (first)
+ fputs_filtered ("(", stream);
+ else
fputs_filtered (" | ", stream);
+
fputs_filtered ("unknown: 0x", stream);
print_longest (stream, 'x', 0, val);
+ fputs_filtered (")", stream);
+ }
+ else if (first)
+ {
+ /* Nothing has been printed and the value is 0, the enum value must
+ have been 0. */
+ fputs_filtered ("0", stream);
+ }
+ else
+ {
+ /* Something has been printed, close the parenthesis. */
+ fputs_filtered (")", stream);
}
-
- fputs_filtered (")", stream);
}
else
print_longest (stream, 'd', 0, val);