[RFA/dwarf-2] Fix for the null record problem

[RFA/dwarf-2] Fix for the null record problem

[Joel Brobecker]

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

Hello,

This is a followup on the thread that started with:

   http://sources.redhat.com/ml/gdb-patches/2004-02/msg00058.html

The test proposed under that thread was dropped because an empty
struct is not legal C. However, it is legal in Ada, and I've seen
a message saying that it is also legal in C++.

I have found the source of the problem, and suggest the attached patch.
The problem was that GDB was mistakenly deducing that the empry record
was only a stub because of the lack of fields, and was therefore tagging
it with TYPE_FLAG_STUB. This is not correct. Instead, the right
approach, I believe, is to check for the DW_AT_declaration attribute.

2004-02-19  J. Brobecker  <brobecker@gnat.com>

        * dwarf2read.c (read_structure_scope): Identify stub types
        using the DW_AT_declaration attribute.

tested on x86-linux. No regression. Fixes the testcase that was
proposed by Andrew (even though it is not legal C) and the Ada
case.

OK to apply?

Thanks,
-- 
Joel

PS: BTW, my past 3 months of traveling will soon be over. The past
couple of weeks have been very hectic. I have noticed that there were
some messages directed at me to which I haven't answered yet. I am
really sorry. I have kept these messages and will try to answer them
soon.

[-- Attachment #2: null_record.diff --]
[-- Type: text/plain, Size: 963 bytes --]

Index: dwarf2read.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2read.c,v
retrieving revision 1.130
diff -u -p -r1.130 dwarf2read.c
--- dwarf2read.c	28 Jan 2004 18:43:06 -0000	1.130
+++ dwarf2read.c	19 Feb 2004 13:58:42 -0000
@@ -3077,6 +3077,9 @@ read_structure_scope (struct die_info *d
       TYPE_LENGTH (type) = 0;
     }
 
+  if (dwarf2_attr (die, DW_AT_declaration, cu) != NULL)
+    TYPE_FLAGS (type) |= TYPE_FLAG_STUB;
+
   /* We need to add the type field to the die immediately so we don't
      infinitely recurse when dealing with pointers to the structure
      type within the structure itself. */
@@ -3213,11 +3216,6 @@ read_structure_scope (struct die_info *d
       new_symbol (die, type, cu);
 
       do_cleanups (back_to);
-    }
-  else
-    {
-      /* No children, must be stub. */
-      TYPE_FLAGS (type) |= TYPE_FLAG_STUB;
     }
 
   processing_current_prefix = previous_prefix;

Re: [RFA/dwarf-2] Fix for the null record problem

[Elena Zannoni]

Joel Brobecker writes:
 > Hello,
 > 
 > This is a followup on the thread that started with:
 > 
 >    http://sources.redhat.com/ml/gdb-patches/2004-02/msg00058.html
 > 
 > The test proposed under that thread was dropped because an empty
 > struct is not legal C. However, it is legal in Ada, and I've seen
 > a message saying that it is also legal in C++.
 > 

Let's try the Vulcan mind meld: "We need a gdb.ada directory. We need
a gdb.ada directory. We need a gdb.ada directory. We need a gdb.ada
directory." :-) 

Seriously, I'd like to see a testcase that FAIL->PASS with this patch.
Can somebody get a C++ testcase, at least?

the patch looks sensible, but I would like to see the testcase go in
at the same time, or we'll forget.

elena


 > I have found the source of the problem, and suggest the attached patch.
 > The problem was that GDB was mistakenly deducing that the empry record
 > was only a stub because of the lack of fields, and was therefore tagging
 > it with TYPE_FLAG_STUB. This is not correct. Instead, the right
 > approach, I believe, is to check for the DW_AT_declaration attribute.
 > 
 > 2004-02-19  J. Brobecker  <brobecker@gnat.com>
 > 
 >         * dwarf2read.c (read_structure_scope): Identify stub types
 >         using the DW_AT_declaration attribute.
 > 
 > tested on x86-linux. No regression. Fixes the testcase that was
 > proposed by Andrew (even though it is not legal C) and the Ada
 > case.
 > 
 > OK to apply?
 > 
 > Thanks,
 > -- 
 > Joel
 > 
 > PS: BTW, my past 3 months of traveling will soon be over. The past
 > couple of weeks have been very hectic. I have noticed that there were
 > some messages directed at me to which I haven't answered yet. I am
 > really sorry. I have kept these messages and will try to answer them
 > soon.
 > Index: dwarf2read.c
 > ===================================================================
 > RCS file: /cvs/src/src/gdb/dwarf2read.c,v
 > retrieving revision 1.130
 > diff -u -p -r1.130 dwarf2read.c
 > --- dwarf2read.c	28 Jan 2004 18:43:06 -0000	1.130
 > +++ dwarf2read.c	19 Feb 2004 13:58:42 -0000
 > @@ -3077,6 +3077,9 @@ read_structure_scope (struct die_info *d
 >        TYPE_LENGTH (type) = 0;
 >      }
 >  
 > +  if (dwarf2_attr (die, DW_AT_declaration, cu) != NULL)
 > +    TYPE_FLAGS (type) |= TYPE_FLAG_STUB;
 > +
 >    /* We need to add the type field to the die immediately so we don't
 >       infinitely recurse when dealing with pointers to the structure
 >       type within the structure itself. */
 > @@ -3213,11 +3216,6 @@ read_structure_scope (struct die_info *d
 >        new_symbol (die, type, cu);
 >  
 >        do_cleanups (back_to);
 > -    }
 > -  else
 > -    {
 > -      /* No children, must be stub. */
 > -      TYPE_FLAGS (type) |= TYPE_FLAG_STUB;
 >      }
 >  
 >    processing_current_prefix = previous_prefix;

Re: [RFA/dwarf-2] Fix for the null record problem

[Andrew Cagney]

> Joel Brobecker writes:
>  > Hello,
>  > 
>  > This is a followup on the thread that started with:
>  > 
>  >    http://sources.redhat.com/ml/gdb-patches/2004-02/msg00058.html
>  > 
>  > The test proposed under that thread was dropped because an empty
>  > struct is not legal C. However, it is legal in Ada, and I've seen
>  > a message saying that it is also legal in C++.
>  > 
> 
> Let's try the Vulcan mind meld: "We need a gdb.ada directory. We need
> a gdb.ada directory. We need a gdb.ada directory. We need a gdb.ada
> directory." :-) 
> 
> Seriously, I'd like to see a testcase that FAIL->PASS with this patch.
> Can somebody get a C++ testcase, at least?
> 
> the patch looks sensible, but I would like to see the testcase go in
> at the same time, or we'll forget.

A separate struct0 test?  If the compile fails, skip it.  It doesn't 
need to be as evil as structs.exp.

Andrew

Re: [RFA/dwarf-2] Fix for the null record problem

[Joel Brobecker]

Hello Elena,

> Let's try the Vulcan mind meld: "We need a gdb.ada directory. We need
> a gdb.ada directory. We need a gdb.ada directory. We need a gdb.ada
> directory." :-) 

We need a gdb.ada directory, we ...

Yes I agree, and this is high on my GDB todo list. I just haven't
got around to doing it yet. But I'll do it hopefully sometimes soon.
(still traveling for a few more days, which explains my silence).

> Seriously, I'd like to see a testcase that FAIL->PASS with this patch.
> Can somebody get a C++ testcase, at least?

I don't mind at all writing an Ada test for this, but I think it would
be more profitable for the GDB community if that test was written using
C++. That's because it is more common to have a C++ compiler installed
compared to an Ada one. Probably it wouldn't hurt to have both?

> the patch looks sensible, but I would like to see the testcase go in
> at the same time, or we'll forget.

OK, I'll work on the C++ test, shouldn't be too hard to create one.
But it'll have to wait for a few days until I get home.

>  > 2004-02-19  J. Brobecker  <brobecker@gnat.com>
>  > 
>  >         * dwarf2read.c (read_structure_scope): Identify stub types
>  >         using the DW_AT_declaration attribute.

Thanks,
-- 
Joel

Re: [RFA/dwarf-2] Fix for the null record problem

[Joel Brobecker]

>  > This is a followup on the thread that started with:
>  > 
>  >    http://sources.redhat.com/ml/gdb-patches/2004-02/msg00058.html
>  > 
>  > The test proposed under that thread was dropped because an empty
>  > struct is not legal C. However, it is legal in Ada, and I've seen
>  > a message saying that it is also legal in C++.
> 
> Seriously, I'd like to see a testcase that FAIL->PASS with this patch.
> Can somebody get a C++ testcase, at least?
> 
> the patch looks sensible, but I would like to see the testcase go in
> at the same time, or we'll forget.

I tried to update one of the C++ testcase to include an empty struct,
but my C++ is completely rusty. 

In class2.cc, I tried adding

        struct empty_struct {};

Is that a struct type definition or a class definition. In any case,
GDB has no trouble at all printing the description of this type:

        (gdb) ptype empty_struct 
        type = class empty_struct {
        }

Does anybody know of a way to produce the "<incomplete type>" bug
with C++? Otherwise, there is my upcoming null_record.exp test,
but I need a bit more time before the first Ada testcase can be
committed.

>  > I have found the source of the problem, and suggest the attached patch.
>  > The problem was that GDB was mistakenly deducing that the empry record
>  > was only a stub because of the lack of fields, and was therefore tagging
>  > it with TYPE_FLAG_STUB. This is not correct. Instead, the right
>  > approach, I believe, is to check for the DW_AT_declaration attribute.
>  > 
>  > 2004-02-19  J. Brobecker  <brobecker@gnat.com>
>  > 
>  >         * dwarf2read.c (read_structure_scope): Identify stub types
>  >         using the DW_AT_declaration attribute.
>  > 
>  > tested on x86-linux. No regression. Fixes the testcase that was
>  > proposed by Andrew (even though it is not legal C) and the Ada
>  > case.
>  > 
>  > OK to apply?
>  > 
>  > Thanks,
>  > -- 
>  > Joel
>  > 
>  > PS: BTW, my past 3 months of traveling will soon be over. The past
>  > couple of weeks have been very hectic. I have noticed that there were
>  > some messages directed at me to which I haven't answered yet. I am
>  > really sorry. I have kept these messages and will try to answer them
>  > soon.
>  > Index: dwarf2read.c
>  > ===================================================================
>  > RCS file: /cvs/src/src/gdb/dwarf2read.c,v
>  > retrieving revision 1.130
>  > diff -u -p -r1.130 dwarf2read.c
>  > --- dwarf2read.c	28 Jan 2004 18:43:06 -0000	1.130
>  > +++ dwarf2read.c	19 Feb 2004 13:58:42 -0000
>  > @@ -3077,6 +3077,9 @@ read_structure_scope (struct die_info *d
>  >        TYPE_LENGTH (type) = 0;
>  >      }
>  >  
>  > +  if (dwarf2_attr (die, DW_AT_declaration, cu) != NULL)
>  > +    TYPE_FLAGS (type) |= TYPE_FLAG_STUB;
>  > +
>  >    /* We need to add the type field to the die immediately so we don't
>  >       infinitely recurse when dealing with pointers to the structure
>  >       type within the structure itself. */
>  > @@ -3213,11 +3216,6 @@ read_structure_scope (struct die_info *d
>  >        new_symbol (die, type, cu);
>  >  
>  >        do_cleanups (back_to);
>  > -    }
>  > -  else
>  > -    {
>  > -      /* No children, must be stub. */
>  > -      TYPE_FLAGS (type) |= TYPE_FLAG_STUB;
>  >      }
>  >  
>  >    processing_current_prefix = previous_prefix;

-- 
Joel

Re: [RFA/dwarf-2] Fix for the null record problem

[Daniel Jacobowitz]

On Wed, Feb 25, 2004 at 06:31:08PM -0800, Joel Brobecker wrote:
> >  > This is a followup on the thread that started with:
> >  > 
> >  >    http://sources.redhat.com/ml/gdb-patches/2004-02/msg00058.html
> >  > 
> >  > The test proposed under that thread was dropped because an empty
> >  > struct is not legal C. However, it is legal in Ada, and I've seen
> >  > a message saying that it is also legal in C++.
> > 
> > Seriously, I'd like to see a testcase that FAIL->PASS with this patch.
> > Can somebody get a C++ testcase, at least?
> > 
> > the patch looks sensible, but I would like to see the testcase go in
> > at the same time, or we'll forget.
> 
> I tried to update one of the C++ testcase to include an empty struct,
> but my C++ is completely rusty. 
> 
> In class2.cc, I tried adding
> 
>         struct empty_struct {};
> 
> Is that a struct type definition or a class definition. In any case,
> GDB has no trouble at all printing the description of this type:
> 
>         (gdb) ptype empty_struct 
>         type = class empty_struct {
>         }
> 
> Does anybody know of a way to produce the "<incomplete type>" bug
> with C++? Otherwise, there is my upcoming null_record.exp test,
> but I need a bit more time before the first Ada testcase can be
> committed.

It occurs to me that C++ probably will not show the problem.  The empty
struct is legal, but has size 1; the language requires:
 struct empty_struct { } array[10];
 assert (&array[0] != &array[1]);

This is one of the many quirks of the GNU C extension in question.

I'd like to have the empty struct test anyway.  Remember to add a
variable of that type if you add a type to class2.cc; or newer GCCs
will just elide the type.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA/dwarf-2] Fix for the null record problem

[Joel Brobecker]

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

> I'd like to have the empty struct test anyway.  Remember to add a
> variable of that type if you add a type to class2.cc; or newer GCCs
> will just elide the type.

Would the following patch be ok to apply?

2004-02-26  J. Brobecker  <brobecker@gnat.com>

        * gdb.cp/class2.cc (empty): New class.
        (refer): New function.
        (main): Declare an object of type empty and use it.
        * gdb.cp/class2.exp: Print the value of an object of type empty.

Tested on x86-linux.

-- 
Joel

[-- Attachment #2: empty.diff --]
[-- Type: text/plain, Size: 1294 bytes --]

Index: class2.cc
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.cp/class2.cc,v
retrieving revision 1.2
diff -u -r1.2 class2.cc
--- class2.cc	11 Feb 2004 14:01:25 -0000	1.2
+++ class2.cc	26 Feb 2004 18:51:32 -0000
@@ -48,10 +48,19 @@
   ;
 }
 
+struct empty {};
+
+// Stop the compiler from optimizing away data.
+void refer (empty *)
+{
+  ;
+}
+
 int main (void)
 {
   A alpha, *aap, *abp;
   B beta, *bbp;
+  empty e;
 
   alpha.a1 = 100;
   beta.a1 = 200; beta.b1 = 201; beta.b2 = 202;
@@ -59,6 +68,7 @@
   aap = &alpha; refer (aap);
   abp = &beta;  refer (abp);
   bbp = &beta;  refer (bbp);
+  refer (&e);
 
   return 0;  // marker return 0
 } // marker close brace
Index: class2.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.cp/class2.exp,v
retrieving revision 1.1
diff -u -r1.1 class2.exp
--- class2.exp	25 Nov 2003 15:01:31 -0000	1.1
+++ class2.exp	26 Feb 2004 18:51:32 -0000
@@ -113,3 +113,7 @@
 gdb_test "print * (B *) abp" \
     "= {.*a1 = 200.*b1 = 201.*b2 = 202}" \
     "print * (B *) abp at marker return 0"
+
+# Printing the value of an object containing no data fields:
+
+gdb_test "p e" "= {<No data fields>}" "print object with no data fields"

Re: [RFA/dwarf-2] Fix for the null record problem

[Joel Brobecker]

Hello Elena,

> Let's try the Vulcan mind meld: "We need a gdb.ada directory. We need
> a gdb.ada directory. We need a gdb.ada directory. We need a gdb.ada
> directory." :-) 

OK, now done| :-)

> Seriously, I'd like to see a testcase that FAIL->PASS with this patch.
> Can somebody get a C++ testcase, at least?

With this patch, we see a test in gdb.ada/null_record.exp FAIL->PASS.

> the patch looks sensible, but I would like to see the testcase go in
> at the same time, or we'll forget.

I've checked that the patch below is still applicable. Would you mind
reviewing it and let me know if it's ok to check it in?

Thank you,
-- 
Joel

>  > I have found the source of the problem, and suggest the attached patch.
>  > The problem was that GDB was mistakenly deducing that the empry record
>  > was only a stub because of the lack of fields, and was therefore tagging
>  > it with TYPE_FLAG_STUB. This is not correct. Instead, the right
>  > approach, I believe, is to check for the DW_AT_declaration attribute.
>  > 
>  > 2004-02-19  J. Brobecker  <brobecker@gnat.com>
>  > 
>  >         * dwarf2read.c (read_structure_scope): Identify stub types
>  >         using the DW_AT_declaration attribute.
>  > 
>  > tested on x86-linux. No regression. Fixes the testcase that was
>  > proposed by Andrew (even though it is not legal C) and the Ada
>  > case.
>  > 
>  > OK to apply?
>  > 
>  > Thanks,
>  > -- 
>  > Joel
>  > 
>  > Index: dwarf2read.c
>  > ===================================================================
>  > RCS file: /cvs/src/src/gdb/dwarf2read.c,v
>  > retrieving revision 1.130
>  > diff -u -p -r1.130 dwarf2read.c
>  > --- dwarf2read.c	28 Jan 2004 18:43:06 -0000	1.130
>  > +++ dwarf2read.c	19 Feb 2004 13:58:42 -0000
>  > @@ -3077,6 +3077,9 @@ read_structure_scope (struct die_info *d
>  >        TYPE_LENGTH (type) = 0;
>  >      }
>  >  
>  > +  if (dwarf2_attr (die, DW_AT_declaration, cu) != NULL)
>  > +    TYPE_FLAGS (type) |= TYPE_FLAG_STUB;
>  > +
>  >    /* We need to add the type field to the die immediately so we don't
>  >       infinitely recurse when dealing with pointers to the structure
>  >       type within the structure itself. */
>  > @@ -3213,11 +3216,6 @@ read_structure_scope (struct die_info *d
>  >        new_symbol (die, type, cu);
>  >  
>  >        do_cleanups (back_to);
>  > -    }
>  > -  else
>  > -    {
>  > -      /* No children, must be stub. */
>  > -      TYPE_FLAGS (type) |= TYPE_FLAG_STUB;
>  >      }
>  >  
>  >    processing_current_prefix = previous_prefix;

Re: [RFA/dwarf-2] Fix for the null record problem

[Joel Brobecker]

Ping?

The patch is quite tiny, seems almost obvious, and Elena seemed happy
with it. But it didn't get in because she asked that a testcase be added
first (on Feb 19):
<<
the patch looks sensible, but I would like to see the testcase go in
at the same time, or we'll forget.
>>

The new testcase has been checked in now, so I was wondering if somebody
had a moment to have a look at it, and confirm Elena's first review?

Thanks,

> >  > I have found the source of the problem, and suggest the attached patch.
> >  > The problem was that GDB was mistakenly deducing that the empry record
> >  > was only a stub because of the lack of fields, and was therefore tagging
> >  > it with TYPE_FLAG_STUB. This is not correct. Instead, the right
> >  > approach, I believe, is to check for the DW_AT_declaration attribute.
> >  > 
> >  > 2004-02-19  J. Brobecker  <brobecker@gnat.com>
> >  > 
> >  >         * dwarf2read.c (read_structure_scope): Identify stub types
> >  >         using the DW_AT_declaration attribute.
> >  > 
> >  > tested on x86-linux. No regression. Fixes the testcase that was
> >  > proposed by Andrew (even though it is not legal C) and the Ada
> >  > case.
> >  > 
> >  > OK to apply?
> >  > 
> >  > Thanks,
> >  > -- 
> >  > Joel
> >  > 
> >  > Index: dwarf2read.c
> >  > ===================================================================
> >  > RCS file: /cvs/src/src/gdb/dwarf2read.c,v
> >  > retrieving revision 1.130
> >  > diff -u -p -r1.130 dwarf2read.c
> >  > --- dwarf2read.c	28 Jan 2004 18:43:06 -0000	1.130
> >  > +++ dwarf2read.c	19 Feb 2004 13:58:42 -0000
> >  > @@ -3077,6 +3077,9 @@ read_structure_scope (struct die_info *d
> >  >        TYPE_LENGTH (type) = 0;
> >  >      }
> >  >  
> >  > +  if (dwarf2_attr (die, DW_AT_declaration, cu) != NULL)
> >  > +    TYPE_FLAGS (type) |= TYPE_FLAG_STUB;
> >  > +
> >  >    /* We need to add the type field to the die immediately so we don't
> >  >       infinitely recurse when dealing with pointers to the structure
> >  >       type within the structure itself. */
> >  > @@ -3213,11 +3216,6 @@ read_structure_scope (struct die_info *d
> >  >        new_symbol (die, type, cu);
> >  >  
> >  >        do_cleanups (back_to);
> >  > -    }
> >  > -  else
> >  > -    {
> >  > -      /* No children, must be stub. */
> >  > -      TYPE_FLAGS (type) |= TYPE_FLAG_STUB;
> >  >      }
> >  >  
> >  >    processing_current_prefix = previous_prefix;

-- 
Joel

Re: [RFA/dwarf-2] Fix for the null record problem

[Jim Blandy]

Joel Brobecker <brobecker@gnat.com> writes:
> The patch is quite tiny, seems almost obvious, and Elena seemed happy
> with it. But it didn't get in because she asked that a testcase be added
> first (on Feb 19):
> <<
> the patch looks sensible, but I would like to see the testcase go in
> at the same time, or we'll forget.
> >>
> 
> The new testcase has been checked in now, so I was wondering if somebody
> had a moment to have a look at it, and confirm Elena's first review?

I think it looks fine; I've just one question before you commit.

As it stands, the code sets TYPE_FLAG_STUB if the type die has no
children, or if die_is_declaration (die, cu) is true.  Your patch
correctly ditches the first criterion; no problems there.

But it also modifies the second criterion as well, without comment.
In particular, die_is_declaration checks for both DW_AT_declaration
and DW_AT_specification, but your patch only tests for
DW_AT_declaration.  I think this is correct: in section 5.6.1, the
Dwarf 2 spec says that the definition of the type has a
DW_AT_specification attribute pointing to the declaration.  Since it's
the definition of the type that actually lists the fields,
DW_AT_specification should not cause GDB to mark the type as a stub.
Just the opposite: the referent of that attribute is the stub.

The following C++ code produces Dwarf 2 info where the definition of
struct s::t has a DW_AT_specification attribute, but GDB doesn't skip
it, and I don't really understand why:

    struct s {
      struct t;
      struct t *p;
    };

    struct s::t
    {
      int i;
    };

Have you followed all this through already?  What's going on here?

Re: [RFA/dwarf-2] Fix for the null record problem

[Daniel Jacobowitz]

On Wed, Apr 14, 2004 at 12:21:44PM -0500, Jim Blandy wrote:
> 
> Joel Brobecker <brobecker@gnat.com> writes:
> > The patch is quite tiny, seems almost obvious, and Elena seemed happy
> > with it. But it didn't get in because she asked that a testcase be added
> > first (on Feb 19):
> > <<
> > the patch looks sensible, but I would like to see the testcase go in
> > at the same time, or we'll forget.
> > >>
> > 
> > The new testcase has been checked in now, so I was wondering if somebody
> > had a moment to have a look at it, and confirm Elena's first review?
> 
> I think it looks fine; I've just one question before you commit.
> 
> As it stands, the code sets TYPE_FLAG_STUB if the type die has no
> children, or if die_is_declaration (die, cu) is true.  Your patch
> correctly ditches the first criterion; no problems there.
> 
> But it also modifies the second criterion as well, without comment.
> In particular, die_is_declaration checks for both DW_AT_declaration
> and DW_AT_specification, but your patch only tests for
> DW_AT_declaration.  I think this is correct: in section 5.6.1, the
> Dwarf 2 spec says that the definition of the type has a
> DW_AT_specification attribute pointing to the declaration.  Since it's
> the definition of the type that actually lists the fields,
> DW_AT_specification should not cause GDB to mark the type as a stub.
> Just the opposite: the referent of that attribute is the stub.
> 
> The following C++ code produces Dwarf 2 info where the definition of
> struct s::t has a DW_AT_specification attribute, but GDB doesn't skip
> it, and I don't really understand why:

Did you misread die_is_declaration?

  return (dwarf2_attr (die, DW_AT_declaration, cu)
          && ! dwarf2_attr (die, DW_AT_specification, cu));

I don't even know what that DW_AT_specification test is doing there -
the idea of a declaration with a specification is pretty peculiar.
But the important bit is that it's not going to report that something
is a declartion, if it has a DW_AT_specification.

If the check is important, then Joel should probably use
!die_is_declaration.  This was added by Jason Merrill in 2000, without
much of an explanation; here it is:
 http://sources.redhat.com/ml/gdb-patches/2000-q1/msg00015.html

It's not clear why the specification check is there; the important bit
was presumably:
 > !   if (die->has_children)
 > !   if (die->has_children && ! die_is_declaration (die))

i.e. the point of the patch was to add the DW_AT_declaration check.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA/dwarf-2] Fix for the null record problem

[Jim Blandy]

Daniel Jacobowitz <drow@false.org> writes:
> > The following C++ code produces Dwarf 2 info where the definition of
> > struct s::t has a DW_AT_specification attribute, but GDB doesn't skip
> > it, and I don't really understand why:
> 
> Did you misread die_is_declaration?
> 
>   return (dwarf2_attr (die, DW_AT_declaration, cu)
>           && ! dwarf2_attr (die, DW_AT_specification, cu));

Yes, I misread it.  That explains a lot.  :)

> I don't even know what that DW_AT_specification test is doing there -
> the idea of a declaration with a specification is pretty peculiar.
> ...
> It's not clear why the specification check is there; the important bit
> was presumably:
>  > !   if (die->has_children)
>  > !   if (die->has_children && ! die_is_declaration (die))
> 
> i.e. the point of the patch was to add the DW_AT_declaration check.

Yeah.  The other place it's used is in checking data members of
structs.  There, too, you shouldn't have declarations with
specifications.  I'll ask Jason if he remembers.

Joel,how about dropping the DW_AT_specification test from
die_is_declaration, and then using it in the new test in your patch?

Re: [RFA/dwarf-2] Fix for the null record problem

[Joel Brobecker]

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

Hello Jim,

Good catch about the is_declaration() case.

> Joel, how about dropping the DW_AT_specification test from
> die_is_declaration, and then using it in the new test in your patch?

Based on Jason's answer, it looks like the DW_AT_specification check
was indeed necessary. I have added a comment in that respect in the
little cleanup patch you suggested earlier. See:

    http://sources.redhat.com/ml/gdb-patches/2004-04/msg00325.html

Here a new version of the initial patch that follows your suggestion
of using die_is_declaration(). It's indeed more correct, and even
easier to read.

2004-04-15  Joel Brobecker  <brobecker@gnat.com>

        * dwarf2read.c (read_structure_scope): Identify stub types
        using die_is_declaration() only.

Tested on x86-linux, no regression.
OK to apply?

Thanks,
-- 
Joel

[-- Attachment #2: dwarf2read.c.null-record.diff --]
[-- Type: text/plain, Size: 764 bytes --]

--- dwarf2read.c.cleanup	2004-04-15 13:23:39.000000000 -0700
+++ dwarf2read.c	2004-04-15 13:36:10.000000000 -0700
@@ -3144,6 +3144,9 @@ read_structure_type (struct die_info *di
       TYPE_LENGTH (type) = 0;
     }
 
+  if (die_is_declaration (die, cu))
+    TYPE_FLAGS (type) |= TYPE_FLAG_STUB;
+
   /* We need to add the type field to the die immediately so we don't
      infinitely recurse when dealing with pointers to the structure
      type within the structure itself. */
@@ -3240,11 +3243,6 @@ read_structure_type (struct die_info *di
 
       do_cleanups (back_to);
     }
-  else
-    {
-      /* No children, must be stub. */
-      TYPE_FLAGS (type) |= TYPE_FLAG_STUB;
-    }
 
   processing_current_prefix = previous_prefix;
   if (back_to != NULL)

Re: [RFA/dwarf-2] Fix for the null record problem

[Joel Brobecker]

> 2004-04-15  Joel Brobecker  <brobecker@gnat.com>
> 
>         * dwarf2read.c (read_structure_scope): Identify stub types
>         using die_is_declaration() only.

Approved privately by JimB (Thanks Jim!), just checked in.

-- 
Joel

Re: [RFA/dwarf-2] Fix for the null record problem

[Jim Blandy]

Joel Brobecker <brobecker@gnat.com> writes:

> > 2004-04-15  Joel Brobecker  <brobecker@gnat.com>
> > 
> >         * dwarf2read.c (read_structure_scope): Identify stub types
> >         using die_is_declaration() only.
> 
> Approved privately by JimB (Thanks Jim!), just checked in.

Only private because I hit the wrong key and didn't notice.  Yes, this
is approved.

Re: [RFA/dwarf-2] Fix for the null record problem

[Jim Blandy]

Actually, I just realized: it's incorrect to write

        if (dwarf2_attr (die, DW_AT_declaration, cu)) ...;

when you want to do ... when die is a declaration.  As with any flag
attribute, you must write:

        decl_attr = dwarf2_attr (die, DW_AT_declaration, cu);
        if (decl_attr && DW_UNSND (decl_attr)) ...;

(It might not be bad to add a function 'attribute_true_p (DIE, ATTR)'
that captures this logic, because it's used a lot.  But that should be
a separate cleanup; don't feel obliged.)

Re: [RFA/dwarf-2] Fix for the null record problem

[Michael Elizabeth Chastain]

I like the idea, but there are two gotchas:

. update the copyright year in class2.exp
. quote the '{' and '}' chars in class2.exp:

  "= \{<No data fields>\}"

With these two changes, and if you re-test, then I approve the patch.

Michael C

===

2004-02-26  J. Brobecker  <brobecker@gnat.com>

        * gdb.cp/class2.cc (empty): New class.
        (refer): New function.
        (main): Declare an object of type empty and use it.
        * gdb.cp/class2.exp: Print the value of an object of type empty.

Re: [RFA/dwarf-2] Fix for the null record problem

[Joel Brobecker]

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

> I like the idea, but there are two gotchas:
> 
> . update the copyright year in class2.exp
    (sound of a club banging on my head)

> . quote the '{' and '}' chars in class2.exp:
> 
>   "= \{<No data fields>\}"
> 
> With these two changes, and if you re-test, then I approve the patch.

Thanks. Here is what I ended up checking in.

2004-02-26  J. Brobecker  <brobecker@gnat.com>

        * gdb.cp/class2.cc (empty): New class.
        (refer): New function.
        (main): Declare an object of type empty and use it.
        * gdb.cp/class2.exp: Print the value of an object of type empty.

Re-tested on x86-linux.

-- 
Joel

[-- Attachment #2: empty.diff --]
[-- Type: text/plain, Size: 1562 bytes --]

Index: class2.cc
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.cp/class2.cc,v
retrieving revision 1.2
diff -u -r1.2 class2.cc
--- class2.cc	11 Feb 2004 14:01:25 -0000	1.2
+++ class2.cc	26 Feb 2004 20:09:08 -0000
@@ -48,10 +48,19 @@
   ;
 }
 
+struct empty {};
+
+// Stop the compiler from optimizing away data.
+void refer (empty *)
+{
+  ;
+}
+
 int main (void)
 {
   A alpha, *aap, *abp;
   B beta, *bbp;
+  empty e;
 
   alpha.a1 = 100;
   beta.a1 = 200; beta.b1 = 201; beta.b2 = 202;
@@ -59,6 +68,7 @@
   aap = &alpha; refer (aap);
   abp = &beta;  refer (abp);
   bbp = &beta;  refer (bbp);
+  refer (&e);
 
   return 0;  // marker return 0
 } // marker close brace
Index: class2.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.cp/class2.exp,v
retrieving revision 1.1
diff -u -r1.1 class2.exp
--- class2.exp	25 Nov 2003 15:01:31 -0000	1.1
+++ class2.exp	26 Feb 2004 20:09:08 -0000
@@ -1,4 +1,4 @@
-# Copyright 2003 Free Software Foundation, Inc.
+# Copyright 2003, 2004 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
@@ -113,3 +113,7 @@
 gdb_test "print * (B *) abp" \
     "= {.*a1 = 200.*b1 = 201.*b2 = 202}" \
     "print * (B *) abp at marker return 0"
+
+# Printing the value of an object containing no data fields:
+
+gdb_test "p e" "= \{<No data fields>\}" "print object with no data fields"

Re: [RFA/dwarf-2] Fix for the null record problem

[Michael Elizabeth Chastain]

Thanks Joel.

===

2004-02-26  J. Brobecker  <brobecker@gnat.com>

        * gdb.cp/class2.cc (empty): New class.
        (refer): New function.
        (main): Declare an object of type empty and use it.
        * gdb.cp/class2.exp: Print the value of an object of type empty.