[lldb] Unify CalculateMD5 return types #90921
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
|
@llvm/pr-subscribers-lldb Author: Anthony Ha (Awfa) ChangesOverviewIn my previous PR: #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 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
Test gapsUnfortunately, I had to modify Full diff: https://github.com/llvm/llvm-project/pull/90921.diff 9 Files Affected:
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<llvm::MD5::MD5Result>
+ 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<llvm::MD5::MD5Result>
+ 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<llvm::MD5::MD5Result> 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<llvm::MD5::MD5Result>
+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);
+ return m_gdb_client_up->CalculateMD5(file_spec);
}
void PlatformRemoteGDBServer::CalculateTrapHandlerSymbolNames() {
diff --git a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
index d83fc386f59427..0ae1f3cb4199cf 100644
--- a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
+++ b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
@@ -146,8 +146,8 @@ class PlatformRemoteGDBServer : public Platform, private UserIDResolver {
void CalculateTrapHandlerSymbolNames() override;
- bool CalculateMD5(const FileSpec &file_spec, uint64_t &low,
- uint64_t &high) override;
+ llvm::ErrorOr<llvm::MD5::MD5Result>
+ CalculateMD5(const FileSpec &file_spec) override;
const lldb::UnixSignalsSP &GetRemoteUnixSignals() override;
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
index 7498a070c26094..db9fb37a9a3c30 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -3418,8 +3418,8 @@ bool GDBRemoteCommunicationClient::GetFileExists(
return true;
}
-bool GDBRemoteCommunicationClient::CalculateMD5(
- const lldb_private::FileSpec &file_spec, uint64_t &low, uint64_t &high) {
+llvm::ErrorOr<llvm::MD5::MD5Result> GDBRemoteCommunicationClient::CalculateMD5(
+ const lldb_private::FileSpec &file_spec) {
std::string path(file_spec.GetPath(false));
lldb_private::StreamString stream;
stream.PutCString("vFile:MD5:");
@@ -3428,11 +3428,11 @@ bool GDBRemoteCommunicationClient::CalculateMD5(
if (SendPacketAndWaitForResponse(stream.GetString(), response) ==
PacketResult::Success) {
if (response.GetChar() != 'F')
- return false;
+ return std::make_error_code(std::errc::illegal_byte_sequence);
if (response.GetChar() != ',')
- return false;
+ return std::make_error_code(std::errc::illegal_byte_sequence);
if (response.Peek() && *response.Peek() == 'x')
- return false;
+ return std::make_error_code(std::errc::no_such_file_or_directory);
// GDBRemoteCommunicationServerCommon::Handle_vFile_MD5 concatenates low and
// high hex strings. We can't use response.GetHexMaxU64 because that can't
@@ -3455,25 +3455,33 @@ bool GDBRemoteCommunicationClient::CalculateMD5(
auto part =
response.GetStringRef().substr(response.GetFilePos(), MD5_HALF_LENGTH);
if (part.size() != MD5_HALF_LENGTH)
- return false;
+ return std::make_error_code(std::errc::illegal_byte_sequence);
response.SetFilePos(response.GetFilePos() + part.size());
+ uint64_t low;
if (part.getAsInteger(/*radix=*/16, low))
- return false;
+ return std::make_error_code(std::errc::illegal_byte_sequence);
// Get high part
part =
response.GetStringRef().substr(response.GetFilePos(), MD5_HALF_LENGTH);
if (part.size() != MD5_HALF_LENGTH)
- return false;
+ return std::make_error_code(std::errc::illegal_byte_sequence);
response.SetFilePos(response.GetFilePos() + part.size());
+ uint64_t high;
if (part.getAsInteger(/*radix=*/16, high))
- return false;
+ return std::make_error_code(std::errc::illegal_byte_sequence);
- return true;
+ llvm::MD5::MD5Result result;
+ llvm::support::endian::write<uint64_t, llvm::endianness::little>(
+ result.data(), low);
+ llvm::support::endian::write<uint64_t, llvm::endianness::little>(
+ result.data() + 8, high);
+
+ return result;
}
- return false;
+ return std::make_error_code(std::errc::operation_canceled);
}
bool GDBRemoteCommunicationClient::AvoidGPackets(ProcessGDBRemote *process) {
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
index 4be7eb00f42b97..898d176abc3465 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
@@ -392,7 +392,7 @@ class GDBRemoteCommunicationClient : public GDBRemoteClientBase {
*command_output, // Pass nullptr if you don't want the command output
const Timeout<std::micro> &timeout);
- bool CalculateMD5(const FileSpec &file_spec, uint64_t &low, uint64_t &high);
+ llvm::ErrorOr<llvm::MD5::MD5Result> CalculateMD5(const FileSpec &file_spec);
lldb::DataBufferSP ReadRegister(
lldb::tid_t tid,
diff --git a/lldb/source/Target/Platform.cpp b/lldb/source/Target/Platform.cpp
index 91483ba008f4a3..4af4aa68ccd01c 100644
--- a/lldb/source/Target/Platform.cpp
+++ b/lldb/source/Target/Platform.cpp
@@ -1199,22 +1199,22 @@ Status Platform::PutFile(const FileSpec &source, const FileSpec &destination,
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");
+ llvm::ErrorOr<llvm::MD5::MD5Result> remote_md5 = CalculateMD5(destination);
+ if (std::error_code ec = remote_md5.getError()) {
+ LLDB_LOG(log, "[PutFile] couldn't get md5 sum of destination: {0}",
+ ec.message());
} 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");
+ llvm::ErrorOr<llvm::MD5::MD5Result> local_md5 =
+ llvm::sys::fs::md5_contents(source.GetPath());
+ if (std::error_code ec = local_md5.getError()) {
+ LLDB_LOG(log, "[PutFile] couldn't get md5 sum of source: {0}",
+ ec.message());
} else {
- const auto [local_md5_high, local_md5_low] = local_md5->words();
LLDB_LOGF(log, "[PutFile] destination md5: %016" PRIx64 "%016" PRIx64,
- dest_md5_high, dest_md5_low);
+ remote_md5->high(), remote_md5->low());
LLDB_LOGF(log, "[PutFile] local md5: %016" PRIx64 "%016" PRIx64,
- local_md5_high, local_md5_low);
- requires_upload =
- local_md5_high != dest_md5_high || local_md5_low != dest_md5_low;
+ local_md5->high(), local_md5->low());
+ requires_upload = *remote_md5 != *local_md5;
}
}
@@ -1339,15 +1339,11 @@ lldb_private::Status Platform::RunShellCommand(
return Status("unable to run a remote command without a platform");
}
-bool Platform::CalculateMD5(const FileSpec &file_spec, uint64_t &low,
- uint64_t &high) {
+llvm::ErrorOr<llvm::MD5::MD5Result>
+Platform::CalculateMD5(const FileSpec &file_spec) {
if (!IsHost())
- return false;
- auto Result = llvm::sys::fs::md5_contents(file_spec.GetPath());
- if (!Result)
- return false;
- std::tie(high, low) = Result->words();
- return true;
+ return std::make_error_code(std::errc::not_supported);
+ return llvm::sys::fs::md5_contents(file_spec.GetPath());
}
void Platform::SetLocalCacheDirectory(const char *local) {
diff --git a/lldb/source/Target/RemoteAwarePlatform.cpp b/lldb/source/Target/RemoteAwarePlatform.cpp
index 0bd6c9251c858a..9a41a423cadd3b 100644
--- a/lldb/source/Target/RemoteAwarePlatform.cpp
+++ b/lldb/source/Target/RemoteAwarePlatform.cpp
@@ -266,11 +266,11 @@ Status RemoteAwarePlatform::Unlink(const FileSpec &file_spec) {
return Platform::Unlink(file_spec);
}
-bool RemoteAwarePlatform::CalculateMD5(const FileSpec &file_spec, uint64_t &low,
- uint64_t &high) {
+llvm::ErrorOr<llvm::MD5::MD5Result>
+RemoteAwarePlatform::CalculateMD5(const FileSpec &file_spec) {
if (m_remote_platform_sp)
- return m_remote_platform_sp->CalculateMD5(file_spec, low, high);
- return Platform::CalculateMD5(file_spec, low, high);
+ return m_remote_platform_sp->CalculateMD5(file_spec);
+ return Platform::CalculateMD5(file_spec);
}
FileSpec RemoteAwarePlatform::GetRemoteWorkingDirectory() {
|
Cool - can you merge this for me? I don't have write access. |
|
@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. |
JDevlieghere
added a commit
that referenced
this pull request
May 3, 2024
This reverts commit 2f58b9a.
JDevlieghere
added a commit
that referenced
this pull request
May 3, 2024
JDevlieghere
pushed a commit
that referenced
this pull request
May 9, 2024
This is a retake of #90921 which got reverted because I forgot to modify the CalculateMD5 unit test I had added in #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: #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<llvm::MD5::MD5Result>`. 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.
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.
Overview
In my previous PR: #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<llvm::MD5::MD5Result>.The suggestion was to go for
std::optional<>but I opted forllvm::ErrorOr<>because local calculate md5 was already possibly returningErrorOr.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
Remote file doesn't exist
Remote file differs
Remote file matches
Test gaps
Unfortunately, I had to modify
lldb/source/Plugins/Platform/MacOSX/PlatformDarwinDevice.cppand I can't test the changes there. Hopefully, the existing test suite / code review from whomever is reading this will catch any issues.