* [PATCH v2 1/4] [gdb/cli] Skip string copy in source_cache::ensure
2023-10-13 12:19 [PATCH v2 0/4] [gdb/cli] Allow source highlighting to be interrupted Tom de Vries
@ 2023-10-13 12:19 ` Tom de Vries
2023-10-13 16:34 ` Lancelot SIX
2023-10-13 12:19 ` [PATCH v2 2/4] [gdb/cli] Factor out try_source_highlight Tom de Vries
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Tom de Vries @ 2023-10-13 12:19 UTC (permalink / raw)
To: gdb-patches
In function source_cache::ensure we have:
...
std::ostringstream output;
...
contents = output.str();
...
The last line causes an unnecessary string copy.
C++20 allows us to skip it, like this:
...
contents = std::move (output).str();
...
Use the more efficient version if compiling with -std=c++20.
Tested on x86_64-linux.
---
gdb/source-cache.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/gdb/source-cache.c b/gdb/source-cache.c
index 77b357cb42b..7ef54411c69 100644
--- a/gdb/source-cache.c
+++ b/gdb/source-cache.c
@@ -252,7 +252,11 @@ source_cache::ensure (struct symtab *s)
std::istringstream input (contents);
std::ostringstream output;
highlighter->highlight (input, output, lang_name, fullname);
+#if __cplusplus >= 202002L
+ contents = std::move (output).str();
+#else
contents = output.str ();
+#endif
already_styled = true;
}
catch (...)
--
2.35.3
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2 1/4] [gdb/cli] Skip string copy in source_cache::ensure
2023-10-13 12:19 ` [PATCH v2 1/4] [gdb/cli] Skip string copy in source_cache::ensure Tom de Vries
@ 2023-10-13 16:34 ` Lancelot SIX
2023-10-16 7:40 ` Tom de Vries
0 siblings, 1 reply; 12+ messages in thread
From: Lancelot SIX @ 2023-10-13 16:34 UTC (permalink / raw)
To: Tom de Vries; +Cc: gdb-patches
Hi Tom,
On Fri, Oct 13, 2023 at 02:19:50PM +0200, Tom de Vries wrote:
> In function source_cache::ensure we have:
> ...
> std::ostringstream output;
> ...
> contents = output.str();
> ...
> The last line causes an unnecessary string copy.
>
> C++20 allows us to skip it, like this:
> ...
> contents = std::move (output).str();
> ...
Looking at cppreference[1], it suggests that this new C++ overload for
str should be equivalent to
content = std::move (*output.rbuf ()).str ();
Should we use this construct instead regardless of C++'s version and
avoid the #ifdef? That being said, I find the C++20 version easier to
read, so it's up to you.
Best,
Lancelot.
[1] https://en.cppreference.com/w/cpp/io/basic_ostringstream/str
>
> Use the more efficient version if compiling with -std=c++20.
>
> Tested on x86_64-linux.
> ---
> gdb/source-cache.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/gdb/source-cache.c b/gdb/source-cache.c
> index 77b357cb42b..7ef54411c69 100644
> --- a/gdb/source-cache.c
> +++ b/gdb/source-cache.c
> @@ -252,7 +252,11 @@ source_cache::ensure (struct symtab *s)
> std::istringstream input (contents);
> std::ostringstream output;
> highlighter->highlight (input, output, lang_name, fullname);
> +#if __cplusplus >= 202002L
> + contents = std::move (output).str();
> +#else
> contents = output.str ();
> +#endif
> already_styled = true;
> }
> catch (...)
> --
> 2.35.3
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/4] [gdb/cli] Skip string copy in source_cache::ensure
2023-10-13 16:34 ` Lancelot SIX
@ 2023-10-16 7:40 ` Tom de Vries
0 siblings, 0 replies; 12+ messages in thread
From: Tom de Vries @ 2023-10-16 7:40 UTC (permalink / raw)
To: Lancelot SIX; +Cc: gdb-patches
On 10/13/23 18:34, Lancelot SIX wrote:
> Hi Tom,
>
> On Fri, Oct 13, 2023 at 02:19:50PM +0200, Tom de Vries wrote:
>> In function source_cache::ensure we have:
>> ...
>> std::ostringstream output;
>> ...
>> contents = output.str();
>> ...
>> The last line causes an unnecessary string copy.
>>
>> C++20 allows us to skip it, like this:
>> ...
>> contents = std::move (output).str();
>> ...
>
> Looking at cppreference[1], it suggests that this new C++ overload for
> str should be equivalent to
>
> content = std::move (*output.rbuf ()).str ();
>
> Should we use this construct instead regardless of C++'s version and
> avoid the #ifdef? That being said, I find the C++20 version easier to
> read, so it's up to you.
>
I've played around with this a bit, and found out that I can also use
"contents = std::move (output).str();" in c++11. So I've just dropped
the ifdef.
Thanks,
- Tom
> Best,
> Lancelot.
>
> [1] https://en.cppreference.com/w/cpp/io/basic_ostringstream/str
>
>>
>> Use the more efficient version if compiling with -std=c++20.
>>
>> Tested on x86_64-linux.
>> ---
>> gdb/source-cache.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/gdb/source-cache.c b/gdb/source-cache.c
>> index 77b357cb42b..7ef54411c69 100644
>> --- a/gdb/source-cache.c
>> +++ b/gdb/source-cache.c
>> @@ -252,7 +252,11 @@ source_cache::ensure (struct symtab *s)
>> std::istringstream input (contents);
>> std::ostringstream output;
>> highlighter->highlight (input, output, lang_name, fullname);
>> +#if __cplusplus >= 202002L
>> + contents = std::move (output).str();
>> +#else
>> contents = output.str ();
>> +#endif
>> already_styled = true;
>> }
>> catch (...)
>> --
>> 2.35.3
>>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 2/4] [gdb/cli] Factor out try_source_highlight
2023-10-13 12:19 [PATCH v2 0/4] [gdb/cli] Allow source highlighting to be interrupted Tom de Vries
2023-10-13 12:19 ` [PATCH v2 1/4] [gdb/cli] Skip string copy in source_cache::ensure Tom de Vries
@ 2023-10-13 12:19 ` Tom de Vries
2023-10-13 16:52 ` Lancelot SIX
2023-10-13 12:19 ` [PATCH v2 3/4] [gdb/cli] Keep track of styling failures in source_cache Tom de Vries
2023-10-13 12:19 ` [PATCH v2 4/4] [gdb/cli] Allow source highlighting to be interrupted Tom de Vries
3 siblings, 1 reply; 12+ messages in thread
From: Tom de Vries @ 2023-10-13 12:19 UTC (permalink / raw)
To: gdb-patches
Function source_cache::ensure contains some code using the GNU
source-highlight library.
The code is a sizable chunk of the function, and contains conditional
compilation in a slightly convoluted way:
...
if (!already_styled)
#endif /* HAVE_SOURCE_HIGHLIGHT */
{
...
Fix this by factoring out the code into new function try_source_highlight,
such that:
- source_cache::ensure is easier to read, and
- the conditional compilation is at the level of the function body.
Tested on x86_64-linux.
---
gdb/source-cache.c | 99 +++++++++++++++++++++++++++-------------------
1 file changed, 59 insertions(+), 40 deletions(-)
diff --git a/gdb/source-cache.c b/gdb/source-cache.c
index 7ef54411c69..9757721e932 100644
--- a/gdb/source-cache.c
+++ b/gdb/source-cache.c
@@ -191,6 +191,63 @@ get_language_name (enum language lang)
#endif /* HAVE_SOURCE_HIGHLIGHT */
+/* Try to highlight CONTENTS from file FULLNAME in language LANG using
+ the GNU source-higlight library. Return true if highlighting
+ succeeded. */
+
+static bool
+try_source_highlight (std::string &contents ATTRIBUTE_UNUSED,
+ enum language lang ATTRIBUTE_UNUSED,
+ const std::string &fullname ATTRIBUTE_UNUSED)
+{
+#ifdef HAVE_SOURCE_HIGHLIGHT
+ if (!use_gnu_source_highlight)
+ return false;
+
+ const char *lang_name = get_language_name (lang);
+ if (lang_name == nullptr)
+ return false;
+
+ /* The global source highlight object, or null if one was
+ never constructed. This is stored here rather than in
+ the class so that we don't need to include anything or do
+ conditional compilation in source-cache.h. */
+ static srchilite::SourceHighlight *highlighter;
+
+ bool styled = false;
+ try
+ {
+ if (highlighter == nullptr)
+ {
+ highlighter = new srchilite::SourceHighlight ("esc.outlang");
+ highlighter->setStyleFile ("esc.style");
+ }
+
+ std::istringstream input (contents);
+ std::ostringstream output;
+ highlighter->highlight (input, output, lang_name, fullname);
+#if __cplusplus >= 202002L
+ contents = std::move (output).str();
+#else
+ contents = output.str ();
+#endif
+ styled = true;
+ }
+ catch (...)
+ {
+ /* Source Highlight will throw an exception if
+ highlighting fails. One possible reason it can fail
+ is if the language is unknown -- which matters to gdb
+ because Rust support wasn't added until after 3.1.8.
+ Ignore exceptions here. */
+ }
+
+ return styled;
+#else
+ return false;
+#endif /* HAVE_SOURCE_HIGHLIGHT */
+}
+
/* See source-cache.h. */
bool
@@ -230,48 +287,10 @@ source_cache::ensure (struct symtab *s)
if (source_styling && gdb_stdout->can_emit_style_escape ())
{
-#ifdef HAVE_SOURCE_HIGHLIGHT
- bool already_styled = false;
- const char *lang_name = get_language_name (s->language ());
- if (lang_name != nullptr && use_gnu_source_highlight)
- {
- /* The global source highlight object, or null if one was
- never constructed. This is stored here rather than in
- the class so that we don't need to include anything or do
- conditional compilation in source-cache.h. */
- static srchilite::SourceHighlight *highlighter;
-
- try
- {
- if (highlighter == nullptr)
- {
- highlighter = new srchilite::SourceHighlight ("esc.outlang");
- highlighter->setStyleFile ("esc.style");
- }
-
- std::istringstream input (contents);
- std::ostringstream output;
- highlighter->highlight (input, output, lang_name, fullname);
-#if __cplusplus >= 202002L
- contents = std::move (output).str();
-#else
- contents = output.str ();
-#endif
- already_styled = true;
- }
- catch (...)
- {
- /* Source Highlight will throw an exception if
- highlighting fails. One possible reason it can fail
- is if the language is unknown -- which matters to gdb
- because Rust support wasn't added until after 3.1.8.
- Ignore exceptions here and fall back to
- un-highlighted text. */
- }
- }
+ bool already_styled
+ = try_source_highlight (contents, s->language (), fullname);
if (!already_styled)
-#endif /* HAVE_SOURCE_HIGHLIGHT */
{
gdb::optional<std::string> ext_contents;
ext_contents = ext_lang_colorize (fullname, contents);
--
2.35.3
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2 2/4] [gdb/cli] Factor out try_source_highlight
2023-10-13 12:19 ` [PATCH v2 2/4] [gdb/cli] Factor out try_source_highlight Tom de Vries
@ 2023-10-13 16:52 ` Lancelot SIX
2023-10-16 7:07 ` Tom de Vries
0 siblings, 1 reply; 12+ messages in thread
From: Lancelot SIX @ 2023-10-13 16:52 UTC (permalink / raw)
To: Tom de Vries; +Cc: gdb-patches
Hi Tom
On Fri, Oct 13, 2023 at 02:19:51PM +0200, Tom de Vries wrote:
> Function source_cache::ensure contains some code using the GNU
> source-highlight library.
>
> The code is a sizable chunk of the function, and contains conditional
> compilation in a slightly convoluted way:
> ...
> if (!already_styled)
> #endif /* HAVE_SOURCE_HIGHLIGHT */
> {
> ...
>
> Fix this by factoring out the code into new function try_source_highlight,
> such that:
> - source_cache::ensure is easier to read, and
> - the conditional compilation is at the level of the function body.
>
> Tested on x86_64-linux.
> ---
> gdb/source-cache.c | 99 +++++++++++++++++++++++++++-------------------
> 1 file changed, 59 insertions(+), 40 deletions(-)
>
> diff --git a/gdb/source-cache.c b/gdb/source-cache.c
> index 7ef54411c69..9757721e932 100644
> --- a/gdb/source-cache.c
> +++ b/gdb/source-cache.c
> @@ -191,6 +191,63 @@ get_language_name (enum language lang)
>
> #endif /* HAVE_SOURCE_HIGHLIGHT */
>
> +/* Try to highlight CONTENTS from file FULLNAME in language LANG using
> + the GNU source-higlight library. Return true if highlighting
> + succeeded. */
> +
> +static bool
> +try_source_highlight (std::string &contents ATTRIBUTE_UNUSED,
> + enum language lang ATTRIBUTE_UNUSED,
> + const std::string &fullname ATTRIBUTE_UNUSED)
> +{
> +#ifdef HAVE_SOURCE_HIGHLIGHT
> + if (!use_gnu_source_highlight)
> + return false;
> +
> + const char *lang_name = get_language_name (lang);
> + if (lang_name == nullptr)
> + return false;
> +
> + /* The global source highlight object, or null if one was
> + never constructed. This is stored here rather than in
> + the class so that we don't need to include anything or do
> + conditional compilation in source-cache.h. */
> + static srchilite::SourceHighlight *highlighter;
Even if this is pre-existing code, I think I would be tempted to get the
initialization of HIGHLIGHTER outside of the TRY block (new can throw,
we might not want to hide it).
Using an immediately invoked lambda does the trick and works well with
static variables:
static srchilite::SourceHighlight *highlighter
= []()
{
srchilite::SourceHighlight * h = new srchilite::SourceHighlight ("esc.outlang");
h->setStyleFile ("esc.style");
return h;
} ();
Best,
Lancelot.
> +
> + bool styled = false;
> + try
> + {
> + if (highlighter == nullptr)
> + {
> + highlighter = new srchilite::SourceHighlight ("esc.outlang");
> + highlighter->setStyleFile ("esc.style");
> + }
> +
> + std::istringstream input (contents);
> + std::ostringstream output;
> + highlighter->highlight (input, output, lang_name, fullname);
> +#if __cplusplus >= 202002L
> + contents = std::move (output).str();
> +#else
> + contents = output.str ();
> +#endif
> + styled = true;
> + }
> + catch (...)
> + {
> + /* Source Highlight will throw an exception if
> + highlighting fails. One possible reason it can fail
> + is if the language is unknown -- which matters to gdb
> + because Rust support wasn't added until after 3.1.8.
> + Ignore exceptions here. */
> + }
> +
> + return styled;
> +#else
> + return false;
> +#endif /* HAVE_SOURCE_HIGHLIGHT */
> +}
> +
> /* See source-cache.h. */
>
> bool
> @@ -230,48 +287,10 @@ source_cache::ensure (struct symtab *s)
>
> if (source_styling && gdb_stdout->can_emit_style_escape ())
> {
> -#ifdef HAVE_SOURCE_HIGHLIGHT
> - bool already_styled = false;
> - const char *lang_name = get_language_name (s->language ());
> - if (lang_name != nullptr && use_gnu_source_highlight)
> - {
> - /* The global source highlight object, or null if one was
> - never constructed. This is stored here rather than in
> - the class so that we don't need to include anything or do
> - conditional compilation in source-cache.h. */
> - static srchilite::SourceHighlight *highlighter;
> -
> - try
> - {
> - if (highlighter == nullptr)
> - {
> - highlighter = new srchilite::SourceHighlight ("esc.outlang");
> - highlighter->setStyleFile ("esc.style");
> - }
> -
> - std::istringstream input (contents);
> - std::ostringstream output;
> - highlighter->highlight (input, output, lang_name, fullname);
> -#if __cplusplus >= 202002L
> - contents = std::move (output).str();
> -#else
> - contents = output.str ();
> -#endif
> - already_styled = true;
> - }
> - catch (...)
> - {
> - /* Source Highlight will throw an exception if
> - highlighting fails. One possible reason it can fail
> - is if the language is unknown -- which matters to gdb
> - because Rust support wasn't added until after 3.1.8.
> - Ignore exceptions here and fall back to
> - un-highlighted text. */
> - }
> - }
> + bool already_styled
> + = try_source_highlight (contents, s->language (), fullname);
>
> if (!already_styled)
> -#endif /* HAVE_SOURCE_HIGHLIGHT */
> {
> gdb::optional<std::string> ext_contents;
> ext_contents = ext_lang_colorize (fullname, contents);
> --
> 2.35.3
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2 2/4] [gdb/cli] Factor out try_source_highlight
2023-10-13 16:52 ` Lancelot SIX
@ 2023-10-16 7:07 ` Tom de Vries
2023-10-16 8:25 ` Lancelot SIX
0 siblings, 1 reply; 12+ messages in thread
From: Tom de Vries @ 2023-10-16 7:07 UTC (permalink / raw)
To: Lancelot SIX; +Cc: gdb-patches
On 10/13/23 18:52, Lancelot SIX wrote:
> Hi Tom
>
> On Fri, Oct 13, 2023 at 02:19:51PM +0200, Tom de Vries wrote:
>> Function source_cache::ensure contains some code using the GNU
>> source-highlight library.
>>
>> The code is a sizable chunk of the function, and contains conditional
>> compilation in a slightly convoluted way:
>> ...
>> if (!already_styled)
>> #endif /* HAVE_SOURCE_HIGHLIGHT */
>> {
>> ...
>>
>> Fix this by factoring out the code into new function try_source_highlight,
>> such that:
>> - source_cache::ensure is easier to read, and
>> - the conditional compilation is at the level of the function body.
>>
>> Tested on x86_64-linux.
>> ---
>> gdb/source-cache.c | 99 +++++++++++++++++++++++++++-------------------
>> 1 file changed, 59 insertions(+), 40 deletions(-)
>>
>> diff --git a/gdb/source-cache.c b/gdb/source-cache.c
>> index 7ef54411c69..9757721e932 100644
>> --- a/gdb/source-cache.c
>> +++ b/gdb/source-cache.c
>> @@ -191,6 +191,63 @@ get_language_name (enum language lang)
>>
>> #endif /* HAVE_SOURCE_HIGHLIGHT */
>>
>> +/* Try to highlight CONTENTS from file FULLNAME in language LANG using
>> + the GNU source-higlight library. Return true if highlighting
>> + succeeded. */
>> +
>> +static bool
>> +try_source_highlight (std::string &contents ATTRIBUTE_UNUSED,
>> + enum language lang ATTRIBUTE_UNUSED,
>> + const std::string &fullname ATTRIBUTE_UNUSED)
>> +{
>> +#ifdef HAVE_SOURCE_HIGHLIGHT
>> + if (!use_gnu_source_highlight)
>> + return false;
>> +
>> + const char *lang_name = get_language_name (lang);
>> + if (lang_name == nullptr)
>> + return false;
>> +
>> + /* The global source highlight object, or null if one was
>> + never constructed. This is stored here rather than in
>> + the class so that we don't need to include anything or do
>> + conditional compilation in source-cache.h. */
>> + static srchilite::SourceHighlight *highlighter;
>
> Even if this is pre-existing code, I think I would be tempted to get the
> initialization of HIGHLIGHTER outside of the TRY block (new can throw,
> we might not want to hide it).
>
Hi,
The pre-existing code is a result of commit f64e2f40454 ("[gdb] Catch
exception when constructing the highlighter"), which does the opposite
of what you propose here.
And I think the rationale given there still holds.
I suppose if we're interested in not hiding that new throws, you could
catch the exceptions of interest and handle them appropriately, by say
issuing a warning or rethrowing.
I will leave this as is for now.
Thanks,
- Tom
> Using an immediately invoked lambda does the trick and works well with
> static variables:
>
> static srchilite::SourceHighlight *highlighter
> = []()
> {
> srchilite::SourceHighlight * h = new srchilite::SourceHighlight ("esc.outlang");
> h->setStyleFile ("esc.style");
> return h;
> } ();
>
> Best,
> Lancelot.
>
>> +
>> + bool styled = false;
>> + try
>> + {
>> + if (highlighter == nullptr)
>> + {
>> + highlighter = new srchilite::SourceHighlight ("esc.outlang");
>> + highlighter->setStyleFile ("esc.style");
>> + }
>> +
>> + std::istringstream input (contents);
>> + std::ostringstream output;
>> + highlighter->highlight (input, output, lang_name, fullname);
>> +#if __cplusplus >= 202002L
>> + contents = std::move (output).str();
>> +#else
>> + contents = output.str ();
>> +#endif
>> + styled = true;
>> + }
>> + catch (...)
>> + {
>> + /* Source Highlight will throw an exception if
>> + highlighting fails. One possible reason it can fail
>> + is if the language is unknown -- which matters to gdb
>> + because Rust support wasn't added until after 3.1.8.
>> + Ignore exceptions here. */
>> + }
>> +
>> + return styled;
>> +#else
>> + return false;
>> +#endif /* HAVE_SOURCE_HIGHLIGHT */
>> +}
>> +
>> /* See source-cache.h. */
>>
>> bool
>> @@ -230,48 +287,10 @@ source_cache::ensure (struct symtab *s)
>>
>> if (source_styling && gdb_stdout->can_emit_style_escape ())
>> {
>> -#ifdef HAVE_SOURCE_HIGHLIGHT
>> - bool already_styled = false;
>> - const char *lang_name = get_language_name (s->language ());
>> - if (lang_name != nullptr && use_gnu_source_highlight)
>> - {
>> - /* The global source highlight object, or null if one was
>> - never constructed. This is stored here rather than in
>> - the class so that we don't need to include anything or do
>> - conditional compilation in source-cache.h. */
>> - static srchilite::SourceHighlight *highlighter;
>> -
>> - try
>> - {
>> - if (highlighter == nullptr)
>> - {
>> - highlighter = new srchilite::SourceHighlight ("esc.outlang");
>> - highlighter->setStyleFile ("esc.style");
>> - }
>> -
>> - std::istringstream input (contents);
>> - std::ostringstream output;
>> - highlighter->highlight (input, output, lang_name, fullname);
>> -#if __cplusplus >= 202002L
>> - contents = std::move (output).str();
>> -#else
>> - contents = output.str ();
>> -#endif
>> - already_styled = true;
>> - }
>> - catch (...)
>> - {
>> - /* Source Highlight will throw an exception if
>> - highlighting fails. One possible reason it can fail
>> - is if the language is unknown -- which matters to gdb
>> - because Rust support wasn't added until after 3.1.8.
>> - Ignore exceptions here and fall back to
>> - un-highlighted text. */
>> - }
>> - }
>> + bool already_styled
>> + = try_source_highlight (contents, s->language (), fullname);
>>
>> if (!already_styled)
>> -#endif /* HAVE_SOURCE_HIGHLIGHT */
>> {
>> gdb::optional<std::string> ext_contents;
>> ext_contents = ext_lang_colorize (fullname, contents);
>> --
>> 2.35.3
>>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2 2/4] [gdb/cli] Factor out try_source_highlight
2023-10-16 7:07 ` Tom de Vries
@ 2023-10-16 8:25 ` Lancelot SIX
0 siblings, 0 replies; 12+ messages in thread
From: Lancelot SIX @ 2023-10-16 8:25 UTC (permalink / raw)
To: Tom de Vries; +Cc: gdb-patches
>
> Hi,
>
> The pre-existing code is a result of commit f64e2f40454 ("[gdb] Catch
> exception when constructing the highlighter"), which does the opposite of
> what you propose here.
>
> And I think the rationale given there still holds.
>
Hi Tom,
Thanks for pointing this out, I missed this commit.
Best,
Lancelot.
> I suppose if we're interested in not hiding that new throws, you could catch
> the exceptions of interest and handle them appropriately, by say issuing a
> warning or rethrowing.
>
> I will leave this as is for now.
>
> Thanks,
> - Tom
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 3/4] [gdb/cli] Keep track of styling failures in source_cache
2023-10-13 12:19 [PATCH v2 0/4] [gdb/cli] Allow source highlighting to be interrupted Tom de Vries
2023-10-13 12:19 ` [PATCH v2 1/4] [gdb/cli] Skip string copy in source_cache::ensure Tom de Vries
2023-10-13 12:19 ` [PATCH v2 2/4] [gdb/cli] Factor out try_source_highlight Tom de Vries
@ 2023-10-13 12:19 ` Tom de Vries
2023-10-13 12:19 ` [PATCH v2 4/4] [gdb/cli] Allow source highlighting to be interrupted Tom de Vries
3 siblings, 0 replies; 12+ messages in thread
From: Tom de Vries @ 2023-10-13 12:19 UTC (permalink / raw)
To: gdb-patches
In source_cache::ensure, keep track of which files failed to be styled, and
don't attempt to style them again in case the file dropped out of the cache.
Tested on x86_64-linux.
---
gdb/source-cache.c | 24 ++++++++++++++++++++++--
gdb/source-cache.h | 4 ++++
2 files changed, 26 insertions(+), 2 deletions(-)
diff --git a/gdb/source-cache.c b/gdb/source-cache.c
index 9757721e932..f8c98d7e23f 100644
--- a/gdb/source-cache.c
+++ b/gdb/source-cache.c
@@ -285,7 +285,8 @@ source_cache::ensure (struct symtab *s)
return false;
}
- if (source_styling && gdb_stdout->can_emit_style_escape ())
+ if (source_styling && gdb_stdout->can_emit_style_escape ()
+ && m_no_styling_files.count (fullname) == 0)
{
bool already_styled
= try_source_highlight (contents, s->language (), fullname);
@@ -295,7 +296,26 @@ source_cache::ensure (struct symtab *s)
gdb::optional<std::string> ext_contents;
ext_contents = ext_lang_colorize (fullname, contents);
if (ext_contents.has_value ())
- contents = std::move (*ext_contents);
+ {
+ contents = std::move (*ext_contents);
+ already_styled = true;
+ }
+ }
+
+ if (!already_styled)
+ {
+ /* Styling failed. Styling can fail for instance for these
+ reasons:
+ - the language is not supported.
+ - the language cannot not be auto-detected from the file name.
+ - 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.insert (fullname);
}
}
diff --git a/gdb/source-cache.h b/gdb/source-cache.h
index bec6598ff9f..f1d30b43ce0 100644
--- a/gdb/source-cache.h
+++ b/gdb/source-cache.h
@@ -66,6 +66,7 @@ class source_cache
{
m_source_map.clear ();
m_offset_cache.clear ();
+ m_no_styling_files.clear ();
}
private:
@@ -95,6 +96,9 @@ class source_cache
/* The file offset cache. The key is the full name of the source
file. */
std::unordered_map<std::string, std::vector<off_t>> m_offset_cache;
+
+ /* The list of files where styling failed. */
+ std::unordered_set<std::string> m_no_styling_files;
};
/* The global source cache. */
--
2.35.3
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH v2 4/4] [gdb/cli] Allow source highlighting to be interrupted
2023-10-13 12:19 [PATCH v2 0/4] [gdb/cli] Allow source highlighting to be interrupted Tom de Vries
` (2 preceding siblings ...)
2023-10-13 12:19 ` [PATCH v2 3/4] [gdb/cli] Keep track of styling failures in source_cache Tom de Vries
@ 2023-10-13 12:19 ` Tom de Vries
2023-10-13 17:00 ` Lancelot SIX
3 siblings, 1 reply; 12+ messages in thread
From: Tom de Vries @ 2023-10-13 12:19 UTC (permalink / raw)
To: gdb-patches
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
@@ -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);
}
}
--
2.35.3
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2 4/4] [gdb/cli] Allow source highlighting to be interrupted
2023-10-13 12:19 ` [PATCH v2 4/4] [gdb/cli] Allow source highlighting to be interrupted Tom de Vries
@ 2023-10-13 17:00 ` Lancelot SIX
2023-10-16 7:47 ` Tom de Vries
0 siblings, 1 reply; 12+ messages in thread
From: Lancelot SIX @ 2023-10-13 17:00 UTC (permalink / raw)
To: Tom de Vries; +Cc: gdb-patches
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2 4/4] [gdb/cli] Allow source highlighting to be interrupted
2023-10-13 17:00 ` Lancelot SIX
@ 2023-10-16 7:47 ` Tom de Vries
0 siblings, 0 replies; 12+ messages in thread
From: Tom de Vries @ 2023-10-16 7:47 UTC (permalink / raw)
To: Lancelot SIX; +Cc: gdb-patches
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
>>
^ permalink raw reply [flat|nested] 12+ messages in thread