Skip to content

Commit

Permalink
[lldb] Unify CalculateMD5 return types (#90921)
Browse files Browse the repository at this point in the history
# 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

![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.

Co-authored-by: Anthony Ha <antha@microsoft.com>
  • Loading branch information
Awfa and Awfa committed May 3, 2024
1 parent f8a9973 commit 2f58b9a
Show file tree
Hide file tree
Showing 9 changed files with 60 additions and 52 deletions.
4 changes: 2 additions & 2 deletions lldb/include/lldb/Target/Platform.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
4 changes: 2 additions & 2 deletions lldb/include/lldb/Target/RemoteAwarePlatform.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
16 changes: 10 additions & 6 deletions lldb/source/Plugins/Platform/MacOSX/PlatformDarwinDevice.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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:");
Expand All @@ -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
Expand All @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
36 changes: 16 additions & 20 deletions lldb/source/Target/Platform.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}

Expand Down Expand Up @@ -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) {
Expand Down
8 changes: 4 additions & 4 deletions lldb/source/Target/RemoteAwarePlatform.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down

0 comments on commit 2f58b9a

Please sign in to comment.