Author: Anthony Ha Date: 2024-05-09T15:57:46-07:00 New Revision: 95f208f97e709139c3ecbce552bcf1e34b9fcf12
URL: https://github.com/llvm/llvm-project/commit/95f208f97e709139c3ecbce552bcf1e34b9fcf12 DIFF: https://github.com/llvm/llvm-project/commit/95f208f97e709139c3ecbce552bcf1e34b9fcf12.diff LOG: [lldb] Unify CalculateMD5 return types (#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<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. Added: Modified: lldb/include/lldb/Target/Platform.h lldb/include/lldb/Target/RemoteAwarePlatform.h lldb/source/Plugins/Platform/MacOSX/PlatformDarwinDevice.cpp lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h lldb/source/Target/Platform.cpp lldb/source/Target/RemoteAwarePlatform.cpp lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp Removed: ################################################################################ diff --git a/lldb/include/lldb/Target/Platform.h b/lldb/include/lldb/Target/Platform.h index ad9c9dcbe684b..e05c79cb501bf 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 d183815e1c8b0..0b9d79f9ff038 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 52777909a1f82..82156aca8cf15 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 diff er - 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 0dce5add2e375..4684947ede209 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 d83fc386f5942..0ae1f3cb4199c 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 7498a070c2609..db9fb37a9a3c3 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 4be7eb00f42b9..898d176abc346 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 91483ba008f4a..4af4aa68ccd01 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 0bd6c9251c858..9a41a423cadd3 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() { diff --git a/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp b/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp index 6b11ec43a65dd..24111396b0ac6 100644 --- a/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp +++ b/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp @@ -595,10 +595,8 @@ TEST_F(GDBRemoteCommunicationClientTest, WriteMemoryTags) { TEST_F(GDBRemoteCommunicationClientTest, CalculateMD5) { FileSpec file_spec("/foo/bar", FileSpec::Style::posix); - uint64_t low, high; - std::future<bool> async_result = std::async(std::launch::async, [&] { - return client.CalculateMD5(file_spec, low, high); - }); + std::future<ErrorOr<MD5::MD5Result>> async_result = std::async( + std::launch::async, [&] { return client.CalculateMD5(file_spec); }); lldb_private::StreamString stream; stream.PutCString("vFile:MD5:"); @@ -607,11 +605,12 @@ TEST_F(GDBRemoteCommunicationClientTest, CalculateMD5) { "F," "deadbeef01020304" "05060708deadbeef"); - ASSERT_TRUE(async_result.get()); + auto result = async_result.get(); // Server and client puts/parses low, and then high const uint64_t expected_low = 0xdeadbeef01020304; const uint64_t expected_high = 0x05060708deadbeef; - EXPECT_EQ(expected_low, low); - EXPECT_EQ(expected_high, high); + ASSERT_TRUE(result); + EXPECT_EQ(expected_low, result->low()); + EXPECT_EQ(expected_high, result->high()); } _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
