From patchwork Fri Jan 6 10:25:38 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 62789 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 75E7038432EB for ; Fri, 6 Jan 2023 10:28:01 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 75E7038432EB DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1673000881; bh=1QNGNGvvgHJKqpFKcpBrXQWUkhPqlYz3Z01pzDcGsrQ=; h=To:Cc:Subject:Date:In-Reply-To:References:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=GaG2k+E8JC1+P3qzmWUPvz5s6ppf2baGCRVUSfuZfn1+j2lY5XXs2tVzKB6JdyRj1 sZTl7FSETxHXo5EAof6WP++XYiR8ozSoK/VHwpekxuTvgm+FEjhav8EaEkZBRynxMN VftN3sWmOLCskl7WZ7KkFaT5aQUbvVzYxVxWBZRo= X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 33B823858D39 for ; Fri, 6 Jan 2023 10:26:19 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 33B823858D39 Received: from mail-ej1-f71.google.com (mail-ej1-f71.google.com [209.85.218.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-343-EYh7UKNpMxOgL4UmSbXmHQ-1; Fri, 06 Jan 2023 05:26:17 -0500 X-MC-Unique: EYh7UKNpMxOgL4UmSbXmHQ-1 Received: by mail-ej1-f71.google.com with SMTP id gb10-20020a170907960a00b007c192c5482eso881201ejc.7 for ; Fri, 06 Jan 2023 02:26:17 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=1QNGNGvvgHJKqpFKcpBrXQWUkhPqlYz3Z01pzDcGsrQ=; b=uAbkERlO7tOA7vMw7DUsM23CSux38iKa5Sx1eabW7qbtgXVDi07yMeeX8p8+TfwFk7 HalMYEXdrMNr/6xSDvZNbeZ8K9zfn2diu68Z5nWiyi6HLKlaMX2ncYvmUP7FcAHPvIW1 JQ47CEnuEfxr79+APCwJP7Qfnui2qtaSJGr324xj4P49FX5Y37HcHuUEp6f3MbWQUROh V3LnRb4zQ2w7FgSSonr0N6ttSYMHg2dxjbQ6ctcdkAt/teG3Tl8Evy2PFMpOSOmzmdXI t6rjrzoY35CPE7egzD4ejm3gBwAvOIKDlhYL5ghXGhtTwGZpGUBwmhWMSO8a3YDcDW29 rqvw== X-Gm-Message-State: AFqh2kqPcOn82gsXp8DhyVfIxtPn0SGrs+lMl+bHv9+S59qaBM0t1F+H p2GJuCITwqugsFLe14JIUVbPOjTfcSAGGST9flmsRoOtRqNM/ifIFzPVOY/OCCXgMCpQfdE38Kl rDZdE21bqKD5TOFlzpzESNOkvKLMeFUjMnRqf/0iVbojTyeC984mcnUdCpfE9GLO2g7zlGHe3CA == X-Received: by 2002:a17:907:1392:b0:84d:1689:74f9 with SMTP id vs18-20020a170907139200b0084d168974f9mr2166065ejb.9.1673000776117; Fri, 06 Jan 2023 02:26:16 -0800 (PST) X-Google-Smtp-Source: AMrXdXuEYJtSv6yYG0T2psEq4uBZTYQxy49yBr30qlDuwg8KboTFR2wI21zQdOR0gFGrYoncb6Uknw== X-Received: by 2002:a17:907:1392:b0:84d:1689:74f9 with SMTP id vs18-20020a170907139200b0084d168974f9mr2166040ejb.9.1673000775520; Fri, 06 Jan 2023 02:26:15 -0800 (PST) Received: from localhost (95.72.115.87.dyn.plus.net. [87.115.72.95]) by smtp.gmail.com with ESMTPSA id k8-20020a1709062a4800b0083ffb81f01esm267076eje.136.2023.01.06.02.26.14 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 06 Jan 2023 02:26:15 -0800 (PST) To: gdb-patches@sourceware.org Cc: Andrew Burgess Subject: [PATCH 11/15] gdb/tui: rewrite of tui_source_window_base to handle very long lines Date: Fri, 6 Jan 2023 10:25:38 +0000 Message-Id: X-Mailer: git-send-email 2.25.4 In-Reply-To: References: MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-11.9 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Andrew Burgess via Gdb-patches From: Andrew Burgess Reply-To: Andrew Burgess Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org Sender: "Gdb-patches" This commit addresses an issue that is exposed by the test script gdb.tui/tui-disasm-long-lines.exp, that is, tui_source_window_base does not handle very long lines. The problem can be traced back to the newpad call in tui_source_window_base::show_source_content, this is where we allocate a backing pad to hold the window content. Unfortunately, there appears to be a limit to the size of pad that can be allocated, and the gdb.tui/tui-disasm-long-lines.exp test goes beyond this limit. As a consequence the newpad call fails and returns nullptr. It just so happens that the reset of the tui_source_window_base code can handle the pad being nullptr (this happens anyway when the window is first created, so we already depend on nullptr handling), so all that happens is the source window displays no content. ... well, sort of ... something weird does happen in the command window, we seem to see a whole bunch of blank lines. I've not bothered to track down exactly what's happening there, but it's some consequence of GDB attempting to write content to a WINDOW* that is nullptr. Before explaining my solution, I'll outline how things currently work: Consider we have the following window content to display: aaaaaaaaaa bbbbbbbbbbbbbbbbbbbb ccccccccccccccc the longest line here is 20 characters. If our display window is 10 characters wide, then we will create a pad that is 20 characters wide, and then copy the lines of content into the pad: .--------------------. |aaaaaaaaaa | |bbbbbbbbbbbbbbbbbbbb| |ccccccccccccccc | .--------------------. Now we will copy a 10 character wide view into this pad to the display, our display will then see: .----------. |aaaaaaaaaa| |bbbbbbbbbb| |cccccccccc| .----------. As the user scrolls left and right we adjust m_horizontal_offset and use this to select which part of the pad is copied onto the display. The benefit of this is that we only need to copy the content to the pad once, which includes processing the ansi escape sequences, and then the user can scroll left and right as much as they want relatively cheaply. The problem then, is that if the longest content line is very long, then we try to allocate a very large pad, which can fail. What I propose is that we allow both the pad and the display view to scroll. Once we allow this, then it becomes possible to allocate a pad that is smaller than the longest display line. We then copy part of the content into the pad. As the user scrolls the view left and right GDB will continue to copy content from the pad just as it does right now. But, when the user scrolls to the edge of the pad, GDB will copy a new block of content into the pad, and then update the view as normal. This all works fine so long as the maximum pad size is larger than the current window size - which seems a reasonable restriction, if ncurses can't support a pad of a given size it seems likely it will not support a display window of that size either. If we return to our example above, but this time we assume that the maximum pad size is 15 characters, then initially the pad would be loaded like this: .---------------. |aaaaaaaaaa | |bbbbbbbbbbbbbbb| |ccccccccccccccc| .---------------. Notice that the last 5 characters from the 'b' line are no longer included in the pad. There is still enough content though to fill the 10 character wide display, just as we did before. The pad contents remain unchanged until the user scrolls the display right to this point: .----------. |aaaaa | |bbbbbbbbbb| |cccccccccc| .----------. Now, when the user scrolls right once more GDB spots that the user has reached the end of the pad, and the pad contents are reloaded, like this: .---------------. |aaaaa | |bbbbbbbbbbbbbbb| |cccccccccc | .---------------. The display can now be updated from the pad again just like normal. With this change in place the gdb.tui/tui-disasm-long-lines.exp test now correctly loads the assembler code, and we can scroll around as expected. Most of the changes are pretty mundane, just updating to match the above. One interesting change though is the new member function tui_source_window_base::puts_to_pad_with_skip. This replaces direct calls to tui_puts when copying content to the pad. The content strings contain ansi escape sequences. When these strings are written to the pad these escape sequences are translated into ncurses attribute setting calls. Now however, we sometimes only write a partial string to the pad, skipping some of the leading content. Imagine then that we have a content line like this: "\033[31mABCDEFGHIJKLM\033[0m" Now the escape sequences in this content mean that the actual content (the 'ABCDEFGHIJKLM') will have a red foreground color. If we want to copy this to the pad, but skip the first 3 characters, then what we expect is to have the pad contain 'DEFGHIJKLM', but this text should still have a red foreground color. It is this problem that puts_to_pad_with_skip solves. This function skips some number of printable characters, but processes all the escape sequences. This means that when we do start printing the actual content the content will have the expected attributes. / --- .../gdb.tui/tui-disasm-long-lines.exp | 6 +- gdb/tui/tui-winsource.c | 177 ++++++++++++++++-- gdb/tui/tui-winsource.h | 54 +++++- 3 files changed, 215 insertions(+), 22 deletions(-) diff --git a/gdb/testsuite/gdb.tui/tui-disasm-long-lines.exp b/gdb/testsuite/gdb.tui/tui-disasm-long-lines.exp index acc4c54063f..345f7d21109 100644 --- a/gdb/testsuite/gdb.tui/tui-disasm-long-lines.exp +++ b/gdb/testsuite/gdb.tui/tui-disasm-long-lines.exp @@ -39,10 +39,6 @@ if {![Term::prepare_for_tui]} { return } -# Just check the command does not cause gdb to crash. It is worth -# noting that the asm window does infact fail to correctly display the -# disassembler output at this point, but initially we are just -# checking that GDB doesn't crash, fixing the asm display will come -# later. Term::command_no_prompt_prefix "layout asm" Term::check_box "asm box" 0 0 80 15 +Term::check_box_contents "check asm box contents" 0 0 80 15 "
" diff --git a/gdb/tui/tui-winsource.c b/gdb/tui/tui-winsource.c index 87099ac26f5..6e22638ec74 100644 --- a/gdb/tui/tui-winsource.c +++ b/gdb/tui/tui-winsource.c @@ -170,6 +170,7 @@ tui_source_window_base::update_source_window_as_is erase_source_content (); else { + validate_scroll_offsets (); update_breakpoint_info (nullptr, false); show_source_content (); update_exec_info (); @@ -231,6 +232,67 @@ tui_source_window_base::do_erase_source_content (const char *str) } } +/* See tui-winsource.h. */ + +void +tui_source_window_base::puts_to_pad_with_skip (const char *string, int skip) +{ + gdb_assert (m_pad.get () != nullptr); + WINDOW *w = m_pad.get (); + + while (skip > 0) + { + const char *next = strpbrk (string, "\033"); + + /* Print the plain text prefix. */ + size_t n_chars = next == nullptr ? strlen (string) : next - string; + if (n_chars > 0) + { + if (skip > 0) + { + if (skip < n_chars) + { + string += skip; + n_chars -= skip; + skip = 0; + } + else + { + skip -= n_chars; + string += n_chars; + n_chars = 0; + } + } + + if (n_chars > 0) + { + std::string copy (string, n_chars); + tui_puts (copy.c_str (), w); + } + } + + /* We finished. */ + if (next == nullptr) + break; + + gdb_assert (*next == '\033'); + + int n_read; + if (skip_ansi_escape (next, &n_read)) + { + std::string copy (next, n_read); + tui_puts (copy.c_str (), w); + next += n_read; + } + else + gdb_assert_not_reached ("unhandled escape"); + + string = next; + } + + if (*string != '\0') + tui_puts (string, w); +} /* Redraw the complete line of a source or disassembly window. */ void @@ -243,7 +305,8 @@ tui_source_window_base::show_source_line (int lineno) tui_set_reverse_mode (m_pad.get (), true); wmove (m_pad.get (), lineno, 0); - tui_puts (line->line.c_str (), m_pad.get ()); + puts_to_pad_with_skip (line->line.c_str (), m_pad_offset); + if (line->is_exec_point) tui_set_reverse_mode (m_pad.get (), false); } @@ -257,13 +320,25 @@ tui_source_window_base::refresh_window () the screen, potentially creating a flicker. */ wnoutrefresh (handle.get ()); - int pad_width = std::max (m_max_length, width); - int left_margin = 1 + TUI_EXECINFO_SIZE + extra_margin (); - int view_width = width - left_margin - 1; - int pad_x = std::min (pad_width - view_width, m_horizontal_offset); - /* Ensure that an equal number of scrolls will work if the user - scrolled beyond where we clip. */ - m_horizontal_offset = pad_x; + int pad_width = getmaxx (m_pad.get ()); + int left_margin = this->left_margin (); + int view_width = this->view_width (); + int content_width = m_max_length; + int pad_x = m_horizontal_offset - m_pad_offset; + + gdb_assert (m_pad_offset >= 0); + gdb_assert (m_horizontal_offset + view_width + <= std::max (content_width, view_width)); + gdb_assert (pad_x >= 0); + gdb_assert (m_horizontal_offset >= 0); + + /* This function can be called before the pad has been allocated, this + should only occur during the initial startup. In this case the first + condition in the following asserts will not be true, but the nullptr + check will. */ + gdb_assert (pad_width > 0 || m_pad.get () == nullptr); + gdb_assert (pad_x + view_width <= pad_width || m_pad.get () == nullptr); + prefresh (m_pad.get (), 0, pad_x, y + 1, x + left_margin, y + m_content.size (), x + left_margin + view_width - 1); } @@ -275,11 +350,51 @@ tui_source_window_base::show_source_content () check_and_display_highlight_if_needed (); - int pad_width = std::max (m_max_length, width); - if (m_pad == nullptr || pad_width > getmaxx (m_pad.get ()) - || m_content.size () > getmaxy (m_pad.get ())) - m_pad.reset (newpad (m_content.size (), pad_width)); + /* The pad should be at least as wide as the window, but ideally, as wide + as the content, however, for some very wide content this might not be + possible. */ + int required_pad_width = std::max (m_max_length, width); + int required_pad_height = m_content.size (); + + /* If the required pad width is wider than the previously requested pad + width, then we might want to grow the pad. */ + if (required_pad_width > m_pad_requested_width + || required_pad_height > getmaxy (m_pad.get ())) + { + /* The current pad width. */ + int pad_width = m_pad == nullptr ? 0 : getmaxx (m_pad.get ()); + + gdb_assert (pad_width <= m_pad_requested_width); + + /* If the current pad width is smaller than the previously requested + pad width, then this means we previously failed to allocate a + bigger pad. There's no point asking again, so we'll just make so + with the pad we currently have. */ + if (pad_width == m_pad_requested_width + || required_pad_height > getmaxy (m_pad.get ())) + { + pad_width = required_pad_width; + + do + { + /* Try to allocate a new pad. */ + m_pad.reset (newpad (required_pad_height, pad_width)); + + if (m_pad == nullptr) + { + int reduced_width = std::max (pad_width / 2, width); + if (reduced_width == pad_width) + error (_("failed to setup source window")); + pad_width = reduced_width; + } + } + while (m_pad == nullptr); + } + + m_pad_requested_width = required_pad_width; + } + gdb_assert (m_pad != nullptr); werase (m_pad.get ()); for (int lineno = 0; lineno < m_content.size (); lineno++) show_source_line (lineno); @@ -370,6 +485,35 @@ tui_source_window_base::refill () update_source_window_as_is (m_gdbarch, sal); } +/* See tui-winsource.h. */ + +bool +tui_source_window_base::validate_scroll_offsets () +{ + int original_pad_offset = m_pad_offset; + + if (m_horizontal_offset < 0) + m_horizontal_offset = 0; + + int content_width = m_max_length; + int pad_width = getmaxx (m_pad.get ()); + int view_width = this->view_width (); + + if (m_horizontal_offset + view_width > content_width) + m_horizontal_offset = std::max (content_width - view_width, 0); + + if ((m_horizontal_offset + view_width) > (m_pad_offset + pad_width)) + { + m_pad_offset = std::min (m_horizontal_offset, content_width - pad_width); + m_pad_offset = std::max (m_pad_offset, 0); + } + else if (m_horizontal_offset < m_pad_offset) + m_pad_offset = std::max (m_horizontal_offset + view_width - pad_width, 0); + + gdb_assert (m_pad_offset >= 0); + return (original_pad_offset != m_pad_offset); +} + /* Scroll the source forward or backward horizontally. */ void @@ -377,10 +521,11 @@ tui_source_window_base::do_scroll_horizontal (int num_to_scroll) { if (!m_content.empty ()) { - int offset = m_horizontal_offset + num_to_scroll; - if (offset < 0) - offset = 0; - m_horizontal_offset = offset; + m_horizontal_offset += num_to_scroll; + + if (validate_scroll_offsets ()) + show_source_content (); + refresh_window (); } } diff --git a/gdb/tui/tui-winsource.h b/gdb/tui/tui-winsource.h index bf0ca96c09b..2762afff010 100644 --- a/gdb/tui/tui-winsource.h +++ b/gdb/tui/tui-winsource.h @@ -181,16 +181,68 @@ struct tui_source_window_base : public tui_win_info /* Used for horizontal scroll. */ int m_horizontal_offset = 0; + /* Check that the current values of M_HORIZONTAL_OFFSET and M_PAD_OFFSET + make sense given the current M_MAX_LENGTH (content width), WIDTH + (window size), and window margins. After calling this function + M_HORIZONTAL_OFFSET and M_PAD_OFFSET might have been adjusted to + reduce unnecessary whitespace on the right side of the window. + + If M_PAD_OFFSET is adjusted then this function returns true + indicating that the pad contents need to be reloaded by calling + show_source_content. If M_PAD_OFFSET is not adjusted then this + function returns false, the window contents might still need + redrawing if M_HORIZONTAL_OFFSET was adjusted, but right now, this + function is only called in contexts where the window is going to be + redrawn anyway. */ + bool validate_scroll_offsets (); + + /* Return the size of the left margin space, this is the space used to + display things like breakpoint markers. */ + int left_margin () const + { return 1 + TUI_EXECINFO_SIZE + extra_margin (); } + + /* Return the width of the area that is available for window content. + This is the window width minus the borders and the left margin, which + is used for displaying things like breakpoint markers. */ + int view_width () const + { return width - left_margin () - 1; } + void show_source_content (); + /* Write STRING to the window M_PAD, but skip the first SKIP printable + characters. Any escape sequences within the first SKIP characters are + still processed though. This means if we have this string: + + "\033[31mABCDEFGHIJKLM\033[0m" + + and call this function with a skip value of 3, then we effectively + write this string to M_PAD: + + "\033[31mDEFGHIJKLM\033[0m" + + the initial escape that sets the color will still be applied. */ + void puts_to_pad_with_skip (const char *string, int skip); + /* Called when the user "set style enabled" setting is changed. */ void style_changed (); /* A token used to register and unregister an observer. */ gdb::observers::token m_observable; - /* Pad used to display fixme mumble */ + /* Pad to hold some, or all, of the window contents. Content is then + copied from this pad to the screen as the user scrolls horizontally, + this avoids the need to recalculate the screen contents each time the + user does a horizontal scroll. */ std::unique_ptr m_pad; + + /* When M_PAD was allocated, this holds the width that was initially + asked for. If we ask for a very large pad then the allocation may + fail, and we might instead allocate a narrower pad. */ + int m_pad_requested_width = 0; + + /* If M_PAD is not as wide as the content (so less than M_MAX_LENGTH) + then this value indicates the offset at which the pad contents begin. */ + int m_pad_offset = 0; };