[RFC] Cleanup for make_source_files_completion_list

[RFC] Cleanup for make_source_files_completion_list

[Keith Seitz]

Hi,

make_source_files_completion_list is only ever called with the first 
parameter twice, e.g.,

   fn_list = make_source_files_completion_list (text, text);

Is there any reason to not whack the second parameter as the following 
patch does?

Keith

ChangeLog
2013-05-07  Keith Seitz  <keiths@redhat.com>

	* symtab.h (make_source_files_completion_list): Remove unused
	second parameter.
	* completer.c (location_completer): Update call to
	make_source_files_completion_list.
	* symtab.c (maybe_add_partial_symtab_filename): Likewise.
	(make_source_files_completion_list): Remove unused second
	parameter.
	Pass text to add_filename_to_list instead of word.
	Set datum.word to text, too.

diff --git a/gdb/symtab.h b/gdb/symtab.h
index c735e16..8703f77 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -1241,8 +1241,7 @@ extern VEC (char_ptr) 
*make_file_symbol_completion_list (const char *,
  							 const char *,
  							 const char *);

-extern VEC (char_ptr) *make_source_files_completion_list (const char *,
-							  const char *);
+extern VEC (char_ptr) *make_source_files_completion_list (const char *);

  /* symtab.c */

diff --git a/gdb/completer.c b/gdb/completer.c
index e132651..b4845cb 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -270,7 +270,7 @@ location_completer (struct cmd_list_element *ignore,
  	 name, they cannot be asking for completion on files.  */
        if (strcspn (text,
  		   gdb_completer_file_name_break_characters) == text_len)
-	fn_list = make_source_files_completion_list (text, text);
+	fn_list = make_source_files_completion_list (text);
      }

    n_syms = VEC_length (char_ptr, list);
diff --git a/gdb/symtab.c b/gdb/symtab.c
index def556b..201fa82 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -4682,7 +4682,7 @@ maybe_add_partial_symtab_filename (const char 
*filename, const char *fullname,
     NULL.  */

  VEC (char_ptr) *
-make_source_files_completion_list (const char *text, const char *word)
+make_source_files_completion_list (const char *text)
  {
    struct symtab *s;
    struct objfile *objfile;
@@ -4711,7 +4711,7 @@ make_source_files_completion_list (const char 
*text, const char *word)
  	{
  	  /* This file matches for a completion; add it to the current
  	     list of matches.  */
-	  add_filename_to_list (s->filename, text, word, &list);
+	  add_filename_to_list (s->filename, text, text, &list);
  	}
        else
  	{
@@ -4723,13 +4723,13 @@ make_source_files_completion_list (const char 
*text, const char *word)
  	  if (base_name != s->filename
  	      && !filename_seen (filename_seen_cache, base_name, 1)
  	      && filename_ncmp (base_name, text, text_len) == 0)
-	    add_filename_to_list (base_name, text, word, &list);
+	    add_filename_to_list (base_name, text, text, &list);
  	}
      }

    datum.filename_seen_cache = filename_seen_cache;
    datum.text = text;
-  datum.word = word;
+  datum.word = text;
    datum.text_len = text_len;
    datum.list = &list;
    map_partial_symbol_filenames (maybe_add_partial_symtab_filename, &datum,

Re: [RFC] Cleanup for make_source_files_completion_list

[Tom Tromey]

>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:

Keith> make_source_files_completion_list is only ever called with the first
Keith> parameter twice, e.g.,
Keith>   fn_list = make_source_files_completion_list (text, text);
Keith> Is there any reason to not whack the second parameter as the following
Keith> patch does?

It makes me wonder if those calls are wrong somehow.

But if those calls are correct then the patch is ok.

Tom

Re: [RFC] Cleanup for make_source_files_completion_list

[Doug Evans]

On Tue, May 7, 2013 at 12:46 PM, Keith Seitz <keiths@redhat.com> wrote:
> Hi,
>
> make_source_files_completion_list is only ever called with the first
> parameter twice, e.g.,
>
>   fn_list = make_source_files_completion_list (text, text);
>
> Is there any reason to not whack the second parameter as the following patch
> does?

I looked into this a bit.
All make_foo_completion_list functions take "text" and "word".
And you can see how they're used in, e.g.,
completion_list_add_name and add_filename_to_list.
[grep for " + 5" in symtab.c.  Heh.]

The caller of make_source_files_completion_list explains the decision
to pass "text" for "word" here:

      /* If we only have file names as possible completion, we should
         bring them in sync with what rl_complete expects.  The
         problem is that if the user types "break /foo/b TAB", and the
         possible completions are "/foo/bar" and "/foo/baz"
         rl_complete expects us to return "bar" and "baz", without the
         leading directories, as possible completions, because `word'
         starts at the "b".  But we ignore the value of `word' when we
         call make_source_files_completion_list above (because that
         would not DTRT when the completion results in both symbols
         and file names), so make_source_files_completion_list returns
         the full "/foo/bar" and "/foo/baz" strings.  This produces
         wrong results when, e.g., there's only one possible
         completion, because rl_complete will prepend "/foo/" to each
         candidate completion.  The loop below removes that leading
         part.  */

Note that if you remove "word" from make_source_files_completion_list
then add_filename_to_list collapses to a trivial function (which would
otherwise be great except there's basic core functionality that I
think should be kept).  :-)

So to maintain consistency with all "make_foo_completion_list"
functions I think I'd like to keep make_source_files_completion_list
as is.  Could be missing something of course.

OTOH the "text" arg to completion_list_add_name can go, it's unused.

Re: [RFC] Cleanup for make_source_files_completion_list

[Keith Seitz]

On 05/09/2013 04:45 PM, Doug Evans wrote:
> The caller of make_source_files_completion_list explains the decision
> to pass "text" for "word" here:
>
>        /* If we only have file names as possible completion, we should
>           bring them in sync with what rl_complete expects.  The
>           problem is that if the user types "break /foo/b TAB", and the
>           possible completions are "/foo/bar" and "/foo/baz"
>           rl_complete expects us to return "bar" and "baz", without the
>           leading directories, as possible completions, because `word'
>           starts at the "b".  But we ignore the value of `word' when we
>           call make_source_files_completion_list above (because that
>           would not DTRT when the completion results in both symbols
>           and file names), so make_source_files_completion_list returns
>           the full "/foo/bar" and "/foo/baz" strings.  This produces
>           wrong results when, e.g., there's only one possible
>           completion, because rl_complete will prepend "/foo/" to each
>           candidate completion.  The loop below removes that leading
>           part.  */
>
> Note that if you remove "word" from make_source_files_completion_list
> then add_filename_to_list collapses to a trivial function (which would
> otherwise be great except there's basic core functionality that I
> think should be kept).  :-)

Indeed. I looked into this a bit further after Tom's comments, too. As 
it is, I am convinced that this patch should not be accepted. [aka, Tom 
is right -- those callers of make_source_files_completion_list which 
pass text twice are suspicious.]

However, I think that the comment quoted above (and the surrounding 
block of code which does the copying) should be removed and the callers 
of make_source_files_completion list should pass text AND word instead 
of text and text. add_filename_to_list (in symtab.c) does this text - 
word adjustment already, so it is not necessary to repeat it here. WDYT?

> OTOH the "text" arg to completion_list_add_name can go, it's unused.

I'll take a look at that, too, while I'm at it. That didn't come up 
during my completer implementation, so it escaped scrutiny.

Keith

Re: [RFC] Cleanup for make_source_files_completion_list

[Keith Seitz]

On 05/13/2013 11:42 AM, Keith Seitz wrote:
> However, I think that the comment quoted above (and the surrounding
> block of code which does the copying) should be removed and the callers
> of make_source_files_completion list should pass text AND word instead
> of text and text. add_filename_to_list (in symtab.c) does this text -
> word adjustment already, so it is not necessary to repeat it here. WDYT?

Something like this? [This causes no regressions in the test suite or by 
manually hacking at it.]

Keith

ChangeLog
2013-05-13  Keith Seitz  <keiths@redhat.com>

	* completer.c (location_completer): Pass 'word' to
	make_source_files_completion_list.
	Do not adjust filenames -- add_filename_to_list, called
	from make_source_files_completion_list, will do it.

diff --git a/gdb/completer.c b/gdb/completer.c
index e132651..28d27f8 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -270,7 +270,7 @@ location_completer (struct cmd_list_element *ignore,
  	 name, they cannot be asking for completion on files.  */
        if (strcspn (text,
  		   gdb_completer_file_name_break_characters) == text_len)
-	fn_list = make_source_files_completion_list (text, text);
+	fn_list = make_source_files_completion_list (text, word);
      }

    n_syms = VEC_length (char_ptr, list);
@@ -296,32 +296,7 @@ location_completer (struct cmd_list_element *ignore,
      {
        /* Nothing.  */
      }
-  else if (n_files)
-    {
-      char *fn;
-
-      /* If we only have file names as possible completion, we should
-	 bring them in sync with what rl_complete expects.  The
-	 problem is that if the user types "break /foo/b TAB", and the
-	 possible completions are "/foo/bar" and "/foo/baz"
-	 rl_complete expects us to return "bar" and "baz", without the
-	 leading directories, as possible completions, because `word'
-	 starts at the "b".  But we ignore the value of `word' when we
-	 call make_source_files_completion_list above (because that
-	 would not DTRT when the completion results in both symbols
-	 and file names), so make_source_files_completion_list returns
-	 the full "/foo/bar" and "/foo/baz" strings.  This produces
-	 wrong results when, e.g., there's only one possible
-	 completion, because rl_complete will prepend "/foo/" to each
-	 candidate completion.  The loop below removes that leading
-	 part.  */
-      for (ix = 0; VEC_iterate (char_ptr, list, ix, fn); ++ix)
-	{
-	  memmove (fn, fn + (word - text),
-		   strlen (fn) + 1 - (word - text));
-	}
-    }
-  else if (!n_syms)
+  else if (!n_syms && !n_files)
      {
        /* No completions at all.  As the final resort, try completing
  	 on the entire text as a symbol.  */

Re: [RFC] Cleanup for make_source_files_completion_list

[Keith Seitz]

On 05/13/2013 02:11 PM, Keith Seitz wrote:
> On 05/13/2013 11:42 AM, Keith Seitz wrote:
>> However, I think that the comment quoted above (and the surrounding
>> block of code which does the copying) should be removed and the callers
>> of make_source_files_completion list should pass text AND word instead
>> of text and text. add_filename_to_list (in symtab.c) does this text -
>> word adjustment already, so it is not necessary to repeat it here. WDYT?
>
> Something like this? [This causes no regressions in the test suite or by
> manually hacking at it.]

Ok, no, this won't work either. The text in the comment (which I 
proposed to remove) hints at the problem:

$ cat same.c
int
same (int a)
{
    return a;
}

int
main (void)
{
   return same (0);
}
$ gcc -g same.c -o same
$ gdb -q same
(gdb) b same.\t\t
[blah blah blah]
c
same
(gdb)

Compare this with unpatched gdb:
(gdb) b same.\t\t
[blah blah blah]
same.c
same

So this needs to stay put. I'll see if I can generate a test case for this.

Thanks to Tom for bringing this up.

Keith

[RFA] completer test [was Re: [RFC] Cleanup for make_source_files_completion_list]

[Keith Seitz]

[-- Attachment #1: Type: text/plain, Size: 609 bytes --]

On 05/15/2013 10:36 AM, Keith Seitz wrote:
> $ cat same.c
> int
> same (int a)
> {
>     return a;
> }
>
> int
> main (void)
> {
>    return same (0);
> }
> $ gcc -g same.c -o same
> $ gdb -q same
> (gdb) b same.\t\t
> [blah blah blah]
> c
> same
> (gdb)
>
> Compare this with unpatched gdb:
> (gdb) b same.\t\t
> [blah blah blah]
> same.c
> same
>
> So this needs to stay put. I'll see if I can generate a test case for this.

And here is the test case.

Keith

testsuite/ChangeLog
2013-05-15  Keith Seitz  <keiths@redhat.com>

	* gdb.base/filesym.exp: New completer test.
	* gdb.base/filesym.c: New file.



[-- Attachment #2: completer-test.patch --]
[-- Type: text/x-patch, Size: 2804 bytes --]

diff --git a/gdb/testsuite/gdb.base/filesym.c b/gdb/testsuite/gdb.base/filesym.c
new file mode 100644
index 0000000..11b3b25
--- /dev/null
+++ b/gdb/testsuite/gdb.base/filesym.c
@@ -0,0 +1,28 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2013 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+int
+filesym (int arg)
+{
+  return arg;
+}
+
+int
+main (void)
+{
+  return filesym (0);
+}
diff --git a/gdb/testsuite/gdb.base/filesym.exp b/gdb/testsuite/gdb.base/filesym.exp
new file mode 100644
index 0000000..c9e9c20
--- /dev/null
+++ b/gdb/testsuite/gdb.base/filesym.exp
@@ -0,0 +1,50 @@
+# Copyright 2013 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This series of completion tests checks the completion output
+# on a name which is potentially both a symbol name and a file name.
+
+standard_testfile
+
+if {[prepare_for_testing $testfile.exp $testfile $srcfile debug]} {
+    return -1
+}
+
+# Test completion list for "filesy".  We expect the immediate result to
+# complete to "filesym"; completing again, we expect the symbol name and the
+# filename (in that order).
+
+send_gdb "break filesy\t"
+gdb_expect {
+     -re "m\$" {
+	pass "complete on \"filesy\""
+
+	# Now ask for the completion list
+	send_gdb "\t\t"
+	gdb_expect {
+	    -re ".*filesym\[ \t\]+filesym.c.*$gdb_prompt break filesym\$" {
+		pass "completion list for \"filesym\""
+	    }
+
+	    default {
+		fail "completion list for \"filesym\""
+	    }
+	}
+    }
+
+    default {
+	fail "complete on \"filesy\""
+    }
+}

Re: [RFA] completer test [was Re: [RFC] Cleanup for make_source_files_completion_list]

[Tom Tromey]

>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:

Keith> 2013-05-15  Keith Seitz  <keiths@redhat.com>

Keith> 	* gdb.base/filesym.exp: New completer test.
Keith> 	* gdb.base/filesym.c: New file.

Ok.  Thanks for doing this.

Tom

Re: [RFA] completer test [was Re: [RFC] Cleanup for make_source_files_completion_list]

[Keith Seitz]

On 05/15/2013 01:01 PM, Tom Tromey wrote:
>>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:
>
> Keith> 2013-05-15  Keith Seitz  <keiths@redhat.com>
>
> Keith> 	* gdb.base/filesym.exp: New completer test.
> Keith> 	* gdb.base/filesym.c: New file.
>
> Ok.  Thanks for doing this.

Committed. Hopefully it will prevent the next person from making the 
same mistake.

Keith

Re: [RFA] completer test [was Re: [RFC] Cleanup for make_source_files_completion_list]

[Sergio Durigan Junior]

Hi Keith,

I know you have already committed the patch, but I just would like to
ask a few things.

On Wednesday, May 15 2013, Keith Seitz wrote:

> diff --git a/gdb/testsuite/gdb.base/filesym.exp b/gdb/testsuite/gdb.base/filesym.exp
> new file mode 100644
> index 0000000..c9e9c20
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/filesym.exp

> +# Test completion list for "filesy".  We expect the immediate result to
> +# complete to "filesym"; completing again, we expect the symbol name and the
> +# filename (in that order).
> +
> +send_gdb "break filesy\t"
> +gdb_expect {
> +     -re "m\$" {
> +	pass "complete on \"filesy\""
> +
> +	# Now ask for the completion list
> +	send_gdb "\t\t"
> +	gdb_expect {
> +	    -re ".*filesym\[ \t\]+filesym.c.*$gdb_prompt break filesym\$" {
> +		pass "completion list for \"filesym\""
> +	    }
> +
> +	    default {
> +		fail "completion list for \"filesym\""
> +	    }
> +	}
> +    }
> +
> +    default {
> +	fail "complete on \"filesy\""
> +    }
> +}

Wouldn't it be better to use the "complete" command?  Here is what I see
when I use it:

    (gdb) complete break filesy
    break filesym
    break filesym.c

Also, ISTR "send_gdb" is deprecated, and one should use
"gdb_test_multiple" instead.  WDYT?

Thanks,

-- 
Sergio

Re: [RFA] completer test [was Re: [RFC] Cleanup for make_source_files_completion_list]

[Keith Seitz]

On 05/15/2013 03:33 PM, Sergio Durigan Junior wrote:
> Wouldn't it be better to use the "complete" command?  Here is what I see
> when I use it:
>
>      (gdb) complete break filesy
>      break filesym
>      break filesym.c

Is that necessarily "better" than testing what a user would actually 
type? I don't know. gdb.base/completion.exp uses both forms.

> Also, ISTR "send_gdb" is deprecated, and one should use
> "gdb_test_multiple" instead.  WDYT?

Is send_gdb deprecated or gdb_expect? Or is their direct use discouraged?

This is the first I've heard of send_gdb being deprecated. As far as I 
can tell, there is no other way to directly test completion this way. I 
do see, though, that completion.exp uses gdb_test_multiple instead of 
gdb_expect... If it truly is deprecated, I would expect send_gdb to be 
made "private" in some way. [deprecated_send_gdb?] Or at least mentioned 
in lib/gdb.exp.

If there is a preference for one or the other [or an actual policy], I 
will certainly make necessary changes.

I'm using a similar test strategy for my explicit completion tests, 
which I am about to submit...

Thank you for bringing this up.

Keith

Re: [RFA] completer test [was Re: [RFC] Cleanup for make_source_files_completion_list]

[Sergio Durigan Junior]

On Wednesday, May 15 2013, Keith Seitz wrote:

> On 05/15/2013 03:33 PM, Sergio Durigan Junior wrote:
>> Wouldn't it be better to use the "complete" command?  Here is what I see
>> when I use it:
>>
>>      (gdb) complete break filesy
>>      break filesym
>>      break filesym.c
>
> Is that necessarily "better" than testing what a user would actually
> type? I don't know. gdb.base/completion.exp uses both forms.

AFAIU "complete" is a non-interactive form of \t\t (maybe written
exactly to make the testing process easier), and both forms should work
correspondingly.  If they doesn't, that's readline's issue.

Anyway, when I said "better" I mean "clearer".  The code could be a lot
simpler than it is.  But I will definitely not bikeshed on this :-).

>> Also, ISTR "send_gdb" is deprecated, and one should use
>> "gdb_test_multiple" instead.  WDYT?
>
> Is send_gdb deprecated or gdb_expect? Or is their direct use discouraged?

ISTR their direct use is discouraged, yes.  Sorry for the confusion.

> This is the first I've heard of send_gdb being deprecated. As far as I
> can tell, there is no other way to directly test completion this
> way. I do see, though, that completion.exp uses gdb_test_multiple
> instead of gdb_expect... If it truly is deprecated, I would expect
> send_gdb to be made "private" in some way. [deprecated_send_gdb?] Or
> at least mentioned in lib/gdb.exp.

As I said above, their usage is discouraged, but there are still lots
and lots of old places which use them.

> If there is a preference for one or the other [or an actual policy], I
> will certainly make necessary changes.

Sure, I'd wait for a maintainer's opinion on that, just in case...

> Thank you for bringing this up.

My pleasure.  And thanks for the patch!

-- 
Sergio

Re: [RFA] completer test [was Re: [RFC] Cleanup for make_source_files_completion_list]

[Stan Shebs]

On 5/15/13 4:47 PM, Keith Seitz wrote:
> On 05/15/2013 03:33 PM, Sergio Durigan Junior wrote:
>> Wouldn't it be better to use the "complete" command?  Here is what I see
>> when I use it:
>>
>>      (gdb) complete break filesy
>>      break filesym
>>      break filesym.c
> 
> Is that necessarily "better" than testing what a user would actually
> type? I don't know. gdb.base/completion.exp uses both forms.
> 
>> Also, ISTR "send_gdb" is deprecated, and one should use
>> "gdb_test_multiple" instead.  WDYT?
> 
> Is send_gdb deprecated or gdb_expect? Or is their direct use discouraged?
> 
> This is the first I've heard of send_gdb being deprecated. As far as I
> can tell, there is no other way to directly test completion this way. I
> do see, though, that completion.exp uses gdb_test_multiple instead of
> gdb_expect... If it truly is deprecated, I would expect send_gdb to be
> made "private" in some way. [deprecated_send_gdb?] Or at least mentioned
> in lib/gdb.exp.

I think send_gdb and gdb_expect are still key building blocks, but most
of their uses have been for multi-outcome tests that can be done as well
or better with gdb_test_multiple.

It may even be that gdb_test_multiple suffices for all of the existing
send_gdb/gdb_expect pairs, but I don't imagine anyone has the fortitude
to work through every one of them. :-)

Stan
stan@codesourcery.com

Re: [RFA] completer test [was Re: [RFC] Cleanup for make_source_files_completion_list]

[Pedro Alves]

On 05/16/2013 01:27 AM, Stan Shebs wrote:
> It may even be that gdb_test_multiple suffices for all of the existing
> send_gdb/gdb_expect pairs, but I don't imagine anyone has the fortitude
> to work through every one of them. :-)

Michael Snyder went through a lot of them in 2010.  E.g.,

2010-06-02  Michael Snyder  <msnyder@vmware.com>

        * gdb.trace/actions.exp: Use gdb_test_no_output.
        * gdb.trace/circ.exp: Ditto.
        * gdb.trace/packetlen.exp: Ditto.
        * gdb.trace/save-trace.exp: Ditto.
...
2010-05-28  Michael Snyder  <msnyder@vmware.com>

        * gdb.reverse/break-precsave.exp:
        Replace uses of send_gdb / gdb_expect.
        * gdb.reverse/break-reverse.exp:
...
(many others)

We have a lot less remaining uses of gdb_expect in tests since, but
we do still have many more than I expected.  I'm a bit surprised;
I thought we had fewer:

early 2010 tree:
 $ find . -name "*.exp" | grep -v "lib/" | xargs grep gdb_expect | wc -l
 1849

vs current tree:
 $ find . -name "*.exp" | grep -v "lib/" | xargs grep gdb_expect | wc -l
 739

-- 
Pedro Alves

Re: [RFA] completer test [was Re: [RFC] Cleanup for make_source_files_completion_list]

[Joel Brobecker]

> >     (gdb) complete break filesy
> >     break filesym
> >     break filesym.c
> 
> Is that necessarily "better" than testing what a user would actually
> type? I don't know. gdb.base/completion.exp uses both forms.

I haven't verified yet that the issue you fixed is the same as
the issue I noticed, and put aside for later. But my initial
investigation at the time showed that the problem did not appear
when using the "complete" command, only when using the interactive
form via the "tab" key.

> >Also, ISTR "send_gdb" is deprecated, and one should use
> >"gdb_test_multiple" instead.  WDYT?
> 
> Is send_gdb deprecated or gdb_expect? Or is their direct use discouraged?

My take on this is that it is very highly discouraged, at least
for gdb_expect, and should only be used when none of the standard
gdb_test et al routines can do the job. The reason behind it is
that using gdb_expect directly, you are missing out on the standard
processing of various known conditions (internal errors, timeouts,
etc).

-- 
Joel

Re: [RFA] completer test [was Re: [RFC] Cleanup for make_source_files_completion_list]

[Joel Brobecker]

> > Is that necessarily "better" than testing what a user would actually
> > type? I don't know. gdb.base/completion.exp uses both forms.
> 
> I haven't verified yet that the issue you fixed is the same as
> the issue I noticed, and put aside for later. But my initial
> investigation at the time showed that the problem did not appear
> when using the "complete" command, only when using the interactive
> form via the "tab" key.

Reviewing the entire email thread, I think it's a totally different
issue. I am referring to:

    (gdb) b simple<TAB>
    /[...]/cleanups.c:264: internal-error: restore_my_cleanups: Assertion `*pmy_
+chain == SENTINEL_CLEANUP' failed.
    A problem internal to GDB has been detected,
    further debugging may prove unreliable.
    Quit this debugging session? (y or n)

First guess is that this is happening inside ada-lang. And
indeed, when forcing the language to C, no problemo...

That's the long way of saying disregard my first paragraph above :-).
-- 
Joel

Re: [RFA] completer test [was Re: [RFC] Cleanup for make_source_files_completion_list]

[Keith Seitz]

On 05/15/2013 10:49 PM, Joel Brobecker wrote:
> Reviewing the entire email thread, I think it's a totally different
> issue. I am referring to:
>
>      (gdb) b simple<TAB>
>      /[...]/cleanups.c:264: internal-error: restore_my_cleanups: Assertion `*pmy_
> +chain == SENTINEL_CLEANUP' failed.
>      A problem internal to GDB has been detected,
>      further debugging may prove unreliable.
>      Quit this debugging session? (y or n)

Actually, I have a patch for this. I ran across the same thing with 
Insight. This assert is being triggered by find_frame_funname, which 
contains a dangling cleanup. This is in ada-lang.c in two places.

I will be submitting as soon as I have enough coffee.

Keith

Re: [RFA] completer test [was Re: [RFC] Cleanup for make_source_files_completion_list]

[Joel Brobecker]

> Actually, I have a patch for this. I ran across the same thing with
> Insight. This assert is being triggered by find_frame_funname, which
> contains a dangling cleanup. This is in ada-lang.c in two places.
> 
> I will be submitting as soon as I have enough coffee.

For the record, I briefly looked at the patch, and I think they
are different issues, so thanks for taking a look!

I suspect we are going to find a large number of issues in
ada-lang, simply because I never quite grasped the fact that
cleanups should not be leaked under normal execution (only
when an exception is raised, in which case they get automatically
performed).

-- 
Joel

Re: [RFA] completer test [was Re: [RFC] Cleanup for make_source_files_completion_list]

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> I suspect we are going to find a large number of issues in
Joel> ada-lang, simply because I never quite grasped the fact that
Joel> cleanups should not be leaked under normal execution (only
Joel> when an exception is raised, in which case they get automatically
Joel> performed).

The checker found two -- the one you fixed in
ada_make_symbol_completion_list, and also a dangling cleanup in
old_renaming_is_invisible.  This one seemed hard to fix and so
in my patch series I just punted on it by marking it with
the dangling_cleanup attribute.

Tom

Re: [RFA] completer test [was Re: [RFC] Cleanup for make_source_files_completion_list]

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 657 bytes --]

> The checker found two -- the one you fixed in
> ada_make_symbol_completion_list, and also a dangling cleanup in
> old_renaming_is_invisible.  This one seemed hard to fix and so
> in my patch series I just punted on it by marking it with
> the dangling_cleanup attribute.

Do you see something obviously wrong with the attached patch?
I haven't tested it yet, and I am not sure how much I can test it,
given that old-style renamings have been replaced 6 years ago.
I was looking at the option of removing support for them, but
I actually know of some users still using an ancient version
of the compiler but with a new version of the debugger...

-- 
Joel

[-- Attachment #2: old_renaming_is_invisible-cleanup.diff --]
[-- Type: text/x-diff, Size: 1244 bytes --]

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index fa6db0f..b6d5e0b 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -4726,17 +4726,20 @@ static int
 old_renaming_is_invisible (const struct symbol *sym, const char *function_name)
 {
   char *scope;
+  struct cleanup *old_chain;
 
   if (SYMBOL_CLASS (sym) != LOC_TYPEDEF)
     return 0;
 
   scope = xget_renaming_scope (SYMBOL_TYPE (sym));
-
-  make_cleanup (xfree, scope);
+  old_chain = make_cleanup (xfree, scope);
 
   /* If the rename has been defined in a package, then it is visible.  */
   if (is_package_name (scope))
-    return 0;
+    {
+      do_cleanups (old_chain);
+      return 0;
+    }
 
   /* Check that the rename is in the current function scope by checking
      that its name starts with SCOPE.  */
@@ -4748,7 +4751,12 @@ old_renaming_is_invisible (const struct symbol *sym, const char *function_name)
   if (strncmp (function_name, "_ada_", 5) == 0)
     function_name += 5;
 
-  return (strncmp (function_name, scope, strlen (scope)) != 0);
+  {
+    int is_invisible = strncmp (function_name, scope, strlen (scope)) != 0;
+
+    do_cleanups (old_chain);
+    return is_invisible;
+  }
 }
 
 /* Remove entries from SYMS that corresponds to a renaming entity that

Re: [RFA] completer test [was Re: [RFC] Cleanup for make_source_files_completion_list]

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> Do you see something obviously wrong with the attached patch?
Joel> I haven't tested it yet, and I am not sure how much I can test it,
Joel> given that old-style renamings have been replaced 6 years ago.
Joel> I was looking at the option of removing support for them, but
Joel> I actually know of some users still using an ancient version
Joel> of the compiler but with a new version of the debugger...

For some reason I thought the function needed to leave a dangling
cleanup, but I see now I was mistaken.
It looks good to me.

Tom

Checked in: [RFA] completer test [was Re: [RFC] Cleanup for make_source_files_completion_list]

[Joel Brobecker]

> Joel> Do you see something obviously wrong with the attached patch?
> Joel> I haven't tested it yet, and I am not sure how much I can test it,
> Joel> given that old-style renamings have been replaced 6 years ago.
> Joel> I was looking at the option of removing support for them, but
> Joel> I actually know of some users still using an ancient version
> Joel> of the compiler but with a new version of the debugger...
> 
> For some reason I thought the function needed to leave a dangling
> cleanup, but I see now I was mistaken.
> It looks good to me.

Cool, thanks! Attached is what I checked in, after a round of testing on
x86_64-linux.

gdb/ChangeLog:

        * ada-lang.c (old_renaming_is_invisible): Fix cleanup leak.

Thanks again for pointing this cleanup leak out.

-- 
Joel

Re: Checked in: [RFA] completer test [was Re: [RFC] Cleanup for make_source_files_completion_list]

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 279 bytes --]

> Cool, thanks! Attached is what I checked in, after a round of testing on
> x86_64-linux.
> 
> gdb/ChangeLog:
> 
>         * ada-lang.c (old_renaming_is_invisible): Fix cleanup leak.
> 
> Thanks again for pointing this cleanup leak out.

Really attached, this time...

-- 
Joel

[-- Attachment #2: 0001-Ada-Fix-cleanup-leak-in-ada-lang.c-old_renaming_is_i.patch --]
[-- Type: text/x-diff, Size: 2052 bytes --]

From aea9d783272e628dea85555ebebc0f3891fd5f77 Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Tue, 21 May 2013 09:40:01 +0400
Subject: [PATCH] [Ada] Fix cleanup leak in
 ada-lang.c:old_renaming_is_invisible

gdb/ChangeLog:

	* ada-lang.c (old_renaming_is_invisible): Fix cleanup leak.
---
 gdb/ChangeLog  |    4 ++++
 gdb/ada-lang.c |   16 ++++++++++++----
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 19a4cd6..c48f7e7 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@
+2013-05-21  Joel Brobecker  <brobecker@adacore.com>
+
+	* ada-lang.c (old_renaming_is_invisible): Fix cleanup leak.
+
 2013-05-21  Hui Zhu  <hui@codesourcery.com>
 
 	* breakpoint.c (dprintf_breakpoint_ops): Remove its static.
diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index fa6db0f..b6d5e0b 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -4726,17 +4726,20 @@ static int
 old_renaming_is_invisible (const struct symbol *sym, const char *function_name)
 {
   char *scope;
+  struct cleanup *old_chain;
 
   if (SYMBOL_CLASS (sym) != LOC_TYPEDEF)
     return 0;
 
   scope = xget_renaming_scope (SYMBOL_TYPE (sym));
-
-  make_cleanup (xfree, scope);
+  old_chain = make_cleanup (xfree, scope);
 
   /* If the rename has been defined in a package, then it is visible.  */
   if (is_package_name (scope))
-    return 0;
+    {
+      do_cleanups (old_chain);
+      return 0;
+    }
 
   /* Check that the rename is in the current function scope by checking
      that its name starts with SCOPE.  */
@@ -4748,7 +4751,12 @@ old_renaming_is_invisible (const struct symbol *sym, const char *function_name)
   if (strncmp (function_name, "_ada_", 5) == 0)
     function_name += 5;
 
-  return (strncmp (function_name, scope, strlen (scope)) != 0);
+  {
+    int is_invisible = strncmp (function_name, scope, strlen (scope)) != 0;
+
+    do_cleanups (old_chain);
+    return is_invisible;
+  }
 }
 
 /* Remove entries from SYMS that corresponds to a renaming entity that
-- 
1.7.10.4

Re: [RFA] completer test [was Re: [RFC] Cleanup for make_source_files_completion_list]

[Sergio Durigan Junior]

On Thursday, May 16 2013, Joel Brobecker wrote:

> I haven't verified yet that the issue you fixed is the same as
> the issue I noticed, and put aside for later. But my initial
> investigation at the time showed that the problem did not appear
> when using the "complete" command, only when using the interactive
> form via the "tab" key.

That's interesting, I really thought the "complete" command would have
the exact same effect as the interactive form.  Living and learning.

-- 
Sergio

Re: [RFA] completer test [was Re: [RFC] Cleanup for make_source_files_completion_list]

[Joel Brobecker]

> That's interesting, I really thought the "complete" command would have
> the exact same effect as the interactive form.  Living and learning.

Same here... (I was actually pretty disappointed, as I like avoiding
the extra complication of dealing with the interactive-side of
things).

-- 
Joel

Re: [RFA] completer test [was Re: [RFC] Cleanup for make_source_files_completion_list]

[Pedro Alves]

On 05/16/2013 12:47 AM, Keith Seitz wrote:
> On 05/15/2013 03:33 PM, Sergio Durigan Junior wrote:
>> Wouldn't it be better to use the "complete" command?  Here is what I see
>> when I use it:
>>
>>      (gdb) complete break filesy
>>      break filesym
>>      break filesym.c
> 
> Is that necessarily "better" than testing what a user would actually type? I don't know. gdb.base/completion.exp uses both forms.

Yeah.  Testing just "complete" doesn't cover everything
necessary.  See thread around:

 http://sourceware.org/ml/gdb-patches/2011-05/msg00032.html

"complete" is useful for frontends, so it's good to test it too.

An idea would be to have a mechanism (procedure, whatever) in (or used by)
completion.exp that given an input string and the expected completions
exercises both "complete" and "\t" with a single call.  E.g.,:

test_completion "break filesy" { "break" "break filesym.c" }

Probably, the completion.exp tests already test the specifics of gdb<->readline
interaction that only the \t code paths trigger, independent of specific
completer, so for testing specific completers it's probably fine to just
go with using the simpler "complete" way only.  If we had the mechanism
suggested above, then we wouldn't have to think about considering whether
all bases are covered though.  We'd just always use it.  :-)

This patch seems to involve testing sort of actual completion mechanics,
so it looks like a specific case where testing (at least) the \t variant
is good.

> This is the first I've heard of send_gdb being deprecated. As far as I can tell, there is no other way to directly test completion this way. I do see, though, that completion.exp uses gdb_test_multiple instead of gdb_expect... If it truly is deprecated, I would expect send_gdb to be made "private" in some way. [deprecated_send_gdb?] Or at least mentioned in lib/gdb.exp.

Yeah.  Testing completion is one of the cases that need to use it.
If we had a higher level mechanism/procedure that simplified \t completion
testing, we could hide those send_gdb calls though.  :-)

> If there is a preference for one or the other [or an actual policy], I will certainly make necessary changes.
> 
> I'm using a similar test strategy for my explicit completion tests, which I am about to submit...

Please use gdb_test_multiple.  See e.g., how completion.exp itself
simplified much when converted to it:

 http://sourceware.org/ml/gdb-patches/2011-05/msg00063.html

-- 
Pedro Alves

Re: [RFA] completer test [was Re: [RFC] Cleanup for make_source_files_completion_list]

[Joel Brobecker]

> Please use gdb_test_multiple.  See e.g., how completion.exp itself
> simplified much when converted to it:
> 
>  http://sourceware.org/ml/gdb-patches/2011-05/msg00063.html

I also just checked in a test that performs a completion test
using send_gdb + gdb_test_multiple...

http://www.sourceware.org/ml/gdb-patches/2013-05/msg00573.html

-- 
Joel

Re: [RFA] completer test [was Re: [RFC] Cleanup for make_source_files_completion_list]

[Pedro Alves]

On 05/16/2013 10:26 AM, Joel Brobecker wrote:
>> Please use gdb_test_multiple.  See e.g., how completion.exp itself
>> simplified much when converted to it:
>>
>>  http://sourceware.org/ml/gdb-patches/2011-05/msg00063.html
> 
> I also just checked in a test that performs a completion test
> using send_gdb + gdb_test_multiple...
> 
> http://www.sourceware.org/ml/gdb-patches/2013-05/msg00573.html

Ah,

> --- a/gdb/testsuite/gdb.ada/complete.exp
> +++ b/gdb/testsuite/gdb.ada/complete.exp
> @@ -175,4 +175,19 @@ test_gdb_complete "pck." \
>  test_gdb_complete "<pck__my" \
>                    "p <pck__my_global_variable>"
...

That made me look at what test_gdb_complete is, and then I
noticed a FIXME that's super easily fixed nowadays.  :-)

 http://sourceware.org/ml/gdb-patches/2013-05/msg00584.html

I went ahead and checked in the fix as obvious.

-- 
Pedro Alves