-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[lldb][Commands][NFC] Extract memory find expression evaluation into helpers #143686
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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"); | ||
Michael137 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| 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) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be two separate error messages?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea that seems reasonable. Happy to address that in a separate patch (just to keep things simple for this particular fix) |
||
| 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 { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,24 @@ | ||
| #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 | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mildly confused about the "pass a string" advice. Is this user-visible and if yes, what's an example where this would be shown?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I also understand this isn't your error message)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea this is user visible. It would show exactly this error message:
What it's trying to say is to use the
memory find -soption instead. Could definitely be clearerThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see. How about?