Re: [PATCH] Handle correctly passing a bad interpreter name to new-ui

[Pedro Alves <palves at redhat dot com>]

On 07/20/2016 06:46 PM, Simon Marchi wrote:
> On 16-07-19 01:34 PM, Pedro Alves wrote:
>> On 07/13/2016 03:13 PM, Simon Marchi wrote:
>>> When a bad interpreter name is passed to new-ui, such as:
>>>
>>>   (gdb)  new-ui bloop /dev/pts/10
>>>
>>> A partially created UI is left in the UI list, with interp set to NULL.
>>> Trying to do anything that will print on this UI (such as "start") will
>>> cause a segmentation fault.
>>
>> Whoops.  Thanks for fixing this.
>>
>>>
>>> gdb/ChangeLog:
>>>
>>> 	* top.h (make_delete_ui_cleanup): New declaration.
>>> 	* top.c (delete_ui_cleanup): New function.
>>> 	(make_delete_ui_cleanup): New function.
>>> 	(new_ui_command): Create restore_ui cleanup earlier, create a
>>> 	delete_ui cleanup and discard it on success.
>>>
>>> gdb/testsuite/ChangeLog:
>>>
>>> 	* gdb.base/new-ui.exp (do_test_invalid_interpreter): New
>>> 	procedure.
>>> ---
>>>  gdb/testsuite/gdb.base/new-ui.exp | 23 +++++++++++++++++++++++
>>>  gdb/top.c                         | 34 +++++++++++++++++++++++++---------
>>>  gdb/top.h                         |  3 +++
>>>  3 files changed, 51 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/gdb/testsuite/gdb.base/new-ui.exp b/gdb/testsuite/gdb.base/new-ui.exp
>>> index f3f66db..ddff8c4 100644
>>> --- a/gdb/testsuite/gdb.base/new-ui.exp
>>> +++ b/gdb/testsuite/gdb.base/new-ui.exp
>>> @@ -143,4 +143,27 @@ proc do_test {} {
>>>      }
>>>  }
>>>  
>>> +proc do_test_invalid_interpreter {} {
>>
>> An intro comment would be nice.  Something like the
>> "wrong interpreter" comment below.
> 
> Right, that comment makes more sense here.
> 
>>> +    global testfile
>>> +
>>> +    clean_restart $testfile
>>> +
>>> +    spawn -pty
>>> +    set extra_tty_name $spawn_out(slave,name)
>>> +
>>> +    if ![runto_main] {
>>> +	untested "could not run to main"
>>> +	return -1
>>> +    }
>>
>> Is this runto_main needed?
> 
> Woops no, copy-pasta.
> 
>>> +
>>> +    # Test that asking for a wrong interpreter is properly reported.
>>> +    gdb_test "new-ui bloop $extra_tty_name" "Interpreter `bloop' unrecognized"
>>
>> lowercase "Interpreter".
> 
> Well, that's the actual output from GDB, if I change it it won't match anymore :).

Whoops, misread it as test message.  :-P  But hey, I still caught a problem
then.  :-)  "$extra_tty_name" is not stable, thus you're ending up with
an unstable test message that depends on number of pts's already taken
on your system, phase of the moon, etc.

> 
>> Did you consider instead adding this to do_test, just before
>> the "new-ui console" test?  We do a couple other basic "new-ui"
>> tests there too, like the no args, and no-repeat checks.  Seems this
>> fits right in.  Since do_test also runs execution commands that print to
>> both consoles, that would give the same coverage.  I ask because do_test is
>> the driver procedure for the real tests, and with the new procedure we
>> now have multiple clean_restart calls with no with_test_prefix, which is
>> something that pedantically doesn't look 100% right to me because it can
>> cause duplicate test messages (on internal errors, etc.).
> 
> I don't mind, it's just that I like to have small, independent test sequences in
> each exp.  It's easier to analyze when you try to fix a test than when you have a
> single big sequence, where you need to weed out what's relevant to what is failing
> and what's not.

Thought that might be the reason.  That's a fine principle.

> But you are right, we should use with_test_prefix at the root level.
> 
> Perhaps it would make sense to put all the "invalid usages" tests in the separate
> procedure then, and leave the main procedure to test the real stuff.  We could also
> factor out the extra pty creation, or just create a single one that we pass to all
> the procedures.

All reasonable choices.

> 
> I'll send a v2 based on your upcoming comments.

As long as my "duplicate test messages" itch is addressed, I'll be
fine with whatever you prefer.

Thanks,
Pedro Alves