[v3,4/4,gdb/cli] Allow source highlighting to be interrupted

  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(-)
  

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.

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
>
  

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
>>
  

Hi,

On 2023-10-16 10:17, Tom de Vries wrote:

>  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)

A ctor with one parameter should usually be "explicit".

> +    : 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 ());

How sure are we that we won't ever print source listings while the inferior
has the terminal (either both for I/O, or just for input with is_ours_for_output)?

I was trying to think of scenarios, but not much came out.  I'm imagining maybe a
Python script hooking into events, internal breakpoints, or timers, or some such and
printing sources while the inferior is running?  OTOH, that printing might well come out
badly formatted as the Python layer doesn't expose target_terminal::{ours,ours_for_output}()
to Python.  Maybe we don't need to worry about it.  At least we have the assertion.

If we ever do manage to get here with "!target_terminal::ours ()" , then we'd want to
be sure that a Ctrl-C that happens to be pressed exactly while GDB is printing
the sources isn't immediately interpreted as a request to cancel source highlight,
because the source highlight may well end up being quickly, and the user was just
unlucky enough that Ctrl-C was pressed in that short window.  In that scenario, it
is better to assume that the source highlight will finish quickly, and that the
Ctrl-C is meant to interrupt the inferior.  The remote target handles this by
only asking the user the second time the user presses
Ctrl-C -- see remote.c:remote_target::remote_serial_quit_handler().
But we can worry about this if we ever come to it, assuming it isn't really expected
to end up printing sources while the inferior is running.  It's just unfortunate that
if such case does exits, reproducing it won't be super easy, as it'll be timing
dependent.

Assuming we don't want to bother with that for now, the approach in the patch seems
fine to me.  A few nits/comments below.

> +
> +    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 ());

I don't think it's usual for secondary prompts / queries to end in newline, and then
the "(y or n)" appearing on the next line?  I think most (all?) other queries / secondary
prompts leave the "(y or n)" on the same line as the question text?

Also, I think we should skip the query if sync_quit_force_run is set, so that
if we got a SIGTERM we abort quickly and end up in the top level exiting GDB ASAP.
To make that work properly we'll need to tweak try_source_highlight to not swallow
gdb_exception_forced_quit, though, it currently swallows _everything_.


> +    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;

It doesn't hurt to do it this way, but the QUIT idea looked more appealing when it seemed
like you could get rid of the check_quit_flag() call too, in v1.  With the query in the middle,
the end result is just that default_quit_handler ends up in "quit ();", which you could call
directly here:

    /* Interrupt highlighting.  */
    quit ();

But OTOH, the default_quit_handler approach is more future proof, especially considering
SIGTERM handling in maybe_quit and quit, which would move elsewhere someday.  So yeah, hmm,
better do it the default_quit_handler/QUIT way after all.

> +  }
> +
> +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);
> +      });

clear_event_listener is never used, so this could instead be a simpler SCOPE_EXIT:

      SCOPE_EXIT { highlighter->setHighlightEventListener (nullptr); };
  

On 10/17/23 02:20, Pedro Alves wrote:
> Hi,
> 
> On 2023-10-16 10:17, Tom de Vries wrote:
> 
>>   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)
> 
> A ctor with one parameter should usually be "explicit".
> 

Hi,

thanks for the review.

Done.

>> +    : 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 ());
> 
> How sure are we that we won't ever print source listings while the inferior
> has the terminal (either both for I/O, or just for input with is_ours_for_output)?
> 

I'm not too sure.

> I was trying to think of scenarios, but not much came out.  I'm imagining maybe a
> Python script hooking into events, internal breakpoints, or timers, or some such and
> printing sources while the inferior is running?  OTOH, that printing might well come out
> badly formatted as the Python layer doesn't expose target_terminal::{ours,ours_for_output}()
> to Python.  Maybe we don't need to worry about it.  At least we have the assertion.
> 

I've gone back and forth a bit about this, I don't particularly like the 
assert, but I like the alternatives less I guess.  I've thought about 
adding a way to enable the assert in development builds (to detect it if 
the situation happens) and disable it in releases (to give end users a 
stable debugger).  But I've left it as is for now.

> If we ever do manage to get here with "!target_terminal::ours ()" , then we'd want to
> be sure that a Ctrl-C that happens to be pressed exactly while GDB is printing
> the sources isn't immediately interpreted as a request to cancel source highlight,
> because the source highlight may well end up being quickly, and the user was just
> unlucky enough that Ctrl-C was pressed in that short window.  In that scenario, it
> is better to assume that the source highlight will finish quickly, and that the
> Ctrl-C is meant to interrupt the inferior.  The remote target handles this by
> only asking the user the second time the user presses
> Ctrl-C -- see remote.c:remote_target::remote_serial_quit_handler().
> But we can worry about this if we ever come to it, assuming it isn't really expected
> to end up printing sources while the inferior is running.  It's just unfortunate that
> if such case does exits, reproducing it won't be super easy, as it'll be timing
> dependent.
> 
> Assuming we don't want to bother with that for now, the approach in the patch seems
> fine to me.  A few nits/comments below.
> 
>> +
>> +    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 ());
> 
> I don't think it's usual for secondary prompts / queries to end in newline, and then
> the "(y or n)" appearing on the next line?  I think most (all?) other queries / secondary
> prompts leave the "(y or n)" on the same line as the question text?
> 

Done.

> Also, I think we should skip the query if sync_quit_force_run is set, so that
> if we got a SIGTERM we abort quickly and end up in the top level exiting GDB ASAP.
> To make that work properly we'll need to tweak try_source_highlight to not swallow
> gdb_exception_forced_quit, though, it currently swallows _everything_.
> 
> 

Thanks, good point, I overlooked that.   I've restructured the patch 
into a v4 patch series, and added handling of SIGTERM.

>> +    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;
> 
> It doesn't hurt to do it this way, but the QUIT idea looked more appealing when it seemed
> like you could get rid of the check_quit_flag() call too, in v1.  With the query in the middle,
> the end result is just that default_quit_handler ends up in "quit ();", which you could call
> directly here:
> 
>      /* Interrupt highlighting.  */
>      quit ();
> 
> But OTOH, the default_quit_handler approach is more future proof, especially considering
> SIGTERM handling in maybe_quit and quit, which would move elsewhere someday.  So yeah, hmm,
> better do it the default_quit_handler/QUIT way after all.
> 

OK :)

>> +  }
>> +
>> +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);
>> +      });
> 
> clear_event_listener is never used, so this could instead be a simpler SCOPE_EXIT:
> 
>        SCOPE_EXIT { highlighter->setHighlightEventListener (nullptr); };
> 

Done.

I've posted the v4 here ( 
https://sourceware.org/pipermail/gdb-patches/2023-October/203391.html ).

Thanks,
- Tom