[RFC] Fix bug report 11479

[RFC] Fix bug report 11479

[Pierre Muller]

  I finally for once decide to use the bug database.
I reported bug 11479, with a simple test case.
http://sourceware.org/bugzilla/show_bug.cgi?id=11479

  Here is the patch that fixes the problem for me.

  This is not a direct RFA for two reasons:
 1) I don't remember how the ChangeLog should reflect the fact
that the commit is related to this bug.
 2) concerning  the fix in stabsread.c,
this chain of different types that all point to the same main type
could apparently be something else than just
a simple 'const' or 'volatile' modifier.
  But I don't think that I know enough
about the other possibilities to know if I should exclude them
from my patch by checking if only TYPE_CONST and TYPE_VOLATILE are
different.

Pierre


2010-04-08  Pierre Muller  <muller@ics.u-strasbg.fr>

	* stabsread.c (read_struct_type): Also set length of
	other types in the chain.
	
Testsuite ChangeLog entry:

2010-04-08  Pierre Muller  <muller@ics.u-strasbg.fr>
	Test for bug 11479.
	* gdb.stabs/gdb11479.exp: New file.
	* gdb.stabs/gdb11479.c: New file.

Index: src/gdb/stabsread.c
===================================================================
RCS file: /cvs/src/src/gdb/stabsread.c,v
retrieving revision 1.124
diff -u -p -r1.124 stabsread.c
--- src/gdb/stabsread.c	5 Apr 2010 22:43:47 -0000	1.124
+++ src/gdb/stabsread.c	8 Apr 2010 16:35:24 -0000
@@ -3448,9 +3448,17 @@ read_struct_type (char **pp, struct type
 
   {
     int nbits;
+    struct type *ntype;
     TYPE_LENGTH (type) = read_huge_number (pp, 0, &nbits, 0);
     if (nbits != 0)
       return error_type (pp, objfile);
+    ntype = TYPE_CHAIN (type);
+    while (ntype != type)
+      {
+	if (TYPE_LENGTH(ntype) == 0)
+	  TYPE_LENGTH (ntype) = TYPE_LENGTH (type);
+	ntype = TYPE_CHAIN (ntype);
+      }
   }
 
   /* Now read the baseclasses, if any, read the regular C struct or C++
Index: src/gdb/testsuite/gdb.stabs/gdb11479.exp
===================================================================
RCS file: testsuite/gdb.stabs/gdb11479.exp
diff -N testsuite/gdb.stabs/gdb11479.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ src/gdb/testsuite/gdb.stabs/gdb11479.exp	8 Apr 2010 16:35:24 -0000
@@ -0,0 +1,51 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2010 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/>.
+
+# Please email any bugs, comments, and/or additions to this file to:
+# bug-gdb@gnu.org
+
+# Test GDB stabs problem with qualified parameter of forward types.
+
+if $tracelevel then {
+	strace $tracelevel
+}
+
+set prms_id 0
+set bug_id 0
+
+set testfile "gdb11479"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable
{debug}] != "" } {
+    untested "couldn't compile ${srcdir}/${subdir}/${srcfile}"
+    return -1
+}
+
+# Start with a fresh gdb.
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+# Regression test for a cleanup bug in the charset code.
+gdb_test "rb test" "" "Set breakpoints"
+gdb_test "run" "Breakpoint .* test2 .*" "Stop at first breakpoint"
+gdb_test "print *t" ".*\{x = 5, y = 25, b = 2.5\}.*" "Inspect t in test2"
+gdb_test "continue" "Breakpoint .* test .*" "Stop at first breakpoint"
+gdb_test "print *t" ".*\{x = 5, y = 25, b = 2.5\}.*" "Inspect t in test"
+
+gdb_exit 
Index: src/gdb/testsuite/gdb.stabs/gdb11479.c
===================================================================
RCS file: testsuite/gdb.stabs/gdb11479.c
diff -N testsuite/gdb.stabs/gdb11479.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ src/gdb/testsuite/gdb.stabs/gdb11479.c	8 Apr 2010 16:35:24 -0000
@@ -0,0 +1,61 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010 Free Software Foundation, Inc.
+
+   Contributed by Pierre Muller.
+
+   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/>.
+
+   Please email any bugs, comments, and/or additions to this file to:
+   bug-gdb@gnu.org  */
+
+/* Qualifiers of forward types are not resolved correctly with stabs.  */
+
+struct dummy;
+
+const void *
+test (const struct dummy *t)
+{
+  const struct dummy *tt;
+  tt = t;
+  return t;
+}
+
+void *
+test2 (struct dummy *t)
+{
+  struct dummy *tt;
+  tt = t;
+  return t;
+}
+
+
+struct dummy {
+ int x;
+ int y;
+ double b;
+} tag_dummy;
+
+
+int
+main ()
+{
+  struct dummy tt;
+  tt.x = 5;
+  tt.y = 25;
+  tt.b = 2.5;
+  test2 (&tt);
+  test (&tt);
+  return 0;
+}

Re: [RFC] Fix bug report 11479

[Joel Brobecker]

>   This is not a direct RFA for two reasons:
[...]
>  2) concerning  the fix in stabsread.c, this chain of different types
>  that all point to the same main type could apparently be something
>  else than just a simple 'const' or 'volatile' modifier.  But I don't
>  think that I know enough about the other possibilities to know if I
>  should exclude them from my patch by checking if only TYPE_CONST and
>  TYPE_VOLATILE are different.

It's OK to post an RFA even if there are questions you don't know how
to answer. If we don't either, then we'll just have to go with what
we have and fix any problem we might find later.

> 2010-04-08  Pierre Muller  <muller@ics.u-strasbg.fr>
> 
> 	* stabsread.c (read_struct_type): Also set length of
> 	other types in the chain.

It would have been useful if you had also provided a copy of the stabs
for us to look at. I think I managed to generate them from the C file
you provided:

        .stabs  "t:p(0,21)=*(0,22)=k(0,23)=xsdummy:",160,0,28,-24

Followed later on by the actual definition:

        .stabs  "dummy:T(0,23)=s16x:(0,1),0,32;y:(0,1),32,32;b:(0,13),64,64;;",128,0,0,0

What I don't get is that the xsdummy should lead to what we call an
undefined type in stabsread.c, which should be put in the undef_types
queue. This queue is then processed at the end to fix all undefined
types, including the various volatile/const variants in the type chain.

Any idea why this is not working this way for your example?

> 2010-04-08  Pierre Muller  <muller@ics.u-strasbg.fr>
> 	Test for bug 11479.
> 	* gdb.stabs/gdb11479.exp: New file.
> 	* gdb.stabs/gdb11479.c: New file.

Just a small cleanup request:

> +# Please email any bugs, comments, and/or additions to this file to:
> +# bug-gdb@gnu.org

I think we decided to remove this from the header - does this email
address even works nowadays?

> +set testfile "gdb11479"
> +set srcfile ${testfile}.c
> +set binfile ${objdir}/${subdir}/${testfile}
> +if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable
> {debug}] != "" } {
> +    untested "couldn't compile ${srcdir}/${subdir}/${srcfile}"
> +    return -1
> +}
> +
> +# Start with a fresh gdb.
> +gdb_exit
> +gdb_start
> +gdb_reinitialize_dir $srcdir/$subdir
> +gdb_load ${binfile}

The above can be replaced by:

    set testfile "gdb11479"
    set srcfile ${testfile}.c
    if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile}] } {
        return -1



> +   Please email any bugs, comments, and/or additions to this file to:
> +   bug-gdb@gnu.org  */

Same as above.

-- 
Joel

RE: [RFC] Fix bug report 11479

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Joel Brobecker
> Envoyé : Monday, April 12, 2010 5:55 PM
> À : Pierre Muller
> Cc : gdb-patches@sourceware.org
> Objet : Re: [RFC] Fix bug report 11479
> 
> >   This is not a direct RFA for two reasons:
> [...]
> >  2) concerning  the fix in stabsread.c, this chain of different types
> >  that all point to the same main type could apparently be something
> >  else than just a simple 'const' or 'volatile' modifier.  But I don't
> >  think that I know enough about the other possibilities to know if I
> >  should exclude them from my patch by checking if only TYPE_CONST and
> >  TYPE_VOLATILE are different.
> 
> It's OK to post an RFA even if there are questions you don't know how
> to answer. If we don't either, then we'll just have to go with what
> we have and fix any problem we might find later.

  OK, I will remember that.
 
> > 2010-04-08  Pierre Muller  <muller@ics.u-strasbg.fr>
> >
> > 	* stabsread.c (read_struct_type): Also set length of
> > 	other types in the chain.
> 
> It would have been useful if you had also provided a copy of the stabs
> for us to look at. I think I managed to generate them from the C file
> you provided:
> 
>         .stabs  "t:p(0,21)=*(0,22)=k(0,23)=xsdummy:",160,0,28,-24
Yes, this is what I also got. 
> Followed later on by the actual definition:
> 
>         .stabs
> "dummy:T(0,23)=s16x:(0,1),0,32;y:(0,1),32,32;b:(0,13),64,64;;",128,0,0,
> 0
> 
> What I don't get is that the xsdummy should lead to what we call an
> undefined type in stabsread.c, which should be put in the undef_types
> queue. 
  Yes, 'x' is handled line 1540,
and 's' stands for structure, and if followed by a name
that is not yet defined, it will lead to a call
to add_undefined_type.
> This queue is then processed at the end to fix all undefined
> types, including the various volatile/const variants in the type chain.

  But this is the trouble,
the chain was not cycled before my patch, 
and thus the 'const type' was never resolved and its length
was still left at zero.
  I didn't really get what the loop line 4465
is supposed to do, but it only operates on LOC_TYPEDEF,
and on the file_symbol level, not at argument list of functions...

The const or volatile modifier simply create
another type with the same main_type and is added to the chain.
So the chain seemed the easiest place for me to fix this.

> Any idea why this is not working this way for your example?
  Yes, see above. 
> > 2010-04-08  Pierre Muller  <muller@ics.u-strasbg.fr>
> > 	Test for bug 11479.
> > 	* gdb.stabs/gdb11479.exp: New file.
> > 	* gdb.stabs/gdb11479.c: New file.
> 
> Just a small cleanup request:
> 
> > +# Please email any bugs, comments, and/or additions to this file to:
> > +# bug-gdb@gnu.org

  I took just one other bug report and copied it...

Pierre

Re: [RFC] Fix bug report 11479

[Joel Brobecker]

Pierre,

>   But this is the trouble,
> the chain was not cycled before my patch, 
> and thus the 'const type' was never resolved and its length
> was still left at zero.
>   I didn't really get what the loop line 4465
> is supposed to do, but it only operates on LOC_TYPEDEF,
> and on the file_symbol level, not at argument list of functions...

I need to think about this for a little longer. I no longer understand
stabsread as much as I would like to :-(.

-- 
Joel

Re: [RFC] Fix bug report 11479

[Joel Brobecker]

> >   But this is the trouble,
> > the chain was not cycled before my patch, 
> > and thus the 'const type' was never resolved and its length
> > was still left at zero.
> >   I didn't really get what the loop line 4465
> > is supposed to do, but it only operates on LOC_TYPEDEF,
> > and on the file_symbol level, not at argument list of functions...

Pierre - I understand now what you were trying to say.  The problem in
this situation is that there are no global variable of the type that
we need to fix.

I think that the proper solution would be to enhance function
cleanup_undefined_types_1 to also look at symbols inside function
symbols.  That way, the problem should be fixed for all kinds of
types, not just structs...

The following should work: In the loop over file_symbols, check
the symbol type: If it is a function, then get the function block,
and iterate again on all symbols inside that block. If not a function,
then we match the symbol itself.  We will probably need to move the
symbol matching condition to its own function to avoid duplication...

-- 
Joel

RE: [RFC] Fix bug report 11479

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Joel Brobecker
> Envoyé : Wednesday, April 21, 2010 5:11 PM
> À : Pierre Muller
> Cc : gdb-patches@sourceware.org
> Objet : Re: [RFC] Fix bug report 11479
> 
> > >   But this is the trouble,
> > > the chain was not cycled before my patch,
> > > and thus the 'const type' was never resolved and its length
> > > was still left at zero.
> > >   I didn't really get what the loop line 4465
> > > is supposed to do, but it only operates on LOC_TYPEDEF,
> > > and on the file_symbol level, not at argument list of functions...
> 
> Pierre - I understand now what you were trying to say.  The problem in
> this situation is that there are no global variable of the type that
> we need to fix.
> 
> I think that the proper solution would be to enhance function
> cleanup_undefined_types_1 to also look at symbols inside function
> symbols.  That way, the problem should be fixed for all kinds of
> types, not just structs...

 I do not think that this is enough:
 the problem is more that TYPE_STUB macro refers
to the flag_stub of main_type, which means that
once the main_type flag_stub has been cleared,
all types having the same main_type will also return
zero for TYPE_STUB.
> The following should work: In the loop over file_symbols, check
> the symbol type: If it is a function, then get the function block,
> and iterate again on all symbols inside that block. If not a function,
> then we match the symbol itself.  We will probably need to move the
> symbol matching condition to its own function to avoid duplication...

  I still think that we should cycle over the type_chain
when the type is parsed.
  Otherwise we need your suggestion, plus a
check of for all types still having zero length.

But I still think that my patch is
already a good step. At least for 
the problem I presented in the bug report.

Pierre

Re: [RFC] Fix bug report 11479

[Joel Brobecker]

> > I think that the proper solution would be to enhance function
> > cleanup_undefined_types_1 to also look at symbols inside function
> > symbols.  That way, the problem should be fixed for all kinds of
> > types, not just structs...
> 
>  I do not think that this is enough: the problem is more that
>  TYPE_STUB macro refers to the flag_stub of main_type, which means
>  that once the main_type flag_stub has been cleared, all types having
>  the same main_type will also return zero for TYPE_STUB.

It's definitely something i got myself confused over. In the stabs I posted:

        .stabs  "t:p(0,21)=*(0,22)=k(0,23)=xsdummy:",160,0,28,-24
        .stabs "dummy:T(0,23)=s16x:(0,1),0,32;y:(0,1),32,32;b:(0,13),64,64;;",128,0,0,0

Type dummy is numbered (0,23), whereas I talked myself into thinking
that type (0,23) would the be constant version of type dummy.  With
that confusion cleared, we do indeed clear the stub condition on our
type at the time when we read the second stabs line above. So, by the
time we reach the cleanup_undefined_types_1 stage, the type is actually
no longer undefined.

>   I still think that we should cycle over the type_chain when the type
>   is parsed.

I am afraid this might be the only solution indeed. For some reason,
even after our pretty comprehensive investigating, I am still pretty
reluctant about it, and I think it is because it looks wrong to
perform this relatively unintuitive operation, and just do it for
structs. Conceivably, you'd need to do the same for enums as well,
right?

I think we can reduce my anxiety by:
  - Adding detailed comments explaining what and why;
  - Putting the code inside a function so that we can reuse it elsewhere
    should the need arise;

Do you think that this would be an acceptable approach?

Sorry if I am slow, but stabsread, and stabs in general, are two horrible
deprecated pieces of technology - so it's hard to remain current.

-- 
Joel

[RFA-v2] Fix bug report 11479

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Joel Brobecker
> Envoyé : Thursday, April 22, 2010 3:11 AM
> À : Pierre Muller
> Cc : gdb-patches@sourceware.org
> Objet : Re: [RFC] Fix bug report 11479
> 
> > > I think that the proper solution would be to enhance function
> > > cleanup_undefined_types_1 to also look at symbols inside function
> > > symbols.  That way, the problem should be fixed for all kinds of
> > > types, not just structs...
> >
> >  I do not think that this is enough: the problem is more that
> >  TYPE_STUB macro refers to the flag_stub of main_type, which means
> >  that once the main_type flag_stub has been cleared, all types having
> >  the same main_type will also return zero for TYPE_STUB.
> 
> It's definitely something i got myself confused over. In the stabs I
> posted:
> 
>         .stabs  "t:p(0,21)=*(0,22)=k(0,23)=xsdummy:",160,0,28,-24
>         .stabs
> "dummy:T(0,23)=s16x:(0,1),0,32;y:(0,1),32,32;b:(0,13),64,64;;",128,0,0,
> 0
> 
> Type dummy is numbered (0,23), whereas I talked myself into thinking
> that type (0,23) would the be constant version of type dummy.  With
> that confusion cleared, we do indeed clear the stub condition on our
> type at the time when we read the second stabs line above. So, by the
> time we reach the cleanup_undefined_types_1 stage, the type is actually
> no longer undefined.

  We agree here.
 
> >   I still think that we should cycle over the type_chain when the
> type
> >   is parsed.
> 
> I am afraid this might be the only solution indeed. For some reason,
> even after our pretty comprehensive investigating, I am still pretty
> reluctant about it, and I think it is because it looks wrong to
> perform this relatively unintuitive operation, and just do it for
> structs. Conceivably, you'd need to do the same for enums as well,
> right?

  You are perfectly right, and I only understood now
what you meant.
  The patch below does now handle enumerations as well,
 and the new testsuite exp file also checks it.
 The previous version indeed only handled struct and not enums ...
are there any other types that can be opaque?
 Unions are also handled inside read_struct_type
so this should be OK.

> I think we can reduce my anxiety by:
>   - Adding detailed comments explaining what and why;
>   - Putting the code inside a function so that we can reuse it
> elsewhere

  That is what I did.
 
> Do you think that this would be an acceptable approach?

  Yes, I hope this patch handles it as you 
wanted.
 
> Sorry if I am slow, but stabsread, and stabs in general, are two
> horrible
> deprecated pieces of technology - so it's hard to remain current.

  Thanks for the suggestions,

  Here is the new version,
tested on gcc16, one significant difference was
2 of the tests inside gdb.stabs/gdb11479.exp 
that FAIL without the patch and pass with the patch.
  
Is this OK?

Pierre Muller


2010-04-22  Pierre Muller  <muller@ics.u-strasbg.fr>

	PR stabs/11479.
	* stabsread.c (set_length_in_type_chain): New function.
	(read_struct_type): Call set_length_in_type_chain function.
	(read_enum_type): Idem.

Testsuite ChangeLog entry:

2010-04-22  Pierre Muller  <muller@ics.u-strasbg.fr>

	PR stabs/11479.
	* gdb.stabs/gdb11479.exp: New file.
	* gdb.stabs/gdb11479.c: New file.


Index: src/gdb/stabsread.c
===================================================================
RCS file: /cvs/src/src/gdb/stabsread.c,v
retrieving revision 1.124
diff -u -p -r1.124 stabsread.c
--- src/gdb/stabsread.c	5 Apr 2010 22:43:47 -0000	1.124
+++ src/gdb/stabsread.c	22 Apr 2010 07:29:06 -0000
@@ -3393,6 +3393,23 @@ complain_about_struct_wipeout (struct ty
 	     _("struct/union type gets multiply defined: %s%s"), kind,
name);
 }
 
+/* Set the length for all variants of a same main_type, which are
+   connected in the closed chain.  */
+
+static void
+set_length_in_type_chain (struct type * type)
+{
+  struct type *ntype = TYPE_CHAIN (type);
+
+  while (ntype != type)
+    {
+      if (TYPE_LENGTH(ntype) == 0)
+	TYPE_LENGTH (ntype) = TYPE_LENGTH (type);
+      else
+        complain_about_struct_wipeout (ntype);
+      ntype = TYPE_CHAIN (ntype);
+    }
+}
 
 /* Read the description of a structure (or union type) and return an object
    describing the type.
@@ -3451,6 +3468,7 @@ read_struct_type (char **pp, struct type
     TYPE_LENGTH (type) = read_huge_number (pp, 0, &nbits, 0);
     if (nbits != 0)
       return error_type (pp, objfile);
+    set_length_in_type_chain (type);
   }
 
   /* Now read the baseclasses, if any, read the regular C struct or C++
@@ -3615,6 +3633,7 @@ read_enum_type (char **pp, struct type *
   /* Now fill in the fields of the type-structure.  */
 
   TYPE_LENGTH (type) = gdbarch_int_bit (gdbarch) / HOST_CHAR_BIT;
+  set_length_in_type_chain (type);
   TYPE_CODE (type) = TYPE_CODE_ENUM;
   TYPE_STUB (type) = 0;
   if (unsigned_enum)
Index: src/gdb/testsuite/gdb.stabs/gdb11479.c
===================================================================
RCS file: testsuite/gdb.stabs/gdb11479.c
diff -N src/gdb/testsuite/gdb.stabs/gdb11479.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ src/gdb/testsuite/gdb.stabs/gdb11479.c	22 Apr 2010 07:42:19 -0000
@@ -0,0 +1,69 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010 Free Software Foundation, Inc.
+
+   Contributed by Pierre Muller.
+
+   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/>.
+
+   Please email any bugs, comments, and/or additions to this file to:
+   bug-gdb@gnu.org  */
+
+/* Qualifiers of forward types are not resolved correctly with stabs.  */
+
+struct dummy;
+
+enum dummy_enum;
+
+const void *
+test (const struct dummy *t)
+{
+  const struct dummy *tt;
+  enum dummy_enum *e;
+  tt = t;
+  return t;
+}
+
+void *
+test2 (struct dummy *t)
+{
+  struct dummy *tt;
+  const enum dummy_enum *e;
+  tt = t;
+  return t;
+}
+
+
+struct dummy {
+ int x;
+ int y;
+ double b;
+} tag_dummy;
+
+enum dummy_enum {
+  enum1,
+  enum2
+};
+
+int
+main ()
+{
+  struct dummy tt;
+  tt.x = 5;
+  tt.y = 25;
+  tt.b = 2.5;
+  test2 (&tt);
+  test (&tt);
+  return 0;
+}
Index: src/gdb/testsuite/gdb.stabs/gdb11479.exp
===================================================================
RCS file: testsuite/gdb.stabs/gdb11479.exp
diff -N src/gdb/testsuite/gdb.stabs/gdb11479.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ src/gdb/testsuite/gdb.stabs/gdb11479.exp	22 Apr 2010 09:49:24 -0000
@@ -0,0 +1,59 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2010 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/>.
+
+# Please email any bugs, comments, and/or additions to this file to:
+# bug-gdb@gnu.org
+
+# Test GDB stabs problem with qualified parameter of forward types.
+
+if $tracelevel then {
+	strace $tracelevel
+}
+
+set prms_id 0
+set bug_id 0
+
+set testfile "gdb11479"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable
{debug additional_flags=-gstabs}] != "" } {
+    if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}"
executable {debug}] != "" } {
+	untested "couldn't compile ${srcdir}/${subdir}/${srcfile}"
+	return -1
+    }
+}
+
+# Start with a fresh gdb.
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+# Regression test for a cleanup bug in the charset code.
+gdb_test "rb test" "" "Set breakpoints"
+gdb_test "run" "Breakpoint .* test2 .*" "Stop at first breakpoint"
+# Check that the struct is read in correctly
+gdb_test "print *t" ".*\{x = 5, y = 25, b = 2.5\}.*" "Inspect t in test2"
+# Check that the enum type length has been set to a non-zero value
+gdb_test "print sizeof (*e)" "= \[1-9\]*" "sizeof (e) in test2"
+gdb_test "continue" "Breakpoint .* test .*" "Stop at first breakpoint"
+gdb_test "print *t" ".*\{x = 5, y = 25, b = 2.5\}.*" "Inspect t in test"
+# Check that the enum type length has been set to a non-zero value
+gdb_test "print sizeof (*e)" "= \[1-9\]*" "sizeof (e) in test"
+
+gdb_exit
+

Re: [RFA-v2] Fix bug report 11479

[Joel Brobecker]

> 2010-04-22  Pierre Muller  <muller@ics.u-strasbg.fr>
> 
> 	PR stabs/11479.
> 	* stabsread.c (set_length_in_type_chain): New function.
> 	(read_struct_type): Call set_length_in_type_chain function.
> 	(read_enum_type): Idem.

The code portion is pre-approved with the comments below. Just go ahead
and check it in.

> 2010-04-22  Pierre Muller  <muller@ics.u-strasbg.fr>
> 
> 	PR stabs/11479.
> 	* gdb.stabs/gdb11479.exp: New file.
> 	* gdb.stabs/gdb11479.c: New file.

This portion can be checked in separately after we answer the few questions
I have (see below).

> +/* Set the length for all variants of a same main_type, which are
> +   connected in the closed chain.  */

We're missing the "why" this is useful. How about the following?

/* Set the length for all variants of a same main_type, which are
   connected in the closed chain.
   
   This is something that needs to be done when a type is defined *after*
   some cross references to this type have already been read.  Consider
   for instance the following scenario where we have the following two
   stabs entries:

        .stabs  "t:p(0,21)=*(0,22)=k(0,23)=xsdummy:",160,0,28,-24
        .stabs  "dummy:T(0,23)=s16x:(0,1),0,3[...]"

   A stubbed version of type dummy is created while processing the first
   stabs entry.  The length of that type is initially set to zero, since
   it is unknown at this point.  Also, a "constant" variation of type
   "dummy" is created as well (this is the "(0,22)=k(0,23)" section of
   the stabs line).

   The second stabs entry allows us to replace the stubbed definition
   with the real definition.  However, we still need to adjust the length
   of the "constant" variation of that type, as its length was left
   untouched during the main type replacement...  */

> +static void
> +set_length_in_type_chain (struct type * type)
                                         ^^  No space after the *

> +   Please email any bugs, comments, and/or additions to this file to:
> +   bug-gdb@gnu.org  */

Can you remove this bit?

> +# Please email any bugs, comments, and/or additions to this file to:
> +# bug-gdb@gnu.org

Same for this one...

> +set testfile "gdb11479"
> +set srcfile ${testfile}.c
> +set binfile ${objdir}/${subdir}/${testfile}
> +if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable
> {debug additional_flags=-gstabs}] != "" } {
> +    if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}"
> executable {debug}] != "" } {
> +	untested "couldn't compile ${srcdir}/${subdir}/${srcfile}"
> +	return -1
> +    }
> +}
> +
> +# Start with a fresh gdb.
> +gdb_exit
> +gdb_start
> +gdb_reinitialize_dir $srcdir/$subdir
> +gdb_load ${binfile}

Can you actually use "prepare_for_testing" here? The only thing that
is a bit out of the ordinary is request the -gstabs debug flag.  I am
actually wondering if we really want that, since this may not work on
certain platforms while we might still want to use the test with the
default debug format.  On the other hand, you probably want to make sure
that your test gets run with -gstabs on Windows, so we have two
incompatible objectives...

How's about we do 2 builds? One with standard debug, and another with
-gstabs? We can repeat the testing by putting the gdb_test calls inside
a proc and then call it twice? I think that prepare_for_testing should
be able to handle the case where you need to add additional_flags as
well. In fact:

    gdb.base/commands.exp:if { [prepare_for_testing commands.exp commands run.c {debug additional_flags=-DFAKEARGV}] } {

> +# Regression test for a cleanup bug in the charset code.

?

> +gdb_exit

Harmless, but unnecessary. Let's toss it.

-- 
Joel

RE: [RFA-v2] Fix bug report 11479

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Joel Brobecker
> Envoyé : Thursday, April 22, 2010 2:20 PM
> À : Pierre Muller
> Cc : gdb-patches@sourceware.org
> Objet : Re: [RFA-v2] Fix bug report 11479
> 
> > 2010-04-22  Pierre Muller  <muller@ics.u-strasbg.fr>
> >
> > 	PR stabs/11479.
> > 	* stabsread.c (set_length_in_type_chain): New function.
> > 	(read_struct_type): Call set_length_in_type_chain function.
> > 	(read_enum_type): Idem.
> 
> The code portion is pre-approved with the comments below. Just go ahead
> and check it in.

  Thanks, I did commit this part with your
detailed comment.
 
> > 2010-04-22  Pierre Muller  <muller@ics.u-strasbg.fr>
> >
> > 	PR stabs/11479.
> > 	* gdb.stabs/gdb11479.exp: New file.
> > 	* gdb.stabs/gdb11479.c: New file.
> 
> This portion can be checked in separately after we answer the few
> questions
> I have (see below).
> > +   Please email any bugs, comments, and/or additions to this file
> to:
> > +   bug-gdb@gnu.org  */
> 
> Can you remove this bit?
Done. 
> > +# Please email any bugs, comments, and/or additions to this file to:
> > +# bug-gdb@gnu.org
> 
> Same for this one...
OK. 
> > +set testfile "gdb11479"
> > +set srcfile ${testfile}.c
> > +set binfile ${objdir}/${subdir}/${testfile}
> > +if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}"
> executable
> > {debug additional_flags=-gstabs}] != "" } {
> > +    if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}"
> > executable {debug}] != "" } {
> > +	untested "couldn't compile ${srcdir}/${subdir}/${srcfile}"
> > +	return -1
> > +    }
> > +}
> > +
> > +# Start with a fresh gdb.
> > +gdb_exit
> > +gdb_start
> > +gdb_reinitialize_dir $srcdir/$subdir
> > +gdb_load ${binfile}
> 
> Can you actually use "prepare_for_testing" here? The only thing that
> is a bit out of the ordinary is request the -gstabs debug flag.  I am
> actually wondering if we really want that, since this may not work on
> certain platforms while we might still want to use the test with the
> default debug format.  
  This is a stabs specific bug,
when I first tried to test it on gcc16 from the compile farm,
I got no difference in the output.
  This was due to the fact that gcc16 is a 64-bit linux machine
and as such uses dwarf debug format by default.
  So I thought, why not try to force using stabs debugging,
and fall back to normal compilation if this fails.

>On the other hand, you probably want to make sure
> that your test gets run with -gstabs on Windows, so we have two
> incompatible objectives...
> 
> How's about we do 2 builds? One with standard debug, and another with
> -gstabs? We can repeat the testing by putting the gdb_test calls inside
> a proc and then call it twice? I think that prepare_for_testing should
> be able to handle the case where you need to add additional_flags as
> well. In fact:
> 
>     gdb.base/commands.exp:if { [prepare_for_testing commands.exp
> commands run.c {debug additional_flags=-DFAKEARGV}] } {
> 
> > +# Regression test for a cleanup bug in the charset code.
> 
> ?
  You probably guessed now which file I used to
create those new ones...
Removed. 
> > +gdb_exit
> 
> Harmless, but unnecessary. Let's toss it.
Removed at the end, but reintroduced below...

 I am not convinced that a second round using
default compiler debug settings is really useful,
unless the same bug reappears once in another debug format,
which is pretty unlikely.

 But I still wrote a new version with two builds.

Is this OK for you?

Pierre

Testsuite ChangeLog entry:

2010-04-22  Pierre Muller  <muller@ics.u-strasbg.fr>

	PR stabs/11479.
	* gdb.stabs/gdb11479.exp: New file.
	* gdb.stabs/gdb11479.c: New file.

Index: testsuite/gdb.stabs/gdb11479.c
===================================================================
RCS file: testsuite/gdb.stabs/gdb11479.c
diff -N testsuite/gdb.stabs/gdb11479.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.stabs/gdb11479.c	22 Apr 2010 13:02:08 -0000
@@ -0,0 +1,66 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010 Free Software Foundation, Inc.
+
+   Contributed by Pierre Muller.
+
+   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/>.
+
+   Qualifiers of forward types are not resolved correctly with stabs.  */
+
+struct dummy;
+
+enum dummy_enum;
+
+const void *
+test (const struct dummy *t)
+{
+  const struct dummy *tt;
+  enum dummy_enum *e;
+  tt = t;
+  return t;
+}
+
+void *
+test2 (struct dummy *t)
+{
+  struct dummy *tt;
+  const enum dummy_enum *e;
+  tt = t;
+  return t;
+}
+
+
+struct dummy {
+ int x;
+ int y;
+ double b;
+} tag_dummy;
+
+enum dummy_enum {
+  enum1,
+  enum2
+};
+
+int
+main ()
+{
+  struct dummy tt;
+  tt.x = 5;
+  tt.y = 25;
+  tt.b = 2.5;
+  test2 (&tt);
+  test (&tt);
+  return 0;
+}
Index: testsuite/gdb.stabs/gdb11479.exp
===================================================================
RCS file: testsuite/gdb.stabs/gdb11479.exp
diff -N testsuite/gdb.stabs/gdb11479.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.stabs/gdb11479.exp	22 Apr 2010 13:02:08 -0000
@@ -0,0 +1,56 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2010 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/>.
+
+# Test GDB stabs problem with qualified parameter of forward types.
+
+if $tracelevel then {
+	strace $tracelevel
+}
+
+set prms_id 0
+set bug_id 0
+
+set testfile "gdb11479"
+
+proc do_test {version} {
+    gdb_test "rb test" "" "Set breakpoints $version"
+    gdb_test "run" "Breakpoint .* test2 .*" "Stop at first breakpoint
$version"
+    # Check that the struct is read in correctly
+    gdb_test "print *t" ".*\{x = 5, y = 25, b = 2.5\}.*" \
+	"Inspect t in test2 $version"
+    # Check that the enum type length has been set to a non-zero value
+    gdb_test "print sizeof (*e)" "= \[1-9\]*" "sizeof (e) in test2
$version"
+    gdb_test "continue" "Breakpoint .* test .*" \
+	"Stop at first breakpoint $version"
+    gdb_test "print *t" ".*\{x = 5, y = 25, b = 2.5\}.*" \
+	"Inspect t in test $version"
+    # Check that the enum type length has been set to a non-zero value
+    gdb_test "print sizeof (*e)" "= \[1-9\]*" "sizeof (e) in test $version"
+}
+
+if { [prepare_for_testing $testfile.exp $testfile $testfile.c {debug
additional_flags=-gstabs}] == 0 } {
+    do_test forced_stabs
+}
+
+# Without this gdb_exit the executable is still opened
+# by GDB which can generate a compilation failure. 
+gdb_exit
+
+if { [prepare_for_testing $testfile.exp $testfile $testfile.c {debug}] == 0
} {
+    do_test natural_debug_format
+}
+

Re: [RFA-v2] Fix bug report 11479

[Joel Brobecker]

> > > +gdb_exit
> > 
> > Harmless, but unnecessary. Let's toss it.
> Removed at the end, but reintroduced below...

Hmmm, yes, that's right - on Windows, the filesystem locks the files
being opened.

> I am not convinced that a second round using default compiler debug
> settings is really useful, unless the same bug reappears once in
> another debug format, which is pretty unlikely.

Probably a fair comment, but since stabs is pretty much doomed, this
allows us to reuse the test even after stabs support gets removed
(or on platforms where stabs is not supported).

> 2010-04-22  Pierre Muller  <muller@ics.u-strasbg.fr>
> 
> 	PR stabs/11479.
> 	* gdb.stabs/gdb11479.exp: New file.
> 	* gdb.stabs/gdb11479.c: New file.

This is OK.

Thanks,
-- 
Joel

RE: [RFA-v2] Fix bug report 11479

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Joel Brobecker
> Envoyé : Thursday, April 22, 2010 3:27 PM
> À : Pierre Muller
> Cc : gdb-patches@sourceware.org
> Objet : Re: [RFA-v2] Fix bug report 11479
> 
> > > > +gdb_exit
> > >
> > > Harmless, but unnecessary. Let's toss it.
> > Removed at the end, but reintroduced below...
> 
> Hmmm, yes, that's right - on Windows, the filesystem locks the files
> being opened.

  I think that there still is a place inside the testsuite where
the same problem appears...
 
> > I am not convinced that a second round using default compiler debug
> > settings is really useful, unless the same bug reappears once in
> > another debug format, which is pretty unlikely.
> 
> Probably a fair comment, but since stabs is pretty much doomed, this
> allows us to reuse the test even after stabs support gets removed
> (or on platforms where stabs is not supported).
> 
> > 2010-04-22  Pierre Muller  <muller@ics.u-strasbg.fr>
> >
> > 	PR stabs/11479.
> > 	* gdb.stabs/gdb11479.exp: New file.
> > 	* gdb.stabs/gdb11479.c: New file.
> 
> This is OK.

  Thanks for the review,
committed.

Pierre