Patchwork Readline: Cleanup some warnings

login
register
mail settings
Submitter Alan Hayward
Date Jan. 31, 2019, 5:24 p.m.
Message ID <3463805B-A8BF-4C20-ACE3-C21AE3F7DB62@arm.com>
Download mbox | patch
Permalink /patch/31259/
State New
Headers show

Comments

Alan Hayward - Jan. 31, 2019, 5:24 p.m.
> On 31 Jan 2019, at 10:02, Alan Hayward <Alan.Hayward@arm.com> wrote:

> 

> 

> 

>> On 31 Jan 2019, at 07:59, Joel Brobecker <brobecker@adacore.com> wrote:

>> 

>>> (Posted this first to binutils, then was directed back here and

>>> pointed at the upstream readline.  Looking at the upstream

>>> readline it has already fixed the issues below in the same way.

>>> Tested by running the gdb testsuite - couldn't see a readline

>>> specific suite.) 

>>> 

>>> Cleanup the readline warnings that gdb buildbot complains about.

>>> 

>>> To prevent wcwidth missing declaration warnings, add the SOURCE /

>>> EXTENSION macros to config.in that have already checked for in

>>> configure.  Use the exact same list as GDB - it seemed sensible

>>> to add all of them.

>>> 

>>> Ensure pid is a long before printing as one.  Also fix GNU style.

>>> 

>>> Check the return value of write the same way as history_do_write ().

>>> 

>>> These changes are consistent with upstream readline.

>>> 

>>> readline/ChangeLog.gdb:

>>> 

>>> 2019-01-30  Alan Hayward  <alan.hayward@arm.com>

>>> 

>>> 	* config.h.in: Add SOURCE/EXTENSION macros.

>>> 	* histfile.c (history_truncate_file): Check return of write.

>>> 	* util.c: Ensure pid is long.

>> 

>> If it is a backport

> 

> Technically, not a back port because I wrote the changes then realised

> they are the same as upstream.  Just a quick thought - would it help

> with future rebasing if I ensured the changes were *exactly* the same?

> (For example, the config.h.in changes are in a different place in the 

> file with different comments).

> 

>> from mainline readline, and you've run

>> the testsuite (readline is being necessarily extensively covered,

>> but at least it is used implicitly, since it provides the framework

>> for the interactive prompt, which is being driven via expect/tcl),

>> it's OK to push.


Thinking about what you said, I’ve updated the config.h.in code so it
is a direct backport, and pushed.  Functionally it’s the same as the
original version. Patch pasted below.
Joel Brobecker - Feb. 1, 2019, 8:05 a.m.
> >> If it is a backport
> > 
> > Technically, not a back port because I wrote the changes then realised
> > they are the same as upstream.  Just a quick thought - would it help
> > with future rebasing if I ensured the changes were *exactly* the same?
> > (For example, the config.h.in changes are in a different place in the 
> > file with different comments).
> > 
> >> from mainline readline, and you've run
> >> the testsuite (readline is being necessarily extensively covered,
> >> but at least it is used implicitly, since it provides the framework
> >> for the interactive prompt, which is being driven via expect/tcl),
> >> it's OK to push.
> 
> Thinking about what you said, I’ve updated the config.h.in code so it
> is a direct backport, and pushed.  Functionally it’s the same as the
> original version. Patch pasted below.

Thank you. It's always better if it is an exact backport, because
it facilitates future resyncs with the upstream versions.
Tom Tromey - Feb. 1, 2019, 12:47 p.m.
>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> Thank you. It's always better if it is an exact backport, because
Joel> it facilitates future resyncs with the upstream versions.

Speaking of, I still have a branch with a newer import on it.  It could
use some more testing.  It's been a while since I poked at this, so I
don't really remember, but I think it fails a couple of gdb tests.  So,
some readline debugging is also needed.

Tom
Philippe Waroquiers - Feb. 1, 2019, 6:54 p.m.
On Fri, 2019-02-01 at 05:47 -0700, Tom Tromey wrote:
> > > > > > "Joel" == Joel Brobecker <brobecker@adacore.com> writes:
> 
> Joel> Thank you. It's always better if it is an exact backport, because
> Joel> it facilitates future resyncs with the upstream versions.
> 
> Speaking of, I still have a branch with a newer import on it.  It could
> use some more testing.  It's been a while since I poked at this, so I
> don't really remember, but I think it fails a couple of gdb tests.  So,
> some readline debugging is also needed.
To solve a leak, I now configure/build GDB to use
the debian stable readline, and I only have one more
test failing compared to the GDB 6.2:

"This 'by design' leak is fixed in readline 7, while
the GDB readline version is 6.2 according to the last
'import' message in readline ChangeLog.gdb.

On my system (debian 9.6), the leaks are fixed by using
   --with-system-readline=yes
with the system readline version being 7.0-3

Switching to this readline version causes the following checks to fail:
FAIL: gdb.gdb/selftest.exp: send SIGINT signal to child process (timeout)
FAIL: gdb.gdb/selftest.exp: thread 1 (timeout)
FAIL: gdb.gdb/selftest.exp: backtrace through signal handler (timeout)

Apart of that, no problem seen."

Philippe
Pedro Alves - Feb. 6, 2019, 7:56 p.m.
On 02/01/2019 06:54 PM, Philippe Waroquiers wrote:
> 
> Switching to this readline version causes the following checks to fail:
> FAIL: gdb.gdb/selftest.exp: send SIGINT signal to child process (timeout)
> FAIL: gdb.gdb/selftest.exp: thread 1 (timeout)
> FAIL: gdb.gdb/selftest.exp: backtrace through signal handler (timeout)
> 
> Apart of that, no problem seen."

See the description in commit 4a11f2065906, which was later
reverted:

~~~
    Sync readline/ to version 7.0 alpha
...
    After the sync there is one testsuite regression, the test
    "signal SIGINT" in gdb.gdb/selftest.exp which now FAILs.  Previously,
    the readline 6.2 SIGINT handler would temporarily reinstall the
    underlying application's SIGINT handler and immediately re-raise SIGINT
    so that the orginal handler gets invoked.  But now (since readline 6.3)
    its SIGINT handler does not re-raise SIGINT or directly invoke the
    original handler; it now sets a flag marking that SIGINT was raised, and
    waits until readline explicitly has control to call the application's
    SIGINT handler.  Anyway, because SIGINT is no longer re-raised from
    within readline's SIGINT handler, doing "signal SIGINT" with a stopped
    inferior gdb process will no longer resume and then immediately stop the
    process (since there is no 2nd SIGINT to immediately catch).  Instead,
    the inferior gdb process will now just print "Quit" and continue to run.
    So with this commit, this particular test case is adjusted to reflect
    this change in behavior (we now have to send a 2nd SIGINT manually to
    stop it).
~~~

Thanks,
Pedro Alves

Patch

diff --git a/readline/config.h.in b/readline/config.h.in

index 86d86cfa3d..c194e761a4 100644

--- a/readline/config.h.in

+++ b/readline/config.h.in

@@ -1,5 +1,15 @@ 

 /* config.h.in.  Maintained by hand. */

+/* Template definitions for autoconf */

+#undef __EXTENSIONS__

+#undef _ALL_SOURCE

+#undef _GNU_SOURCE

+#undef _POSIX_SOURCE

+#undef _POSIX_1_SOURCE

+#undef _POSIX_PTHREAD_SEMANTICS

+#undef _TANDEM_SOURCE

+#undef _MINIX

+

 /* Define NO_MULTIBYTE_SUPPORT to not compile in support for multibyte
    characters, even if the OS supports them. */
 #undef NO_MULTIBYTE_SUPPORT
diff --git a/readline/histfile.c b/readline/histfile.c

index fffeb3fd31..56cbbf0498 100644

--- a/readline/histfile.c

+++ b/readline/histfile.c

@@ -407,7 +407,8 @@  history_truncate_file (fname, lines)

      truncate to. */
   if (bp > buffer && ((file = open (filename, O_WRONLY|O_TRUNC|O_BINARY, 0600)) != -1))
     {
-      write (file, bp, chars_read - (bp - buffer));

+      if (write (file, bp, chars_read - (bp - buffer)) < 0)

+       rv = errno;


 #if defined (__BEOS__)
       /* BeOS ignores O_TRUNC. */
diff --git a/readline/util.c b/readline/util.c

index d402fce842..13bd00c09c 100644

--- a/readline/util.c

+++ b/readline/util.c

@@ -515,11 +515,11 @@  _rl_tropen ()

           (sh_get_env_value ("TEMP")
            ? sh_get_env_value ("TEMP")
            : "."),
-          getpid());

+          getpid ());

 #else
-  sprintf (fnbuf, "/var/tmp/rltrace.%ld", getpid());

+  sprintf (fnbuf, "/var/tmp/rltrace.%ld", (long) getpid ());

 #endif
-  unlink(fnbuf);

+  unlink (fnbuf);

   _rl_tracefp = fopen (fnbuf, "w+");
   return _rl_tracefp != 0;
 }