[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
https://github.com/Awfa created https://github.com/llvm/llvm-project/pull/88812
This PR adds a check within `PutFile` to exit early when both local and
destination files have matching MD5 hashes. If they differ, or there is trouble
getting the hashes, the regular code path to put the file is run.
As I needed this to talk to an `lldb-server` which runs the gdb-remote
protocol, I enabled `CalculateMD5` within `Platform/gdb-server` and also found
and fixed a parsing bug within it as well. Before this PR, the client is
incorrectly parsing the response packet containing the checksum; after this PR,
hopefully this is fixed. There is a test for the parsing behavior included in
this PR.
>From 095696411172034f80233f1722e293c1f458d67f Mon Sep 17 00:00:00 2001
From: Anthony Ha
Date: Mon, 15 Apr 2024 19:34:19 +
Subject: [PATCH] [lldb] Skip remote PutFile when MD5 hashes equal
---
.../gdb-server/PlatformRemoteGDBServer.cpp| 9 +
.../gdb-server/PlatformRemoteGDBServer.h | 3 ++
.../GDBRemoteCommunicationClient.cpp | 36 +--
lldb/source/Target/Platform.cpp | 30 +++-
.../GDBRemoteCommunicationClientTest.cpp | 23
5 files changed, 98 insertions(+), 3 deletions(-)
diff --git
a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
index 88f1ad15b6b485..d9f3e40019cf29 100644
--- a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
+++ b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
@@ -684,6 +684,15 @@ Status PlatformRemoteGDBServer::RunShellCommand(
signo_ptr, command_output, timeout);
}
+bool PlatformRemoteGDBServer::CalculateMD5(const FileSpec &file_spec,
+ uint64_t &low, uint64_t &high) {
+ if (IsConnected()) {
+// Note that high/low are switched in the gdb remote communication client
+return m_gdb_client_up->CalculateMD5(file_spec, high, low);
+ }
+ return false;
+}
+
void PlatformRemoteGDBServer::CalculateTrapHandlerSymbolNames() {
m_trap_handlers.push_back(ConstString("_sigtramp"));
}
diff --git a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
index 638f7db5ef800c..d83fc386f59427 100644
--- a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
+++ b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
@@ -146,6 +146,9 @@ class PlatformRemoteGDBServer : public Platform, private
UserIDResolver {
void CalculateTrapHandlerSymbolNames() override;
+ bool CalculateMD5(const FileSpec &file_spec, uint64_t &low,
+uint64_t &high) override;
+
const lldb::UnixSignalsSP &GetRemoteUnixSignals() override;
size_t ConnectToWaitingProcesses(lldb_private::Debugger &debugger,
diff --git
a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
index 1f6116967a4f64..8c79d5fecf12fe 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -3433,8 +3433,40 @@ bool GDBRemoteCommunicationClient::CalculateMD5(
return false;
if (response.Peek() && *response.Peek() == 'x')
return false;
-low = response.GetHexMaxU64(false, UINT64_MAX);
-high = response.GetHexMaxU64(false, UINT64_MAX);
+
+// GDBRemoteCommunicationServerCommon::Handle_vFile_MD5 concatenates low
and
+// high hex strings. We can't use response.GetHexMaxU64 because that can't
+// handle the concatenated hex string. What would happen is parsing the low
+// would consume the whole response packet - which is a bug. Instead, we
get
+// the byte string for each low and high hex separately, and parse them.
+//
+// An alternate way to handle this is to change the server to put a
+// delimiter between the low/high parts, and change the client to parse the
+// delimiter. However, we choose not to do this so existing lldb-servers
+// don't have to be patched
+
+// Get low part
+auto part = response.GetStringRef().substr(response.GetFilePos(),
+ sizeof(uint64_t) * 2);
+if (part.size() != sizeof(uint64_t) * 2)
+ return false;
+response.SetFilePos(response.GetFilePos() + part.size());
+
+bool conversionErrored = part.getAsInteger(16, low);
+if (conversionErrored)
+ return false;
+
+// Get high part
+part = response.GetStringRef().substr(response.GetFilePos(),
+ sizeof(uint64_t) * 2);
+if (part.size() != sizeof(uint64_t) * 2)
+ return false;
+response.SetFilePos(response.GetFilePos() + part.size());
+
+conversionError
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
@@ -3433,8 +3433,40 @@ bool GDBRemoteCommunicationClient::CalculateMD5( return false; if (response.Peek() && *response.Peek() == 'x') return false; -low = response.GetHexMaxU64(false, UINT64_MAX); -high = response.GetHexMaxU64(false, UINT64_MAX); + +// GDBRemoteCommunicationServerCommon::Handle_vFile_MD5 concatenates low and +// high hex strings. We can't use response.GetHexMaxU64 because that can't +// handle the concatenated hex string. What would happen is parsing the low +// would consume the whole response packet - which is a bug. Instead, we get Awfa wrote: `response.GetHexMaxU64` consumes the response stream until a non-valid hex character is encountered. This is just the wrong function to use because the server returns both the low/high parts concatenated. When it was used, what would happen is `low = response.GetHexMaxU64...` would consume the whole packet, when it should just be consuming the first half of it. This also leads to `high` being set to 0 because `low` consumed the entire packet. This part of the patch isn't a workaround, it's the fix. This fixes the client so it can correctly parse the server's response. https://github.com/llvm/llvm-project/pull/88812 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
@@ -684,6 +684,15 @@ Status PlatformRemoteGDBServer::RunShellCommand(
signo_ptr, command_output, timeout);
}
+bool PlatformRemoteGDBServer::CalculateMD5(const FileSpec &file_spec,
+ uint64_t &low, uint64_t &high) {
+ if (IsConnected()) {
Awfa wrote:
Should I change the other functions in `PlatformRemoteGDBServer` to do the
same? For example `PlatformRemoteGDBServer::GetFileExists` does the same
currently.
https://github.com/llvm/llvm-project/pull/88812
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
@@ -684,6 +684,15 @@ Status PlatformRemoteGDBServer::RunShellCommand(
signo_ptr, command_output, timeout);
}
+bool PlatformRemoteGDBServer::CalculateMD5(const FileSpec &file_spec,
+ uint64_t &low, uint64_t &high) {
+ if (IsConnected()) {
+// Note that high/low are switched in the gdb remote communication client
Awfa wrote:
Good point - I don't see a reason why it is swapped there. I'll change it so
it's just consistent.
https://github.com/llvm/llvm-project/pull/88812
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
@@ -1184,7 +1184,7 @@ bool Platform::IsCompatibleArchitecture(const ArchSpec
&arch,
Status Platform::PutFile(const FileSpec &source, const FileSpec &destination,
uint32_t uid, uint32_t gid) {
Log *log = GetLog(LLDBLog::Platform);
- LLDB_LOGF(log, "[PutFile] Using block by block transfer\n");
+ LLDB_LOGF(log, "[PutFile] Using block by block transfer");
Awfa wrote:
When testing this change, and using `log enable lldb platform` to read the
logs, I just noticed this had a `\n` and removed it since `LLDB_LOGF` already
adds a new line, and it didn't seem like most other logs had `\n`.
Is this bad to have in the patch?
https://github.com/llvm/llvm-project/pull/88812
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
@@ -1197,6 +1197,34 @@ Status Platform::PutFile(const FileSpec &source, const
FileSpec &destination,
if (!source_file)
return Status(source_file.takeError());
Status error;
+
+ bool requires_upload = true;
+ {
Awfa wrote:
This is how I write code to scope the variables `dest_md5_low`,
`dest_md5_high`, `success` away from the main scope of the function.
Is this not good to do in the llvm-project?
https://github.com/llvm/llvm-project/pull/88812
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
@@ -3433,8 +3433,40 @@ bool GDBRemoteCommunicationClient::CalculateMD5( return false; if (response.Peek() && *response.Peek() == 'x') return false; -low = response.GetHexMaxU64(false, UINT64_MAX); -high = response.GetHexMaxU64(false, UINT64_MAX); + +// GDBRemoteCommunicationServerCommon::Handle_vFile_MD5 concatenates low and +// high hex strings. We can't use response.GetHexMaxU64 because that can't +// handle the concatenated hex string. What would happen is parsing the low +// would consume the whole response packet - which is a bug. Instead, we get +// the byte string for each low and high hex separately, and parse them. +// +// An alternate way to handle this is to change the server to put a +// delimiter between the low/high parts, and change the client to parse the +// delimiter. However, we choose not to do this so existing lldb-servers +// don't have to be patched + +// Get low part +auto part = response.GetStringRef().substr(response.GetFilePos(), + sizeof(uint64_t) * 2); Awfa wrote: Will do https://github.com/llvm/llvm-project/pull/88812 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
@@ -1197,6 +1197,34 @@ Status Platform::PutFile(const FileSpec &source, const
FileSpec &destination,
if (!source_file)
return Status(source_file.takeError());
Status error;
+
+ bool requires_upload = true;
+ {
+uint64_t dest_md5_low, dest_md5_high;
+bool success = CalculateMD5(destination, dest_md5_low, dest_md5_high);
+if (!success) {
+ LLDB_LOGF(log, "[PutFile] couldn't get md5 sum of destination");
+} else {
+ auto local_md5 = llvm::sys::fs::md5_contents(source.GetPath());
+ if (!local_md5) {
+LLDB_LOGF(log, "[PutFile] couldn't get md5 sum of source");
+ } else {
+uint64_t local_md5_high, local_md5_low;
+std::tie(local_md5_high, local_md5_low) = local_md5->words();
Awfa wrote:
Will do
https://github.com/llvm/llvm-project/pull/88812
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
https://github.com/Awfa updated https://github.com/llvm/llvm-project/pull/88812
>From 095696411172034f80233f1722e293c1f458d67f Mon Sep 17 00:00:00 2001
From: Anthony Ha
Date: Mon, 15 Apr 2024 19:34:19 +
Subject: [PATCH 1/2] [lldb] Skip remote PutFile when MD5 hashes equal
---
.../gdb-server/PlatformRemoteGDBServer.cpp| 9 +
.../gdb-server/PlatformRemoteGDBServer.h | 3 ++
.../GDBRemoteCommunicationClient.cpp | 36 +--
lldb/source/Target/Platform.cpp | 30 +++-
.../GDBRemoteCommunicationClientTest.cpp | 23
5 files changed, 98 insertions(+), 3 deletions(-)
diff --git
a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
index 88f1ad15b6b485..d9f3e40019cf29 100644
--- a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
+++ b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
@@ -684,6 +684,15 @@ Status PlatformRemoteGDBServer::RunShellCommand(
signo_ptr, command_output, timeout);
}
+bool PlatformRemoteGDBServer::CalculateMD5(const FileSpec &file_spec,
+ uint64_t &low, uint64_t &high) {
+ if (IsConnected()) {
+// Note that high/low are switched in the gdb remote communication client
+return m_gdb_client_up->CalculateMD5(file_spec, high, low);
+ }
+ return false;
+}
+
void PlatformRemoteGDBServer::CalculateTrapHandlerSymbolNames() {
m_trap_handlers.push_back(ConstString("_sigtramp"));
}
diff --git a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
index 638f7db5ef800c..d83fc386f59427 100644
--- a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
+++ b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
@@ -146,6 +146,9 @@ class PlatformRemoteGDBServer : public Platform, private
UserIDResolver {
void CalculateTrapHandlerSymbolNames() override;
+ bool CalculateMD5(const FileSpec &file_spec, uint64_t &low,
+uint64_t &high) override;
+
const lldb::UnixSignalsSP &GetRemoteUnixSignals() override;
size_t ConnectToWaitingProcesses(lldb_private::Debugger &debugger,
diff --git
a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
index 1f6116967a4f64..8c79d5fecf12fe 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -3433,8 +3433,40 @@ bool GDBRemoteCommunicationClient::CalculateMD5(
return false;
if (response.Peek() && *response.Peek() == 'x')
return false;
-low = response.GetHexMaxU64(false, UINT64_MAX);
-high = response.GetHexMaxU64(false, UINT64_MAX);
+
+// GDBRemoteCommunicationServerCommon::Handle_vFile_MD5 concatenates low
and
+// high hex strings. We can't use response.GetHexMaxU64 because that can't
+// handle the concatenated hex string. What would happen is parsing the low
+// would consume the whole response packet - which is a bug. Instead, we
get
+// the byte string for each low and high hex separately, and parse them.
+//
+// An alternate way to handle this is to change the server to put a
+// delimiter between the low/high parts, and change the client to parse the
+// delimiter. However, we choose not to do this so existing lldb-servers
+// don't have to be patched
+
+// Get low part
+auto part = response.GetStringRef().substr(response.GetFilePos(),
+ sizeof(uint64_t) * 2);
+if (part.size() != sizeof(uint64_t) * 2)
+ return false;
+response.SetFilePos(response.GetFilePos() + part.size());
+
+bool conversionErrored = part.getAsInteger(16, low);
+if (conversionErrored)
+ return false;
+
+// Get high part
+part = response.GetStringRef().substr(response.GetFilePos(),
+ sizeof(uint64_t) * 2);
+if (part.size() != sizeof(uint64_t) * 2)
+ return false;
+response.SetFilePos(response.GetFilePos() + part.size());
+
+conversionErrored = part.getAsInteger(16, high);
+if (conversionErrored)
+ return false;
+
return true;
}
return false;
diff --git a/lldb/source/Target/Platform.cpp b/lldb/source/Target/Platform.cpp
index 4ce290dfbe035f..cdbafb17a5df4d 100644
--- a/lldb/source/Target/Platform.cpp
+++ b/lldb/source/Target/Platform.cpp
@@ -1184,7 +1184,7 @@ bool Platform::IsCompatibleArchitecture(const ArchSpec
&arch,
Status Platform::PutFile(const FileSpec &source, const FileSpec &destination,
uint32_t uid, uint32_t gid) {
Log *log = GetLog(LLDBLog::Platform);
- LLDB_LOGF(log, "[PutFile]
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
@@ -684,6 +684,15 @@ Status PlatformRemoteGDBServer::RunShellCommand(
signo_ptr, command_output, timeout);
}
+bool PlatformRemoteGDBServer::CalculateMD5(const FileSpec &file_spec,
+ uint64_t &low, uint64_t &high) {
+ if (IsConnected()) {
Awfa wrote:
I'll change just this instance to meet your suggestion
https://github.com/llvm/llvm-project/pull/88812
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Have lldb-server assign ports to children in platform mode (PR #88845)
https://github.com/Awfa created https://github.com/llvm/llvm-project/pull/88845
`lldb-server`'s platform mode seems to have an issue with its
`--min-gdbserver-port` `--max-gdbserver-port` flags (and probably the
`--gdbserver-port` flag, but I didn't test it).
How the platform code seems to work is that it listens on a port, and whenever
there's an incoming connection, it forks the process to handle the connection.
To handle the port flags, the main process uses an instance of the helper class
`GDBRemoteCommunicationServerPlatform::PortMap`, that can be configured and
track usages of ports. The child process handling the platform connection, can
then use the port map to allocate a port for the gdb-server connection it will
make (this is another process it spawns).
However, in the current code, this works only once. After the first connection
is handled by forking a child process, the main platform listener code loops
around, and then 'forgets' about the port map. This is because this code:
```cpp
GDBRemoteCommunicationServerPlatform platform(
acceptor_up->GetSocketProtocol(), acceptor_up->GetSocketScheme());
if (!gdbserver_portmap.empty()) {
platform.SetPortMap(std::move(gdbserver_portmap));
}
```
is within the connection listening loop. This results in the
`gdbserver_portmap` being moved into the platform object at the beginning of
the first iteration of the loop, but on the second iteration, after the first
fork, the next instance of the platform object will not have its platform port
mapped.
The result of this bug is that subsequent connections to the platform, when
spawning the gdb-remote connection, will be supplied a random port - which
isn't bounded by the `--min-gdbserver-port` and `--max-gdbserver--port`
parameters passed in by the user.
This PR fixes this issue by having the port map be maintained by the parent
platform listener process. On connection, the listener allocates a single
available port from the port map, associates the child process pid with the
port, and lets the connection handling child use that single port number.
Additionally, when cleaning up child processes, the main listener process
tracks the child that exited to deallocate the previously associated port, so
it can be reused for a new connection.
For review:
- I haven't used C++ optionals before this, and was going off of surrounding
related code. Let me know if I got something wrong.
- I'm not sure where this code could be tested. This was only manually tested
by me so far.
>From 3d75f42b5f61ea126001919491aa09ebd15ba9f8 Mon Sep 17 00:00:00 2001
From: Anthony Ha
Date: Mon, 15 Apr 2024 19:36:34 +
Subject: [PATCH] [lldb] Have lldb-server assign ports to children in platform
mode
---
lldb/tools/lldb-server/lldb-platform.cpp | 41
1 file changed, 28 insertions(+), 13 deletions(-)
diff --git a/lldb/tools/lldb-server/lldb-platform.cpp
b/lldb/tools/lldb-server/lldb-platform.cpp
index 3e126584eb25b4..384709ba79b656 100644
--- a/lldb/tools/lldb-server/lldb-platform.cpp
+++ b/lldb/tools/lldb-server/lldb-platform.cpp
@@ -282,17 +282,10 @@ int main_platform(int argc, char *argv[]) {
}
}
- do {
-GDBRemoteCommunicationServerPlatform platform(
-acceptor_up->GetSocketProtocol(), acceptor_up->GetSocketScheme());
-
-if (port_offset > 0)
- platform.SetPortOffset(port_offset);
-
-if (!gdbserver_portmap.empty()) {
- platform.SetPortMap(std::move(gdbserver_portmap));
-}
+ GDBRemoteCommunicationServerPlatform platform(
+ acceptor_up->GetSocketProtocol(), acceptor_up->GetSocketScheme());
+ do {
const bool children_inherit_accept_socket = true;
Connection *conn = nullptr;
error = acceptor_up->Accept(children_inherit_accept_socket, conn);
@@ -301,13 +294,31 @@ int main_platform(int argc, char *argv[]) {
exit(socket_error);
}
printf("Connection established.\n");
+
+std::optional port = 0;
if (g_server) {
// Collect child zombie processes.
#if !defined(_WIN32)
- while (waitpid(-1, nullptr, WNOHANG) > 0)
-;
+ auto waitResult = waitpid(-1, nullptr, WNOHANG);
+ while (waitResult > 0) {
+// waitResult is the child pid
+gdbserver_portmap.FreePortForProcess(waitResult);
+waitResult = waitpid(-1, nullptr, WNOHANG);
+ }
#endif
- if (fork()) {
+ llvm::Expected available_port =
+ gdbserver_portmap.GetNextAvailablePort();
+ if (available_port)
+port = *available_port;
+
+ else {
+fprintf(stderr, "no available port for connection - dropping...\n");
+delete conn;
+continue;
+ }
+ auto childPid = fork();
+ if (childPid) {
+gdbserver_portmap.AssociatePortWithProcess(*available_port, childPid);
// Parent doesn't need a connection to the lldb client
delete conn;
@@ -324,6 +335,10 @@ int main_platform(int argc, char *argv[]) {
// connect
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
https://github.com/Awfa updated https://github.com/llvm/llvm-project/pull/88812
>From 095696411172034f80233f1722e293c1f458d67f Mon Sep 17 00:00:00 2001
From: Anthony Ha
Date: Mon, 15 Apr 2024 19:34:19 +
Subject: [PATCH 1/3] [lldb] Skip remote PutFile when MD5 hashes equal
---
.../gdb-server/PlatformRemoteGDBServer.cpp| 9 +
.../gdb-server/PlatformRemoteGDBServer.h | 3 ++
.../GDBRemoteCommunicationClient.cpp | 36 +--
lldb/source/Target/Platform.cpp | 30 +++-
.../GDBRemoteCommunicationClientTest.cpp | 23
5 files changed, 98 insertions(+), 3 deletions(-)
diff --git
a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
index 88f1ad15b6b485..d9f3e40019cf29 100644
--- a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
+++ b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
@@ -684,6 +684,15 @@ Status PlatformRemoteGDBServer::RunShellCommand(
signo_ptr, command_output, timeout);
}
+bool PlatformRemoteGDBServer::CalculateMD5(const FileSpec &file_spec,
+ uint64_t &low, uint64_t &high) {
+ if (IsConnected()) {
+// Note that high/low are switched in the gdb remote communication client
+return m_gdb_client_up->CalculateMD5(file_spec, high, low);
+ }
+ return false;
+}
+
void PlatformRemoteGDBServer::CalculateTrapHandlerSymbolNames() {
m_trap_handlers.push_back(ConstString("_sigtramp"));
}
diff --git a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
index 638f7db5ef800c..d83fc386f59427 100644
--- a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
+++ b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
@@ -146,6 +146,9 @@ class PlatformRemoteGDBServer : public Platform, private
UserIDResolver {
void CalculateTrapHandlerSymbolNames() override;
+ bool CalculateMD5(const FileSpec &file_spec, uint64_t &low,
+uint64_t &high) override;
+
const lldb::UnixSignalsSP &GetRemoteUnixSignals() override;
size_t ConnectToWaitingProcesses(lldb_private::Debugger &debugger,
diff --git
a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
index 1f6116967a4f64..8c79d5fecf12fe 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -3433,8 +3433,40 @@ bool GDBRemoteCommunicationClient::CalculateMD5(
return false;
if (response.Peek() && *response.Peek() == 'x')
return false;
-low = response.GetHexMaxU64(false, UINT64_MAX);
-high = response.GetHexMaxU64(false, UINT64_MAX);
+
+// GDBRemoteCommunicationServerCommon::Handle_vFile_MD5 concatenates low
and
+// high hex strings. We can't use response.GetHexMaxU64 because that can't
+// handle the concatenated hex string. What would happen is parsing the low
+// would consume the whole response packet - which is a bug. Instead, we
get
+// the byte string for each low and high hex separately, and parse them.
+//
+// An alternate way to handle this is to change the server to put a
+// delimiter between the low/high parts, and change the client to parse the
+// delimiter. However, we choose not to do this so existing lldb-servers
+// don't have to be patched
+
+// Get low part
+auto part = response.GetStringRef().substr(response.GetFilePos(),
+ sizeof(uint64_t) * 2);
+if (part.size() != sizeof(uint64_t) * 2)
+ return false;
+response.SetFilePos(response.GetFilePos() + part.size());
+
+bool conversionErrored = part.getAsInteger(16, low);
+if (conversionErrored)
+ return false;
+
+// Get high part
+part = response.GetStringRef().substr(response.GetFilePos(),
+ sizeof(uint64_t) * 2);
+if (part.size() != sizeof(uint64_t) * 2)
+ return false;
+response.SetFilePos(response.GetFilePos() + part.size());
+
+conversionErrored = part.getAsInteger(16, high);
+if (conversionErrored)
+ return false;
+
return true;
}
return false;
diff --git a/lldb/source/Target/Platform.cpp b/lldb/source/Target/Platform.cpp
index 4ce290dfbe035f..cdbafb17a5df4d 100644
--- a/lldb/source/Target/Platform.cpp
+++ b/lldb/source/Target/Platform.cpp
@@ -1184,7 +1184,7 @@ bool Platform::IsCompatibleArchitecture(const ArchSpec
&arch,
Status Platform::PutFile(const FileSpec &source, const FileSpec &destination,
uint32_t uid, uint32_t gid) {
Log *log = GetLog(LLDBLog::Platform);
- LLDB_LOGF(log, "[PutFile]
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
@@ -1184,7 +1184,7 @@ bool Platform::IsCompatibleArchitecture(const ArchSpec
&arch,
Status Platform::PutFile(const FileSpec &source, const FileSpec &destination,
uint32_t uid, uint32_t gid) {
Log *log = GetLog(LLDBLog::Platform);
- LLDB_LOGF(log, "[PutFile] Using block by block transfer\n");
+ LLDB_LOGF(log, "[PutFile] Using block by block transfer");
Awfa wrote:
I decided to leave it out of this PR
https://github.com/llvm/llvm-project/pull/88812
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
@@ -1197,6 +1197,34 @@ Status Platform::PutFile(const FileSpec &source, const
FileSpec &destination,
if (!source_file)
return Status(source_file.takeError());
Status error;
+
+ bool requires_upload = true;
+ {
Awfa wrote:
Just removed the scoping - so the md5 vars are part of the main scope.
https://github.com/llvm/llvm-project/pull/88812
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Have lldb-server assign ports to children in platform mode (PR #88845)
https://github.com/Awfa updated https://github.com/llvm/llvm-project/pull/88845
>From 3d75f42b5f61ea126001919491aa09ebd15ba9f8 Mon Sep 17 00:00:00 2001
From: Anthony Ha
Date: Mon, 15 Apr 2024 19:36:34 +
Subject: [PATCH 1/2] [lldb] Have lldb-server assign ports to children in
platform mode
---
lldb/tools/lldb-server/lldb-platform.cpp | 41
1 file changed, 28 insertions(+), 13 deletions(-)
diff --git a/lldb/tools/lldb-server/lldb-platform.cpp
b/lldb/tools/lldb-server/lldb-platform.cpp
index 3e126584eb25b4..384709ba79b656 100644
--- a/lldb/tools/lldb-server/lldb-platform.cpp
+++ b/lldb/tools/lldb-server/lldb-platform.cpp
@@ -282,17 +282,10 @@ int main_platform(int argc, char *argv[]) {
}
}
- do {
-GDBRemoteCommunicationServerPlatform platform(
-acceptor_up->GetSocketProtocol(), acceptor_up->GetSocketScheme());
-
-if (port_offset > 0)
- platform.SetPortOffset(port_offset);
-
-if (!gdbserver_portmap.empty()) {
- platform.SetPortMap(std::move(gdbserver_portmap));
-}
+ GDBRemoteCommunicationServerPlatform platform(
+ acceptor_up->GetSocketProtocol(), acceptor_up->GetSocketScheme());
+ do {
const bool children_inherit_accept_socket = true;
Connection *conn = nullptr;
error = acceptor_up->Accept(children_inherit_accept_socket, conn);
@@ -301,13 +294,31 @@ int main_platform(int argc, char *argv[]) {
exit(socket_error);
}
printf("Connection established.\n");
+
+std::optional port = 0;
if (g_server) {
// Collect child zombie processes.
#if !defined(_WIN32)
- while (waitpid(-1, nullptr, WNOHANG) > 0)
-;
+ auto waitResult = waitpid(-1, nullptr, WNOHANG);
+ while (waitResult > 0) {
+// waitResult is the child pid
+gdbserver_portmap.FreePortForProcess(waitResult);
+waitResult = waitpid(-1, nullptr, WNOHANG);
+ }
#endif
- if (fork()) {
+ llvm::Expected available_port =
+ gdbserver_portmap.GetNextAvailablePort();
+ if (available_port)
+port = *available_port;
+
+ else {
+fprintf(stderr, "no available port for connection - dropping...\n");
+delete conn;
+continue;
+ }
+ auto childPid = fork();
+ if (childPid) {
+gdbserver_portmap.AssociatePortWithProcess(*available_port, childPid);
// Parent doesn't need a connection to the lldb client
delete conn;
@@ -324,6 +335,10 @@ int main_platform(int argc, char *argv[]) {
// connections while a connection is active.
acceptor_up.reset();
}
+
+GDBRemoteCommunicationServerPlatform::PortMap portmap_for_child;
+portmap_for_child.AllowPort(*port);
+platform.SetPortMap(std::move(portmap_for_child));
platform.SetConnection(std::unique_ptr(conn));
if (platform.IsConnected()) {
>From 27782fb03b3705e9ff4a5b3cc0d17c8448919be9 Mon Sep 17 00:00:00 2001
From: Anthony Ha
Date: Wed, 17 Apr 2024 00:46:58 +
Subject: [PATCH 2/2] amend! [lldb] Have lldb-server assign ports to children
in platform mode
[lldb] Have lldb-server assign ports to children in platform mode
---
lldb/tools/lldb-server/lldb-platform.cpp | 24 ++--
1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/lldb/tools/lldb-server/lldb-platform.cpp
b/lldb/tools/lldb-server/lldb-platform.cpp
index 384709ba79b656..ff91a3318cf0d5 100644
--- a/lldb/tools/lldb-server/lldb-platform.cpp
+++ b/lldb/tools/lldb-server/lldb-platform.cpp
@@ -295,27 +295,31 @@ int main_platform(int argc, char *argv[]) {
}
printf("Connection established.\n");
-std::optional port = 0;
if (g_server) {
// Collect child zombie processes.
#if !defined(_WIN32)
- auto waitResult = waitpid(-1, nullptr, WNOHANG);
- while (waitResult > 0) {
+ ::pid_t waitResult;
+ while ((waitResult = waitpid(-1, nullptr, WNOHANG)) > 0) {
// waitResult is the child pid
gdbserver_portmap.FreePortForProcess(waitResult);
-waitResult = waitpid(-1, nullptr, WNOHANG);
}
#endif
+ // TODO: Clean up portmap for Windows when children die
+
+ // After collecting zombie ports, get the next available
+ GDBRemoteCommunicationServerPlatform::PortMap portmap_for_child;
llvm::Expected available_port =
gdbserver_portmap.GetNextAvailablePort();
if (available_port)
-port = *available_port;
-
+portmap_for_child.AllowPort(*available_port);
else {
-fprintf(stderr, "no available port for connection - dropping...\n");
+fprintf(stderr,
+"no available gdbserver port for connection - dropping...\n");
delete conn;
continue;
}
+ platform.SetPortMap(std::move(portmap_for_child));
+
auto childPid = fork();
if (childPid) {
gdbserver_portmap.AssociatePortWithProcess(*available_port, childPid);
@@ -334,11 +338,11 @@ int main_platfo
[Lldb-commits] [lldb] [lldb] Have lldb-server assign ports to children in platform mode (PR #88845)
@@ -301,13 +294,31 @@ int main_platform(int argc, char *argv[]) {
exit(socket_error);
}
printf("Connection established.\n");
+
+std::optional port = 0;
if (g_server) {
// Collect child zombie processes.
#if !defined(_WIN32)
- while (waitpid(-1, nullptr, WNOHANG) > 0)
-;
+ auto waitResult = waitpid(-1, nullptr, WNOHANG);
+ while (waitResult > 0) {
+// waitResult is the child pid
+gdbserver_portmap.FreePortForProcess(waitResult);
+waitResult = waitpid(-1, nullptr, WNOHANG);
+ }
#endif
- if (fork()) {
+ llvm::Expected available_port =
+ gdbserver_portmap.GetNextAvailablePort();
+ if (available_port)
+port = *available_port;
+
+ else {
Awfa wrote:
In the else branch, the connection to the platform will just be dropped, and an
error printed to stderr.
This is a recoverable error in the sense that when a child platform process
dies, a port will become available again - so it's desirable not to crash
`lldb-server`.
https://github.com/llvm/llvm-project/pull/88845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Have lldb-server assign ports to children in platform mode (PR #88845)
@@ -301,13 +294,31 @@ int main_platform(int argc, char *argv[]) {
exit(socket_error);
}
printf("Connection established.\n");
+
+std::optional port = 0;
if (g_server) {
// Collect child zombie processes.
#if !defined(_WIN32)
- while (waitpid(-1, nullptr, WNOHANG) > 0)
-;
+ auto waitResult = waitpid(-1, nullptr, WNOHANG);
+ while (waitResult > 0) {
Awfa wrote:
Updated
https://github.com/llvm/llvm-project/pull/88845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Have lldb-server assign ports to children in platform mode (PR #88845)
@@ -301,13 +294,31 @@ int main_platform(int argc, char *argv[]) {
exit(socket_error);
}
printf("Connection established.\n");
+
+std::optional port = 0;
Awfa wrote:
I removed the optional port here, and just set the portmap within the
availablePort branch.
I also realized I forgot to take into account the non-server mode, so I put
`platform.SetPortMap(std::move(gdbserver_portmap));` for the else branch where
`lldb-server` isn't running with the `--server` flag.
https://github.com/llvm/llvm-project/pull/88845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Have lldb-server assign ports to children in platform mode (PR #88845)
@@ -324,6 +335,10 @@ int main_platform(int argc, char *argv[]) {
// connections while a connection is active.
acceptor_up.reset();
}
+
+GDBRemoteCommunicationServerPlatform::PortMap portmap_for_child;
+portmap_for_child.AllowPort(*port);
Awfa wrote:
I removed the optional port and changed it - so reading the code should be more
straightforward, and we shouldn't have to think about this.
https://github.com/llvm/llvm-project/pull/88845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
Awfa wrote: > MD5 is insufficient for claiming that files are identical; how do we migrate > this to a secure hash? Is there an attack vector you're concerned about? Or are you wary of workflow friction when a file won't upload to the remote platform because the hashes accidently collide? https://github.com/llvm/llvm-project/pull/88812 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
Awfa wrote: Someone will have to merge for me because it says "Only those with write access to this repository can merge pull requests." Am I supposed to request write access somewhere before making these PRs? I haven't contributed before. https://github.com/llvm/llvm-project/pull/88812 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
Awfa wrote: I got an email that the build failed and I should revert because of this: https://lab.llvm.org/buildbot/#/builders/68/builds/72623 @bulbazord can you or someone with write permissions revert this PR so I have time to triage the issue? https://github.com/llvm/llvm-project/pull/88812 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
Awfa wrote: Thanks for taking a look at the build. I will do a new PR addressing the issues. https://github.com/llvm/llvm-project/pull/88812 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][Docs] Document vFile:exists and vFile:MD5 (PR #89357)
@@ -429,7 +429,43 @@ incompatible with the flags that gdb specifies. // // Response is F, followed by the number of bytes written (base 16) +//-- +// vFile:MD5: +// +// BRIEF +// Generate an MD5 hash of the file at the given path. +// +// EXAMPLE +// +// receive: vFile:MD5:2f746d702f61 +// send (success): F, +// send (failure): F,x +// +// request packet contains the ASCII hex encoded filename +// +// If the hash succeeded, the response is "F," followed by the low 64 Awfa wrote: By the way, I remember observing that the hash that comes out from `CalculateMD5` differed from the `md5sum` command before. Like entirely - not just the halves swapped. https://github.com/llvm/llvm-project/pull/89357 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][Docs] Document vFile:exists and vFile:MD5 (PR #89357)
https://github.com/Awfa edited https://github.com/llvm/llvm-project/pull/89357 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Unify CalculateMD5 return types (PR #90921)
https://github.com/Awfa created https://github.com/llvm/llvm-project/pull/90921
# Overview
In my previous PR: https://github.com/llvm/llvm-project/pull/88812,
@JDevlieghere suggested to match return types of the various calculate md5
functions.
This PR achieves that by changing the various calculate md5 functions to return
`llvm::ErrorOr`.
The suggestion was to go for `std::optional<>` but I opted for
`llvm::ErrorOr<>` because local calculate md5 was already possibly returning
`ErrorOr`.
To make sure I didn't break the md5 calculation functionality, I ran some tests
for the gdb remote client, and things seem to work.
# Testing
1. Remote file doesn't exist
1. Remote file differs
1. Remote file matches
## Test gaps
Unfortunately, I had to modify
`lldb/source/Plugins/Platform/MacOSX/PlatformDarwinDevice.cpp` and I can't test
the changes there. Hopefully, the existing test suite / code review from
whomever is reading this will catch any issues.
>From 3112e3da309a24001aa610b15f2a23892b660fc8 Mon Sep 17 00:00:00 2001
From: Anthony Ha
Date: Thu, 2 May 2024 23:55:47 +
Subject: [PATCH] Unify CalculateMD5 return types
---
lldb/include/lldb/Target/Platform.h | 4 +--
.../include/lldb/Target/RemoteAwarePlatform.h | 4 +--
.../Platform/MacOSX/PlatformDarwinDevice.cpp | 16 +
.../gdb-server/PlatformRemoteGDBServer.cpp| 8 ++---
.../gdb-server/PlatformRemoteGDBServer.h | 4 +--
.../GDBRemoteCommunicationClient.cpp | 30 ++--
.../gdb-remote/GDBRemoteCommunicationClient.h | 2 +-
lldb/source/Target/Platform.cpp | 36 +--
lldb/source/Target/RemoteAwarePlatform.cpp| 8 ++---
9 files changed, 60 insertions(+), 52 deletions(-)
diff --git a/lldb/include/lldb/Target/Platform.h
b/lldb/include/lldb/Target/Platform.h
index ad9c9dcbe684ba..e05c79cb501bf0 100644
--- a/lldb/include/lldb/Target/Platform.h
+++ b/lldb/include/lldb/Target/Platform.h
@@ -649,8 +649,8 @@ class Platform : public PluginInterface {
virtual std::string GetPlatformSpecificConnectionInformation() { return ""; }
- virtual bool CalculateMD5(const FileSpec &file_spec, uint64_t &low,
-uint64_t &high);
+ virtual llvm::ErrorOr
+ CalculateMD5(const FileSpec &file_spec);
virtual uint32_t GetResumeCountForLaunchInfo(ProcessLaunchInfo &launch_info)
{
return 1;
diff --git a/lldb/include/lldb/Target/RemoteAwarePlatform.h
b/lldb/include/lldb/Target/RemoteAwarePlatform.h
index d183815e1c8b07..0b9d79f9ff0380 100644
--- a/lldb/include/lldb/Target/RemoteAwarePlatform.h
+++ b/lldb/include/lldb/Target/RemoteAwarePlatform.h
@@ -58,8 +58,8 @@ class RemoteAwarePlatform : public Platform {
Status SetFilePermissions(const FileSpec &file_spec,
uint32_t file_permissions) override;
- bool CalculateMD5(const FileSpec &file_spec, uint64_t &low,
-uint64_t &high) override;
+ llvm::ErrorOr
+ CalculateMD5(const FileSpec &file_spec) override;
Status GetFileWithUUID(const FileSpec &platform_file, const UUID *uuid,
FileSpec &local_file) override;
diff --git a/lldb/source/Plugins/Platform/MacOSX/PlatformDarwinDevice.cpp
b/lldb/source/Plugins/Platform/MacOSX/PlatformDarwinDevice.cpp
index 52777909a1f825..82156aca8cf159 100644
--- a/lldb/source/Plugins/Platform/MacOSX/PlatformDarwinDevice.cpp
+++ b/lldb/source/Plugins/Platform/MacOSX/PlatformDarwinDevice.cpp
@@ -405,17 +405,21 @@ lldb_private::Status
PlatformDarwinDevice::GetSharedModuleWithLocalCache(
// when going over the *slow* GDB remote transfer mechanism we first
// check the hashes of the files - and only do the actual transfer if
// they differ
- uint64_t high_local, high_remote, low_local, low_remote;
auto MD5 = llvm::sys::fs::md5_contents(module_cache_spec.GetPath());
if (!MD5)
return Status(MD5.getError());
- std::tie(high_local, low_local) = MD5->words();
- m_remote_platform_sp->CalculateMD5(module_spec.GetFileSpec(),
- low_remote, high_remote);
- if (low_local != low_remote || high_local != high_remote) {
+ Log *log = GetLog(LLDBLog::Platform);
+ bool requires_transfer = true;
+ llvm::ErrorOr remote_md5 =
+ m_remote_platform_sp->CalculateMD5(module_spec.GetFileSpec());
+ if (std::error_code ec = remote_md5.getError())
+LLDB_LOG(log, "couldn't get md5 sum from remote: {0}",
+ ec.message());
+ else
+requires_transfer = *MD5 != *remote_md5;
+
[Lldb-commits] [lldb] [lldb] Have lldb-server assign ports to children in platform mode (PR #88845)
https://github.com/Awfa updated https://github.com/llvm/llvm-project/pull/88845
>From 3d75f42b5f61ea126001919491aa09ebd15ba9f8 Mon Sep 17 00:00:00 2001
From: Anthony Ha
Date: Mon, 15 Apr 2024 19:36:34 +
Subject: [PATCH 1/3] [lldb] Have lldb-server assign ports to children in
platform mode
---
lldb/tools/lldb-server/lldb-platform.cpp | 41
1 file changed, 28 insertions(+), 13 deletions(-)
diff --git a/lldb/tools/lldb-server/lldb-platform.cpp
b/lldb/tools/lldb-server/lldb-platform.cpp
index 3e126584eb25b4..384709ba79b656 100644
--- a/lldb/tools/lldb-server/lldb-platform.cpp
+++ b/lldb/tools/lldb-server/lldb-platform.cpp
@@ -282,17 +282,10 @@ int main_platform(int argc, char *argv[]) {
}
}
- do {
-GDBRemoteCommunicationServerPlatform platform(
-acceptor_up->GetSocketProtocol(), acceptor_up->GetSocketScheme());
-
-if (port_offset > 0)
- platform.SetPortOffset(port_offset);
-
-if (!gdbserver_portmap.empty()) {
- platform.SetPortMap(std::move(gdbserver_portmap));
-}
+ GDBRemoteCommunicationServerPlatform platform(
+ acceptor_up->GetSocketProtocol(), acceptor_up->GetSocketScheme());
+ do {
const bool children_inherit_accept_socket = true;
Connection *conn = nullptr;
error = acceptor_up->Accept(children_inherit_accept_socket, conn);
@@ -301,13 +294,31 @@ int main_platform(int argc, char *argv[]) {
exit(socket_error);
}
printf("Connection established.\n");
+
+std::optional port = 0;
if (g_server) {
// Collect child zombie processes.
#if !defined(_WIN32)
- while (waitpid(-1, nullptr, WNOHANG) > 0)
-;
+ auto waitResult = waitpid(-1, nullptr, WNOHANG);
+ while (waitResult > 0) {
+// waitResult is the child pid
+gdbserver_portmap.FreePortForProcess(waitResult);
+waitResult = waitpid(-1, nullptr, WNOHANG);
+ }
#endif
- if (fork()) {
+ llvm::Expected available_port =
+ gdbserver_portmap.GetNextAvailablePort();
+ if (available_port)
+port = *available_port;
+
+ else {
+fprintf(stderr, "no available port for connection - dropping...\n");
+delete conn;
+continue;
+ }
+ auto childPid = fork();
+ if (childPid) {
+gdbserver_portmap.AssociatePortWithProcess(*available_port, childPid);
// Parent doesn't need a connection to the lldb client
delete conn;
@@ -324,6 +335,10 @@ int main_platform(int argc, char *argv[]) {
// connections while a connection is active.
acceptor_up.reset();
}
+
+GDBRemoteCommunicationServerPlatform::PortMap portmap_for_child;
+portmap_for_child.AllowPort(*port);
+platform.SetPortMap(std::move(portmap_for_child));
platform.SetConnection(std::unique_ptr(conn));
if (platform.IsConnected()) {
>From 27782fb03b3705e9ff4a5b3cc0d17c8448919be9 Mon Sep 17 00:00:00 2001
From: Anthony Ha
Date: Wed, 17 Apr 2024 00:46:58 +
Subject: [PATCH 2/3] amend! [lldb] Have lldb-server assign ports to children
in platform mode
[lldb] Have lldb-server assign ports to children in platform mode
---
lldb/tools/lldb-server/lldb-platform.cpp | 24 ++--
1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/lldb/tools/lldb-server/lldb-platform.cpp
b/lldb/tools/lldb-server/lldb-platform.cpp
index 384709ba79b656..ff91a3318cf0d5 100644
--- a/lldb/tools/lldb-server/lldb-platform.cpp
+++ b/lldb/tools/lldb-server/lldb-platform.cpp
@@ -295,27 +295,31 @@ int main_platform(int argc, char *argv[]) {
}
printf("Connection established.\n");
-std::optional port = 0;
if (g_server) {
// Collect child zombie processes.
#if !defined(_WIN32)
- auto waitResult = waitpid(-1, nullptr, WNOHANG);
- while (waitResult > 0) {
+ ::pid_t waitResult;
+ while ((waitResult = waitpid(-1, nullptr, WNOHANG)) > 0) {
// waitResult is the child pid
gdbserver_portmap.FreePortForProcess(waitResult);
-waitResult = waitpid(-1, nullptr, WNOHANG);
}
#endif
+ // TODO: Clean up portmap for Windows when children die
+
+ // After collecting zombie ports, get the next available
+ GDBRemoteCommunicationServerPlatform::PortMap portmap_for_child;
llvm::Expected available_port =
gdbserver_portmap.GetNextAvailablePort();
if (available_port)
-port = *available_port;
-
+portmap_for_child.AllowPort(*available_port);
else {
-fprintf(stderr, "no available port for connection - dropping...\n");
+fprintf(stderr,
+"no available gdbserver port for connection - dropping...\n");
delete conn;
continue;
}
+ platform.SetPortMap(std::move(portmap_for_child));
+
auto childPid = fork();
if (childPid) {
gdbserver_portmap.AssociatePortWithProcess(*available_port, childPid);
@@ -334,11 +338,11 @@ int main_platfo
[Lldb-commits] [lldb] [lldb] Unify CalculateMD5 return types (PR #90921)
Awfa wrote: > Thanks! LGTM. Cool - can you merge this for me? I don't have write access. https://github.com/llvm/llvm-project/pull/90921 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Have lldb-server assign ports to children in platform mode (PR #88845)
https://github.com/Awfa updated https://github.com/llvm/llvm-project/pull/88845
>From 3d75f42b5f61ea126001919491aa09ebd15ba9f8 Mon Sep 17 00:00:00 2001
From: Anthony Ha
Date: Mon, 15 Apr 2024 19:36:34 +
Subject: [PATCH 1/4] [lldb] Have lldb-server assign ports to children in
platform mode
---
lldb/tools/lldb-server/lldb-platform.cpp | 41
1 file changed, 28 insertions(+), 13 deletions(-)
diff --git a/lldb/tools/lldb-server/lldb-platform.cpp
b/lldb/tools/lldb-server/lldb-platform.cpp
index 3e126584eb25b4..384709ba79b656 100644
--- a/lldb/tools/lldb-server/lldb-platform.cpp
+++ b/lldb/tools/lldb-server/lldb-platform.cpp
@@ -282,17 +282,10 @@ int main_platform(int argc, char *argv[]) {
}
}
- do {
-GDBRemoteCommunicationServerPlatform platform(
-acceptor_up->GetSocketProtocol(), acceptor_up->GetSocketScheme());
-
-if (port_offset > 0)
- platform.SetPortOffset(port_offset);
-
-if (!gdbserver_portmap.empty()) {
- platform.SetPortMap(std::move(gdbserver_portmap));
-}
+ GDBRemoteCommunicationServerPlatform platform(
+ acceptor_up->GetSocketProtocol(), acceptor_up->GetSocketScheme());
+ do {
const bool children_inherit_accept_socket = true;
Connection *conn = nullptr;
error = acceptor_up->Accept(children_inherit_accept_socket, conn);
@@ -301,13 +294,31 @@ int main_platform(int argc, char *argv[]) {
exit(socket_error);
}
printf("Connection established.\n");
+
+std::optional port = 0;
if (g_server) {
// Collect child zombie processes.
#if !defined(_WIN32)
- while (waitpid(-1, nullptr, WNOHANG) > 0)
-;
+ auto waitResult = waitpid(-1, nullptr, WNOHANG);
+ while (waitResult > 0) {
+// waitResult is the child pid
+gdbserver_portmap.FreePortForProcess(waitResult);
+waitResult = waitpid(-1, nullptr, WNOHANG);
+ }
#endif
- if (fork()) {
+ llvm::Expected available_port =
+ gdbserver_portmap.GetNextAvailablePort();
+ if (available_port)
+port = *available_port;
+
+ else {
+fprintf(stderr, "no available port for connection - dropping...\n");
+delete conn;
+continue;
+ }
+ auto childPid = fork();
+ if (childPid) {
+gdbserver_portmap.AssociatePortWithProcess(*available_port, childPid);
// Parent doesn't need a connection to the lldb client
delete conn;
@@ -324,6 +335,10 @@ int main_platform(int argc, char *argv[]) {
// connections while a connection is active.
acceptor_up.reset();
}
+
+GDBRemoteCommunicationServerPlatform::PortMap portmap_for_child;
+portmap_for_child.AllowPort(*port);
+platform.SetPortMap(std::move(portmap_for_child));
platform.SetConnection(std::unique_ptr(conn));
if (platform.IsConnected()) {
>From 27782fb03b3705e9ff4a5b3cc0d17c8448919be9 Mon Sep 17 00:00:00 2001
From: Anthony Ha
Date: Wed, 17 Apr 2024 00:46:58 +
Subject: [PATCH 2/4] amend! [lldb] Have lldb-server assign ports to children
in platform mode
[lldb] Have lldb-server assign ports to children in platform mode
---
lldb/tools/lldb-server/lldb-platform.cpp | 24 ++--
1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/lldb/tools/lldb-server/lldb-platform.cpp
b/lldb/tools/lldb-server/lldb-platform.cpp
index 384709ba79b656..ff91a3318cf0d5 100644
--- a/lldb/tools/lldb-server/lldb-platform.cpp
+++ b/lldb/tools/lldb-server/lldb-platform.cpp
@@ -295,27 +295,31 @@ int main_platform(int argc, char *argv[]) {
}
printf("Connection established.\n");
-std::optional port = 0;
if (g_server) {
// Collect child zombie processes.
#if !defined(_WIN32)
- auto waitResult = waitpid(-1, nullptr, WNOHANG);
- while (waitResult > 0) {
+ ::pid_t waitResult;
+ while ((waitResult = waitpid(-1, nullptr, WNOHANG)) > 0) {
// waitResult is the child pid
gdbserver_portmap.FreePortForProcess(waitResult);
-waitResult = waitpid(-1, nullptr, WNOHANG);
}
#endif
+ // TODO: Clean up portmap for Windows when children die
+
+ // After collecting zombie ports, get the next available
+ GDBRemoteCommunicationServerPlatform::PortMap portmap_for_child;
llvm::Expected available_port =
gdbserver_portmap.GetNextAvailablePort();
if (available_port)
-port = *available_port;
-
+portmap_for_child.AllowPort(*available_port);
else {
-fprintf(stderr, "no available port for connection - dropping...\n");
+fprintf(stderr,
+"no available gdbserver port for connection - dropping...\n");
delete conn;
continue;
}
+ platform.SetPortMap(std::move(portmap_for_child));
+
auto childPid = fork();
if (childPid) {
gdbserver_portmap.AssociatePortWithProcess(*available_port, childPid);
@@ -334,11 +338,11 @@ int main_platfo
[Lldb-commits] [lldb] [lldb] Have lldb-server assign ports to children in platform mode (PR #88845)
Awfa wrote: > > And make the note at the end of lldb/docs/use/qemu-testing.rst outdated. > > Removing this is the last thing to do here. Fixed! If this looks good to you, would you also be able to merge it? I don't have write permissions. https://github.com/llvm/llvm-project/pull/88845 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Unify CalculateMD5 return types (PR #90921)
Awfa wrote: @JDevlieghere sorry for the churn - can I get a revert? BuildBot is showing me that I didn't check for compile errors in the gdb platform tests. After revert, I'll go and push a new PR to fix. https://github.com/llvm/llvm-project/pull/90921 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Have lldb-server assign ports to children in platform mode (PR #88845)
Awfa wrote: Is there a way to invoke BuildBot on this branch before it's merged in by the way? I got bit on another PR where it caught some failures I wished were caught before merge. https://github.com/llvm/llvm-project/pull/88845 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Unify CalculateMD5 return types (PR #91029)
https://github.com/Awfa created https://github.com/llvm/llvm-project/pull/91029
This is a retake of https://github.com/llvm/llvm-project/pull/90921 which got
reverted because I forgot to modify the CalculateMD5 unit test I had added in
https://github.com/llvm/llvm-project/pull/88812
The prior failing build is here:
https://lab.llvm.org/buildbot/#/builders/68/builds/73622
To make sure this error doesn't happen, I ran `ninja ProcessGdbRemoteTests` and
then executed the resulting test binary and observed the `CalculateMD5` test
passed.
# Overview
In my previous PR: https://github.com/llvm/llvm-project/pull/88812,
@JDevlieghere suggested to match return types of the various calculate md5
functions.
This PR achieves that by changing the various calculate md5 functions to return
`llvm::ErrorOr`.
The suggestion was to go for `std::optional<>` but I opted for
`llvm::ErrorOr<>` because local calculate md5 was already possibly returning
`ErrorOr`.
To make sure I didn't break the md5 calculation functionality, I ran some tests
for the gdb remote client, and things seem to work.
# Testing
1. Remote file doesn't exist
1. Remote file differs
1. Remote file matches
## Test gaps
Unfortunately, I had to modify
`lldb/source/Plugins/Platform/MacOSX/PlatformDarwinDevice.cpp` and I can't test
the changes there. Hopefully, the existing test suite / code review from
whomever is reading this will catch any issues.
>From 3112e3da309a24001aa610b15f2a23892b660fc8 Mon Sep 17 00:00:00 2001
From: Anthony Ha
Date: Thu, 2 May 2024 23:55:47 +
Subject: [PATCH 1/2] Unify CalculateMD5 return types
---
lldb/include/lldb/Target/Platform.h | 4 +--
.../include/lldb/Target/RemoteAwarePlatform.h | 4 +--
.../Platform/MacOSX/PlatformDarwinDevice.cpp | 16 +
.../gdb-server/PlatformRemoteGDBServer.cpp| 8 ++---
.../gdb-server/PlatformRemoteGDBServer.h | 4 +--
.../GDBRemoteCommunicationClient.cpp | 30 ++--
.../gdb-remote/GDBRemoteCommunicationClient.h | 2 +-
lldb/source/Target/Platform.cpp | 36 +--
lldb/source/Target/RemoteAwarePlatform.cpp| 8 ++---
9 files changed, 60 insertions(+), 52 deletions(-)
diff --git a/lldb/include/lldb/Target/Platform.h
b/lldb/include/lldb/Target/Platform.h
index ad9c9dcbe684ba..e05c79cb501bf0 100644
--- a/lldb/include/lldb/Target/Platform.h
+++ b/lldb/include/lldb/Target/Platform.h
@@ -649,8 +649,8 @@ class Platform : public PluginInterface {
virtual std::string GetPlatformSpecificConnectionInformation() { return ""; }
- virtual bool CalculateMD5(const FileSpec &file_spec, uint64_t &low,
-uint64_t &high);
+ virtual llvm::ErrorOr
+ CalculateMD5(const FileSpec &file_spec);
virtual uint32_t GetResumeCountForLaunchInfo(ProcessLaunchInfo &launch_info)
{
return 1;
diff --git a/lldb/include/lldb/Target/RemoteAwarePlatform.h
b/lldb/include/lldb/Target/RemoteAwarePlatform.h
index d183815e1c8b07..0b9d79f9ff0380 100644
--- a/lldb/include/lldb/Target/RemoteAwarePlatform.h
+++ b/lldb/include/lldb/Target/RemoteAwarePlatform.h
@@ -58,8 +58,8 @@ class RemoteAwarePlatform : public Platform {
Status SetFilePermissions(const FileSpec &file_spec,
uint32_t file_permissions) override;
- bool CalculateMD5(const FileSpec &file_spec, uint64_t &low,
-uint64_t &high) override;
+ llvm::ErrorOr
+ CalculateMD5(const FileSpec &file_spec) override;
Status GetFileWithUUID(const FileSpec &platform_file, const UUID *uuid,
FileSpec &local_file) override;
diff --git a/lldb/source/Plugins/Platform/MacOSX/PlatformDarwinDevice.cpp
b/lldb/source/Plugins/Platform/MacOSX/PlatformDarwinDevice.cpp
index 52777909a1f825..82156aca8cf159 100644
--- a/lldb/source/Plugins/Platform/MacOSX/PlatformDarwinDevice.cpp
+++ b/lldb/source/Plugins/Platform/MacOSX/PlatformDarwinDevice.cpp
@@ -405,17 +405,21 @@ lldb_private::Status
PlatformDarwinDevice::GetSharedModuleWithLocalCache(
// when going over the *slow* GDB remote transfer mechanism we first
// check the hashes of the files - and only do the actual transfer if
// they differ
- uint64_t high_local, high_remote, low_local, low_remote;
auto MD5 = llvm::sys::fs::md5_contents(module_cache_spec.GetPath());
if (!MD5)
return Status(MD5.getError());
- std::tie(high_local, low_local) = MD5->words();
- m_remote_platform_sp->CalculateMD5(module_spec.GetFileSpec(),
- low_remote, high_remote);
- if (low_local != low_remote || high_loc
[Lldb-commits] [lldb] [lldb] Unify CalculateMD5 return types (PR #91029)
Awfa wrote: @JDevlieghere , do you know if there's a way to run buildbot on a merge of this PR and main branch - just to validate the build/tests work before this merge? The [llvm contributing guide](https://llvm.org/docs/MyFirstTypoFix.html#myfirsttypofix-issues-after-landing-your-pr) says this is normal workflow - but I worry since I am using a lot of the maintainers' time to merge / revert the changes for me since I don't have write access. https://github.com/llvm/llvm-project/pull/91029 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Unify CalculateMD5 return types (PR #91029)
https://github.com/Awfa updated https://github.com/llvm/llvm-project/pull/91029
>From 3112e3da309a24001aa610b15f2a23892b660fc8 Mon Sep 17 00:00:00 2001
From: Anthony Ha
Date: Thu, 2 May 2024 23:55:47 +
Subject: [PATCH 1/3] Unify CalculateMD5 return types
---
lldb/include/lldb/Target/Platform.h | 4 +--
.../include/lldb/Target/RemoteAwarePlatform.h | 4 +--
.../Platform/MacOSX/PlatformDarwinDevice.cpp | 16 +
.../gdb-server/PlatformRemoteGDBServer.cpp| 8 ++---
.../gdb-server/PlatformRemoteGDBServer.h | 4 +--
.../GDBRemoteCommunicationClient.cpp | 30 ++--
.../gdb-remote/GDBRemoteCommunicationClient.h | 2 +-
lldb/source/Target/Platform.cpp | 36 +--
lldb/source/Target/RemoteAwarePlatform.cpp| 8 ++---
9 files changed, 60 insertions(+), 52 deletions(-)
diff --git a/lldb/include/lldb/Target/Platform.h
b/lldb/include/lldb/Target/Platform.h
index ad9c9dcbe684ba..e05c79cb501bf0 100644
--- a/lldb/include/lldb/Target/Platform.h
+++ b/lldb/include/lldb/Target/Platform.h
@@ -649,8 +649,8 @@ class Platform : public PluginInterface {
virtual std::string GetPlatformSpecificConnectionInformation() { return ""; }
- virtual bool CalculateMD5(const FileSpec &file_spec, uint64_t &low,
-uint64_t &high);
+ virtual llvm::ErrorOr
+ CalculateMD5(const FileSpec &file_spec);
virtual uint32_t GetResumeCountForLaunchInfo(ProcessLaunchInfo &launch_info)
{
return 1;
diff --git a/lldb/include/lldb/Target/RemoteAwarePlatform.h
b/lldb/include/lldb/Target/RemoteAwarePlatform.h
index d183815e1c8b07..0b9d79f9ff0380 100644
--- a/lldb/include/lldb/Target/RemoteAwarePlatform.h
+++ b/lldb/include/lldb/Target/RemoteAwarePlatform.h
@@ -58,8 +58,8 @@ class RemoteAwarePlatform : public Platform {
Status SetFilePermissions(const FileSpec &file_spec,
uint32_t file_permissions) override;
- bool CalculateMD5(const FileSpec &file_spec, uint64_t &low,
-uint64_t &high) override;
+ llvm::ErrorOr
+ CalculateMD5(const FileSpec &file_spec) override;
Status GetFileWithUUID(const FileSpec &platform_file, const UUID *uuid,
FileSpec &local_file) override;
diff --git a/lldb/source/Plugins/Platform/MacOSX/PlatformDarwinDevice.cpp
b/lldb/source/Plugins/Platform/MacOSX/PlatformDarwinDevice.cpp
index 52777909a1f825..82156aca8cf159 100644
--- a/lldb/source/Plugins/Platform/MacOSX/PlatformDarwinDevice.cpp
+++ b/lldb/source/Plugins/Platform/MacOSX/PlatformDarwinDevice.cpp
@@ -405,17 +405,21 @@ lldb_private::Status
PlatformDarwinDevice::GetSharedModuleWithLocalCache(
// when going over the *slow* GDB remote transfer mechanism we first
// check the hashes of the files - and only do the actual transfer if
// they differ
- uint64_t high_local, high_remote, low_local, low_remote;
auto MD5 = llvm::sys::fs::md5_contents(module_cache_spec.GetPath());
if (!MD5)
return Status(MD5.getError());
- std::tie(high_local, low_local) = MD5->words();
- m_remote_platform_sp->CalculateMD5(module_spec.GetFileSpec(),
- low_remote, high_remote);
- if (low_local != low_remote || high_local != high_remote) {
+ Log *log = GetLog(LLDBLog::Platform);
+ bool requires_transfer = true;
+ llvm::ErrorOr remote_md5 =
+ m_remote_platform_sp->CalculateMD5(module_spec.GetFileSpec());
+ if (std::error_code ec = remote_md5.getError())
+LLDB_LOG(log, "couldn't get md5 sum from remote: {0}",
+ ec.message());
+ else
+requires_transfer = *MD5 != *remote_md5;
+ if (requires_transfer) {
// bring in the remote file
-Log *log = GetLog(LLDBLog::Platform);
LLDB_LOGF(log,
"[%s] module %s/%s needs to be replaced from remote
copy",
(IsHost() ? "host" : "remote"),
diff --git
a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
index 0dce5add2e3759..4684947ede209f 100644
--- a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
+++ b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
@@ -684,12 +684,12 @@ Status PlatformRemoteGDBServer::RunShellCommand(
signo_ptr, command_output, timeout);
}
-bool PlatformRemoteGDBServer::CalculateMD5(const FileSpec &file_spec,
- uint64_t &low, uint64_t &high) {
+llvm::ErrorOr
+PlatformRemoteGDBServer::CalculateMD5(const FileSpec &file_spec) {
if (!IsConnected())
-return false;
+return std::make_error_code(std::errc::not_connected);
- return m_gdb_client_up->CalculateMD5(file_spec, low, high);
+
[Lldb-commits] [lldb] [lldb] Unify CalculateMD5 return types (PR #91029)
Awfa wrote: > ⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️ > > You can test this locally with the following command: > View the diff from clang-format here. This formatter alert is outdated - by the time this arrived, my PR had the issue fixed already. https://github.com/llvm/llvm-project/pull/91029 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Unify CalculateMD5 return types (PR #91029)
Awfa wrote: > > @JDevlieghere , do you know if there's a way to run buildbot on a merge of > > this PR and main branch - just to validate the build/tests work before this > > merge? > > Not that I know. When failures are macOS specific I'm happy to apply a PR > locally and run the test suite and other folks with specific configurations > might do the same, but that's about all I can think of. > > > The [llvm contributing > > guide](https://llvm.org/docs/MyFirstTypoFix.html#myfirsttypofix-issues-after-landing-your-pr) > > says this is normal workflow - but I worry since I am using a lot of the > > maintainers' time to merge / revert the changes for me since I don't have > > write access. > > No worries, it's totally normal and we're happy to help with that! Can I ask you merge this for me? Hopefully no reverts this time :) https://github.com/llvm/llvm-project/pull/91029 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
