diff mbox series

[v2,3/3,gdb/dap] Fix race between dap exit and gdb exit

Message ID 20240221132023.15147-3-tdevries@suse.de
State Committed
Headers
Series [v2,1/3,gdb/dap] Factor out thread_log |

Checks

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

Commit Message

Tom de Vries Feb. 21, 2024, 1:20 p.m. UTC 
  When DAP shuts down due to an EOF event, there's a race between:
- gdb's main thread handling a SIGHUP, and
- the DAP main thread exiting.

Fix this by waiting for DAP's main thread exit during the gdb_exiting event.

Tested on aarch64-linux.

PR dap/31380
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31380
---
 gdb/python/lib/gdb/dap/startup.py | 20 +++++++++++++++++++-
 gdb/testsuite/gdb.dap/eof.exp     |  9 +++++++++
 2 files changed, 28 insertions(+), 1 deletion(-)

Comments

Tom Tromey Feb. 21, 2024, 9:10 p.m. UTC | #1 
>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> When DAP shuts down due to an EOF event, there's a race between:
Tom> - gdb's main thread handling a SIGHUP, and
Tom> - the DAP main thread exiting.

Tom> Fix this by waiting for DAP's main thread exit during the gdb_exiting event.

Tom> Tested on aarch64-linux.

Thanks, this is ok.
Approved-By: Tom Tromey <tom@tromey.com>

Tom> -    start_thread("DAP", really_start_dap)
Tom> +    # Note that we set _dap_thread in both the DAP and the GDB main thread.
Tom> +    global _dap_thread
Tom> +    _dap_thread = start_thread("DAP", really_start_dap)

Tom> +@in_gdb_thread
Tom> +def _on_gdb_exiting(event):
Tom> +    if _dap_thread is None:
Tom> +        # This can happen if the DAP module is imported, but the server not
Tom> +        # started.
Tom> +        thread_log("no dap thread to join")
Tom> +        return
Tom> +
Tom> +    thread_log("joining dap thread ...")
Tom> +    _dap_thread.join()
Tom> +    thread_log("joining dap thread done")
Tom> +
Tom> +
Tom> +gdb.events.gdb_exiting.connect(_on_gdb_exiting)

Originally I'd just pictured something like:

  local = start_thread("DAP", ...)
  gdb.events.gdb_exiting.connect(local.join)

... but wanting logging makes sense I suppose.
The global could still be removed if you care by a nested function.

Tom
Tom de Vries Feb. 22, 2024, 10:39 a.m. UTC | #2 
On 2/21/24 22:10, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> When DAP shuts down due to an EOF event, there's a race between:
> Tom> - gdb's main thread handling a SIGHUP, and
> Tom> - the DAP main thread exiting.
> 
> Tom> Fix this by waiting for DAP's main thread exit during the gdb_exiting event.
> 
> Tom> Tested on aarch64-linux.
> 
> Thanks, this is ok.
> Approved-By: Tom Tromey <tom@tromey.com>
> 

Thanks for the review.

> Tom> -    start_thread("DAP", really_start_dap)
> Tom> +    # Note that we set _dap_thread in both the DAP and the GDB main thread.
> Tom> +    global _dap_thread
> Tom> +    _dap_thread = start_thread("DAP", really_start_dap)
> 
> Tom> +@in_gdb_thread
> Tom> +def _on_gdb_exiting(event):
> Tom> +    if _dap_thread is None:
> Tom> +        # This can happen if the DAP module is imported, but the server not
> Tom> +        # started.
> Tom> +        thread_log("no dap thread to join")
> Tom> +        return
> Tom> +
> Tom> +    thread_log("joining dap thread ...")
> Tom> +    _dap_thread.join()
> Tom> +    thread_log("joining dap thread done")
> Tom> +
> Tom> +
> Tom> +gdb.events.gdb_exiting.connect(_on_gdb_exiting)
> 
> Originally I'd just pictured something like:
> 
>    local = start_thread("DAP", ...)
>    gdb.events.gdb_exiting.connect(local.join)
> 

Aha, I see.

> ... but wanting logging makes sense I suppose.
> The global could still be removed if you care by a nested function.

Done.

I've also make changed the log messages to use DAP upper-case, to be in 
sync with other uses in the log file.

Committed as attached.

Thanks,
- Tom
diff mbox series

Patch

diff --git a/gdb/python/lib/gdb/dap/startup.py b/gdb/python/lib/gdb/dap/startup.py
index 29fe78ecd53..38a2e295e07 100644
--- a/gdb/python/lib/gdb/dap/startup.py
+++ b/gdb/python/lib/gdb/dap/startup.py
@@ -93,7 +93,9 @@  def start_dap(target):
         _dap_thread = threading.current_thread()
         target()
 
-    start_thread("DAP", really_start_dap)
+    # Note that we set _dap_thread in both the DAP and the GDB main thread.
+    global _dap_thread
+    _dap_thread = start_thread("DAP", really_start_dap)
 
 
 def in_gdb_thread(func):
@@ -208,6 +210,22 @@  def exec_and_log(cmd):
         log_stack()
 
 
+@in_gdb_thread
+def _on_gdb_exiting(event):
+    if _dap_thread is None:
+        # This can happen if the DAP module is imported, but the server not
+        # started.
+        thread_log("no dap thread to join")
+        return
+
+    thread_log("joining dap thread ...")
+    _dap_thread.join()
+    thread_log("joining dap thread done")
+
+
+gdb.events.gdb_exiting.connect(_on_gdb_exiting)
+
+
 class Invoker(object):
     """A simple class that can invoke a gdb command."""
 
diff --git a/gdb/testsuite/gdb.dap/eof.exp b/gdb/testsuite/gdb.dap/eof.exp
index a84b1d21e04..05048f2762a 100644
--- a/gdb/testsuite/gdb.dap/eof.exp
+++ b/gdb/testsuite/gdb.dap/eof.exp
@@ -38,3 +38,12 @@  dap_check_log_file
 
 # Check that first log message is present.
 dap_check_log_file_re [string_to_regexp "starting DAP server"]
+
+# There should be one "READ:" for the initialize request, and at least one
+# "WROTE:" for the initialize response.
+dap_check_log_file_re "READ:"
+dap_check_log_file_re "WROTE:"
+
+# Check that all thread termination messages are there.
+dap_check_log_file_re "JSON writer: terminating"
+dap_check_log_file_re "DAP: terminating"