rbreak works poorly with ObjC methods

rbreak works poorly with ObjC methods

[Jason Molenda]

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

Hey gdb'ers,

rbreak_command gets a list of functions matching a regular expression,
then caonicalizes those into filename:function-name and sets
breakpoints on the canonicalized form.  For an Objective-C method
that takes an argument, this form sent to break_command will be
something like
   basicclass.m:'+[BasicClass newWithArg:]'

The filename is separated out by linespec.c:locate_first_half().
locate_first_half will correctly handle filename.m:+[classname
arg:] type arguments, but it cannot cope when the apostrophy is
added and there is a second ":" after the filename-function name
separator.

Unfortunately this can be fixed in several ways. :-)  I believe
the best is to change the way rbreak_command canonicalizes its
arguments to be in the form
   "basicclass.m:+[BasicClass newWithArg:]"

An example patch of this is attached.

Another way to fix this is to modify linespec.c's is_objc_method_format
to recognize a third ObjC method pattern -- :'-[ ... ] -- in addition
to the two it already does.  This fix seems pretty lame in my
opinion--it's pushing the ObjC method spotter in the direction of
being its own little decode_line_1.  A patch showing how this would look
is also attached to this mail note.

The third way to fix this is to add the smarts to detect filename:' ... '
to locate_first_half() itself.  This has the benefit that
is_objc_method_format is still used to detect the actual objc method
and the smarts about the added apostrphy are pushed up to this function.
An example of this approach is also attached.  (Ignore the APPLE LOCAL
markers - this just happens to be the first one I tried)


I'll also attach a testsuite addition to basicclass.exp that shows the
failure.  It's an annoying one because the rbreak command stops setting
breakpoints as soon as one of them fails, so any regexp that happens to
match an ObjC method name with a colon in it will not complete.

None of these changes introduce any new testsuite regressions on
x86 Linux & gcc 3.3.1 -gdwarf2.


Any opinions about how this is best addressed?  I'm rooting for 
rbreak_command myself.  I can file a GNATS bug to track this if
anyone would prefer.


Jason

[-- Attachment #2: rbreak_command.txt --]
[-- Type: text/plain, Size: 812 bytes --]

Index: symtab.c
===================================================================
RCS file: /cvs/src/src/gdb/symtab.c,v
retrieving revision 1.114
diff -u -p -r1.114 symtab.c
--- symtab.c	8 Aug 2003 14:04:02 -0000	1.114
+++ symtab.c	4 Sep 2003 20:29:36 -0000
@@ -3194,10 +3194,11 @@ rbreak_command (char *regexp, int from_t
 	  char *string = alloca (strlen (p->symtab->filename)
 				 + strlen (SYMBOL_LINKAGE_NAME (p->symbol))
 				 + 4);
-	  strcpy (string, p->symtab->filename);
-	  strcat (string, ":'");
+	  strcpy (string, "\"");
+	  strcat (string, p->symtab->filename);
+	  strcat (string, ":");
 	  strcat (string, SYMBOL_LINKAGE_NAME (p->symbol));
-	  strcat (string, "'");
+	  strcat (string, "\"");
 	  break_command (string, from_tty);
 	  print_symbol_info (FUNCTIONS_DOMAIN,
 			     p->symtab,

[-- Attachment #3: is_objc_method_format.txt --]
[-- Type: text/plain, Size: 618 bytes --]

Index: linespec.c
===================================================================
RCS file: /cvs/src/src/gdb/linespec.c,v
retrieving revision 1.51
diff -u -p -r1.51 linespec.c
--- linespec.c	2 Aug 2003 03:59:40 -0000	1.51
+++ linespec.c	4 Sep 2003 20:12:21 -0000
@@ -461,6 +461,9 @@ is_objc_method_format (const char *s)
   /* Handle arguments that are just SYMBOL.  */
   else if ((strchr ("+-", s[0]) != NULL) && (s[1] == '[') && strchr(s, ']'))
     return 1;
+  else if (s[0] == ':' && s[1] == '\'' && strchr ("+-", s[2]) != NULL &&
+           s[3] == '[' && strchr (s, ']'))
+    return 1;
   return 0;
 }
 

[-- Attachment #4: loate_first_half.txt --]
[-- Type: text/plain, Size: 2032 bytes --]

Index: linespec.c
===================================================================
RCS file: /cvs/src/src/gdb/linespec.c,v
retrieving revision 1.51
diff -u -p -r1.51 linespec.c
--- linespec.c	2 Aug 2003 03:59:40 -0000	1.51
+++ linespec.c	4 Sep 2003 18:42:38 -0000
@@ -997,15 +997,33 @@ locate_first_half (char **argptr, int *i
 	  break;
 	}
       /* Check for the end of the first half of the linespec.  End of
-         line, a tab, a double colon or the last single colon, or a
-         space.  But if enclosed in double quotes we do not break on
-         enclosed spaces.  */
+         line, a tab, or a space.  But if enclosed in double quotes we 
+         do not break on enclosed spaces.  */
+      /* APPLE LOCAL: Break out checks for ":"s so we can handle ObjC
+         selector names with :'s correctly.  */
       if (!*p
-	  || p[0] == '\t'
-	  || ((p[0] == ':')
-	      && ((p[1] == ':') || (strchr (p + 1, ':') == NULL)))
-	  || ((p[0] == ' ') && !*is_quote_enclosed))
-	break;
+          || p[0] == '\t'
+          || ((p[0] == ' ') && !*is_quote_enclosed))
+        break;
+
+      /* Check for the end of the first half of the linespec.  
+         A double colon or the last single colon.  */
+      if (p[0] == ':')
+        {
+          /* Look for an Objective-C method name start after the colon
+             or after a quote mark after the colon.  ObjC names can have
+             colons in the argument names.  
+             e.g.  filename.m:'-[classname selector:]'  
+             rbreak_command () canonializes its matches like this.  */
+          int is_objc_method = is_objc_method_format (p + 1) || 
+                               ((p[1] == '\'' || p[1] == '"') &&
+                                is_objc_method_format (p + 2));
+          if (p[1] == ':' || is_objc_method || strchr (p + 1, ':') == NULL)
+            {
+              break;
+            }
+        }
+
       if (p[0] == '.' && strchr (p, ':') == NULL)
 	{
 	  /* Java qualified method.  Find the *last* '.', since the

[-- Attachment #5: testsuite.txt --]
[-- Type: text/plain, Size: 649 bytes --]

Index: basicclass.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.objc/basicclass.exp,v
retrieving revision 1.3
diff -u -p -r1.3 basicclass.exp
--- basicclass.exp	6 Jul 2003 22:27:34 -0000	1.3
+++ basicclass.exp	4 Sep 2003 18:42:10 -0000
@@ -115,6 +115,10 @@ gdb_test "break newWithArg:" \
     "Breakpoint.*at.* file .*$srcfile, line.22.*" \
     "breakpoint class method with colon"
 
+gdb_test "rbreak Arg" \
+    "Breakpoint.*at.* file.*Breakpoint.*at.* file.*"  \
+    "rbreak Arg matches at least two functions"
+
 #
 # Continue until breakpoint (test re-setting breakpoint)
 #

Re: rbreak works poorly with ObjC methods

[Adam Fedor]

On Thu, 2003-09-04 at 15:35, Jason Molenda wrote:
> Hey gdb'ers,
> 
> rbreak_command gets a list of functions matching a regular expression,
> then caonicalizes those into filename:function-name and sets
> breakpoints on the canonicalized form.  For an Objective-C method
> that takes an argument, this form sent to break_command will be
> something like
>    basicclass.m:'+[BasicClass newWithArg:]'
> 
> The filename is separated out by linespec.c:locate_first_half().
> locate_first_half will correctly handle filename.m:+[classname
> arg:] type arguments, but it cannot cope when the apostrophy is
> added and there is a second ":" after the filename-function name
> separator.
> 
> Unfortunately this can be fixed in several ways. :-)  I believe
> the best is to change the way rbreak_command canonicalizes its
> arguments to be in the form
>    "basicclass.m:+[BasicClass newWithArg:]"
> 
> An example patch of this is attached.
> 
> Another way to fix this is to modify linespec.c's is_objc_method_format
> to recognize a third ObjC method pattern -- :'-[ ... ] -- in addition
> to the two it already does.  This fix seems pretty lame in my
> opinion--it's pushing the ObjC method spotter in the direction of
> being its own little decode_line_1.  A patch showing how this would look
> is also attached to this mail note.
> 
> The third way to fix this is to add the smarts to detect filename:' ... '
> to locate_first_half() itself.  This has the benefit that
> is_objc_method_format is still used to detect the actual objc method
> and the smarts about the added apostrphy are pushed up to this function.
> An example of this approach is also attached.  (Ignore the APPLE LOCAL
> markers - this just happens to be the first one I tried)
> 
> 


I probably don't have much say in the final outcome, but I think
changing rbreak is the best also - it seems more consistent with the way
other things are canonicalized