[v2,2/2] Fix gdb.base/completion.exp with --target_board=dwarf4-gdb-index

Message ID 1487870642-29926-3-git-send-email-palves@redhat.com
State New, archived
Headers

Commit Message

Pedro Alves Feb. 23, 2017, 5:24 p.m. UTC
  This patch fixes:

 -FAIL: gdb.base/completion.exp: tab complete break break.c:ma (timeout)
 -FAIL: gdb.base/completion.exp: complete break break.c:ma
 +PASS: gdb.base/completion.exp: tab complete break break.c:ma
 +PASS: gdb.base/completion.exp: delete breakpoint for tab complete break break.c:ma
 +PASS: gdb.base/completion.exp: complete break break.c:ma

When run with --target_board=dwarf4-gdb-index.

The issue here is that make_file_symbol_completion_list_1, used when
completing a symbol restricted to a given source file, uses
lookup_symtab to look up the symtab with the given name, and search
for matching symbols inside.  This assumes that there's only one
symtab for the given source file.  This is an incorrect assumption
with (for example) -fdebug-types-section, where we'll have an extra
extra symtab containing the types.  lookup_symtab finds that symtab,
and inside that symtab there are no functions...

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* symtab.c (make_file_symbol_completion_list_1): Iterate over
	symtabs matching all symtabs with SRCFILE as file name instead of
	only considering the first hit, with lookup_symtab.
---
 gdb/symtab.c | 30 ++++++++----------------------
 1 file changed, 8 insertions(+), 22 deletions(-)
  

Comments

Yao Qi Feb. 24, 2017, 3:34 p.m. UTC | #1
Pedro Alves <palves@redhat.com> writes:

> -  /* Find the symtab for SRCFILE (this loads it if it was not yet read
> -     in).  */
> -  s = lookup_symtab (srcfile);
> -  if (s == NULL)
> +  /* Go through symtabs for SRCFILE and check the externs and statics
> +     for symbols which match.  */
> +  iterate_over_symtabs (srcfile, [&] (symtab *s)
>      {
> -      /* Maybe they typed the file with leading directories, while the
> -	 symbol tables record only its basename.  */
> -      const char *tail = lbasename (srcfile);
> -
> -      if (tail > srcfile)
> -	s = lookup_symtab (tail);
> -    }
> -
> -  /* If we have no symtab for that file, return an empty list.  */
> -  if (s == NULL)
> -    return (return_val);

In the original code, lookup_symtab is called twice, and if we inline
lookup_symtab here, the change in this patch is more readable.

Let us inline lookup_symtab first, the code becomes,

  s = NULL;
  iterate_over_symtabs (srcfile, [&] (symtab *symtab)
    {
      s = symtab;

      add_symtab_completions (SYMTAB_COMPUNIT (s),
			      sym_text, sym_text_len,
			      text, word, TYPE_CODE_UNDEF);
      return true;
    });

  if (s == NULL)
    {
      /* Maybe they typed the file with leading directories, while the
	 symbol tables record only its basename.  */
      const char *tail = lbasename (srcfile);

      if (tail > srcfile)
	iterate_over_symtabs (tail, [&] (symtab *symtab)
			      {
				s = symtab;

				add_symtab_completions (SYMTAB_COMPUNIT (s),
							sym_text, sym_text_len,
							text, word, TYPE_CODE_UNDEF);
				return true;
			      });
    }


then, as you described in commit log, we have to iterate all symtabs
rather than stop on the first matched symtab.  We need to replace
"return true;" with "return false;" above.  Presumably, this replacement
will fix the fails in completion.exp.

Then, it turns out that the whole block "if (s == NULL) {...}" is
removed by this patch.  I'll dig deep to see this block is still needed
or not.
  

Patch

diff --git a/gdb/symtab.c b/gdb/symtab.c
index c0fd0fd..e1a9b8f 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -5391,7 +5391,6 @@  static VEC (char_ptr) *
 make_file_symbol_completion_list_1 (const char *text, const char *word,
 				    const char *srcfile)
 {
-  struct symtab *s;
   /* The symbol we are completing on.  Points in same buffer as text.  */
   const char *sym_text;
   /* Length of sym_text.  */
@@ -5442,28 +5441,15 @@  make_file_symbol_completion_list_1 (const char *text, const char *word,
 
   sym_text_len = strlen (sym_text);
 
-  /* Find the symtab for SRCFILE (this loads it if it was not yet read
-     in).  */
-  s = lookup_symtab (srcfile);
-  if (s == NULL)
+  /* Go through symtabs for SRCFILE and check the externs and statics
+     for symbols which match.  */
+  iterate_over_symtabs (srcfile, [&] (symtab *s)
     {
-      /* Maybe they typed the file with leading directories, while the
-	 symbol tables record only its basename.  */
-      const char *tail = lbasename (srcfile);
-
-      if (tail > srcfile)
-	s = lookup_symtab (tail);
-    }
-
-  /* If we have no symtab for that file, return an empty list.  */
-  if (s == NULL)
-    return (return_val);
-
-  /* Go through this symtab and check the externs and statics for
-     symbols which match.  */
-  add_symtab_completions (SYMTAB_COMPUNIT (s),
-			  sym_text, sym_text_len,
-			  text, word, TYPE_CODE_UNDEF);
+      add_symtab_completions (SYMTAB_COMPUNIT (s),
+			      sym_text, sym_text_len,
+			      text, word, TYPE_CODE_UNDEF);
+      return false;
+    });
 
   return (return_val);
 }