Skip to content

[lldb] Multithreading lldb-server works on Windows now; fixed gdb port mapping #100670

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

[lldb] Multithreading lldb-server works on Windows now; fixed gdb port mapping #100670

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7fb5b2e
[lldb] Multithreading lldb-server works on Windows now; fixed gdb por…
slydiman Jul 24, 2024
17916b6
Merged with #100659
slydiman Jul 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions lldb/include/lldb/Host/FileSystem.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,12 @@ class FileSystem {

static FileSystem &Instance();

static void InitializePerThread() {
lldbassert(!InstancePerThread() && "Already initialized.");
InstancePerThread().emplace(llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>(
llvm::vfs::createPhysicalFileSystem().release()));
}

template <class... T> static void Initialize(T &&...t) {
lldbassert(!InstanceImpl() && "Already initialized.");
InstanceImpl().emplace(std::forward<T>(t)...);
Expand Down Expand Up @@ -206,6 +212,7 @@ class FileSystem {

private:
static std::optional<FileSystem> &InstanceImpl();
static std::optional<FileSystem> &InstancePerThread();
llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> m_fs;
std::unique_ptr<TildeExpressionResolver> m_tilde_resolver;
std::string m_home_directory;
Expand Down
8 changes: 8 additions & 0 deletions lldb/source/Host/common/FileSystem.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,15 @@ void FileSystem::Terminate() {
InstanceImpl().reset();
}

std::optional<FileSystem> &FileSystem::InstancePerThread() {
static thread_local std::optional<FileSystem> t_fs;
return t_fs;
}

std::optional<FileSystem> &FileSystem::InstanceImpl() {
std::optional<FileSystem> &fs = InstancePerThread();
if (fs)
return fs;
static std::optional<FileSystem> g_fs;
return g_fs;
}
Expand Down
12 changes: 12 additions & 0 deletions lldb/source/Host/posix/PipePosix.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,18 @@ Status PipePosix::ReadWithTimeout(void *buf, size_t size,
bytes_read += result;
if (bytes_read == size || result == 0)
break;

// This is the workaround for the following bug in Linux multithreading
// select() https://bugzilla.kernel.org/show_bug.cgi?id=546
// ReadWithTimeout() with a non-zero timeout is used only to
// read the port number from the gdbserver pipe
// in GDBRemoteCommunication::StartDebugserverProcess().
// The port number may be "1024\0".."65535\0".
if (timeout.count() > 0 && size == 6 && bytes_read == 5 &&
static_cast<char *>(buf)[4] == '\0') {
break;
}
Comment on lines +328 to +337
Copy link
Collaborator

Choose a reason for hiding this comment

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

It sounds like the consensus on that bug is that this is the application's (i.e. our) fault. We should fix the issue instead.

Copy link
Contributor Author

@slydiman slydiman Jul 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think so. Note select() works fine with the pipe closed on other side in the single thread. select() hangs when called simultaneously from multiple threads. I tried multiple simultaneous connections to lldb-server platform to launch lldb-server gdbserver. It worked 50/50 in case of 2 connections and 100% failed in case of 3+ connections. Instead of using select() I tried

  • use poll()
  • use read(size = 0)
  • use non blocked pipe and call read() w/o select() or poll()
  • change pipe buffer size
    Nothing helped. It is the bug in the kernel. read() will hang too if the pipe is closed on the other side. Non blocking read() will return EAGAIN instead of 0. The system just does not recognize the closed pipe in case of multithreading.
    So, I don't see a way to fix this on Linux. The only way is a workaround.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm sorry, but I find this extremely hard to believe. Pipes and the ability to read them until EOF are as old as UNIX. We're not doing anything special here. It's going to take a lot more than a reference to a 20 year old REJECTED INVALID bug to convince me this is a kernel issue.

Also note that the situation mentioned in that bug is different from what (I think) you're describing here. In their case, the pipe is NOT closed on the other side. The pipe is closed on the side that's doing the selecting:

Consider a multithreaded program (pthreads). One thread reads from
a file descriptor (typically a TCP or UDP socket). At some time,
another thread decides to close(2) the file descriptor while the
first thread is blocked, waiting for input data.

In other words, this is your basic race in the application code, and it's the applications (ours) responsibility to fix it. While I'm not a kernel maintainer, I think I have a pretty good idea why they said the application is buggy, and why they didn't want to fix it -- it's because fixing it probably will not make the application correct.

The problem there is that the application has calls (select (or friends) and close) on two threads with no synchronization between them. Now if select happens to run first, then it's not completely unreasonable to expect that close will terminate that select, and the kernel could in theory make sure it does that (apparently, some operating systems do just that). The problem is what happens if select does not run first. What if close does ? In this case, select will return an error (as the bug reporter expects), but only if the FD hasn't been reused in the mean time. And since linux always assigns the lowest FD available (I think POSIX mandates that), it's very likely that the very next operation (perhaps on a third thread) which creates an fd will get the same FD as we've just closed. If that happens, then the select will NOT return an error (the kernel has no way to know that it's referring to the old fd) and will happily start listening on the new fd. Since you usually aren't able to control all operations that could possibly create a new FD, this kind of pattern would be buggy except in extremely limited circumstances.


} else if (errno == EINTR) {
continue;
} else {
Expand Down
10 changes: 8 additions & 2 deletions lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ struct ForkLaunchInfo {
execve(info.argv[0], const_cast<char *const *>(info.argv), info.envp);

#if defined(__linux__)
if (errno == ETXTBSY) {
for (int i = 0; i < 50; ++i) {
// On android M and earlier we can get this error because the adb daemon
// can hold a write handle on the executable even after it has finished
// uploading it. This state lasts only a short time and happens only when
Expand All @@ -210,7 +210,13 @@ struct ForkLaunchInfo {
// shell" command in the fork() child before it has had a chance to exec.)
// Since this state should clear up quickly, wait a while and then give it
// one more go.
usleep(50000);
// If `lldb-server platform` copies the executable in one thread and
// launches gdbserver in another thread (fork+execve), the FD may stay
// opened in the forked child process until execve() even if the first
// thread closed the file. Let's wait a while.
if (errno != ETXTBSY)
break;
usleep(100000);
execve(info.argv[0], const_cast<char *const *>(info.argv), info.envp);
}
#endif
Expand Down
26 changes: 21 additions & 5 deletions lldb/source/Host/windows/ProcessLauncherWindows.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,14 +113,30 @@ ProcessLauncherWindows::LaunchProcess(const ProcessLaunchInfo &launch_info,
// command line is not empty, its contents may be modified by CreateProcessW.
WCHAR *pwcommandLine = wcommandLine.empty() ? nullptr : &wcommandLine[0];

BOOL result = ::CreateProcessW(
wexecutable.c_str(), pwcommandLine, NULL, NULL, TRUE, flags, env_block,
wworkingDirectory.size() == 0 ? NULL : wworkingDirectory.c_str(),
&startupinfo, &pi);
BOOL result;
DWORD last_error = 0;
// This is the workaround for the error "The process cannot access the file
// because it is being used by another process". Note the executable file is
// installed to the target by the process `lldb-server platform`, but launched
// by the process `lldb-server gdbserver`. Sometimes system may block the file
// for some time after copying.
for (int i = 0; i < 50; ++i) {
result = ::CreateProcessW(
wexecutable.c_str(), pwcommandLine, NULL, NULL, TRUE, flags, env_block,
wworkingDirectory.size() == 0 ? NULL : wworkingDirectory.c_str(),
&startupinfo, &pi);
if (!result) {
last_error = ::GetLastError();
if (last_error != ERROR_SHARING_VIOLATION)
break;
::Sleep(100);
} else
break;
}

if (!result) {
// Call GetLastError before we make any other system calls.
error.SetError(::GetLastError(), eErrorTypeWin32);
error.SetError(last_error, eErrorTypeWin32);
// Note that error 50 ("The request is not supported") will occur if you
// try debug a 64-bit inferior from a 32-bit LLDB.
}
Expand Down
15 changes: 13 additions & 2 deletions lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -646,7 +646,9 @@ GDBRemoteCommunicationServerCommon::Handle_vFile_Size(
packet.GetHexByteString(path);
if (!path.empty()) {
uint64_t Size;
if (llvm::sys::fs::file_size(path, Size))
FileSpec file_spec(path);
FileSystem::Instance().Resolve(file_spec);
if (llvm::sys::fs::file_size(file_spec.GetPath(), Size))
return SendErrorResponse(5);
StreamString response;
response.PutChar('F');
Expand Down Expand Up @@ -725,7 +727,9 @@ GDBRemoteCommunicationServerCommon::Handle_vFile_unlink(
packet.SetFilePos(::strlen("vFile:unlink:"));
std::string path;
packet.GetHexByteString(path);
Status error(llvm::sys::fs::remove(path));
FileSpec file_spec(path);
FileSystem::Instance().Resolve(file_spec);
Status error(llvm::sys::fs::remove(file_spec.GetPath()));
StreamString response;
response.Printf("F%x,%x", error.GetError(), error.GetError());
return SendPacketNoLock(response.GetString());
Expand All @@ -744,6 +748,13 @@ GDBRemoteCommunicationServerCommon::Handle_qPlatform_shell(
// uint32_t timeout = packet.GetHexMaxU32(false, 32);
if (packet.GetChar() == ',')
packet.GetHexByteString(working_dir);
else {
auto cwd = FileSystem::Instance()
.GetVirtualFileSystem()
->getCurrentWorkingDirectory();
if (cwd)
working_dir = *cwd;
}
int status, signo;
std::string output;
FileSpec working_spec(working_dir);
Expand Down
Loading
Loading