[RFA] PR 11530: Fix and test case

[RFA] PR 11530: Fix and test case

[Pierre Muller]

 This is a problem reported by Jan Kratochvil
http://sourceware.org/bugzilla/show_bug.cgi?id=11530

  After some debugging, I found this fix,
and wrote a test case.
  Tested on gcc16 2 FAIL within gdb11530.exp
with pristine disappear with patched versions.
  No other change (apart from the tests
that are unstable, like:
ext-run.exp (I often get an 'internal buffer is full' error
for that test)
attach-into-signal.exp
and watchthreads2.exp

Is this patch OK?

Pierre Muller
Pascal language support maintainer for GDB

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

	PR exp/11530.
	* gdbtypes.c (lookup_struct_elt_type): Also lookup
	names of unnamed structures or unions.

testsuite ChangeLog entry:

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

	PR exp/11530.
	* testsuite/gdb.base/gdb11530.c: New file.
	* testsuite/gdb.base/gdb11530.exp: New file.


Index: src/gdb/gdbtypes.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbtypes.c,v
retrieving revision 1.189
diff -u -p -r1.189 gdbtypes.c
--- src/gdb/gdbtypes.c	21 Apr 2010 23:21:03 -0000	1.189
+++ src/gdb/gdbtypes.c	29 Apr 2010 21:23:35 -0000
@@ -1246,6 +1246,13 @@ lookup_struct_elt_type (struct type *typ
 	{
 	  return TYPE_FIELD_TYPE (type, i);
 	}
+      else if (!t_field_name || *t_field_name == '\0')
+	{
+	  struct type *subtype = lookup_struct_elt_type (
+				   TYPE_FIELD_TYPE (type, i), name, 1);
+	  if (subtype != NULL)
+	    return subtype;
+	}
     }
 
   /* OK, it's not in this class.  Recursively check the baseclasses.  */
Index: src/gdb/testsuite/gdb.base/gdb11530.c
===================================================================
RCS file: testsuite/gdb.base/gdb11530.c
diff -N testsuite/gdb.base/gdb11530.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ src/gdb/testsuite/gdb.base/gdb11530.c	29 Apr 2010 21:23:37 -0000
@@ -0,0 +1,35 @@
+/* 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/>.
+
+   Test for problem related to unnamed unions.  */
+
+struct a
+  {
+    union
+      {
+        int i;
+      };
+  } a;
+
+int
+main (void)
+{
+  return sizeof (a.i);
+}
+
Index: src/gdb/testsuite/gdb.base/gdb11530.exp
===================================================================
RCS file: testsuite/gdb.base/gdb11530.exp
diff -N testsuite/gdb.base/gdb11530.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ src/gdb/testsuite/gdb.base/gdb11530.exp	29 Apr 2010 21:23:37 -0000
@@ -0,0 +1,44 @@
+# 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 bug report 11531.
+# This is a problem related to CANNOT_STEP_HW_WATCHPOINTS macro.
+# It affects Solaris native targets.
+
+if $tracelevel then {
+	strace $tracelevel
+}
+
+set prms_id 0
+set bug_id 0
+
+set testfile "gdb11530"
+
+if { [prepare_for_testing $testfile.exp $testfile $testfile.c {debug}] } {
+    return -1;
+}
+
+
+if { ![runto main] } then {
+    fail "run to main"
+    return
+}
+
+gdb_test "print a.i" " = 0"
+gdb_test "print sizeof (a.i)" " = \[0-9\]+"
+gdb_test "print sizeof (a.i) == sizeof (int)" " = 1"
+

Re: [RFA] PR 11530: Fix and test case

[Jan Kratochvil]

On Thu, 29 Apr 2010 23:50:15 +0200, Pierre Muller wrote:
> --- src/gdb/gdbtypes.c	21 Apr 2010 23:21:03 -0000	1.189
> +++ src/gdb/gdbtypes.c	29 Apr 2010 21:23:35 -0000
> @@ -1246,6 +1246,13 @@ lookup_struct_elt_type (struct type *typ
>  	{
>  	  return TYPE_FIELD_TYPE (type, i);
>  	}
> +      else if (!t_field_name || *t_field_name == '\0')
> +	{
> +	  struct type *subtype = lookup_struct_elt_type (
> +				   TYPE_FIELD_TYPE (type, i), name, 1);

IMO
	  struct type *subtype;

	  subtype = lookup_struct_elt_type TYPE_FIELD_TYPE (type, i), name, 1);

> +	  if (subtype != NULL)
> +	    return subtype;
> +	}
>      }


I was now thinking about a possible name clash.  As these anonymous
structs/unions are a GNU extension there is no offical standard for it but the
GCC texinfo file describes it as an error which GCC currently does not report.
I have not found a GCC PR for it.

------------------------------------------------------------------------------
(gcc)Unnamed Fields
 You must never create such structures that cause ambiguous field
definitions.  For example, this structure:

     struct {
       int a;
       struct {
         int a;
       };
     } foo;

 It is ambiguous which `a' is being referred to with `foo.a'.  Such
constructs are not supported and must be avoided.  In the future, such
constructs may be detected and treated as compilation errors.
------------------------------------------------------------------------------


> +struct a
> +  {
> +    union
> +      {
> +        int i;
> +      };
> +  } a;

Possibly to test also `struct' there to get better test coverage.


> +if { ![runto main] } then {
          runto_main

But I do not understand how those embedded remote stubs work.



Thanks,
Jan

Re: [RFA] PR 11530: Fix and test case

[Joseph S. Myers]

On Fri, 30 Apr 2010, Jan Kratochvil wrote:

> I was now thinking about a possible name clash.  As these anonymous
> structs/unions are a GNU extension there is no offical standard for it but the
> GCC texinfo file describes it as an error which GCC currently does not report.
> I have not found a GCC PR for it.

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=4784

I expect this (and 10676) to be fixed as part of implementing the C1X 
version of anonymous structures and unions.

-- 
Joseph S. Myers
joseph@codesourcery.com

RE: [RFA] PR 11530: Fix and test case

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Jan Kratochvil
> Envoyé : Friday, April 30, 2010 10:20 AM
> À : Pierre Muller
> Cc : gdb-patches@sourceware.org
> Objet : Re: [RFA] PR 11530: Fix and test case
> 
> On Thu, 29 Apr 2010 23:50:15 +0200, Pierre Muller wrote:
> > --- src/gdb/gdbtypes.c	21 Apr 2010 23:21:03 -0000	1.189
> > +++ src/gdb/gdbtypes.c	29 Apr 2010 21:23:35 -0000
> > @@ -1246,6 +1246,13 @@ lookup_struct_elt_type (struct type *typ
> >  	{
> >  	  return TYPE_FIELD_TYPE (type, i);
> >  	}
> > +      else if (!t_field_name || *t_field_name == '\0')
> > +	{
> > +	  struct type *subtype = lookup_struct_elt_type (
> > +				   TYPE_FIELD_TYPE (type, i), name, 1);
> 
> IMO
> 	  struct type *subtype;
> 
> 	  subtype = lookup_struct_elt_type TYPE_FIELD_TYPE (type, i),
> name, 1);

  This looks better, indeed.
 
> > +	  if (subtype != NULL)
> > +	    return subtype;
> > +	}
> >      }
> 
> 
> I was now thinking about a possible name clash.  As these anonymous
> structs/unions are a GNU extension there is no offical standard for it
> but the
> GCC texinfo file describes it as an error which GCC currently does not
> report.
> I have not found a GCC PR for it.

  I don't want to try to test those GCC bugs
I just want to fix the simple case in which there is
no, name conflict.

  With my patch, GDB will find the first instance
that matches the field name, but I don't even know 
if the ordering given by the debug information necessarily 
follows the definition order in the source!

  Anyhow, this patch is only useful for sizeof (),
not for printing of the value itself, which
was already working...


Pierre Muller
Pascal language support maintainer for GDB

Re: [RFA] PR 11530: Fix and test case

[Tom Tromey]

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

Pierre> I just want to fix the simple case in which there is
Pierre> no, name conflict.

It seems pretty reasonable to me.

Pierre>   With my patch, GDB will find the first instance
Pierre> that matches the field name, but I don't even know 
Pierre> if the ordering given by the debug information necessarily 
Pierre> follows the definition order in the source!

I skimmed this part of the DWARF 4 spec (5.5.6 Data Member Entries) but
I didn't see any such requirement.

I'm not too concerned about this.

Pierre>   Anyhow, this patch is only useful for sizeof (),
Pierre> not for printing of the value itself, which
Pierre> was already working...

That is interesting.  Why does it work in one case but not the other?

Tom

RE: [RFA] PR 11530: Fix and test case

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Tom Tromey
> Envoyé : Friday, April 30, 2010 7:30 PM
> À : Pierre Muller
> Cc : 'Jan Kratochvil'; gdb-patches@sourceware.org
> Objet : Re: [RFA] PR 11530: Fix and test case
> 
> >>>>> "Pierre" == Pierre Muller <pierre.muller@ics-cnrs.unistra.fr>
> writes:
> 
> Pierre> I just want to fix the simple case in which there is
> Pierre> no, name conflict.
> 
> It seems pretty reasonable to me.
> 
> Pierre>   With my patch, GDB will find the first instance
> Pierre> that matches the field name, but I don't even know
> Pierre> if the ordering given by the debug information necessarily
> Pierre> follows the definition order in the source!
> 
> I skimmed this part of the DWARF 4 spec (5.5.6 Data Member Entries) but
> I didn't see any such requirement.
> 
> I'm not too concerned about this.
> 
> Pierre>   Anyhow, this patch is only useful for sizeof (),
> Pierre> not for printing of the value itself, which
> Pierre> was already working...
> 
> That is interesting.  Why does it work in one case but not the other?

  for the first test 'p a.i'
gdb calls search_struct_field,
which has already some code (around line 1842)
to cope for unnamed fields.
  There was no equivalent in the lookup_struct_elt_type
function.

  By the way, the search_struct_field function
also allows a union called "else" to be searched.
Should this also be inserted into lookup_struct_elt_type
or is this only a left over from Chill language?

Pierre

Re: [RFA] PR 11530: Fix and test case

[Tom Tromey]

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

%om> That is interesting.  Why does it work in one case but not the other?

Pierre>   for the first test 'p a.i'
Pierre> gdb calls search_struct_field,
Pierre> which has already some code (around line 1842)
Pierre> to cope for unnamed fields.
Pierre>   There was no equivalent in the lookup_struct_elt_type
Pierre> function.

Thanks.

Pierre>   By the way, the search_struct_field function
Pierre> also allows a union called "else" to be searched.
Pierre> Should this also be inserted into lookup_struct_elt_type
Pierre> or is this only a left over from Chill language?

I was not even aware of this case.  I have no idea what it is for.
I suppose that's the danger of putting language-specific code into a
generic function...

Tom

Re: [RFA] PR 11530: Fix and test case

[Tom Tromey]

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

Pierre> If the compiler is not gcc we should skip the test,
Pierre> is that what you want?

Pierre> if {![test_compiler_info gcc-*]} {
Pierre> 	return 0
Pierre> }

Pierre> Would this fit?

Yes, thanks.

Tom

RE: [RFA] PR 11530: Fix and test case

[Pierre Muller]

> Pierre> +struct a
> Pierre> +  {
> Pierre> +    union
> Pierre> +      {
> Pierre> +        int i;
> Pierre> +      };
> Pierre> +  } a;
> 
> Since this is a C extension, I think that the test cases should
> probably
> be dependent on using GCC.

  I have no idea how to do this....

After lokking into several testsuite exp files:

If the compiler is not gcc we should skip the test,
is that what you want?


if {![test_compiler_info gcc-*]} {
	return 0
}

Would this fit?

Pierre

Re: [RFA] PR 11530: Fix and test case

[Tom Tromey]

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

Pierre>  This is a problem reported by Jan Kratochvil
Pierre> http://sourceware.org/bugzilla/show_bug.cgi?id=11530

Thanks for working on this.

Pierre> +struct a
Pierre> +  {
Pierre> +    union
Pierre> +      {
Pierre> +        int i;
Pierre> +      };
Pierre> +  } a;

Since this is a C extension, I think that the test cases should probably
be dependent on using GCC.

Tom