Fix dwarf2_name caching bug

Message ID 20200313151732.17943-1-tromey@adacore.com
State New
Headers show
Series
  • Fix dwarf2_name caching bug
Related show

Commit Message

Tom Tromey March 13, 2020, 3:17 p.m.
PR gdb/25663 points out that dwarf2_name will cache a value in the
bcache and then return a substring.  However, this substring return is
only done on the branch that caches the value -- so if the function is
called twice with the same arguments, it will return different values.

This patch fixes this problem.

This area is strange.  We cache the entire demangled string, but only
return the suffix.  I looked at caching just the suffix, but it turns
out that anonymous_struct_prefix assumes that the entire string is
stored.  Also weird is that this code is demangling the linkage name
and then storing the demangled form back into the linkage name
attribute -- that seems bad, because what if some code wants to find
the actual linkage name?

Fixing these issues was non-trivial, though; and in the meantime this
patch seems like an improvement.  Regression tested on x86-64
Fedora 30.

gdb/ChangeLog
2020-03-13  Tom Tromey  <tromey@adacore.com>

	PR gdb/25663:
	* dwarf2/read.c (dwarf2_name): Strip leading namespaces after
	putting value into bcache.
---
 gdb/ChangeLog     |  6 ++++++
 gdb/dwarf2/read.c | 18 ++++++++----------
 2 files changed, 14 insertions(+), 10 deletions(-)

-- 
2.21.1

Comments

Simon Marchi March 16, 2020, 5:26 p.m. | #1
On 2020-03-13 11:17 a.m., Tom Tromey wrote:
> PR gdb/25663 points out that dwarf2_name will cache a value in the

> bcache and then return a substring.  However, this substring return is

> only done on the branch that caches the value -- so if the function is

> called twice with the same arguments, it will return different values.

> 

> This patch fixes this problem.

> 

> This area is strange.  We cache the entire demangled string, but only

> return the suffix.  I looked at caching just the suffix, but it turns

> out that anonymous_struct_prefix assumes that the entire string is

> stored.  Also weird is that this code is demangling the linkage name

> and then storing the demangled form back into the linkage name

> attribute -- that seems bad, because what if some code wants to find

> the actual linkage name?

> 

> Fixing these issues was non-trivial, though; and in the meantime this

> patch seems like an improvement.  Regression tested on x86-64

> Fedora 30.

> 

> gdb/ChangeLog

> 2020-03-13  Tom Tromey  <tromey@adacore.com>

> 

> 	PR gdb/25663:

> 	* dwarf2/read.c (dwarf2_name): Strip leading namespaces after

> 	putting value into bcache.

> ---

>  gdb/ChangeLog     |  6 ++++++

>  gdb/dwarf2/read.c | 18 ++++++++----------

>  2 files changed, 14 insertions(+), 10 deletions(-)

> 

> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c

> index 1706b96cc04..88a60c1c73b 100644

> --- a/gdb/dwarf2/read.c

> +++ b/gdb/dwarf2/read.c

> @@ -21799,19 +21799,17 @@ dwarf2_name (struct die_info *die, struct dwarf2_cu *cu)

>  	      if (demangled == nullptr)

>  		return nullptr;

>  

> -	      const char *base;

> -

>  	      DW_STRING (attr) = objfile->intern (demangled.get ());

>  	      DW_STRING_IS_CANONICAL (attr) = 1;

> -

> -	      /* Strip any leading namespaces/classes, keep only the base name.

> -		 DW_AT_name for named DIEs does not contain the prefixes.  */

> -	      base = strrchr (DW_STRING (attr), ':');

> -	      if (base && base > DW_STRING (attr) && base[-1] == ':')

> -		return &base[1];

> -	      else

> -		return DW_STRING (attr);

>  	    }

> +

> +	  /* Strip any leading namespaces/classes, keep only the base name.

> +	     DW_AT_name for named DIEs does not contain the prefixes.  */

> +	  const char *base = strrchr (DW_STRING (attr), ':');

> +	  if (base && base > DW_STRING (attr) && base[-1] == ':')

> +	    return &base[1];

> +	  else

> +	    return DW_STRING (attr);

>  	}

>        break;

>  

> -- 

> 2.21.1


Hi Tom,

That LGTM.

Simon

Patch

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 1706b96cc04..88a60c1c73b 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -21799,19 +21799,17 @@  dwarf2_name (struct die_info *die, struct dwarf2_cu *cu)
 	      if (demangled == nullptr)
 		return nullptr;
 
-	      const char *base;
-
 	      DW_STRING (attr) = objfile->intern (demangled.get ());
 	      DW_STRING_IS_CANONICAL (attr) = 1;
-
-	      /* Strip any leading namespaces/classes, keep only the base name.
-		 DW_AT_name for named DIEs does not contain the prefixes.  */
-	      base = strrchr (DW_STRING (attr), ':');
-	      if (base && base > DW_STRING (attr) && base[-1] == ':')
-		return &base[1];
-	      else
-		return DW_STRING (attr);
 	    }
+
+	  /* Strip any leading namespaces/classes, keep only the base name.
+	     DW_AT_name for named DIEs does not contain the prefixes.  */
+	  const char *base = strrchr (DW_STRING (attr), ':');
+	  if (base && base > DW_STRING (attr) && base[-1] == ':')
+	    return &base[1];
+	  else
+	    return DW_STRING (attr);
 	}
       break;