[v2,4/4,gdb/cli] Allow source highlighting to be interrupted
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_gdb_build--master-arm |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-arm |
success
|
Testing passed
|
Commit Message
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.
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 | 62 ++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 60 insertions(+), 2 deletions(-)
Comments
Hi Tom,
I have a comment below.
On Fri, Oct 13, 2023 at 02:19:53PM +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.
>
> 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 | 62 ++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 60 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/source-cache.c b/gdb/source-cache.c
> index f8c98d7e23f..57d9f3b8ab4 100644
> --- a/gdb/source-cache.c
> +++ b/gdb/source-cache.c
> @@ -223,9 +266,21 @@ try_source_highlight (std::string &contents ATTRIBUTE_UNUSED,
> highlighter->setStyleFile ("esc.style");
> }
>
> + gdb_highlight_event_listener event_listener (fullname);
> + highlighter->setHighlightEventListener (&event_listener);
In this instance, highlighter outlives event_listener but keeps a
reference to it. Arguably, the event listener will be reset to point to
another instance before the next invocation of highlight, but I think it
would be cleaner to make sure that the event listener is cleared anyway:
auto clear_event_listener = make_scope_exit ([highlighter]()
{
highlighter->setHighlightEventListener (nullptr);
});
Who knows, maybe one day highlighter's dtor might want to notify
something.
WDYT?
Best,
Lancelot.
> +
> 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);
> + }
> #if __cplusplus >= 202002L
> contents = std::move (output).str();
> #else
> @@ -308,13 +363,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
>
On 10/13/23 19:00, Lancelot SIX wrote:
> Hi Tom,
>
> I have a comment below.
>
> On Fri, Oct 13, 2023 at 02:19:53PM +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.
>>
>> 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 | 62 ++++++++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 60 insertions(+), 2 deletions(-)
>>
>> diff --git a/gdb/source-cache.c b/gdb/source-cache.c
>> index f8c98d7e23f..57d9f3b8ab4 100644
>> --- a/gdb/source-cache.c
>> +++ b/gdb/source-cache.c
>> @@ -223,9 +266,21 @@ try_source_highlight (std::string &contents ATTRIBUTE_UNUSED,
>> highlighter->setStyleFile ("esc.style");
>> }
>>
>> + gdb_highlight_event_listener event_listener (fullname);
>> + highlighter->setHighlightEventListener (&event_listener);
>
> In this instance, highlighter outlives event_listener but keeps a
> reference to it. Arguably, the event listener will be reset to point to
> another instance before the next invocation of highlight, but I think it
> would be cleaner to make sure that the event listener is cleared anyway:
>
> auto clear_event_listener = make_scope_exit ([highlighter]()
> {
> highlighter->setHighlightEventListener (nullptr);
> });
>
> Who knows, maybe one day highlighter's dtor might want to notify
> something.
>
> WDYT?
>
That's a good idea, thanks, I've added it to the patch.
The only change I had to make was to drop the highlighter capture, I got
some error because of that:
...
/data/vries/gdb/src/gdb/source-cache.c: In function ‘bool
try_source_highlight(std::__cxx11::string&, language, const string&)’:
/data/vries/gdb/src/gdb/source-cache.c:273:53: error: capture of
variable ‘highlighter’ with non-automatic storage duration [-Werror]
...
I'll post a v3 after retesting.
Thanks,
- Tom
> Best,
> Lancelot.
>
>> +
>> 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);
>> + }
>> #if __cplusplus >= 202002L
>> contents = std::move (output).str();
>> #else
>> @@ -308,13 +363,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
>>
@@ -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
+ {
+ /* 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,21 @@ try_source_highlight (std::string &contents ATTRIBUTE_UNUSED,
highlighter->setStyleFile ("esc.style");
}
+ gdb_highlight_event_listener event_listener (fullname);
+ highlighter->setHighlightEventListener (&event_listener);
+
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);
+ }
#if __cplusplus >= 202002L
contents = std::move (output).str();
#else
@@ -308,13 +363,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);
}
}