[lldb][Commands][NFC] Extract memory find expression evaluation into helpers #143686

Michael137 · 2025-06-11T11:21:13Z

This patch factors out the -e option logic into two helper functions. The EvaluateExpression helper might seem redundant but I'll be adding to it in a follow-up patch to fix an issue when running memory find -e for Swift targets.

Also adds test coverage for the error cases that were previously untested.

rdar://152113525

…helpers This patch factors out the `-e` option logic into two helper functions. The `EvaluateExpression` helper might seem redundant but I'll be adding to it in a follow-up patch to fix and issue when running `memory find -e` for Swift targets. Also adds test coverage for the error cases that were previously untested. rdar://152113525

llvmbot · 2025-06-11T11:22:07Z

@llvm/pr-subscribers-lldb

Author: Michael Buch (Michael137)

Changes

This patch factors out the -e option logic into two helper functions. The EvaluateExpression helper might seem redundant but I'll be adding to it in a follow-up patch to fix an issue when running memory find -e for Swift targets.

Also adds test coverage for the error cases that were previously untested.

rdar://152113525

Full diff: https://github.com/llvm/llvm-project/pull/143686.diff

3 Files Affected:

(modified) lldb/source/Commands/CommandObjectMemory.cpp (+61-43)
(modified) lldb/test/API/functionalities/memory/find/TestMemoryFind.py (+31)
(modified) lldb/test/API/functionalities/memory/find/main.cpp (+16)

diff --git a/lldb/source/Commands/CommandObjectMemory.cpp b/lldb/source/Commands/CommandObjectMemory.cpp
index 7140333bb3cde..34230df9b2727 100644
--- a/lldb/source/Commands/CommandObjectMemory.cpp
+++ b/lldb/source/Commands/CommandObjectMemory.cpp
@@ -885,6 +885,55 @@ class CommandObjectMemoryRead : public CommandObjectParsed {
 #define LLDB_OPTIONS_memory_find
 #include "CommandOptions.inc"
 
+static llvm::Error CopyExpressionResult(ValueObject &result,
+                                        DataBufferHeap &buffer) {
+  uint64_t value = result.GetValueAsUnsigned(0);
+  auto size_or_err = result.GetCompilerType().GetByteSize(nullptr);
+  if (!size_or_err)
+    return size_or_err.takeError();
+
+  switch (*size_or_err) {
+  case 1: {
+    uint8_t byte = (uint8_t)value;
+    buffer.CopyData(&byte, 1);
+  } break;
+  case 2: {
+    uint16_t word = (uint16_t)value;
+    buffer.CopyData(&word, 2);
+  } break;
+  case 4: {
+    uint32_t lword = (uint32_t)value;
+    buffer.CopyData(&lword, 4);
+  } break;
+  case 8: {
+    buffer.CopyData(&value, 8);
+  } break;
+  case 3:
+  case 5:
+  case 6:
+  case 7:
+    return llvm::createStringError("unknown type. pass a string instead");
+  default:
+    return llvm::createStringError(
+        "result size larger than 8 bytes. pass a string instead");
+  }
+
+  return llvm::Error::success();
+}
+
+static llvm::Expected<ValueObjectSP>
+EvaluateExpression(llvm::StringRef expression, StackFrame &frame,
+                   Process &process) {
+  ValueObjectSP result_sp;
+  auto status =
+      process.GetTarget().EvaluateExpression(expression, &frame, result_sp);
+  if (status != eExpressionCompleted || !result_sp)
+    return llvm::createStringError(
+        "expression evaluation failed. pass a string instead");
+
+  return result_sp;
+}
+
 // Find the specified data in memory
 class CommandObjectMemoryFind : public CommandObjectParsed {
 public:
@@ -1026,49 +1075,18 @@ class CommandObjectMemoryFind : public CommandObjectParsed {
       }
       buffer.CopyData(str);
     } else if (m_memory_options.m_expr.OptionWasSet()) {
-      StackFrame *frame = m_exe_ctx.GetFramePtr();
-      ValueObjectSP result_sp;
-      if ((eExpressionCompleted ==
-           process->GetTarget().EvaluateExpression(
-               m_memory_options.m_expr.GetValueAs<llvm::StringRef>().value_or(
-                   ""),
-               frame, result_sp)) &&
-          result_sp) {
-        uint64_t value = result_sp->GetValueAsUnsigned(0);
-        std::optional<uint64_t> size = llvm::expectedToOptional(
-            result_sp->GetCompilerType().GetByteSize(nullptr));
-        if (!size)
-          return;
-        switch (*size) {
-        case 1: {
-          uint8_t byte = (uint8_t)value;
-          buffer.CopyData(&byte, 1);
-        } break;
-        case 2: {
-          uint16_t word = (uint16_t)value;
-          buffer.CopyData(&word, 2);
-        } break;
-        case 4: {
-          uint32_t lword = (uint32_t)value;
-          buffer.CopyData(&lword, 4);
-        } break;
-        case 8: {
-          buffer.CopyData(&value, 8);
-        } break;
-        case 3:
-        case 5:
-        case 6:
-        case 7:
-          result.AppendError("unknown type. pass a string instead");
-          return;
-        default:
-          result.AppendError(
-              "result size larger than 8 bytes. pass a string instead");
-          return;
-        }
-      } else {
-        result.AppendError(
-            "expression evaluation failed. pass a string instead");
+      auto result_or_err = EvaluateExpression(
+          m_memory_options.m_expr.GetValueAs<llvm::StringRef>().value_or(""),
+          m_exe_ctx.GetFrameRef(), *process);
+      if (!result_or_err) {
+        result.AppendError(llvm::toString(result_or_err.takeError()));
+        return;
+      }
+
+      ValueObjectSP result_sp = *result_or_err;
+
+      if (auto err = CopyExpressionResult(*result_sp, buffer)) {
+        result.AppendError(llvm::toString(std::move(err)));
         return;
       }
     } else {
diff --git a/lldb/test/API/functionalities/memory/find/TestMemoryFind.py b/lldb/test/API/functionalities/memory/find/TestMemoryFind.py
index 09611cc808777..1b9acb8d3090e 100644
--- a/lldb/test/API/functionalities/memory/find/TestMemoryFind.py
+++ b/lldb/test/API/functionalities/memory/find/TestMemoryFind.py
@@ -79,3 +79,34 @@ def test_memory_find(self):
             'memory find -s "nothere" `stringdata` `stringdata+10`',
             substrs=["data not found within the range."],
         )
+
+        # Expression results with unsupported result types.
+        self.expect(
+            'memory find -e "ThreeBytes{}" `&bytedata[0]` `&bytedata[2]`',
+            substrs=["unknown type."],
+            error=True,
+        )
+
+        self.expect(
+            'memory find -e "FiveBytes{}" `&bytedata[0]` `&bytedata[2]`',
+            substrs=["unknown type."],
+            error=True,
+        )
+
+        self.expect(
+            'memory find -e "SixBytes{}" `&bytedata[0]` `&bytedata[2]`',
+            substrs=["unknown type."],
+            error=True,
+        )
+
+        self.expect(
+            'memory find -e "SevenBytes{}" `&bytedata[0]` `&bytedata[2]`',
+            substrs=["unknown type."],
+            error=True,
+        )
+
+        self.expect(
+            'memory find -e "NineBytes{}" `&bytedata[0]` `&bytedata[2]`',
+            substrs=["result size larger than 8 bytes."],
+            error=True,
+        )
diff --git a/lldb/test/API/functionalities/memory/find/main.cpp b/lldb/test/API/functionalities/memory/find/main.cpp
index e3dcfc762ee0f..a5ee27c5c487d 100644
--- a/lldb/test/API/functionalities/memory/find/main.cpp
+++ b/lldb/test/API/functionalities/memory/find/main.cpp
@@ -1,9 +1,25 @@
 #include <stdio.h>
 #include <stdint.h>
 
+template<size_t T>
+struct [[gnu::packed]] Payload {
+  uint8_t data[T];
+};
+
+using ThreeBytes = Payload<3>;
+using FiveBytes = Payload<5>;
+using SixBytes = Payload<5>;
+using SevenBytes = Payload<7>;
+using NineBytes = Payload<9>;
+
 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};
+    ThreeBytes b1;
+    FiveBytes b2;
+    SixBytes b3;
+    SevenBytes b4;
+    NineBytes b5;
     return 0; // break here
 }

github-actions · 2025-06-11T11:24:32Z

✅ With the latest revision this PR passed the C/C++ code formatter.

adrian-prantl · 2025-06-11T14:51:49Z

lldb/source/Commands/CommandObjectMemory.cpp

+  case 5:
+  case 6:
+  case 7:
+    return llvm::createStringError("unknown type. pass a string instead");


Suggested change

return llvm::createStringError("unknown type. pass a string instead");

return llvm::createStringError("unknown type. Pass a string instead");

Mildly confused about the "pass a string" advice. Is this user-visible and if yes, what's an example where this would be shown?

(I also understand this isn't your error message)

Yea this is user visible. It would show exactly this error message:

unknown type. Pass a string instead

What it's trying to say is to use the memory find -s option instead. Could definitely be clearer

Ah I see. How about?

Only expressions resulting in 1, 2, 4, or 8-byte-sized values are supported. For other pattern sizes the --string (-s) option may be used.

lldb/source/Commands/CommandObjectMemory.cpp

adrian-prantl · 2025-06-11T14:52:38Z

lldb/source/Commands/CommandObjectMemory.cpp

+  ValueObjectSP result_sp;
+  auto status =
+      process.GetTarget().EvaluateExpression(expression, &frame, result_sp);
+  if (status != eExpressionCompleted || !result_sp)


Should this be two separate error messages?

Not sure what the value would be tbh. Both indicate that something went wrong during expression evaluation. But if you disagree happy to split it into two

I thought it would be nice to bubble up the error from the expression, but that is presumably attached to result_sp and this condition just catches execution failures, not compiler diagnostics, which should be much rarer. I think this is fine.

Yea that seems reasonable. Happy to address that in a separate patch (just to keep things simple for this particular fix)

adrian-prantl

Anyway, this is good otherwise!

github-actions · 2025-06-12T13:56:36Z

✅ With the latest revision this PR passed the Python code formatter.

This adds tests for: * llvm#143686 * llvm#143860 rdar://152113525

…helpers (llvm#143686) This patch factors out the `-e` option logic into two helper functions. The `EvaluateExpression` helper might seem redundant but I'll be adding to it in a follow-up patch to fix an issue when running `memory find -e` for Swift targets. Also adds test coverage for the error cases that were previously untested. rdar://152113525 (cherry picked from commit 1c1df94)

This adds tests for: * llvm#143686 * llvm#143860 rdar://152113525 (cherry picked from commit 276cb9d)

(depends on #143686) There were two issues previously preventing `memory find -e` expressions to succeed when stopped in Swift frames: 1. We weren't getting the dynamic type of the result `ValueObject`. For Swift this would fail when we tried to produce a scalar value out of it because the static VO wasn't sufficient to get to the integer value. Hence we add a call to `GetQualifiedRepresentationIfAvailable` (which is what we do for expressions in `OptionArgParser::ToAddress` too). 2. We weren't passing an `ExecutionContextScope` to `GetByteSize`, which Swift relied on to get the size of the result type. My plan is to add an API test for this on the Apple `swiftlang/llvm-project` fork. I considered an alternative where we use `OptionArgParser::ToAddress` for `memory find -e` expressions, but it got a bit icky when trying to figure out how many bytes we should copy out of the result into the `DataBufferHeap` (currently we rely on the size of the result variable type). This gets even trickier when we were to pass an expression that was actually a hex digit or a number into `ToAddress`. rdar://152113525

(depends on llvm#143686) There were two issues previously preventing `memory find -e` expressions to succeed when stopped in Swift frames: 1. We weren't getting the dynamic type of the result `ValueObject`. For Swift this would fail when we tried to produce a scalar value out of it because the static VO wasn't sufficient to get to the integer value. Hence we add a call to `GetQualifiedRepresentationIfAvailable` (which is what we do for expressions in `OptionArgParser::ToAddress` too). 2. We weren't passing an `ExecutionContextScope` to `GetByteSize`, which Swift relied on to get the size of the result type. My plan is to add an API test for this on the Apple `swiftlang/llvm-project` fork. I considered an alternative where we use `OptionArgParser::ToAddress` for `memory find -e` expressions, but it got a bit icky when trying to figure out how many bytes we should copy out of the result into the `DataBufferHeap` (currently we rely on the size of the result variable type). This gets even trickier when we were to pass an expression that was actually a hex digit or a number into `ToAddress`. rdar://152113525 (cherry picked from commit c6da2c8)

…43860) (depends on llvm/llvm-project#143686) There were two issues previously preventing `memory find -e` expressions to succeed when stopped in Swift frames: 1. We weren't getting the dynamic type of the result `ValueObject`. For Swift this would fail when we tried to produce a scalar value out of it because the static VO wasn't sufficient to get to the integer value. Hence we add a call to `GetQualifiedRepresentationIfAvailable` (which is what we do for expressions in `OptionArgParser::ToAddress` too). 2. We weren't passing an `ExecutionContextScope` to `GetByteSize`, which Swift relied on to get the size of the result type. My plan is to add an API test for this on the Apple `swiftlang/llvm-project` fork. I considered an alternative where we use `OptionArgParser::ToAddress` for `memory find -e` expressions, but it got a bit icky when trying to figure out how many bytes we should copy out of the result into the `DataBufferHeap` (currently we rely on the size of the result variable type). This gets even trickier when we were to pass an expression that was actually a hex digit or a number into `ToAddress`. rdar://152113525

…helpers (llvm#143686) This patch factors out the `-e` option logic into two helper functions. The `EvaluateExpression` helper might seem redundant but I'll be adding to it in a follow-up patch to fix an issue when running `memory find -e` for Swift targets. Also adds test coverage for the error cases that were previously untested. rdar://152113525

(depends on llvm#143686) There were two issues previously preventing `memory find -e` expressions to succeed when stopped in Swift frames: 1. We weren't getting the dynamic type of the result `ValueObject`. For Swift this would fail when we tried to produce a scalar value out of it because the static VO wasn't sufficient to get to the integer value. Hence we add a call to `GetQualifiedRepresentationIfAvailable` (which is what we do for expressions in `OptionArgParser::ToAddress` too). 2. We weren't passing an `ExecutionContextScope` to `GetByteSize`, which Swift relied on to get the size of the result type. My plan is to add an API test for this on the Apple `swiftlang/llvm-project` fork. I considered an alternative where we use `OptionArgParser::ToAddress` for `memory find -e` expressions, but it got a bit icky when trying to figure out how many bytes we should copy out of the result into the `DataBufferHeap` (currently we rely on the size of the result variable type). This gets even trickier when we were to pass an expression that was actually a hex digit or a number into `ToAddress`. rdar://152113525

…helpers (llvm#143686) This patch factors out the `-e` option logic into two helper functions. The `EvaluateExpression` helper might seem redundant but I'll be adding to it in a follow-up patch to fix an issue when running `memory find -e` for Swift targets. Also adds test coverage for the error cases that were previously untested. rdar://152113525

(depends on llvm#143686) There were two issues previously preventing `memory find -e` expressions to succeed when stopped in Swift frames: 1. We weren't getting the dynamic type of the result `ValueObject`. For Swift this would fail when we tried to produce a scalar value out of it because the static VO wasn't sufficient to get to the integer value. Hence we add a call to `GetQualifiedRepresentationIfAvailable` (which is what we do for expressions in `OptionArgParser::ToAddress` too). 2. We weren't passing an `ExecutionContextScope` to `GetByteSize`, which Swift relied on to get the size of the result type. My plan is to add an API test for this on the Apple `swiftlang/llvm-project` fork. I considered an alternative where we use `OptionArgParser::ToAddress` for `memory find -e` expressions, but it got a bit icky when trying to figure out how many bytes we should copy out of the result into the `DataBufferHeap` (currently we rely on the size of the result variable type). This gets even trickier when we were to pass an expression that was actually a hex digit or a number into `ToAddress`. rdar://152113525

Michael137 requested review from adrian-prantl and labath June 11, 2025 11:21

Michael137 requested a review from JDevlieghere as a code owner June 11, 2025 11:21

llvmbot added the lldb label Jun 11, 2025

fixup! clang-format

e6622bd

adrian-prantl reviewed Jun 11, 2025

View reviewed changes

Michael137 mentioned this pull request Jun 12, 2025

[lldb][Commands] Fix memory find for Swift expressions #143860

Merged

adrian-prantl self-requested a review June 12, 2025 13:52

adrian-prantl approved these changes Jun 12, 2025

View reviewed changes

fixup! improve error message

695a2fe

Michael137 force-pushed the lldb/memory-find-expression-1 branch from e2d6bc8 to 695a2fe Compare June 12, 2025 13:53

fixup! python format

fc08ba5

Michael137 merged commit 1c1df94 into llvm:main Jun 12, 2025
7 checks passed

Michael137 deleted the lldb/memory-find-expression-1 branch June 12, 2025 15:49

Michael137 added a commit to swiftlang/llvm-project that referenced this pull request Jun 12, 2025

[lldb][test] Add test for Swift expressions in the "memory find" command

276cb9d

This adds tests for: * llvm#143686 * llvm#143860 rdar://152113525

Michael137 mentioned this pull request Jun 12, 2025

[lldb][test] Add test for Swift expressions in the "memory find" command swiftlang/llvm-project#10830

Merged

Michael137 added a commit to swiftlang/llvm-project that referenced this pull request Jun 12, 2025

[lldb][test] Add test for Swift expressions in the "memory find" command

fbfa805

This adds tests for: * llvm#143686 * llvm#143860 rdar://152113525 (cherry picked from commit 276cb9d)

	return llvm::createStringError("unknown type. pass a string instead");
	return llvm::createStringError("unknown type. Pass a string instead");

[lldb][Commands][NFC] Extract memory find expression evaluation into helpers #143686

[lldb][Commands][NFC] Extract memory find expression evaluation into helpers #143686

Uh oh!

Conversation

Michael137 commented Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jun 11, 2025

Uh oh!

github-actions bot commented Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrian-prantl left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Michael137 commented Jun 11, 2025 •

edited

Loading

github-actions bot commented Jun 11, 2025 •

edited

Loading

github-actions bot commented Jun 12, 2025 •

edited

Loading