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

[Pedro Alves <palves at redhat dot com>]

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.

> +    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?

> +
> +    # Test that asking for a wrong interpreter is properly reported.
> +    gdb_test "new-ui bloop $extra_tty_name" "Interpreter `bloop' unrecognized"

lowercase "Interpreter".

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.).

> +
> +    # Test that we can continue normally (this used to crash).
> +    if ![runto_main] {
> +	fail "could not run to main"
> +    }
> +}
> +
>  do_test
> +do_test_invalid_interpreter
> diff --git a/gdb/top.c b/gdb/top.c
> index 3174f3c..cf0b544 100644
> --- a/gdb/top.c
> +++ b/gdb/top.c
> @@ -324,6 +324,20 @@ delete_ui (struct ui *todel)
>    free_ui (ui);
>  }
>  
> +static void
> +delete_ui_cleanup (void *void_ui)
> +{
> +  struct ui *ui = (struct ui *) void_ui;
> +
> +  delete_ui (ui);
> +}
> +
> +struct cleanup *
> +make_delete_ui_cleanup (struct ui *ui)
> +{
> +  return make_cleanup (delete_ui_cleanup, ui);
> +}

Intro comments.

Thanks,
Pedro Alves