MinGW status for readline

MinGW status for readline

[Daniel Jacobowitz]

Hi Chet,

I just tried to use GDB with readline 5.1, on MinGW32.  I have four
items to report.  #1 and #4 have readline patches, #2 I'll fix in GDB,
#3 is a readline bug that I do not know how to fix - I'd appreciate
advice on that one!

1.  Readline 5.1 currently fails to build for MinGW, in tilde.c.  The
problem is:

      dirname = glue_prefix_and_suffix (user_entry->pw_dir, filename, user_len);

I've just moved the else branch inside the #if defined (HAVE_GETPWENT),
from just below.  With that change, readline builds.  Here's the patch:

2006-04-07  Daniel Jacobowitz  <dan@codesourcery.com>

	* tilde.c (tilde_expand_word): Don't use pw_dir if !HAVE_GETPWENT.

--- /space/symbian/readline/readline-5.1/tilde.c	2005-05-07 14:49:51.000000000 -0400
+++ ./tilde.c	2006-04-07 14:16:10.000000000 -0400
@@ -410,12 +410,12 @@ tilde_expand_word (filename)
       if (dirname == 0)
 	dirname = savestring (filename);
     }
+#if defined (HAVE_GETPWENT)
   else
     {
       free (username);
       dirname = glue_prefix_and_suffix (user_entry->pw_dir, filename, user_len);
     }
-#if defined (HAVE_GETPWENT)
   endpwent ();
 #endif
   return (dirname);


2.  The changes in the handling of multi-key sequences cause control to
return to GDB between \0340 and the following 'H' (two-character
sequence indicating an arrow key).  This used to happen entirely in
readline.  I don't think this is a bug in readline, though it did
surprise me.  It also broke GDB's select implementation for MinGW32,
but that's my problem, not yours.

3.  RL_STATE_MULTIKEY also broke macros which push multi-key sequences
into the input stream.  For instance, put this in .inputrc:
  Control-y: "\e[D"

Then try to use that.  It works in bash, which does not use callback
mode; it fails in GDB, which does.  All the other readline-using apps
I checked on my system don't use callbacks, so I couldn't be sure it
wasn't GDB's fault, but I don't think it is.

What happens in GDB is that rl_callback_read_char has special handling
for various states, including RL_STATE_MULTIKEY.  But if there's a
macro in progress, it just loops at the bottom checking
RL_STATE_MACROINPUT and calling readline_internal_char.  This doesn't
use the keymap saved in _rl_kscxt.  So the escape character is not
properly handled, and I get a beep followed by a literal [D.  I think
this is readline's fault.

4.  There's some use of uninitialized variables when checking for
pending input on mingw32, because we exercise the no-select code path,
so here's a patch to fix that.  I was hoping that this would fix #2,
but I'm going to have to fix that in GDB instead.  The patch is still
right, though, so please apply.  (The uninitialized variable was
chars_available.)

2006-04-07  Daniel Jacobowitz  <dan@codesourcery.com>

	* input.c (rl_gather_tyi): Use _kbhit for Windows consoles.
	(_rl_input_available): Likewise.

--- readline-5.1.orig/input.c	2005-07-04 22:30:24.000000000 -0400
+++ readline-5.1/input.c	2006-04-07 15:04:57.000000000 -0400
@@ -220,6 +220,16 @@ rl_gather_tyi ()
     }
 #endif /* O_NDELAY */
 
+#if defined (__MINGW32__)
+  /* We use getch to read console input, so use the same
+     mechanism to check for more.  Otherwise, we don't know.  */
+  if (isatty (fileno (rl_instream)))
+    chars_avail = _kbhit ();
+  else
+    chars_avail = 0;
+  result = 0;
+#endif
+
   /* If there's nothing available, don't waste time trying to read
      something. */
   if (chars_avail <= 0)
@@ -305,6 +315,13 @@ _rl_input_available ()
 
 #endif
 
+#if defined (__MINGW32__)
+  /* We use getch to read console input, so use the same
+     mechanism to check for more.  Otherwise, we don't know.  */
+  if (isatty (fileno (rl_instream)))
+    return _kbhit ();
+#endif
+
   return 0;
 }
 


-- 
Daniel Jacobowitz
CodeSourcery

Re: MinGW status for readline

[Bob Rossi]

> 3.  RL_STATE_MULTIKEY also broke macros which push multi-key sequences
> into the input stream.  For instance, put this in .inputrc:
>   Control-y: "\e[D"
> 
> Then try to use that.  It works in bash, which does not use callback
> mode; it fails in GDB, which does.  All the other readline-using apps
> I checked on my system don't use callbacks, so I couldn't be sure it
> wasn't GDB's fault, but I don't think it is.

I use callback mode. Did you apply these patches?
  ftp://ftp.gnu.org/gnu/readline/readline-5.1-patches/
One of them definatly fixed a bug with multi key for me.

Bob ROssi

Re: MinGW status for readline

[Daniel Jacobowitz]

On Fri, Apr 07, 2006 at 04:08:12PM -0400, Bob Rossi wrote:
> > 3.  RL_STATE_MULTIKEY also broke macros which push multi-key sequences
> > into the input stream.  For instance, put this in .inputrc:
> >   Control-y: "\e[D"
> > 
> > Then try to use that.  It works in bash, which does not use callback
> > mode; it fails in GDB, which does.  All the other readline-using apps
> > I checked on my system don't use callbacks, so I couldn't be sure it
> > wasn't GDB's fault, but I don't think it is.
> 
> I use callback mode. Did you apply these patches?
>   ftp://ftp.gnu.org/gnu/readline/readline-5.1-patches/
> One of them definatly fixed a bug with multi key for me.

Yes, sorry for not mentioning that.  I did apply those patches.

-- 
Daniel Jacobowitz
CodeSourcery

Re: MinGW status for readline

[Chet Ramey]

Daniel Jacobowitz wrote:

> 1.  Readline 5.1 currently fails to build for MinGW, in tilde.c.  The
> problem is:
> 
>       dirname = glue_prefix_and_suffix (user_entry->pw_dir, filename, user_len);
> 
> I've just moved the else branch inside the #if defined (HAVE_GETPWENT),
> from just below.  With that change, readline builds.  Here's the patch:

Thanks, though that patch introduces a memory leak.


> 2.  The changes in the handling of multi-key sequences cause control to
> return to GDB between \0340 and the following 'H' (two-character
> sequence indicating an arrow key).  This used to happen entirely in
> readline.  I don't think this is a bug in readline, though it did
> surprise me.  It also broke GDB's select implementation for MinGW32,
> but that's my problem, not yours.

One of the forces driving all of the contextual changes to readline
between versions 5.0 and 5.1 was the desire to avoid blocking reads
that weren't generated by the application.  This is one of those
places.  Even if readline thinks that this is the beginning of a
multi-character key sequence, it will still return control to the
application.

> 3.  RL_STATE_MULTIKEY also broke macros which push multi-key sequences
> into the input stream.  For instance, put this in .inputrc:
>   Control-y: "\e[D"
> 
> Then try to use that.  It works in bash, which does not use callback
> mode; it fails in GDB, which does.  All the other readline-using apps
> I checked on my system don't use callbacks, so I couldn't be sure it
> wasn't GDB's fault, but I don't think it is.

Thanks, I fixed it.

Chet

-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
( ``Discere est Dolere'' -- chet )
				       Live Strong.  No day but today.
Chet Ramey, ITS, CWRU    chet@case.edu    http://cnswww.cns.cwru.edu/~chet/

Re: MinGW status for readline

[Daniel Jacobowitz]

On Tue, Apr 11, 2006 at 09:44:42AM -0400, Chet Ramey wrote:
> Daniel Jacobowitz wrote:
> 
> > 1.  Readline 5.1 currently fails to build for MinGW, in tilde.c.  The
> > problem is:
> > 
> >       dirname = glue_prefix_and_suffix (user_entry->pw_dir, filename, user_len);
> > 
> > I've just moved the else branch inside the #if defined (HAVE_GETPWENT),
> > from just below.  With that change, readline builds.  Here's the patch:
> 
> Thanks, though that patch introduces a memory leak.

How?  Oh, if HAVE_GETPWNAME but not HAVE_GETPWENT?  I'm not sure why
you've got a call to endpwent there at all; I thought that only closed
getpwent, not getpwnam, and that path doesn't call getpwent.

> Thanks, I fixed it.

Great, thank you!

-- 
Daniel Jacobowitz
CodeSourcery

Re: MinGW status for readline

[Chet Ramey]

Daniel Jacobowitz wrote:
> On Tue, Apr 11, 2006 at 09:44:42AM -0400, Chet Ramey wrote:
>> Daniel Jacobowitz wrote:
>>
>>> 1.  Readline 5.1 currently fails to build for MinGW, in tilde.c.  The
>>> problem is:
>>>
>>>       dirname = glue_prefix_and_suffix (user_entry->pw_dir, filename, user_len);
>>>
>>> I've just moved the else branch inside the #if defined (HAVE_GETPWENT),
>>> from just below.  With that change, readline builds.  Here's the patch:
>> Thanks, though that patch introduces a memory leak.
> 
> How?  Oh, if HAVE_GETPWNAME but not HAVE_GETPWENT?  I'm not sure why
> you've got a call to endpwent there at all; I thought that only closed
> getpwent, not getpwnam, and that path doesn't call getpwent.

No, you never free username.  isolate_tilde_prefix allocates memory
whether you have the getpwent family available or not.

(I call endpwent() because its sole usual purpose is to close any open
file descriptors on the password database, whatever it is.  getpwent
and getpwnam usually keep the database open.)

Chet

-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
( ``Discere est Dolere'' -- chet )
				       Live Strong.  No day but today.
Chet Ramey, ITS, CWRU    chet@case.edu    http://cnswww.cns.cwru.edu/~chet/

Re: MinGW status for readline

[Daniel Jacobowitz]

On Tue, Apr 11, 2006 at 10:15:07AM -0400, Chet Ramey wrote:
> No, you never free username.  isolate_tilde_prefix allocates memory
> whether you have the getpwent family available or not.

It should be?  user_entry = 0, so the other branch of the if is taken.

> (I call endpwent() because its sole usual purpose is to close any open
> file descriptors on the password database, whatever it is.  getpwent
> and getpwnam usually keep the database open.)

Oh, huh.  So much for useful man pages.

-- 
Daniel Jacobowitz
CodeSourcery

Re: MinGW status for readline

[Chet Ramey]

Daniel Jacobowitz wrote:
> On Tue, Apr 11, 2006 at 10:15:07AM -0400, Chet Ramey wrote:
>> No, you never free username.  isolate_tilde_prefix allocates memory
>> whether you have the getpwent family available or not.
> 
> It should be?  user_entry = 0, so the other branch of the if is taken.

?

`username' is assigned before user_entry is.  It really should be freed
outside the if-then block, now that I'm looking at it.

>> (I call endpwent() because its sole usual purpose is to close any open
>> file descriptors on the password database, whatever it is.  getpwent
>> and getpwnam usually keep the database open.)
> 
> Oh, huh.  So much for useful man pages.

The BSD man pages, at least, say

	The endpwent() function closes any open files.

Chet
-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
( ``Discere est Dolere'' -- chet )
				       Live Strong.  No day but today.
Chet Ramey, ITS, CWRU    chet@case.edu    http://cnswww.cns.cwru.edu/~chet/

Re: MinGW status for readline

[Daniel Jacobowitz]

On Tue, Apr 11, 2006 at 11:05:03AM -0400, Chet Ramey wrote:
> Daniel Jacobowitz wrote:
> > On Tue, Apr 11, 2006 at 10:15:07AM -0400, Chet Ramey wrote:
> >> No, you never free username.  isolate_tilde_prefix allocates memory
> >> whether you have the getpwent family available or not.
> > 
> > It should be?  user_entry = 0, so the other branch of the if is taken.
> 
> ?
> 
> `username' is assigned before user_entry is.  It really should be freed
> outside the if-then block, now that I'm looking at it.

Well, yeah.  But:


#if defined (HAVE_GETPWNAM)
  user_entry = getpwnam (username);
#else
  user_entry = 0;
#endif
  if (user_entry == 0)
    {
      /* If the calling program has a special syntax for expanding tildes,
         and we couldn't find a standard expansion, then let them try.  */
      if (tilde_expansion_failure_hook)
        {
          expansion = (*tilde_expansion_failure_hook) (username);
          if (expansion)
            {
              dirname = glue_prefix_and_suffix (expansion, filename, user_len);
              free (expansion);
            }
        }
      free (username);
      /* If we don't have a failure hook, or if the failure hook did not
         expand the tilde, return a copy of what we were passed. */
      if (dirname == 0)
        dirname = savestring (filename);
    }
#if defined (HAVE_GETPWENT)
  else
    {
      free (username);
      dirname = glue_prefix_and_suffix (user_entry->pw_dir, filename, user_len);
    }
  endpwent ();
#endif

Suppose !HAVE_GETPWENT.  Then probably also !HAVE_GETPWNAM.  So
user_entry = 0, so the first half of the if is taken, and username gets
freed.

I suppose this would make more sense though:

#if defined (HAVE_GETPWNAM)
  else
    {
      free (username);
      dirname = glue_prefix_and_suffix (user_entry->pw_dir, filename, user_len);
    }
#endif
#if defined (HAVE_GETPWENT)
  endpwent ();
#endif

> 
> >> (I call endpwent() because its sole usual purpose is to close any open
> >> file descriptors on the password database, whatever it is.  getpwent
> >> and getpwnam usually keep the database open.)
> > 
> > Oh, huh.  So much for useful man pages.
> 
> The BSD man pages, at least, say
> 
> 	The endpwent() function closes any open files.
> 
> Chet
> -- 
> ``The lyf so short, the craft so long to lerne.'' - Chaucer
> ( ``Discere est Dolere'' -- chet )
> 				       Live Strong.  No day but today.
> Chet Ramey, ITS, CWRU    chet@case.edu    http://cnswww.cns.cwru.edu/~chet/
> 

-- 
Daniel Jacobowitz
CodeSourcery