Re: [RFC] Fix bug report 11479

Re: [RFC] Fix bug report 11479

[Tom Tromey]

>>>>> "Pierre" == Pierre Muller <pierre.muller@ics-cnrs.unistra.fr> writes:

Pierre>  1) I don't remember how the ChangeLog should reflect the fact
Pierre> that the commit is related to this bug.

I use "PR category/nnn", but I think the commit script actually
recognizes a few formats.  E.g.:

2010-04-05  Sergio Durigan Junior  <sergiodj@redhat.com>

	PR gdb/10736:
	* xml-syscall.c (my_gdb_datadir): New variable to keep track of
        [...]

Tom

[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