diff mbox series

[6/7] Handle ^C in ~scoped_remote_fd

Message ID 20240827140021.32660-6-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:
...
	try
	  {
	    fileio_error remote_errno;
	    m_remote->remote_hostio_close (m_fd, &remote_errno);
	  }
	catch (...)
	  {
	    /* Swallow exception before it escapes the dtor.  If
	       something goes wrong, likely the connection is gone,
	       and there's nothing else that can be done.  */
	  }
...

This also swallows gdb_exception_quit and gdb_exception_forced_quit.  I don't
know whether these can actually happen here, but if not it's better to
accommodate for the possibility anyway.

Fix this by handling gdb_exception explicitly, and setting the quit flag if
necessary.

It could be that "catch (...)" should be replaced by
"catch (const gdb_exception &)" but that depends on what kind of exception
remote_hostio_close is expected to throw, and I don't know that, so I'm
leaving it as is.

Tested on aarch64-linux.
---
 gdb/remote.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

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

Tom> This also swallows gdb_exception_quit and gdb_exception_forced_quit.  I don't
Tom> know whether these can actually happen here, but if not it's better to
Tom> accommodate for the possibility anyway.

Yeah, it's generally hard to know without digging pretty depp.

Tom> Fix this by handling gdb_exception explicitly, and setting the quit flag if
Tom> necessary.

Tom> It could be that "catch (...)" should be replaced by
Tom> "catch (const gdb_exception &)" but that depends on what kind of exception
Tom> remote_hostio_close is expected to throw, and I don't know that, so I'm
Tom> leaving it as is.

I think this is an improvement but while reading it I was left wondering
if it does the right thing for a forced quit.

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

Tom
Tom de Vries Sept. 24, 2024, 1:19 p.m. UTC | #2 
On 9/23/24 19:53, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> This also swallows gdb_exception_quit and gdb_exception_forced_quit.  I don't
> Tom> know whether these can actually happen here, but if not it's better to
> Tom> accommodate for the possibility anyway.
> 
> Yeah, it's generally hard to know without digging pretty depp.
> 
> Tom> Fix this by handling gdb_exception explicitly, and setting the quit flag if
> Tom> necessary.
> 
> Tom> It could be that "catch (...)" should be replaced by
> Tom> "catch (const gdb_exception &)" but that depends on what kind of exception
> Tom> remote_hostio_close is expected to throw, and I don't know that, so I'm
> Tom> leaving it as is.
> 
> I think this is an improvement but while reading it I was left wondering
> if it does the right thing for a forced quit.
> 

Good point.  I grepped for set_forced_quit_flag and found 
~scoped_switch_fork_info, which has an identical problem, so I've 
updated the patch by just copying the solution from there:
...
         catch (const gdb_exception_quit &ex)
           {
             /* We can't throw from a destructor, so re-set the quit 
flag 

               for later QUIT checking.  */
             set_quit_flag ();
           }
         catch (const gdb_exception_forced_quit &ex)
           {
             /* Like above, but (eventually) cause GDB to terminate by 
 

                setting sync_quit_force_run.  */
             set_force_quit_flag ();
           }
...

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

Thank for the review, pushed with that change.

Thanks,
- Tom
diff mbox series

Patch

diff --git a/gdb/remote.c b/gdb/remote.c
index 2c3988cb507..7cf26321b98 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -13237,6 +13237,16 @@  class scoped_remote_fd
 	    fileio_error remote_errno;
 	    m_remote->remote_hostio_close (m_fd, &remote_errno);
 	  }
+	catch (const gdb_exception &e)
+	  {
+	    if (e.reason != RETURN_ERROR)
+	      {
+		/* Try to propagate gdb_exception_quit and
+		   gdb_exception_forced_quit.  We can't rethrow, so do the
+		   next best thing.  */
+		set_quit_flag ();
+	      }
+	  }
 	catch (...)
 	  {
 	    /* Swallow exception before it escapes the dtor.  If