[review v3] Defer minimal symbol name-setting

["Pedro Alves (Code Review)" <gerrit@gnutoolchain-gerrit.osci.io>]

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.  */
|  

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I4fe3993b99fb3a43968067806e294d48e377fd76
Gerrit-Change-Number: 166
Gerrit-PatchSet: 3
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Fri, 22 Nov 2019 16:15:21 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment