Re: [PATCHv2 1/3] gdb: work around prompt corruption caused by bracketed-paste-mode

[Andrew Burgess <aburgess@redhat.com>]

* Simon Marchi <simark@simark.ca> [2022-03-25 14:32:42 -0400]:

> On 2022-03-14 06:23, Andrew Burgess via Gdb-patches wrote:
> > In this commit:
> >
> >   commit b4f26d541aa7224b70d363932e816e6e1a857633
> >   Date:   Tue Mar 2 13:42:37 2021 -0700
> >
> >       Import GNU Readline 8.1
> >
> > We imported readline 8.1 into GDB.  As a consequence bug PR cli/28833
> > was reported.  This bug spotted that, when the user terminated GDB by
> > sending EOF (usually bound to Ctrl+d), the last prompt would become
> > corrupted.  Here's what happens, the user is sat at a prompt like
> > this:
> >
> >   (gdb)
> >
> > And then the user sends EOF (Ctrl+d), we now see this:
> >
> >   quit)
> >   ... gdb terminates, and we return to the shell ...
> >
> > Notice the 'quit' was printed over the prompt.
> >
> > This problem is a result of readline 8.1 enabling bracketed paste mode
> > by default.  This problem is present in readline 8.0 too, but in that
> > version of readline bracketed paste mode is off by default, so a user
> > will not see the bug unless they specifically enable the feature.
> >
> > Bracketed paste mode is available in readline 7.0 too, but the bug
> > is not present in this version of readline, see below for why.
> >
> > What causes this problem is how readline disables bracketed paste
> > mode.  Bracketed paste mode is a terminal feature that is enabled and
> > disabled by readline emitting a specific escape sequence.  The problem
> > for GDB is that the escape sequence to disable bracketed paste mode
> > includes a '\r' character at the end, see this thread for more
> > details:
> >
> >   https://lists.gnu.org/archive/html/bug-bash/2018-01/msg00097.html
> >
> > The change to add the '\r' character to the escape sequence used to
> > disable bracketed paste mode was introduced between readline 7.0 and
> > readline 8.0, this is why the bug would not occur when using older
> > versions of readline (note: I don't know if its even possible to build
> > GDB using readline 7.0.  That really isn't important, I'm just
> > documenting the history of this issue).
> >
> > So, the escape sequence to disable bracketed paste mode is emitted
> > from the readline function rl_deprep_terminal, this is called after
> > the user has entered a complete command and pressed return, or, if the
> > user sends EOF.
> >
> > However, these two cases are slightly different.  In the first case,
> > when the user has entered a command and pressed return, the cursor
> > will have moved to the next, empty, line, before readline emits the
> > escape sequence to leave bracketed paste mode.  The final '\r'
> > character moves the cursor back to the beginning of this empty line,
> > which is harmless.
> >
> > For the EOF case though, this is not what happens.  Instead, the
> > escape sequence to leave bracketed paste mode is emitted on the same
> > line as the prompt.  The final '\r' moves the cursor back to the start
> > of the prompt line.  This leaves us ready to override the prompt.
> >
> > It is worth noting, that this is not the intended behaviour of
> > readline, in rl_deprep_terminal, readline should emit a '\n' character
> > when EOF is seen.  However, due to a bug in readline this does not
> > happen (the _rl_eof_found flag is never set).  This is the first
> > readline bug that effects GDB.
> >
> > GDB prints the 'quit' message from command_line_handler (in
> > event-top.c), this function is called (indirectly) from readline to
> > process the complete command line, but also in the EOF case (in which
> > case the command line is set to nullptr).  As this is part of the
> > callback to process a complete command, this is called after readline
> > has disabled bracketed paste mode (by calling rl_deprep_terminal).
> >
> > And so, when bracketed paste mode is in use, rl_deprep_terminal leaves
> > the cursor at the start of the prompt line (in the EOF case), and
> > command_line_handler then prints 'quit', which overwrites the prompt.
> >
> > The solution to this problem is to print the 'quit' message earlier,
> > before rl_deprep_terminal is called.  This is easy to do by using the
> > rl_deprep_term_function hook.  It is this hook that usually calls
> > rl_deprep_terminal, however, if we replace this with a new function,
> > we can print the 'quit' string, and then call rl_deprep_terminal
> > ourselves.  This allows the 'quit' to be printed before
> > rl_deprep_terminal is called.
> >
> > The problem here is that there is no way in rl_deprep_terminal to know
> > if readline is processing EOF or not, and as a result, we don't know
> > when we should print 'quit'.  This is the second readline bug that
> > effects GDB.
> >
> > Both of these readline issues are discussed in this thread:
> >
> >   https://lists.gnu.org/archive/html/bug-readline/2022-02/msg00021.html
> >
> > The result of that thread was that readline was patched to address
> > both of these issues.
> >
> > Now it should be easy to backport the readline fix to GDB's in tree
> > copy of readline, and then change GDB to make use of these fixes to
> > correctly print the 'quit' string.
> >
> > However, we are just about to branch GDB 12, and there is concern from
> > some that changing readline this close to a new release is a risky
> > idea, see this thread:
> >
> >   https://sourceware.org/pipermail/gdb-patches/2022-March/186391.html
> >
> > So, this commit doesn't change readline at all.  Instead, this commit
> > is the smallest possible GDB change in order to avoid the prompt
> > corruption.
> >
> > In this commit I change GDB to print the 'quit' string on the line
> > after the prompt, but only when bracketed paste mode is on.  This
> > avoids the overwriting issue, the user sees this:
> >
> >   (gdb)
> >   quit
> >   ... gdb terminates, and returns to the shell ...
> >
> > This isn't ideal, but is better than the existing behaviour.  After
> > GDB 12 has branched, we can backport the readline fix, and apply a
> > real fix to GDB.
> >
> > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28833
> 
> Hi Andrew,
> 
> I see this test fail on the following setup:
> 
>  - Ubuntu 18.04
>  - Building with --with-system-readline (distro package libreadline-dev
>    installed, which is readline 7.0)
> 
> 
> ^[[?2004h(gdb) set height 0^M
> ^[[?2004l^[[?2004h(gdb) set width 0^M
> ^[[?2004l^[[?2004h(gdb) dir^M
> ^[[?2004l^[[?2004hReinitialize source path to empty? (y or n) y^M
> ^[[?2004lSource directories searched: $cdir:$cwd^M
> ^[[?2004h(gdb) dir /build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.base^M
> ^[[?2004lSource directories searched: /build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.base:$cdir:$cwd^M
> ^[[?2004h(gdb) ^M
> ^[[?2004l^[[?2004h(gdb) ^[[?2004l^M
> quit^M
> FAIL: gdb.base/eof-exit.exp: with non-dump terminal: with bracketed-paste-mode on: close GDB with eof (missed the prompt)
> 
> 
> It's clearly related to bracketed-paste-mode, since these 2004 escape
> sequences turn bracketed-paste-mode on and off (accordin to
> https://wiki2.org/en/ISO/IEC_6429).  But I don't know what happens
> exactly.
> 
> Here's a Dockerfile that can be used to reproduce the failure, hopefully
> it makes it easier:
> 
>     FROM ubuntu:18.04
>     RUN apt-get -y update && \
>       apt-get -y full-upgrade && \
>       DEBIAN_FRONTEND=noninteractive TZ=Etc/UTC apt-get -y install build-essential flex bison git texinfo libreadline-dev dejagnu libexpat1-dev libgmp-dev
>     RUN git clone --depth 1 git://sourceware.org/git/binutils-gdb.git
>     RUN mkdir build
>     WORKDIR /build
>     RUN ../binutils-gdb/configure --with-system-readline
>     RUN make -j $(nproc) all-gdb
>     WORKDIR /build/gdb
>     RUN make check TESTS="gdb.base/eof-exit.exp"

Thanks for the awesome steps to reproduce!  This made it super easy to
track down the problem.

Basically the problem is that I only took readline 8+ into account
when writing the test, this spin of ubuntu is on readline 7.

The patch below changes the test FAIL into a KFAIL, which, if you were
on readline 8 would be the best we could do.

But given you're on readline 7, we should be able to do better,
getting this to a PASS!  But that will require changes to GDB itself.

I'd like to propose that first we merge the patch below, this removes
the FAIL, then next week I'll post a follow on patch for GDB that
should get this test PASSing for readline 7.

Thoughts?

Thanks,
Andrew

---

commit 7a0add556420e2ef814a6cd58501a832fdb1fb90
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Sat Mar 26 13:41:33 2022 +0000

    gdb/testsuite: fix test failure when building against readline v7
    
    The test added in the commit:
    
      commit a6b413d24ccc5d76179bab866834e11fd6fec294
      Date:   Fri Mar 11 14:44:03 2022 +0000
    
          gdb: work around prompt corruption caused by bracketed-paste-mode
    
    Was not written with readline 7 in mind, only readline 8+.  Between
    readline 7 and 8 the escape sequence used to disable bracketed paste
    mode changed, an additional '\r' character was added to the end.  In
    fact, it was the addition of this '\r' character that triggered the
    issue for which the above commit is part of the solution.
    
    Anyway, the test tries to spot the case where the output from GDB is
    not perfect, but does have the above work around applied.  However,
    the pattern in the test assume that the problematic '\r' will be
    present, and this is only true for readline 8+.  With readline 7 the
    test was failing.
    
    In this commit I generalise the pattern a little so that the test will
    still KFAIL with readline 7.
    
    It's a little unfortunate that the test is KFAILing with readline 7,
    as without the problematic '\r' there's actually no reason that GDB
    couldn't "do the right thing" in this case, in which case, the test
    would PASS, but that would require changes within GDB itself.
    
    My preference then is that initially we patch the test to get it
    KFAILing, then in a separate commit I can modify GDB so that it can
    PASS with readline 7.

diff --git a/gdb/testsuite/gdb.base/eof-exit.exp b/gdb/testsuite/gdb.base/eof-exit.exp
index d604d029974..2d9530ccebe 100644
--- a/gdb/testsuite/gdb.base/eof-exit.exp
+++ b/gdb/testsuite/gdb.base/eof-exit.exp
@@ -38,7 +38,7 @@ proc run_test {} {
 	-re "$::gdb_prompt \[^\n\]*\r\[^\n\]*quit" {
 	    fail "$gdb_test_name (misplaced \\r)"
 	}
-	-re "$::gdb_prompt \[^\n\]*\r\[^\n\]*\r\nquit\r\n" {
+	-re "$::gdb_prompt (?:\[^\n\]*\r)?\[^\n\]*\r\nquit\r\n" {
 	    # For versions of readline that don't include the
 	    # RL_STATE_EOF patch, then the 'quit' is printed on the
 	    # subsequent line.