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

[Tom de Vries <tdevries@suse.de>]

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