[PATCH] Speed up psymbol reading by removing a copy

[PATCH] Speed up psymbol reading by removing a copy

[Tom Tromey]

I noticed that cp_canonicalize_string and friends copy a
unique_xmalloc_ptr to a std::string.  However, this copy isn't
genuinely needed anywhere, and it serves to slow down DWARF psymbol
reading.

This patch removes the copy and updates the callers to adapt.

This speeds up the reader from 1.906 seconds (mean of 10 runs, of gdb
on a copy of itself) to 1.888 seconds (mean of 10 runs, on the same
copy as the first trial).

gdb/ChangeLog
2020-04-04  Tom Tromey  <tom@tromey.com>

	* symtab.h (class demangle_result_storage) <set_malloc_ptr>: New
	overload.
	<swap_string, m_string>: Remove.
	* symtab.c (demangle_for_lookup, completion_list_add_symbol):
	Update.
	* stabsread.c (define_symbol, read_type): Update.
	* linespec.c (find_linespec_symbols): Update.
	* gnu-v3-abi.c (gnuv3_get_typeid): Update.
	* dwarf2/read.c (dwarf2_canonicalize_name): Update.
	* dbxread.c (read_dbx_symtab): Update.
	* cp-support.h (cp_canonicalize_string_full)
	(cp_canonicalize_string, cp_canonicalize_string_no_typedefs):
	Return unique_xmalloc_ptr.
	* cp-support.c (inspect_type): Update.
	(cp_canonicalize_string_full): Return unique_xmalloc_ptr.
	(cp_canonicalize_string_no_typedefs, cp_canonicalize_string):
	Likewise.
	* c-typeprint.c (print_name_maybe_canonical): Update.
	* break-catch-throw.c (check_status_exception_catchpoint):
	Update.
---
 gdb/ChangeLog           | 23 +++++++++++++++++++++++
 gdb/break-catch-throw.c |  2 +-
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/gdb/break-catch-throw.c b/gdb/break-catch-throw.c
index 7c684de40b6..59293c4e570 100644
--- a/gdb/break-catch-throw.c
+++ b/gdb/break-catch-throw.c
@@ -148,7 +148,6 @@ check_status_exception_catchpoint (struct bpstats *bs)
   struct exception_catchpoint *self
     = (struct exception_catchpoint *) bs->breakpoint_at;
   std::string type_name;
-  gdb::unique_xmalloc_ptr<char> canon;
 
   bkpt_breakpoint_ops.check_status (bs);
   if (bs->stop == 0)
@@ -158,6 +157,7 @@ check_status_exception_catchpoint (struct bpstats *bs)
     return;
 
   const char *name = nullptr;
+  gdb::unique_xmalloc_ptr<char> canon;
   try
     {
       struct value *typeinfo_arg;
-- 
2.17.2

Re: [PATCH] Speed up psymbol reading by removing a copy

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

Tom> I noticed that cp_canonicalize_string and friends copy a
Tom> unique_xmalloc_ptr to a std::string.  However, this copy isn't
Tom> genuinely needed anywhere, and it serves to slow down DWARF psymbol
Tom> reading.

Tom> This patch removes the copy and updates the callers to adapt.

Tom> This speeds up the reader from 1.906 seconds (mean of 10 runs, of gdb
Tom> on a copy of itself) to 1.888 seconds (mean of 10 runs, on the same
Tom> copy as the first trial).

I didn't notice at the time, but this message didn't include the entire
patch.  I've appended the full patch now.

Tom

commit 85517f992b95d51baa7a50d5c7fae32f32166429
Author: Tom Tromey <tom@tromey.com>
Date:   Sun Mar 29 11:38:21 2020 -0600

    Speed up psymbol reading by removing a copy
    
    I noticed that cp_canonicalize_string and friends copy a
    unique_xmalloc_ptr to a std::string.  However, this copy isn't
    genuinely needed anywhere, and it serves to slow down DWARF psymbol
    reading.
    
    This patch removes the copy and updates the callers to adapt.
    
    This speeds up the reader from 1.906 seconds (mean of 10 runs, of gdb
    on a copy of itself) to 1.888 seconds (mean of 10 runs, on the same
    copy as the first trial).
    
    gdb/ChangeLog
    2020-04-18  Tom Tromey  <tom@tromey.com>
    
            * symtab.h (class demangle_result_storage) <set_malloc_ptr>: New
            overload.
            <swap_string, m_string>: Remove.
            * symtab.c (demangle_for_lookup, completion_list_add_symbol):
            Update.
            * stabsread.c (define_symbol, read_type): Update.
            * linespec.c (find_linespec_symbols): Update.
            * gnu-v3-abi.c (gnuv3_get_typeid): Update.
            * dwarf2/read.c (dwarf2_canonicalize_name): Update.
            * dbxread.c (read_dbx_symtab): Update.
            * cp-support.h (cp_canonicalize_string_full)
            (cp_canonicalize_string, cp_canonicalize_string_no_typedefs):
            Return unique_xmalloc_ptr.
            * cp-support.c (inspect_type): Update.
            (cp_canonicalize_string_full): Return unique_xmalloc_ptr.
            (cp_canonicalize_string_no_typedefs, cp_canonicalize_string):
            Likewise.
            * c-typeprint.c (print_name_maybe_canonical): Update.
            * break-catch-throw.c (check_status_exception_catchpoint):
            Update.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 7078129fe7c..d5419ee0fd5 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,26 @@
+2020-04-18  Tom Tromey  <tom@tromey.com>
+
+	* symtab.h (class demangle_result_storage) <set_malloc_ptr>: New
+	overload.
+	<swap_string, m_string>: Remove.
+	* symtab.c (demangle_for_lookup, completion_list_add_symbol):
+	Update.
+	* stabsread.c (define_symbol, read_type): Update.
+	* linespec.c (find_linespec_symbols): Update.
+	* gnu-v3-abi.c (gnuv3_get_typeid): Update.
+	* dwarf2/read.c (dwarf2_canonicalize_name): Update.
+	* dbxread.c (read_dbx_symtab): Update.
+	* cp-support.h (cp_canonicalize_string_full)
+	(cp_canonicalize_string, cp_canonicalize_string_no_typedefs):
+	Return unique_xmalloc_ptr.
+	* cp-support.c (inspect_type): Update.
+	(cp_canonicalize_string_full): Return unique_xmalloc_ptr.
+	(cp_canonicalize_string_no_typedefs, cp_canonicalize_string):
+	Likewise.
+	* c-typeprint.c (print_name_maybe_canonical): Update.
+	* break-catch-throw.c (check_status_exception_catchpoint):
+	Update.
+
 2020-04-18  Tom Tromey  <tom@tromey.com>
 
 	* xcoffread.c (enter_line_range, scan_xcoff_symtab): Update.
diff --git a/gdb/break-catch-throw.c b/gdb/break-catch-throw.c
index 07dcc7dc0e7..59293c4e570 100644
--- a/gdb/break-catch-throw.c
+++ b/gdb/break-catch-throw.c
@@ -156,26 +156,28 @@ check_status_exception_catchpoint (struct bpstats *bs)
   if (self->pattern == NULL)
     return;
 
+  const char *name = nullptr;
+  gdb::unique_xmalloc_ptr<char> canon;
   try
     {
       struct value *typeinfo_arg;
-      std::string canon;
 
       fetch_probe_arguments (NULL, &typeinfo_arg);
       type_name = cplus_typename_from_type_info (typeinfo_arg);
 
       canon = cp_canonicalize_string (type_name.c_str ());
-      if (!canon.empty ())
-	std::swap (type_name, canon);
+      name = (canon == nullptr
+	      ? canon.get ()
+	      : type_name.c_str ());
     }
   catch (const gdb_exception_error &e)
     {
       exception_print (gdb_stderr, e);
     }
 
-  if (!type_name.empty ())
+  if (name != nullptr)
     {
-      if (self->pattern->exec (type_name.c_str (), 0, NULL, 0) != 0)
+      if (self->pattern->exec (name, 0, NULL, 0) != 0)
 	bs->stop = 0;
     }
 }
diff --git a/gdb/c-exp.y b/gdb/c-exp.y
index feab51a8e2c..f84691a62e7 100644
--- a/gdb/c-exp.y
+++ b/gdb/c-exp.y
@@ -1739,13 +1739,14 @@ oper:	OPERATOR NEW
 
 			  c_print_type ($2, NULL, &buf, -1, 0,
 					&type_print_raw_options);
+			  std::string name = std::move (buf.string ());
 
 			  /* This also needs canonicalization.  */
-			  std::string canon
-			    = cp_canonicalize_string (buf.c_str ());
-			  if (canon.empty ())
-			    canon = std::move (buf.string ());
-			  $$ = operator_stoken ((" " + canon).c_str ());
+			  gdb::unique_xmalloc_ptr<char> canon
+			    = cp_canonicalize_string (name.c_str ());
+			  if (canon != nullptr)
+			    name = canon.get ();
+			  $$ = operator_stoken ((" " + name).c_str ());
 			}
 	;
 
diff --git a/gdb/c-typeprint.c b/gdb/c-typeprint.c
index 50d0eaa2dde..aaf9e0dfe0a 100644
--- a/gdb/c-typeprint.c
+++ b/gdb/c-typeprint.c
@@ -84,14 +84,14 @@ print_name_maybe_canonical (const char *name,
 			    const struct type_print_options *flags,
 			    struct ui_file *stream)
 {
-  std::string s;
+  gdb::unique_xmalloc_ptr<char> s;
 
   if (!flags->raw)
     s = cp_canonicalize_string_full (name,
 				     find_typedef_for_canonicalize,
 				     (void *) flags);
 
-  fputs_filtered (!s.empty () ? s.c_str () : name, stream);
+  fputs_filtered (s != nullptr ? s.get () : name, stream);
 }
 
 \f
diff --git a/gdb/cp-support.c b/gdb/cp-support.c
index 6601272e717..92a2e3b4904 100644
--- a/gdb/cp-support.c
+++ b/gdb/cp-support.c
@@ -274,12 +274,13 @@ inspect_type (struct demangle_parse_info *info,
 
 		 Canonicalize the name again, and store it in the
 		 current node (RET_COMP).  */
-	      std::string canon = cp_canonicalize_string_no_typedefs (name);
+	      gdb::unique_xmalloc_ptr<char> canon
+		= cp_canonicalize_string_no_typedefs (name);
 
-	      if (!canon.empty ())
+	      if (canon != nullptr)
 		{
 		  /* Copy the canonicalization into the obstack.  */
-		  name = copy_string_to_obstack (&info->obstack, canon.c_str (), &len);
+		  name = copy_string_to_obstack (&info->obstack, canon.get (), &len);
 		}
 
 	      ret_comp->u.s_name.s = name;
@@ -506,16 +507,15 @@ replace_typedefs (struct demangle_parse_info *info,
 
 /* Parse STRING and convert it to canonical form, resolving any
    typedefs.  If parsing fails, or if STRING is already canonical,
-   return the empty string.  Otherwise return the canonical form.  If
+   return nullptr.  Otherwise return the canonical form.  If
    FINDER is not NULL, then type components are passed to FINDER to be
    looked up.  DATA is passed verbatim to FINDER.  */
 
-std::string
+gdb::unique_xmalloc_ptr<char>
 cp_canonicalize_string_full (const char *string,
 			     canonicalization_ftype *finder,
 			     void *data)
 {
-  std::string ret;
   unsigned int estimated_len;
   std::unique_ptr<demangle_parse_info> info;
 
@@ -531,41 +531,42 @@ cp_canonicalize_string_full (const char *string,
 							    estimated_len);
       gdb_assert (us);
 
-      ret = us.get ();
       /* Finally, compare the original string with the computed
 	 name, returning NULL if they are the same.  */
-      if (ret == string)
-	return std::string ();
+      if (strcmp (us.get (), string) == 0)
+	return nullptr;
+
+      return us;
     }
 
-  return ret;
+  return nullptr;
 }
 
 /* Like cp_canonicalize_string_full, but always passes NULL for
    FINDER.  */
 
-std::string
+gdb::unique_xmalloc_ptr<char>
 cp_canonicalize_string_no_typedefs (const char *string)
 {
   return cp_canonicalize_string_full (string, NULL, NULL);
 }
 
 /* Parse STRING and convert it to canonical form.  If parsing fails,
-   or if STRING is already canonical, return the empty string.
+   or if STRING is already canonical, return nullptr.
    Otherwise return the canonical form.  */
 
-std::string
+gdb::unique_xmalloc_ptr<char>
 cp_canonicalize_string (const char *string)
 {
   std::unique_ptr<demangle_parse_info> info;
   unsigned int estimated_len;
 
   if (cp_already_canonical (string))
-    return std::string ();
+    return nullptr;
 
   info = cp_demangled_name_to_comp (string, NULL);
   if (info == NULL)
-    return std::string ();
+    return nullptr;
 
   estimated_len = strlen (string) * 2;
   gdb::unique_xmalloc_ptr<char> us (cp_comp_to_string (info->tree,
@@ -575,15 +576,13 @@ cp_canonicalize_string (const char *string)
     {
       warning (_("internal error: string \"%s\" failed to be canonicalized"),
 	       string);
-      return std::string ();
+      return nullptr;
     }
 
-  std::string ret (us.get ());
-
-  if (ret == string)
-    return std::string ();
+  if (strcmp (us.get (), string) == 0)
+    return nullptr;
 
-  return ret;
+  return us;
 }
 
 /* Convert a mangled name to a demangle_component tree.  *MEMORY is
diff --git a/gdb/cp-support.h b/gdb/cp-support.h
index 6f148342714..6ca898315bb 100644
--- a/gdb/cp-support.h
+++ b/gdb/cp-support.h
@@ -77,15 +77,16 @@ struct demangle_parse_info
 
 /* Functions from cp-support.c.  */
 
-extern std::string cp_canonicalize_string (const char *string);
+extern gdb::unique_xmalloc_ptr<char> cp_canonicalize_string
+  (const char *string);
 
-extern std::string cp_canonicalize_string_no_typedefs (const char *string);
+extern gdb::unique_xmalloc_ptr<char> cp_canonicalize_string_no_typedefs
+  (const char *string);
 
 typedef const char *(canonicalization_ftype) (struct type *, void *);
 
-extern std::string cp_canonicalize_string_full (const char *string,
-						canonicalization_ftype *finder,
-						void *data);
+extern gdb::unique_xmalloc_ptr<char> cp_canonicalize_string_full
+  (const char *string, canonicalization_ftype *finder, void *data);
 
 extern char *cp_class_name_from_physname (const char *physname);
 
diff --git a/gdb/dbxread.c b/gdb/dbxread.c
index c0155593e3b..1e1a5dc9b97 100644
--- a/gdb/dbxread.c
+++ b/gdb/dbxread.c
@@ -1434,12 +1434,13 @@ read_dbx_symtab (minimal_symbol_reader &reader, struct objfile *objfile)
  	  if (psymtab_language == language_cplus)
  	    {
 	      std::string name (namestring, p - namestring);
-	      std::string new_name = cp_canonicalize_string (name.c_str ());
-	      if (!new_name.empty ())
+	      gdb::unique_xmalloc_ptr<char> new_name
+		= cp_canonicalize_string (name.c_str ());
+	      if (new_name != nullptr)
 		{
-		  sym_len = new_name.length ();
+		  sym_len = strlen (new_name.get ());
 		  sym_name = obstack_strdup (&objfile->objfile_obstack,
-					     new_name);
+					     new_name.get ());
 		}
 	    }
 
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 41db511c851..180500ff396 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -21449,13 +21449,11 @@ dwarf2_canonicalize_name (const char *name, struct dwarf2_cu *cu,
 {
   if (name && cu->language == language_cplus)
     {
-      std::string canon_name = cp_canonicalize_string (name);
+      gdb::unique_xmalloc_ptr<char> canon_name
+	= cp_canonicalize_string (name);
 
-      if (!canon_name.empty ())
-	{
-	  if (canon_name != name)
-	    name = objfile->intern (canon_name);
-	}
+      if (canon_name != nullptr)
+	name = objfile->intern (canon_name.get ());
     }
 
   return name;
diff --git a/gdb/gnu-v3-abi.c b/gdb/gnu-v3-abi.c
index 89574ec9ed5..aa9884dc6e4 100644
--- a/gdb/gnu-v3-abi.c
+++ b/gdb/gnu-v3-abi.c
@@ -1064,7 +1064,8 @@ gnuv3_get_typeid (struct value *value)
   struct type *type;
   struct gdbarch *gdbarch;
   struct value *result;
-  std::string type_name, canonical;
+  std::string type_name;
+  gdb::unique_xmalloc_ptr<char> canonical;
 
   /* We have to handle values a bit trickily here, to allow this code
      to work properly with non_lvalue values that are really just
@@ -1092,8 +1093,9 @@ gnuv3_get_typeid (struct value *value)
      uses.  E.g., GDB tends to use "const char *" as a type name, but
      the demangler uses "char const *".  */
   canonical = cp_canonicalize_string (type_name.c_str ());
-  if (!canonical.empty ())
-    type_name = canonical;
+  const char *name = (canonical == nullptr
+		      ? type_name.c_str ()
+		      : canonical.get ());
 
   typeinfo_type = gnuv3_get_typeid_type (gdbarch);
 
@@ -1109,19 +1111,19 @@ gnuv3_get_typeid (struct value *value)
       vtable = gnuv3_get_vtable (gdbarch, type, address);
       if (vtable == NULL)
 	error (_("cannot find typeinfo for object of type '%s'"),
-	       type_name.c_str ());
+	       name);
       typeinfo_value = value_field (vtable, vtable_field_type_info);
       result = value_ind (value_cast (make_pointer_type (typeinfo_type, NULL),
 				      typeinfo_value));
     }
   else
     {
-      std::string sym_name = std::string ("typeinfo for ") + type_name;
+      std::string sym_name = std::string ("typeinfo for ") + name;
       bound_minimal_symbol minsym
 	= lookup_minimal_symbol (sym_name.c_str (), NULL, NULL);
 
       if (minsym.minsym == NULL)
-	error (_("could not find typeinfo symbol for '%s'"), type_name.c_str ());
+	error (_("could not find typeinfo symbol for '%s'"), name);
 
       result = value_at_lazy (typeinfo_type, BMSYMBOL_VALUE_ADDRESS (minsym));
     }
diff --git a/gdb/linespec.c b/gdb/linespec.c
index 0eb3bc5b8d4..1a8429c51bf 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -3899,9 +3899,10 @@ find_linespec_symbols (struct linespec_state *state,
 		       std::vector <block_symbol> *symbols,
 		       std::vector<bound_minimal_symbol> *minsyms)
 {
-  std::string canon = cp_canonicalize_string_no_typedefs (lookup_name);
-  if (!canon.empty ())
-    lookup_name = canon.c_str ();
+  gdb::unique_xmalloc_ptr<char> canon
+    = cp_canonicalize_string_no_typedefs (lookup_name);
+  if (canon != nullptr)
+    lookup_name = canon.get ();
 
   /* It's important to not call expand_symtabs_matching unnecessarily
      as it can really slow things down (by unnecessarily expanding
diff --git a/gdb/stabsread.c b/gdb/stabsread.c
index 5ef7748a9eb..eac47407105 100644
--- a/gdb/stabsread.c
+++ b/gdb/stabsread.c
@@ -738,7 +738,7 @@ define_symbol (CORE_ADDR valu, const char *string, int desc, int type,
   else
     {
     normal:
-      std::string new_name;
+      gdb::unique_xmalloc_ptr<char> new_name;
 
       if (sym->language () == language_cplus)
 	{
@@ -748,8 +748,8 @@ define_symbol (CORE_ADDR valu, const char *string, int desc, int type,
 	  name[p - string] = '\0';
 	  new_name = cp_canonicalize_string (name);
 	}
-      if (!new_name.empty ())
-	sym->compute_and_set_names (new_name, true, objfile->per_bfd);
+      if (new_name != nullptr)
+	sym->compute_and_set_names (new_name.get (), true, objfile->per_bfd);
       else
 	sym->compute_and_set_names (gdb::string_view (string, p - string), true,
 				    objfile->per_bfd);
@@ -1637,10 +1637,10 @@ read_type (const char **pp, struct objfile *objfile)
 	      memcpy (name, *pp, p - *pp);
 	      name[p - *pp] = '\0';
 
-	      std::string new_name = cp_canonicalize_string (name);
-	      if (!new_name.empty ())
+	      gdb::unique_xmalloc_ptr<char> new_name = cp_canonicalize_string (name);
+	      if (new_name != nullptr)
 		type_name = obstack_strdup (&objfile->objfile_obstack,
-					    new_name);
+					    new_name.get ());
 	    }
 	  if (type_name == NULL)
 	    {
diff --git a/gdb/symtab.c b/gdb/symtab.c
index dc079efbc27..ddf2aaf26c7 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -1854,9 +1854,9 @@ demangle_for_lookup (const char *name, enum language lang,
 
       /* If we were given a non-mangled name, canonicalize it
 	 according to the language (so far only for C++).  */
-      std::string canon = cp_canonicalize_string (name);
-      if (!canon.empty ())
-	return storage.swap_string (canon);
+      gdb::unique_xmalloc_ptr<char> canon = cp_canonicalize_string (name);
+      if (canon != nullptr)
+	return storage.set_malloc_ptr (std::move (canon));
     }
   else if (lang == language_d)
     {
@@ -5339,10 +5339,10 @@ completion_list_add_symbol (completion_tracker &tracker,
       /* The call to canonicalize returns the empty string if the input
 	 string is already in canonical form, thanks to this we don't
 	 remove the symbol we just added above.  */
-      std::string str
+      gdb::unique_xmalloc_ptr<char> str
 	= cp_canonicalize_string_no_typedefs (sym->natural_name ());
-      if (!str.empty ())
-	tracker.remove_completion (str.c_str ());
+      if (str != nullptr)
+	tracker.remove_completion (str.get ());
     }
 }
 
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 77f60e6c24a..0c22bde6e83 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -2301,20 +2301,18 @@ bool iterate_over_symbols_terminated
 /* Storage type used by demangle_for_lookup.  demangle_for_lookup
    either returns a const char * pointer that points to either of the
    fields of this type, or a pointer to the input NAME.  This is done
-   this way because the underlying functions that demangle_for_lookup
-   calls either return a std::string (e.g., cp_canonicalize_string) or
-   a malloc'ed buffer (libiberty's demangled), and we want to avoid
-   unnecessary reallocation/string copying.  */
+   this way to avoid depending on the precise details of the storage
+   for the string.  */
 class demangle_result_storage
 {
 public:
 
-  /* Swap the std::string storage with STR, and return a pointer to
-     the beginning of the new string.  */
-  const char *swap_string (std::string &str)
+  /* Swap the malloc storage to STR, and return a pointer to the
+     beginning of the new string.  */
+  const char *set_malloc_ptr (gdb::unique_xmalloc_ptr<char> &&str)
   {
-    std::swap (m_string, str);
-    return m_string.c_str ();
+    m_malloc = std::move (str);
+    return m_malloc.get ();
   }
 
   /* Set the malloc storage to now point at PTR.  Any previous malloc
@@ -2328,7 +2326,6 @@ class demangle_result_storage
 private:
 
   /* The storage.  */
-  std::string m_string;
   gdb::unique_xmalloc_ptr<char> m_malloc;
 };
 

Re: [PATCH] Speed up psymbol reading by removing a copy

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
Tom> I noticed that cp_canonicalize_string and friends copy a
Tom> unique_xmalloc_ptr to a std::string.  However, this copy isn't
Tom> genuinely needed anywhere, and it serves to slow down DWARF psymbol
Tom> reading.

Tom> This patch removes the copy and updates the callers to adapt.

Tom> This speeds up the reader from 1.906 seconds (mean of 10 runs, of gdb
Tom> on a copy of itself) to 1.888 seconds (mean of 10 runs, on the same
Tom> copy as the first trial).

Tom> I didn't notice at the time, but this message didn't include the entire
Tom> patch.  I've appended the full patch now.

I'm checking this in now. 

Tom

[committed][gdb] Fix catch throw regexp matching

[Tom de Vries]

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

[was: Re: [PATCH] Speed up psymbol reading by removing a copy ]
On 08-05-2020 22:15, Tom Tromey wrote:
>>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
> 
>>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
> Tom> I noticed that cp_canonicalize_string and friends copy a
> Tom> unique_xmalloc_ptr to a std::string.  However, this copy isn't
> Tom> genuinely needed anywhere, and it serves to slow down DWARF psymbol
> Tom> reading.
> 
> Tom> This patch removes the copy and updates the callers to adapt.
> 
> Tom> This speeds up the reader from 1.906 seconds (mean of 10 runs, of gdb
> Tom> on a copy of itself) to 1.888 seconds (mean of 10 runs, on the same
> Tom> copy as the first trial).
> 
> Tom> I didn't notice at the time, but this message didn't include the entire
> Tom> patch.  I've appended the full patch now.
> 
> I'm checking this in now. 

This caused a regression, fixed in commit below.

Thanks,
- Tom


[-- Attachment #2: 0001-gdb-Fix-catch-throw-regexp-matching.patch --]
[-- Type: text/x-patch, Size: 3227 bytes --]

[gdb] Fix catch throw regexp matching

When running test-case gdb.mi/mi-catch-cpp-exceptions.exp, we have:
...
FAIL: gdb.mi/mi-catch-cpp-exceptions.exp: all with invalid regexp: run until \
  breakpoint in main (unknown output after running)
...

This is a regression since commit 596dc4adff "Speed up psymbol reading by
removing a copy".

Before that commit, we have:
...
$ gdb \
    -batch \
    ./outputs/gdb.mi/mi-catch-cpp-exceptions/mi-catch-cpp-exceptions \
    -ex "break 67" \
    -ex "catch throw -r blahblah" \
    -ex r
Breakpoint 1 at 0x4008e5: file mi-catch-cpp-exceptions.cc, line 67.
Catchpoint 2 (throw)

Breakpoint 1, main () at mi-catch-cpp-exceptions.cc:67
67                  return 1;   /* Stop here.  */
...
In other words:
- we set a breakpoint somewhere in main,
- we set a catchpoint with a regexp that is intended to not match any
  exception, and
- run to the breakpoint, without the catchpoint triggering.

After the commit, we have:
...
$ gdb \
    -batch \
    ./outputs/gdb.mi/mi-catch-cpp-exceptions/mi-catch-cpp-exceptions \
    -ex "break 67" \
    -ex "catch throw -r blahblah" \
    -ex r
Breakpoint 1 at 0x4008e5: file mi-catch-cpp-exceptions.cc, line 67.
Catchpoint 2 (throw)

Catchpoint 2 (exception thrown), 0x00007ffff7ab037e in __cxa_throw () from \
  /usr/lib64/libstdc++.so.6
...
In other words, the catchpoint triggers.

This is caused by this bit of the commit:
...
       type_name = cplus_typename_from_type_info (typeinfo_arg);

       canon = cp_canonicalize_string (type_name.c_str ());
-      if (!canon.empty ())
-       std::swap (type_name, canon);
+      name = (canon == nullptr
+	      ? canon.get ()
+	      : type_name.c_str ());
     }
   catch (const gdb_exception_error &e)
     {
       exception_print (gdb_stderr, e);
     }

-  if (!type_name.empty ())
+  if (name != nullptr)
     {
-      if (self->pattern->exec (type_name.c_str (), 0, NULL, 0) != 0)
+      if (self->pattern->exec (name, 0, NULL, 0) != 0)
...

Before the commit, we have:
- type_name == "my_exception"
- canon = ""
and the !type_name.empty () test succeeds, and gdb executes the
self->pattern->exec call.

After the commit, we have:
- type_name == "my_exception"
- canon == NULL
- name == NULL
and the name != nullptr test fails, and gdb doesn't execute the
self->pattern->exec call.

Fix this by inverting the condition for the calculation of name:
...
-      name = (canon == nullptr
+      name = (canon != nullptr
...

Build and tested on x86_64-linux.

gdb/ChangeLog:

2020-05-09  Tom de Vries  <tdevries@suse.de>

	PR gdb/25955
	* break-catch-throw.c (check_status_exception_catchpoint): Fix name
	calculation.

---
 gdb/break-catch-throw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/break-catch-throw.c b/gdb/break-catch-throw.c
index 59293c4e57..7f4a9f955d 100644
--- a/gdb/break-catch-throw.c
+++ b/gdb/break-catch-throw.c
@@ -166,7 +166,7 @@ check_status_exception_catchpoint (struct bpstats *bs)
       type_name = cplus_typename_from_type_info (typeinfo_arg);
 
       canon = cp_canonicalize_string (type_name.c_str ());
-      name = (canon == nullptr
+      name = (canon != nullptr
 	      ? canon.get ()
 	      : type_name.c_str ());
     }

Re: [committed][gdb] Fix catch throw regexp matching

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> This caused a regression, fixed in commit below.

Thank you.

Tom