[lldb] Optimized lldb-server memory usage #100666
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MAX_PATH is definitely larger than 6 bytes we are expecting for this message, and could be rather large depending on the target OS (4K for some Linux OSs). Since the buffer gets allocated on the stack we better be conservative and allocate what we actually need.
|
@llvm/pr-subscribers-lldb Author: Dmitry Vasilyev (slydiman) ChangesMAX_PATH is definitely larger than 6 bytes we are expecting for this message, and could be rather large depending on the target OS (4K for some Linux OSs). Since the buffer gets allocated on the stack we better be conservative and allocate what we actually need. Full diff: https://github.com/llvm/llvm-project/pull/100666.diff 1 Files Affected:
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
index 187370eb36cae..b961c0e42469a 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
@@ -1145,7 +1145,8 @@ Status GDBRemoteCommunication::StartDebugserverProcess(
if (socket_pipe.CanWrite())
socket_pipe.CloseWriteFileDescriptor();
if (socket_pipe.CanRead()) {
- char port_cstr[PATH_MAX] = {0};
+ // The port number may be "1024\0".."65535\0".
+ char port_cstr[6] = {0};
port_cstr[0] = '\0';
size_t num_bytes = sizeof(port_cstr);
// Read port from pipe with 10 second timeout.
|
slydiman
added a commit
to slydiman/llvm-project
that referenced
this pull request
Jul 25, 2024
…t mapping Removed fork(). Used threads and the common thread-safe port map for all platform connections. Updated lldb::FileSystem to use llvm::vfs::createPhysicalFileSystem() with an own virtual working directory per thread. This patch depends on llvm#100659, llvm#100666. This patch fixes llvm#97537, llvm#90923, llvm#56346. lldb-server has been tested on Windows with 50 connections and 100 processes launched simultaneously. Tested also the cross build with Linux x86_64 host and Linux Aarch64 target.
bulbazord
reviewed
Jul 26, 2024
| @@ -1145,7 +1145,8 @@ Status GDBRemoteCommunication::StartDebugserverProcess( | |||
| if (socket_pipe.CanWrite()) | |||
| socket_pipe.CloseWriteFileDescriptor(); | |||
| if (socket_pipe.CanRead()) { | |||
| char port_cstr[PATH_MAX] = {0}; | |||
| // The port number may be "1024\0".."65535\0". | |||
| char port_cstr[6] = {0}; | |||
| port_cstr[0] = '\0'; | |||
There was a problem hiding this comment.
Do we still need this line? Seems redundant since the array is initialized to this value anyway.
|
Hm, I was worried that we might get the filepath of a unix socket in some use case, but we llvm::to_integer(port_cstr) shortly later, so that can't be the case. |
labath
approved these changes
Jul 26, 2024
| @@ -1145,8 +1145,8 @@ Status GDBRemoteCommunication::StartDebugserverProcess( | |||
| if (socket_pipe.CanWrite()) | |||
| socket_pipe.CloseWriteFileDescriptor(); | |||
| if (socket_pipe.CanRead()) { | |||
| char port_cstr[PATH_MAX] = {0}; | |||
| port_cstr[0] = '\0'; | |||
| // The port number may be "1024\0".."65535\0". | |||
There was a problem hiding this comment.
Technically, if you're running as root, it could also be one of the smaller values. I'd just document the largest value.
slydiman
added a commit
to slydiman/llvm-project
that referenced
this pull request
Aug 1, 2024
…t mapping Removed fork(). Used threads and the common thread-safe port map for all platform connections. Updated lldb::FileSystem to use llvm::vfs::createPhysicalFileSystem() with an own virtual working directory per thread. This patch depends on llvm#100659, llvm#100666. This patch fixes llvm#97537, llvm#90923, llvm#56346. lldb-server has been tested on Windows with 50 connections and 100 processes launched simultaneously. Tested also the cross build with Linux x86_64 host and Linux Aarch64 target.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
MAX_PATH is definitely larger than 6 bytes we are expecting for this message, and could be rather large depending on the target OS (4K for some Linux OSs).
Since the buffer gets allocated on the stack we better be conservative and allocate what we actually need.