Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[lldb] Fixed lldb-server crash (TestLogHandler was not thread safe) #101326

Merged
merged 1 commit into from
Jul 31, 2024

Conversation

slydiman
Copy link
Contributor

Host::LaunchProcess() requires to SetMonitorProcessCallback. This callback is called from the child process monitor thread. We cannot control this thread anyway. lldb-server may crash if there is a logging around this callback because TestLogHandler is not thread safe. I faced this issue debugging 100 simultaneous child processes. Note StreamLogHandler::Emit() in lldb/source/Utility/Log.cpp already contains the similar mutex.

Host::LaunchProcess() requires to SetMonitorProcessCallback. This callback is called from the child process monitor thread. We cannot control this thread anyway. lldb-server may crash if there is a logging around this callback because TestLogHandler is not thread safe. I faced this issue debugging 100 simultaneous child processes. Note StreamLogHandler::Emit() in lldb/source/Utility/Log.cpp already contains the similar mutex.
@llvmbot
Copy link
Member

llvmbot commented Jul 31, 2024

@llvm/pr-subscribers-lldb

Author: Dmitry Vasilyev (slydiman)

Changes

Host::LaunchProcess() requires to SetMonitorProcessCallback. This callback is called from the child process monitor thread. We cannot control this thread anyway. lldb-server may crash if there is a logging around this callback because TestLogHandler is not thread safe. I faced this issue debugging 100 simultaneous child processes. Note StreamLogHandler::Emit() in lldb/source/Utility/Log.cpp already contains the similar mutex.


Full diff: https://github.com/llvm/llvm-project/pull/101326.diff

1 Files Affected:

  • (modified) lldb/tools/lldb-server/LLDBServerUtilities.cpp (+2)
diff --git a/lldb/tools/lldb-server/LLDBServerUtilities.cpp b/lldb/tools/lldb-server/LLDBServerUtilities.cpp
index c3a8df19e969e..5facfbf3105e9 100644
--- a/lldb/tools/lldb-server/LLDBServerUtilities.cpp
+++ b/lldb/tools/lldb-server/LLDBServerUtilities.cpp
@@ -27,11 +27,13 @@ class TestLogHandler : public LogHandler {
       : m_stream_sp(stream_sp) {}
 
   void Emit(llvm::StringRef message) override {
+    std::lock_guard<std::mutex> guard(m_mutex);
     (*m_stream_sp) << message;
     m_stream_sp->flush();
   }
 
 private:
+  std::mutex m_mutex;
   std::shared_ptr<raw_ostream> m_stream_sp;
 };
 

slydiman added a commit to slydiman/llvm-project that referenced this pull request Jul 31, 2024
…t on Windows

`lldb-server platform --server` works on Windows now w/o multithreading. The rest functionality remains unchanged.

Added also PipeWindows::WriteWithTimeout(), fixed PipeWindows::ReadWithTimeout() and missing initialization of  m_read_overlapped.hEvent in the constructor PipeWindows(lldb::pipe_t read, lldb::pipe_t write).

Fixes llvm#90923, fixes llvm#56346.

Depends on llvm#101326.

This is the part 1 of the replacement of llvm#100670.

In the part 2 I plan to switch `lldb-server gdbserver` to use `--fd` and listen a common gdb port for all gdbserver connections. Then we can remove gdb port mapping to fix llvm#97537.
Copy link
Collaborator

@labath labath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense, though it would probably be even better to just use the real StreamLogHandler class. I think all that's needed would be to add a new constructor to it.

@slydiman slydiman merged commit 93fecc2 into llvm:main Jul 31, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants