Add real-type to MI output

Message ID 1424132545-26322-1-git-send-email-simon.marchi@ericsson.com
State New, archived
Headers

Commit Message

Simon Marchi Feb. 17, 2015, 12:22 a.m. UTC
  This patch adds a new "real-type" field to MI output, so that whenever
we give the type of a variable, we also give its real type. By real
type, I mean the type after resolving all layers of typedefs (i.e. what
check_typedef does).

[more about the name choice lower]

We are trying to solve a minor annoyance in Eclipse CDT. When a pointer
is declared using a typedef in a structure, such as:

  typedef int* IntPointer;
  struct ExampleStruct {
    IntPointer member;
  };

  struct ExampleStruct e;

the response from gdb when listing the children of "e" looks like:

  ^done,numchild="1",children=[child={name="var3.member",exp="member",numchild="1",type="IntPointer",thread-id="1"}],has_more="0"

CDT has no easy way of knowing that type "IntPointer" is a pointer, and
that it should ask gdb for the value of "member" and display it.
Instead, it assumes that it's a structure and displays "{...}" as the
value. By adding the real-type information:

...,type="IntPointer",real-type="int *",...

CDT is able to determine that "member" has a value of its own and
display it. By teaching CDT about that new field, we get the right
result:

  http://i.imgur.com/Sx5ZPfO.png

About the naming:

Right now, the term "real type" means the run-time type, in the sense of
looking at an object through a pointer to a virtual class. However, it's
not part of any API or significant user interface (only one I found is
in the output of "whatis"), so it's not set in stone. If I could
choose, I would give "real type" the meaning of the current patch, and
call the other one the "runtime type". Or is there already a term for it?
What do you think?

Other ideas that went through my mind, if "real type" is not an option:

 * canonical-type
 * resolved-type
 * underlying-type

Another issue. It doesn't affect us, but I wasn't sure about this case:

  struct blah {
    ...
  };
  typedef struct blah blah_t;

  blah_t *b;

What should real-type return for b? Right now it returns "blah_t *", but
should it return "struct blah *" instead?

I will add the doc bits, news and a test when the code part is OK.

gdb/ChangeLog:

	* mi/mi-cmd-var.c (print_varobj): Add real type field.
	(varobj_update_one): Same.
	* varobj.c (varobj_get_real_type): New function.
	* varobj.c (varobj_get_real_type): Same.
---
 gdb/mi/mi-cmd-var.c | 13 +++++++++++++
 gdb/varobj.c        | 14 ++++++++++++++
 gdb/varobj.h        |  2 ++
 3 files changed, 29 insertions(+)
  

Comments

Vladimir Prus Feb. 17, 2015, 6:26 a.m. UTC | #1
Hi Simon,

On 02/17/2015 03:22 AM, Simon Marchi wrote:
> This patch adds a new "real-type" field to MI output, so that whenever
> we give the type of a variable, we also give its real type. By real
> type, I mean the type after resolving all layers of typedefs (i.e. what
> check_typedef does).
>
> [more about the name choice lower]
>
> We are trying to solve a minor annoyance in Eclipse CDT. When a pointer
> is declared using a typedef in a structure, such as:
>
>    typedef int* IntPointer;
>    struct ExampleStruct {
>      IntPointer member;
>    };
>
>    struct ExampleStruct e;
>
> the response from gdb when listing the children of "e" looks like:
>
>    ^done,numchild="1",children=[child={name="var3.member",exp="member",numchild="1",type="IntPointer",thread-id="1"}],has_more="0"
>
> CDT has no easy way of knowing that type "IntPointer" is a pointer, and
> that it should ask gdb for the value of "member" and display it.
> Instead, it assumes that it's a structure and displays "{...}" as the
> value. By adding the real-type information:
>
> ...,type="IntPointer",real-type="int *",...
>
> CDT is able to determine that "member" has a value of its own and
> display it. By teaching CDT about that new field, we get the right
> result:
>
>    http://i.imgur.com/Sx5ZPfO.png

I don't think it's the best way to accomplish the desired effect. Instead, you can invoke -var-list-children with the
--simple-values option. Then, you'd have either "{...}" as value or the pointer value reported directly by GDB. You'd
also potentially save a roundtrip to get the value.

Actually, just did something similar internally - it was a bit tricky to get that in Eclipse DSF, and I had a special
case, but still seems better than expanding the interface with GDB?

Another case, what do you do with std::shared_ptr<Foo> or boost::shared_ptr<Foo>? Both would be naturally shown
similarly to Foo* - where you see the value immediately and can expand the object - and GDB would be able to do
so with a suitable pretty-printer, but if a frontend is supposed to parse types, that won't work.

> About the naming:
>
> Right now, the term "real type" means the run-time type, in the sense of
> looking at an object through a pointer to a virtual class. However, it's
> not part of any API or significant user interface (only one I found is
> in the output of "whatis"), so it's not set in stone. If I could
> choose, I would give "real type" the meaning of the current patch, and
> call the other one the "runtime type". Or is there already a term for it?
> What do you think?
>
> Other ideas that went through my mind, if "real type" is not an option:
>
>   * canonical-type
>   * resolved-type
>   * underlying-type
>
> Another issue. It doesn't affect us, but I wasn't sure about this case:
>
>    struct blah {
>      ...
>    };
>    typedef struct blah blah_t;
>
>    blah_t *b;
>
> What should real-type return for b? Right now it returns "blah_t *", but
> should it return "struct blah *" instead?

If if were to return 'struct blah *', what would frontend do? Try to parse the type?
Trying to parse 'int *' is already a tad risky - i.e. do you handle all possible syntax,
like:

	std::vector<My::Namespace::C> *

Quite some time ago we discussed the idea of exposing GDB type system to a frontend:

	https://www.sourceware.org/ml/gdb/2006-04/msg00016.html

However, that will not be easy, and so far, frontends did not need entire type.
  
Simon Marchi Feb. 17, 2015, 6 p.m. UTC | #2
Hi Vladimir,

Thanks for the comments.

On 02/17/2015 01:26 AM, Vladimir Prus wrote:
> I don't think it's the best way to accomplish the desired effect. Instead, you can invoke -var-list-children with the
> --simple-values option. Then, you'd have either "{...}" as value or the pointer value reported directly by GDB. You'd
> also potentially save a roundtrip to get the value.

Oh, I didn't know about --simple-values (or --all-values). Actually, I think only --all-values prints the "{...}".
Thanks for pointing that out.

So right now, CDT requests the list of children without the values, and then requests only the values of the children
that are visible in the variable view, using -var-evaluate-expression. If we used -var-list-children with --simple-values
or --all-values, we would get all the values in one shot, which is I think what we want to avoid (for big structures and/or
slow targets).

However, the solution might be even simpler. If CDT asked GDB the value (-var-evaluate-expression) of all visible children,
regardless of their types, we wouldn't have this problem to begin with. We would always get the right thing. For arrays and
structures, GDB would return "{...}". For the IntPointer variable, it will return the numerical value. We would send a few
more -var-evaluate-expression, but not that much more, since it's still limited by what the variables view displays. I just
tried it, it seems to work fine...

The only thing CDT doesn't know now is that the IntPointer value is editable. I think it would have to issue a
-var-show-attributes to know that. Do you know why we don't have this information right away in the -var-list-children
output?

> Actually, just did something similar internally - it was a bit tricky to get that in Eclipse DSF, and I had a special
> case, but still seems better than expanding the interface with GDB?

You are right, it's much better to work with existing interfaces.

> Another case, what do you do with std::shared_ptr<Foo> or boost::shared_ptr<Foo>? Both would be naturally shown
> similarly to Foo* - where you see the value immediately and can expand the object - and GDB would be able to do
> so with a suitable pretty-printer, but if a frontend is supposed to parse types, that won't work.

IIUC, if a pretty printer exists for a type, CDT always asks GDB for the value, regardless of the type. Since a shared_ptr's
value comes from a pretty printer, I think it would've worked.

>> About the naming:
>>
>> Right now, the term "real type" means the run-time type, in the sense of
>> looking at an object through a pointer to a virtual class. However, it's
>> not part of any API or significant user interface (only one I found is
>> in the output of "whatis"), so it's not set in stone. If I could
>> choose, I would give "real type" the meaning of the current patch, and
>> call the other one the "runtime type". Or is there already a term for it?
>> What do you think?
>>
>> Other ideas that went through my mind, if "real type" is not an option:
>>
>>   * canonical-type
>>   * resolved-type
>>   * underlying-type
>>
>> Another issue. It doesn't affect us, but I wasn't sure about this case:
>>
>>    struct blah {
>>      ...
>>    };
>>    typedef struct blah blah_t;
>>
>>    blah_t *b;
>>
>> What should real-type return for b? Right now it returns "blah_t *", but
>> should it return "struct blah *" instead?
> 
> If if were to return 'struct blah *', what would frontend do? Try to parse the type?
> Trying to parse 'int *' is already a tad risky - i.e. do you handle all possible syntax,
> like:
> 
> 	std::vector<My::Namespace::C> *
> 
> Quite some time ago we discussed the idea of exposing GDB type system to a frontend:
> 
> 	https://www.sourceware.org/ml/gdb/2006-04/msg00016.html
> 
> However, that will not be easy, and so far, frontends did not need entire type.

CDT already does its fair share of GDB type parsing, but I agree that it's an easy source of headache. Then less CDT needs to do
the better.

Simon
  

Patch

diff --git a/gdb/mi/mi-cmd-var.c b/gdb/mi/mi-cmd-var.c
index d9b37f8..730b92a 100644
--- a/gdb/mi/mi-cmd-var.c
+++ b/gdb/mi/mi-cmd-var.c
@@ -75,6 +75,15 @@  print_varobj (struct varobj *var, enum print_values print_values,
   if (type != NULL)
     {
       ui_out_field_string (uiout, "type", type);
+
+      xfree (type);
+    }
+
+  type = varobj_get_real_type (var);
+  if (type != NULL)
+    {
+      ui_out_field_string (uiout, "real-type", type);
+
       xfree (type);
     }
 
@@ -780,9 +789,13 @@  varobj_update_one (struct varobj *var, enum print_values print_values,
       if (r->type_changed)
 	{
 	  char *type_name = varobj_get_type (r->varobj);
+	  char *real_type_name = varobj_get_real_type (r->varobj);
 
 	  ui_out_field_string (uiout, "new_type", type_name);
+	  ui_out_field_string (uiout, "new_real_type", real_type_name);
+
 	  xfree (type_name);
+	  xfree (real_type_name);
 	}
 
       if (r->type_changed || r->children_changed)
diff --git a/gdb/varobj.c b/gdb/varobj.c
index 43ea96f..bc4e1c2 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -989,6 +989,20 @@  varobj_get_type (struct varobj *var)
   return type_to_string (var->type);
 }
 
+/* Same as varobj_get_type, but with typedefs resolved. */
+
+char *
+varobj_get_real_type (struct varobj *var)
+{
+  /* For the "fake" variables, do not return a type.  (Its type is
+     NULL, too.)
+     Do not return a type for invalid variables as well.  */
+  if (CPLUS_FAKE_CHILD (var) || !var->root->is_valid)
+    return NULL;
+
+  return type_to_string (check_typedef (var->type));
+}
+
 /* Obtain the type of an object variable.  */
 
 struct type *
diff --git a/gdb/varobj.h b/gdb/varobj.h
index 6fe7009..9210661 100644
--- a/gdb/varobj.h
+++ b/gdb/varobj.h
@@ -285,6 +285,8 @@  extern char *varobj_get_type (struct varobj *var);
 
 extern struct type *varobj_get_gdb_type (const struct varobj *var);
 
+extern char *varobj_get_real_type (struct varobj *var);
+
 extern char *varobj_get_path_expr (const struct varobj *var);
 
 extern const struct language_defn *