[lldb] Fix OP_deref evaluation for large integer results #159460
Merged
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
felipepiovezan
requested review from
JDevlieghere,
adrian-prantl and
jasonmolenda
|
@llvm/pr-subscribers-lldb Author: Felipe de Azevedo Piovezan (felipepiovezan) ChangesWhen evaluating any DWARF expression that ended in OP_deref and whose previous value on the dwarf stack -- the pointer address for the deref -- was a load address, we were treating the result itself as a pointer, calling Process:FixCodeAddress(result). This is wrong: there's no guarantee that the result is a pointer itself. Full diff: https://github.com/llvm/llvm-project/pull/159460.diff 3 Files Affected:
diff --git a/lldb/source/Expression/DWARFExpression.cpp b/lldb/source/Expression/DWARFExpression.cpp
index 5040351f4975b..4f9d6ebf27bf0 100644
--- a/lldb/source/Expression/DWARFExpression.cpp
+++ b/lldb/source/Expression/DWARFExpression.cpp
@@ -909,8 +909,6 @@ static llvm::Error Evaluate_DW_OP_deref(DWARFExpression::Stack &stack,
" for DW_OP_deref",
pointer_addr),
error.takeError());
- if (ABISP abi_sp = process->GetABI())
- pointer_value = abi_sp->FixCodeAddress(pointer_value);
stack.back().GetScalar() = pointer_value;
stack.back().ClearContext();
} break;
diff --git a/lldb/unittests/Expression/CMakeLists.txt b/lldb/unittests/Expression/CMakeLists.txt
index 4c58b3c5e3922..a22341d9155cb 100644
--- a/lldb/unittests/Expression/CMakeLists.txt
+++ b/lldb/unittests/Expression/CMakeLists.txt
@@ -6,12 +6,16 @@ add_lldb_unittest(ExpressionTests
CppModuleConfigurationTest.cpp
ExpressionTest.cpp
+ LINK_COMPONENTS
+ AArch64
+ Support
LINK_LIBS
lldbCore
lldbPluginObjectFileELF
lldbPluginObjectFileWasm
lldbPluginSymbolVendorWasm
lldbPluginPlatformLinux
+ lldbPluginABIAArch64
lldbPluginExpressionParserClang
lldbPluginTypeSystemClang
lldbUtility
diff --git a/lldb/unittests/Expression/DWARFExpressionTest.cpp b/lldb/unittests/Expression/DWARFExpressionTest.cpp
index 5a5d3aba0e207..3ea232b70f863 100644
--- a/lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ b/lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -6,6 +6,9 @@
//
//===----------------------------------------------------------------------===//
+#include "lldb/Target/ABI.h"
+#include "Plugins/ABI/AArch64/ABISysV_arm64.h"
+#include "llvm/Support/TargetSelect.h"
#include "lldb/Expression/DWARFExpression.h"
#include "Plugins/ObjectFile/wasm/ObjectFileWasm.h"
#include "Plugins/Platform/Linux/PlatformLinux.h"
@@ -190,15 +193,39 @@ class DWARFExpressionMockProcessTest : public ::testing::Test {
void SetUp() override {
FileSystem::Initialize();
HostInfo::Initialize();
+ LLVMInitializeAArch64TargetInfo();
+ LLVMInitializeAArch64TargetMC();
platform_linux::PlatformLinux::Initialize();
+ ABISysV_arm64::Initialize();
}
void TearDown() override {
platform_linux::PlatformLinux::Terminate();
HostInfo::Terminate();
FileSystem::Terminate();
+ ABISysV_arm64::Terminate();
}
};
+struct PlatformTargetDebugger {
+ lldb::PlatformSP platform_sp;
+ lldb::TargetSP target_sp;
+ lldb::DebuggerSP debugger_sp;
+};
+
+/// A helper function to create <Platform, Target, Debugger> objects with the
+/// "aarch64-pc-linux" ArchSpec.
+static PlatformTargetDebugger CreateTarget() {
+ ArchSpec arch("aarch64-pc-linux");
+ Platform::SetHostPlatform(
+ platform_linux::PlatformLinux::CreateInstance(true, &arch));
+ lldb::PlatformSP platform_sp;
+ lldb::TargetSP target_sp;
+ lldb::DebuggerSP debugger_sp = Debugger::CreateInstance();
+ debugger_sp->GetTargetList().CreateTarget(
+ *debugger_sp, "", arch, eLoadDependentsNo, platform_sp, target_sp);
+ return PlatformTargetDebugger{platform_sp, target_sp, debugger_sp};
+}
+
// NB: This class doesn't use the override keyword to avoid
// -Winconsistent-missing-override warnings from the compiler. The
// inconsistency comes from the overriding definitions in the MOCK_*** macros.
@@ -1135,3 +1162,76 @@ TEST_F(DWARFExpressionMockProcessTest, DW_OP_piece_file_addr) {
ASSERT_EQ(result->GetValueType(), Value::ValueType::HostAddress);
ASSERT_THAT(result->GetBuffer().GetData(), ElementsAre(0x11, 0x22));
}
+
+/// A Process whose `ReadMemory` override queries a DenseMap.
+struct MockProcessWithMemRead : Process {
+ using addr_t = lldb::addr_t;
+
+ llvm::DenseMap<addr_t, addr_t> memory_map;
+
+ MockProcessWithMemRead(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp,
+ llvm::DenseMap<addr_t, addr_t> &&memory_map)
+ : Process(target_sp, listener_sp), memory_map(memory_map) {}
+ size_t DoReadMemory(addr_t vm_addr, void *buf, size_t size,
+ Status &error) override {
+ assert(memory_map.contains(vm_addr));
+ assert(size == sizeof(addr_t));
+ *reinterpret_cast<addr_t *>(buf) = memory_map[vm_addr];
+ return sizeof(addr_t);
+ }
+ size_t ReadMemory(addr_t addr, void *buf, size_t size,
+ Status &status) override {
+ return DoReadMemory(addr, buf, size, status);
+ }
+ bool CanDebug(lldb::TargetSP, bool) override { return true; }
+ Status DoDestroy() override { return Status(); }
+ llvm::StringRef GetPluginName() override { return ""; }
+ void RefreshStateAfterStop() override {}
+ bool DoUpdateThreadList(ThreadList &, ThreadList &) override { return false; }
+};
+
+/// Sets the value of register x22 to "42".
+/// Creates a process whose memory address 42 contains the value
+/// memory[42] = ((0xffULL) << 56) | 0xabcdef;
+/// The expression DW_OP_breg22, 0, DW_OP_deref should produce that same value,
+/// without clearing the top byte 0xff.
+TEST_F(DWARFExpressionMockProcessTest, DW_op_deref_no_ptr_fixing) {
+ llvm::DenseMap<lldb::addr_t, lldb::addr_t> memory;
+ constexpr lldb::addr_t expected_value = ((0xffULL) << 56) | 0xabcdefULL;
+ memory[42] = expected_value;
+
+ PlatformTargetDebugger test_setup = CreateTarget();
+ lldb::ProcessSP process_sp = std::make_shared<MockProcessWithMemRead>(
+ test_setup.target_sp, Listener::MakeListener("dummy"), std::move(memory));
+ auto thread = std::make_shared<MockThread>(*process_sp);
+ lldb::RegisterContextSP reg_ctx_sp =
+ std::make_shared<MockRegisterContext>(*thread, RegisterValue(42ull));
+ thread->SetRegisterContext(reg_ctx_sp);
+ process_sp->GetThreadList().AddThread(thread);
+
+ auto evaluate_expr = [&](auto &expr_data) {
+ DataExtractor extractor(expr_data, sizeof(expr_data),
+ lldb::eByteOrderLittle,
+ /*addr_size*/ 8);
+ DWARFExpression expr(extractor);
+
+ ExecutionContext exe_ctx(process_sp);
+ llvm::Expected<Value> result = DWARFExpression::Evaluate(
+ &exe_ctx, reg_ctx_sp.get(), /*module_sp*/ nullptr, extractor,
+ /*unit*/ nullptr, lldb::eRegisterKindLLDB,
+ /*initial_value_ptr=*/nullptr,
+ /*object_address_ptr=*/nullptr);
+ return result;
+ };
+
+ uint8_t expr_reg[] = {DW_OP_breg22, 0};
+ llvm::Expected<Value> result_reg = evaluate_expr(expr_reg);
+ ASSERT_THAT_EXPECTED(result_reg, llvm::Succeeded());
+ ASSERT_EQ(result_reg->GetValueType(), Value::ValueType::LoadAddress);
+ ASSERT_EQ(result_reg->GetScalar().ULongLong(), 42ull);
+
+ uint8_t expr_deref[] = {DW_OP_breg22, 0, DW_OP_deref};
+ llvm::Expected<Value> result_deref = evaluate_expr(expr_deref);
+ ASSERT_THAT_EXPECTED(result_deref, llvm::Succeeded());
+ ASSERT_EQ(result_deref->GetScalar().ULongLong(), expected_value);
+}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
When evaluating any DWARF expression that ended in OP_deref and whose previous value on the dwarf stack -- the pointer address for the deref -- was a load address, we were treating the result itself as a pointer, calling Process:FixCodeAddress(result). This is wrong: there's no guarantee that the result is a pointer itself.
felipepiovezan
force-pushed
the
felipe/op_deref_bugdix
branch
from
f41fba6 to
ef8ba40
Compare
jasonmolenda
approved these changes
Sep 17, 2025
JDevlieghere
approved these changes
Sep 17, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/163/builds/26651 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/181/builds/28097 Here is the relevant piece of the build log for the reference |
Seems like a real problem. Will revert while I debug. |
felipepiovezan
added a commit
to felipepiovezan/llvm-project
that referenced
this pull request
Sep 17, 2025
…lts (llvm#159460)"" The original had an issue on "AArch-less" bots. Fixed it with some ifdefs around the presence of the AArch ABI plugin. This reverts commit 1a4685d.
felipepiovezan
added a commit
to felipepiovezan/llvm-project
that referenced
this pull request
Sep 18, 2025
…lts (llvm#159460)"" The original had an issue on "AArch-less" bots. Fixed it with some ifdefs around the presence of the AArch ABI plugin. This reverts commit 1a4685d. (cherry picked from commit 40eb976)
felipepiovezan
added a commit
to felipepiovezan/llvm-project
that referenced
this pull request
Sep 18, 2025
…lts (llvm#159460)"" The original had an issue on "AArch-less" bots. Fixed it with some ifdefs around the presence of the AArch ABI plugin. Note for the cherry-pick: the test was removed as the related test file in this branch is too old. This reverts commit 1a4685d. Cherry-picked from 40eb976.
kimsh02
pushed a commit
to kimsh02/llvm-project
that referenced
this pull request
Sep 19, 2025
When evaluating any DWARF expression that ended in OP_deref and whose previous value on the dwarf stack -- the pointer address for the deref -- was a load address, we were treating the result itself as a pointer, calling Process:FixCodeAddress(result). This is wrong: there's no guarantee that the result is a pointer itself.
kimsh02
pushed a commit
to kimsh02/llvm-project
that referenced
this pull request
Sep 19, 2025
llvm#159482) …lts (llvm#159460)"" The original had an issue on "AArch-less" bots. Fixed it with some ifdefs around the presence of the AArch ABI plugin. This reverts commit 1a4685d.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
When evaluating any DWARF expression that ended in OP_deref and whose previous value on the dwarf stack -- the pointer address for the deref -- was a load address, we were treating the result itself as a pointer, calling Process:FixCodeAddress(result). This is wrong: there's no guarantee that the result is a pointer itself.