[PATCH v2][PR gdb/24052] Implement 'set print zero-values on|off'

[PATCH v2][PR gdb/24052] Implement 'set print zero-values on|off'

[Hannes Domani]

With this option it's possible to suppress any zero value members when
printing a structure or array.

Consider this example:

(gdb) p t1
$1 = {
  i1 = 0,
  i2 = 0,
  i3 = 1,
  d1 = 0,
  d2 = 2.5,
  d3 = 0,
  p1 = 0x407980 <ix>,
  p2 = 0x0,
  p3 = 0x0,
  t1 = {
    v1 = 0,
    v2 = 0
  },
  t2 = {
    v1 = 3,
    v2 = 0
  },
  t3 = {
    v1 = 4,
    v2 = 5
  },
  ia = {0, 1, 2, 0, 0, 3, 4, 5, 0, 6},
  ea = {0, 0, 0},
  ipa = {0x0, 0x407980 <ix>, 0x0, 0x0, 0x403020 <t1>}
}

With suppressed zero value members, the ouput is concise:

(gdb) set print zero-values off
(gdb) p t1
$2 = {
  i3 = 1,
  d2 = 2.5,
  p1 = 0x407980 <ix>,
  t2 = {
    v1 = 3
  },
  t3 = {
    v1 = 4,
    v2 = 5
  },
  ia = {1, 2, 3, 4, 5, 6},
  ipa = {0x407980 <ix>, 0x403020 <t1>}
}

gdb/ChangeLog:

2020-05-30  Hannes Domani  <ssbssa@yahoo.de>

	PR gdb/24052
	* cp-valprint.c (cp_print_value_fields): Skip zero value members
	if requested.
	* python/py-value.c (valpy_format_string): Add zero_values keyword.
	* valprint.c (struct value_print_options): Add zero_value_print.
	(value_is_zero): New function.
	(value_print_array_elements): Skip zero value members if requested.
	(show_zero_value_print): New function.
	* valprint.h (struct value_print_options): Add zero_value_print.
	(value_is_zero): Declare.

gdb/doc/ChangeLog:

2020-05-30  Hannes Domani  <ssbssa@yahoo.de>

	PR gdb/24052
	* gdb.texinfo: Document 'print zero-values'.
	* python.texi: Document format_string keyword 'zero_values'.

gdb/testsuite/ChangeLog:

2020-05-30  Hannes Domani  <ssbssa@yahoo.de>

	PR gdb/24052
	* gdb.base/options.exp: Add -zero-values in the print completion
	list.
	* gdb.base/zero-values.c: New test.
	* gdb.base/zero-values.exp: New file.
	* gdb.python/py-format-string.c (main): Add variables with
	zero-value members.
	* gdb.python/py-format-string.exp (test_zero_values): New proc.
	(test_all_common): Call test_zero_values.
---
v2:
- Add python format_string keyword.
- Don't print zero value array elements.
---
 gdb/cp-valprint.c                             | 12 +++++
 gdb/doc/gdb.texinfo                           | 18 ++++++-
 gdb/doc/python.texi                           |  5 ++
 gdb/python/py-value.c                         |  7 ++-
 gdb/testsuite/gdb.base/options.exp            |  1 +
 gdb/testsuite/gdb.base/zero-values.c          | 40 ++++++++++++++
 gdb/testsuite/gdb.base/zero-values.exp        | 32 +++++++++++
 gdb/testsuite/gdb.python/py-format-string.c   |  3 ++
 gdb/testsuite/gdb.python/py-format-string.exp | 17 ++++++
 gdb/valprint.c                                | 54 ++++++++++++++++++-
 gdb/valprint.h                                |  7 +++
 11 files changed, 192 insertions(+), 4 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/zero-values.c
 create mode 100644 gdb/testsuite/gdb.base/zero-values.exp

diff --git a/gdb/cp-valprint.c b/gdb/cp-valprint.c
index 0c79b025bd..6be63eea10 100644
--- a/gdb/cp-valprint.c
+++ b/gdb/cp-valprint.c
@@ -194,6 +194,18 @@ cp_print_value_fields (struct value *val, struct ui_file *stream,
 	      && field_is_static (&type->field (i)))
 	    continue;
 
+	  /* If requested, skip printing of zero value fields.  */
+	  if (!options->zero_value_print
+	      && !field_is_static (&type->field (i)))
+	    {
+	      if (TYPE_FIELD_IGNORE (type, i))
+		continue;
+
+	      struct value *v = value_primitive_field (val, 0, i, type);
+	      if (value_is_zero (v))
+		continue;
+	    }
+
 	  if (fields_seen)
 	    {
 	      fputs_filtered (",", stream);
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 84755d374e..810fe52858 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -1978,7 +1978,7 @@ on @code{-} after the command name.  For example:
 (@value{GDBP}) print -@key{TAB}@key{TAB}
 -address         -max-depth       -raw-values      -union
 -array           -null-stop       -repeats         -vtbl
--array-indexes   -object          -static-members
+-array-indexes   -object          -static-members  -zero-values
 -elements        -pretty          -symbol
 @end smallexample
 
@@ -9757,6 +9757,10 @@ Set printing of unions interior to structures.  Related setting:
 @item -vtbl [@code{on}|@code{off}]
 Set printing of C++ virtual function tables.  Related setting:
 @ref{set print vtbl}.
+
+@item -zero-values [@code{on}|@code{off}]
+Set printing of zero value members. Related setting: @ref{set print
+zero-values}.
 @end table
 
 Because the @code{print} command accepts arbitrary expressions which
@@ -11546,6 +11550,18 @@ Do not pretty print C@t{++} virtual function tables.
 
 @item show print vtbl
 Show whether C@t{++} virtual function tables are pretty printed, or not.
+
+@anchor{set print zero-values}
+@item set print zero-values
+@itemx set print zero-values on
+@cindex zero value members
+Print zero value members of structures and arrays.  The default is on.
+
+@item set print zero-values off
+Do not print zero value members of structures and arrays.
+
+@item show print zero-values
+Show whether zero value members are printed or not.
 @end table
 
 @node Pretty Printing
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index a38f1dab42..e86fb7eab9 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -905,6 +905,11 @@ corresponding symbol name (if one exists), @code{False} if it shouldn't
 should be expanded, @code{False} if they shouldn't (see @code{set print
 union} in @ref{Print Settings}).
 
+@item zero_values
+@code{True} if zero value members should be included in the string
+representation of structures and arrays, @code{False} if they shouldn't
+(see @code{set print zero-values} in @ref{Print Settings}).
+
 @item deref_refs
 @code{True} if C@t{++} references should be resolved to the value they
 refer to, @code{False} (the default) if they shouldn't.  Note that, unlike
diff --git a/gdb/python/py-value.c b/gdb/python/py-value.c
index 2ebbe0a356..c45705c377 100644
--- a/gdb/python/py-value.c
+++ b/gdb/python/py-value.c
@@ -617,6 +617,7 @@ valpy_format_string (PyObject *self, PyObject *args, PyObject *kw)
       "array_indexes",		/* See set print array-indexes on|off.  */
       "symbols",		/* See set print symbol on|off.  */
       "unions",			/* See set print union on|off.  */
+      "zero_values",		/* See set print zero-values on|off.  */
       /* C++ options.  */
       "deref_refs",		/* No corresponding setting.  */
       "actual_objects",		/* See set print object on|off.  */
@@ -660,13 +661,14 @@ valpy_format_string (PyObject *self, PyObject *args, PyObject *kw)
   PyObject *array_indexes_obj = NULL;
   PyObject *symbols_obj = NULL;
   PyObject *unions_obj = NULL;
+  PyObject *zero_values_obj = NULL;
   PyObject *deref_refs_obj = NULL;
   PyObject *actual_objects_obj = NULL;
   PyObject *static_members_obj = NULL;
   char *format = NULL;
   if (!gdb_PyArg_ParseTupleAndKeywords (args,
 					kw,
-					"|O!O!O!O!O!O!O!O!O!IIIs",
+					"|O!O!O!O!O!O!O!O!O!O!IIIs",
 					keywords,
 					&PyBool_Type, &raw_obj,
 					&PyBool_Type, &pretty_arrays_obj,
@@ -674,6 +676,7 @@ valpy_format_string (PyObject *self, PyObject *args, PyObject *kw)
 					&PyBool_Type, &array_indexes_obj,
 					&PyBool_Type, &symbols_obj,
 					&PyBool_Type, &unions_obj,
+					&PyBool_Type, &zero_values_obj,
 					&PyBool_Type, &deref_refs_obj,
 					&PyBool_Type, &actual_objects_obj,
 					&PyBool_Type, &static_members_obj,
@@ -696,6 +699,8 @@ valpy_format_string (PyObject *self, PyObject *args, PyObject *kw)
     return NULL;
   if (!copy_py_bool_obj (&opts.unionprint, unions_obj))
     return NULL;
+  if (!copy_py_bool_obj (&opts.zero_value_print, zero_values_obj))
+    return NULL;
   if (!copy_py_bool_obj (&opts.deref_ref, deref_refs_obj))
     return NULL;
   if (!copy_py_bool_obj (&opts.objectprint, actual_objects_obj))
diff --git a/gdb/testsuite/gdb.base/options.exp b/gdb/testsuite/gdb.base/options.exp
index 37a7e1ec8a..1c8bb923e6 100644
--- a/gdb/testsuite/gdb.base/options.exp
+++ b/gdb/testsuite/gdb.base/options.exp
@@ -174,6 +174,7 @@ proc_with_prefix test-print {{prefix ""}} {
 	"-symbol"
 	"-union"
 	"-vtbl"
+	"-zero-values"
     }
 
     global binfile
diff --git a/gdb/testsuite/gdb.base/zero-values.c b/gdb/testsuite/gdb.base/zero-values.c
new file mode 100644
index 0000000000..0a281d9671
--- /dev/null
+++ b/gdb/testsuite/gdb.base/zero-values.c
@@ -0,0 +1,40 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2020 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+int ix;
+struct two
+{
+  int v1, v2;
+};
+struct t
+{
+  int i1, i2, i3;
+  double d1, d2, d3;
+  int *p1, *p2, *p3;
+  struct two t1, t2, t3;
+  int ia[10];
+  int ea[3];
+  int *ipa[5];
+} t1 = {0, 0, 1, 0, 2.5, 0, &ix, 0, 0, {0, 0}, {3, 0}, {4, 5},
+	{0, 1, 2, 0, 0, 3, 4, 5, 0, 6}, {0, 0, 0},
+	{0, &ix, 0, 0, &t1.i1}};
+
+int
+main ()
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/zero-values.exp b/gdb/testsuite/gdb.base/zero-values.exp
new file mode 100644
index 0000000000..a8f377b8f7
--- /dev/null
+++ b/gdb/testsuite/gdb.base/zero-values.exp
@@ -0,0 +1,32 @@
+# Copyright 2020 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test disabled printing of zero-values in structures and arrays.
+
+standard_testfile
+
+if {[prepare_for_testing $testfile.exp $testfile $srcfile debug]} {
+    untested $testfile.exp
+    return -1
+}
+
+if ![runto_main] {
+    untested $testfile.exp
+    return -1
+}
+
+gdb_test "print t1" " = \\{i1 = 0, i2 = 0, i3 = 1, d1 = 0, d2 = 2\\.5, d3 = 0, p1 = $hex <ix>, p2 = 0x0, p3 = 0x0, t1 = \\{v1 = 0, v2 = 0\\}, t2 = \\{v1 = 3, v2 = 0\\}, t3 = \\{v1 = 4, v2 = 5\\}, ia = \\{0, 1, 2, 0, 0, 3, 4, 5, 0, 6\\}, ea = \\{0, 0, 0\\}, ipa = \\{0x0, $hex <ix>, 0x0, 0x0, $hex <t1>\\}\\}"
+
+gdb_test "print -zero-values off -- t1" " = \\{i3 = 1, d2 = 2\\.5, p1 = $hex <ix>, t2 = \\{v1 = 3\\}, t3 = \\{v1 = 4, v2 = 5\\}, ia = \\{1, 2, 3, 4, 5, 6\\}, ipa = \\{$hex <ix>, $hex <t1>\\}\\}"
diff --git a/gdb/testsuite/gdb.python/py-format-string.c b/gdb/testsuite/gdb.python/py-format-string.c
index a771370fde..ee1c4ae684 100644
--- a/gdb/testsuite/gdb.python/py-format-string.c
+++ b/gdb/testsuite/gdb.python/py-format-string.c
@@ -113,6 +113,9 @@ main ()
 
   int *a_symbol_pointer = &global_symbol;
 
+  point_t zero_one_point = { 0, 1 };
+  int an_array_with_zeroes[] = { 1, 2, 0, 3, 4, 0, 5, 0 };
+
 #ifdef __cplusplus
   Deriv a_deriv (123);
   Base &a_base_ref = a_deriv;
diff --git a/gdb/testsuite/gdb.python/py-format-string.exp b/gdb/testsuite/gdb.python/py-format-string.exp
index f809ef30cb..5b78799062 100644
--- a/gdb/testsuite/gdb.python/py-format-string.exp
+++ b/gdb/testsuite/gdb.python/py-format-string.exp
@@ -874,6 +874,22 @@ proc test_format {} {
   }
 }
 
+proc test_zero_values {} {
+  check_format_string "zero_one_point" \
+    "raw=True" \
+    "\\{x = 0, y = 1\\}"
+  check_format_string "zero_one_point" \
+    "raw=True, zero_values=False" \
+    "\\{y = 1\\}"
+
+  check_format_string "an_array_with_zeroes" \
+    "raw=True" \
+    "\\{1, 2, 0, 3, 4, 0, 5, 0\\}"
+  check_format_string "an_array_with_zeroes" \
+    "raw=True, zero_values=False" \
+    "\\{1, 2, 3, 4, 5\\}"
+}
+
 # Test mixing options.
 proc test_mixed {} {
   global current_lang
@@ -948,6 +964,7 @@ proc test_all_common {} {
   test_max_depth
   test_repeat_threshold
   test_format
+  test_zero_values
   # Multiple options mixed together.
   test_mixed
   # Various error conditions.
diff --git a/gdb/valprint.c b/gdb/valprint.c
index 2cb7779161..fccdaccb4a 100644
--- a/gdb/valprint.c
+++ b/gdb/valprint.c
@@ -117,7 +117,8 @@ struct value_print_options user_print_options =
   0,				/* summary */
   1,				/* symbol_print */
   PRINT_MAX_DEPTH_DEFAULT,	/* max_depth */
-  1				/* finish_print */
+  1,				/* finish_print */
+  1,				/* zero_value_print */
 };
 
 /* Initialize *OPTS to be a copy of the user print options.  */
@@ -1859,6 +1860,23 @@ maybe_print_array_index (struct type *index_type, LONGEST index,
 
 /* See valprint.h.  */
 
+bool
+value_is_zero (struct value *v)
+{
+  struct type *type = check_typedef (value_type (v));
+  const gdb_byte *addr = value_contents_for_printing (v);
+  unsigned int len = TYPE_LENGTH (type);
+  unsigned int zeros;
+  for (zeros = 0; zeros < len; zeros++)
+    {
+      if (addr[zeros] != 0)
+	return false;
+    }
+  return true;
+}
+
+/* See valprint.h.  */
+
 void
 value_print_array_elements (struct value *val, struct ui_file *stream,
 			    int recurse,
@@ -1905,11 +1923,21 @@ value_print_array_elements (struct value *val, struct ui_file *stream,
 
   annotate_array_section_begin (i, elttype);
 
+  bool need_comma = i != 0;
   for (; i < len && things_printed < options->print_max; i++)
     {
       scoped_value_mark free_values;
 
-      if (i != 0)
+      /* If requested, skip printing of zero value fields.  */
+      if (!options->zero_value_print)
+	{
+	  struct value *v = value_from_component (val, elttype,
+						  eltlen * i);
+	  if (value_is_zero (v))
+	    continue;
+	}
+
+      if (need_comma)
 	{
 	  if (options->prettyformat_arrays)
 	    {
@@ -1963,6 +1991,8 @@ value_print_array_elements (struct value *val, struct ui_file *stream,
 	  annotate_elt ();
 	  things_printed++;
 	}
+
+      need_comma = true;
     }
   annotate_array_section_end ();
   if (i < len)
@@ -2967,6 +2997,17 @@ show_static_field_print (struct ui_file *file, int from_tty,
 		    value);
 }
 
+/* Controls printing of zero value members.  */
+static void
+show_zero_value_print (struct ui_file *file, int from_tty,
+		       struct cmd_list_element *c,
+		       const char *value)
+{
+  fprintf_filtered (file,
+		    _("Printing of zero value members is %s.\n"),
+		    value);
+}
+
 \f
 
 /* A couple typedefs to make writing the options a bit more
@@ -3110,6 +3151,15 @@ pretty-printers for that value.")
     N_("Show printing of C++ virtual function tables."),
     NULL, /* help_doc */
   },
+
+  boolean_option_def {
+    "zero-values",
+    [] (value_print_options *opt) { return &opt->zero_value_print; },
+    show_zero_value_print, /* show_cmd_cb */
+    N_("Set printing of zero value members."),
+    N_("Show printing of zero value members."),
+    NULL, /* help_doc */
+  },
 };
 
 /* See valprint.h.  */
diff --git a/gdb/valprint.h b/gdb/valprint.h
index 57bc0339fc..81b11452a4 100644
--- a/gdb/valprint.h
+++ b/gdb/valprint.h
@@ -100,6 +100,9 @@ struct value_print_options
 
   /* Whether "finish" should print the value.  */
   bool finish_print;
+
+  /* If true, print fields with a zero value.  */
+  bool zero_value_print;
 };
 
 /* Create an option_def_group for the value_print options, with OPTS
@@ -129,6 +132,10 @@ extern void maybe_print_array_index (struct type *index_type, LONGEST index,
 				     const struct value_print_options *);
 
 
+/* Check if V only contains zero bytes.  */
+
+extern bool value_is_zero (struct value *v);
+
 /* Print elements of an array.  */
 
 extern void value_print_array_elements (struct value *, struct ui_file *, int,
-- 
2.26.2

Re: [PATCH v2][PR gdb/24052] Implement 'set print zero-values on|off'

[Pedro Alves]

Hi,

I take it that false booleans are also considered
zero, and not shown.  I guess that might be reasonable, though
I could also see someone arguing about it.

Enums with zero value are more dubious, like:

 enum foo { FOO = -1, BAR = 0, QUX = 1 };

From looking at the implementation it seems to me like zero-values off
makes us not print a BAR value.  Not sure that's really desirable.

It seems more clear to me that it'd be desirable to suppress a
zero with flag enums than with regular enums, like with this:

 enum bitmask { FOO_BIT = 1, BAR_BIT = 2, QUX_BIT = 4 };

Here's where you can check whether you have a flag enum:

 /* * True if this type is a "flag" enum.  A flag enum is one where all
    the values are pairwise disjoint when "and"ed together.  This
    affects how enum values are printed.  */

 #define TYPE_FLAG_ENUM(t) (TYPE_MAIN_TYPE (t)->flag_flag_enum)

I think we should have tests for these cases.  

There's also the question of how this interacts with Python
pretty printers:

- If there's a printer for a type or typedef that makes it so that a
zero is actually printed as some string other than "0", e.g., "JackPot!",
do we really want to suppress it?

- OTOH, if a printer decides to print a non-zero value as literal "0",
do we want to show it?

Whatever we decide, I think we should document expected behavior.

Also, it would be good to test function and method pointers too.

> @@ -194,6 +194,18 @@ cp_print_value_fields (struct value *val, struct ui_file *stream,
>  	      && field_is_static (&type->field (i)))
>  	    continue;
>  
> +	  /* If requested, skip printing of zero value fields.  */
> +	  if (!options->zero_value_print
> +	      && !field_is_static (&type->field (i)))

Not sure whether you copied this by mistake from the code above, but
skipping static fields seems wrong, since there's an option
for that.  I think we should keep the the options orthogonal.

( I could now argue that this calls for a testcase making sure
that they stay orthogonal.  :-) )

> +bool
> +value_is_zero (struct value *v)
> +{
> +  struct type *type = check_typedef (value_type (v));
> +  const gdb_byte *addr = value_contents_for_printing (v);
> +  unsigned int len = TYPE_LENGTH (type);
> +  unsigned int zeros;
> +  for (zeros = 0; zeros < len; zeros++)

Write:

  for (unsigned int zeros = 0; zeros < len; zeros++)

But I have to say that I find it weird to name the iterator
variable "zeros".  Standard "i" would be better, IMO.

TYPE_LENGTH(type) is cheap, so you could also get rid of
the len variable if you'd like.

> +    {

You can drop this level of braces.

> +      if (addr[zeros] != 0)
> +	return false;
> +    }
> +  return true;
> +/* Controls printing of zero value members.  */
> +static void

There should be an empty line after the comment.

> +show_zero_value_print (struct ui_file *file, int from_tty,
> +		       struct cmd_list_element *c,
> +		       const char *value)

Finally, this needs a gdb/NEWS entry.


Thanks,
Pedro Alves

Re: [PATCH v2][PR gdb/24052] Implement 'set print zero-values on|off'

[Hannes Domani]

 Am Samstag, 30. Mai 2020, 20:00:12 MESZ hat Pedro Alves <palves@redhat.com> Folgendes geschrieben:

> Hi,
>
> I take it that false booleans are also considered
> zero, and not shown.  I guess that might be reasonable, though
> I could also see someone arguing about it.
>
> Enums with zero value are more dubious, like:
>
> enum foo { FOO = -1, BAR = 0, QUX = 1 };
>
> From looking at the implementation it seems to me like zero-values off
> makes us not print a BAR value.  Not sure that's really desirable.

It's what I would expect, but that might just be my opinion.


> It seems more clear to me that it'd be desirable to suppress a
> zero with flag enums than with regular enums, like with this:
>
> enum bitmask { FOO_BIT = 1, BAR_BIT = 2, QUX_BIT = 4 };
>
> Here's where you can check whether you have a flag enum:
>
> /* * True if this type is a "flag" enum.  A flag enum is one where all
>     the values are pairwise disjoint when "and"ed together.  This
>     affects how enum values are printed.  */
>
> #define TYPE_FLAG_ENUM(t) (TYPE_MAIN_TYPE (t)->flag_flag_enum)
>
> I think we should have tests for these cases.
>
> There's also the question of how this interacts with Python
> pretty printers:
>
> - If there's a printer for a type or typedef that makes it so that a
> zero is actually printed as some string other than "0", e.g., "JackPot!",
> do we really want to suppress it?
>
> - OTOH, if a printer decides to print a non-zero value as literal "0",
> do we want to show it?
>
> Whatever we decide, I think we should document expected behavior.
>
> Also, it would be good to test function and method pointers too.

The way this is implemented, if I have e.g. this kinda situation:

struct point
{
  int x, y;
};
struct line
{
  point start;
  point end;
};
line l = { {0, 0}, {2, 3} };

Then it would check first if the whole l.start was just zero bytes, and
in that case, not print it at all.

But if I have to check the individual struct members for special cases,
like enums or pretty printers, this whole approach will not work.

(Maybe that means my approach is wrong.)


> > @@ -194,6 +194,18 @@ cp_print_value_fields (struct value *val, struct ui_file *stream,
> >            && field_is_static (&type->field (i)))
> >          continue;
> >
> > +      /* If requested, skip printing of zero value fields.  */
> > +      if (!options->zero_value_print
> > +          && !field_is_static (&type->field (i)))
>
> Not sure whether you copied this by mistake from the code above, but
> skipping static fields seems wrong, since there's an option
> for that.  I think we should keep the the options orthogonal.
>
> ( I could now argue that this calls for a testcase making sure
> that they stay orthogonal.  :-) )

It doesn't skip static fields, they are just not checked for a zero-value.
And that was intentional, mainly for the same reason as above.


> > +bool
> > +value_is_zero (struct value *v)
> > +{
> > +  struct type *type = check_typedef (value_type (v));
> > +  const gdb_byte *addr = value_contents_for_printing (v);
> > +  unsigned int len = TYPE_LENGTH (type);
> > +  unsigned int zeros;
> > +  for (zeros = 0; zeros < len; zeros++)
>
> Write:
>
>   for (unsigned int zeros = 0; zeros < len; zeros++)
>
> But I have to say that I find it weird to name the iterator
> variable "zeros".  Standard "i" would be better, IMO.
>
> TYPE_LENGTH(type) is cheap, so you could also get rid of
> the len variable if you'd like.
>
> > +    {
>
> You can drop this level of braces.
>
> > +      if (addr[zeros] != 0)
> > +    return false;
> > +    }
> > +  return true;
> > +/* Controls printing of zero value members.  */
> > +static void
>
> There should be an empty line after the comment.

OK.


> > +show_zero_value_print (struct ui_file *file, int from_tty,
> > +              struct cmd_list_element *c,
> > +              const char *value)
>
> Finally, this needs a gdb/NEWS entry.

Like this, added to the "New commands" section?:

set print zero-values [on|off]
show print zero-values
  This controls whether GDB prints zero value members of structures and
  arrays.  The default is 'on'.


Hannes

Re: [PATCH v2][PR gdb/24052] Implement 'set print zero-values on|off'

[Pedro Alves]

On 5/31/20 1:06 AM, Hannes Domani via Gdb-patches wrote:
>  Am Samstag, 30. Mai 2020, 20:00:12 MESZ hat Pedro Alves <palves@redhat.com> Folgendes geschrieben:
> 
>> Hi,
>>
>> I take it that false booleans are also considered
>> zero, and not shown.  I guess that might be reasonable, though
>> I could also see someone arguing about it.
>>
>> Enums with zero value are more dubious, like:
>>
>> enum foo { FOO = -1, BAR = 0, QUX = 1 };
>>
>> From looking at the implementation it seems to me like zero-values off
>> makes us not print a BAR value.  Not sure that's really desirable.
> 
> It's what I would expect, but that might just be my opinion.

I now see that the original reporter also mentioned enums.

The thing is that an enum does not measure a quantity or offset.
"0" has no usual particular significance compared to
other enumerators.  While with pointers and integrals, usually
"0" has significance, meaning no quantity, no offset or no object,
or in general absence of the property being measured by the variable.

For example, here's GDB's auto_boolean:

 /* * A generic, not quite boolean, enumeration.  This is used for
    set/show commands in which the options are on/off/automatic.  */
 enum auto_boolean
 {
   AUTO_BOOLEAN_TRUE,
   AUTO_BOOLEAN_FALSE,
   AUTO_BOOLEAN_AUTO
 };

I'd think it confusing that "zero-values off" would hide
AUTO_BOOLEAN_TRUE, but not AUTO_BOOLEAN_FALSE.

Here:

extern enum language_mode
  {
    language_mode_auto, language_mode_manual
  }
language_mode;

What's the significance of hiding auto but not manual?

Here:

/* alignment enum */
enum ui_align
  {
    ui_left = -1,
    ui_center,
    ui_right,
    ui_noalign
  };

Why hide ui_center, instead of the other enumerators?

Etc.

> 
> 
>> It seems more clear to me that it'd be desirable to suppress a
>> zero with flag enums than with regular enums, like with this:
>>
>> enum bitmask { FOO_BIT = 1, BAR_BIT = 2, QUX_BIT = 4 };
>>
>> Here's where you can check whether you have a flag enum:
>>
>> /* * True if this type is a "flag" enum.  A flag enum is one where all
>>      the values are pairwise disjoint when "and"ed together.  This
>>      affects how enum values are printed.  */
>>
>> #define TYPE_FLAG_ENUM(t) (TYPE_MAIN_TYPE (t)->flag_flag_enum)
>>
>> I think we should have tests for these cases.
>>
>> There's also the question of how this interacts with Python
>> pretty printers:
>>
>> - If there's a printer for a type or typedef that makes it so that a
>> zero is actually printed as some string other than "0", e.g., "JackPot!",
>> do we really want to suppress it?
>>
>> - OTOH, if a printer decides to print a non-zero value as literal "0",
>> do we want to show it?
>>
>> Whatever we decide, I think we should document expected behavior.
>>
>> Also, it would be good to test function and method pointers too.
> 
> The way this is implemented, if I have e.g. this kinda situation:
> 
> struct point
> {
>   int x, y;
> };
> struct line
> {
>   point start;
>   point end;
> };
> line l = { {0, 0}, {2, 3} };
> 
> Then it would check first if the whole l.start was just zero bytes, and
> in that case, not print it at all.
> 
> But if I have to check the individual struct members for special cases,
> like enums or pretty printers, this whole approach will not work.
> 
> (Maybe that means my approach is wrong.)

Not sure about wrong but at least it will require tweaking, even if
you ignore printers and enums, for the fact that it ignores
structure padding.

For example, apply this on top of your patch:

From c9210531b1a57b05bce7b7da46575050cda15bf8 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Sun, 31 May 2020 14:08:17 +0100
Subject: [PATCH] padding

---
 gdb/testsuite/gdb.base/zero-values.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/gdb/testsuite/gdb.base/zero-values.c b/gdb/testsuite/gdb.base/zero-values.c
index 0a281d9671b..b2309398108 100644
--- a/gdb/testsuite/gdb.base/zero-values.c
+++ b/gdb/testsuite/gdb.base/zero-values.c
@@ -15,6 +15,8 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
+#include <string.h>
+
 int ix;
 struct two
 {
@@ -33,8 +35,29 @@ struct t
 	{0, 1, 2, 0, 0, 3, 4, 5, 0, 6}, {0, 0, 0},
 	{0, &ix, 0, 0, &t1.i1}};
 
+struct padding
+{
+  char c;
+  int i;
+};
+
+struct outer
+{
+  struct padding pad1;
+  struct padding pad2;
+};
+
+struct outer g_out;
+
 int
 main ()
 {
+  struct outer out;
+  memset (&out, 0xff, sizeof (out));
+  out.pad1.c = 0;
+  out.pad1.i = 0;
+  out.pad2.c = 0;
+  out.pad2.i = 0;
+
   return 0;
 }

-- 
2.14.5

Now run to the "return 0;" line, and see:

 (gdb) p g_out
 $1 = {pad1 = {c = 0 '\000', i = 0}, pad2 = {c = 0 '\000', i = 0}}
 (gdb) p out
 $2 = {pad1 = {c = 0 '\000', i = 0}, pad2 = {c = 0 '\000', i = 0}}

 (gdb) p -zero-values off -- g_out
 $3 = {}
 (gdb) p -zero-values off -- out
 $4 = {pad1 = {}, pad2 = {}}

As you see, $3 and $4 gave different outputs, due to the padding.

There's still the question about pretty printing open, though.

(BTW, while quickly playing with it, I wondered whether we could come
up with an option name that would make it so that "on" would 
hide the zeroes, so that you could get away without having to
write "off" by default, like "p -z -- out".  That may conflict
with the potential desire to expand on/off into an enumeration
with other variants other than on/off, though.)


>>> @@ -194,6 +194,18 @@ cp_print_value_fields (struct value *val, struct ui_file *stream,
>>>             && field_is_static (&type->field (i)))
>>>           continue;
>>>
>>> +      /* If requested, skip printing of zero value fields.  */
>>> +      if (!options->zero_value_print
>>> +          && !field_is_static (&type->field (i)))
>>
>> Not sure whether you copied this by mistake from the code above, but
>> skipping static fields seems wrong, since there's an option
>> for that.  I think we should keep the the options orthogonal.
>>
>> ( I could now argue that this calls for a testcase making sure
>> that they stay orthogonal.  :-) )
> 
> It doesn't skip static fields, they are just not checked for a zero-value.
> And that was intentional, mainly for the same reason as above.

I don't get it.  If they are not skipped, why wouldn't
"print -zero-value off" hide zero values in static fields?

Apply this further patch on top of the other one:

From d3e174a53712c1c0a797d201e10d186b35174509 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Sun, 31 May 2020 14:29:53 +0100
Subject: [PATCH] static

---
 gdb/testsuite/gdb.base/zero-values.c   | 5 +++++
 gdb/testsuite/gdb.base/zero-values.exp | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.base/zero-values.c b/gdb/testsuite/gdb.base/zero-values.c
index b2309398108..57ce6ba71db 100644
--- a/gdb/testsuite/gdb.base/zero-values.c
+++ b/gdb/testsuite/gdb.base/zero-values.c
@@ -31,10 +31,15 @@ struct t
   int ia[10];
   int ea[3];
   int *ipa[5];
+
+  static int static_field;
+
 } t1 = {0, 0, 1, 0, 2.5, 0, &ix, 0, 0, {0, 0}, {3, 0}, {4, 5},
 	{0, 1, 2, 0, 0, 3, 4, 5, 0, 6}, {0, 0, 0},
 	{0, &ix, 0, 0, &t1.i1}};
 
+int t::static_field = 0;
+
 struct padding
 {
   char c;
diff --git a/gdb/testsuite/gdb.base/zero-values.exp b/gdb/testsuite/gdb.base/zero-values.exp
index a8f377b8f7e..8f3c2cca3cb 100644
--- a/gdb/testsuite/gdb.base/zero-values.exp
+++ b/gdb/testsuite/gdb.base/zero-values.exp
@@ -17,7 +17,7 @@
 
 standard_testfile
 
-if {[prepare_for_testing $testfile.exp $testfile $srcfile debug]} {
+if {[prepare_for_testing $testfile.exp $testfile $srcfile {debug c++}]} {
     untested $testfile.exp
     return -1
 }

-- 
2.14.5

And now observe:

 (gdb) p t1
 $1 = {i1 = 0, i2 = 0, i3 = 1, d1 = 0, d2 = 2.5, d3 = 0, p1 = 0x601110 <ix>, p2 = 0x0, p3 = 0x0, t1 = {v1 = 0, v2 = 0}, t2 = {v1 = 3, v2 = 0}, t3 = {v1 = 4, v2 = 5}, ia = {0, 1, 2, 0, 0, 3, 4, 5, 0, 6}, ea = {0, 0, 0}, ipa = {0x0, 0x601110 <ix>, 0x0, 0x0, 0x601040 <t1>}, static static_field = 0}

 (gdb) p -zero-values off -- t1
 $2 = {i3 = 1, d2 = 2.5, p1 = 0x601110 <ix>, t2 = {v1 = 3}, t3 = {v1 = 4, v2 = 5}, ia = {1, 2, 3, 4, 5, 6}, ipa = {0x601110 <ix>, 0x601040 <t1>}, static static_field = 0}

Why print "static_field = 0" when zero-values is off?

>>> +show_zero_value_print (struct ui_file *file, int from_tty,
>>> +              struct cmd_list_element *c,
>>> +              const char *value)
>>
>> Finally, this needs a gdb/NEWS entry.
> 
> Like this, added to the "New commands" section?:
> 
> set print zero-values [on|off]
> show print zero-values
>   This controls whether GDB prints zero value members of structures and
>   arrays.  The default is 'on'.
Yes, something like that.   I guess also for unions.  You should add a
test for unions too, btw.

Note that the documentation bits will be reviewed/approved
by Eli.

Thanks,
Pedro Alves

Re: [PATCH v2][PR gdb/24052] Implement 'set print zero-values on|off'

[Pedro Alves]

On 5/31/20 2:38 PM, Pedro Alves via Gdb-patches wrote:
> Yes, something like that.   I guess also for unions.  You should add a
> test for unions too, btw.

I think it would be good to test bitfields too.  Not sure the
value_is_zero check works correctly with bitfields as is, since
that is checking byte per byte.

Thanks,
Pedro Alves

Re: [PATCH v2][PR gdb/24052] Implement 'set print zero-values on|off'

[Hannes Domani]

 Am Sonntag, 31. Mai 2020, 15:39:05 MESZ hat Pedro Alves <palves@redhat.com> Folgendes geschrieben:

> On 5/31/20 1:06 AM, Hannes Domani via Gdb-patches wrote:
> >  Am Samstag, 30. Mai 2020, 20:00:12 MESZ hat Pedro Alves <palves@redhat.com> Folgendes geschrieben:
> >
> >> Hi,
> >>
> >> I take it that false booleans are also considered
> >> zero, and not shown.  I guess that might be reasonable, though
> >> I could also see someone arguing about it.
> >>
> >> Enums with zero value are more dubious, like:
> >>
> >> enum foo { FOO = -1, BAR = 0, QUX = 1 };
> >>
> >> From looking at the implementation it seems to me like zero-values off
> >> makes us not print a BAR value.  Not sure that's really desirable.
> >
> > It's what I would expect, but that might just be my opinion.
>
> I now see that the original reporter also mentioned enums.
>
> The thing is that an enum does not measure a quantity or offset.
> "0" has no usual particular significance compared to
> other enumerators.  While with pointers and integrals, usually
> "0" has significance, meaning no quantity, no offset or no object,
> or in general absence of the property being measured by the variable.
>
> For example, here's GDB's auto_boolean:
>
> /* * A generic, not quite boolean, enumeration.  This is used for
>     set/show commands in which the options are on/off/automatic.  */
> enum auto_boolean
> {
>   AUTO_BOOLEAN_TRUE,
>   AUTO_BOOLEAN_FALSE,
>   AUTO_BOOLEAN_AUTO
> };
>
> I'd think it confusing that "zero-values off" would hide
> AUTO_BOOLEAN_TRUE, but not AUTO_BOOLEAN_FALSE.
>
> Here:
>
> extern enum language_mode
>   {
>     language_mode_auto, language_mode_manual
>   }
> language_mode;
>
> What's the significance of hiding auto but not manual?
>
> Here:
>
> /* alignment enum */
> enum ui_align
>   {
>     ui_left = -1,
>     ui_center,
>     ui_right,
>     ui_noalign
>   };
>
> Why hide ui_center, instead of the other enumerators?
>
> Etc.

It seems we have very different views about this.
I don't think it's confusing at all to hide AUTO_BOOLEAN_TRUE/
language_mode_auto/ui_center in these cases.

(For me it's more confusing that AUTO_BOOLEAN_TRUE is first in this enum.)

If you don't want to hide it, just don't use -zero-values off.


> >
> >
> >> It seems more clear to me that it'd be desirable to suppress a
> >> zero with flag enums than with regular enums, like with this:
> >>
> >> enum bitmask { FOO_BIT = 1, BAR_BIT = 2, QUX_BIT = 4 };
> >>
> >> Here's where you can check whether you have a flag enum:
> >>
> >> /* * True if this type is a "flag" enum.  A flag enum is one where all
> >>      the values are pairwise disjoint when "and"ed together.  This
> >>      affects how enum values are printed.  */
> >>
> >> #define TYPE_FLAG_ENUM(t) (TYPE_MAIN_TYPE (t)->flag_flag_enum)
> >>
> >> I think we should have tests for these cases.
> >>
> >> There's also the question of how this interacts with Python
> >> pretty printers:
> >>
> >> - If there's a printer for a type or typedef that makes it so that a
> >> zero is actually printed as some string other than "0", e.g., "JackPot!",
> >> do we really want to suppress it?
> >>
> >> - OTOH, if a printer decides to print a non-zero value as literal "0",
> >> do we want to show it?
> >>
> >> Whatever we decide, I think we should document expected behavior.
> >>
> >> Also, it would be good to test function and method pointers too.
> >
> > The way this is implemented, if I have e.g. this kinda situation:
> >
> > struct point
> > {
> >   int x, y;
> > };
> > struct line
> > {
> >   point start;
> >   point end;
> > };
> > line l = { {0, 0}, {2, 3} };
> >
> > Then it would check first if the whole l.start was just zero bytes, and
> > in that case, not print it at all.
> >
> > But if I have to check the individual struct members for special cases,
> > like enums or pretty printers, this whole approach will not work.
> >
> > (Maybe that means my approach is wrong.)
>
> Not sure about wrong but at least it will require tweaking, even if
> you ignore printers and enums, for the fact that it ignores
> structure padding.
>
> For example, apply this on top of your patch:
>
> From c9210531b1a57b05bce7b7da46575050cda15bf8 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Sun, 31 May 2020 14:08:17 +0100
> Subject: [PATCH] padding
>
> ---
> gdb/testsuite/gdb.base/zero-values.c | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/gdb/testsuite/gdb.base/zero-values.c b/gdb/testsuite/gdb.base/zero-values.c
> index 0a281d9671b..b2309398108 100644
> --- a/gdb/testsuite/gdb.base/zero-values.c
> +++ b/gdb/testsuite/gdb.base/zero-values.c
> @@ -15,6 +15,8 @@
>     You should have received a copy of the GNU General Public License
>     along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>
> +#include <string.h>
> +
> int ix;
> struct two
> {
> @@ -33,8 +35,29 @@ struct t
>     {0, 1, 2, 0, 0, 3, 4, 5, 0, 6}, {0, 0, 0},
>     {0, &ix, 0, 0, &t1.i1}};
>
> +struct padding
> +{
> +  char c;
> +  int i;
> +};
> +
> +struct outer
> +{
> +  struct padding pad1;
> +  struct padding pad2;
> +};
> +
> +struct outer g_out;
> +
> int
> main ()
> {
> +  struct outer out;
> +  memset (&out, 0xff, sizeof (out));
> +  out.pad1.c = 0;
> +  out.pad1.i = 0;
> +  out.pad2.c = 0;
> +  out.pad2.i = 0;
> +
>   return 0;
> }
>
> --
> 2.14.5
>
> Now run to the "return 0;" line, and see:
>
> (gdb) p g_out
> $1 = {pad1 = {c = 0 '\000', i = 0}, pad2 = {c = 0 '\000', i = 0}}
> (gdb) p out
> $2 = {pad1 = {c = 0 '\000', i = 0}, pad2 = {c = 0 '\000', i = 0}}
>
> (gdb) p -zero-values off -- g_out
> $3 = {}
> (gdb) p -zero-values off -- out
> $4 = {pad1 = {}, pad2 = {}}
>
> As you see, $3 and $4 gave different outputs, due to the padding.

I agree that this might be weird, but I kinda see this as a feature.


> There's still the question about pretty printing open, though.
>
> (BTW, while quickly playing with it, I wondered whether we could come
> up with an option name that would make it so that "on" would
> hide the zeroes, so that you could get away without having to
> write "off" by default, like "p -z -- out".  That may conflict
> with the potential desire to expand on/off into an enumeration
> with other variants other than on/off, though.)
>
>
> >>> @@ -194,6 +194,18 @@ cp_print_value_fields (struct value *val, struct ui_file *stream,
> >>>             && field_is_static (&type->field (i)))
> >>>           continue;
> >>>
> >>> +      /* If requested, skip printing of zero value fields.  */
> >>> +      if (!options->zero_value_print
> >>> +          && !field_is_static (&type->field (i)))
> >>
> >> Not sure whether you copied this by mistake from the code above, but
> >> skipping static fields seems wrong, since there's an option
> >> for that.  I think we should keep the the options orthogonal.
> >>
> >> ( I could now argue that this calls for a testcase making sure
> >> that they stay orthogonal.  :-) )
> >
> > It doesn't skip static fields, they are just not checked for a zero-value.
> > And that was intentional, mainly for the same reason as above.
>
> I don't get it.  If they are not skipped, why wouldn't
> "print -zero-value off" hide zero values in static fields?
>
> Apply this further patch on top of the other one:
>
> From d3e174a53712c1c0a797d201e10d186b35174509 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Sun, 31 May 2020 14:29:53 +0100
> Subject: [PATCH] static
>
> ---
> gdb/testsuite/gdb.base/zero-values.c  | 5 +++++
> gdb/testsuite/gdb.base/zero-values.exp | 2 +-
> 2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/testsuite/gdb.base/zero-values.c b/gdb/testsuite/gdb.base/zero-values.c
> index b2309398108..57ce6ba71db 100644
> --- a/gdb/testsuite/gdb.base/zero-values.c
> +++ b/gdb/testsuite/gdb.base/zero-values.c
> @@ -31,10 +31,15 @@ struct t
>   int ia[10];
>   int ea[3];
>   int *ipa[5];
> +
> +  static int static_field;
> +
> } t1 = {0, 0, 1, 0, 2.5, 0, &ix, 0, 0, {0, 0}, {3, 0}, {4, 5},
>     {0, 1, 2, 0, 0, 3, 4, 5, 0, 6}, {0, 0, 0},
>     {0, &ix, 0, 0, &t1.i1}};
>
> +int t::static_field = 0;
> +
> struct padding
> {
>   char c;
> diff --git a/gdb/testsuite/gdb.base/zero-values.exp b/gdb/testsuite/gdb.base/zero-values.exp
> index a8f377b8f7e..8f3c2cca3cb 100644
> --- a/gdb/testsuite/gdb.base/zero-values.exp
> +++ b/gdb/testsuite/gdb.base/zero-values.exp
> @@ -17,7 +17,7 @@
>
> standard_testfile
>
> -if {[prepare_for_testing $testfile.exp $testfile $srcfile debug]} {
> +if {[prepare_for_testing $testfile.exp $testfile $srcfile {debug c++}]} {
>     untested $testfile.exp
>     return -1
> }
>
> --
> 2.14.5
>
> And now observe:
>
> (gdb) p t1
> $1 = {i1 = 0, i2 = 0, i3 = 1, d1 = 0, d2 = 2.5, d3 = 0, p1 = 0x601110 <ix>, p2 = 0x0, p3 = 0x0, t1 = {v1 = 0, v2 = 0}, t2 = {v1 = 3, v2 = 0}, t3 = {v1 = 4, v2 = 5}, ia = {0, 1, 2, 0, 0, 3, 4, 5, 0, 6}, ea = {0, 0, 0}, ipa = {0x0, 0x601110 <ix>, 0x0, 0x0, 0x601040 <t1>}, static static_field = 0}
>
> (gdb) p -zero-values off -- t1
> $2 = {i3 = 1, d2 = 2.5, p1 = 0x601110 <ix>, t2 = {v1 = 3}, t3 = {v1 = 4, v2 = 5}, ia = {1, 2, 3, 4, 5, 6}, ipa = {0x601110 <ix>, 0x601040 <t1>}, static static_field = 0}
>
> Why print "static_field = 0" when zero-values is off?

When printing structures, I usually don't care about the static members.

And with -zero-values off it should just display the parts that have some kind
of value.
So now I kinda want to hide all static members when -zero-values off, no
matter what their real value is.


> >>> +show_zero_value_print (struct ui_file *file, int from_tty,
> >>> +              struct cmd_list_element *c,
> >>> +              const char *value)
> >>
> >> Finally, this needs a gdb/NEWS entry.
> >
> > Like this, added to the "New commands" section?:
> >
> > set print zero-values [on|off]
> > show print zero-values
> >   This controls whether GDB prints zero value members of structures and
> >   arrays.  The default is 'on'.
> Yes, something like that.  I guess also for unions.  You should add a
> test for unions too, btw.

OK.


> Note that the documentation bits will be reviewed/approved
> by Eli.


Hannes

Re: [PATCH v2][PR gdb/24052] Implement 'set print zero-values on|off'

[Pedro Alves]

On 5/31/20 3:58 PM, Hannes Domani via Gdb-patches wrote:
>  Am Sonntag, 31. Mai 2020, 15:39:05 MESZ hat Pedro Alves <palves@redhat.com> Folgendes geschrieben:

>> The thing is that an enum does not measure a quantity or offset.
>> "0" has no usual particular significance compared to
>> other enumerators.  While with pointers and integrals, usually
>> "0" has significance, meaning no quantity, no offset or no object,
>> or in general absence of the property being measured by the variable.
>>
>> For example, here's GDB's auto_boolean:
>>
>> /* * A generic, not quite boolean, enumeration.  This is used for
>>      set/show commands in which the options are on/off/automatic.  */
>> enum auto_boolean
>> {
>>    AUTO_BOOLEAN_TRUE,
>>    AUTO_BOOLEAN_FALSE,
>>    AUTO_BOOLEAN_AUTO
>> };
>>
>> I'd think it confusing that "zero-values off" would hide
>> AUTO_BOOLEAN_TRUE, but not AUTO_BOOLEAN_FALSE.
>>
>> Here:
>>
>> extern enum language_mode
>>    {
>>      language_mode_auto, language_mode_manual
>>    }
>> language_mode;
>>
>> What's the significance of hiding auto but not manual?
>>
>> Here:
>>
>> /* alignment enum */
>> enum ui_align
>>    {
>>      ui_left = -1,
>>      ui_center,
>>      ui_right,
>>      ui_noalign
>>    };
>>
>> Why hide ui_center, instead of the other enumerators?
>>
>> Etc.
> 
> It seems we have very different views about this.
> I don't think it's confusing at all to hide AUTO_BOOLEAN_TRUE/
> language_mode_auto/ui_center in these cases.
> 

OK, if such different views are both reasonable, then this
normally means that the larger set of users will also contain
people with such opposing views, which calls for making it
optional.  Maybe:

 set print zero-values all / non-enums / none

> (For me it's more confusing that AUTO_BOOLEAN_TRUE is first in this enum.)
> 
> If you don't want to hide it, just don't use -zero-values off.
> 

Even the original reporter in the PR suggested only removing
zero enums under an option:

 "Optionally also removing those enums which evaluate to zero would
 save even more unneeded information."

>> (gdb) p g_out
>> $1 = {pad1 = {c = 0 '\000', i = 0}, pad2 = {c = 0 '\000', i = 0}}
>> (gdb) p out
>> $2 = {pad1 = {c = 0 '\000', i = 0}, pad2 = {c = 0 '\000', i = 0}}
>>
>> (gdb) p -zero-values off -- g_out
>> $3 = {}
>> (gdb) p -zero-values off -- out
>> $4 = {pad1 = {}, pad2 = {}}
>>
>> As you see, $3 and $4 gave different outputs, due to the padding.
> 
> I agree that this might be weird, but I kinda see this as a feature.

What's the value of the feature?  I think it's a hard to justify feature,
because garbage in padding happens randomly, and naturally, and doesn't
affect the value at the language level.  I very much question the value
in wanting a different output here, and I hazard a guess that you were
initially surprised with this case too.  IMO it's just a bug not to
consider it.

>> Why print "static_field = 0" when zero-values is off?
> 
> When printing structures, I usually don't care about the static members.
> 
> And with -zero-values off it should just display the parts that have some kind
> of value.
> So now I kinda want to hide all static members when -zero-values off, no
> matter what their real value is.

NAK.  Let's keep options orthogonal.

Thanks,
Pedro Alves

Re: [PATCH v2][PR gdb/24052] Implement 'set print zero-values on|off'

[Hannes Domani]

 Am Sonntag, 31. Mai 2020, 17:21:16 MESZ hat Pedro Alves <palves@redhat.com> Folgendes geschrieben:

> On 5/31/20 3:58 PM, Hannes Domani via Gdb-patches wrote:
> >  Am Sonntag, 31. Mai 2020, 15:39:05 MESZ hat Pedro Alves <palves@redhat.com> Folgendes geschrieben:
>
> >> The thing is that an enum does not measure a quantity or offset.
> >> "0" has no usual particular significance compared to
> >> other enumerators.  While with pointers and integrals, usually
> >> "0" has significance, meaning no quantity, no offset or no object,
> >> or in general absence of the property being measured by the variable.
> >>
> >> For example, here's GDB's auto_boolean:
> >>
> >> /* * A generic, not quite boolean, enumeration.  This is used for
> >>      set/show commands in which the options are on/off/automatic.  */
> >> enum auto_boolean
> >> {
> >>    AUTO_BOOLEAN_TRUE,
> >>    AUTO_BOOLEAN_FALSE,
> >>    AUTO_BOOLEAN_AUTO
> >> };
> >>
> >> I'd think it confusing that "zero-values off" would hide
> >> AUTO_BOOLEAN_TRUE, but not AUTO_BOOLEAN_FALSE.
> >>
> >> Here:
> >>
> >> extern enum language_mode
> >>    {
> >>      language_mode_auto, language_mode_manual
> >>    }
> >> language_mode;
> >>
> >> What's the significance of hiding auto but not manual?
> >>
> >> Here:
> >>
> >> /* alignment enum */
> >> enum ui_align
> >>    {
> >>      ui_left = -1,
> >>      ui_center,
> >>      ui_right,
> >>      ui_noalign
> >>    };
> >>
> >> Why hide ui_center, instead of the other enumerators?
> >>
> >> Etc.
> >
> > It seems we have very different views about this.
> > I don't think it's confusing at all to hide AUTO_BOOLEAN_TRUE/
> > language_mode_auto/ui_center in these cases.
> >
>
> OK, if such different views are both reasonable, then this
> normally means that the larger set of users will also contain
> people with such opposing views, which calls for making it
> optional.  Maybe:
>
> set print zero-values all / non-enums / none
>
> > (For me it's more confusing that AUTO_BOOLEAN_TRUE is first in this enum.)
> >
> > If you don't want to hide it, just don't use -zero-values off.
> >
>
> Even the original reporter in the PR suggested only removing
> zero enums under an option:
>
> "Optionally also removing those enums which evaluate to zero would
> save even more unneeded information."
>
> >> (gdb) p g_out
> >> $1 = {pad1 = {c = 0 '\000', i = 0}, pad2 = {c = 0 '\000', i = 0}}
> >> (gdb) p out
> >> $2 = {pad1 = {c = 0 '\000', i = 0}, pad2 = {c = 0 '\000', i = 0}}
> >>
> >> (gdb) p -zero-values off -- g_out
> >> $3 = {}
> >> (gdb) p -zero-values off -- out
> >> $4 = {pad1 = {}, pad2 = {}}
> >>
> >> As you see, $3 and $4 gave different outputs, due to the padding.
> >
> > I agree that this might be weird, but I kinda see this as a feature.
>
> What's the value of the feature?  I think it's a hard to justify feature,
> because garbage in padding happens randomly, and naturally, and doesn't
> affect the value at the language level.  I very much question the value
> in wanting a different output here, and I hazard a guess that you were
> initially surprised with this case too.  IMO it's just a bug not to
> consider it.
>
> >> Why print "static_field = 0" when zero-values is off?
> >
> > When printing structures, I usually don't care about the static members.
> >
> > And with -zero-values off it should just display the parts that have some kind
> > of value.
> > So now I kinda want to hide all static members when -zero-values off, no
> > matter what their real value is.
>
> NAK.  Let's keep options orthogonal.

OK.
I will first try to fix the padding and static members.
Once I got that, enums.

But I have no idea how python pretty printers could be handled.


Hannes