diff mbox series

[4/8,gdb/dap] Make dap log printing thread-safe

Message ID 20240219082341.21313-4-tdevries@suse.de
State Committed
Headers
Series [1/8,gdb/testsuite] Set up dap log file in gdb.dap/type_checker.exp |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 fail Patch failed to apply
linaro-tcwg-bot/tcwg_gdb_build--master-arm fail Patch failed to apply

Commit Message

Tom de Vries Feb. 19, 2024, 8:23 a.m. UTC 
  I read that printing from different python threads is thread-unsafe, and I
noticed that the dap log printing is used from different threads, but doesn't
take care to make the printing thread-safe.

Fix this by using a lock to access LoggingParam.log_file.

Tested on aarch64-linux.
---
 gdb/python/lib/gdb/dap/startup.py | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

Comments

Tom Tromey Feb. 20, 2024, 3:32 p.m. UTC | #1 
>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> I read that printing from different python threads is thread-unsafe, and I
Tom> noticed that the dap log printing is used from different threads, but doesn't
Tom> take care to make the printing thread-safe.

Tom> Fix this by using a lock to access LoggingParam.log_file.

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


I wonder if we should just switch to the Python logging module.

Tom> -    log_file = None
Tom> +    lock = threading.Lock()
Tom> +    with lock:
Tom> +        log_file = None

I can't imagine locking is needed here.

Tom
Tom de Vries Feb. 21, 2024, 1:31 p.m. UTC | #2 
On 2/20/24 16:32, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> I read that printing from different python threads is thread-unsafe, and I
> Tom> noticed that the dap log printing is used from different threads, but doesn't
> Tom> take care to make the printing thread-safe.
> 
> Tom> Fix this by using a lock to access LoggingParam.log_file.
> 
> Ok.
> Approved-By: Tom Tromey <tom@tromey.com>
> 
> 
> I wonder if we should just switch to the Python logging module.
> 
> Tom> -    log_file = None
> Tom> +    lock = threading.Lock()
> Tom> +    with lock:
> Tom> +        log_file = None
> 
> I can't imagine locking is needed here.

Dropped that bit, and committed.

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 edffc00902b..fc86442db42 100644
--- a/gdb/python/lib/gdb/dap/startup.py
+++ b/gdb/python/lib/gdb/dap/startup.py
@@ -145,7 +145,9 @@  class LoggingParam(gdb.Parameter):
     set_doc = "Set the DAP logging status."
     show_doc = "Show the DAP logging status."
 
-    log_file = None
+    lock = threading.Lock()
+    with lock:
+        log_file = None
 
     def __init__(self):
         super().__init__(
@@ -154,12 +156,13 @@  class LoggingParam(gdb.Parameter):
         self.value = None
 
     def get_set_string(self):
-        # Close any existing log file, no matter what.
-        if self.log_file is not None:
-            self.log_file.close()
-            self.log_file = None
-        if self.value is not None:
-            self.log_file = open(self.value, "w")
+        with dap_log.lock:
+            # Close any existing log file, no matter what.
+            if self.log_file is not None:
+                self.log_file.close()
+                self.log_file = None
+            if self.value is not None:
+                self.log_file = open(self.value, "w")
         return ""
 
 
@@ -168,9 +171,10 @@  dap_log = LoggingParam()
 
 def log(something, level=LogLevel.DEFAULT):
     """Log SOMETHING to the log file, if logging is enabled."""
-    if dap_log.log_file is not None and level <= _log_level.value:
-        print(something, file=dap_log.log_file)
-        dap_log.log_file.flush()
+    with dap_log.lock:
+        if dap_log.log_file is not None and level <= _log_level.value:
+            print(something, file=dap_log.log_file)
+            dap_log.log_file.flush()
 
 
 def thread_log(something, level=LogLevel.DEFAULT):
@@ -183,9 +187,10 @@  def thread_log(something, level=LogLevel.DEFAULT):
 
 def log_stack(level=LogLevel.DEFAULT):
     """Log a stack trace to the log file, if logging is enabled."""
-    if dap_log.log_file is not None and level <= _log_level.value:
-        traceback.print_exc(file=dap_log.log_file)
-        dap_log.log_file.flush()
+    with dap_log.lock:
+        if dap_log.log_file is not None and level <= _log_level.value:
+            traceback.print_exc(file=dap_log.log_file)
+            dap_log.log_file.flush()
 
 
 @in_gdb_thread