Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 103 additions & 0 deletions lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@
#include "lldb/Host/Host.h"
#include "lldb/Utility/StringExtractorGDBRemote.h"

#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/ScopeExit.h"
#include "llvm/ADT/StringMap.h"
#include "llvm/ADT/StringSwitch.h"
Expand Down Expand Up @@ -2762,6 +2763,108 @@ size_t ProcessGDBRemote::DoReadMemory(addr_t addr, void *buf, size_t size,
return 0;
}

llvm::SmallVector<llvm::MutableArrayRef<uint8_t>>
ProcessGDBRemote::ReadMemoryRanges(
llvm::ArrayRef<Range<lldb::addr_t, size_t>> ranges,
llvm::MutableArrayRef<uint8_t> buffer) {
if (!m_gdb_comm.GetMultiMemReadSupported())
return Process::ReadMemoryRanges(ranges, buffer);

llvm::Expected<StringExtractorGDBRemote> response =
SendMultiMemReadPacket(ranges);
if (!response) {
LLDB_LOG_ERROR(GetLog(GDBRLog::Process), response.takeError(),
"MultiMemRead error response: {0}");
return Process::ReadMemoryRanges(ranges, buffer);
}

llvm::StringRef response_str = response->GetStringRef();
const unsigned expected_num_ranges = ranges.size();
llvm::Expected<llvm::SmallVector<llvm::MutableArrayRef<uint8_t>>>
parsed_response =
ParseMultiMemReadPacket(response_str, buffer, expected_num_ranges);
if (!parsed_response) {
LLDB_LOG_ERROR(GetLog(GDBRLog::Process), parsed_response.takeError(),
"MultiMemRead error parsing response: {0}");
return Process::ReadMemoryRanges(ranges, buffer);
}
return std::move(*parsed_response);
}

llvm::Expected<StringExtractorGDBRemote>
ProcessGDBRemote::SendMultiMemReadPacket(
llvm::ArrayRef<Range<lldb::addr_t, size_t>> ranges) {
std::string packet_str;
llvm::raw_string_ostream stream(packet_str);
stream << "MultiMemRead:ranges:";

auto range_to_stream = [&](auto range) {
// the "-" marker omits the '0x' prefix.
stream << llvm::formatv("{0:x-},{1:x-}", range.base, range.size);
};
llvm::interleave(ranges, stream, range_to_stream, ",");
stream << ";";

StringExtractorGDBRemote response;
GDBRemoteCommunication::PacketResult packet_result =
m_gdb_comm.SendPacketAndWaitForResponse(packet_str.data(), response,
GetInterruptTimeout());
if (packet_result != GDBRemoteCommunication::PacketResult::Success)
return llvm::createStringError(
llvm::formatv("MultiMemRead failed to send packet: '{0}'", packet_str));

if (response.IsErrorResponse())
return llvm::createStringError(
llvm::formatv("MultiMemRead failed: '{0}'", response.GetStringRef()));

if (!response.IsNormalResponse())
return llvm::createStringError(llvm::formatv(
"MultiMemRead unexpected response: '{0}'", response.GetStringRef()));

return response;
}

llvm::Expected<llvm::SmallVector<llvm::MutableArrayRef<uint8_t>>>
ProcessGDBRemote::ParseMultiMemReadPacket(llvm::StringRef response_str,
llvm::MutableArrayRef<uint8_t> buffer,
unsigned expected_num_ranges) {
// The sizes and the data are separated by a `;`.
auto [sizes_str, memory_data] = response_str.split(';');
if (sizes_str.size() == response_str.size())
return llvm::createStringError(llvm::formatv(
"MultiMemRead response missing field separator ';' in: '{0}'",
response_str));

llvm::SmallVector<llvm::MutableArrayRef<uint8_t>> read_results;

// Sizes are separated by a `,`.
for (llvm::StringRef size_str : llvm::split(sizes_str, ',')) {
uint64_t read_size;
if (size_str.getAsInteger(16, read_size))
return llvm::createStringError(llvm::formatv(
"MultiMemRead response has invalid size string: {0}", size_str));

if (memory_data.size() < read_size)
return llvm::createStringError(
llvm::formatv("MultiMemRead response did not have enough data, "
"requested sizes: {0}",
sizes_str));

llvm::StringRef region_to_read = memory_data.take_front(read_size);
memory_data = memory_data.drop_front(read_size);

assert(buffer.size() >= read_size);
llvm::MutableArrayRef<uint8_t> region_to_write =
buffer.take_front(read_size);
buffer = buffer.drop_front(read_size);

memcpy(region_to_write.data(), region_to_read.data(), read_size);
read_results.push_back(region_to_write);
}

return read_results;
}

bool ProcessGDBRemote::SupportsMemoryTagging() {
return m_gdb_comm.GetMemoryTaggingSupported();
}
Expand Down
16 changes: 16 additions & 0 deletions lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,22 @@ class ProcessGDBRemote : public Process,
size_t DoReadMemory(lldb::addr_t addr, void *buf, size_t size,
Status &error) override;

/// Override of ReadMemoryRanges that uses MultiMemRead to optimize this
/// operation.
llvm::SmallVector<llvm::MutableArrayRef<uint8_t>>
ReadMemoryRanges(llvm::ArrayRef<Range<lldb::addr_t, size_t>> ranges,
llvm::MutableArrayRef<uint8_t> buf) override;

private:
llvm::Expected<StringExtractorGDBRemote>
SendMultiMemReadPacket(llvm::ArrayRef<Range<lldb::addr_t, size_t>> ranges);

llvm::Expected<llvm::SmallVector<llvm::MutableArrayRef<uint8_t>>>
ParseMultiMemReadPacket(llvm::StringRef response_str,
llvm::MutableArrayRef<uint8_t> buffer,
unsigned expected_num_ranges);

public:
Status
WriteObjectFile(std::vector<ObjectFile::LoadableData> entries) override;

Expand Down
27 changes: 27 additions & 0 deletions lldb/test/API/lang/objc/foundation/TestObjCMethodsNSError.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,3 +45,30 @@ def test_NSError_p(self):
],
)
self.runCmd("process continue")

@skipIfOutOfTreeDebugserver
def test_runtime_types_efficient_memreads(self):
# Test that we use an efficient reading of memory when reading
# Objective-C method descriptions.
logfile = os.path.join(self.getBuildDir(), "log.txt")
self.runCmd(f"log enable -f {logfile} gdb-remote packets process")
self.addTearDownHook(lambda: self.runCmd("log disable gdb-remote packets"))

self.build()
self.target, process, thread, bkpt = lldbutil.run_to_source_breakpoint(
self, "// Break here for NSString tests", lldb.SBFileSpec("main.m", False)
)

self.runCmd(f"proc plugin packet send StartTesting", check=False)
self.expect('expression str = [NSString stringWithCString: "new"]')
self.runCmd(f"proc plugin packet send EndTesting", check=False)

self.assertTrue(os.path.exists(logfile))
log_text = open(logfile).read()
log_text = log_text.split("StartTesting", 1)[-1].split("EndTesting", 1)[0]

# This test is only checking that the packet it used at all (and that
# no errors are produced). It doesn't check that the packet is being
# used to solve a problem in an optimal way.
self.assertIn("MultiMemRead:", log_text)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also assert that it is in there one or N times whatever is correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sadly I don't think we can do this, as it is very dependent on debug info for core objective C libraries. I wish we had a more direct way of testing things, Jason and I could not come up with anything better

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we had a command that exclusively could be completed with only a MultiMemRead, we might be able to assert that no x/m packets were sent. But this core operation will still do memory reads to get c-strings for the methods; we didn't tackle Process::MultiCStringRead, but even if we had there would be a handful of simple memory reads issued while evaluating the expression. I think confirming the presence of at least one MultiMemRead: packet may be the best we could do here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very dependent on debug info for core objective C libraries

Understood. Then add a comment to that effect.

This makes it clear the test is not checking whether the underlying layers batch reads in some optimal way. Just that it uses the new thing at all.

I want to have a look around lldb for a non-Darwin use for this packet, might be a case where we can expect only one. I'm fine with these tests as they are though.

self.assertNotIn("MultiMemRead error", log_text)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that text appear if there's an error? Won't it just be an Exx reply code from debugserver?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh i should add a comment about this, the sneaky thing is that I also enabled the gdbremote process log

Loading