[Lldb-commits] [lldb] [lldb] Unify CalculateMD5 return types (PR #91029)

Anthony Ha via lldb-commits lldb-commits at lists.llvm.org
Fri May 3 16:30:19 PDT 2024


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<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
![image](https://github.com/llvm/llvm-project/assets/1326275/b26859e2-18c3-4685-be8f-c6b6a5a4bc77)

1. Remote file differs
![image](https://github.com/llvm/llvm-project/assets/1326275/cbdb3c58-555a-401b-9444-c5ff4c04c491)

1. Remote file matches
![image](https://github.com/llvm/llvm-project/assets/1326275/07561572-22d1-4e0a-988f-bc91b5c2ffce)

## 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 <antha at microsoft.com>
Date: Thu, 2 May 2024 23:55:47 +0000
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<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() {

>From 44223e9a62d116c50b52985ab944a5e2d48de95a Mon Sep 17 00:00:00 2001
From: Anthony Ha <antha at microsoft.com>
Date: Fri, 3 May 2024 23:25:31 +0000
Subject: [PATCH 2/2] amend! Unify CalculateMD5 return types

Unify CalculateMD5 return types
---
 .../gdb-remote/GDBRemoteCommunicationClientTest.cpp  | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp b/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
index 6b11ec43a65dd8..6dcf7ef3eaaba9 100644
--- a/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
+++ b/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
@@ -595,9 +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;
@@ -607,11 +606,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());
 }



More information about the lldb-commits mailing list