Re: [RFC] Fixing gdb.base/completion.exp (PR testsuite/12649)

[Pedro Alves <pedro at codesourcery dot com>]

On Monday 02 May 2011 15:52:29, Jan Kratochvil wrote:
> On Mon, 02 May 2011 16:19:25 +0200, Pedro Alves wrote:
> > On Sunday 01 May 2011 10:16:30, Jan Kratochvil wrote:
> > > The "complete" command appraoch does introduce this new kind of race.
> > > 
> > > But the patch can be commited in two parts if it is preferred although
> > > reviewing these racy send_gdb-gdb_expect cases for the intermediate step is
> > > tricky and it gets dropped immediately afterwards.
> > 
> > What do you mean is dropped immediately afterwards?
> 
> Replacing this send_gdb + gdb_expect by gdb_test "complete ..." makes the GDB
> codebase/testsuite more maintainable so I thought it could be changed now.
> 
> I found easier to replace the current constructs by gdb_test "complete ..." at
> once although one can fix the gdb_expect first and delete it afterwards if you
> wish.
> 
> Or are you against the replacement by gdb_test "complete ..."?

The "\t" method of completion interacts with readline, the
"complete command" method doesn't.  I think it's useful and
important to test the "\t" version, especially since it's
what CLI users are using.

As principle, I don't like rewriting to make a not understood
problem go away.  In this particular case, since it would be desirable
to keep at least one instance of the original form, we get to fix
at least that instance.  But when we know how to fix it, we can
fix all of the instances.  And then the original motivation to rewrite
using a different method disappears or at least diminishes.

> 
> I have checked the code paths (despite what Tom says) and personally I cannot
> imagine a difference between \t and the "complete" command.
> 
> 
> > > > @@ -410,7 +365,7 @@ gdb_expect  {
> > > >  		    timeout           {fail "(timeout) complete 'p \"break1.'"}
> > > >  		}
> > > >  	    }
> > > > -	-re "^p \"break1\\..*$"
> > > > +	-re "^p \"break1\\...*$"
> > > >  	    {	send_gdb "\n"
> > > >  		gdb_expect {
> > > >  		    -re ".*$gdb_prompt $" { fail "complete 'p \"break1.'"}
> > > 
> > > I do not see this change as valid/relevant.
> > 
> > The pattern above reads:
> > 
> > 	-re "^p \"break1\\.c\"$"\
> > ...
> > 	-re "^p \"break1\\..*$"
> > ...
> > 
> > It looked like "^p \"break1\\.c" could wrongly match the latter pattern,
> > if the "c" wasn't in the buffer yet?
> 
> Aha.  But this testcase always FAILs (which it always considers as XFAIL)
> because:
> (1) gdb.base/break1.o prevents the completion (during in-tree build)
> (2) GDB 7.3.50.20110502-cvs now completes it (with bundled readline) as:
> 	>p "break1.c < (note the trailing space)
>     instead of expected
> 	>p "break1.c"<
>     so it FAILs/XFAILs anyway.

> So I am not sure what should be there when it cannot work anyway.

Thanks.  Sounds like break1.c was added (2003) after the test was
originally written (199[89], and since the completion never worked as
intended by the author of test, always XFAILing, it was never
noticed break1.o was in the way.  I somehow not noticed the \",
making it so that my change wasn't enough anyway (needed yet one more
dot, or something better).  I'll remove it.

-- 
Pedro Alves