[RFA/DWARF] Set TYPE_FLAG_STUB for enum DIEs that are declarations only

[RFA/DWARF] Set TYPE_FLAG_STUB for enum DIEs that are declarations only

[Joel Brobecker]

Long explaination but small patch :-).

For those who are not familiar with Ada, the declaration of types
whose implementation must be opaque is done as follow:

        type My_Opaque type is private;

And then the language expects that you provide the definition
of that type in the private section of your package spec, like so:

        private
           type My_Opaque is (One, Two, Three);

But there has been a small amendment to that which allows a developer
to provide the actual definition not in the spec, but in the body.
This is an approach that is not used very often, but still legitimate.
These types are called "Taft Amendment Types" (TAT) because they were
introduced by Tucker Taft, I believe.

Unfortunately, we found out that this can cause trouble in the debugger.
When compiling a unit that uses a TAT, the compiler does not look
inside the package body, but only in the package spec. So it knows that
we have an object of that type, but it doesn't know what that type is.
As a result, it generates a bogus "stub" enumeration type and sets
its AT_declaration attribute. The debugger missed that part, and so
we ended up thinking that our object was a degenerated enum type,
and printed its value accordingly:

        (gdb) p w.e.all
        $1 = 0

The expected output is:

        (gdb) p w.e.all
        $1 = (month => 8, year => 1974)

The fix is to flag the type as a TYPE_FLAG_STUB, so that we will
know to lookup the complete definition.

2008-01-03  Joel Brobecker  <brobecker@adacore.com>

        * dwarf2read.c (read_enumeration_type): Flag type as stub if
        the given die is a declaration.

I also wrote a small testcase that reproduces the issue:

2008-01-03  Joel Brobecker  <brobecker@adacore.com>

        * gdb.ada/taft_type/pck.ads, gdb.ada/taft_type/pck.adb,
        gdb.ada/taft_type/p.adb: New files.
        * gdb.ada/taft_type.exp: New testcase.

Tested on x86-linux. No regression.
OK to apply?

Thanks,
-- 
Joel

[RFA/DWARF] Set TYPE_FLAG_STUB for enum DIEs that are declarations only

[Joel Brobecker]

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

[resending with the patch, sorry...]

Long explaination but small patch :-).

For those who are not familiar with Ada, the declaration of types
whose implementation must be opaque is done as follow:

        type My_Opaque type is private;

And then the language expects that you provide the definition
of that type in the private section of your package spec, like so:

        private
           type My_Opaque is (One, Two, Three);

But there has been a small amendment to that which allows a developer
to provide the actual definition not in the spec, but in the body.
This is an approach that is not used very often, but still legitimate.
These types are called "Taft Amendment Types" (TAT) because they were
introduced by Tucker Taft, I believe.

Unfortunately, we found out that this can cause trouble in the debugger.
When compiling a unit that uses a TAT, the compiler does not look
inside the package body, but only in the package spec. So it knows that
we have an object of that type, but it doesn't know what that type is.
As a result, it generates a bogus "stub" enumeration type and sets
its AT_declaration attribute. The debugger missed that part, and so
we ended up thinking that our object was a degenerated enum type,
and printed its value accordingly:

        (gdb) p w.e.all
        $1 = 0

The expected output is:

        (gdb) p w.e.all
        $1 = (month => 8, year => 1974)

The fix is to flag the type as a TYPE_FLAG_STUB, so that we will
know to lookup the complete definition.

2008-01-03  Joel Brobecker  <brobecker@adacore.com>

        * dwarf2read.c (read_enumeration_type): Flag type as stub if
        the given die is a declaration.

I also wrote a small testcase that reproduces the issue:

2008-01-03  Joel Brobecker  <brobecker@adacore.com>

        * gdb.ada/taft_type/pck.ads, gdb.ada/taft_type/pck.adb,
        gdb.ada/taft_type/p.adb: New files.
        * gdb.ada/taft_type.exp: New testcase.

Tested on x86-linux. No regression.
OK to apply?

Thanks,
-- 
Joel

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

Index: dwarf2read.c
===================================================================
--- dwarf2read.c	(revision 59)
+++ dwarf2read.c	(revision 60)
@@ -4237,6 +4237,9 @@ read_enumeration_type (struct die_info *
       TYPE_LENGTH (type) = 0;
     }
 
+  if (die_is_declaration (die, cu))
+    TYPE_FLAGS (type) |= TYPE_FLAG_STUB;
+
   set_die_type (die, type, cu);
 }
 

[-- Attachment #3: taft-tc.diff --]
[-- Type: text/plain, Size: 4959 bytes --]

Index: gdb.ada/taft_type.exp
===================================================================
--- gdb.ada/taft_type.exp	(revision 0)
+++ gdb.ada/taft_type.exp	(revision 61)
@@ -0,0 +1,46 @@
+# Copyright 2008 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 3 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, see <http://www.gnu.org/licenses/>.
+
+if $tracelevel then {
+    strace $tracelevel
+}
+
+load_lib "ada.exp"
+
+set testdir "taft_type"
+set testfile "${testdir}/p"
+set srcfile ${srcdir}/${subdir}/${testfile}.adb
+set binfile ${objdir}/${subdir}/${testfile}
+
+file mkdir ${objdir}/${subdir}/${testdir}
+if {[gdb_compile_ada "${srcfile}" "${binfile}" executable [list debug ]] != "" } {
+  return -1
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+set bp_location [gdb_get_line_number "START" ${testdir}/p.adb]
+if ![runto "p.adb:$bp_location" ] then {
+  perror "Couldn't run ${testfile}"
+  return
+} 
+
+gdb_test "print w.e.all" \
+         "\\(month => 8, year => 1974\\)" \
+         "print w.e.all"
+
Index: gdb.ada/taft_type/pck.adb
===================================================================
--- gdb.ada/taft_type/pck.adb	(revision 0)
+++ gdb.ada/taft_type/pck.adb	(revision 61)
@@ -0,0 +1,29 @@
+--  Copyright 2008 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 3 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, see <http://www.gnu.org/licenses/>.
+
+
+package body Pck is
+
+   type Empty is record
+      Month : Integer;
+      Year  : Integer;
+   end record;
+
+   function Create return Wrap is
+   begin
+      return (E => new Empty'(Month => 8, Year => 1974));
+   end Create;
+
+end Pck;
Index: gdb.ada/taft_type/pck.ads
===================================================================
--- gdb.ada/taft_type/pck.ads	(revision 0)
+++ gdb.ada/taft_type/pck.ads	(revision 61)
@@ -0,0 +1,31 @@
+--  Copyright 2008 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 3 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, see <http://www.gnu.org/licenses/>.
+
+package Pck is
+
+   type Wrap is private;
+
+   function Create return Wrap;
+
+private
+
+   type Empty;
+   type Empty_Access is access Empty;
+
+   type Wrap is record
+      E : Empty_Access;
+   end record;
+
+end Pck;
Index: gdb.ada/taft_type/p.adb
===================================================================
--- gdb.ada/taft_type/p.adb	(revision 0)
+++ gdb.ada/taft_type/p.adb	(revision 61)
@@ -0,0 +1,23 @@
+--  Copyright 2008 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 3 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, see <http://www.gnu.org/licenses/>.
+
+with Pck; use Pck;
+
+procedure P is
+   W : Wrap;
+begin
+   W := Create;
+end P;  --  START
+

Re: [RFA/DWARF] Set TYPE_FLAG_STUB for enum DIEs that are declarations only

[Daniel Jacobowitz]

On Thu, Jan 03, 2008 at 07:40:55AM -0800, Joel Brobecker wrote:
> 2008-01-03  Joel Brobecker  <brobecker@adacore.com>
> 
>         * dwarf2read.c (read_enumeration_type): Flag type as stub if
>         the given die is a declaration.

OK.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA/DWARF] Set TYPE_FLAG_STUB for enum DIEs that are declarations only

[Joel Brobecker]

> > 2008-01-03  Joel Brobecker  <brobecker@adacore.com>
> > 
> >         * dwarf2read.c (read_enumeration_type): Flag type as stub if
> >         the given die is a declaration.
> 
> OK.

Thank you! Now checked in.

-- 
Joel

Re: [RFA/DWARF] Set TYPE_FLAG_STUB for enum DIEs that are declarations only

[Eli Zaretskii]

> Date: Thu, 3 Jan 2008 07:40:55 -0800
> From: Joel Brobecker <brobecker@adacore.com>
> 
> --- dwarf2read.c	(revision 59)
> +++ dwarf2read.c	(revision 60)
> @@ -4237,6 +4237,9 @@ read_enumeration_type (struct die_info *
>        TYPE_LENGTH (type) = 0;
>      }
>  
> +  if (die_is_declaration (die, cu))
> +    TYPE_FLAGS (type) |= TYPE_FLAG_STUB;
> +

I'd suggest a comment here to explain why we do this.

Re: [RFA/DWARF] Set TYPE_FLAG_STUB for enum DIEs that are declarations only

[Joel Brobecker]

> > +  if (die_is_declaration (die, cu))
> > +    TYPE_FLAGS (type) |= TYPE_FLAG_STUB;
> > +
> 
> I'd suggest a comment here to explain why we do this.

I am the kind that usually writes a lot of comments, but I think
it would be surperfluous in this case: TYPE_FLAG_STUB and a non-zero
DW_AT_declaration attribute are two ways of saying the same thing.
Are you sure you want a comment?

  /* If the type is a declaration, then the definition is not
     complete.  Mark the type as a stub.  */

This comment is just repeating the code.

-- 
Joel

Re: [RFA/DWARF] Set TYPE_FLAG_STUB for enum DIEs that are declarations only

[Eli Zaretskii]

> Date: Sat, 5 Jan 2008 07:05:14 -0800
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: gdb-patches@sourceware.org
> 
> TYPE_FLAG_STUB and a non-zero DW_AT_declaration attribute are two
> ways of saying the same thing.

And this is supposed to be known to Joe Random Hacker reading the code
because ...?  Should we assume that whoever reads dwarf2read.c is
necessarily a DWARF-2 expert?

> Are you sure you want a comment?
> 
>   /* If the type is a declaration, then the definition is not
>      complete.  Mark the type as a stub.  */
> 
> This comment is just repeating the code.

That's not the kind of comment I was suggesting.  I meant some summary
of the long explanation you posted as a preamble to the patch, which
explained why debugging Ada programs needed that.

Re: [RFA/DWARF] Set TYPE_FLAG_STUB for enum DIEs that are declarations only

[Joel Brobecker]

> And this is supposed to be known to Joe Random Hacker reading the code
> because ...?  Should we assume that whoever reads dwarf2read.c is
> necessarily a DWARF-2 expert?

I honestly believe that whoever reads dwarf2read.c needs to understand
a minimum the DWARF standard - otherwise he's wasting his time. And
I personally not in the business of duplicating the DWARF standard
in our code unless it's obscure.

> That's not the kind of comment I was suggesting.  I meant some summary
> of the long explanation you posted as a preamble to the patch, which
> explained why debugging Ada programs needed that.

The preample was an introduction to an part of the Ada language, not
a description of a problem.  The "stub" DIE is a routine concept,
that we see all the time with C opaque types. In fact, any type
can be incomplete.

But rather than arguing this ad vitam aeternam, here's a suggestion:

  /* Enumeration DIEs descriptions can be imcomplete.  In Ada, any
     type can be declared as private in the package spec, and then
     defined only inside the package body.  Such types are known as
     Taft Amendment Types.  When another package uses such a type,
     an incomplete DIE may be generated by the compiler.  */

-- 
Joel

Re: [RFA/DWARF] Set TYPE_FLAG_STUB for enum DIEs that are declarations only

[Joel Brobecker]

Hi Eli,

> > That's not the kind of comment I was suggesting.  I meant some summary
> > of the long explanation you posted as a preamble to the patch, which
> > explained why debugging Ada programs needed that.
> 
> The preample was an introduction to an part of the Ada language, not
> a description of a problem.  The "stub" DIE is a routine concept,
> that we see all the time with C opaque types. In fact, any type
> can be incomplete.
> 
> But rather than arguing this ad vitam aeternam, here's a suggestion:

First of all, I have to apologize for writing this way to you.
I admit to being fustrated, but my fustration was not with you. it
was with myself, because I don't understand exactly what it is that
you'd like me to put in the comment.

I know this situation will happen again in the future because we
all have different conceptions of what necessary documentation is.
I will try to say it openly when I don't understand, so that you
can help me (please :).

Do you still think a comment should be added? If yes, is the comment
I suggested a step in the right direction?

>   /* Enumeration DIEs descriptions can be imcomplete.  In Ada, any
>      type can be declared as private in the package spec, and then
>      defined only inside the package body.  Such types are known as
>      Taft Amendment Types.  When another package uses such a type,
>      an incomplete DIE may be generated by the compiler.  */

-- 
Joel

Re: [RFA/DWARF] Set TYPE_FLAG_STUB for enum DIEs that are declarations only

[Eli Zaretskii]

> Date: Mon, 7 Jan 2008 21:57:14 -0800
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: gdb-patches@sourceware.org
> 
> > The preample was an introduction to an part of the Ada language, not
> > a description of a problem.  The "stub" DIE is a routine concept,
> > that we see all the time with C opaque types. In fact, any type
> > can be incomplete.
> > 
> > But rather than arguing this ad vitam aeternam, here's a suggestion:
> 
> First of all, I have to apologize for writing this way to you.

I never received that, so no harm done.

> Do you still think a comment should be added? If yes, is the comment
> I suggested a step in the right direction?
> 
> >   /* Enumeration DIEs descriptions can be imcomplete.  In Ada, any
> >      type can be declared as private in the package spec, and then
> >      defined only inside the package body.  Such types are known as
> >      Taft Amendment Types.  When another package uses such a type,
> >      an incomplete DIE may be generated by the compiler.  */

Yes, thanks.  All I wanted was some kind of reference to Ada and
private type declaration.  Armed with these clues, an intelligent
reader will find out the rest.

Re: [RFA/DWARF] Set TYPE_FLAG_STUB for enum DIEs that are declarations only

[Joel Brobecker]

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

> Yes, thanks.  All I wanted was some kind of reference to Ada and
> private type declaration.  Armed with these clues, an intelligent
> reader will find out the rest.

Great! I checked the following change in:

2008-01-08  Joel Brobecker  <brobecker@adacore.com>

        * dwarf2read.c (read_enumeration_type): Add comment.

Thanks,
-- 
Joel

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

Index: dwarf2read.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2read.c,v
retrieving revision 1.247
diff -u -p -r1.247 dwarf2read.c
--- dwarf2read.c	3 Jan 2008 17:29:55 -0000	1.247
+++ dwarf2read.c	9 Jan 2008 04:26:35 -0000
@@ -4233,6 +4233,11 @@ read_enumeration_type (struct die_info *
       TYPE_LENGTH (type) = 0;
     }
 
+  /* The enumeration DIE can be incomplete.  In Ada, any type can be
+     declared as private in the package spec, and then defined only
+     inside the package body.  Such types are known as Taft Amendment
+     Types.  When another package uses such a type, an incomplete DIE
+     may be generated by the compiler.  */
   if (die_is_declaration (die, cu))
     TYPE_FLAGS (type) |= TYPE_FLAG_STUB;