diff mbox

Building the 7.8.90 pretest on MinGW

Message ID 83vbk82fkg.fsf@gnu.org
State New
Headers show

Commit Message

Eli Zaretskii Jan. 15, 2015, 4:07 p.m. UTC
I've built the pretest using MinGW32.  I found 2 issues: one with
gnulib's time.h (reported to the gnulib list), the other with the
recent changes in tui/.  Specifically, starting "gdb -tui" fails with
this error message:

     Cannot enable the TUI: error opening terminal [TERM=<unset>]

This happens because the recent additions to tui.c make assumptions
about the curses library that don't hold for the MinGW port of
ncurses, e.g. that $TERM must be set.

The patch below fixes this for me.  OK to install it (master and
branch)?

2015-01-15  Eli Zaretskii  <eliz@gnu.org>

	* gdb/tui/tui.c (tui_enable) [__MINGW32__]: If the call to 'newterm'
	fails with the 1st arg NULL, try again with "unknown".  Don't test
	the "cup" capability: it isn't supported by the Windows port of
	ncurses, but the Windows console driver is still capable of
	supporting TUI.

Comments

Doug Evans Jan. 15, 2015, 7:49 p.m. UTC | #1
On Thu, Jan 15, 2015 at 8:07 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> I've built the pretest using MinGW32.  I found 2 issues: one with
> gnulib's time.h (reported to the gnulib list), the other with the
> recent changes in tui/.  Specifically, starting "gdb -tui" fails with
> this error message:
>
>      Cannot enable the TUI: error opening terminal [TERM=<unset>]
>
> This happens because the recent additions to tui.c make assumptions
> about the curses library that don't hold for the MinGW port of
> ncurses, e.g. that $TERM must be set.
>
> The patch below fixes this for me.  OK to install it (master and
> branch)?
>
> 2015-01-15  Eli Zaretskii  <eliz@gnu.org>
>
>         * gdb/tui/tui.c (tui_enable) [__MINGW32__]: If the call to 'newterm'
>         fails with the 1st arg NULL, try again with "unknown".  Don't test
>         the "cup" capability: it isn't supported by the Windows port of
>         ncurses, but the Windows console driver is still capable of
>         supporting TUI.

Ok by me.
Eli Zaretskii Jan. 16, 2015, 8:49 a.m. UTC | #2
> Date: Thu, 15 Jan 2015 18:07:11 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> I've built the pretest using MinGW32.  I found 2 issues: one with
> gnulib's time.h (reported to the gnulib list)

The gnulib problem was fixed upstream, see

  http://lists.gnu.org/archive/html/bug-gnulib/2015-01/msg00044.html

Can we please import that fix into GDB, so that the next releases will
not have the problem?  (I don't know how to import from gnulib, let
alone do it for just some of the modules.)

TIA
Eli Zaretskii Jan. 19, 2015, 5:48 p.m. UTC | #3
> Date: Thu, 15 Jan 2015 11:49:06 -0800
> From: Doug Evans <dje@google.com>
> Cc: gdb-patches <gdb-patches@sourceware.org>
> 
> On Thu, Jan 15, 2015 at 8:07 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> > I've built the pretest using MinGW32.  I found 2 issues: one with
> > gnulib's time.h (reported to the gnulib list), the other with the
> > recent changes in tui/.  Specifically, starting "gdb -tui" fails with
> > this error message:
> >
> >      Cannot enable the TUI: error opening terminal [TERM=<unset>]
> >
> > This happens because the recent additions to tui.c make assumptions
> > about the curses library that don't hold for the MinGW port of
> > ncurses, e.g. that $TERM must be set.
> >
> > The patch below fixes this for me.  OK to install it (master and
> > branch)?
> >
> > 2015-01-15  Eli Zaretskii  <eliz@gnu.org>
> >
> >         * gdb/tui/tui.c (tui_enable) [__MINGW32__]: If the call to 'newterm'
> >         fails with the 1st arg NULL, try again with "unknown".  Don't test
> >         the "cup" capability: it isn't supported by the Windows port of
> >         ncurses, but the Windows console driver is still capable of
> >         supporting TUI.
> 
> Ok by me.

Thanks.  I'd like to hear from Pedro as well, as the changes which
caused this were committed by him.
Joel Brobecker Jan. 20, 2015, 6:27 p.m. UTC | #4
> The gnulib problem was fixed upstream, see
> 
>   http://lists.gnu.org/archive/html/bug-gnulib/2015-01/msg00044.html
> 
> Can we please import that fix into GDB, so that the next releases will
> not have the problem?  (I don't know how to import from gnulib, let
> alone do it for just some of the modules.)

No one's really answered, so I'll provide some feedback: We can add
one or more modules, but I'm pretty sure the code for all modules need
to be from the same version of gnulib. This means we need to import
a more recent version of gnulib, but IIRC, the past few upgrades
haven't always been completely smooth (although, perhaps it was
more about the fact that we were adding new modules, rather than
the fact that we were upgrading). In any case, doing the upgrade
itself is fairly simple, but one needs to have enough time afterwards
to follow up on any issues that come up from the update.

I'd do it for you, but I don't have much of the time to followup
on any breakage caused by it. If others are willing to help with
that, I could give the gnulib update a go.
Eli Zaretskii Jan. 20, 2015, 6:36 p.m. UTC | #5
> Date: Tue, 20 Jan 2015 19:27:41 +0100
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: gdb-patches@sourceware.org
> 
> > The gnulib problem was fixed upstream, see
> > 
> >   http://lists.gnu.org/archive/html/bug-gnulib/2015-01/msg00044.html
> > 
> > Can we please import that fix into GDB, so that the next releases will
> > not have the problem?  (I don't know how to import from gnulib, let
> > alone do it for just some of the modules.)
> 
> No one's really answered, so I'll provide some feedback: We can add
> one or more modules, but I'm pretty sure the code for all modules need
> to be from the same version of gnulib.

What about just "cherry-picking" that single change?  Or would that
cause trouble further down the line, when we do update gnulib?
Joel Brobecker Jan. 20, 2015, 7:08 p.m. UTC | #6
> > No one's really answered, so I'll provide some feedback: We can add
> > one or more modules, but I'm pretty sure the code for all modules need
> > to be from the same version of gnulib.
> 
> What about just "cherry-picking" that single change?  Or would that
> cause trouble further down the line, when we do update gnulib?

It doesn't work with the way we import gnulib - we import using
a SHA1 as a reference so that others can repeat the exact same
import when adding new modules. If you are curious, the script we use
is at gdb/gnulib/update-gnulib.sh (see GNULIB_COMMIT_SHA1).
Pedro Alves Jan. 22, 2015, 11:05 a.m. UTC | #7
Sorry for the delay.

On 01/19/2015 05:48 PM, Eli Zaretskii wrote:

> Thanks.  I'd like to hear from Pedro as well, as the changes which
> caused this were committed by him.

This is OK with me.

A couple questions though:

The "cup" check is there to make sure that e.g., starting GDB
in a shell within emacs doesn't result in a messed up session.
Did you try that?  I imagine that cases like when stdin is a
pipe, like e.g., when starting mingw gdb in a cygwin shell
or in a cygwin ssh session, may result in a messed up screen.

I mildly wonder whether pdcurses works here as is without
this patch, thus whether the #ifdef check should distinguish
ncurses from pdcurses somehow.  IIUC, some people build with
pdcurses instead of GNU ncurses.

Thanks,
Pedro Alves
Eli Zaretskii Jan. 22, 2015, 4:08 p.m. UTC | #8
> Date: Thu, 22 Jan 2015 11:05:09 +0000
> From: Pedro Alves <palves@redhat.com>
> CC: gdb-patches@sourceware.org
> 
> Sorry for the delay.
> 
> On 01/19/2015 05:48 PM, Eli Zaretskii wrote:
> 
> > Thanks.  I'd like to hear from Pedro as well, as the changes which
> > caused this were committed by him.
> 
> This is OK with me.

Thanks, I will push shortly.

> A couple questions though:
> 
> The "cup" check is there to make sure that e.g., starting GDB
> in a shell within emacs doesn't result in a messed up session.
> Did you try that?

You mean, start GDB under Emacs as

   gdb -tui -i=mi ...

?  It's a strange thing to do, but I did try it now, and didn't see
any problems.  Which isn't surprising: Emacs injects "TERM=emacs" into
the environment inherited by GDB, so the Windows ncurses driver
doesn't activate itself.

> I imagine that cases like when stdin is a pipe, like e.g., when
> starting mingw gdb in a cygwin shell or in a cygwin ssh session, may
> result in a messed up screen.

Why would it? pipes fail the isatty test.

> I mildly wonder whether pdcurses works here as is without
> this patch, thus whether the #ifdef check should distinguish
> ncurses from pdcurses somehow.

I have no idea, ncurses seems to be much more actively developed than
pdcurses, so I switched long ago.

> IIUC, some people build with pdcurses instead of GNU ncurses.

Indeed, so I hope they will speak up.
Pedro Alves Jan. 22, 2015, 5:03 p.m. UTC | #9
On 01/22/2015 04:08 PM, Eli Zaretskii wrote:

>> A couple questions though:
>>
>> The "cup" check is there to make sure that e.g., starting GDB
>> in a shell within emacs doesn't result in a messed up session.
>> Did you try that?
> 
> You mean, start GDB under Emacs as
> 
>    gdb -tui -i=mi ...

No, I mean, start a shell buffer in emacs, start gdb within that,
and do "layout src".

See https://sourceware.org/bugzilla/show_bug.cgi?id=17519.

Could you try that?

> 
> ?  It's a strange thing to do, but I did try it now, and didn't see
> any problems.  Which isn't surprising: Emacs injects "TERM=emacs" into
> the environment inherited by GDB, so the Windows ncurses driver
> doesn't activate itself.
> 
>> I imagine that cases like when stdin is a pipe, like e.g., when
>> starting mingw gdb in a cygwin shell or in a cygwin ssh session, may
>> result in a messed up screen.
> 
> Why would it? pipes fail the isatty test.

Right.  I recalled that Windows isatty returns true on all
sorts of character devices, like serial ports or the NUL device,
not just consoles, but confused pipes.  Pipes are not one of
those.  I see that gnulib has a isatty module that checks that
exactly -- it uses GetConsoleMode to make sure input is a real
console handle.  We don't import that gnulib module presently, but
if we need that console check it sounds like importing that
module would be way to fix it.

Thanks,
Pedro Alves
Eli Zaretskii Jan. 22, 2015, 5:27 p.m. UTC | #10
> Date: Thu, 22 Jan 2015 17:03:30 +0000
> From: Pedro Alves <palves@redhat.com>
> CC: dje@google.com, gdb-patches@sourceware.org
> 
> No, I mean, start a shell buffer in emacs, start gdb within that,
> and do "layout src".
> 
> See https://sourceware.org/bugzilla/show_bug.cgi?id=17519.
> 
> Could you try that?

It says "TUI mode not allowed".  (Tested in GDB 7.8.1 built with TUI,
I don't have a newer binary where I type this.)

> > Why would it? pipes fail the isatty test.
> 
> Right.  I recalled that Windows isatty returns true on all
> sorts of character devices, like serial ports or the NUL device,
> not just consoles, but confused pipes.  Pipes are not one of
> those.  I see that gnulib has a isatty module that checks that
> exactly -- it uses GetConsoleMode to make sure input is a real
> console handle.  We don't import that gnulib module presently, but
> if we need that console check it sounds like importing that
> module would be way to fix it.

Fix what?  TUI doesn't need this fix.  The only practical problem with
MS runtime's isatty is that the null device doesn't fail it, but
that's of a marginal importance for GDB, I think.  That issue is
important for filters and other batch-style programs where redirection
to or from the null device is frequently used.
Pedro Alves Jan. 22, 2015, 5:46 p.m. UTC | #11
On 01/22/2015 05:27 PM, Eli Zaretskii wrote:
>> Date: Thu, 22 Jan 2015 17:03:30 +0000
>> From: Pedro Alves <palves@redhat.com>
>> CC: dje@google.com, gdb-patches@sourceware.org
>>
>> No, I mean, start a shell buffer in emacs, start gdb within that,
>> and do "layout src".
>>
>> See https://sourceware.org/bugzilla/show_bug.cgi?id=17519.
>>
>> Could you try that?
> 
> It says "TUI mode not allowed".  (Tested in GDB 7.8.1 built with TUI,
> I don't have a newer binary where I type this.)

Ok, that's before my patch, but it means that isatty returned
false, so we're good after the patch too.

> 
>>> Why would it? pipes fail the isatty test.
>>
>> Right.  I recalled that Windows isatty returns true on all
>> sorts of character devices, like serial ports or the NUL device,
>> not just consoles, but confused pipes.  Pipes are not one of
>> those.  I see that gnulib has a isatty module that checks that
>> exactly -- it uses GetConsoleMode to make sure input is a real
>> console handle.  We don't import that gnulib module presently, but
>> if we need that console check it sounds like importing that
>> module would be way to fix it.
> 
> Fix what?  

Let me rephrase it:

 We don't import that gnulib module presently, but
 if we need that console check it sounds like importing that
 module would be way to add it.

Stress on the "IF".

> TUI doesn't need this fix.  The only practical problem with
> MS runtime's isatty is that the null device doesn't fail it, but
> that's of a marginal importance for GDB, I think.  That issue is
> important for filters and other batch-style programs where redirection
> to or from the null device is frequently used.

Since pipes already fail the isatty check, I agree it's of
marginal importance.

Thanks,
Pedro Alves
diff mbox

Patch

--- gdb/tui/tui.c~0	2015-01-13 14:14:48 +0200
+++ gdb/tui/tui.c	2015-01-15 10:52:01 +0200
@@ -425,6 +425,12 @@  tui_enable (void)
 	error (_("Cannot enable the TUI when output is not a terminal"));
 
       s = newterm (NULL, stdout, stdin);
+#ifdef __MINGW32__
+      /* The MinGW port of ncurses requires $TERM to be unset in order
+	 to activate the Windows console driver.  */
+      if (s == NULL)
+	s = newterm ("unknown", stdout, stdin);
+#endif
       if (s == NULL)
 	{
 	  error (_("Cannot enable the TUI: error opening terminal [TERM=%s]"),
@@ -432,7 +438,9 @@  tui_enable (void)
 	}
       w = stdscr;
 
-      /* Check required terminal capabilities.  */
+      /* Check required terminal capabilities.  The MinGW port of
+	 ncurses does have them, but doesn't expose them through "cup".  */
+#ifndef __MINGW32__
       cap = tigetstr ("cup");
       if (cap == NULL || cap == (char *) -1 || *cap == '\0')
 	{
@@ -442,6 +450,7 @@  tui_enable (void)
 		   "terminal doesn't support cursor addressing [TERM=%s]"),
 		 gdb_getenv_term ());
 	}
+#endif
 
       cbreak ();
       noecho ();