[lldb] Fix and speedup the memory find command
#104193
Conversation
|
✅ With the latest revision this PR passed the Python code formatter. |
nikic
changed the title
labath
changed the title
memory find command
|
@llvm/pr-subscribers-lldb Author: Pavel Labath (labath) ChangesThis patch fixes an issue where the To make matters worse, it would not terminate after encountering this The patch fixes this first problem (*) by detecting read errors and (*) The fix does not work on windows yet, because the ReadMemory API Full diff: https://github.com/llvm/llvm-project/pull/104193.diff 3 Files Affected:
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index b2a0f13b9a1549..5d066d264bd3f5 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -114,33 +114,6 @@ class ProcessOptionValueProperties
}
};
-class ProcessMemoryIterator {
-public:
- ProcessMemoryIterator(Process &process, lldb::addr_t base)
- : m_process(process), m_base_addr(base) {}
-
- bool IsValid() { return m_is_valid; }
-
- uint8_t operator[](lldb::addr_t offset) {
- if (!IsValid())
- return 0;
-
- uint8_t retval = 0;
- Status error;
- if (0 == m_process.ReadMemory(m_base_addr + offset, &retval, 1, error)) {
- m_is_valid = false;
- return 0;
- }
-
- return retval;
- }
-
-private:
- Process &m_process;
- const lldb::addr_t m_base_addr;
- bool m_is_valid = true;
-};
-
static constexpr OptionEnumValueElement g_follow_fork_mode_values[] = {
{
eFollowParent,
@@ -3367,21 +3340,48 @@ lldb::addr_t Process::FindInMemory(lldb::addr_t low, lldb::addr_t high,
if (region_size < size)
return LLDB_INVALID_ADDRESS;
+ // See "Boyer-Moore string search algorithm".
std::vector<size_t> bad_char_heuristic(256, size);
- ProcessMemoryIterator iterator(*this, low);
-
for (size_t idx = 0; idx < size - 1; idx++) {
decltype(bad_char_heuristic)::size_type bcu_idx = buf[idx];
bad_char_heuristic[bcu_idx] = size - idx - 1;
}
- for (size_t s = 0; s <= (region_size - size);) {
+
+ // Memory we're currently searching through.
+ llvm::SmallVector<uint8_t, 0> mem;
+ // Position of the memory buffer.
+ addr_t mem_pos = low;
+ // Maximum number of bytes read (and buffered). We need to read at least
+ // `size` bytes for a successful match.
+ const size_t max_read_size = std::max<size_t>(size, 0x10000);
+
+ for (addr_t s = low; s <= (high - size);) {
+ if (s + size > mem.size() + mem_pos) {
+ // We need to read more data. We don't attempt to reuse the data we've
+ // already read (up to `size-1` bytes from `s` to `mem_pos+mem.size()`).
+ // This is fine for patterns much smaller than max_read_size. For very
+ // long patterns we may need to do something more elaborate.
+ mem.resize_for_overwrite(max_read_size);
+ Status error;
+ mem.resize(
+ ReadMemory(s, mem.data(), std::min(mem.size(), high - s), error));
+ mem_pos = s;
+ if (error.Fail() || size > mem.size()) {
+ // We didn't read enough data. Skip to the next memory region.
+ MemoryRegionInfo info;
+ error = GetMemoryRegionInfo(mem_pos + mem.size(), info);
+ if (error.Fail())
+ break;
+ s = info.GetRange().GetRangeEnd();
+ continue;
+ }
+ }
int64_t j = size - 1;
- while (j >= 0 && buf[j] == iterator[s + j])
+ while (j >= 0 && buf[j] == mem[s + j - mem_pos])
j--;
if (j < 0)
- return low + s;
- else
- s += bad_char_heuristic[iterator[s + size - 1]];
+ return s; // We have a match.
+ s += bad_char_heuristic[mem[s + size - 1 - mem_pos]];
}
return LLDB_INVALID_ADDRESS;
diff --git a/lldb/test/API/functionalities/memory/find/TestMemoryFind.py b/lldb/test/API/functionalities/memory/find/TestMemoryFind.py
index 09611cc808777d..72acfb3d600701 100644
--- a/lldb/test/API/functionalities/memory/find/TestMemoryFind.py
+++ b/lldb/test/API/functionalities/memory/find/TestMemoryFind.py
@@ -10,14 +10,16 @@
class MemoryFindTestCase(TestBase):
+
+ NO_DEBUG_INFO_TESTCASE = True
+
def setUp(self):
# Call super's setUp().
TestBase.setUp(self)
# Find the line number to break inside main().
self.line = line_number("main.cpp", "// break here")
- def test_memory_find(self):
- """Test the 'memory find' command."""
+ def _prepare_inferior(self):
self.build()
exe = self.getBuildArtifact("a.out")
self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
@@ -39,7 +41,10 @@ def test_memory_find(self):
# The breakpoint should have a hit count of 1.
lldbutil.check_breakpoint(self, bpno=1, expected_hit_count=1)
- # Test the memory find commands.
+ def test_memory_find(self):
+ """Test the 'memory find' command."""
+
+ self._prepare_inferior()
# Empty search string should be handled.
self.expect(
@@ -79,3 +84,22 @@ def test_memory_find(self):
'memory find -s "nothere" `stringdata` `stringdata+10`',
substrs=["data not found within the range."],
)
+
+ @expectedFailureWindows
+ def test_memory_find_with_holes(self):
+ self._prepare_inferior()
+
+ pagesize = self.frame().FindVariable("pagesize").GetValueAsUnsigned()
+ mem_with_holes = (
+ self.frame().FindVariable("mem_with_holes").GetValueAsUnsigned()
+ )
+ matches_var = self.frame().FindVariable("matches")
+ self.assertEqual(matches_var.GetNumChildren(), 4)
+ matches = [
+ f"data found at location: {matches_var.GetChildAtIndex(i).GetValueAsUnsigned():#x}"
+ for i in range(4)
+ ]
+ self.expect(
+ 'memory find -c 5 -s "needle" `mem_with_holes` `mem_with_holes+5*pagesize`',
+ substrs=matches + ["no more matches within the range"],
+ )
diff --git a/lldb/test/API/functionalities/memory/find/main.cpp b/lldb/test/API/functionalities/memory/find/main.cpp
index e3dcfc762ee0f9..e752c583d70f61 100644
--- a/lldb/test/API/functionalities/memory/find/main.cpp
+++ b/lldb/test/API/functionalities/memory/find/main.cpp
@@ -1,9 +1,76 @@
-#include <stdio.h>
-#include <stdint.h>
-
-int main (int argc, char const *argv[])
-{
- const char* stringdata = "hello world; I like to write text in const char pointers";
- uint8_t bytedata[] = {0xAA,0xBB,0xCC,0xDD,0xEE,0xFF,0x00,0x11,0x22,0x33,0x44,0x55,0x66,0x77,0x88,0x99};
- return 0; // break here
+#include <cstdint>
+#include <cstdio>
+#include <cstdlib>
+#include <cstring>
+#include <initializer_list>
+#include <iostream>
+
+#ifdef _WIN32
+#include "Windows.h"
+
+int getpagesize() {
+ SYSTEM_INFO system_info;
+ GetSystemInfo(&system_info);
+ return system_info.dwPageSize;
+}
+
+char *allocate_memory_with_holes() {
+ int pagesize = getpagesize();
+ void *mem = VirtualAlloc(nullptr, 5 * pagesize, MEM_RESERVE, PAGE_NOACCESS);
+ if (!mem) {
+ std::cerr << std::system_category().message(GetLastError()) << std::endl;
+ exit(1);
+ }
+ char *bytes = static_cast<char *>(mem);
+ for (int page : {0, 2, 4}) {
+ if (!VirtualAlloc(bytes + page * pagesize, pagesize, MEM_COMMIT,
+ PAGE_READWRITE)) {
+ std::cerr << std::system_category().message(GetLastError()) << std::endl;
+ exit(1);
+ }
+ }
+ return bytes;
+}
+#else
+#include "sys/mman.h"
+#include "unistd.h"
+
+char *allocate_memory_with_holes() {
+ int pagesize = getpagesize();
+ void *mem = mmap(nullptr, 5 * pagesize, PROT_READ | PROT_WRITE,
+ MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+ if (mem == MAP_FAILED) {
+ perror("mmap");
+ exit(1);
+ }
+ char *bytes = static_cast<char *>(mem);
+ for (int page : {1, 3}) {
+ if (munmap(bytes + page * pagesize, pagesize) != 0) {
+ perror("munmap");
+ exit(1);
+ }
+ }
+ return bytes;
+}
+#endif
+
+int main(int argc, char const *argv[]) {
+ const char *stringdata =
+ "hello world; I like to write text in const char pointers";
+ uint8_t bytedata[] = {0xAA, 0xBB, 0xCC, 0xDD, 0xEE, 0xFF, 0x00, 0x11,
+ 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, 0x99};
+
+ char *mem_with_holes = allocate_memory_with_holes();
+ int pagesize = getpagesize();
+ char *matches[] = {
+ mem_with_holes, // Beginning of memory
+ mem_with_holes + 2 * pagesize, // After a hole
+ mem_with_holes + 2 * pagesize +
+ pagesize / 2, // Middle of a block, after an existing match.
+ mem_with_holes + 5 * pagesize - 7, // End of memory
+ };
+ for (char *m : matches)
+ strcpy(m, "needle");
+
+ return 0; // break here
}
|
It seems the minimal chunk is 64KB and the maximal chunk may be very long (more than 1MB). Something is wrong with the first memory block in tests Note the capacity=0 is a special case here |
| for (size_t s = 0; s <= (region_size - size);) { | ||
|
|
||
| // Memory we're currently searching through. | ||
| llvm::SmallVector<uint8_t, 0> mem; |
There was a problem hiding this comment.
If you are setting N = 0 for the llvm::SmallVector, might as well just use std::vector?
There was a problem hiding this comment.
I'm using the SmallVector for the resize_for_overwrite functionality (allocating memory without initializing it).
lldb/source/Target/Process.cpp
Outdated
| const size_t max_read_size = std::max<size_t>(size, 0x10000); | ||
|
|
||
| for (addr_t s = low; s <= (high - size);) { | ||
| if (s + size > mem.size() + mem_pos) { |
There was a problem hiding this comment.
Maybe re-order the second part so it's: address + size > other_address + other_size . Right now it flips the order for the second one.
| for i in range(4) | ||
| ] | ||
| self.expect( | ||
| 'memory find -c 5 -s "needle" `mem_with_holes` `mem_with_holes+5*pagesize`', |
There was a problem hiding this comment.
It would be a bit clearer if you used the expanded options names here.
There was a problem hiding this comment.
Could remove a magic number if the program had a mem_with_holes_end var, but you'd need the 5 for --count still anyway and you could do sizeof(... to get that but that makes the test's expectations unclear IMO.
...so leave it as is but I did think about it.
There was a problem hiding this comment.
Done (the first part).
Depends on how you look at it, I guess. The actual read size will be the minimum of this number and the "remainder of memory to be searched" ( I wanted to keep the algorithm simple and keep the entire (potential) match in memory at once, as trying to read in the middle of matching would get messy. I don't think anyone searches for strings this long, but if we do want to (better) support that use case, I think that the best solution would be to switch to an algorithm (e.g. KMP) which scans the search space in a strictly linear fashion -- as that will make it easy to read the memory in arbitrary increments.
Interesting. It does work on my machine. I'll try to debug this.
That would allocate the 64k on the stack, which isn't good practice (I doubt it will make things faster, and it can cause us to blow the stack). 64k is 1000x larger than the preffered small vector size. In practice there will only ever be one heap allocation here -- first |
|
|
||
| // allocate_memory_with_holes returns a pointer to five pages of memory, where | ||
| // the second and fourth page in the range are inaccessible (even to debugging | ||
| // APIs). We use this to test lldb's ability to skip over in accessible blocks. |
There was a problem hiding this comment.
inaccessible as one word
|
LGTM,
SmallVector.h: Probably it is better to just allocate the own buffer once at the beginning and do not resize anything. |
|
I figured out the problem, and it's kind of hilarious. Turns out (linux?) lldb-server does not actually return partial reads (i.e., it behaves like windows), but the reason that the test succeeds (for me) is that the holes in the allocated memory get filled my memory allocated by lldb. For whatever reason (different kernel) this does not happen on the bot. I guess I'll need to fix the memory read issues first.. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
The linux read fix is in #106532. |
This patch fixes an issue where the `memory find` command would effectively stop searching after encountering a memory read error (which could happen due to unreadable memory), without giving any indication that it has done so (it would just print it could not find the pattern). To make matters worse, it would not terminate after encountering this error, but rather proceed to slowly increment the address pointer, which meant that searching a large region could take a very long time (and give the appearance that lldb is actually searching for the thing). The patch fixes this first problem (*) by detecting read errors and skipping over (using GetMemoryRegionInfo) the unreadable parts of memory and resuming the search after them. It also reads the memory in bulk (up to 1MB), which speeds up the search significantly (up to 6x for live processes, 18x for core files). (*) The fix does not work on windows yet, because the ReadMemory API does not return partial results (like it does for other systems). I'm preparing a separate patch to deal with that.
This patch fixes an issue where the
memory findcommand wouldeffectively stop searching after encountering a memory read error (which
could happen due to unreadable memory), without giving any indication
that it has done so (it would just print it could not find the pattern).
To make matters worse, it would not terminate after encountering this
error, but rather proceed to slowly increment the address pointer, which
meant that searching a large region could take a very long time (and
give the appearance that lldb is actually searching for the thing).
The patch fixes this first problem (*) by detecting read errors and
skipping over (using GetMemoryRegionInfo) the unreadable parts of memory
and resuming the search after them. It also reads the memory in bulk
(
max(sizeof(pattern))), which speeds up the search significantly (up to 6x forlive processes, 18x for core files).
(*) The fix does not work on windows yet, because the ReadMemory API
does not return partial results (like it does for other systems). I'm
preparing a separate patch to deal with that.