[lldb][Commands][NFC] Extract memory find expression evaluation into helpers #143686
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…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
Member
|
@llvm/pr-subscribers-lldb Author: Michael Buch (Michael137) ChangesThis patch factors out the 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:
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
}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| case 5: | ||
| case 6: | ||
| case 7: | ||
| return llvm::createStringError("unknown type. pass a string instead"); |
Collaborator
There was a problem hiding this comment.
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?
Collaborator
There was a problem hiding this comment.
(I also understand this isn't your error message)
Member
Author
There was a problem hiding this comment.
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
Collaborator
There was a problem hiding this comment.
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.
| ValueObjectSP result_sp; | ||
| auto status = | ||
| process.GetTarget().EvaluateExpression(expression, &frame, result_sp); | ||
| if (status != eExpressionCompleted || !result_sp) |
Collaborator
There was a problem hiding this comment.
Should this be two separate error messages?
Member
Author
There was a problem hiding this comment.
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
Collaborator
There was a problem hiding this comment.
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.
Member
Author
There was a problem hiding this comment.
Yea that seems reasonable. Happy to address that in a separate patch (just to keep things simple for this particular fix)
adrian-prantl
approved these changes
Jun 12, 2025
Collaborator
adrian-prantl
left a comment
adrian-prantl
left a comment
There was a problem hiding this comment.
Anyway, this is good otherwise!
Michael137
force-pushed
the
lldb/memory-find-expression-1
branch
from
e2d6bc8 to
695a2fe
Compare
|
✅ With the latest revision this PR passed the Python code formatter. |
Michael137
added a commit
to swiftlang/llvm-project
that referenced
this pull request
Jun 12, 2025
This adds tests for: * llvm#143686 * llvm#143860 rdar://152113525
Michael137
added a commit
to swiftlang/llvm-project
that referenced
this pull request
Jun 12, 2025
…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)
Michael137
added a commit
to swiftlang/llvm-project
that referenced
this pull request
Jun 12, 2025
This adds tests for: * llvm#143686 * llvm#143860 rdar://152113525 (cherry picked from commit 276cb9d)
Michael137
added a commit
that referenced
this pull request
Jun 12, 2025
(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
Michael137
added a commit
to swiftlang/llvm-project
that referenced
this pull request
Jun 12, 2025
(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)
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Jun 12, 2025
…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
tomtor
pushed a commit
to tomtor/llvm-project
that referenced
this pull request
Jun 14, 2025
…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
tomtor
pushed a commit
to tomtor/llvm-project
that referenced
this pull request
Jun 14, 2025
(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
akuhlens
pushed a commit
to akuhlens/llvm-project
that referenced
this pull request
Jun 24, 2025
…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
akuhlens
pushed a commit
to akuhlens/llvm-project
that referenced
this pull request
Jun 24, 2025
(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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch factors out the
-eoption logic into two helper functions. TheEvaluateExpressionhelper might seem redundant but I'll be adding to it in a follow-up patch to fix an issue when runningmemory find -efor Swift targets.Also adds test coverage for the error cases that were previously untested.
rdar://152113525