[RFA] varobj: call CHECK_TYPEDEF

[RFA] varobj: call CHECK_TYPEDEF

[Keith Seitz]

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

Hi,

Well, I am not really sure what this does, or why it is necessary
(check_typedef has no comment in gdbtypes.c), but it's what type_print
code does.

Occasionally, some (C++?) files can show no child objects because
nfields will be zero. After calling CHECK_TYPEDEF, nfields is suddenly
non-zero.

This was first reported as insight/219.

Keith

ChangeLog
2003-04-24  Keith Seitz  <keiths@redhat.com>

        * varobj.c (get_type): Call CHECK_TYPEDEF.
        (get_type_deref): Likewise.



[-- Attachment #2: varobj-check_typedef.patch --]
[-- Type: text/x-patch, Size: 651 bytes --]

Index: varobj.c
===================================================================
RCS file: /cvs/src/src/gdb/varobj.c,v
retrieving revision 1.38
diff -u -p -r1.38 varobj.c
--- varobj.c	4 Dec 2002 00:05:54 -0000	1.38
+++ varobj.c	24 Apr 2003 20:12:52 -0000
@@ -1394,6 +1394,7 @@ get_type (struct varobj *var)
   while (type != NULL && TYPE_CODE (type) == TYPE_CODE_TYPEDEF)
     type = TYPE_TARGET_TYPE (type);
 
+  CHECK_TYPEDEF (type);
   return type;
 }
 
@@ -1409,6 +1410,7 @@ get_type_deref (struct varobj *var)
 		       || TYPE_CODE (type) == TYPE_CODE_REF))
     type = get_target_type (type);
 
+  CHECK_TYPEDEF (type);
   return type;
 }
 

Re: [RFA] varobj: call CHECK_TYPEDEF

[Daniel Jacobowitz]

On Thu, Apr 24, 2003 at 01:16:38PM -0700, Keith Seitz wrote:
> Hi,
> 
> Well, I am not really sure what this does, or why it is necessary
> (check_typedef has no comment in gdbtypes.c), but it's what type_print
> code does.
> 
> Occasionally, some (C++?) files can show no child objects because
> nfields will be zero. After calling CHECK_TYPEDEF, nfields is suddenly
> non-zero.
> 
> This was first reported as insight/219.

CHECK_TYPEDEF does just about what the name suggests - it replaces a
TYPE_CODE_TYPEDEF type with the type (TYPE_CODE_STRUCT in this case)
that it points to.  Feel free to add a comment to that effect.

The question is whether the caller of get_type ever wants the typedef;
depending on how it's used these calls may belong in particular
callers, not in get_type.

> 
> Keith
> 
> ChangeLog
> 2003-04-24  Keith Seitz  <keiths@redhat.com>
> 
>         * varobj.c (get_type): Call CHECK_TYPEDEF.
>         (get_type_deref): Likewise.
> 
> 

> Index: varobj.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/varobj.c,v
> retrieving revision 1.38
> diff -u -p -r1.38 varobj.c
> --- varobj.c	4 Dec 2002 00:05:54 -0000	1.38
> +++ varobj.c	24 Apr 2003 20:12:52 -0000
> @@ -1394,6 +1394,7 @@ get_type (struct varobj *var)
>    while (type != NULL && TYPE_CODE (type) == TYPE_CODE_TYPEDEF)
>      type = TYPE_TARGET_TYPE (type);
>  
> +  CHECK_TYPEDEF (type);
>    return type;
>  }
>  
> @@ -1409,6 +1410,7 @@ get_type_deref (struct varobj *var)
>  		       || TYPE_CODE (type) == TYPE_CODE_REF))
>      type = get_target_type (type);
>  
> +  CHECK_TYPEDEF (type);
>    return type;
>  }
>  


-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA] varobj: call CHECK_TYPEDEF

[Keith Seitz]

On Thu, 2003-04-24 at 13:18, Daniel Jacobowitz wrote:
> CHECK_TYPEDEF does just about what the name suggests - it replaces a
> TYPE_CODE_TYPEDEF type with the type (TYPE_CODE_STRUCT in this case)
> that it points to.  Feel free to add a comment to that effect.

Ah! Duh! :-)

> The question is whether the caller of get_type ever wants the typedef;
> depending on how it's used these calls may belong in particular
> callers, not in get_type.

Hmm. Ok, well I would think that varobj would not want the typedef. For
example, if we have, "typedef struct foo Bar" and I declare a variable
of type Bar, varobj should report the number of children as the number
of children of the struct itself, not the typedef. get_type and
get_target_type already explicitly bypass TYPE_CODE_TYPEDEF.

Now the real ambiguity is: where to put this in varobj's case? Since
get_type (and consequently get_target_type and get_type_deref) are used
all over the place, so we would want to use CHECK_TYPEDEF there. But how
to not call it when not necessary? In the case I'm currently working on,
the TYPE_CODE (type) is TYPE_CODE_PTR and TYPE_CODE (TYPE_TARGET_TYPE
(type)) is TYPE_CODE_STRUCT. Perhaps there is something else I'm
missing? (Of course, this is still how whatis_command does it...)

??
Keith

Re: [RFA] varobj: call CHECK_TYPEDEF

[Daniel Jacobowitz]

On Thu, Apr 24, 2003 at 01:46:55PM -0700, Keith Seitz wrote:
> On Thu, 2003-04-24 at 13:18, Daniel Jacobowitz wrote:
> > CHECK_TYPEDEF does just about what the name suggests - it replaces a
> > TYPE_CODE_TYPEDEF type with the type (TYPE_CODE_STRUCT in this case)
> > that it points to.  Feel free to add a comment to that effect.
> 
> Ah! Duh! :-)
> 
> > The question is whether the caller of get_type ever wants the typedef;
> > depending on how it's used these calls may belong in particular
> > callers, not in get_type.
> 
> Hmm. Ok, well I would think that varobj would not want the typedef. For
> example, if we have, "typedef struct foo Bar" and I declare a variable
> of type Bar, varobj should report the number of children as the number
> of children of the struct itself, not the typedef. get_type and
> get_target_type already explicitly bypass TYPE_CODE_TYPEDEF.
> 
> Now the real ambiguity is: where to put this in varobj's case? Since
> get_type (and consequently get_target_type and get_type_deref) are used
> all over the place, so we would want to use CHECK_TYPEDEF there. But how
> to not call it when not necessary? In the case I'm currently working on,
> the TYPE_CODE (type) is TYPE_CODE_PTR and TYPE_CODE (TYPE_TARGET_TYPE
> (type)) is TYPE_CODE_STRUCT. Perhaps there is something else I'm
> missing? (Of course, this is still how whatis_command does it...)

The important question is whether get_type is ever used to display the
object's type; in which case we should say Bar, not "struct foo".

I haven't looked at where it's used, so I don't know.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA] varobj: call CHECK_TYPEDEF

[Keith Seitz]

On Thu, 2003-04-24 at 13:47, Daniel Jacobowitz wrote:
> The important question is whether get_type is ever used to display the
> object's type; in which case we should say Bar, not "struct foo".

Yes, I think this is already a buglet in varobj. Now that I understand
the whole CHECK_TYPEDEF thing, I'll file a bug report for it.

Keith

Re: [RFA] varobj: call CHECK_TYPEDEF

[Keith Seitz]

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

On Thu, 2003-04-24 at 13:35, Andrew Cagney wrote:

> Don't forget a gdb.mi/insight219 testcase.

Doh! How about the attached? (This is the minimal test that I could get
to reproduce it. It must use multiple files. That's why it never showed
up before, apparently.)

Keith

testsuite/gdb.mi/ChangeLog
2003-04-24  Keith Seitz  <keiths@redhat.com>

        * insight219.exp: New test.
        * insight219.cc: New file.
        * insight219.h: New file.
        * insight219-b.cc: New file.


[-- Attachment #2: insight219-test.patch --]
[-- Type: text/x-patch, Size: 2910 bytes --]

Index: insight219-b.cc
===================================================================
RCS file: insight219-b.cc
diff -N insight219-b.cc
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ insight219-b.cc	24 Apr 2003 21:54:00 -0000
@@ -0,0 +1,4 @@
+#include "insight219.h"
+
+B::~B() {}
+
Index: insight219.cc
===================================================================
RCS file: insight219.cc
diff -N insight219.cc
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ insight219.cc	24 Apr 2003 21:54:00 -0000
@@ -0,0 +1,8 @@
+#include "insight219.h"
+
+int
+main(int argc, char *argv[])
+{
+  B *b = new B();
+  return 0;
+};
Index: insight219.exp
===================================================================
RCS file: insight219.exp
diff -N insight219.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ insight219.exp	24 Apr 2003 21:54:00 -0000
@@ -0,0 +1,61 @@
+# Copyright 2003 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 2 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, write to the Free Software
+# Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.  
+
+# Please email any bugs, comments, and/or additions to this file to:
+# bug-gdb@prep.ai.mit.edu
+
+#
+# test insight/792
+#
+
+if { [skip_cplus_tests] } { continue }
+
+load_lib mi-support.exp
+set MIFLAGS "-i=mi"
+
+gdb_exit
+if [mi_gdb_start] {
+    continue
+}
+
+set testfile insight219
+set srcfiles [list insight219.cc insight219-b.cc]
+set binfile $objdir/$subdir/$testfile
+
+if [get_compiler_info ${binfile} "c++"] {
+    return -1;
+}
+
+set srcs ""
+foreach srcfile $srcfiles {
+    set srcs "$srcs $srcdir/$subdir/$srcfile"
+}
+
+if {[gdb_compile $srcs $binfile executable {debug c++}] != ""} {
+  gdb_suppress_entire_file "Testcase compile failed, so all test in this file will automatically fail."
+}
+
+# Test that children of classes are properly reported
+
+# Run to main
+mi_run_to_main
+
+mi_gdb_test "-var-create - * b" \
+  "(&\".*\"\r\n)*\\^done,name=\"var1\",numchild=\"1\",type=\"class B \\\*\"" \
+  "create var for class B"
+
+mi_gdb_exit
+return 0
Index: insight219.h
===================================================================
RCS file: insight219.h
diff -N insight219.h
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ insight219.h	24 Apr 2003 21:54:00 -0000
@@ -0,0 +1,8 @@
+class B
+{
+public:
+    virtual ~B();
+
+private:
+    int b;
+};

Re: [RFA] varobj: call CHECK_TYPEDEF

[Andrew Cagney]

> On Thu, 2003-04-24 at 13:35, Andrew Cagney wrote:
> 
> 
>> Don't forget a gdb.mi/insight219 testcase.
> 
> 
> Doh! How about the attached? (This is the minimal test that I could get
> to reproduce it. It must use multiple files. That's why it never showed
> up before, apparently.)

Yes, anything like that.  BTW, does ...

struct t
{
   int a;
   int b;
};

typedef struct t T;

main()
{
   static T v = {...};
}

tickle it?


> Index: insight219-b.cc
> ===================================================================
> RCS file: insight219-b.cc
> diff -N insight219-b.cc
> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ insight219-b.cc	24 Apr 2003 21:54:00 -0000
> @@ -0,0 +1,4 @@
> +#include "insight219.h"
> +
> +B::~B() {}
> +


> Index: insight219.cc
> ===================================================================
> RCS file: insight219.cc
> diff -N insight219.cc
> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ insight219.cc	24 Apr 2003 21:54:00 -0000
> @@ -0,0 +1,8 @@
> +#include "insight219.h"
> +
> +int
> +main(int argc, char *argv[])
> +{
> +  B *b = new B();
> +  return 0;
> +};
> Index: insight219.h
> ===================================================================
> RCS file: insight219.h
> diff -N insight219.h
> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ insight219.h	24 Apr 2003 21:54:00 -0000
> @@ -0,0 +1,8 @@
> +class B
> +{
> +public:
> +    virtual ~B();
> +
> +private:
> +    int b;
> +};

Re: [RFA] varobj: call CHECK_TYPEDEF

[Keith Seitz]

On Thu, 2003-04-24 at 15:18, Andrew Cagney wrote:
> BTW, does ...
> 
> struct t
> {
>    int a;
>    int b;
> };
> 
> typedef struct t T;
> 
> main()
> {
>    static T v = {...};
> }
> 
> tickle it?

Nope, that works properly (except for it being reported as "struct t"
instead of "T").

Keith

Re: [RFA] varobj: call CHECK_TYPEDEF

[Daniel Jacobowitz]

On Thu, Apr 24, 2003 at 03:29:36PM -0700, Keith Seitz wrote:
> On Thu, 2003-04-24 at 15:18, Andrew Cagney wrote:
> > BTW, does ...
> > 
> > struct t
> > {
> >    int a;
> >    int b;
> > };
> > 
> > typedef struct t T;
> > 
> > main()
> > {
> >    static T v = {...};
> > }
> > 
> > tickle it?
> 
> Nope, that works properly (except for it being reported as "struct t"
> instead of "T").

[Keith, you had "test for insight/792" in your posted testsuite patch.]

There's at least one compiler bug in this area, where DWARF-2 debug info
will say struct t instead of T.  I fixed it for 3.3 and (I think?)
3.2.3.  Just in case you start getting confused :)

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA] varobj: call CHECK_TYPEDEF

[Andrew Cagney]

> On Thu, Apr 24, 2003 at 03:29:36PM -0700, Keith Seitz wrote:
> 
>> On Thu, 2003-04-24 at 15:18, Andrew Cagney wrote:
> 
>> > BTW, does ...
>> > 
>> > struct t
>> > {
>> >    int a;
>> >    int b;
>> > };
>> > 
>> > typedef struct t T;
>> > 
>> > main()
>> > {
>> >    static T v = {...};
>> > }
>> > 
>> > tickle it?
> 
>> 
>> Nope, that works properly (except for it being reported as "struct t"
>> instead of "T").
> 
> 
> [Keith, you had "test for insight/792" in your posted testsuite patch.]
> 
> There's at least one compiler bug in this area, where DWARF-2 debug info
> will say struct t instead of T.  I fixed it for 3.3 and (I think?)
> 3.2.3.  Just in case you start getting confused :)

So, rather than `bug', `probable gcc bug'?

Andrew

Re: [RFA] varobj: call CHECK_TYPEDEF

[Daniel Jacobowitz]

On Thu, Apr 24, 2003 at 10:15:06PM -0400, Andrew Cagney wrote:
> >On Thu, Apr 24, 2003 at 03:29:36PM -0700, Keith Seitz wrote:
> >
> >>On Thu, 2003-04-24 at 15:18, Andrew Cagney wrote:
> >
> >>> BTW, does ...
> >>> 
> >>> struct t
> >>> {
> >>>    int a;
> >>>    int b;
> >>> };
> >>> 
> >>> typedef struct t T;
> >>> 
> >>> main()
> >>> {
> >>>    static T v = {...};
> >>> }
> >>> 
> >>> tickle it?
> >
> >>
> >>Nope, that works properly (except for it being reported as "struct t"
> >>instead of "T").
> >
> >
> >[Keith, you had "test for insight/792" in your posted testsuite patch.]
> >
> >There's at least one compiler bug in this area, where DWARF-2 debug info
> >will say struct t instead of T.  I fixed it for 3.3 and (I think?)
> >3.2.3.  Just in case you start getting confused :)
> 
> So, rather than `bug', `probable gcc bug'?

Depends, I imagine GDB has bugs here too.  Someone should try Keith's
testcase with a fixed GCC I suppose.

(If your compiler has the bug it will show up in the MI testsuite
already; don't remember exactly where.)

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA] varobj: call CHECK_TYPEDEF

[Keith Seitz]

On Thu, 2003-04-24 at 15:18, Andrew Cagney wrote:

So where do we stand with this? Shall I check it in and deal with the
fallout? (I need to go through and double-check all the typedef stuff in
varobj anyway, but at least this will allow users to use varobj, even if
it isn't cosmetically perfect.)

Keith

> > Index: insight219-b.cc
> > ===================================================================
> > RCS file: insight219-b.cc
> > diff -N insight219-b.cc
> > --- /dev/null	1 Jan 1970 00:00:00 -0000
> > +++ insight219-b.cc	24 Apr 2003 21:54:00 -0000
> > @@ -0,0 +1,4 @@
> > +#include "insight219.h"
> > +
> > +B::~B() {}
> > +
> 
> 
> > Index: insight219.cc
> > ===================================================================
> > RCS file: insight219.cc
> > diff -N insight219.cc
> > --- /dev/null	1 Jan 1970 00:00:00 -0000
> > +++ insight219.cc	24 Apr 2003 21:54:00 -0000
> > @@ -0,0 +1,8 @@
> > +#include "insight219.h"
> > +
> > +int
> > +main(int argc, char *argv[])
> > +{
> > +  B *b = new B();
> > +  return 0;
> > +};
> > Index: insight219.h
> > ===================================================================
> > RCS file: insight219.h
> > diff -N insight219.h
> > --- /dev/null	1 Jan 1970 00:00:00 -0000
> > +++ insight219.h	24 Apr 2003 21:54:00 -0000
> > @@ -0,0 +1,8 @@
> > +class B
> > +{
> > +public:
> > +    virtual ~B();
> > +
> > +private:
> > +    int b;
> > +};
> 
> 

Re: [RFA] varobj: call CHECK_TYPEDEF

[Andrew Cagney]

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

Er, this is C++, Daniel, David or MichaelC?

Andrew

[-- Attachment #2: mailbox-message://ac131313@movemail/fsf/gdb/patches#15270852 --]
[-- Type: message/rfc822, Size: 3081 bytes --]

From: Keith Seitz <keiths@redhat.com>
To: Andrew Cagney <ac131313@redhat.com>
Cc: "gdb-patches@sources.redhat.com" <gdb-patches@sources.redhat.com>
Subject: Re: [RFA] varobj: call CHECK_TYPEDEF
Date: 11 Jun 2003 13:15:10 -0700
Message-ID: <1055362509.1571.63.camel@lindt.uglyboxes.com>

On Thu, 2003-04-24 at 15:18, Andrew Cagney wrote:

So where do we stand with this? Shall I check it in and deal with the
fallout? (I need to go through and double-check all the typedef stuff in
varobj anyway, but at least this will allow users to use varobj, even if
it isn't cosmetically perfect.)

Keith

> > Index: insight219-b.cc
> > ===================================================================
> > RCS file: insight219-b.cc
> > diff -N insight219-b.cc
> > --- /dev/null	1 Jan 1970 00:00:00 -0000
> > +++ insight219-b.cc	24 Apr 2003 21:54:00 -0000
> > @@ -0,0 +1,4 @@
> > +#include "insight219.h"
> > +
> > +B::~B() {}
> > +
> 
> 
> > Index: insight219.cc
> > ===================================================================
> > RCS file: insight219.cc
> > diff -N insight219.cc
> > --- /dev/null	1 Jan 1970 00:00:00 -0000
> > +++ insight219.cc	24 Apr 2003 21:54:00 -0000
> > @@ -0,0 +1,8 @@
> > +#include "insight219.h"
> > +
> > +int
> > +main(int argc, char *argv[])
> > +{
> > +  B *b = new B();
> > +  return 0;
> > +};
> > Index: insight219.h
> > ===================================================================
> > RCS file: insight219.h
> > diff -N insight219.h
> > --- /dev/null	1 Jan 1970 00:00:00 -0000
> > +++ insight219.h	24 Apr 2003 21:54:00 -0000
> > @@ -0,0 +1,8 @@
> > +class B
> > +{
> > +public:
> > +    virtual ~B();
> > +
> > +private:
> > +    int b;
> > +};
> 
> 

Re: [RFA] varobj: call CHECK_TYPEDEF

[David Carlton]

On 11 Jun 2003 13:15:10 -0700, Keith Seitz <keiths@redhat.com> said:

> So where do we stand with this?

I've just gone and looked over the thread and at Keith's patch; I
think the idea is sound, but the implementation isn't.  The comments
at the top of get_type say that it's supposed to skip past typedefs,
so calling CHECK_TYPEDEF certainly seems legitimate.  But
CHECK_TYPEDEF calls check_typedef, which already goes through chains
of typedefs, so you can get rid of the loop in get_type.  So I would
rewrite get_type as follows:

static struct type *
get_type (struct varobj *var)
{
  struct type *type;
  type = var->type;

  if (type != NULL)
    CHECK_TYPEDEF (type);

  return type;
}

And I don't think you should change get_type_deref at all, but you
should change get_target_type as follows:

static struct type *
get_target_type (struct type *type)
{
  if (type != NULL)
    {
      type = TYPE_TARGET_TYPE (type);

      if (type != NULL)
	CHECK_TYPEDEF (type);
    }

  return type;
}

(I do wonder a bit if all of those "type != NULL" checks are
necessary, but that's a separate question.)

I'm not qualified to approve these patches, but the idea seems obvious
to me.

David Carlton
carlton@math.stanford.edu

Re: [RFA] varobj: call CHECK_TYPEDEF

[Keith Seitz]

On Wed, 2003-06-11 at 16:49, David Carlton wrote: 
> I've just gone and looked over the thread and at Keith's patch; I
> think the idea is sound, but the implementation isn't.  The comments
> at the top of get_type say that it's supposed to skip past typedefs,
> so calling CHECK_TYPEDEF certainly seems legitimate.  But
> CHECK_TYPEDEF calls check_typedef, which already goes through chains
> of typedefs, so you can get rid of the loop in get_type.

Yup, I think you are correct. I'm sure that I was just being laz^Whasty.
:-)

I'll note that there is still one failure in the testsuite.
gdb.mi/mi-var-display.exp: create local variable weird (aka insight's
c_variable 6.22) fails because the output type is now considered "struct
_struct_decl" instead of it's typedef name "weird".

I believe it is a bug below varobj, though. In varobj_create,
gdb_evaluate_expression is called. It returns the struct value for the
expression. It returns a type that looks like:

var->value->type->main_type->code = TYPE_CODE_PTR
var->value->type->main_type->target_type->main_type->code =
TYPE_CODE_STRUCT, tag_name="_struct_decl"

I think that this is wrong, and it should be "TYPE_CODE_TYPEDEF" and
"weird_struct"...

Or am I yet again being laz^Whasty? :-)
Keith

Re: [RFA] varobj: call CHECK_TYPEDEF

[Daniel Jacobowitz]

On Wed, Jun 11, 2003 at 05:36:02PM -0700, Keith Seitz wrote:
> On Wed, 2003-06-11 at 16:49, David Carlton wrote: 
> > I've just gone and looked over the thread and at Keith's patch; I
> > think the idea is sound, but the implementation isn't.  The comments
> > at the top of get_type say that it's supposed to skip past typedefs,
> > so calling CHECK_TYPEDEF certainly seems legitimate.  But
> > CHECK_TYPEDEF calls check_typedef, which already goes through chains
> > of typedefs, so you can get rid of the loop in get_type.
> 
> Yup, I think you are correct. I'm sure that I was just being laz^Whasty.
> :-)

David's analysis sounds right to me.  I'll look over the actual code
tomorrow, really I will...

> I'll note that there is still one failure in the testsuite.
> gdb.mi/mi-var-display.exp: create local variable weird (aka insight's
> c_variable 6.22) fails because the output type is now considered "struct
> _struct_decl" instead of it's typedef name "weird".
> 
> I believe it is a bug below varobj, though. In varobj_create,
> gdb_evaluate_expression is called. It returns the struct value for the
> expression. It returns a type that looks like:
> 
> var->value->type->main_type->code = TYPE_CODE_PTR
> var->value->type->main_type->target_type->main_type->code =
> TYPE_CODE_STRUCT, tag_name="_struct_decl"
> 
> I think that this is wrong, and it should be "TYPE_CODE_TYPEDEF" and
> "weird_struct"...
> 
> Or am I yet again being laz^Whasty? :-)
> Keith

I am 99.99% certain that this test is a compiler problem, not a GDB
problem, and that I fixed it in GCC a few months ago.  My memory's
going though :P

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA] varobj: call CHECK_TYPEDEF

[Daniel Jacobowitz]

On Wed, Jun 11, 2003 at 09:28:10PM -0400, Daniel Jacobowitz wrote:
> On Wed, Jun 11, 2003 at 05:36:02PM -0700, Keith Seitz wrote:
> > On Wed, 2003-06-11 at 16:49, David Carlton wrote: 
> > > I've just gone and looked over the thread and at Keith's patch; I
> > > think the idea is sound, but the implementation isn't.  The comments
> > > at the top of get_type say that it's supposed to skip past typedefs,
> > > so calling CHECK_TYPEDEF certainly seems legitimate.  But
> > > CHECK_TYPEDEF calls check_typedef, which already goes through chains
> > > of typedefs, so you can get rid of the loop in get_type.
> > 
> > Yup, I think you are correct. I'm sure that I was just being laz^Whasty.
> > :-)
> 
> David's analysis sounds right to me.  I'll look over the actual code
> tomorrow, really I will...

I don't think David's conversions are quite right; close though. 
However, something here is fishy.

Compare:
/* This returns the type of the variable. This skips past typedefs
   and returns the real type of the variable. It also dereferences
   pointers and references.
vs.
static struct type *
get_type (struct varobj *var)
{ 
  struct type *type;
  type = var->type;

  while (type != NULL && TYPE_CODE (type) == TYPE_CODE_TYPEDEF)
    type = TYPE_TARGET_TYPE (type);

  return type;
}

That doesn't dereference pointers and references!  It looks like
get_type got inserted between get_type_deref and its comments?  Can you
confirm that it shouldn't dereference?

Assuming get_type is not supposed to dereference pointers, then this
would work:

/* This returns the type of the variable. It also skips past typedefs
   to return the real type of the variable.

   NOTE: TYPE_TARGET_TYPE should NOT be used anywhere in this file
   except within get_target_type and get_type. */
static struct type *
get_type (struct varobj *var)
{
  struct type *type;  
  type = var->type;

  if (type != NULL)
    type = check_typedef (type);

  return type;
}

/* This returns the target type (or NULL) of TYPE, also skipping
   past typedefs, just like get_type ().
  
   NOTE: TYPE_TARGET_TYPE should NOT be used anywhere in this file
   except within get_target_type and get_type. */
static struct type *
get_target_type (struct type *type)
{
  if (type != NULL) 
    {
      type = TYPE_TARGET_TYPE (type);
      type = check_typedef (type);
    }

  return type;
}

Test results for that look OK but I don't have insight built at the
moment, so I didn't test it.


-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA] varobj: call CHECK_TYPEDEF

[Daniel Jacobowitz]

On Thu, Jun 19, 2003 at 03:28:45PM -0400, Daniel Jacobowitz wrote:
> On Wed, Jun 11, 2003 at 09:28:10PM -0400, Daniel Jacobowitz wrote:
> > On Wed, Jun 11, 2003 at 05:36:02PM -0700, Keith Seitz wrote:
> > > On Wed, 2003-06-11 at 16:49, David Carlton wrote: 
> > > > I've just gone and looked over the thread and at Keith's patch; I
> > > > think the idea is sound, but the implementation isn't.  The comments
> > > > at the top of get_type say that it's supposed to skip past typedefs,
> > > > so calling CHECK_TYPEDEF certainly seems legitimate.  But
> > > > CHECK_TYPEDEF calls check_typedef, which already goes through chains
> > > > of typedefs, so you can get rid of the loop in get_type.
> > > 
> > > Yup, I think you are correct. I'm sure that I was just being laz^Whasty.
> > > :-)
> > 
> > David's analysis sounds right to me.  I'll look over the actual code
> > tomorrow, really I will...
> 
> I don't think David's conversions are quite right; close though. 
> However, something here is fishy.
> 
> Compare:
> /* This returns the type of the variable. This skips past typedefs
>    and returns the real type of the variable. It also dereferences
>    pointers and references.
> vs.
> static struct type *
> get_type (struct varobj *var)
> { 
>   struct type *type;
>   type = var->type;
> 
>   while (type != NULL && TYPE_CODE (type) == TYPE_CODE_TYPEDEF)
>     type = TYPE_TARGET_TYPE (type);
> 
>   return type;
> }
> 
> That doesn't dereference pointers and references!  It looks like
> get_type got inserted between get_type_deref and its comments?  Can you
> confirm that it shouldn't dereference?
> 
> Assuming get_type is not supposed to dereference pointers, then this
> would work:
> 
> /* This returns the type of the variable. It also skips past typedefs
>    to return the real type of the variable.
> 
>    NOTE: TYPE_TARGET_TYPE should NOT be used anywhere in this file
>    except within get_target_type and get_type. */
> static struct type *
> get_type (struct varobj *var)
> {
>   struct type *type;  
>   type = var->type;
> 
>   if (type != NULL)
>     type = check_typedef (type);
> 
>   return type;
> }
> 
> /* This returns the target type (or NULL) of TYPE, also skipping
>    past typedefs, just like get_type ().
>   
>    NOTE: TYPE_TARGET_TYPE should NOT be used anywhere in this file
>    except within get_target_type and get_type. */
> static struct type *
> get_target_type (struct type *type)
> {
>   if (type != NULL) 
>     {
>       type = TYPE_TARGET_TYPE (type);
>       type = check_typedef (type);
>     }
> 
>   return type;
> }
> 
> Test results for that look OK but I don't have insight built at the
> moment, so I didn't test it.

I take that back.  Test results are abominable; everything crashes
because I misunderstood the use of get_target_type.  Try this patch.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

2003-06-19  Daniel Jacobowitz  <drow@mvista.com>

	* varobj.c (get_type, get_target_type): Use check_typedef.

Index: varobj.c
===================================================================
RCS file: /cvs/src/src/gdb/varobj.c,v
retrieving revision 1.38
diff -u -p -r1.38 varobj.c
--- varobj.c	4 Dec 2002 00:05:54 -0000	1.38
+++ varobj.c	19 Jun 2003 19:42:17 -0000
@@ -1379,9 +1379,8 @@ make_cleanup_free_variable (struct varob
   return make_cleanup (do_free_variable_cleanup, var);
 }
 
-/* This returns the type of the variable. This skips past typedefs
-   and returns the real type of the variable. It also dereferences
-   pointers and references.
+/* This returns the type of the variable. It also skips past typedefs
+   to return the real type of the variable.
 
    NOTE: TYPE_TARGET_TYPE should NOT be used anywhere in this file
    except within get_target_type and get_type. */
@@ -1391,8 +1390,8 @@ get_type (struct varobj *var)
   struct type *type;
   type = var->type;
 
-  while (type != NULL && TYPE_CODE (type) == TYPE_CODE_TYPEDEF)
-    type = TYPE_TARGET_TYPE (type);
+  if (type != NULL)
+    type = check_typedef (type);
 
   return type;
 }
@@ -1423,8 +1422,8 @@ get_target_type (struct type *type)
   if (type != NULL)
     {
       type = TYPE_TARGET_TYPE (type);
-      while (type != NULL && TYPE_CODE (type) == TYPE_CODE_TYPEDEF)
-	type = TYPE_TARGET_TYPE (type);
+      if (type != NULL)
+	type = check_typedef (type);
     }
 
   return type;

Re: [RFA] varobj: call CHECK_TYPEDEF

[Keith Seitz]

On Thu, 2003-06-19 at 12:45, Daniel Jacobowitz wrote:
> > That doesn't dereference pointers and references!  It looks like
> > get_type got inserted between get_type_deref and its comments?  Can you
> > confirm that it shouldn't dereference?

Yes, I believe the comment is wrong. There is another function to do
dereferences...

> I take that back.  Test results are abominable; everything crashes
> because I misunderstood the use of get_target_type.  Try this patch.

Your patch passes all the insight varobj tests (c_variable.exp,
cpp_variable.exp, insight219.exp). We still have the one (existing)
failure, as expected (aka: gcc bug, as I recall).

Anyway, I would like to encourage you to check this patch in (and you
could close insight/219, too :-)

Keith

Re: [RFA] varobj: call CHECK_TYPEDEF

[Daniel Jacobowitz]

On Thu, Jun 19, 2003 at 01:38:16PM -0700, Keith Seitz wrote:
> On Thu, 2003-06-19 at 12:45, Daniel Jacobowitz wrote:
> > > That doesn't dereference pointers and references!  It looks like
> > > get_type got inserted between get_type_deref and its comments?  Can you
> > > confirm that it shouldn't dereference?
> 
> Yes, I believe the comment is wrong. There is another function to do
> dereferences...
> 
> > I take that back.  Test results are abominable; everything crashes
> > because I misunderstood the use of get_target_type.  Try this patch.
> 
> Your patch passes all the insight varobj tests (c_variable.exp,
> cpp_variable.exp, insight219.exp). We still have the one (existing)
> failure, as expected (aka: gcc bug, as I recall).
> 
> Anyway, I would like to encourage you to check this patch in (and you
> could close insight/219, too :-)

I've checked it in.  I can't edit the insight PR database, though.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA] varobj: call CHECK_TYPEDEF

[Keith Seitz]

On Thu, 2003-06-19 at 13:41, Daniel Jacobowitz wrote:

> I've checked it in.

Thanks!

> I can't edit the insight PR database, though.

I guess I mistakenly thought that all those with access to gdb would
have access to insight. I guess not. I'll close it.

Thank you (and David) again for looking into this.

Keith