diff mbox

[RFC] new-ui command under windows using NamedPipe

Message ID 7c1c5343-3171-d454-8507-507d5b8a24a5@redhat.com
State New
Headers show

Commit Message

Pedro Alves July 18, 2019, 4:24 p.m. UTC 
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.

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

Comments

Andrew Burgess July 19, 2019, 8:50 a.m. UTC | #1 
* 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
>
Pedro Alves July 19, 2019, 8:54 a.m. UTC | #2 
On 7/19/19 9:50 AM, Andrew Burgess wrote:

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

Yes, in gdb/testsuite/README:

 ~~~~
 FORCE_SEPARATE_MI_TTY

 Setting FORCE_MI_SEPARATE_UI to 1 forces all MI testing to start GDB
 in console mode, with MI running on a separate TTY, on a secondary UI
 started with "new-ui".
 ~~~~

Thanks,
Pedro Alves
diff mbox

Patch

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 ();
   }