[3/3] Fix gdb.base/completion.exp with --target_board=dwarf4-gdb-index

  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 | 42 ++++++++----------------------------------
 1 file changed, 8 insertions(+), 34 deletions(-)
  

Pedro Alves <palves@redhat.com> writes:

Hi Pedro,
I don't know much about symbtab,

> -  /* 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, [&] (struct symtab *symtab)
>      {
> -      /* 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);

This is removed, do we have something equivalent in iterate_over_symtabs?

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



> -
> -  b = BLOCKVECTOR_BLOCK (SYMTAB_BLOCKVECTOR (s), GLOBAL_BLOCK);
> -  ALL_BLOCK_SYMBOLS (b, iter, sym)
> -    {
> -      COMPLETION_LIST_ADD_SYMBOL (sym, sym_text, sym_text_len, text, word);
> -    }
> -
> -  b = BLOCKVECTOR_BLOCK (SYMTAB_BLOCKVECTOR (s), STATIC_BLOCK);
> -  ALL_BLOCK_SYMBOLS (b, iter, sym)
> -    {
> -      COMPLETION_LIST_ADD_SYMBOL (sym, sym_text, sym_text_len, text, word);
> -    }
> +      add_symtab_completions (symtab->compunit_symtab,

use SYMTAB_COMPUNIT (s)

> +			      sym_text, sym_text_len,
> +			      text, word, TYPE_CODE_UNDEF);

Can you split this part out this patch as a pure refactor?
  

On 02/23/2017 04:02 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
> Hi Pedro,
> I don't know much about symbtab,
> 
>> -  /* 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, [&] (struct symtab *symtab)
>>      {
>> -      /* 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);
> 
> This is removed, do we have something equivalent in iterate_over_symtabs?

AFAICS, yes, though via other means.

iterate_over_some_symtabs matches both the symtab name, and if that
fails, tries the symtab's resolved fullname.

The path actually fixes a bug related to this, it seems.

Before the patch, any random absolute path would match, like:

 (gdb) b /silly/path/asdfasdfsdafasdfasdfsadfasdfasdfasdfasdfasdf/test.c:<tab>
 a       foo     int     main()  

But then actually setting the breakpoint would fail:

 (gdb) b /silly/path/asdfasdfsdafasdfasdfsadfasdfasdfasdfasdfasdf/test.c:main
 No source file named /silly/path/asdfasdfsdafasdfasdfsadfasdfasdfasdfasdfasdf/test.c.
 Make breakpoint pending on future shared library load? (y or [n]) n

While after the patch, only full paths that gdb can resolve in
find_and_open_source, according to the source lookup rules, match for
completion.  I.e., in my quick test, completing "/home/pedro/tmp/test.c:",
which is the source file's real path, matches for completion, but the
silly path above does not.

If I move the source file elsewhere and use the "dir" command to
point GDB at it, completion now finds it there:

 (gdb) b /home/pedro/tmp2/test.c:<tab>
 #nothing
 (gdb) dir /home/pedro/tmp2/
 Source directories searched: /home/pedro/tmp2:$cdir:$cwd
 (gdb) b /home/pedro/tmp2/test.c:
 a       foo     int     main()  
 (gdb) b /home/pedro/tmp/dirs/ctor.cc:main
 Note: breakpoint 1 also set at pc 0x4005ba.

So this patch makes completion consistent with the rest of
gdb, which seems like a desirable change.

I'll try to come up with a test case for this.

Thanks,
Pedro Alves
  

So meanwhile I've pushed in patches #1 and #2.  That leaves the bug
fixing, now split in two patches.

On 02/23/2017 04:02 PM, Yao Qi wrote:
>> > +      add_symtab_completions (symtab->compunit_symtab,
> use SYMTAB_COMPUNIT (s)
> 
>> > +			      sym_text, sym_text_len,
>> > +			      text, word, TYPE_CODE_UNDEF);
> Can you split this part out this patch as a pure refactor?

Sure thing.  I'll send them as replies to this email.

Pedro Alves (2):
  symtab.c: Small refactor
  Fix gdb.base/completion.exp with --target_board=dwarf4-gdb-index

 gdb/symtab.c | 42 ++++++++----------------------------------
 1 file changed, 8 insertions(+), 34 deletions(-)