[patch] Use the string returned by pretty printer for MI varobjs instead of "{...}"

[patch] Use the string returned by pretty printer for MI varobjs instead of "{...}"

[xgsa]

[-- Attachment #1: Type: text/plain, Size: 728 bytes --]

Hi,

The documentation for -var-create says that "for a varobj whose type is some sort of aggregate (e.g., a struct), or for a dynamic varobj, the 'value' attribute will not be interesting". That's not true, as dynamic varobj could also have no children (so its value will be not "{...}" and will be interesting). However I think the string value of varobj that has pretty printer installed should always be returned via MI. It could contain such a useful information as container length or reference count for smart pointers. Moreover, it seems that this information cannot be accessed via MI in another way. This patch contains the proposed fix, documentation and test suite update.

No regressions on linux32.


Thanks,
Anton

[-- Attachment #2: dynamic_show_str3.ChangeLog --]
[-- Type: text/plain, Size: 785 bytes --]

gdb/doc/ChangeLog:

2012-04-22  Anton Gorenkov <xgsa@yandex.ru>

	(GDB/MI Variable Objects): Fix the description for the "value" 
	attribute of -var-create MI command.


gdb/testsuite/ChangeLog:

2012-04-22  Anton Gorenkov <xgsa@yandex.ru>

	* gdb.python/py-mi.exp: Use mi_create_floating_varobj_with_value 
	instead of mi_create_dynamic_varobj and provide the varobj value.
	* lib/mi-support.exp (mi_create_floating_varobj_with_value): New function.
	(mi_create_floating_varobj): updated to avoid code duplication.
	(mi_create_dynamic_varobj): Remove.


gdb/ChangeLog:

2012-04-22  Anton Gorenkov <xgsa@yandex.ru>

	* varobj.c (dynamic_varobj_has_child_method): Remove function.
	(value_get_print_value): Remove check for dynamic varobj children
	and add None return value handling.


[-- Attachment #3: dynamic_show_str3.diff --]
[-- Type: text/plain, Size: 6238 bytes --]

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 574809d..de50cca 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -29482,8 +29482,7 @@ reliable for a dynamic varobj.  Instead, you must examine the
 
 @item value
 The varobj's scalar value.  For a varobj whose type is some sort of
-aggregate (e.g., a @code{struct}), or for a dynamic varobj, this value
-will not be interesting.
+aggregate (e.g., a @code{struct}), this value will not be interesting.
 
 @item type
 The varobj's type.  This is a string representation of the type, as
diff --git a/gdb/testsuite/gdb.python/py-mi.exp b/gdb/testsuite/gdb.python/py-mi.exp
index 8d54a8d..bf3ed72 100644
--- a/gdb/testsuite/gdb.python/py-mi.exp
+++ b/gdb/testsuite/gdb.python/py-mi.exp
@@ -51,7 +51,7 @@ mi_gdb_test "python execfile ('${remote_python_file}')" ""
 mi_continue_to_line [gdb_get_line_number {MI breakpoint here} ${srcfile}] \
   "step to breakpoint"
 
-mi_create_dynamic_varobj container c \
+mi_create_floating_varobj_with_value container c {{...}} \
   "create container varobj, no pretty-printing"
 
 mi_list_varobj_children container {
@@ -62,7 +62,7 @@ mi_list_varobj_children container {
 
 mi_delete_varobj container "delete varobj"
 
-mi_create_dynamic_varobj nscont nstype \
+mi_create_floating_varobj_with_value nscont nstype {{...}} \
   "create nscont varobj, no pretty-printing"
 
 mi_list_varobj_children nscont {
@@ -89,7 +89,7 @@ mi_gdb_test "-var-update string" \
     "\\^done,changelist=\\\[{name=\"string\",in_scope=\"true\",type_changed=\"false\",dynamic=\"1\",has_more=\"0\"}\\\]" \
     "update string varobj after assignment"
 
-mi_create_dynamic_varobj container c \
+mi_create_floating_varobj_with_value container c {container \\"container\\" with 0 elements} \
   "create container varobj"
 
 mi_list_varobj_children container {
@@ -204,7 +204,7 @@ mi_next "next over update 5"
 
 # Regression test: examine an object that has no children, then update
 # it to ensure that we don't print the children.
-mi_create_dynamic_varobj container2 c2 \
+mi_create_floating_varobj_with_value container2 c2 {container \\"container2\\" with 0 elements} \
   "create second container varobj"
 
 mi_gdb_test "-var-update container2" \
@@ -225,7 +225,7 @@ mi_continue_to_line \
     [gdb_get_line_number {MI outer breakpoint here} ${srcfile}] \
     "step to outer breakpoint"
 
-mi_create_dynamic_varobj outer outer \
+mi_create_floating_varobj_with_value outer outer {x = 0} \
   "create outer varobj"
 
 mi_list_varobj_children outer {
@@ -255,7 +255,7 @@ mi_continue_to_line \
     [gdb_get_line_number {break to inspect struct and union} ${srcfile}] \
     "step to outer breakpoint"
 
-mi_create_dynamic_varobj nscont nstype \
+mi_create_floating_varobj_with_value nscont nstype {{...}} \
   "create nstype varobj"
 
 mi_list_varobj_children nscont {
@@ -277,7 +277,7 @@ mi_gdb_test "-var-set-visualizer nscont gdb.default_visualizer" \
 
 mi_gdb_test "python exception_flag = True" ""
 
-mi_create_dynamic_varobj nstype2 nstype2 \
+mi_create_floating_varobj_with_value nstype2 nstype2 {{...}} \
   "create nstype2 varobj"
 
 mi_list_varobj_children nstype2 {
diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp
index 401565d..84ff83a 100644
--- a/gdb/testsuite/lib/mi-support.exp
+++ b/gdb/testsuite/lib/mi-support.exp
@@ -1246,12 +1246,18 @@ proc mi_create_varobj { name expression testname } {
         $testname
 }
 
-proc mi_create_floating_varobj { name expression testname } {
+# Creates floating varobj named NAME for EXPRESSION and checks its VALUE.
+proc mi_create_floating_varobj_with_value {name expression value testname} {
     mi_gdb_test "-var-create $name @ $expression" \
-        "\\^done,name=\"$name\",numchild=\"\(-1\|\[0-9\]+\)\",value=\".*\",type=.*" \
+        "\\^done,name=\"$name\",numchild=\"\(-1\|\[0-9\]+\)\",value=\"$value\",type=.*" \
         $testname
 }
 
+# Same as mi_create_floating_varobj_with_value, but does not check the value.
+proc mi_create_floating_varobj { name expression testname } {
+    mi_create_floating_varobj_with_value $name $expression ".*" $testname
+}
+
 
 # Same as mi_create_varobj, but also checks the reported type
 # of the varobj.
@@ -1261,14 +1267,6 @@ proc mi_create_varobj_checked { name expression type testname } {
         $testname
 }
 
-# Same as mi_create_floating_varobj, but assumes the test is creating
-# a dynamic varobj that has children, so the value must be "{...}".
-proc mi_create_dynamic_varobj {name expression testname} {
-    mi_gdb_test "-var-create $name @ $expression" \
-        "\\^done,name=\"$name\",numchild=\"\(-1\|\[0-9\]+\)\",value=\"{\\.\\.\\.}\",type=.*" \
-        $testname
-}
-
 # Deletes the specified NAME. 
 proc mi_delete_varobj { name testname } {
     mi_gdb_test "-var-delete $name" \
diff --git a/gdb/varobj.c b/gdb/varobj.c
index 3086499..7bd24f5 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -1053,19 +1053,6 @@ install_dynamic_child (struct varobj *var,
     }
 }
 
-static int
-dynamic_varobj_has_child_method (struct varobj *var)
-{
-  struct cleanup *back_to;
-  PyObject *printer = var->pretty_printer;
-  int result;
-
-  back_to = varobj_ensure_python_env (var);
-  result = PyObject_HasAttr (printer, gdbpy_children_cst);
-  do_cleanups (back_to);
-  return result;
-}
-
 #endif
 
 static int
@@ -2866,14 +2853,6 @@ value_get_print_value (struct value *value, enum varobj_display_formats format,
 
     if (value_formatter)
       {
-	/* First check to see if we have any children at all.  If so,
-	   we simply return {...}.  */
-	if (dynamic_varobj_has_child_method (var))
-	  {
-	    do_cleanups (old_chain);
-	    return xstrdup ("{...}");
-	  }
-
 	if (PyObject_HasAttr (value_formatter, gdbpy_to_string_cst))
 	  {
 	    struct value *replacement;
@@ -2888,6 +2867,13 @@ value_get_print_value (struct value *value, enum varobj_display_formats format,
 	      {
 		make_cleanup_py_decref (output);
 
+		/* Handle returned None properly.  */
+		if (output == Py_None)
+		  {
+		    do_cleanups (old_chain);
+		    return xstrdup ("{...}");
+		  }
+
 		/* If this is a lazy string, extract it.  For lazy
 		   strings we always print as a string, so set
 		   string_print.  */

Re: [patch] Use the string returned by pretty printer for MI varobjs instead of "{...}"

[xgsa]

Ping, could anybody review the patch?


Thanks,
Anton

-------- Original message --------
> Hi,
>
> The documentation for -var-create says that "for a varobj whose type is some sort of aggregate (e.g., a struct), or for a dynamic varobj, the 'value' attribute will not be interesting". That's not true, as dynamic varobj could also have no children (so its value will be not "{...}" and will be interesting). However I think the string value of varobj that has pretty printer installed should always be returned via MI. It could contain such a useful information as container length or reference count for smart pointers. Moreover, it seems that this information cannot be accessed via MI in another way. This patch contains the proposed fix, documentation and test suite update.
>
> No regressions on linux32.
>
>
> Thanks,
> Anton

Re: [patch] Use the string returned by pretty printer for MI varobjs instead of "{...}"

[xgsa]

Ping! Please, review the patch.

Anton.

-------- Original message --------
> Hi,
>
> The documentation for -var-create says that "for a varobj whose type is some sort of aggregate (e.g., a struct), or for a dynamic varobj, the 'value' attribute will not be interesting". That's not true, as dynamic varobj could also have no children (so its value will be not "{...}" and will be interesting). However I think the string value of varobj that has pretty printer installed should always be returned via MI. It could contain such a useful information as container length or reference count for smart pointers. Moreover, it seems that this information cannot be accessed via MI in another way. This patch contains the proposed fix, documentation and test suite update.
>
> No regressions on linux32.
>
>
> Thanks,
> Anton

Re: [patch] Use the string returned by pretty printer for MI varobjs instead of "{...}"

[Joel Brobecker]

> Ping! Please, review the patch.

I tried, a couple of times. But in the end, I couldn't find enough
time to understand that part of the code and approve. My guess
is that Tom has the best knowlege of the area. I'd wait until
he comes back...

-- 
Joel

Re: [patch] Use the string returned by pretty printer for MI varobjs instead of "{...}"

[xgsa]

Tom, it seems you are here. Please, look through this path when you have 
time.

Thanks,
Anton.

-------- Original message --------
>> Ping! Please, review the patch.
> I tried, a couple of times. But in the end, I couldn't find enough
> time to understand that part of the code and approve. My guess
> is that Tom has the best knowlege of the area. I'd wait until
> he comes back...
>

Re: [patch] Use the string returned by pretty printer for MI varobjs instead of "{...}"

[xgsa]

Ping! I am still asking about the review. It should not be too long. The 
patch contains only a few changes in code and test suite fixture for it.

Thanks,
Anton

-------- Original message --------
> Tom, it seems you are here. Please, look through this path when you 
> have time.
>
> Thanks,
> Anton.
>
> -------- Original message --------
>>> Ping! Please, review the patch.
>> I tried, a couple of times. But in the end, I couldn't find enough
>> time to understand that part of the code and approve. My guess
>> is that Tom has the best knowlege of the area. I'd wait until
>> he comes back...
>>
>
>
>
>

Re: [patch] Use the string returned by pretty printer for MI varobjs instead of "{...}"

[Eli Zaretskii]

> From: xgsa <xgsa@yandex.ua>
> Envelope-From: xgsa@yandex.ru
> Date: Mon, 23 Apr 2012 00:09:57 +0400
> 
> 
> The documentation for -var-create says that "for a varobj whose type is some sort of aggregate (e.g., a struct), or for a dynamic varobj, the 'value' attribute will not be interesting". That's not true, as dynamic varobj could also have no children (so its value will be not "{...}" and will be interesting). However I think the string value of varobj that has pretty printer installed should always be returned via MI. It could contain such a useful information as container length or reference count for smart pointers. Moreover, it seems that this information cannot be accessed via MI in another way. This patch contains the proposed fix, documentation and test suite update.

The documentation part is fine with me.

Thanks.

Re: [patch] Use the string returned by pretty printer for MI varobjs instead of "{...}"

[Tom Tromey]

>>>>> "Anton" == xgsa  <xgsa@yandex.ua> writes:

Anton> The documentation for -var-create says that "for a varobj whose type
Anton> is some sort of aggregate (e.g., a struct), or for a dynamic varobj,
Anton> the 'value' attribute will not be interesting". That's not true, as
Anton> dynamic varobj could also have no children (so its value will be not
Anton> "{...}" and will be interesting). However I think the string value of
Anton> varobj that has pretty printer installed should always be returned via
Anton> MI. It could contain such a useful information as container length or
Anton> reference count for smart pointers. Moreover, it seems that this
Anton> information cannot be accessed via MI in another way. This patch
Anton> contains the proposed fix, documentation and test suite update.

The patch itself looks fine, but I am not sure whether it should go in.

My recollection is that the code originally worked this way, but
Vladimir asked for the "{...}" behavior specifically.

I've CC'd him for comments.

I am in favor of this change, as I never understood the current
behavior; so in the absence of comments I will approve it.

I'm sorry about the delay.

Tom

Re: [patch] Use the string returned by pretty printer for MI varobjs instead of "{...}"

[André Pönitz]

On Tue, May 15, 2012 at 10:28:25AM -0600, Tom Tromey wrote:
> >>>>> "Anton" == xgsa  <xgsa@yandex.ua> writes:
> 
> Anton> The documentation for -var-create says that "for a varobj whose type
> Anton> is some sort of aggregate (e.g., a struct), or for a dynamic varobj,
> Anton> the 'value' attribute will not be interesting". That's not true, as
> Anton> dynamic varobj could also have no children (so its value will be not
> Anton> "{...}" and will be interesting). However I think the string value of
> Anton> varobj that has pretty printer installed should always be returned via
> Anton> MI. It could contain such a useful information as container length or
> Anton> reference count for smart pointers. Moreover, it seems that this
> Anton> information cannot be accessed via MI in another way. This patch
> Anton> contains the proposed fix, documentation and test suite update.
> 
> The patch itself looks fine, but I am not sure whether it should go in.
> 
> My recollection is that the code originally worked this way, but
> Vladimir asked for the "{...}" behavior specifically.

It might affect frontends taking a "{...}" as indication that there
are childrens. In fact, I just found some code using that as a shortcut.

The safe way would be to add a new field 'formatted-value' or similar
with the new value, and perhaps even announce the feature using
-list-features, but I doubt this is worth the trouble.

I am not sure how frontend developers interpret "27.4 gdb/mi Development
and Front Ends", or how much they rely on it in general.

The list of "to be expexted changes" mentions
  * New MI commands may be added.
  * New fields may be added to the output of any MI command.
  * The range of values for fields with specified values, e.g., in_scope
    (see -var-update) may be extended. 

It's not directly obviously to me that this list is meant to be
exhaustive, and even if, whether the third bullet point covers
changing existing values, but there is precedence that changing
values was prefered over adding a new field in the past, so there
is no real reason to not do it again.

Andre'

Re: [patch] Use the string returned by pretty printer for MI varobjs instead of "{...}"

[Tom Tromey]

>>>>> "André" == André Pönitz <andre.poenitz@mathematik.tu-chemnitz.de> writes:

Tom> My recollection is that the code originally worked this way, but
Tom> Vladimir asked for the "{...}" behavior specifically.

André> It might affect frontends taking a "{...}" as indication that there
André> are childrens. In fact, I just found some code using that as a shortcut.

That code is just plain broken, though.  The has_more field conveys this
information.

Tom

Re: [patch] Use the string returned by pretty printer for MI varobjs instead of "{...}"

[André Pönitz]

On Wed, May 16, 2012 at 08:58:42AM -0600, Tom Tromey wrote:
> >>>>> <andre.poenitz@mathematik.tu-chemnitz.de> writes:
> 
> Tom> My recollection is that the code originally worked this way, but
> Tom> Vladimir asked for the "{...}" behavior specifically.
> 
> André> It might affect frontends taking a "{...}" as indication that
> André> there are childrens. In fact, I just found some code using that
> André> as a shortcut.
> 
> That code is just plain broken, though.  The has_more field conveys
> this information.

If you say so.

'git blame' thinks you added it in September 2009, so gdb 7.0 is
probably the first one that has it.  Calling code "broken" just
because it handles (also...) "legacy" setups is a bit of a stretch
in my opinion.

In any case, I was merely trying to convey the idea that an
unsuspecting frontend implementor might read the documentation
in a way that would be incompatible with the change.

I am personally not affected by any change in the -var-*
output, so maybe I should not have bothered to comment at all.

If you really want to read more into the comment than what was
actually written, then perhaps a friendly nudge to adapt the
documentation to existing practice.

Andre'

Re: [patch] Use the string returned by pretty printer for MI varobjs instead of "{...}"

[Vladimir Prus]

On 17/05/12 16:19, André Pönitz wrote:
> On Wed, May 16, 2012 at 08:58:42AM -0600, Tom Tromey wrote:
>>>>>>> <andre.poenitz@mathematik.tu-chemnitz.de>  writes:
>>
>> Tom>  My recollection is that the code originally worked this way, but
>> Tom>  Vladimir asked for the "{...}" behavior specifically.
>>
>> André>  It might affect frontends taking a "{...}" as indication that
>> André>  there are childrens. In fact, I just found some code using that
>> André>  as a shortcut.
>>
>> That code is just plain broken, though.  The has_more field conveys
>> this information.
>
> If you say so.
>
> 'git blame' thinks you added it in September 2009, so gdb 7.0 is
> probably the first one that has it.  Calling code "broken" just
> because it handles (also...) "legacy" setups is a bit of a stretch
> in my opinion.
>
> In any case, I was merely trying to convey the idea that an
> unsuspecting frontend implementor might read the documentation
> in a way that would be incompatible with the change.
>
> I am personally not affected by any change in the -var-*
> output, so maybe I should not have bothered to comment at all.
>
> If you really want to read more into the comment than what was
> actually written, then perhaps a friendly nudge to adapt the
> documentation to existing practice.

I has been quite a while, but I think I was primarily concerned that GDB would abuse value string on varobj
to show what GDB thinks is the right rendering of data. E.g. while "[5]" as top-level value might be a reasonable
rendition for an array with 5 children, it's not entirely clear why GDB's machine interface should take liberties
at telling frontend how to render that. Neither "[", nor 5, nor "]" is integral part of data, and as soon as
GDB outputs that, frontend has no way of knowing whether this is GDB trying to be helpful, or really interesting
data that cannot be obtained in any other way.

-- 
Vladimir Prus
CodeSourcery / Mentor Graphics
http://www.mentor.com/embedded-software/

Re: [patch] Use the string returned by pretty printer for MI varobjs instead of "{...}"

[xgsa]

> I has been quite a while, but I think I was primarily concerned that 
> GDB would abuse value string on varobj
> to show what GDB thinks is the right rendering of data. E.g. while 
> "[5]" as top-level value might be a reasonable
> rendition for an array with 5 children, it's not entirely clear why 
> GDB's machine interface should take liberties
> at telling frontend how to render that. Neither "[", nor 5, nor "]" is 
> integral part of data, and as soon as
> GDB outputs that, frontend has no way of knowing whether this is GDB 
> trying to be helpful, or really interesting
> data that cannot be obtained in any other way. 

I cannot figure out why the frontend need to know it? I think, it should 
just output what GDB returns, cause GDB knows better how to visualize 
the concrete data type due to pretty printers mechanism. Otherwise, the 
frontend should have something similar to pretty printers that will 
visualize different types in a comfortable way. In any case if the 
implemented behavior is not desirable the string() method of pretty 
printer could be left unimplemented (or return None). It will give the 
behavior you described above, but could also be extended if necessary.


Thanks,
Anton

Re: [patch] Use the string returned by pretty printer for MI varobjs instead of "{...}"

[xgsa]

So could I check this patch in or not? I have written my arguments for 
these changes, Vladimir has written his ones against them. It seems Tom 
in favor of these changes too. So what is the final decision? Could 
somebody say "these changes are approved" or "no, these changes should 
not go in" or what should be done in such cases?

Thanks,
Anton

-------- Original message --------
>> I has been quite a while, but I think I was primarily concerned that 
>> GDB would abuse value string on varobj
>> to show what GDB thinks is the right rendering of data. E.g. while 
>> "[5]" as top-level value might be a reasonable
>> rendition for an array with 5 children, it's not entirely clear why 
>> GDB's machine interface should take liberties
>> at telling frontend how to render that. Neither "[", nor 5, nor "]" 
>> is integral part of data, and as soon as
>> GDB outputs that, frontend has no way of knowing whether this is GDB 
>> trying to be helpful, or really interesting
>> data that cannot be obtained in any other way. 
>
> I cannot figure out why the frontend need to know it? I think, it 
> should just output what GDB returns, cause GDB knows better how to 
> visualize the concrete data type due to pretty printers mechanism. 
> Otherwise, the frontend should have something similar to pretty 
> printers that will visualize different types in a comfortable way. In 
> any case if the implemented behavior is not desirable the string() 
> method of pretty printer could be left unimplemented (or return None). 
> It will give the behavior you described above, but could also be 
> extended if necessary.
>
>
> Thanks,
> Anton
>
>

Re: [patch] Use the string returned by pretty printer for MI varobjs instead of "{...}"

[Tom Tromey]

>>>>> "André" == André Pönitz <andre.poenitz@mathematik.tu-chemnitz.de> writes:

André> 'git blame' thinks you added it in September 2009, so gdb 7.0 is
André> probably the first one that has it.  Calling code "broken" just
André> because it handles (also...) "legacy" setups is a bit of a stretch
André> in my opinion.

We're only talking about dynamic varobjs here, which the MI user must
explicitly request.  Checking a dynamic varobj for "{...}" has always
been wrong, since they were first introduced.

Looking at this for non-dynamic varobjs... well, I wouldn't do that, but
it isn't relevant.

André> If you really want to read more into the comment than what was
André> actually written, then perhaps a friendly nudge to adapt the
André> documentation to existing practice.

As far as I know the documentation is correct.

Tom