Patchwork MI: Allow non-raw varobj evaluation

login
register
mail settings
Submitter Simon Marchi
Date Feb. 2, 2018, 4:26 a.m.
Message ID <a7d57157-139c-57d1-383a-65ac8047056e@polymtl.ca>
Download mbox | patch
Permalink /patch/25758/
State New
Headers show

Comments

Simon Marchi - Feb. 2, 2018, 4:26 a.m.
On 2018-01-25 06:11 AM, Leszek Swirski via gdb-patches wrote:
> On Thu, Jan 25, 2018 at 2:53 AM, Simon Marchi <simon.marchi@polymtl.ca> wrote:
>>
>> Could you give an example to reproduce this problem?  I tried with a vector of vector, but -var-evaluate-expression just prints "{...}":
>>
>>   -var-evaluate-expression var1
>>   ^done,value="{...}"
>>
>> so I am not sure what problem you are after.
> 
> Hi Simon,
> 
> So, this is a bit of an edge case, which I encountered with the
> chromium pretty printers. Effectively, it's when a pretty printer
> returns another object in its to_string, rather than a string.
> 
> Consider the following code:
> 
>         struct Foo { int val; };
>         struct Wrapper { Foo foo; };
> 
>         int main() {
>                 Wrapper w;
>                 w.foo.val = 23;
>         }
> 
> and this pretty printer file:
> 
>         import gdb.printing
> 
>         class FooPrinter:
>           def __init__(self, val):
>             self.val = val
>           def to_string(self):
>             return "Foo" + str(self.val["val"])
> 
>         class WrapperPrinter:
>           def __init__(self, val):
>             self.val = val
>           def to_string(self):
>             return self.val["foo"]
> 
>         test_printer = gdb.printing.RegexpCollectionPrettyPrinter("test")
>         test_printer.add_printer('Foo', '^Foo$', FooPrinter)
>         test_printer.add_printer('Wrapper', '^Wrapper$', WrapperPrinter)
> 
>         gdb.printing.register_pretty_printer(None, test_printer)
> 
> Setting a breakpoint at the end of the function, we call the following commands:
> 
>         -enable-pretty-printing
>         ^done
> 
>         -var-create var_w @ w
>         ^done,name="var_w",numchild="0",value="{val =
> 23}",type="Wrapper",dynamic="1",has_more="0"
>         -var-create var_w_foo @ w.foo
>         ^done,name="var_w_foo",numchild="0",value="Foo23",type="Foo",dynamic="1",has_more="0"
> 
>         -var-evaluate-expression var_w
>         ^done,value="{val = 23}"
>         -var-evaluate-expression var_w_foo
>         ^done,value="Foo23"
> 
>         -data-evaluate-expression w
>         ^done,value="Foo23"
>         -data-evaluate-expression w.foo
>         ^done,value="Foo23"
> 
> So, in the -var-evaluate-expression var_w case, we print the "raw"
> value of w.foo, while in the -data-evaluate-expression w case, we
> print the pretty printed w.foo value. After my patch, all of the above
> print "Foo23".
> 
> Note that this isn't encountered very often, probably because it
> disappears if I replace `return self.val["foo"]` with `return
> str(self.val["foo"])`. But, it does feel like the wrong behaviour.

Thanks for the explanation, the problem is now very clear, and I also think the current
behavior is not the right one.  It's documented that to_string can return a gdb.Value,
and there's no reason why we shouldn't apply pretty printers to this value.  I put your
explanation in an updated patch below.

Also, from what I understand we don't have a test for this, so I converted your example
to a test case.  If you are fine, I would push the following patch.


From f52e7d0ce13095db5e73466a55fe6752de46a5ee Mon Sep 17 00:00:00 2001
From: Leszek Swirski via gdb-patches <gdb-patches@sourceware.org>
Date: Thu, 1 Feb 2018 23:23:28 -0500
Subject: [PATCH] MI: Allow non-raw varobj evaluation

Make the MI variable object expression evaluation, with the
-var-evaluate-expression command, recursively call pretty printers, to
match the output of normal expression printing.

Consider the following code:

        struct Foo { int val; };
        struct Wrapper { Foo foo; };

        int main() {
                Wrapper w;
                w.foo.val = 23;
        }

and this pretty printer file:

        import gdb.printing

        class FooPrinter:
          def __init__(self, val):
            self.val = val
          def to_string(self):
            return "Foo" + str(self.val["val"])

        class WrapperPrinter:
          def __init__(self, val):
            self.val = val
          def to_string(self):
            return self.val["foo"]

        test_printer = gdb.printing.RegexpCollectionPrettyPrinter("test")
        test_printer.add_printer('Foo', '^Foo$', FooPrinter)
        test_printer.add_printer('Wrapper', '^Wrapper$', WrapperPrinter)

        gdb.printing.register_pretty_printer(None, test_printer)

Setting a breakpoint at the end of the function, we call the following commands:

        -enable-pretty-printing
        ^done

        -var-create var_w @ w
        ^done,name="var_w",numchild="0",value="{val = 23}",type="Wrapper",dynamic="1",has_more="0"
        -var-create var_w_foo @ w.foo
        ^done,name="var_w_foo",numchild="0",value="Foo23",type="Foo",dynamic="1",has_more="0"

        -var-evaluate-expression var_w
        ^done,value="{val = 23}"
        -var-evaluate-expression var_w_foo
        ^done,value="Foo23"

        -data-evaluate-expression w
        ^done,value="Foo23"
        -data-evaluate-expression w.foo
        ^done,value="Foo23"

So, in the -var-evaluate-expression var_w case, we print the "raw" value
of w.foo, while in the -data-evaluate-expression w case, we print the
pretty printed w.foo value. After this patch, all of the above print
"Foo23".

gdb/ChangeLog:

	* varobj.c (varobj_formatted_print_options): Allow recursive
	pretty printing if pretty printing is enabled.

gdb/testsuite/ChangeLog:

	* gdb.python/py-prettyprint.c
	(struct to_string_returns_value_inner,
	struct to_string_returns_value_wrapper): New.
	(main): Add tsrvw variable.
	* gdb.python/py-prettyprint.py (ToStringReturnsValueInner,
	ToStringReturnsValueWrapper): New classes.
	(register_pretty_printers): Register new pretty-printers.
	* gdb.python/py-prettyprint.exp (run_lang_tests): Test printing
	recursive pretty printer.
	* gdb.python/py-mi.exp: Likewise.
---
 gdb/ChangeLog                               |  5 +++++
 gdb/testsuite/ChangeLog                     | 14 ++++++++++++++
 gdb/testsuite/gdb.python/py-mi.exp          | 10 ++++++++++
 gdb/testsuite/gdb.python/py-prettyprint.c   | 11 +++++++++++
 gdb/testsuite/gdb.python/py-prettyprint.exp |  4 ++++
 gdb/testsuite/gdb.python/py-prettyprint.py  | 26 +++++++++++++++++++++++++-
 gdb/varobj.c                                |  2 +-
 7 files changed, 70 insertions(+), 2 deletions(-)
Doug Evans via gdb-patches - Feb. 2, 2018, 9 a.m.
On Fri, 2 Feb 2018, 04:27 Simon Marchi, <simon.marchi@polymtl.ca> wrote:
>
> Also, from what I understand we don't have a test for this, so I converted your example
> to a test case.  If you are fine, I would push the following patch.

That looks fantastic, I'm happy for you to push the amended patch and
thank you for your due diligence.
Simon Marchi - Feb. 2, 2018, 7:04 p.m.
On 2018-02-02 04:00, Leszek Swirski via gdb-patches wrote:
> On Fri, 2 Feb 2018, 04:27 Simon Marchi, <simon.marchi@polymtl.ca> 
> wrote:
>> 
>> Also, from what I understand we don't have a test for this, so I 
>> converted your example
>> to a test case.  If you are fine, I would push the following patch.
> 
> That looks fantastic, I'm happy for you to push the amended patch and
> thank you for your due diligence.

Thanks for the fix, the patch is now pushed.

Simon

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 7081f9ec53..3e384b5c29 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@ 
+2018-01-24  Leszek Swirski  <leszeks@google.com>
+
+	* varobj.c (varobj_formatted_print_options): Allow recursive
+	pretty printing if pretty printing is enabled.
+
 2018-02-01  Leszek Swirski  <leszeks@google.com>

 	* c-exp.y (lex_one_token, classify_name, yylex): Don't classify
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 1558ed18fe..0251584448 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,17 @@ 
+2018-02-01  Simon Marchi  <simon.marchi@polymtl.ca>
+	    Leszek Swirski  <leszeks@google.com>
+
+	* gdb.python/py-prettyprint.c
+	(struct to_string_returns_value_inner,
+	struct to_string_returns_value_wrapper): New.
+	(main): Add tsrvw variable.
+	* gdb.python/py-prettyprint.py (ToStringReturnsValueInner,
+	ToStringReturnsValueWrapper): New classes.
+	(register_pretty_printers): Register new pretty-printers.
+	* gdb.python/py-prettyprint.exp (run_lang_tests): Test printing
+	recursive pretty printer.
+	* gdb.python/py-mi.exp: Likewise.
+
 2018-02-01  Leszek Swirski  <leszeks@google.com>

 	* gdb.cp/filename.cc, gdb.cp/filename.exp: Test that member
diff --git a/gdb/testsuite/gdb.python/py-mi.exp b/gdb/testsuite/gdb.python/py-mi.exp
index bbe1266724..f9829478f0 100644
--- a/gdb/testsuite/gdb.python/py-mi.exp
+++ b/gdb/testsuite/gdb.python/py-mi.exp
@@ -295,6 +295,16 @@  mi_gdb_test "-var-evaluate-expression me" \
 mi_create_dynamic_varobj children_as_list children_as_list 1 \
     "printer whose children are returned as a list"

+# Test that when a pretty-printer returns a gdb.Value in its to_string, we call
+# the pretty-printer of that value too.
+mi_create_varobj_checked tsrvw tsrvw  \
+    "struct to_string_returns_value_wrapper" \
+    "create tsrvw varobj"
+mi_check_varobj_value tsrvw "Inner to_string 1989" "check tsrvw varobj value"
+mi_gdb_test "-data-evaluate-expression tsrvw" \
+    "\\^done,value=\"Inner to_string 1989\"" \
+    "check tsrvw expression value"
+
 # Regression test for bug 14741.
 mi_continue_to_line \
     [gdb_get_line_number {breakpoint bug 14741} ${srcfile}] \
diff --git a/gdb/testsuite/gdb.python/py-prettyprint.c b/gdb/testsuite/gdb.python/py-prettyprint.c
index fc0d829e04..35ef5e0756 100644
--- a/gdb/testsuite/gdb.python/py-prettyprint.c
+++ b/gdb/testsuite/gdb.python/py-prettyprint.c
@@ -112,6 +112,16 @@  class Fake
 };
 #endif

+struct to_string_returns_value_inner
+{
+  int val;
+};
+
+struct to_string_returns_value_wrapper
+{
+  struct to_string_returns_value_inner inner;
+};
+
 struct substruct {
   int a;
   int b;
@@ -284,6 +294,7 @@  main ()
   struct lazystring estring, estring2, estring3;
   struct hint_error hint_error;
   struct children_as_list children_as_list;
+  struct to_string_returns_value_wrapper tsrvw = { { 1989 } };

   nstype.elements = narray;
   nstype.len = 0;
diff --git a/gdb/testsuite/gdb.python/py-prettyprint.exp b/gdb/testsuite/gdb.python/py-prettyprint.exp
index 5df6f69158..2671cb8471 100644
--- a/gdb/testsuite/gdb.python/py-prettyprint.exp
+++ b/gdb/testsuite/gdb.python/py-prettyprint.exp
@@ -64,6 +64,10 @@  proc run_lang_tests {exefile lang} {

     gdb_test "print arraystruct" " = {$nl *y = 7, *$nl *x = { a=<23> b=<$hex>,  a=<24> b=<$hex>} *$nl *}"

+    # Test that when a pretty-printer returns a gdb.Value in its to_string, we
+    # call the pretty-printer of that value too.
+    gdb_test "print tsrvw" " = Inner to_string 1989"
+
     if {$lang == "c++"} {
 	gdb_test "print cps" "=  a=<8> b=<$hex>"
 	gdb_test "print cpss" " = {$nl *zss = 9, *$nl *s =  a=<10> b=<$hex>$nl}"
diff --git a/gdb/testsuite/gdb.python/py-prettyprint.py b/gdb/testsuite/gdb.python/py-prettyprint.py
index ea6caaea43..7357f05cc9 100644
--- a/gdb/testsuite/gdb.python/py-prettyprint.py
+++ b/gdb/testsuite/gdb.python/py-prettyprint.py
@@ -84,6 +84,25 @@  class NoStringContainerPrinter (object):
     def children(self):
         return _iterator_except (self.val['elements'], self.val['len'])

+# See ToStringReturnsValueWrapper.
+class ToStringReturnsValueInner:
+
+    def __init__(self, val):
+        self.val = val
+
+    def to_string(self):
+        return 'Inner to_string {}'.format(int(self.val['val']))
+
+# Test a printer that returns a gdb.Value in its to_string.  That gdb.Value
+# also has its own pretty-printer.
+class ToStringReturnsValueWrapper:
+
+    def __init__(self, val):
+        self.val = val
+
+    def to_string(self):
+        return self.val['inner']
+
 class pp_s (object):
     def __init__(self, val):
         self.val = val
@@ -316,7 +335,12 @@  def register_pretty_printers ():
     pretty_printers_dict[re.compile ('^string_repr$')] = string_print
     pretty_printers_dict[re.compile ('^container$')] = ContainerPrinter
     pretty_printers_dict[re.compile ('^justchildren$')] = NoStringContainerPrinter
-
+
+    pretty_printers_dict[re.compile ('^struct to_string_returns_value_inner$')] = ToStringReturnsValueInner
+    pretty_printers_dict[re.compile ('^to_string_returns_value_inner$')] = ToStringReturnsValueInner
+    pretty_printers_dict[re.compile ('^struct to_string_returns_value_wrapper$')] = ToStringReturnsValueWrapper
+    pretty_printers_dict[re.compile ('^to_string_returns_value_wrapper$')] = ToStringReturnsValueWrapper
+
     pretty_printers_dict[re.compile ('^struct ns$')]  = pp_ns
     pretty_printers_dict[re.compile ('^ns$')]  = pp_ns

diff --git a/gdb/varobj.c b/gdb/varobj.c
index b6a2d8f369..f23243f3b7 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -2274,7 +2274,7 @@  varobj_formatted_print_options (struct value_print_options *opts,
 {
   get_formatted_print_options (opts, format_code[(int) format]);
   opts->deref_ref = 0;
-  opts->raw = 1;
+  opts->raw = !pretty_printing;
 }

 std::string