diff mbox series

[7/7] Handle SIGTERM in run_events

Message ID 20240827140021.32660-7-tdevries@suse.de
State Committed
Headers
Series [1/7] Handle ^C in gnu_source_highlight_test |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Test passed

Commit Message

Tom de Vries Aug. 27, 2024, 2 p.m. UTC 
  While reviewing "catch (...)" uses I came across:
...
  for (auto &item : local)
    {
      try
	{
	  item ();
	}
      catch (...)
	{
	  /* Ignore exceptions in the callback.  */
	}
    }
...

This means that when an item throws a gdb_exception_forced_quit,
the exception is ignored and following items are executed.

Fix this by handling gdb_exception_forced_quit explicity, and immediately
rethrowing it.

I wondered about ^C, and couldn't decide whether current behaviour is ok, so
I left this alone, but I made the issue explicit in the source code.

As for the "catch (...)", I think that it should let a non-gdb_exception
propagate, so I've narrowed it to "catch (const gdb_exception &)".

My rationale for this is as follows.

There seem to be a few ways that "catch (...)" is allowed in gdb:
- clean-up and rethrow (basically the SCOPE_EXIT pattern)
- catch and handle an exception from a call into an external c++ library

Since we're dealing with neither of those here, we remove the "catch (...)".

Tested on aarch64-linux.
---
 gdb/run-on-main-thread.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

Comments

Tom Tromey Sept. 23, 2024, 6 p.m. UTC | #1 
>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> +      catch (const gdb_exception_quit &e)
Tom> +	{
Tom> +	  /* Should cancelation of a runnable event cancel the execution of
Tom> +	     the following one?  The answer is not clear, so keep doing what
Tom> +	     we've done sofar: ignore this exception.  */

Typo, "so far".

Maybe this should work differently in some cases, I'm not sure; but I
think right now DAP depends on QUIT working in these runnables.

Approved-By: Tom Tromey <tom@tromey.com>

Tom
Tom de Vries Sept. 24, 2024, 2:46 p.m. UTC | #2 
On 9/23/24 20:00, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> +      catch (const gdb_exception_quit &e)
> Tom> +	{
> Tom> +	  /* Should cancelation of a runnable event cancel the execution of
> Tom> +	     the following one?  The answer is not clear, so keep doing what
> Tom> +	     we've done sofar: ignore this exception.  */
> 
> Typo, "so far".
> 
> Maybe this should work differently in some cases, I'm not sure; but I
> think right now DAP depends on QUIT working in these runnables.
> 

FWIW, I've done a test run with:
...
diff --git a/gdb/run-on-main-thread.c b/gdb/run-on-main-thread.c
index a6213d4d85e..e6864600cbd 100644
--- a/gdb/run-on-main-thread.c
+++ b/gdb/run-on-main-thread.c
@@ -86,6 +86,7 @@ run_events (int error, gdb_client_data client_data)
  	  /* Should cancelation of a runnable event cancel the execution of
  	     the following one?  The answer is not clear, so keep doing what
  	     we've done sofar: ignore this exception.  */
+	  gdb_assert (false);
  	}
        catch (const gdb_exception &)
  	{
...
and it didn't trigger once.

> Approved-By: Tom Tromey <tom@tromey.com>

Thanks for the review, pushed.

- Tom
Tom de Vries Sept. 25, 2024, 1:07 p.m. UTC | #3 
On 9/24/24 16:46, Tom de Vries wrote:
> On 9/23/24 20:00, Tom Tromey wrote:
>>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
>>
>> Tom> +      catch (const gdb_exception_quit &e)
>> Tom> +    {
>> Tom> +      /* Should cancelation of a runnable event cancel the 
>> execution of
>> Tom> +         the following one?  The answer is not clear, so keep 
>> doing what
>> Tom> +         we've done sofar: ignore this exception.  */
>>
>> Typo, "so far".
>>
>> Maybe this should work differently in some cases, I'm not sure; but I
>> think right now DAP depends on QUIT working in these runnables.
>>
> 
> FWIW, I've done a test run with:
> ...
> diff --git a/gdb/run-on-main-thread.c b/gdb/run-on-main-thread.c
> index a6213d4d85e..e6864600cbd 100644
> --- a/gdb/run-on-main-thread.c
> +++ b/gdb/run-on-main-thread.c
> @@ -86,6 +86,7 @@ run_events (int error, gdb_client_data client_data)
>         /* Should cancelation of a runnable event cancel the execution of
>            the following one?  The answer is not clear, so keep doing what
>            we've done sofar: ignore this exception.  */
> +      gdb_assert (false);
>       }
>         catch (const gdb_exception &)
>       {
> ...
> and it didn't trigger once.
> 
>> Approved-By: Tom Tromey <tom@tromey.com>
> 
> Thanks for the review, pushed.

And, I forgot to fix the typo you mentioned.

There's one more spot that needs the same fix:
...
gdb/symtab.c:	  /* Cust is best found sofar, save it.  */
...

I've submitted a series that adds a spell checker based on a list I 
found at wikipedia ( 
https://sourceware.org/pipermail/gdb-patches/2024-September/211928.html ).

That list doesn't contains "sofar->so far", but we could extend the 
functionality of the script by adding 
gdb/contrib/common-misspellings.txt and using that in addition.

Thanks,
- Tom
diff mbox series

Patch

diff --git a/gdb/run-on-main-thread.c b/gdb/run-on-main-thread.c
index e30dabaff03..a6213d4d85e 100644
--- a/gdb/run-on-main-thread.c
+++ b/gdb/run-on-main-thread.c
@@ -74,7 +74,20 @@  run_events (int error, gdb_client_data client_data)
 	{
 	  item ();
 	}
-      catch (...)
+      catch (const gdb_exception_forced_quit &e)
+	{
+	  /* GDB is terminating, so:
+	     - make sure this is propagated, and
+	     - no need to keep running things, so propagate immediately.  */
+	  throw;
+	}
+      catch (const gdb_exception_quit &e)
+	{
+	  /* Should cancelation of a runnable event cancel the execution of
+	     the following one?  The answer is not clear, so keep doing what
+	     we've done sofar: ignore this exception.  */
+	}
+      catch (const gdb_exception &)
 	{
 	  /* Ignore exceptions in the callback.  */
 	}