FYI: minor simplification in add_psymbol_to_bcache

FYI: minor simplification in add_psymbol_to_bcache

[Tom Tromey]

This patch changes add_psymbol_to_bcache in two ways.

First, it removes the gratuitous use of a 'static' local variable.

Second, it removes the copying of 'name'.  I think this copy is not
needed because SYMBOL_SET_NAMES handles this case itself.

Built and regtested on x86-64 (compile farm).
I'll wait a while in case anybody has any comments on this.

Tom

2009-11-03  Tom Tromey  <tromey@redhat.com>

	* symfile.c (add_psymbol_to_bcache): Don't copy name.  Make
	'psymbol' non-static.

Index: symfile.c
===================================================================
RCS file: /cvs/src/src/gdb/symfile.c,v
retrieving revision 1.251
diff -u -r1.251 symfile.c
--- symfile.c	2 Nov 2009 14:59:51 -0000	1.251
+++ symfile.c	3 Nov 2009 20:08:33 -0000
@@ -3106,19 +3106,9 @@
 		       enum language language, struct objfile *objfile,
 		       int *added)
 {
-  char *buf = name;  
-  /* psymbol is static so that there will be no uninitialized gaps in the
-     structure which might contain random data, causing cache misses in
-     bcache. */
-  static struct partial_symbol psymbol;
-  
-  if (name[namelength] != '\0')
-    {
-      buf = alloca (namelength + 1);
-      /* Create local copy of the partial symbol */
-      memcpy (buf, name, namelength);
-      buf[namelength] = '\0';
-    }
+  struct partial_symbol psymbol;
+
+  memset (&psymbol, 0, sizeof (struct partial_symbol));
   /* val and coreaddr are mutually exclusive, one of them *will* be zero */
   if (val != 0)
     {
@@ -3133,7 +3123,7 @@
   PSYMBOL_DOMAIN (&psymbol) = domain;
   PSYMBOL_CLASS (&psymbol) = class;
 
-  SYMBOL_SET_NAMES (&psymbol, buf, namelength, objfile);
+  SYMBOL_SET_NAMES (&psymbol, name, namelength, objfile);
 
   /* Stash the partial symbol away in the cache */
   return bcache_full (&psymbol, sizeof (struct partial_symbol),

Re: FYI: minor simplification in add_psymbol_to_bcache

[Daniel Jacobowitz]

On Tue, Nov 03, 2009 at 01:10:24PM -0700, Tom Tromey wrote:
> This patch changes add_psymbol_to_bcache in two ways.
> 
> First, it removes the gratuitous use of a 'static' local variable.

It adds a memset... if you assume that this is a hot function, then it
was probably avoided on purpose.  On the other hand, the way it's
implemented is quite dodgy; there will be bogus padding if
sizeof (long) != sizeof (CORE_ADDR) and you alternate adding symbols
with VAL and COREADDR set.

> Second, it removes the copying of 'name'.  I think this copy is not
> needed because SYMBOL_SET_NAMES handles this case itself.

Yes, must predate SYMBOL_SET_NAMES>

I have no objection.

-- 
Daniel Jacobowitz
CodeSourcery

Re: FYI: minor simplification in add_psymbol_to_bcache

[Tom Tromey]

>>>>> "Daniel" == Daniel Jacobowitz <drow@false.org> writes:

Tom> First, it removes the gratuitous use of a 'static' local variable.

Daniel> It adds a memset... if you assume that this is a hot function, then it
Daniel> was probably avoided on purpose.  On the other hand, the way it's
Daniel> implemented is quite dodgy; there will be bogus padding if
Daniel> sizeof (long) != sizeof (CORE_ADDR) and you alternate adding symbols
Daniel> with VAL and COREADDR set.

TBH I was not thinking too soundly, I suppose, when I nuked this.  I
think your point is a good one.  If you'd prefer, I will leave this
static and just zero out that field.

Tom

Re: FYI: minor simplification in add_psymbol_to_bcache

[Daniel Jacobowitz]

On Wed, Nov 04, 2009 at 12:16:27PM -0700, Tom Tromey wrote:
> TBH I was not thinking too soundly, I suppose, when I nuked this.  I
> think your point is a good one.  If you'd prefer, I will leave this
> static and just zero out that field.

Unless you have a good reason not to, I suggest making that change.

-- 
Daniel Jacobowitz
CodeSourcery

Re: FYI: minor simplification in add_psymbol_to_bcache

[Tom Tromey]

>>>>> "Daniel" == Daniel Jacobowitz <drow@false.org> writes:

>> On Wed, Nov 04, 2009 at 12:16:27PM -0700, Tom Tromey wrote:
>> TBH I was not thinking too soundly, I suppose, when I nuked this.  I
>> think your point is a good one.  If you'd prefer, I will leave this
>> static and just zero out that field.

Daniel> Unless you have a good reason not to, I suggest making that change.

Here is the patch I am checking in.

Tom

2009-11-05  Tom Tromey  <tromey@redhat.com>

	* symfile.c (add_psymbol_to_bcache): Make 'psymbol' static again.
	Zero the 'value' field.

Index: symfile.c
===================================================================
RCS file: /cvs/src/src/gdb/symfile.c,v
retrieving revision 1.253
diff -u -r1.253 symfile.c
--- symfile.c	5 Nov 2009 19:53:04 -0000	1.253
+++ symfile.c	5 Nov 2009 22:15:30 -0000
@@ -3105,9 +3105,15 @@
 		       enum language language, struct objfile *objfile,
 		       int *added)
 {
-  struct partial_symbol psymbol;
-
-  memset (&psymbol, 0, sizeof (struct partial_symbol));
+  /* psymbol is static so that there will be no uninitialized gaps in the
+     structure which might contain random data, causing cache misses in
+     bcache. */
+  static struct partial_symbol psymbol;
+
+  /* However, we must ensure that the entire 'value' field has been
+     zeroed before assigning to it, because an assignment may not
+     write the entire field.  */
+  memset (&psymbol.ginfo.value, 0, sizeof (psymbol.ginfo.value));
   /* val and coreaddr are mutually exclusive, one of them *will* be zero */
   if (val != 0)
     {