public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Tom Tromey <tromey@redhat.com>
To: gdb-patches@sourceware.org
Subject: FYI: change type searching
Date: Wed, 01 Sep 2010 22:22:00 -0000	[thread overview]
Message-ID: <m3wrr59m1v.fsf@fleche.redhat.com> (raw)

I'm checking this in.

I noticed a bad bug when using a DWARF index.  If you search for the
type "char" (in my case it was being done via gdb.lookup_type), gdb
would happily expand all symtabs before returning the answer.

This happens because lookup_typename uses lookup_symbol, which first
searches for public symbols.  The index doesn't record this bit of info,
and since "char" is defined pretty much everywhere, it eagerly expands
each symtab, only to find that the public "char" isn't defined there.

All of this searching is pointless, though, because types are made
static, not public.

This patch fixes the situation first by changing the quick method to
return on the first match, and second by avoiding the useless lookups
when searching for a type.

IMO it would be nice if we had a domain specifically for types.  I
looked at this a little but it seems pretty invasive, so I didn't
attempt it.

Built and regtested on x86-64 (compile farm).
I also ran gdb locally to verify that the eager expansion no longer
happens.

Tom

2010-09-01  Tom Tromey  <tromey@redhat.com>

	* symtab.h (lookup_type_symbol): Declare.
	* symtab.c (lookup_symbol_in_language_full): Rename from
	lookup_symbol_in_language.  Add 'for_type' argument.
	(lookup_symbol_in_language): New function.
	(lookup_type_symbol): Likewise.
	(lookup_symbol_aux): Add 'for_type' argument.
	(match_symbol_aux): New function.
	(lookup_symbol_aux_symtabs): Use expand_one_symtab_matching.
	(match_transparent_type): New function.
	(basic_lookup_transparent_type): Use expand_one_symtab_matching.
	* symfile.h (struct quick_symbol_functions)
	<pre_expand_symtabs_matching>: Remove.
	<expand_one_symtab_matching>: New field.
	* psymtab.c (expand_one_symtab_matching_psymtabs): New function.
	(pre_expand_symtabs_matching_psymtabs): Remove.
	(psym_functions): Update.
	* gdbtypes.c (lookup_typename): Use lookup_type_symbol.
	* dwarf2read.c (dw2_lookup_symbol): Update comment.
	(dw2_pre_expand_symtabs_matching): Remove.
	(dw2_expand_one_symtab_matching): New function.
	(dwarf2_gdb_index_functions): Update.

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 78491c8..c5dc1a5 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -2140,7 +2140,7 @@ static struct symtab *
 dw2_lookup_symbol (struct objfile *objfile, int block_index,
 		   const char *name, domain_enum domain)
 {
-  /* We do all the work in the pre_expand_symtabs_matching hook
+  /* We do all the work in the expand_one_symtab_matching hook
      instead.  */
   return NULL;
 }
@@ -2171,12 +2171,46 @@ dw2_do_expand_symtabs_matching (struct objfile *objfile, const char *name)
     }
 }
 
-static void
-dw2_pre_expand_symtabs_matching (struct objfile *objfile,
-				 int kind, const char *name,
-				 domain_enum domain)
+static struct symbol *
+dw2_expand_one_symtab_matching (struct objfile *objfile,
+				int kind, const char *name,
+				domain_enum domain,
+				struct symbol *(*matcher) (struct symtab *,
+							   int,
+							   const char *,
+							   domain_enum,
+							   void *),
+				void *data)
 {
-  dw2_do_expand_symtabs_matching (objfile, name);
+  dw2_setup (objfile);
+
+  if (dwarf2_per_objfile->index_table)
+    {
+      offset_type *vec;
+
+      if (find_slot_in_mapped_hash (dwarf2_per_objfile->index_table,
+				    name, &vec))
+	{
+	  offset_type i, len = MAYBE_SWAP (*vec);
+	  for (i = 0; i < len; ++i)
+	    {
+	      offset_type cu_index = MAYBE_SWAP (vec[i + 1]);
+	      struct dwarf2_per_cu_data *cu = dw2_get_cu (cu_index);
+	      struct symtab *symtab;
+	      struct symbol *sym;
+
+	      if (cu->v.quick->symtab)
+		continue;
+
+	      symtab = dw2_instantiate_symtab (objfile, cu);
+	      sym = matcher (symtab, kind, name, domain, data);
+	      if (sym)
+		return sym;
+	    }
+	}
+    }
+
+  return NULL;
 }
 
 static void
@@ -2479,7 +2513,7 @@ const struct quick_symbol_functions dwarf2_gdb_index_functions =
   dw2_forget_cached_source_info,
   dw2_lookup_symtab,
   dw2_lookup_symbol,
-  dw2_pre_expand_symtabs_matching,
+  dw2_expand_one_symtab_matching,
   dw2_print_stats,
   dw2_dump,
   dw2_relocate,
@@ -5940,9 +5974,9 @@ dwarf2_attach_fields_to_type (struct field_info *fip, struct type *type,
 	(B_TYPE *) TYPE_ALLOC (type, B_BYTES (nfields));
       B_CLRALL (TYPE_FIELD_PROTECTED_BITS (type), nfields);
 
-      TYPE_FIELD_IGNORE_BITS (type) =
-	(B_TYPE *) TYPE_ALLOC (type, B_BYTES (nfields));
-      B_CLRALL (TYPE_FIELD_IGNORE_BITS (type), nfields);
+      /* We don't set TYPE_FIELD_IGNORE_BITS here.  The DWARF reader
+	 never sets any bits in that array, so leaving it NULL lets us
+	 save a little memory.  */
     }
 
   /* If the type has baseclasses, allocate and clear a bit vector for
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index da4d673..882d245 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -1051,7 +1051,7 @@ lookup_typename (const struct language_defn *language,
   struct symbol *sym;
   struct type *tmp;
 
-  sym = lookup_symbol (name, block, VAR_DOMAIN, 0);
+  sym = lookup_type_symbol (name, block, VAR_DOMAIN, language->la_language);
   if (sym == NULL || SYMBOL_CLASS (sym) != LOC_TYPEDEF)
     {
       tmp = language_lookup_primitive_type_by_name (language, gdbarch, name);
diff --git a/gdb/psymtab.c b/gdb/psymtab.c
index bc47681..44ccb0f 100644
--- a/gdb/psymtab.c
+++ b/gdb/psymtab.c
@@ -421,12 +421,18 @@ lookup_symbol_aux_psymtabs (struct objfile *objfile,
   return NULL;
 }
 
-static void
-pre_expand_symtabs_matching_psymtabs (struct objfile *objfile,
-				      int kind, const char *name,
-				      domain_enum domain)
+static struct symbol *
+expand_one_symtab_matching_psymtabs (struct objfile *objfile,
+				     int kind, const char *name,
+				     domain_enum domain,
+				     struct symbol *(*matcher) (struct symtab *,
+								int,
+								const char *,
+								domain_enum,
+								void *),
+				     void *data)
 {
-  /* Nothing.  */
+  return NULL;
 }
 
 /* Look, in partial_symtab PST, for symbol whose natural name is NAME.
@@ -1207,7 +1213,7 @@ const struct quick_symbol_functions psym_functions =
   forget_cached_source_info_partial,
   lookup_symtab_via_partial_symtab,
   lookup_symbol_aux_psymtabs,
-  pre_expand_symtabs_matching_psymtabs,
+  expand_one_symtab_matching_psymtabs,
   print_psymtab_stats_for_objfile,
   dump_psymtabs_for_objfile,
   relocate_psymtabs,
diff --git a/gdb/symfile.h b/gdb/symfile.h
index 5815354..84b770d 100644
--- a/gdb/symfile.h
+++ b/gdb/symfile.h
@@ -171,14 +171,25 @@ struct quick_symbol_functions
 				   int kind, const char *name,
 				   domain_enum domain);
 
-  /* This is called to expand symbol tables before looking up a
-     symbol.  A backend can choose to implement this and then have its
-     `lookup_symbol' hook always return NULL, or the reverse.  (It
-     doesn't make sense to implement both.)  The arguments are as for
-     `lookup_symbol'.  */
-  void (*pre_expand_symtabs_matching) (struct objfile *objfile,
-				       int kind, const char *name,
-				       domain_enum domain);
+  /* Expand each symbol table in OBJFILE that may have items matching
+     KIND, NAME, and DOMAIN -- these arguments are as for
+     `lookup_symbol'.  For each such symbol table, call MATCHER with
+     the symbol table and DATA arguments.  If MATCHER returns NULL,
+     keep going.  Otherwise, return the result of MATCHER.  If MATCHER
+     never returns non-NULL, return NULL.  A backend can choose to
+     implement this and then have its `lookup_symbol' hook always
+     return NULL, or the reverse.  (It doesn't make sense to implement
+     both.)  */
+  struct symbol *(*expand_one_symtab_matching)
+    (struct objfile *objfile,
+     int kind, const char *name,
+     domain_enum domain,
+     struct symbol *(*matcher) (struct symtab *symtab,
+				int kind,
+				const char *name,
+				domain_enum domain,
+				void *data),
+     void *data);
 
   /* Print statistics about any indices loaded for OBJFILE.  The
      statistics should be printed to gdb_stdout.  This is used for
diff --git a/gdb/symtab.c b/gdb/symtab.c
index d43d573..5bcd315 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -91,7 +91,8 @@ static struct symbol *lookup_symbol_aux (const char *name,
 					 const struct block *block,
 					 const domain_enum domain,
 					 enum language language,
-					 int *is_a_field_of_this);
+					 int *is_a_field_of_this,
+					 int for_type);
 
 static
 struct symbol *lookup_symbol_aux_local (const char *name,
@@ -994,6 +995,8 @@ fixup_symbol_section (struct symbol *sym, struct objfile *objfile)
    C++: if IS_A_FIELD_OF_THIS is nonzero on entry, check to see if
    NAME is a field of the current implied argument `this'.  If so set
    *IS_A_FIELD_OF_THIS to 1, otherwise set it to zero.
+   FOR_TYPE is non-zero if searching specifically for a type; zero
+   otherwise.
    BLOCK_FOUND is set to the block in which NAME is found (in the case of
    a field of `this', value_of_this sets BLOCK_FOUND to the proper value.) */
 
@@ -1007,10 +1010,10 @@ fixup_symbol_section (struct symbol *sym, struct objfile *objfile)
    variable and thus can probably assume it will never hit the C++
    code).  */
 
-struct symbol *
-lookup_symbol_in_language (const char *name, const struct block *block,
-			   const domain_enum domain, enum language lang,
-			   int *is_a_field_of_this)
+static struct symbol *
+lookup_symbol_in_language_full (const char *name, const struct block *block,
+				const domain_enum domain, enum language lang,
+				int *is_a_field_of_this, int for_type)
 {
   char *demangled_name = NULL;
   const char *modified_name = NULL;
@@ -1075,12 +1078,41 @@ lookup_symbol_in_language (const char *name, const struct block *block,
     }
 
   returnval = lookup_symbol_aux (modified_name, block, domain, lang,
-				 is_a_field_of_this);
+				 is_a_field_of_this, for_type);
   do_cleanups (cleanup);
 
   return returnval;
 }
 
+/* Find the definition for a specified symbol name NAME
+   in domain DOMAIN, visible from lexical block BLOCK.
+   Returns the struct symbol pointer, or zero if no symbol is found.
+   C++: if IS_A_FIELD_OF_THIS is nonzero on entry, check to see if
+   NAME is a field of the current implied argument `this'.  If so set
+   *IS_A_FIELD_OF_THIS to 1, otherwise set it to zero.
+   BLOCK_FOUND is set to the block in which NAME is found (in the case of
+   a field of `this', value_of_this sets BLOCK_FOUND to the proper value.) */
+
+struct symbol *
+lookup_symbol_in_language (const char *name, const struct block *block,
+			   const domain_enum domain, enum language lang,
+			   int *is_a_field_of_this)
+{
+  return lookup_symbol_in_language_full (name, block, domain, lang,
+					 is_a_field_of_this, 0);
+}
+
+/* Like lookup_symbol_in_language, but search specifically for a
+   type.  */
+
+struct symbol *
+lookup_type_symbol (const char *name, const struct block *block,
+		    const domain_enum domain, enum language lang)
+{
+  return lookup_symbol_in_language_full (name, block, domain, lang,
+					 NULL, 1);
+}
+
 /* Behave like lookup_symbol_in_language, but performed with the
    current language.  */
 
@@ -1101,7 +1133,8 @@ lookup_symbol (const char *name, const struct block *block,
 static struct symbol *
 lookup_symbol_aux (const char *name, const struct block *block,
 		   const domain_enum domain, enum language language,
-		   int *is_a_field_of_this)
+		   int *is_a_field_of_this,
+		   int for_type)
 {
   struct symbol *sym;
   const struct language_defn *langdef;
@@ -1165,14 +1198,20 @@ lookup_symbol_aux (const char *name, const struct block *block,
     }
 
   /* Now do whatever is appropriate for LANGUAGE to look
-     up static and global variables.  */
+     up static and global variables.  If we are searching for a type,
+     we bypass this lookup, because types aren't global.  */
 
-  sym = langdef->la_lookup_symbol_nonlocal (name, block, domain);
-  if (sym != NULL)
-    return sym;
+  if (!for_type)
+    {
+      sym = langdef->la_lookup_symbol_nonlocal (name, block, domain);
+      if (sym != NULL)
+	return sym;
+    }
 
-  /* Now search all static file-level symbols.  Not strictly correct,
-     but more useful than an error.  */
+  /* Now search all static file-level symbols.  When searching for a
+     type, this is what we generally want, because types are put into
+     the file scope.  For other objects, not strictly correct, but
+     more useful than an error.  */
 
   return lookup_static_symbol_aux (name, domain);
 }
@@ -1327,6 +1366,35 @@ lookup_global_symbol_from_objfile (const struct objfile *main_objfile,
   return NULL;
 }
 
+/* A helper for lookup_symbol_aux_symtabs that is passed as a callback
+   to the expand_one_symtab_matching quick function.  */
+
+static struct symbol *
+match_symbol_aux (struct symtab *symtab,
+		  int kind, const char *name, domain_enum domain,
+		  void *arg)
+{
+  struct objfile *objfile = arg;
+
+  if (symtab->primary)
+    {
+      struct symbol *sym;
+      struct blockvector *bv;
+      const struct block *block;
+
+      bv = BLOCKVECTOR (symtab);
+      block = BLOCKVECTOR_BLOCK (bv, kind);
+      sym = lookup_block_symbol (block, name, domain);
+      if (sym)
+	{
+	  block_found = block;
+	  return fixup_symbol_section (sym, objfile);
+	}
+    }
+
+  return NULL;
+}
+
 /* Check to see if the symbol is defined in one of the symtabs.
    BLOCK_INDEX should be either GLOBAL_BLOCK or STATIC_BLOCK,
    depending on whether or not we want to search global symbols or
@@ -1344,11 +1412,6 @@ lookup_symbol_aux_symtabs (int block_index, const char *name,
 
   ALL_OBJFILES (objfile)
   {
-    if (objfile->sf)
-      objfile->sf->qf->pre_expand_symtabs_matching (objfile,
-						    block_index,
-						    name, domain);
-
     ALL_OBJFILE_SYMTABS (objfile, s)
       if (s->primary)
 	{
@@ -1361,6 +1424,17 @@ lookup_symbol_aux_symtabs (int block_index, const char *name,
 	      return fixup_symbol_section (sym, objfile);
 	    }
 	}
+
+    if (objfile->sf)
+      {
+	sym = objfile->sf->qf->expand_one_symtab_matching (objfile,
+							   block_index,
+							   name, domain,
+							   match_symbol_aux,
+							   objfile);
+	if (sym)
+	  return sym;
+      }
   }
 
   return NULL;
@@ -1582,6 +1656,30 @@ basic_lookup_transparent_type_quick (struct objfile *objfile, int kind,
   return NULL;
 }
 
+/* A helper function for basic_lookup_transparent_type that is passed
+   to the expand_one_symtab_matching quick function.  */
+
+static struct symbol *
+match_transparent_type (struct symtab *symtab,
+			int kind, const char *name, domain_enum domain,
+			void *data)
+{
+  if (symtab->primary)
+    {
+      struct blockvector *bv;
+      struct block *block;
+      struct symbol *sym;
+
+      bv = BLOCKVECTOR (symtab);
+      block = BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK);
+      sym = lookup_block_symbol (block, name, STRUCT_DOMAIN);
+      if (sym && !TYPE_IS_OPAQUE (SYMBOL_TYPE (sym)))
+	return sym;
+    }
+
+  return NULL;
+}
+
 /* The standard implementation of lookup_transparent_type.  This code
    was modeled on lookup_symbol -- the parts not relevant to looking
    up types were just left out.  In particular it's assumed here that
@@ -1605,11 +1703,6 @@ basic_lookup_transparent_type (const char *name)
 
   ALL_OBJFILES (objfile)
   {
-    if (objfile->sf)
-      objfile->sf->qf->pre_expand_symtabs_matching (objfile,
-						    GLOBAL_BLOCK,
-						    name, STRUCT_DOMAIN);
-
     ALL_OBJFILE_SYMTABS (objfile, s)
       if (s->primary)
 	{
@@ -1621,6 +1714,18 @@ basic_lookup_transparent_type (const char *name)
 	      return SYMBOL_TYPE (sym);
 	    }
 	}
+
+    if (objfile->sf)
+      {
+	sym
+	  = objfile->sf->qf->expand_one_symtab_matching (objfile,
+							 GLOBAL_BLOCK, name,
+							 STRUCT_DOMAIN,
+							 match_transparent_type,
+							 NULL);
+	if (sym)
+	  return SYMBOL_TYPE (sym);
+      }
   }
 
   ALL_OBJFILES (objfile)
@@ -1640,10 +1745,6 @@ basic_lookup_transparent_type (const char *name)
 
   ALL_OBJFILES (objfile)
   {
-    if (objfile->sf)
-      objfile->sf->qf->pre_expand_symtabs_matching (objfile, STATIC_BLOCK,
-						    name, STRUCT_DOMAIN);
-
     ALL_OBJFILE_SYMTABS (objfile, s)
       {
 	bv = BLOCKVECTOR (s);
@@ -1654,6 +1755,18 @@ basic_lookup_transparent_type (const char *name)
 	    return SYMBOL_TYPE (sym);
 	  }
       }
+
+    if (objfile->sf)
+      {
+	sym
+	  = objfile->sf->qf->expand_one_symtab_matching (objfile,
+							 STATIC_BLOCK, name,
+							 STRUCT_DOMAIN,
+							 match_transparent_type,
+							 NULL);
+	if (sym)
+	  return SYMBOL_TYPE (sym);
+      }
   }
 
   ALL_OBJFILES (objfile)
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 04cb443..f96c3f4 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -977,6 +977,10 @@ extern int find_pc_line_pc_range (CORE_ADDR, CORE_ADDR *, CORE_ADDR *);
 
 extern void reread_symbols (void);
 
+extern struct symbol *lookup_type_symbol (const char* name,
+					  const struct block *block,
+					  const domain_enum domain,
+					  enum language lang);
 extern struct type *lookup_transparent_type (const char *);
 extern struct type *basic_lookup_transparent_type (const char *);
 

             reply	other threads:[~2010-09-01 21:44 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-09-01 22:22 Tom Tromey [this message]
2010-09-06  8:55 ` Daniel Jacobowitz
2010-09-08 17:15   ` Tom Tromey
2010-09-08 17:52     ` Tom Tromey

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=m3wrr59m1v.fsf@fleche.redhat.com \
    --to=tromey@redhat.com \
    --cc=gdb-patches@sourceware.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).