Re: [RFC][PATCH] new-ui command under windows using NamedPipe

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Pedro Alves <palves@redhat.com> [2019-07-18 17:24:34 +0100]:

> On 7/17/19 7:41 PM, LABARTHE Guillaume wrote:
> > Hello,
> > 
> > I am currently working on a front-end for gdb on windows, and trying
> > to use the new-ui command passing as tty name the name of a named pipe
> > without luck.
> > 
> > Then I decided to dig into it so I cloned gdb's repo and started
> > debugging. After some investigation I found out that the problem was
> > that the function new_ui_command in top.c opens the same tty three
> > times (for stdin, sdout and stderr). With windows named pipes the
> > second and third calls to open fail. I then patched the function to
> > open the file only once and pass the same stream for stdin stdout and
> > stderr and that made it work.
> 
> Wow, awesome!  I've been saying for years now that named pipes is
> probably the way to go for Windows.  I had no idea the fix would be
> so simple!
> 
> > 
> > I don't know the implication of my patch on other operating systems or
> > what would be the way to make it specific to windows.
> 
> I tried it on GNU/Linux and things still work.  
> 
> I ran all the MI tests with forced new-ui, with:
> 
>  $ make check TESTS="gdb.mi/*.exp" RUNTESTFLAGS="FORCE_MI_SEPARATE_UI=1"
> 
> and saw no regressions.

Are all of these special flags for testing documented anywhere?  Every
now and then someone will mention some new flag that I've never seen
before and I always think that one day it might be helpful...

Thanks,
Andrew




> 
> > 
> > Can you please advise on the best way to make this patch portable.
> > You will find in the attachments my patch so far.
> It actually looks good as is.  I wrote a ChangeLog, synthesized a
> commit log, tweaked the comments a little bit, and merged it as below.
> 
> Note: we're currently leaking the terminal file if the UI is closed,
> but that happens without your patch as well.
> 
> From afe09f0b6311a4dd1a7e2dc6491550bb228734f8 Mon Sep 17 00:00:00 2001
> From: Guillaume LABARTHE <guillaume.labarthe@gmail.com>
> Date: Thu, 18 Jul 2019 17:20:04 +0100
> Subject: [PATCH] Fix for using named pipes on Windows
> 
> On Windows, passing a named pipe as terminal argument to the new-ui
> command does not work.
> 
> The problem is that the new_ui_command function in top.c opens the
> same tty three times, for stdin, stdout and stderr.  With Windows
> named pipes, the second and third calls to open fail.
> 
> Opening the file only once and passing the same stream for stdin,
> stdout and stderr makes it work.
> 
> Pedro says:
> 
>  I tried it on GNU/Linux and things still work.
>  I ran all the MI tests with forced new-ui, with:
> 
>  $ make check TESTS="gdb.mi/*.exp" RUNTESTFLAGS="FORCE_MI_SEPARATE_UI=1"
> 
>  and saw no regressions.
> 
> gdb/ChangeLog:
> 2019-07-18  Guillaume LABARTHE  <guillaume.labarthe@gmail.com>
> 
> 	* top.c (new_ui_command): Open specified terminal just once.
> ---
>  gdb/ChangeLog |  4 ++++
>  gdb/top.c     | 18 +++++++-----------
>  2 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index fa669daa4b3..d6fe9895a71 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,7 @@
> +2019-07-18  Guillaume LABARTHE  <guillaume.labarthe@gmail.com>
> +
> +	* top.c (new_ui_command): Open specified terminal just once.
> +
>  2019-07-18  Tom Tromey  <tromey@adacore.com>
>  
>  	* symtab.c (main_name): Constify return type.
> diff --git a/gdb/top.c b/gdb/top.c
> index 83a3604688b..60f81b3bf85 100644
> --- a/gdb/top.c
> +++ b/gdb/top.c
> @@ -337,8 +337,6 @@ open_terminal_stream (const char *name)
>  static void
>  new_ui_command (const char *args, int from_tty)
>  {
> -  gdb_file_up stream[3];
> -  int i;
>    int argc;
>    const char *interpreter_name;
>    const char *tty_name;
> @@ -357,13 +355,13 @@ new_ui_command (const char *args, int from_tty)
>    {
>      scoped_restore save_ui = make_scoped_restore (&current_ui);
>  
> -    /* Open specified terminal, once for each of
> -       stdin/stdout/stderr.  */
> -    for (i = 0; i < 3; i++)
> -      stream[i] = open_terminal_stream (tty_name);
> +    /* Open specified terminal.  Note: we used to open it three times,
> +       once for each of stdin/stdout/stderr, but that does not work
> +       with Windows named pipes.  */
> +    gdb_file_up stream = open_terminal_stream (tty_name);
>  
>      std::unique_ptr<ui> ui
> -      (new struct ui (stream[0].get (), stream[1].get (), stream[2].get ()));
> +      (new struct ui (stream.get (), stream.get (), stream.get ()));
>  
>      ui->async = 1;
>  
> @@ -373,10 +371,8 @@ new_ui_command (const char *args, int from_tty)
>  
>      interp_pre_command_loop (top_level_interpreter ());
>  
> -    /* Make sure the files are not closed.  */
> -    stream[0].release ();
> -    stream[1].release ();
> -    stream[2].release ();
> +    /* Make sure the file is not closed.  */
> +    stream.release ();
>  
>      ui.release ();
>    }
> -- 
> 2.14.5
>