[PATCH 0/4] Fix two varobj bugs

[PATCH 0/4] Fix two varobj bugs

[Tom Tromey]

This series fixes a couple of small varobj bugs, and includes a couple
small cleanups as well.

Regression tested on x86-64 Fedora 36.

---
Tom Tromey (4):
      Allow pretty-printer 'children' method to return strings
      Remove dead code from varobj_set_display_format
      Remove variable_default_display
      Fix bug in -var-evaluate-expression

 gdb/testsuite/gdb.python/py-varobj.c   | 28 ++++++++++++++++
 gdb/testsuite/gdb.python/py-varobj.exp | 61 ++++++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.python/py-varobj.py  | 37 +++++++++++++++++++++
 gdb/varobj.c                           | 39 ++++++++--------------
 4 files changed, 140 insertions(+), 25 deletions(-)
---
base-commit: b47a4f92de5c12ac8bdac57b0413e3e05d411832
change-id: 20230901-varobj-fixes-9926a7876ec6

Best regards,
-- 
Tom Tromey <tromey@adacore.com>

[PATCH 1/4] Allow pretty-printer 'children' method to return strings

[Tom Tromey]

A user noticed that, while a pretty-printer can return Python strings
from its "children" method, this does not really work for MI.  I
tracked this down to my_value_of_variable calling into
c_value_of_variable, which specially handles arrays and structures --
not using the actual contents of the string.

Now, this part of MI seems bad to me, but rather than change that,
this applies the fix to only dynamic varobjs, which is the only
scenario where a string like this can really be returned.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=18282
---
 gdb/testsuite/gdb.python/py-varobj.c   | 26 ++++++++++++++++++
 gdb/testsuite/gdb.python/py-varobj.exp | 49 ++++++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.python/py-varobj.py  | 35 ++++++++++++++++++++++++
 gdb/varobj.c                           |  3 +++
 4 files changed, 113 insertions(+)

diff --git a/gdb/testsuite/gdb.python/py-varobj.c b/gdb/testsuite/gdb.python/py-varobj.c
new file mode 100644
index 00000000000..894ce8fca06
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-varobj.c
@@ -0,0 +1,26 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2023 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/>.  */
+
+struct test {
+  int x;
+};
+
+struct test tval = {23};
+
+int main () {
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.python/py-varobj.exp b/gdb/testsuite/gdb.python/py-varobj.exp
new file mode 100644
index 00000000000..0e0978352a5
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-varobj.exp
@@ -0,0 +1,49 @@
+# Copyright (C) 2023 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/>.
+
+# Some varobj tests involving pretty-printers
+
+load_lib mi-support.exp
+set MIFLAGS "-i=mi"
+
+require allow_python_tests
+
+standard_testfile
+
+if {[gdb_compile "$srcdir/$subdir/$srcfile" $binfile executable {debug}] != ""} {
+    return -1
+}
+
+mi_clean_restart $binfile
+
+set pyfile [gdb_remote_download host ${srcdir}/${subdir}/${testfile}.py]
+mi_gdb_test "source ${pyfile}" \
+  ".*\\^done" \
+  "load python file"
+
+mi_gdb_test "-enable-pretty-printing" \
+  "\\^done" \
+  "-enable-pretty-printing"
+
+mi_gdb_test "set python print-stack full" \
+  ".*\\^done" \
+  "set python print-stack full"
+
+mi_runto_main
+
+mi_gdb_test "-var-create tval * tval" \
+   "\\^done.*"
+
+mi_gdb_test "-var-list-children --all-values tval" \
+    ".*value=.*flicker.*"
diff --git a/gdb/testsuite/gdb.python/py-varobj.py b/gdb/testsuite/gdb.python/py-varobj.py
new file mode 100644
index 00000000000..bc31a198297
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-varobj.py
@@ -0,0 +1,35 @@
+# Copyright (C) 2023 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/>.
+
+import gdb.printing
+
+
+class TestPrinter:
+    def __init__(self, val):
+        self.val = val
+
+    def to_string(self):
+        return "map"
+
+    def children(self):
+        yield "1", "flicker"
+
+
+def str_lookup_function(val):
+    lookup_tag = val.type.tag
+    if lookup_tag == "test":
+        return TestPrinter(val)
+
+
+gdb.printing.register_pretty_printer(None, str_lookup_function)
diff --git a/gdb/varobj.c b/gdb/varobj.c
index e7323bed127..52e62aca265 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -2117,6 +2117,9 @@ my_value_of_variable (struct varobj *var, enum varobj_display_formats format)
       if (var->dynamic->pretty_printer != NULL)
 	return varobj_value_get_print_value (var->value.get (), var->format,
 					     var);
+      else if (var->parent != nullptr && varobj_is_dynamic_p (var->parent))
+	return var->print_value;
+
       return (*var->root->lang_ops->value_of_variable) (var, format);
     }
   else

-- 
2.40.1

[PATCH 2/4] Remove dead code from varobj_set_display_format

[Tom Tromey]

varobj_set_display_format takes an enum and exhaustively switches on
the values -- but also has a default.  This default case is dead code.
---
 gdb/varobj.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/gdb/varobj.c b/gdb/varobj.c
index 52e62aca265..760809c6f51 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -488,20 +488,7 @@ enum varobj_display_formats
 varobj_set_display_format (struct varobj *var,
 			   enum varobj_display_formats format)
 {
-  switch (format)
-    {
-    case FORMAT_NATURAL:
-    case FORMAT_BINARY:
-    case FORMAT_DECIMAL:
-    case FORMAT_HEXADECIMAL:
-    case FORMAT_OCTAL:
-    case FORMAT_ZHEXADECIMAL:
-      var->format = format;
-      break;
-
-    default:
-      var->format = variable_default_display (var);
-    }
+  var->format = format;
 
   if (varobj_value_is_changeable_p (var) 
       && var->value != nullptr && !var->value->lazy ())

-- 
2.40.1

[PATCH 3/4] Remove variable_default_display

[Tom Tromey]

variable_default_display has a single caller now, so remove it.
---
 gdb/varobj.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/gdb/varobj.c b/gdb/varobj.c
index 760809c6f51..3ae8d728e82 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -164,8 +164,6 @@ create_child_with_value (struct varobj *parent, int index,
 
 /* Utility routines */
 
-static enum varobj_display_formats variable_default_display (struct varobj *);
-
 static bool update_type_if_necessary (struct varobj *var,
 				      struct value *new_value);
 
@@ -336,7 +334,7 @@ varobj_create (const char *objname,
 	  return NULL;
 	}
 
-      var->format = variable_default_display (var.get ());
+      var->format = FORMAT_NATURAL;
       var->root->valid_block =
 	var->root->floating ? NULL : tracker.block ();
       var->root->global
@@ -1871,14 +1869,6 @@ varobj_get_value_type (const struct varobj *var)
   return type;
 }
 
-/* What is the default display for this variable? We assume that
-   everything is "natural".  Any exceptions?  */
-static enum varobj_display_formats
-variable_default_display (struct varobj *var)
-{
-  return FORMAT_NATURAL;
-}
-
 /*
  * Language-dependencies
  */

-- 
2.40.1

[PATCH 4/4] Fix bug in -var-evaluate-expression

[Tom Tromey]

This bug points out that if one uses -var-set-visualizer with "None"
-- to disable a pretty-printer for a varobj -- then
-var-evaluate-expression will still use pretty-printing.

This is a combination of bugs.  First, setting the visualizer does not
update the display text; and second, computing the display text should
use "raw" when Python is available but no visualizer is desired.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=11738
---
 gdb/testsuite/gdb.python/py-varobj.c   |  2 ++
 gdb/testsuite/gdb.python/py-varobj.exp | 12 ++++++++++++
 gdb/testsuite/gdb.python/py-varobj.py  |  4 +++-
 gdb/varobj.c                           |  9 +++++++++
 4 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.python/py-varobj.c b/gdb/testsuite/gdb.python/py-varobj.c
index 894ce8fca06..88d0e74b7e0 100644
--- a/gdb/testsuite/gdb.python/py-varobj.c
+++ b/gdb/testsuite/gdb.python/py-varobj.c
@@ -21,6 +21,8 @@ struct test {
 
 struct test tval = {23};
 
+struct test *test_ptr = &tval;
+
 int main () {
   return 0;
 }
diff --git a/gdb/testsuite/gdb.python/py-varobj.exp b/gdb/testsuite/gdb.python/py-varobj.exp
index 0e0978352a5..f1eb35265b0 100644
--- a/gdb/testsuite/gdb.python/py-varobj.exp
+++ b/gdb/testsuite/gdb.python/py-varobj.exp
@@ -47,3 +47,15 @@ mi_gdb_test "-var-create tval * tval" \
 
 mi_gdb_test "-var-list-children --all-values tval" \
     ".*value=.*flicker.*"
+
+mi_gdb_test "-var-create test_ptr * test_ptr" \
+   "\\^done.*"
+
+mi_gdb_test "-var-evaluate-expression test_ptr" \
+   "\\^done,value=\"map\""
+mi_gdb_test "-var-set-visualizer test_ptr None" \
+    "\\^done.*"
+# mi_gdb_test "-var-update test_ptr" ".*"
+mi_gdb_test "-var-evaluate-expression test_ptr" \
+   "\\^done,value=\"$hex.*\"" \
+    "evaluate without visualizer"
diff --git a/gdb/testsuite/gdb.python/py-varobj.py b/gdb/testsuite/gdb.python/py-varobj.py
index bc31a198297..578ad14d4df 100644
--- a/gdb/testsuite/gdb.python/py-varobj.py
+++ b/gdb/testsuite/gdb.python/py-varobj.py
@@ -12,6 +12,7 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+import gdb
 import gdb.printing
 
 
@@ -30,6 +31,7 @@ def str_lookup_function(val):
     lookup_tag = val.type.tag
     if lookup_tag == "test":
         return TestPrinter(val)
-
+    if val.type.code == gdb.TYPE_CODE_PTR and val.type.target().tag == "test":
+        return TestPrinter(val.dereference())
 
 gdb.printing.register_pretty_printer(None, str_lookup_function)
diff --git a/gdb/varobj.c b/gdb/varobj.c
index 3ae8d728e82..a4fcbffc311 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -1393,6 +1393,9 @@ varobj_set_visualizer (struct varobj *var, const char *visualizer)
   /* If there are any children now, wipe them.  */
   varobj_delete (var, 1 /* children only */);
   var->num_children = -1;
+
+  /* Also be sure to reset the print value.  */
+  varobj_set_display_format (var, var->format);
 #else
   error (_("Python support required"));
 #endif
@@ -2212,6 +2215,12 @@ varobj_value_get_print_value (struct value *value,
 		return "{...}";
 	    }
 	}
+      else
+	{
+	  /* If we've made it here, we don't want a pretty-printer --
+	     if we had one, it would already have been used.  */
+	  opts.raw = true;
+	}
     }
 #endif
 

-- 
2.40.1

Re: [PATCH 0/4] Fix two varobj bugs

[Keith Seitz]

On 9/1/23 07:47, Tom Tromey via Gdb-patches wrote:
> This series fixes a couple of small varobj bugs, and includes a couple
> small cleanups as well.
> 
> Regression tested on x86-64 Fedora 36.

I've looked over this series, and I don't see any issues.

Reviewed-by: Keith Seitz <keiths@redhat.com>

Keith

> ---
> Tom Tromey (4):
>        Allow pretty-printer 'children' method to return strings
>        Remove dead code from varobj_set_display_format
>        Remove variable_default_display
>        Fix bug in -var-evaluate-expression
> 
>   gdb/testsuite/gdb.python/py-varobj.c   | 28 ++++++++++++++++
>   gdb/testsuite/gdb.python/py-varobj.exp | 61 ++++++++++++++++++++++++++++++++++
>   gdb/testsuite/gdb.python/py-varobj.py  | 37 +++++++++++++++++++++
>   gdb/varobj.c                           | 39 ++++++++--------------
>   4 files changed, 140 insertions(+), 25 deletions(-)
> ---
> base-commit: b47a4f92de5c12ac8bdac57b0413e3e05d411832
> change-id: 20230901-varobj-fixes-9926a7876ec6
> 
> Best regards,

Re: [PATCH 0/4] Fix two varobj bugs

[Tom Tromey]

>> This series fixes a couple of small varobj bugs, and includes a couple
>> small cleanups as well.
>> Regression tested on x86-64 Fedora 36.

Keith> I've looked over this series, and I don't see any issues.

Keith> Reviewed-by: Keith Seitz <keiths@redhat.com>

Thanks, I'm checking this in.

Tom