Re: [PATCH v3 4/4] [gdb/cli] Allow source highlighting to be interrupted

Tom de Vries
2023-10-16T11:26:53Z

On 10/16/23 12:21, Lancelot SIX wrote:
> Hi Tom,
>
> I have one minor nit below.
>
> On Mon, Oct 16, 2023 at 11:17:48AM +0200, Tom de Vries wrote:
>> In PR cli/30934, a problem is reported where gdb becomes unresponsive for
>> 2m13s because the GNU source-highlight library is taking a lot of time to
>> annotate the source.
>>
>> This is due to a problem in the library [1], for which I've posted a
>> patch [2], which brings down the annotation time to 3s.
>>
>> However, GDB should not become unresponsive due to such a problem.
>>
>> Fix this by installing a srchilite::HighlightEventListener that:
>> - checks whether ^C was pressed, and if so
>> - asks the user whether it should interrupt highlighting, and if so
>> - abandons the highlighting by throwing an exception.
>>
>> Interrupting the highlighting looks like this to the user:
>> ...
>> $ gdb -q a.out -ex "b 56"
>> Reading symbols from a.out...
>> Breakpoint 1 at 0x400e2a: file test.cpp, line 56.
>> (gdb) r
>> Starting program: /data/vries/gdb/a.out
>>
>> Breakpoint 1, Solution::numOfSubarrays () at test.cpp:56
>> ^CCancel source styling using GNU source highlight of /data/vries/gdb/test.cpp?
>> ([y] or n) y
>> 56 return (int) t;
>> (gdb)
>> ...
>> Note that line 56 still can be highlighted, just by pygments instead of
>> source-highlight.
>>
>> Tested on x86_64-linux.
>>
>> Co-Authored-By: Lancelot Six <lancelot.six@amd.com>
>>
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30934
>>
>> [1] https://savannah.gnu.org/bugs/?64747
>> [2] https://savannah.gnu.org/patch/index.php?10400
>> ---
>> gdb/source-cache.c | 68 ++++++++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 66 insertions(+), 2 deletions(-)
>>
>> diff --git a/gdb/source-cache.c b/gdb/source-cache.c
>> index aa8e40425c2..39d8bcf7aec 100644
>> --- a/gdb/source-cache.c
>> +++ b/gdb/source-cache.c
>> @@ -37,6 +37,7 @@
>> #include <sstream>
>> #include <srchilite/sourcehighlight.h>
>> #include <srchilite/langmap.h>
>> +#include <srchilite/highlighteventlistener.h>
>> #endif
>>
>> /* The number of source files we'll cache. */
>> @@ -189,6 +190,48 @@ get_language_name (enum language lang)
>> return nullptr;
>> }
>>
>> +/* Class with notify function called during highlighting. */
>> +
>> +class gdb_highlight_event_listener : public srchilite::HighlightEventListener
>> +{
>> +public:
>> + gdb_highlight_event_listener (const std::string &fullname)
>> + : m_fullname (fullname)
>> + {
>> + }
>> +
>> + void notify(const srchilite::HighlightEvent &event) override
> ^
> A space is missing here before the "(".
>
> Otherwise, and FWIW, this LGTM.
>

Hi,

thanks for the review.

It looks like I need to start using gcc's check_GNU_style.sh /
check_GNU_style.py again.

I fixed this, as well as a few other style issues in the series that I
found by running the scripts, but I don't think it requires reposting.

Thanks,
- Tom

> Best, Lancelot.
>
>> + {
>> + /* If the terminal is ours, we can ask the user a question and get an
>> + answer. */
>> + gdb_assert (target_terminal::is_ours ());
>> +
>> + if (!check_quit_flag ())
>> + {
>> + /* User didn't press ^C, nothing to do. */
>> + return;
>> + }
>> +
>> + /* Ask the user what to do. */
>> + int resp
>> + = yquery (_("Cancel source styling using GNU source highlight of %s?\n"),
>> + m_fullname.c_str ());
>> + if (!resp)
>> + {
>> + /* Continue highlighting. */
>> + return;
>> + }
>> +
>> + /* Interrupt highlighting. Note that check_quit_flag clears the
>> + quit_flag, so we have to set it again. */
>> + set_quit_flag ();
>> + QUIT;
>> + }
>> +
>> +private:
>> + const std::string &m_fullname;
>> +};
>> +
>> #endif /* HAVE_SOURCE_HIGHLIGHT */
>>
>> /* Try to highlight CONTENTS from file FULLNAME in language LANG using
>> @@ -223,9 +266,27 @@ try_source_highlight (std::string &contents ATTRIBUTE_UNUSED,
>> highlighter->setStyleFile ("esc.style");
>> }
>>
>> + gdb_highlight_event_listener event_listener (fullname);
>> + highlighter->setHighlightEventListener (&event_listener);
>> + /* Make sure that the highlighter's EventListener doesn't become a
>> + dangling pointer. */
>> + auto clear_event_listener = make_scope_exit ([]()
>> + {
>> + highlighter->setHighlightEventListener (nullptr);
>> + });
>> +
>> std::istringstream input (contents);
>> std::ostringstream output;
>> - highlighter->highlight (input, output, lang_name, fullname);
>> + {
>> + /* We want to be able to interrupt the call to source-highlight. If
>> + the current quit_handler is infrun_quit_handler, pressing ^C is
>> + ignored by QUIT (assuming target_terminal::is_ours), so install the
>> + default quit handler. */
>> + scoped_restore restore_quit_handler
>> + = make_scoped_restore (&quit_handler, default_quit_handler);
>> +
>> + highlighter->highlight (input, output, lang_name, fullname);
>> + }
>> contents = std::move (output).str();
>> styled = true;
>> }
>> @@ -304,13 +365,16 @@ source_cache::ensure (struct symtab *s)
>> reasons:
>> - the language is not supported.
>> - the language cannot not be auto-detected from the file name.
>> + - styling took too long and was interrupted by the user.
>> - no stylers available.
>>
>> Since styling failed, don't try styling the file again after it
>> drops from the cache.
>>
>> Note that clearing the source cache also clears
>> - m_no_styling_files. */
>> + m_no_styling_files, so if styling took too long, and the user
>> + interrupted it, and the source cache gets cleared, the user will
>> + need to interrupt styling again. */
>> m_no_styling_files.insert (fullname);
>> }
>> }
>> --
>> 2.35.3
>>