[patch] not able to set pending breakpoint at qualified C++ function names

[patch] not able to set pending breakpoint at qualified C++ function names

[Karen Osmond]

Hello,

I noticed that it's not possible to set a pending breakpoint of the (C++) 
form "break X::Y::Z" (with breakpoint pending auto, gdb doesn't ask, and 
with it on, no breakpoint is added).  It works fine with "break Q" or to 
whatever the equivalent of X::Y::Z is in terms of filename and linenumber.

Patch follows; I've only just met the gdb source so make of it what you 
will :) 

 * Added not_found_ptr param to decode_compound, find_method and throw 
NOT_FOUND_ERROR to bring them in line with symtab_from_filename, 
decode_variable.
 * Removed cplusplus_error helper function, as these were the only uses. 
(Though, it was adding an extra hint to the error, which perhaps should 
still happen.  Erm, I must admit to not understanding the hint).

cheers,
-karen. 

Index: gdb/linespec.c
===================================================================
RCS file: /cvs/src/src/gdb/linespec.c,v
retrieving revision 1.84
diff -c -p -r1.84 linespec.c
*** gdb/linespec.c	3 Jan 2009 05:57:52 -0000	1.84
--- gdb/linespec.c	11 Feb 2009 18:09:12 -0000
*************** static struct symtabs_and_lines decode_c
*** 65,71 ****
  						 int funfirstline,
  						 char ***canonical,
  						 char *saved_arg,
! 						 char *p);
  
  static struct symbol *lookup_prefix_sym (char **argptr, char *p);
  
--- 65,72 ----
  						 int funfirstline,
  						 char ***canonical,
  						 char *saved_arg,
! 						 char *p,
! 						 int *not_found_ptr);
  
  static struct symbol *lookup_prefix_sym (char **argptr, char *p);
  
*************** static struct symtabs_and_lines find_met
*** 74,89 ****
  					     char *saved_arg,
  					     char *copy,
  					     struct type *t,
! 					     struct symbol *sym_class);
  
  static int collect_methods (char *copy, struct type *t,
  			    struct symbol *sym_class,
  			    struct symbol **sym_arr);
  
- static NORETURN void cplusplus_error (const char *name,
- 				      const char *fmt, ...)
-      ATTR_NORETURN ATTR_FORMAT (printf, 2, 3);
- 
  static int total_number_of_methods (struct type *type);
  
  static int find_methods (struct type *, char *,
--- 75,87 ----
  					     char *saved_arg,
  					     char *copy,
  					     struct type *t,
! 					     struct symbol *sym_class,
! 						 int *not_found_ptr);
  
  static int collect_methods (char *copy, struct type *t,
  			    struct symbol *sym_class,
  			    struct symbol **sym_arr);
  
  static int total_number_of_methods (struct type *type);
  
  static int find_methods (struct type *, char *,
*************** symtabs_and_lines minsym_found (int funf
*** 144,176 ****
  
  /* Helper functions. */
  
- /* Issue a helpful hint on using the command completion feature on
-    single quoted demangled C++ symbols as part of the completion
-    error.  */
- 
- static NORETURN void
- cplusplus_error (const char *name, const char *fmt, ...)
- {
-   struct ui_file *tmp_stream;
-   tmp_stream = mem_fileopen ();
-   make_cleanup_ui_file_delete (tmp_stream);
- 
-   {
-     va_list args;
-     va_start (args, fmt);
-     vfprintf_unfiltered (tmp_stream, fmt, args);
-     va_end (args);
-   }
- 
-   while (*name == '\'')
-     name++;
-   fprintf_unfiltered (tmp_stream,
- 		      ("Hint: try '%s<TAB> or '%s<ESC-?>\n"
- 		       "(Note leading single quote.)"),
- 		      name, name);
-   error_stream (tmp_stream);
- }
- 
  /* Return the number of methods described for TYPE, including the
     methods from types it derives from. This can't be done in the symbol
     reader because the type of the baseclass might still be stubbed
--- 142,147 ----
*************** decode_line_1 (char **argptr, int funfir
*** 765,771 ****
  	
        if (p[0] == '.' || p[1] == ':')
  	return decode_compound (argptr, funfirstline, canonical,
! 				saved_arg, p);
  
        /* No, the first part is a filename; set file_symtab to be that file's
  	 symtab.  Also, move argptr past the filename.  */
--- 736,742 ----
  	
        if (p[0] == '.' || p[1] == ':')
  	return decode_compound (argptr, funfirstline, canonical,
! 				saved_arg, p, not_found_ptr);
  
        /* No, the first part is a filename; set file_symtab to be that file's
  	 symtab.  Also, move argptr past the filename.  */
*************** decode_objc (char **argptr, int funfirst
*** 1195,1201 ****
  
  static struct symtabs_and_lines
  decode_compound (char **argptr, int funfirstline, char ***canonical,
! 		 char *saved_arg, char *p)
  {
    struct symtabs_and_lines values;
    char *p2;
--- 1166,1172 ----
  
  static struct symtabs_and_lines
  decode_compound (char **argptr, int funfirstline, char ***canonical,
! 		 char *saved_arg, char *p, int* not_found_ptr)
  {
    struct symtabs_and_lines values;
    char *p2;
*************** decode_compound (char **argptr, int funf
*** 1362,1368 ****
  	 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 */
  
--- 1333,1339 ----
  	 we'll lookup the whole string in the symbol tables.  */
  
        return find_method (funfirstline, canonical, saved_arg,
! 			  copy, t, sym_class, not_found_ptr);
  
      } /* End if symbol found */
  
*************** decode_compound (char **argptr, int funf
*** 1385,1392 ****
  
    /* Couldn't find any interpretation as classes/namespaces, so give
       up.  The quotes are important if copy is empty.  */
!   cplusplus_error (saved_arg,
! 		   "Can't find member of namespace, class, struct, or union named \"%s\"\n",
  		   copy);
  }
  
--- 1356,1365 ----
  
    /* Couldn't find any interpretation as classes/namespaces, so give
       up.  The quotes are important if copy is empty.  */
!   if (not_found_ptr)
!     *not_found_ptr = 1;
!   throw_error (NOT_FOUND_ERROR, 
! 		   _("Can't find member of namespace, class, struct, or union named \"%s\""),
  		   copy);
  }
  
*************** lookup_prefix_sym (char **argptr, char *
*** 1430,1436 ****
  
  static struct symtabs_and_lines
  find_method (int funfirstline, char ***canonical, char *saved_arg,
! 	     char *copy, struct type *t, struct symbol *sym_class)
  {
    struct symtabs_and_lines values;
    struct symbol *sym = NULL;
--- 1403,1409 ----
  
  static struct symtabs_and_lines
  find_method (int funfirstline, char ***canonical, char *saved_arg,
! 	     char *copy, struct type *t, struct symbol *sym_class, int *not_found_ptr)
  {
    struct symtabs_and_lines values;
    struct symbol *sym = NULL;
*************** find_method (int funfirstline, char ***c
*** 1481,1493 ****
  	}
        else
  	tmp = copy;
        if (tmp[0] == '~')
! 	cplusplus_error (saved_arg,
! 			 "the class `%s' does not have destructor defined\n",
  			 SYMBOL_PRINT_NAME (sym_class));
        else
! 	cplusplus_error (saved_arg,
! 			 "the class %s does not have any method named %s\n",
  			 SYMBOL_PRINT_NAME (sym_class), tmp);
      }
  }
--- 1454,1468 ----
  	}
        else
  	tmp = copy;
+ 	if (not_found_ptr)
+ 		*not_found_ptr = 1;
        if (tmp[0] == '~')
! 	throw_error (NOT_FOUND_ERROR, 
! 			 _("the class `%s' does not have destructor defined"),
  			 SYMBOL_PRINT_NAME (sym_class));
        else
! 	throw_error (NOT_FOUND_ERROR, 
! 			 _("the class %s does not have any method named %s"),
  			 SYMBOL_PRINT_NAME (sym_class), tmp);
      }
  }

Re: [patch] not able to set pending breakpoint at qualified C++ function names

[Tom Tromey]

>>>>> "Karen" == Karen Osmond <karen.osmond@gmail.com> writes:

Hi.  I'm sorry nobody from GDB got back to you sooner.

Karen> I noticed that it's not possible to set a pending breakpoint of
Karen> the (C++) form "break X::Y::Z" (with breakpoint pending auto,
Karen> gdb doesn't ask, and with it on, no breakpoint is added).

Karen> Patch follows; I've only just met the gdb source so make of it
Karen> what you will :)

Karen>  * Removed cplusplus_error helper function, as these were the only uses. 
Karen> (Though, it was adding an extra hint to the error, which perhaps should 
Karen> still happen.  Erm, I must admit to not understanding the hint).

I'd prefer not to remove this hint since it does help people, in my
experience.  What it means is that you can sometimes find valid
completions by quoting the symbol.

I think this could be done by just changing cplusplus_error to end
with something like:

  message = ui_file_xstrdup (stream, &len);
  make_cleanup (xfree, message);
  throw_error (NOT_FOUND_ERROR, "%s", message);

The rest of it seems ok to me.

I assume you don't have a copyright assignment in place.  I think you
probably should; send me email off-list and I will get you started on
that.

A patch for gdb requires a ChangeLog entry.  The GNU Coding Standards
explain how to write one.

Finally, I found a couple of formatting nits.  GDB as a project is
particular about this.  The GNU standards also cover this pretty well.

Karen>   static struct symtabs_and_lines
Karen>   decode_compound (char **argptr, int funfirstline, char ***canonical,
Karen> ! 		 char *saved_arg, char *p, int* not_found_ptr)

Use "int *", not "int* ".

Karen> + 	if (not_found_ptr)
Karen> + 		*not_found_ptr = 1;

The indentation looks funny here; the second line should be 2 spaces
in from the 'if'.

Tom

Re: [patch] not able to set pending breakpoint at qualified C++ function names

[Karen Osmond]

Hello!

Sorry about the excessive quoting below but both the original email and  
its reply are rather old so thought full context might be helpful in this 
case.

On Tue, 28 Apr 2009, Tom Tromey wrote:

> >>>>> "Karen" == Karen Osmond <karen.osmond@gmail.com> writes:
> 
> Hi.  I'm sorry nobody from GDB got back to you sooner.
> 
> Karen> I noticed that it's not possible to set a pending breakpoint of
> Karen> the (C++) form "break X::Y::Z" (with breakpoint pending auto,
> Karen> gdb doesn't ask, and with it on, no breakpoint is added).
> 
> Karen> Patch follows; I've only just met the gdb source so make of it
> Karen> what you will :)
> 
> Karen>  * Removed cplusplus_error helper function, as these were the only uses. 
> Karen> (Though, it was adding an extra hint to the error, which perhaps should 
> Karen> still happen.  Erm, I must admit to not understanding the hint).
> 
> I'd prefer not to remove this hint since it does help people, in my
> experience.  What it means is that you can sometimes find valid
> completions by quoting the symbol.
> 
> I think this could be done by just changing cplusplus_error to end
> with something like:
> 
>   message = ui_file_xstrdup (stream, &len);
>   make_cleanup (xfree, message);
>   throw_error (NOT_FOUND_ERROR, "%s", message);

Thanks for the explanation!  I've changed this as you suggest.
 
> The rest of it seems ok to me.
> 
> I assume you don't have a copyright assignment in place.  I think you
> probably should; send me email off-list and I will get you started on
> that.

Emailed separately.

> A patch for gdb requires a ChangeLog entry.  The GNU Coding Standards
> explain how to write one.
> 
> Finally, I found a couple of formatting nits.  GDB as a project is
> particular about this.  The GNU standards also cover this pretty well.
> 
> Karen>   static struct symtabs_and_lines
> Karen>   decode_compound (char **argptr, int funfirstline, char ***canonical,
> Karen> ! 		 char *saved_arg, char *p, int* not_found_ptr)
> 
> Use "int *", not "int* ".
> 
> Karen> + 	if (not_found_ptr)
> Karen> + 		*not_found_ptr = 1;
> 
> The indentation looks funny here; the second line should be 2 spaces
> in from the 'if'.

Sorry about this - ChangeLog added and nits addressed.  Well, I hope the 
indentation thing is addressed - I'm afraid this file contains a strange 
mix of tabs and spaces, so I've generally copied the leading whitespace on 
preceding lines in each bit.

New patch follows.

cheers,
-karen

2009-06-03  Karen Osmond  <karen.osmond@gmail.com>

	* linespec.c (find_method): Add new not_found_ptr parameter to
	bring in line with symtab_from_filename, decode_variable.
	(decode_compound): Likewise.  Also propagate not_found_ptr to 
	find_method.
	(decode_line_1): Propagage not_found_ptr to decode_compound.
	(cplusplus_error): Now throws NOT_FOUND_ERROR.

Index: gdb/linespec.c
===================================================================
RCS file: /cvs/src/src/gdb/linespec.c,v
retrieving revision 1.85
diff -c -p -r1.85 linespec.c
*** gdb/linespec.c	23 May 2009 16:17:17 -0000	1.85
--- gdb/linespec.c	3 Jun 2009 12:56:34 -0000
*************** static struct symtabs_and_lines decode_c
*** 65,71 ****
  						 int funfirstline,
  						 char ***canonical,
  						 char *saved_arg,
! 						 char *p);
  
  static struct symbol *lookup_prefix_sym (char **argptr, char *p);
  
--- 65,72 ----
  						 int funfirstline,
  						 char ***canonical,
  						 char *saved_arg,
! 						 char *p,
! 						 int *not_found_ptr);
  
  static struct symbol *lookup_prefix_sym (char **argptr, char *p);
  
*************** static struct symtabs_and_lines find_met
*** 74,80 ****
  					     char *saved_arg,
  					     char *copy,
  					     struct type *t,
! 					     struct symbol *sym_class);
  
  static int collect_methods (char *copy, struct type *t,
  			    struct symbol *sym_class,
--- 75,82 ----
  					     char *saved_arg,
  					     char *copy,
  					     struct type *t,
! 					     struct symbol *sym_class,
! 					     int *not_found_ptr);
  
  static int collect_methods (char *copy, struct type *t,
  			    struct symbol *sym_class,
*************** static NORETURN void
*** 152,157 ****
--- 154,161 ----
  cplusplus_error (const char *name, const char *fmt, ...)
  {
    struct ui_file *tmp_stream;
+   long len;
+   char *message;
    tmp_stream = mem_fileopen ();
    make_cleanup_ui_file_delete (tmp_stream);
  
*************** cplusplus_error (const char *name, const
*** 168,174 ****
  		      ("Hint: try '%s<TAB> or '%s<ESC-?>\n"
  		       "(Note leading single quote.)"),
  		      name, name);
!   error_stream (tmp_stream);
  }
  
  /* Return the number of methods described for TYPE, including the
--- 172,181 ----
  		      ("Hint: try '%s<TAB> or '%s<ESC-?>\n"
  		       "(Note leading single quote.)"),
  		      name, name);
! 
!   message = ui_file_xstrdup (tmp_stream, &len);                                   
!   make_cleanup (xfree, message);                                              
!   throw_error (NOT_FOUND_ERROR, "%s", message);  
  }
  
  /* Return the number of methods described for TYPE, including the
*************** decode_line_1 (char **argptr, int funfir
*** 765,771 ****
  	
        if (p[0] == '.' || p[1] == ':')
  	return decode_compound (argptr, funfirstline, canonical,
! 				saved_arg, p);
  
        /* No, the first part is a filename; set file_symtab to be that file's
  	 symtab.  Also, move argptr past the filename.  */
--- 772,778 ----
  	
        if (p[0] == '.' || p[1] == ':')
  	return decode_compound (argptr, funfirstline, canonical,
! 				saved_arg, p, not_found_ptr);
  
        /* No, the first part is a filename; set file_symtab to be that file's
  	 symtab.  Also, move argptr past the filename.  */
*************** decode_objc (char **argptr, int funfirst
*** 1195,1201 ****
  
  static struct symtabs_and_lines
  decode_compound (char **argptr, int funfirstline, char ***canonical,
! 		 char *saved_arg, char *p)
  {
    struct symtabs_and_lines values;
    char *p2;
--- 1202,1208 ----
  
  static struct symtabs_and_lines
  decode_compound (char **argptr, int funfirstline, char ***canonical,
! 		 char *saved_arg, char *p, int *not_found_ptr)
  {
    struct symtabs_and_lines values;
    char *p2;
*************** decode_compound (char **argptr, int funf
*** 1362,1368 ****
  	 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 */
  
--- 1369,1375 ----
  	 we'll lookup the whole string in the symbol tables.  */
  
        return find_method (funfirstline, canonical, saved_arg,
! 			  copy, t, sym_class, not_found_ptr);
  
      } /* End if symbol found */
  
*************** decode_compound (char **argptr, int funf
*** 1385,1390 ****
--- 1392,1399 ----
  
    /* Couldn't find any interpretation as classes/namespaces, so give
       up.  The quotes are important if copy is empty.  */
+   if (not_found_ptr)
+     *not_found_ptr = 1;
    cplusplus_error (saved_arg,
  		   "Can't find member of namespace, class, struct, or union named \"%s\"\n",
  		   copy);
*************** lookup_prefix_sym (char **argptr, char *
*** 1430,1436 ****
  
  static struct symtabs_and_lines
  find_method (int funfirstline, char ***canonical, char *saved_arg,
! 	     char *copy, struct type *t, struct symbol *sym_class)
  {
    struct symtabs_and_lines values;
    struct symbol *sym = NULL;
--- 1439,1445 ----
  
  static struct symtabs_and_lines
  find_method (int funfirstline, char ***canonical, char *saved_arg,
! 	     char *copy, struct type *t, struct symbol *sym_class, int *not_found_ptr)
  {
    struct symtabs_and_lines values;
    struct symbol *sym = NULL;
*************** find_method (int funfirstline, char ***c
*** 1481,1486 ****
--- 1490,1497 ----
  	}
        else
  	tmp = copy;
+       if (not_found_ptr)
+         *not_found_ptr = 1;
        if (tmp[0] == '~')
  	cplusplus_error (saved_arg,
  			 "the class `%s' does not have destructor defined\n",

Re: [patch] not able to set pending breakpoint at qualified C++ function names

[Tom Tromey]

>>>>> "Karen" == Karen Osmond <karen.osmond@gmail.com> writes:

Karen> 2009-06-03  Karen Osmond  <karen.osmond@gmail.com>
Karen> 	* linespec.c (find_method): Add new not_found_ptr parameter to
Karen> 	bring in line with symtab_from_filename, decode_variable.
Karen> 	(decode_compound): Likewise.  Also propagate not_found_ptr to 
Karen> 	find_method.
Karen> 	(decode_line_1): Propagage not_found_ptr to decode_compound.

Typo in "propagate".

Karen> 	(cplusplus_error): Now throws NOT_FOUND_ERROR.

Other than the typo, looks good to me.  This is ok once your paperwork
completes.  Send a note to the list when that happens and someone will
check this in.

Tom

Re: [patch] not able to set pending breakpoint at qualified C++ function names

[Karen Osmond]

On Thu, 4 Jun 2009, Tom Tromey wrote:

> >>>>> "Karen" == Karen Osmond <karen.osmond@gmail.com> writes:
> 
> Karen> 2009-06-03  Karen Osmond  <karen.osmond@gmail.com>
> Karen> 	* linespec.c (find_method): Add new not_found_ptr parameter to
> Karen> 	bring in line with symtab_from_filename, decode_variable.
> Karen> 	(decode_compound): Likewise.  Also propagate not_found_ptr to 
> Karen> 	find_method.
> Karen> 	(decode_line_1): Propagage not_found_ptr to decode_compound.
> 
> Typo in "propagate".
> 
> Karen> 	(cplusplus_error): Now throws NOT_FOUND_ERROR.
> 
> Other than the typo, looks good to me.  This is ok once your paperwork
> completes.  Send a note to the list when that happens and someone will
> check this in.

Paperwork is now sorted - please could somebody commit?

The patch can be found here:
http://sourceware.org/ml/gdb-patches/2009-06/msg00034.html

And here's the ChangeLog again, with typo corrected:

2009-06-22  Karen Osmond  <karen.osmond@gmail.com>

	* linespec.c (find_method): Add new not_found_ptr parameter to
	bring in line with symtab_from_filename, decode_variable.
	(decode_compound): Likewise.  Also propagate not_found_ptr to 
	find_method.
	(decode_line_1): Propagate not_found_ptr to decode_compound.
	(cplusplus_error): Now throws NOT_FOUND_ERROR.

cheers,
-karen.

Re: [patch] not able to set pending breakpoint at qualified C++ function names

[Karen Osmond]

On Mon, 22 Jun 2009, Karen Osmond wrote:
> 2009-06-22  Karen Osmond  <karen.osmond@gmail.com>
> 
> 	* linespec.c (find_method): Add new not_found_ptr parameter to
> 	bring in line with symtab_from_filename, decode_variable.
> 	(decode_compound): Likewise.  Also propagate not_found_ptr to 
> 	find_method.
> 	(decode_line_1): Propagate not_found_ptr to decode_compound.
> 	(cplusplus_error): Now throws NOT_FOUND_ERROR.

Committed.

cheers,
-karen

[patch] Test result regression for gdb.base/psymtab.exp [Re: [patch] not able to set pending breakpoint at qualified C++ function names]

[Jan Kratochvil]

On Thu, 25 Jun 2009 20:09:15 +0200, Karen Osmond wrote:
> On Mon, 22 Jun 2009, Karen Osmond wrote:
> > 2009-06-22  Karen Osmond  <karen.osmond@gmail.com>
> > 
> > 	* linespec.c (find_method): Add new not_found_ptr parameter to
> > 	bring in line with symtab_from_filename, decode_variable.
> > 	(decode_compound): Likewise.  Also propagate not_found_ptr to 
> > 	find_method.
> > 	(decode_line_1): Propagate not_found_ptr to decode_compound.
> > 	(cplusplus_error): Now throws NOT_FOUND_ERROR.
> 
> Committed.

There is now a small testsuite regression (x86_64-fedora-linux-gnu) as GDB now
behaves more correctly.
 Can't find member of namespace, class, struct, or union named "zzz::dummy"
 Hint: try 'zzz::dummy<TAB> or 'zzz::dummy<ESC-?>
 (Note leading single quote.)
-(gdb) PASS: gdb.base/psymtab.exp: Don't search past end of psymtab.
-testcase ../.././gdb/testsuite/gdb.base/psymtab.exp completed in 1 seconds
+Make breakpoint pending on future shared library load? (y or [n]) FAIL: gdb.base/psymtab.exp: Don't search past end of psymtab. (timeout)
+testcase ../.././gdb/testsuite/gdb.base/psymtab.exp completed in 60 seconds


Requesting approval from GDB maintainers.


Thanks,
Jan


2009-06-26  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/psymtab.exp (gdb.base/psymtab.exp): Turn off pending
	breakpoints.

--- a/gdb/testsuite/gdb.base/psymtab.exp
+++ b/gdb/testsuite/gdb.base/psymtab.exp
@@ -61,6 +61,10 @@ gdb_start
 gdb_reinitialize_dir $srcdir/$subdir
 gdb_load ${binfile}
 
+# GDB now correctly asks if the symbol can be found on future library load.
+
+gdb_test "set breakpoint pending off"
+
 # This test is looking for a bug that manifested itself when GDB was
 # looking for a partial symbol such that there wasn't such a partial
 # symbol in the psymtab, but such that the last psym in the psymtab

Re: [patch] Test result regression for gdb.base/psymtab.exp [Re: [patch] not able to set pending breakpoint at qualified C++ function names]

[Pedro Alves]

On Friday 26 June 2009 00:21:12, Jan Kratochvil wrote:
> +# GDB now correctly asks if the symbol can be found on future library load.
> +
> +gdb_test "set breakpoint pending off"
> +

I was going to say: okay, but please refrase the comment to not
say "now correctly asks", as "now" is relative.  In a few years,
it will look outdated.  But I just saw Doug's patch addressing
the same issue, and his comment is nicer.  :-)  :-P

If everyone's fine with it, let's go with Doug's version.

-- 
Pedro Alves