diff mbox

[review] Defer minimal symbol name-setting

Message ID gerrit.1571543710000.I4fe3993b99fb3a43968067806e294d48e377fd76@gnutoolchain-gerrit.osci.io
State New
Headers show

Commit Message

Simon Marchi (Code Review) Oct. 20, 2019, 3:55 a.m. UTC 
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/166
......................................................................

Defer minimal symbol name-setting

Currently the demangled name of a minimal symbol is set when creating
the symbol.  However, there is no intrinsic need to do this.  This
patch instead arranges for the demangling to be done just before the
minsym hash tables are filled.  This will be useful in a later patch.

gdb/ChangeLog
2019-10-19  Tom Tromey  <tom@tromey.com>

	* symtab.h (struct minimal_symbol) <name_set>: New member.
	* minsyms.c (minimal_symbol_reader::record_full): Copy name.
	Don't call symbol_set_names.
	(minimal_symbol_reader::install): Call symbol_set_names.

Change-Id: I4fe3993b99fb3a43968067806e294d48e377fd76
---
M gdb/ChangeLog
M gdb/minsyms.c
M gdb/symtab.h
3 files changed, 28 insertions(+), 1 deletion(-)

Comments

Simon Marchi (Code Review) Oct. 21, 2019, 1:32 p.m. UTC | #1 
Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/166
......................................................................


Patch Set 2:

(1 comment)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/166/2/gdb/minsyms.c 
File gdb/minsyms.c:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/166/2/gdb/minsyms.c@1360 
PS2, Line 1360: 	  if (!msymbols[i].name_set)
In which situation would name_set be already true here?
Simon Marchi (Code Review) Oct. 30, 2019, 9:53 p.m. UTC | #2 
Tom Tromey has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/166
......................................................................


Patch Set 2:

(1 comment)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/166/2/gdb/minsyms.c 
File gdb/minsyms.c:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/166/2/gdb/minsyms.c@1360 
PS2, Line 1360: 
1286 | minimal_symbol_reader::install ()
     | ...
1355 |       m_objfile->per_bfd->msymbols = std::move (msym_holder);
1356 | 
1357 |       msymbols = m_objfile->per_bfd->msymbols.get ();
1358 |       for (int i = 0; i < mcount; ++i)
1359 | 	{
1360 > 	  if (!msymbols[i].name_set)
1361 | 	    {
1362 | 	      symbol_set_names (&msymbols[i], msymbols[i].name,
1363 | 				strlen (msymbols[i].name), 0,
1364 | 				m_objfile->per_bfd);
1365 | 	      msymbols[i].name_set = 1;

> In which situation would name_set be already true here?

I'm under the impression that multiple symbol readers can install
minimal symbols for a given objfile.  This code is to support this
case.

However, I wonder now if this is really the case.  Looking at the
code, I think maybe not.  That would be a nice cleanup to have.
Simon Marchi (Code Review) Oct. 30, 2019, 10:51 p.m. UTC | #3 
Tom Tromey has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/166
......................................................................


Patch Set 2:

(1 comment)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/166/2/gdb/minsyms.c 
File gdb/minsyms.c:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/166/2/gdb/minsyms.c@1360 
PS2, Line 1360: 
1286 | minimal_symbol_reader::install ()
     | ...
1355 |       m_objfile->per_bfd->msymbols = std::move (msym_holder);
1356 | 
1357 |       msymbols = m_objfile->per_bfd->msymbols.get ();
1358 |       for (int i = 0; i < mcount; ++i)
1359 | 	{
1360 > 	  if (!msymbols[i].name_set)
1361 | 	    {
1362 | 	      symbol_set_names (&msymbols[i], msymbols[i].name,
1363 | 				strlen (msymbols[i].name), 0,
1364 | 				m_objfile->per_bfd);
1365 | 	      msymbols[i].name_set = 1;

> I'm under the impression that multiple symbol readers can install […]

Ok, I looked into this, and unfortunately this really can happen.

For example, elfread.c can obviously make minimal symbols itself.
After having done so, though, it can call elfstab_build_psymtabs,
which calls dbx_symfile_read, which can install minimal symbols.

Perhaps with some refactoring a single minimal_symbol_reader
could be created and then passed around.  I am not sure whether
this would really be an improvement -- it may depend on the
other minsym threading changes that Christian is experimenting
with.
Simon Marchi (Code Review) Nov. 22, 2019, 4:15 p.m. UTC | #4 
Pedro Alves has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/166
......................................................................


Patch Set 3: Code-Review+2

(2 comments)

| --- gdb/minsyms.c
| +++ gdb/minsyms.c
| @@ -1348,17 +1353,28 @@ minimal_symbol_reader::install ()
|           The strings themselves are also located in the storage_obstack
|           of this objfile.  */
|  
|        if (m_objfile->per_bfd->minimal_symbol_count != 0)
|  	clear_minimal_symbol_hash_tables (m_objfile);
|  
|        m_objfile->per_bfd->minimal_symbol_count = mcount;
|        m_objfile->per_bfd->msymbols = std::move (msym_holder);
|  
| +      msymbols = m_objfile->per_bfd->msymbols.get ();
| +      for (int i = 0; i < mcount; ++i)
| +	{
| +	  if (!msymbols[i].name_set)
| +	    {
| +	      symbol_set_names (&msymbols[i], msymbols[i].name,
| +				false, m_objfile->per_bfd);
| +	      msymbols[i].name_set = 1;
| +	    }
| +	}

PS3, Line 1371:

I was wondering whether we could do without the name_set field by
doing this symbol_set_names loop only over the new minsyms, before
appending them to the objfile's preexisting minsyms list, but I guess
the need for deduplication shows that we'd be demangling more than
necessary.

| +
|        build_minimal_symbol_hash_tables (m_objfile);
|      }
|  }
|  
|  /* Check if PC is in a shared library trampoline code stub.
|     Return minimal symbol for the trampoline entry or NULL if PC is not
|     in a trampoline code stub.  */
|  
| --- gdb/symtab.h
| +++ gdb/symtab.h
| @@ -683,19 +683,23 @@ struct minimal_symbol : public general_symbol_info
|       the object file format may not carry that piece of information.  */
|    unsigned int has_size : 1;
|  
|    /* For data symbols only, if this is set, then the symbol might be
|       subject to copy relocation.  In this case, a minimal symbol
|       matching the symbol's linkage name is first looked for in the
|       main objfile.  If found, then that address is used; otherwise the
|       address in this symbol is used.  */
|  
|    unsigned maybe_copied : 1;
|  
| +  /* Non-zero if this symbol ever had its demangled name set (even if
| +     it was set to NULL).  */
| +  unsigned int name_set : 1;

PS3, Line 696:

Don't change it, since the other fields are the same, but,

I'm wondering whether nowadays with C++ we shouldn't be writing

  bool name_set : 1;

and then use true/false instead of 0/1.

I assume that it compiles down to the same.

| +
|    /* Minimal symbols with the same hash key are kept on a linked
|       list.  This is the link.  */
|  
|    struct minimal_symbol *hash_next;
|  
|    /* Minimal symbols are stored in two different hash tables.  This is
|       the `next' pointer for the demangled hash table.  */
|
Simon Marchi (Code Review) Nov. 22, 2019, 10:28 p.m. UTC | #5 
Tom Tromey has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/166
......................................................................


Patch Set 3:

(1 comment)

| --- gdb/symtab.h
| +++ gdb/symtab.h
| @@ -683,19 +683,23 @@ struct minimal_symbol : public general_symbol_info
|       the object file format may not carry that piece of information.  */
|    unsigned int has_size : 1;
|  
|    /* For data symbols only, if this is set, then the symbol might be
|       subject to copy relocation.  In this case, a minimal symbol
|       matching the symbol's linkage name is first looked for in the
|       main objfile.  If found, then that address is used; otherwise the
|       address in this symbol is used.  */
|  
|    unsigned maybe_copied : 1;
|  
| +  /* Non-zero if this symbol ever had its demangled name set (even if
| +     it was set to NULL).  */
| +  unsigned int name_set : 1;

PS3, Line 696:

I wasn't sure if there was some caveat to using a boolean
bitfield, or not.  So, I followed the pre-existing code.
Also in the Windows ABI using the same type for bit-fields
yields better packing, which argues for switching them all
at the same time.

| +
|    /* Minimal symbols with the same hash key are kept on a linked
|       list.  This is the link.  */
|  
|    struct minimal_symbol *hash_next;
|  
|    /* Minimal symbols are stored in two different hash tables.  This is
|       the `next' pointer for the demangled hash table.  */
|
diff mbox

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 731f81c..3794946 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,12 @@ 
 2019-10-19  Tom Tromey  <tom@tromey.com>
 
+	* symtab.h (struct minimal_symbol) <name_set>: New member.
+	* minsyms.c (minimal_symbol_reader::record_full): Copy name.
+	Don't call symbol_set_names.
+	(minimal_symbol_reader::install): Call symbol_set_names.
+
+2019-10-19  Tom Tromey  <tom@tromey.com>
+
 	* configure: Rebuild.
 	* configure.ac: Don't check for sigprocmask.
 	* gdbsupport/common.m4 (GDB_AC_COMMON): Check for sigprocmask.
diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index c41e5c3..4e6bd39 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -1129,7 +1129,11 @@ 
   msymbol = &m_msym_bunch->contents[m_msym_bunch_index];
   symbol_set_language (msymbol, language_auto,
 		       &m_objfile->per_bfd->storage_obstack);
-  symbol_set_names (msymbol, name, name_len, copy_name, m_objfile->per_bfd);
+
+  if (copy_name)
+    name = (char *) obstack_copy0 (&m_objfile->per_bfd->storage_obstack,
+				   name, name_len);
+  msymbol->name = name;
 
   SET_MSYMBOL_VALUE_ADDRESS (msymbol, address);
   MSYMBOL_SECTION (msymbol) = section;
@@ -1350,6 +1354,18 @@ 
       m_objfile->per_bfd->minimal_symbol_count = mcount;
       m_objfile->per_bfd->msymbols = std::move (msym_holder);
 
+      msymbols = m_objfile->per_bfd->msymbols.get ();
+      for (int i = 0; i < mcount; ++i)
+	{
+	  if (!msymbols[i].name_set)
+	    {
+	      symbol_set_names (&msymbols[i], msymbols[i].name,
+				strlen (msymbols[i].name), 0,
+				m_objfile->per_bfd);
+	      msymbols[i].name_set = 1;
+	    }
+	}
+
       build_minimal_symbol_hash_tables (m_objfile);
     }
 }
diff --git a/gdb/symtab.h b/gdb/symtab.h
index dc65409..8552bf1 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -689,6 +689,10 @@ 
 
   unsigned maybe_copied : 1;
 
+  /* Non-zero if this symbol ever had its demangled name set (even if
+     it was set to NULL).  */
+  unsigned int name_set : 1;
+
   /* Minimal symbols with the same hash key are kept on a linked
      list.  This is the link.  */