[RFA] "Sort" C++ fieldlists

[RFA] "Sort" C++ fieldlists

[Keith Seitz]

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

Hi,

My method overload resolution patch was committed with the following 
comment (from value_struct_elt_for_reference):

   /* This assumes, of course, that all artificial methods appear
      BEFORE any concrete methods.  */

Turns out that isn't a really great assumption. In particular, it fails 
on GCC 3.4.6. So I'm requesting that the attached patch be approved. The 
patch inserts artificial methods into the type's fieldlist starting at 
index 0 and concrete methods starting at the end index.

This shows no regressions (and no advantages) using GCC on linux. 
However, if you run cpexprs.exp, this patch will fix 17 failures with 
GCC 3.4.6. At the worst, consider this a defensive patch against 
breaking the value_struct_elt_for_reference assumption.

Keith

ChangeLog
2009-12-03  Keith Seitz  <keiths@redhat.com>

	* dwarf2read.c (dwarf2_attach_fn_fields_to_type): Ensure
	that artificial methods are inserted into the fn fieldlist
	before concrete methods.
	* valops.c (value_struct_elt_for_reference): Update comments
	concerning above assumption.

[-- Attachment #2: sort-methods.patch --]
[-- Type: text/plain, Size: 1935 bytes --]

Index: dwarf2read.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2read.c,v
retrieving revision 1.342
diff -u -p -r1.342 dwarf2read.c
--- dwarf2read.c	2 Dec 2009 11:44:35 -0000	1.342
+++ dwarf2read.c	4 Dec 2009 01:55:11 -0000
@@ -4845,14 +4845,24 @@ dwarf2_attach_fn_fields_to_type (struct 
     {
       struct nextfnfield *nfp = flp->head;
       struct fn_fieldlist *fn_flp = &TYPE_FN_FIELDLIST (type, i);
-      int k;
+      int k, start, end;
 
       TYPE_FN_FIELDLIST_NAME (type, i) = flp->name;
       TYPE_FN_FIELDLIST_LENGTH (type, i) = flp->length;
       fn_flp->fn_fields = (struct fn_field *)
 	TYPE_ALLOC (type, sizeof (struct fn_field) * flp->length);
+
+      /* To ease symbol searching, we must require that all artificial
+	 methods appear before any concrete methods in the fieldlist.
+	 See value_struct_elt_for_reference for where this requirement
+	 is needed/used.  */
+      start = 0;
+      end = flp->length - 1;
       for (k = flp->length; (k--, nfp); nfp = nfp->next)
-	fn_flp->fn_fields[k] = nfp->fnfield;
+	{
+	  int idx = (nfp->fnfield.is_artificial) ? start++ : end--;
+	  fn_flp->fn_fields[idx] = nfp->fnfield;
+	}
 
       total_length += flp->length;
     }
Index: valops.c
===================================================================
RCS file: /cvs/src/src/gdb/valops.c,v
retrieving revision 1.228
diff -u -p -r1.228 valops.c
--- valops.c	2 Dec 2009 19:29:42 -0000	1.228
+++ valops.c	4 Dec 2009 01:55:12 -0000
@@ -2721,7 +2721,8 @@ value_struct_elt_for_reference (struct t
 		error (_("non-unique member `%s' requires type instantiation"), name);
 
 	      /* This assumes, of course, that all artificial methods appear
-		 BEFORE any concrete methods.  */
+		 BEFORE any concrete methods.  This requirement is enforced
+		 for DWARF by dwarf2_attach_fn_fields_to_type.  */
 	      j = TYPE_FN_FIELDLIST_LENGTH (t, i) - 1;
 	    }
 

Re: [RFA] "Sort" C++ fieldlists

[Daniel Jacobowitz]

On Thu, Dec 03, 2009 at 06:21:05PM -0800, Keith Seitz wrote:
> Hi,
> 
> My method overload resolution patch was committed with the following
> comment (from value_struct_elt_for_reference):
> 
>   /* This assumes, of course, that all artificial methods appear
>      BEFORE any concrete methods.  */
> 
> Turns out that isn't a really great assumption. In particular, it
> fails on GCC 3.4.6. So I'm requesting that the attached patch be
> approved. The patch inserts artificial methods into the type's
> fieldlist starting at index 0 and concrete methods starting at the
> end index.
> 
> This shows no regressions (and no advantages) using GCC on linux.
> However, if you run cpexprs.exp, this patch will fix 17 failures with
> GCC 3.4.6. At the worst, consider this a defensive patch against
> breaking the value_struct_elt_for_reference assumption.

I guess this is OK... it seems nicer not to impose the restriction.
Is it this easy?

              int ii;
              j = -1;
              for (ii = 0; ii < TYPE_FN_FIELDLIST_LENGTH (t, i);
                   ++ii)
                {
		  /* Skip artificial methods.  This is necessary if, for example,
		     the user wants to "print subclass::subclass" with only
		     one user-defined constructor.  There is no ambiguity in this
		     case.  */
                  if (TYPE_FN_FIELD_ARTIFICIAL (f, ii))
                    continue;

		  /* Desired method is ambiguous if more than one method is
		     defined.  */
		  if (j != -1)
		    error (_("non-unique member `%s' requires type instantiation"), name);

		  j = ii;
                }

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] "Sort" C++ fieldlists

[Keith Seitz]

On 12/04/2009 05:51 AM, Daniel Jacobowitz wrote:
> I guess this is OK... it seems nicer not to impose the restriction.
> Is it this easy?

Wow, talk about trees!

Yes, actually it *is* that simple. Or at least none of my testing can 
prove that your (much) simpler patch is any less effective than my more 
complicated one.

Consider my patch withdrawn!

Keith

Re: [RFA] "Sort" C++ fieldlists

[Daniel Jacobowitz]

On Fri, Dec 04, 2009 at 09:31:49AM -0800, Keith Seitz wrote:
> Yes, actually it *is* that simple. Or at least none of my testing can
> prove that your (much) simpler patch is any less effective than my
> more complicated one.

I've tested and committed this, then.

-- 
Daniel Jacobowitz
CodeSourcery

2009-12-04  Daniel Jacobowitz  <dan@codesourcery.com>

	* valops.c (value_struct_elt_for_reference): Do not rely on
	field order.

Index: valops.c
===================================================================
RCS file: /cvs/src/src/gdb/valops.c,v
retrieving revision 1.228
diff -u -p -r1.228 valops.c
--- valops.c	2 Dec 2009 19:29:42 -0000	1.228
+++ valops.c	4 Dec 2009 20:25:44 -0000
@@ -2700,29 +2700,31 @@ value_struct_elt_for_reference (struct t
 		}
 
 	      if (j == len)
-		error (_("no member function matches that type instantiation"));	    }
+		error (_("no member function matches that type instantiation"));
+	    }
 	  else
 	    {
 	      int ii;
-	      /* Skip artificial methods.  This is necessary if, for example,
-		 the user wants to "print subclass::subclass" with only
-		 one user-defined constructor.  There is no ambiguity in this
-		 case.  */
+
+	      j = -1;
 	      for (ii = 0; ii < TYPE_FN_FIELDLIST_LENGTH (t, i);
 		   ++ii)
 		{
+		  /* Skip artificial methods.  This is necessary if,
+		     for example, the user wants to "print
+		     subclass::subclass" with only one user-defined
+		     constructor.  There is no ambiguity in this
+		     case.  */
 		  if (TYPE_FN_FIELD_ARTIFICIAL (f, ii))
-		    --len;
-		}
+		    continue;
 
-	      /* Desired method is ambiguous if more than one method is
-		 defined.  */
-	      if (len > 1)
-		error (_("non-unique member `%s' requires type instantiation"), name);
-
-	      /* This assumes, of course, that all artificial methods appear
-		 BEFORE any concrete methods.  */
-	      j = TYPE_FN_FIELDLIST_LENGTH (t, i) - 1;
+		  /* Desired method is ambiguous if more than one
+		     method is defined.  */
+		  if (j != -1)
+		    error (_("non-unique member `%s' requires type instantiation"), name);
+
+		  j = ii;
+		}
 	    }
 
 	  if (TYPE_FN_FIELD_STATIC_P (f, j))