Re: [rfa] teach linespec about nested classes

Re: [rfa] teach linespec about nested classes

[Elena Zannoni]

[initially sent to gdb@ rather than gdb-patches@]

David Carlton writes:
 > Here's the last of my namespace/nested class patches.  It modifies
 > decode_compound in linespec.c to change the order in which it does
 > stuff.  Specifically, what it tries now is: given a name A::B::C,
 > it says:
 > 
 > * Is A a class?  If so, look for a method B in it.
 > 
 > * If A isn't a class, is A::B a class?  If so, look for a method C in
 >   it.
 > 
 > * Otherwise, just look up A::B::C.
 > 
 > This is, of course, screwy - the last thing we want to do is look for
 > a method named 'B' in a class named 'A'.  In particular, this fails if
 > we have a class A with a nested class A::B.
 > 
 > This patch remedies the situation: rather than looping through all the
 > double-colons and stopping at each stage to ask if the symbol there is
 > a class, it only asks that question when it reaches the last set of
 > double-colons.  So it's pretty mechanical - basically, all I did was
 > move the code for looking up a class outside of the loop looking for
 > double-colons, and then reindent.  While I was at it, I deleted some
 > dead code; if this patch is approved, I'll commit that part of the
 > patch first before committing the rest of it.

The approach looks ok, but, how does the HP related comment fit in
with the new code? I know it came in with the HP merge, which is a
clue as to its accuracy.... I guess MichaelC found no problems, so it
should be ok.

 > 
 > This patch actually isn't on carlton_dictionary-branch, so it hasn't
 > gotten quite as much testing as my other recent patches.  The reason
 > for that is that the way I handled this issue on the branch is more
 > complex, involves more linespec refactoring, and has known bugs
 > associated to it.  I like this simpler patch a lot better.
 > 
 > Tested on i686-pc-linux-gnu, with 5 different GCC/debug format
 > combinations.  No regressions on any of them; the new test passes on
 > all of them.  OK to commit?
 > 

I have a new version which incorporates the new comments. 
Does this still work for you?



Index: linespec.c
===================================================================
RCS file: /cvs/src/src/gdb/linespec.c,v
retrieving revision 1.55
diff -u -p -r1.55 linespec.c
--- linespec.c	9 Feb 2004 22:29:21 -0000	1.55
+++ linespec.c	10 Feb 2004 01:14:34 -0000
@@ -1215,78 +1215,6 @@ decode_compound (char **argptr, int funf
 
   while (1)
     {
-
-      /* Start of lookup in the symbol tables. */
-
-      /* Lookup in the symbol table the substring between argptr and
-	 p. Note, this call changes the value of argptr.  */
-      /* PASS1: Before the call, argptr->"AAA::inA::fun",
-	 p->"::inA::fun".  After the call: argptr->"inA::fun", p
-	 unchanged.  */
-      /* PASS2: Before the call, argptr->"AAA::inA::fun", p->"::fun".
-	 After the call: argptr->"fun", p->"::fun".  */
-      sym_class = lookup_prefix_sym (argptr, p);
-
-      /* PASS1: assume sym_class == NULL. Skip the whole if-stmt. */
-      /* PASS2: assume sym_class has been found, i.e. "AAA::inA" is a
-	 class. Enter the if-stmt.  */
-      if (sym_class &&
-	  (t = check_typedef (SYMBOL_TYPE (sym_class)),
-	   (TYPE_CODE (t) == TYPE_CODE_STRUCT
-	    || TYPE_CODE (t) == TYPE_CODE_UNION)))
-	{
-	  /* Arg token is not digits => try it as a function name.
-	     Find the next token (everything up to end or next
-	     blank).  */
-	  if (**argptr
-	      && strchr (get_gdb_completer_quote_characters (),
-			 **argptr) != NULL)
-	    {
-	      p = skip_quoted (*argptr);
-	      *argptr = *argptr + 1;
-	    }
-	  else
-	    {
-	      /* PASS2: at this point argptr->"fun".  */
-	      p = *argptr;
-	      while (*p && *p != ' ' && *p != '\t' && *p != ',' && *p != ':')
-		p++;
-	      /* PASS2: at this point p->"".  String ended.  */
-	    }
-
-	  /* Allocate our own copy of the substring between argptr and
-	     p. */
-	  copy = (char *) alloca (p - *argptr + 1);
-	  memcpy (copy, *argptr, p - *argptr);
-	  copy[p - *argptr] = '\0';
-	  if (p != *argptr
-	      && copy[p - *argptr - 1]
-	      && strchr (get_gdb_completer_quote_characters (),
-			 copy[p - *argptr - 1]) != NULL)
-	    copy[p - *argptr - 1] = '\0';
-
-	  /* PASS2: At this point copy->"fun", p->"" */
-
-	  /* No line number may be specified.  */
-	  while (*p == ' ' || *p == '\t')
-	    p++;
-	  *argptr = p;
-
-	  /* Look for copy as a method of sym_class. */
-	  /* PASS2: at this point copy->"fun", sym_class is "AAA:inA".
-	     This concludes the scanning of the string for possible
-	     components matches.  If we find it here, we return. If
-	     not, and we are at the and of the string, we'll get out
-	     of the loop and lookup the whole string in the symbol
-	     tables.  */
-
-	  return find_method (funfirstline, canonical, saved_arg,
-			      copy, t, sym_class);
-	} /* End if symbol found */
-
-      /* End of lookup in the symbol tables.  */
-
-      /* Prepare for next run through the loop.  */
       /* Move pointer up to next possible class/namespace token.  */
 
       p = p2 + 1;	/* Restart with old value +1.  */
@@ -1294,6 +1222,7 @@ decode_compound (char **argptr, int funf
       /* PASS1: at this point p2->"::inA::fun", so p->":inA::fun",
 	 i.e. if there is a double-colon, p will now point to the
 	 second colon. */
+      /* PASS2: p2->"::fun", p->":fun" */
 
       /* Move pointer ahead to next double-colon.  */
       while (*p && (p[0] != ' ') && (p[0] != '\t') && (p[0] != '\''))
@@ -1313,9 +1242,12 @@ decode_compound (char **argptr, int funf
 	     the beginning of this loop (PASS1), we had
 	     p->":inA::fun", we'll trigger this when p has been
 	     advanced to point to "::fun".  */
+	  /* PASS2: we will not trigger this. */
 	  else if ((p[0] == ':') && (p[1] == ':'))
 	    break;	/* Found double-colon.  */
 	  else
+	    /* PASS2: We'll keep getting here, until p->"", at which point
+	       we exit this loop.  */
 	    p++;
 	}
 
@@ -1323,7 +1255,7 @@ decode_compound (char **argptr, int funf
 	break;		/* Out of the while (1).  This would happen
 			   for instance if we have looked up
 			   unsuccessfully all the components of the
-			   string, and p->"".  */
+			   string, and p->""(PASS2)  */
 
       /* We get here if p points to ' ', '\t', '\'', "::" or ""(i.e
 	 string ended). */
@@ -1331,14 +1263,82 @@ decode_compound (char **argptr, int funf
       p2 = p;
       /* Restore argptr as it was on entry to this function.  */
       *argptr = saved_arg2;
-      /* PASS1: at this point p->"::fun" argptr->"AAA::inA::fun".  */
+      /* PASS1: at this point p->"::fun" argptr->"AAA::inA::fun", p2->"::fun".  */
 
       /* All ready for next pass through the loop.  */
     }			/* while (1) */
 
-  /* Last chance attempt -- check entire name as a symbol.  Use "copy"
-     in preparation for jumping out of this block, to be consistent
-     with usage following the jump target.  */
+
+  /* Start of lookup in the symbol tables. */
+
+  /* Lookup in the symbol table the substring between argptr and
+     p. Note, this call changes the value of argptr.  */
+  /* Before the call, argptr->"AAA::inA::fun",
+     p->"", p2->"::fun".  After the call: argptr->"fun", p, p2
+     unchanged.  */
+  sym_class = lookup_prefix_sym (argptr, p2);
+
+  /* Assume sym_class has been found, i.e. "AAA::inA" is a
+     class. Enter the if-stmt.  */
+  if (sym_class &&
+      (t = check_typedef (SYMBOL_TYPE (sym_class)),
+       (TYPE_CODE (t) == TYPE_CODE_STRUCT
+	|| TYPE_CODE (t) == TYPE_CODE_UNION)))
+    {
+      /* Arg token is not digits => try it as a function name.
+	 Find the next token (everything up to end or next
+	 blank).  */
+      if (**argptr
+	  && strchr (get_gdb_completer_quote_characters (),
+		     **argptr) != NULL)
+	{
+	  p = skip_quoted (*argptr);
+	  *argptr = *argptr + 1;
+	}
+      else
+	{
+	  /* At this point argptr->"fun".  */
+	  p = *argptr;
+	  while (*p && *p != ' ' && *p != '\t' && *p != ',' && *p != ':')
+	    p++;
+	  /* At this point p->"".  String ended.  */
+	}
+
+      /* Allocate our own copy of the substring between argptr and
+	 p. */
+      copy = (char *) alloca (p - *argptr + 1);
+      memcpy (copy, *argptr, p - *argptr);
+      copy[p - *argptr] = '\0';
+      if (p != *argptr
+	  && copy[p - *argptr - 1]
+	  && strchr (get_gdb_completer_quote_characters (),
+		     copy[p - *argptr - 1]) != NULL)
+	copy[p - *argptr - 1] = '\0';
+
+      /* At this point copy->"fun", p->"" */
+
+      /* No line number may be specified.  */
+      while (*p == ' ' || *p == '\t')
+	p++;
+      *argptr = p;
+      /* At this point arptr->"".  */
+
+      /* Look for copy as a method of sym_class. */
+      /* At this point copy->"fun", sym_class is "AAA:inA",
+	 saved_arg->"AAA::inA::fun".  This concludes the scanning of
+	 the string for possible components matches.  If we find it
+	 here, we return. If not, and we are at the and of the string,
+	 we'll lookup the whole string in the symbol tables.  */
+
+      return find_method (funfirstline, canonical, saved_arg,
+			  copy, t, sym_class);
+
+    } /* End if symbol found */
+
+
+  /* We couldn't find a class, so check the entire name as a symbol
+     instead.  */
+
   copy = (char *) alloca (p - saved_arg2 + 1);
   memcpy (copy, saved_arg2, p - saved_arg2);
   /* Note: if is_quoted should be true, we snuff out quote here

Re: [rfa] teach linespec about nested classes

[David Carlton]

On Mon, 9 Feb 2004 20:11:52 -0500, Elena Zannoni <ezannoni@redhat.com> said:

> The approach looks ok, but, how does the HP related comment fit in
> with the new code? I know it came in with the HP merge, which is a
> clue as to its accuracy.... I guess MichaelC found no problems, so
> it should be ok.

As far as I can tell from that and from other comments elsewhere, HP
must have been the first people with a C++ compiler that supported
namespaces and that GDB supported.  So some of the namespace-related
comments look more HP-specific than they were.  Also, as far as I can
tell, HP was generating fully-qualified type names long before we
were, so that would have led to some differences as well.  But now I
think that the names should look quite similar for DWARF 2 code and HP
code, so those comments shouldn't be relevant any more.

I couldn't think of any reason why looking up every intermediate name
as a class would make any more sense for HP than it would for other
cases.  And, as you say, MichaelC found no problems, which is good.

> I have a new version which incorporates the new comments.  Does this
> still work for you?

The functionality is fine, but the comments are a little off (it
includes the out-of-date HP comments instead of my new ones).  How
about this version?  I updated my comments to use the same example
that you had used.

David Carlton
carlton@kealia.com

Index: linespec.c
===================================================================
RCS file: /cvs/src/src/gdb/linespec.c,v
retrieving revision 1.55
diff -u -p -r1.55 linespec.c
--- linespec.c	9 Feb 2004 22:29:21 -0000	1.55
+++ linespec.c	10 Feb 2004 18:24:27 -0000
@@ -1180,25 +1180,19 @@ decode_compound (char **argptr, int funf
       && ((*argptr == p) || (p[-1] == ' ') || (p[-1] == '\t')))
     saved_arg2 += 2;
 
-  /* We have what looks like a class or namespace
-     scope specification (A::B), possibly with many
-     levels of namespaces or classes (A::B::C::D).
-
-     Some versions of the HP ANSI C++ compiler (as also possibly
-     other compilers) generate class/function/member names with
-     embedded double-colons if they are inside namespaces. To
-     handle this, we loop a few times, considering larger and
-     larger prefixes of the string as though they were single
-     symbols.  So, if the initially supplied string is
-     A::B::C::D::foo, we have to look up "A", then "A::B",
-     then "A::B::C", then "A::B::C::D", and finally
-     "A::B::C::D::foo" as single, monolithic symbols, because
-     A, B, C or D may be namespaces.
-
-     Note that namespaces can nest only inside other
-     namespaces, and not inside classes.  So we need only
-     consider *prefixes* of the string; there is no need to look up
-     "B::C" separately as a symbol in the previous example.  */
+  /* Given our example "AAA::inA::fun", we have two cases to consider:
+
+     1) AAA::inA is the name of a class.  In that case, presumably it
+        has a method called "fun"; we then look up that method using
+        find_method.
+
+     2) AAA::inA isn't the name of a class.  In that case, either the
+        user made a typo or AAA::inA is the name of a namespace.
+        Either way, we just look up AAA::inA::fun with lookup_symbol.
+
+     Thus, our first task is to find everything before the last set of
+     double-colons and figure out if it's the name of a class.  So we
+     first loop through all of the double-colons.  */
 
   p2 = p;		/* Save for restart.  */
 
@@ -1215,78 +1209,6 @@ decode_compound (char **argptr, int funf
 
   while (1)
     {
-
-      /* Start of lookup in the symbol tables. */
-
-      /* Lookup in the symbol table the substring between argptr and
-	 p. Note, this call changes the value of argptr.  */
-      /* PASS1: Before the call, argptr->"AAA::inA::fun",
-	 p->"::inA::fun".  After the call: argptr->"inA::fun", p
-	 unchanged.  */
-      /* PASS2: Before the call, argptr->"AAA::inA::fun", p->"::fun".
-	 After the call: argptr->"fun", p->"::fun".  */
-      sym_class = lookup_prefix_sym (argptr, p);
-
-      /* PASS1: assume sym_class == NULL. Skip the whole if-stmt. */
-      /* PASS2: assume sym_class has been found, i.e. "AAA::inA" is a
-	 class. Enter the if-stmt.  */
-      if (sym_class &&
-	  (t = check_typedef (SYMBOL_TYPE (sym_class)),
-	   (TYPE_CODE (t) == TYPE_CODE_STRUCT
-	    || TYPE_CODE (t) == TYPE_CODE_UNION)))
-	{
-	  /* Arg token is not digits => try it as a function name.
-	     Find the next token (everything up to end or next
-	     blank).  */
-	  if (**argptr
-	      && strchr (get_gdb_completer_quote_characters (),
-			 **argptr) != NULL)
-	    {
-	      p = skip_quoted (*argptr);
-	      *argptr = *argptr + 1;
-	    }
-	  else
-	    {
-	      /* PASS2: at this point argptr->"fun".  */
-	      p = *argptr;
-	      while (*p && *p != ' ' && *p != '\t' && *p != ',' && *p != ':')
-		p++;
-	      /* PASS2: at this point p->"".  String ended.  */
-	    }
-
-	  /* Allocate our own copy of the substring between argptr and
-	     p. */
-	  copy = (char *) alloca (p - *argptr + 1);
-	  memcpy (copy, *argptr, p - *argptr);
-	  copy[p - *argptr] = '\0';
-	  if (p != *argptr
-	      && copy[p - *argptr - 1]
-	      && strchr (get_gdb_completer_quote_characters (),
-			 copy[p - *argptr - 1]) != NULL)
-	    copy[p - *argptr - 1] = '\0';
-
-	  /* PASS2: At this point copy->"fun", p->"" */
-
-	  /* No line number may be specified.  */
-	  while (*p == ' ' || *p == '\t')
-	    p++;
-	  *argptr = p;
-
-	  /* Look for copy as a method of sym_class. */
-	  /* PASS2: at this point copy->"fun", sym_class is "AAA:inA".
-	     This concludes the scanning of the string for possible
-	     components matches.  If we find it here, we return. If
-	     not, and we are at the and of the string, we'll get out
-	     of the loop and lookup the whole string in the symbol
-	     tables.  */
-
-	  return find_method (funfirstline, canonical, saved_arg,
-			      copy, t, sym_class);
-	} /* End if symbol found */
-
-      /* End of lookup in the symbol tables.  */
-
-      /* Prepare for next run through the loop.  */
       /* Move pointer up to next possible class/namespace token.  */
 
       p = p2 + 1;	/* Restart with old value +1.  */
@@ -1294,6 +1216,7 @@ decode_compound (char **argptr, int funf
       /* PASS1: at this point p2->"::inA::fun", so p->":inA::fun",
 	 i.e. if there is a double-colon, p will now point to the
 	 second colon. */
+      /* PASS2: p2->"::fun", p->":fun" */
 
       /* Move pointer ahead to next double-colon.  */
       while (*p && (p[0] != ' ') && (p[0] != '\t') && (p[0] != '\''))
@@ -1313,9 +1236,12 @@ decode_compound (char **argptr, int funf
 	     the beginning of this loop (PASS1), we had
 	     p->":inA::fun", we'll trigger this when p has been
 	     advanced to point to "::fun".  */
+	  /* PASS2: we will not trigger this. */
 	  else if ((p[0] == ':') && (p[1] == ':'))
 	    break;	/* Found double-colon.  */
 	  else
+	    /* PASS2: We'll keep getting here, until p->"", at which point
+	       we exit this loop.  */
 	    p++;
 	}
 
@@ -1323,7 +1249,7 @@ decode_compound (char **argptr, int funf
 	break;		/* Out of the while (1).  This would happen
 			   for instance if we have looked up
 			   unsuccessfully all the components of the
-			   string, and p->"".  */
+			   string, and p->""(PASS2)  */
 
       /* We get here if p points to ' ', '\t', '\'', "::" or ""(i.e
 	 string ended). */
@@ -1331,14 +1257,84 @@ decode_compound (char **argptr, int funf
       p2 = p;
       /* Restore argptr as it was on entry to this function.  */
       *argptr = saved_arg2;
-      /* PASS1: at this point p->"::fun" argptr->"AAA::inA::fun".  */
+      /* PASS1: at this point p->"::fun" argptr->"AAA::inA::fun",
+	 p2->"::fun".  */
 
       /* All ready for next pass through the loop.  */
     }			/* while (1) */
 
-  /* Last chance attempt -- check entire name as a symbol.  Use "copy"
-     in preparation for jumping out of this block, to be consistent
-     with usage following the jump target.  */
+
+  /* Start of lookup in the symbol tables. */
+
+  /* Lookup in the symbol table the substring between argptr and
+     p. Note, this call changes the value of argptr.  */
+  /* Before the call, argptr->"AAA::inA::fun",
+     p->"", p2->"::fun".  After the call: argptr->"fun", p, p2
+     unchanged.  */
+  sym_class = lookup_prefix_sym (argptr, p2);
+
+  /* If sym_class has been found, and if "AAA::inA" is a class, then
+     we're in case 1 above.  So we look up "fun" as a method of that
+     class.  */
+  if (sym_class &&
+      (t = check_typedef (SYMBOL_TYPE (sym_class)),
+       (TYPE_CODE (t) == TYPE_CODE_STRUCT
+	|| TYPE_CODE (t) == TYPE_CODE_UNION)))
+    {
+      /* Arg token is not digits => try it as a function name.
+	 Find the next token (everything up to end or next
+	 blank).  */
+      if (**argptr
+	  && strchr (get_gdb_completer_quote_characters (),
+		     **argptr) != NULL)
+	{
+	  p = skip_quoted (*argptr);
+	  *argptr = *argptr + 1;
+	}
+      else
+	{
+	  /* At this point argptr->"fun".  */
+	  p = *argptr;
+	  while (*p && *p != ' ' && *p != '\t' && *p != ',' && *p != ':')
+	    p++;
+	  /* At this point p->"".  String ended.  */
+	}
+
+      /* Allocate our own copy of the substring between argptr and
+	 p. */
+      copy = (char *) alloca (p - *argptr + 1);
+      memcpy (copy, *argptr, p - *argptr);
+      copy[p - *argptr] = '\0';
+      if (p != *argptr
+	  && copy[p - *argptr - 1]
+	  && strchr (get_gdb_completer_quote_characters (),
+		     copy[p - *argptr - 1]) != NULL)
+	copy[p - *argptr - 1] = '\0';
+
+      /* At this point copy->"fun", p->"" */
+
+      /* No line number may be specified.  */
+      while (*p == ' ' || *p == '\t')
+	p++;
+      *argptr = p;
+      /* At this point arptr->"".  */
+
+      /* Look for copy as a method of sym_class. */
+      /* At this point copy->"fun", sym_class is "AAA:inA",
+	 saved_arg->"AAA::inA::fun".  This concludes the scanning of
+	 the string for possible components matches.  If we find it
+	 here, we return. If not, and we are at the and of the string,
+	 we'll lookup the whole string in the symbol tables.  */
+
+      return find_method (funfirstline, canonical, saved_arg,
+			  copy, t, sym_class);
+
+    } /* End if symbol found */
+
+
+  /* We couldn't find a class, so we're in case 2 above.  We check the
+     entire name as a symbol instead.  */
+
   copy = (char *) alloca (p - saved_arg2 + 1);
   memcpy (copy, saved_arg2, p - saved_arg2);
   /* Note: if is_quoted should be true, we snuff out quote here

Re: [rfa] teach linespec about nested classes

[Elena Zannoni]

David Carlton writes:
 > On Mon, 9 Feb 2004 20:11:52 -0500, Elena Zannoni <ezannoni@redhat.com> said:
 > 
 > > The approach looks ok, but, how does the HP related comment fit in
 > > with the new code? I know it came in with the HP merge, which is a
 > > clue as to its accuracy.... I guess MichaelC found no problems, so
 > > it should be ok.
 > 
 > As far as I can tell from that and from other comments elsewhere, HP
 > must have been the first people with a C++ compiler that supported
 > namespaces and that GDB supported.  So some of the namespace-related
 > comments look more HP-specific than they were.  Also, as far as I can
 > tell, HP was generating fully-qualified type names long before we
 > were, so that would have led to some differences as well.  But now I
 > think that the names should look quite similar for DWARF 2 code and HP
 > code, so those comments shouldn't be relevant any more.
 > 

definitely HP were first on a lot of the c++ stuff, yes. And they
changed a lot of decode_lie_1 (now decode_compound).

 > I couldn't think of any reason why looking up every intermediate name
 > as a class would make any more sense for HP than it would for other
 > cases.  And, as you say, MichaelC found no problems, which is good.
 > 

probably their compiler has changed too, in 5+ years (this stuff dates
back to 1997).

 > > I have a new version which incorporates the new comments.  Does this
 > > still work for you?
 > 
 > The functionality is fine, but the comments are a little off (it
 > includes the out-of-date HP comments instead of my new ones).  How
 > about this version?  I updated my comments to use the same example
 > that you had used.

groan, I missed that. Sure, you can commit this.

Re: [rfa] teach linespec about nested classes

[David Carlton]

On Wed, 11 Feb 2004 10:35:28 -0500, Elena Zannoni <ezannoni@redhat.com> said:

> Sure, you can commit this.

Thanks, committed.

David Carlton
carlton@kealia.com

Re: [rfa] teach linespec about nested classes

[Michael Elizabeth Chastain]

David Carlton writes:
> The functionality is fine, but the comments are a little off (it
> includes the out-of-date HP comments instead of my new ones).  How
> about this version?  I updated my comments to use the same example
> that you had used.

Works for me on native hppa2.0w-hp-hpux11.11 with hp ansi c and hp aC++.
No regressions, no improvements.

Michael C

Re: [rfa] teach linespec about nested classes

[Elena Zannoni]

Michael Elizabeth Chastain writes:
 > David Carlton writes:
 > > The functionality is fine, but the comments are a little off (it
 > > includes the out-of-date HP comments instead of my new ones).  How
 > > about this version?  I updated my comments to use the same example
 > > that you had used.
 > 
 > Works for me on native hppa2.0w-hp-hpux11.11 with hp ansi c and hp aC++.
 > No regressions, no improvements.
 > 

thanks Michael.

 > Michael C