[5/5] gdb: change print format of flag enums with value 0

Message ID 20200213203035.30157-5-simon.marchi@efficios.com
State New
Headers show
Series
  • [1/5] gnulib: import count-one-bits module and use it
Related show

Commit Message

Simon Marchi Feb. 13, 2020, 8:30 p.m.
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(-)

-- 
2.25.0

Comments

Luis Machado Feb. 17, 2020, 12:08 p.m. | #1
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.
Simon Marchi Feb. 17, 2020, 7:02 p.m. | #2
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
Tom Tromey Feb. 18, 2020, 8:45 p.m. | #3
>>>>> "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
Simon Marchi Feb. 18, 2020, 8:52 p.m. | #4
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

Patch

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