[+,8.3] Don't derive partial_symbol from general_symbol_info

Message ID 20190424190024.28904-1-tom@tromey.com
State New, archived
Headers

Commit Message

Tom Tromey April 24, 2019, 7 p.m. UTC
  This patch partly reverts commit 8a6d42345 ("Change representation of
psymbol to flush out accessors"); specifically, it changes
partial_symbol to no longer derive from general_symbol_info.

The basic problem here is that the bcache compares objects bitwise,
and this change made it less likely that the relevant fields in the
psymbol would be fully initialized.  This could be seen by running a
test under valgrind on the Fedora-i686 buildbot.

I considered a simpler patch, namely just zeroing the psymbol's
"value" field in add_psymbol_to_bcache.  However, it wasn't clear to
me that this memset could not then be optimized away by the compiler.

Regression tested by the buildbot.  I think this should go in 8.3 as
well.

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

	* psymtab.c (psymbol_name_matches, match_partial_symbol)
	(lookup_partial_symbol, print_partial_symbols)
	(recursively_search_psymtabs, sort_pst_symbols, psymbol_hash)
	(psymbol_compare): Update.
	(add_psymbol_to_bcache): Clear the entire psymbol.
	(maintenance_check_psymtabs): Update.
	* psympriv.h (struct partial_symbol): Don't derive from
	general_symbol_info.
	<obj_section, unrelocated_address, address,
	set_unrelocated_address>: Update.
	<ginfo>: New member.
	* dwarf-index-write.c (write_psymbols, debug_names::insert)
	(debug_names::write_psymbols): Update.
---
 gdb/ChangeLog           | 15 ++++++++
 gdb/dwarf-index-write.c |  8 ++---
 gdb/psympriv.h          | 18 ++++++----
 gdb/psymtab.c           | 76 ++++++++++++++++++++++-------------------
 4 files changed, 71 insertions(+), 46 deletions(-)
  

Comments

Tom Tromey May 4, 2019, 7:42 p.m. UTC | #1
>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

Tom> This patch partly reverts commit 8a6d42345 ("Change representation of
Tom> psymbol to flush out accessors"); specifically, it changes
Tom> partial_symbol to no longer derive from general_symbol_info.

[...]
Tom> Regression tested by the buildbot.  I think this should go in 8.3 as
Tom> well.

I'm going to push this now.

Tom
  
Terekhov, Mikhail via Gdb-patches Aug. 28, 2019, 11:08 p.m. UTC | #2
>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
Tom> This patch partly reverts commit 8a6d42345 ("Change representation of
Tom> psymbol to flush out accessors"); specifically, it changes
Tom> partial_symbol to no longer derive from general_symbol_info.

Hi Tom,

I don't really understand this patch, could you elaborate a bit about how
inheriting makes it less likely that fields will be initialized? In
particular the value field, since it sounds like that's where the problem
is.

Thanks!
Christian
  
Tom Tromey Sept. 9, 2019, 4:34 p.m. UTC | #3
Tom> This patch partly reverts commit 8a6d42345 ("Change representation of
Tom> psymbol to flush out accessors"); specifically, it changes
Tom> partial_symbol to no longer derive from general_symbol_info.

Christian> I don't really understand this patch, could you elaborate a bit about how
Christian> inheriting makes it less likely that fields will be initialized? In
Christian> particular the value field, since it sounds like that's where the problem
Christian> is.

The main issue is that there is padding in the struct, and the padding
bytes can take any value.  (Also maybe there was an issue with the value
field, I don't recall.)  However, the bcache relies on the bits being
identical.

The reason the patch works is that it restores this line:

+  memset (&psymbol, 0, sizeof (psymbol));

... ensuring that the memory is zeroed.  This isn't possible when
inheritance is used.

Tom
  

Patch

diff --git a/gdb/dwarf-index-write.c b/gdb/dwarf-index-write.c
index 3f96ffdafb6..8734f99212d 100644
--- a/gdb/dwarf-index-write.c
+++ b/gdb/dwarf-index-write.c
@@ -540,7 +540,7 @@  write_psymbols (struct mapped_symtab *symtab,
     {
       struct partial_symbol *psym = *psymp;
 
-      if (psym->language == language_ada)
+      if (psym->ginfo.language == language_ada)
 	error (_("Ada is not currently supported by the index"));
 
       /* Only add a given psymbol once.  */
@@ -548,7 +548,7 @@  write_psymbols (struct mapped_symtab *symtab,
 	{
 	  gdb_index_symbol_kind kind = symbol_kind (psym);
 
-	  add_index_entry (symtab, symbol_search_name (psym),
+	  add_index_entry (symtab, symbol_search_name (&psym->ginfo),
 			   is_static, kind, cu_index);
 	}
     }
@@ -684,7 +684,7 @@  public:
     const int dwarf_tag = psymbol_tag (psym);
     if (dwarf_tag == 0)
       return;
-    const char *const name = symbol_search_name (psym);
+    const char *const name = symbol_search_name (&psym->ginfo);
     const auto insertpair
       = m_name_to_value_set.emplace (c_str_view (name),
 				     std::set<symbol_value> ());
@@ -1181,7 +1181,7 @@  private:
       {
 	struct partial_symbol *psym = *psymp;
 
-	if (psym->language == language_ada)
+	if (psym->ginfo.language == language_ada)
 	  error (_("Ada is not currently supported by the index"));
 
 	/* Only add a given psymbol once.  */
diff --git a/gdb/psympriv.h b/gdb/psympriv.h
index 7d2ce30f2e6..61d73a316b8 100644
--- a/gdb/psympriv.h
+++ b/gdb/psympriv.h
@@ -33,37 +33,43 @@ 
 /* This structure is space critical.  See space comments at the top of
    symtab.h.  */
 
-struct partial_symbol : public general_symbol_info
+struct partial_symbol
 {
   /* Return the section for this partial symbol, or nullptr if no
      section has been set.  */
   struct obj_section *obj_section (struct objfile *objfile) const
   {
-    if (section >= 0)
-      return &objfile->sections[section];
+    if (ginfo.section >= 0)
+      return &objfile->sections[ginfo.section];
     return nullptr;
   }
 
   /* Return the unrelocated address of this partial symbol.  */
   CORE_ADDR unrelocated_address () const
   {
-    return value.address;
+    return ginfo.value.address;
   }
 
   /* Return the address of this partial symbol, relocated according to
      the offsets provided in OBJFILE.  */
   CORE_ADDR address (const struct objfile *objfile) const
   {
-    return value.address + ANOFFSET (objfile->section_offsets, section);
+    return (ginfo.value.address
+	    + ANOFFSET (objfile->section_offsets, ginfo.section));
   }
 
   /* Set the address of this partial symbol.  The address must be
      unrelocated.  */
   void set_unrelocated_address (CORE_ADDR addr)
   {
-    value.address = addr;
+    ginfo.value.address = addr;
   }
 
+  /* Note that partial_symbol does not derive from general_symbol_info
+     due to the bcache.  See add_psymbol_to_bcache.  */
+
+  struct general_symbol_info ginfo;
+
   /* Name space code.  */
 
   ENUM_BITFIELD(domain_enum_tag) domain : SYMBOL_DOMAIN_BITS;
diff --git a/gdb/psymtab.c b/gdb/psymtab.c
index 584a5e97fd8..78a46997ca8 100644
--- a/gdb/psymtab.c
+++ b/gdb/psymtab.c
@@ -528,10 +528,10 @@  static bool
 psymbol_name_matches (partial_symbol *psym,
 		      const lookup_name_info &lookup_name)
 {
-  const language_defn *lang = language_def (psym->language);
+  const language_defn *lang = language_def (psym->ginfo.language);
   symbol_name_matcher_ftype *name_match
     = get_symbol_name_matcher (lang, lookup_name);
-  return name_match (symbol_search_name (psym), lookup_name, NULL);
+  return name_match (symbol_search_name (&psym->ginfo), lookup_name, NULL);
 }
 
 /* Look in PST for a symbol in DOMAIN whose name matches NAME.  Search
@@ -581,11 +581,12 @@  match_partial_symbol (struct objfile *objfile,
 	  center = bottom + (top - bottom) / 2;
 	  gdb_assert (center < top);
 
-	  enum language lang = (*center)->language;
+	  enum language lang = (*center)->ginfo.language;
 	  const char *lang_ln
 	    = lookup_name.language_lookup_name (lang).c_str ();
 
-	  if (ordered_compare (symbol_search_name (*center), lang_ln) >= 0)
+	  if (ordered_compare (symbol_search_name (&(*center)->ginfo),
+			       lang_ln) >= 0)
 	    top = center;
 	  else
 	    bottom = center + 1;
@@ -595,7 +596,7 @@  match_partial_symbol (struct objfile *objfile,
       while (top <= real_top
 	     && psymbol_name_matches (*top, lookup_name))
 	{
-	  if (symbol_matches_domain ((*top)->language,
+	  if (symbol_matches_domain ((*top)->ginfo.language,
 				     (*top)->domain, domain))
 	    return *top;
 	  top++;
@@ -609,7 +610,7 @@  match_partial_symbol (struct objfile *objfile,
     {
       for (psym = start; psym < start + length; psym++)
 	{
-	  if (symbol_matches_domain ((*psym)->language,
+	  if (symbol_matches_domain ((*psym)->ginfo.language,
 				     (*psym)->domain, domain)
 	      && psymbol_name_matches (*psym, lookup_name))
 	    return *psym;
@@ -692,7 +693,7 @@  lookup_partial_symbol (struct objfile *objfile,
 	  if (!(center < top))
 	    internal_error (__FILE__, __LINE__,
 			    _("failed internal consistency check"));
-	  if (strcmp_iw_ordered (symbol_search_name (*center),
+	  if (strcmp_iw_ordered (symbol_search_name (&(*center)->ginfo),
 				 search_name.get ()) >= 0)
 	    {
 	      top = center;
@@ -708,15 +709,17 @@  lookup_partial_symbol (struct objfile *objfile,
 
       /* For `case_sensitivity == case_sensitive_off' strcmp_iw_ordered will
 	 search more exactly than what matches SYMBOL_MATCHES_SEARCH_NAME.  */
-      while (top >= start && symbol_matches_search_name (*top, lookup_name))
+      while (top >= start && symbol_matches_search_name (&(*top)->ginfo,
+							 lookup_name))
 	top--;
 
       /* Fixup to have a symbol which matches SYMBOL_MATCHES_SEARCH_NAME.  */
       top++;
 
-      while (top <= real_top && symbol_matches_search_name (*top, lookup_name))
+      while (top <= real_top && symbol_matches_search_name (&(*top)->ginfo,
+							    lookup_name))
 	{
-	  if (symbol_matches_domain ((*top)->language,
+	  if (symbol_matches_domain ((*top)->ginfo.language,
 				     (*top)->domain, domain))
 	    return *top;
 	  top++;
@@ -730,9 +733,9 @@  lookup_partial_symbol (struct objfile *objfile,
     {
       for (psym = start; psym < start + length; psym++)
 	{
-	  if (symbol_matches_domain ((*psym)->language,
+	  if (symbol_matches_domain ((*psym)->ginfo.language,
 				     (*psym)->domain, domain)
-	      && symbol_matches_search_name (*psym, lookup_name))
+	      && symbol_matches_search_name (&(*psym)->ginfo, lookup_name))
 	    return *psym;
 	}
     }
@@ -832,10 +835,11 @@  print_partial_symbols (struct gdbarch *gdbarch, struct objfile *objfile,
   while (count-- > 0)
     {
       QUIT;
-      fprintf_filtered (outfile, "    `%s'", (*p)->name);
-      if (symbol_demangled_name (*p) != NULL)
+      fprintf_filtered (outfile, "    `%s'", (*p)->ginfo.name);
+      if (symbol_demangled_name (&(*p)->ginfo) != NULL)
 	{
-	  fprintf_filtered (outfile, "  `%s'", symbol_demangled_name (*p));
+	  fprintf_filtered (outfile, "  `%s'",
+			    symbol_demangled_name (&(*p)->ginfo));
 	}
       fputs_filtered (", ", outfile);
       switch ((*p)->domain)
@@ -1305,7 +1309,8 @@  recursively_search_psymtabs
 	       || (domain == TYPES_DOMAIN
 		   && (*psym)->aclass == LOC_TYPEDEF))
 	      && psymbol_name_matches (*psym, lookup_name)
-	      && (sym_matcher == NULL || sym_matcher (symbol_search_name (*psym))))
+	      && (sym_matcher == NULL
+		  || sym_matcher (symbol_search_name (&(*psym)->ginfo))))
 	    {
 	      /* Found a match, so notify our caller.  */
 	      result = PST_SEARCHED_AND_FOUND;
@@ -1500,8 +1505,8 @@  sort_pst_symbols (struct objfile *objfile, struct partial_symtab *pst)
 
   std::sort (begin, end, [] (partial_symbol *s1, partial_symbol *s2)
     {
-      return strcmp_iw_ordered (symbol_search_name (s1),
-				symbol_search_name (s2)) < 0;
+      return strcmp_iw_ordered (symbol_search_name (&s1->ginfo),
+				symbol_search_name (&s2->ginfo)) < 0;
     });
 }
 
@@ -1548,18 +1553,18 @@  psymbol_hash (const void *addr, int length)
 {
   unsigned long h = 0;
   struct partial_symbol *psymbol = (struct partial_symbol *) addr;
-  unsigned int lang = psymbol->language;
+  unsigned int lang = psymbol->ginfo.language;
   unsigned int domain = psymbol->domain;
   unsigned int theclass = psymbol->aclass;
 
-  h = hash_continue (&psymbol->value, sizeof (psymbol->value), h);
+  h = hash_continue (&psymbol->ginfo.value, sizeof (psymbol->ginfo.value), h);
   h = hash_continue (&lang, sizeof (unsigned int), h);
   h = hash_continue (&domain, sizeof (unsigned int), h);
   h = hash_continue (&theclass, sizeof (unsigned int), h);
   /* Note that psymbol names are interned via symbol_set_names, so
      there's no need to hash the contents of the name here.  */
-  h = hash_continue (&psymbol->name,
-		     sizeof (psymbol->name), h);
+  h = hash_continue (&psymbol->ginfo.name,
+		     sizeof (psymbol->ginfo.name), h);
 
   return h;
 }
@@ -1574,15 +1579,15 @@  psymbol_compare (const void *addr1, const void *addr2, int length)
   struct partial_symbol *sym1 = (struct partial_symbol *) addr1;
   struct partial_symbol *sym2 = (struct partial_symbol *) addr2;
 
-  return (memcmp (&sym1->value, &sym2->value,
-                  sizeof (sym1->value)) == 0
-	  && sym1->language == sym2->language
+  return (memcmp (&sym1->ginfo.value, &sym2->ginfo.value,
+                  sizeof (sym1->ginfo.value)) == 0
+	  && sym1->ginfo.language == sym2->ginfo.language
           && sym1->domain == sym2->domain
           && sym1->aclass == sym2->aclass
 	  /* Note that psymbol names are interned via
 	     symbol_set_names, so there's no need to compare the
 	     contents of the name here.  */
-          && sym1->name == sym2->name);
+          && sym1->ginfo.name == sym2->ginfo.name);
 }
 
 /* Helper function, initialises partial symbol structure and stashes
@@ -1601,17 +1606,16 @@  add_psymbol_to_bcache (const char *name, int namelength, int copy_name,
 		       int *added)
 {
   struct partial_symbol psymbol;
+  memset (&psymbol, 0, sizeof (psymbol));
 
   psymbol.set_unrelocated_address (coreaddr);
-  psymbol.section = section;
+  psymbol.ginfo.section = section;
   psymbol.domain = domain;
   psymbol.aclass = theclass;
-
-  memset (&psymbol.language_specific, 0, sizeof (psymbol.language_specific));
-  psymbol.ada_mangled = 0;
-  symbol_set_language (&psymbol, language,
+  symbol_set_language (&psymbol.ginfo, language,
 		       objfile->partial_symtabs->obstack ());
-  symbol_set_names (&psymbol, name, namelength, copy_name, objfile->per_bfd);
+  symbol_set_names (&psymbol.ginfo, name, namelength, copy_name,
+		    objfile->per_bfd);
 
   /* Stash the partial symbol away in the cache.  */
   return ((struct partial_symbol *)
@@ -2133,13 +2137,13 @@  maintenance_check_psymtabs (const char *ignore, int from_tty)
 	length = ps->n_static_syms;
 	while (length--)
 	  {
-	    sym = block_lookup_symbol (b, symbol_search_name (*psym),
+	    sym = block_lookup_symbol (b, symbol_search_name (&(*psym)->ginfo),
 				       symbol_name_match_type::SEARCH_NAME,
 				       (*psym)->domain);
 	    if (!sym)
 	      {
 		printf_filtered ("Static symbol `");
-		puts_filtered ((*psym)->name);
+		puts_filtered ((*psym)->ginfo.name);
 		printf_filtered ("' only found in ");
 		puts_filtered (ps->filename);
 		printf_filtered (" psymtab\n");
@@ -2151,13 +2155,13 @@  maintenance_check_psymtabs (const char *ignore, int from_tty)
 	length = ps->n_global_syms;
 	while (length--)
 	  {
-	    sym = block_lookup_symbol (b, symbol_search_name (*psym),
+	    sym = block_lookup_symbol (b, symbol_search_name (&(*psym)->ginfo),
 				       symbol_name_match_type::SEARCH_NAME,
 				       (*psym)->domain);
 	    if (!sym)
 	      {
 		printf_filtered ("Global symbol `");
-		puts_filtered ((*psym)->name);
+		puts_filtered ((*psym)->ginfo.name);
 		printf_filtered ("' only found in ");
 		puts_filtered (ps->filename);
 		printf_filtered (" psymtab\n");