[RFA/stabs] fix failed assertion during replacement of undefined type

[RFA/stabs] fix failed assertion during replacement of undefined type

[Joel Brobecker]

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

Hello,

First off: I hate stabs (I feel better now). Unfortunately, we haven't
been able to transition certain platforms such as AIX to DWARF, so
we're stuck with it for a little while longer. I was able to reproduce
the same problem on x86-linux with -gstabs+, so the following analysis
will assume x86-linux.

I came across a case that causes the following internal error while
reading some debugging info.

    (gdb) ptype pck.data_flag
    gdbtypes.c:640: internal-error: replace_type: Assertion `TYPE_INSTANCE_FLAGS (ntype) == TYPE_INSTANCE_FLAGS (type)' failed.
    A problem internal to GDB has been detected,
    further debugging may prove unreliable.
    Quit this debugging session? (y or n)

This type of this variable is define as an enumerated type with
an associated pragma Atomic:

   type Data_T is (One, Two, Three);
   pragma Atomic (Data_T);

The compiler ended up emitting the following debugging info for this
type, which is incomplete:

    .stabs  "pck__data_t:t(0,32)=B(0,33)=xepck__data_t:",128,0,3,0

Here is what happens chonologically:

  1. The reference to pck__data_t ("xepck__data_t") cannot be resolved
     because this type hasn't been defined yet - in fact, this entry
     is a broken attempt at defining it.  So we create a stub type
     for it with name "pck__data_t", and null instance flags.

  2. Now that our referenced type has been created, we're trying to
     create the volatile version ('B') of our new type, and so we
     create a new type with the same name, but different instance
     flags.

  3. After having processed all debugging info for this unit, we try
     to resolve all the types that are still undefined, and we do this
     more or less by matching by name (we actually also check type code
     as well as class & domain).

     For our type above, we end up finding the symbol whose type is
     volatile, and try to replace the undefined version by the one
     we just found.  This is when replace_type reports the failed
     assertion.

The debugging info is screwy, and I've asked our GCC team to look into
this. But in the meantime, it's pretty simple to handle the situation
more gracefully. I had several ideas, one of them being to smash the
instance flags of the undefined type.  But this doesn't bring any
benefit, since we would be replacing our undefined type by an incomplete
type. So I decided to take an even simpler approach and treat the types
as different, and not do the replacement unless the instance flags are
consistent. I've added a comment explaining how we do our matching and
why checking the instance flags is necessary.

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

        * stabsread.c (cleanup_undefined_types_1): Add instance flags check
        in the search for the matching symbol.

Tested on x86-linux with -gstabs+.

I also wrote a new testcase

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

	* gdb.ada/atomic_enum: New test program.
        * gdb.ada/atomic_enum.exp: New testcase.

Before the patch is applied, I get with -gstabs+:

    FAIL: gdb.ada/atomic_enum.exp: ptype pck.data_flag (GDB internal error)

After the patch is applied, I get an XFAIL. And if the example program
is built with DWARF, then I get a PASS before and after.

OK to apply?

Thanks,
-- 
Joel

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

Index: stabsread.c
===================================================================
RCS file: /cvs/src/src/gdb/stabsread.c,v
retrieving revision 1.105
diff -u -p -r1.105 stabsread.c
--- stabsread.c	26 Mar 2008 14:53:28 -0000	1.105
+++ stabsread.c	3 Apr 2008 20:50:52 -0000
@@ -4287,6 +4287,25 @@ cleanup_undefined_types_1 (void)
 {
   struct type **type;
 
+  /* Iterate over every undefined type, and look for a symbol whose type
+     matches our undefined type.  The symbol matches if:
+       1. It is a typedef in the STRUCT domain;
+       2. It has the same name, and same type code;
+       3. The instance flags are identical.
+     
+     It is important to check the instance flags, because we have seen
+     examples where the debug info contained definitions such as:
+
+         "foo_t:t30=B31=xefoo_t:"
+
+     In this case, we have created an undefined type named "foo_t" whose
+     instance flags is null (when processing "xefoo_t"), and then created
+     another type with the same name, but with different instance flags
+     ('B' means volatile).  I think that the definition above is wrong,
+     since the same type cannot be volatile and non-volatile at the same
+     time, but we need to be able to cope with it when it happens.  The
+     approach taken here is to treat these two types as different.  */
+
   for (type = undef_types; type < undef_types + undef_types_length; type++)
     {
       switch (TYPE_CODE (*type))
@@ -4322,7 +4341,10 @@ cleanup_undefined_types_1 (void)
 			    && SYMBOL_DOMAIN (sym) == STRUCT_DOMAIN
 			    && (TYPE_CODE (SYMBOL_TYPE (sym)) ==
 				TYPE_CODE (*type))
-			    && strcmp (DEPRECATED_SYMBOL_NAME (sym), typename) == 0)
+			    && (TYPE_INSTANCE_FLAGS (*type) ==
+				TYPE_INSTANCE_FLAGS (SYMBOL_TYPE (sym)))
+			    && strcmp (DEPRECATED_SYMBOL_NAME (sym),
+				       typename) == 0)
                           replace_type (*type, SYMBOL_TYPE (sym));
 		      }
 		  }

[-- Attachment #3: atomic_enum.exp --]
[-- Type: text/plain, Size: 1423 bytes --]

# 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 "atomic_enum"
set testfile "${testdir}/foo"
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 additional_flags=-gnat05 ]] != "" } {
  return -1
}

gdb_exit
gdb_start
gdb_reinitialize_dir $srcdir/$subdir
gdb_load ${binfile}

set test "ptype pck.data_flag"
gdb_test_multiple "$test" $test {
    -re "type = \\(one, two, three\\).*$gdb_prompt $" {
        pass $test
        }
    -re "type = \\(\\).*$gdb_prompt $" {
        # This is a known compiler problem with Stabs.
        xfail $test
        }
}


[-- Attachment #4: pck.ads --]
[-- Type: text/plain, Size: 797 bytes --]

--  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

   procedure Increment;
   function Is_First return Boolean;

end Pck;

[-- Attachment #5: pck.adb --]
[-- Type: text/plain, Size: 1141 bytes --]

--  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 Data_T is (One, Two, Three);
   pragma Atomic (Data_T);

   Data_Flag : Data_T := One;

   procedure Increment is
   begin
      if Data_Flag = Data_T'Last then
         Data_Flag := Data_T'First;
      else
         Data_Flag := Data_T'Succ (Data_Flag);
      end if;
   end Increment;

   function Is_First return Boolean is
   begin
      return Data_Flag = Data_T'First;
   end Is_First;

end Pck;

[-- Attachment #6: foo.adb --]
[-- Type: text/plain, Size: 810 bytes --]

--  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 Foo is
begin
   if Is_First then
      Increment;
   end if;
end Foo;

Re: [RFA/stabs] fix failed assertion during replacement of undefined type

[Daniel Jacobowitz]

On Thu, Apr 03, 2008 at 02:29:47PM -0700, Joel Brobecker wrote:
> The compiler ended up emitting the following debugging info for this
> type, which is incomplete:
> 
>     .stabs  "pck__data_t:t(0,32)=B(0,33)=xepck__data_t:",128,0,3,0

I think we should simply refuse this.  It's circular and obviously
broken.  We have a reference to a named type and the reference is
clearly intended to be to a non-volatile type, so GDB blowing up is
not entirely unreasonable.

That said, as you know, I care very little about the pathological
cases in our stabs parser; since no one more knowledgeable has
stepped up to object, this seems fine to commit.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA/stabs] fix failed assertion during replacement of undefined type

[Joel Brobecker]

> > The compiler ended up emitting the following debugging info for this
> > type, which is incomplete:
> > 
> >     .stabs  "pck__data_t:t(0,32)=B(0,33)=xepck__data_t:",128,0,3,0
> 
> I think we should simply refuse this.  It's circular and obviously
> broken.  We have a reference to a named type and the reference is
> clearly intended to be to a non-volatile type, so GDB blowing up is
> not entirely unreasonable.

I agree that the stabs is wrong, and that we should refuse it. The problem
is that I haven't found a better way of doing that while reading the stabs,
so I decided to do it during the final phase when we are fixing undefined
types.

-- 
Joel

Re: [RFA/stabs] fix failed assertion during replacement of undefined type

[Joel Brobecker]

> 2008-04-03  Joel Brobecker  <brobecker@adacore.com>
> 
>         * stabsread.c (cleanup_undefined_types_1): Add instance flags check
>         in the search for the matching symbol.
> 
[...]
> 2008-04-03  Joel Brobecker  <brobecker@adacore.com>
> 
> 	* gdb.ada/atomic_enum: New test program.
>       * gdb.ada/atomic_enum.exp: New testcase.
> 

I have applied this patch. I know Daniel was a little disgusted at the stabs
entry that this patch is trying to handle and wasn't very enthousiastic at
getting this one in. I'm putting it in because I think it can help others
outside of AdaCore customers, but I'm absolutely OK with backing it out.

-- 
Joel

Re: [RFA/stabs] fix failed assertion during replacement of undefined type

[Daniel Jacobowitz]

On Fri, Apr 18, 2008 at 06:38:52PM -0700, Joel Brobecker wrote:
> I have applied this patch. I know Daniel was a little disgusted at the stabs
> entry that this patch is trying to handle and wasn't very enthousiastic at
> getting this one in. I'm putting it in because I think it can help others
> outside of AdaCore customers, but I'm absolutely OK with backing it out.

That's fine - sorry if I wasn't clear, but I've no objection to the
patch.

-- 
Daniel Jacobowitz
CodeSourcery