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

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

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