[Lldb-commits] [lldb] [lldb] Support reading DW_OP_piece from file address (PR #94026)
https://github.com/JDevlieghere closed https://github.com/llvm/llvm-project/pull/94026 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Support reading DW_OP_piece from file address (PR #94026)
https://github.com/labath approved this pull request. https://github.com/llvm/llvm-project/pull/94026 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Support reading DW_OP_piece from file address (PR #94026)
https://github.com/clayborg approved this pull request. https://github.com/llvm/llvm-project/pull/94026 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Support reading DW_OP_piece from file address (PR #94026)
https://github.com/JDevlieghere updated
https://github.com/llvm/llvm-project/pull/94026
>From 72844ebd5cf8f74f6db5d1c52d1f557ca942dbee Mon Sep 17 00:00:00 2001
From: Jonas Devlieghere
Date: Fri, 31 May 2024 12:21:28 -0700
Subject: [PATCH 1/5] [lldb] Support reading DW_OP_piece from file address
We received a bug report where someone was trying to print a global
variable without a process. This would succeed in a debug build but fail
in a on optimized build. We traced the issue back to the location being
described by a DW_OP_addr + DW_OP_piece.
The issue is that the DWARF expression evaluator only support reading
pieces from a load address. There's no reason it cannot do the same for
a file address, and indeed, that solves the problem.
I unsuccessfully tried to craft a test case to illustrate the original
example, using a global struct and trying to trick the compiler into
breaking it apart with SROA. Instead I wrote a unit test that uses a
mock target to read memory from.
rdar://127435923
---
lldb/include/lldb/Target/Target.h | 8 +--
lldb/source/Expression/DWARFExpression.cpp| 57 +++---
.../Expression/DWARFExpressionTest.cpp| 60 +++
3 files changed, 100 insertions(+), 25 deletions(-)
diff --git a/lldb/include/lldb/Target/Target.h
b/lldb/include/lldb/Target/Target.h
index 7ad9f33586054..792a4caa76e2d 100644
--- a/lldb/include/lldb/Target/Target.h
+++ b/lldb/include/lldb/Target/Target.h
@@ -1077,9 +1077,9 @@ class Target : public
std::enable_shared_from_this,
// section, then read from the file cache
// 2 - if there is a process, then read from memory
// 3 - if there is no process, then read from the file cache
- size_t ReadMemory(const Address , void *dst, size_t dst_len,
-Status , bool force_live_memory = false,
-lldb::addr_t *load_addr_ptr = nullptr);
+ virtual size_t ReadMemory(const Address , void *dst, size_t dst_len,
+Status , bool force_live_memory = false,
+lldb::addr_t *load_addr_ptr = nullptr);
size_t ReadCStringFromMemory(const Address , std::string _str,
Status , bool force_live_memory = false);
@@ -1615,7 +1615,7 @@ class Target : public
std::enable_shared_from_this,
TargetStats () { return m_stats; }
-private:
+protected:
/// Construct with optional file and arch.
///
/// This member is private. Clients must use
diff --git a/lldb/source/Expression/DWARFExpression.cpp
b/lldb/source/Expression/DWARFExpression.cpp
index c061fd1140fff..326be0d683804 100644
--- a/lldb/source/Expression/DWARFExpression.cpp
+++ b/lldb/source/Expression/DWARFExpression.cpp
@@ -2123,23 +2123,22 @@ bool DWARFExpression::Evaluate(
const Value::ValueType curr_piece_source_value_type =
curr_piece_source_value.GetValueType();
+ Scalar = curr_piece_source_value.GetScalar();
+ const lldb::addr_t addr = scalar.ULongLong(LLDB_INVALID_ADDRESS);
switch (curr_piece_source_value_type) {
case Value::ValueType::Invalid:
return false;
case Value::ValueType::LoadAddress:
if (process) {
if (curr_piece.ResizeData(piece_byte_size) == piece_byte_size) {
-lldb::addr_t load_addr =
-curr_piece_source_value.GetScalar().ULongLong(
-LLDB_INVALID_ADDRESS);
-if (process->ReadMemory(
-load_addr, curr_piece.GetBuffer().GetBytes(),
-piece_byte_size, error) != piece_byte_size) {
+if (process->ReadMemory(addr,
curr_piece.GetBuffer().GetBytes(),
+piece_byte_size,
+error) != piece_byte_size) {
if (error_ptr)
error_ptr->SetErrorStringWithFormat(
"failed to read memory DW_OP_piece(%" PRIu64
-") from 0x%" PRIx64,
-piece_byte_size, load_addr);
+") from load address 0x%" PRIx64,
+piece_byte_size, addr);
return false;
}
} else {
@@ -2153,26 +2152,42 @@ bool DWARFExpression::Evaluate(
}
break;
- case Value::ValueType::FileAddress:
- case Value::ValueType::HostAddress:
-if (error_ptr) {
- lldb::addr_t addr =
curr_piece_source_value.GetScalar().ULongLong(
- LLDB_INVALID_ADDRESS);
+ case Value::ValueType::FileAddress: {
+if (target) {
+ if (curr_piece.ResizeData(piece_byte_size) == piece_byte_size) {
+if (target->ReadMemory(addr, curr_piece.GetBuffer().GetBytes(),
+ piece_byte_size, error,
+
[Lldb-commits] [lldb] [lldb] Support reading DW_OP_piece from file address (PR #94026)
https://github.com/clayborg approved this pull request. LGTM, just one optional comment, see inlined comment. https://github.com/llvm/llvm-project/pull/94026 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Support reading DW_OP_piece from file address (PR #94026)
@@ -1077,9 +1077,9 @@ class Target : public std::enable_shared_from_this, // section, then read from the file cache // 2 - if there is a process, then read from memory // 3 - if there is no process, then read from the file cache - size_t ReadMemory(const Address , void *dst, size_t dst_len, -Status , bool force_live_memory = false, -lldb::addr_t *load_addr_ptr = nullptr); + virtual size_t ReadMemory(const Address , void *dst, size_t dst_len, clayborg wrote: Might be nice to comment that this is virtual for mock testing only. https://github.com/llvm/llvm-project/pull/94026 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Support reading DW_OP_piece from file address (PR #94026)
https://github.com/clayborg edited https://github.com/llvm/llvm-project/pull/94026 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Support reading DW_OP_piece from file address (PR #94026)
https://github.com/JDevlieghere updated
https://github.com/llvm/llvm-project/pull/94026
>From 72844ebd5cf8f74f6db5d1c52d1f557ca942dbee Mon Sep 17 00:00:00 2001
From: Jonas Devlieghere
Date: Fri, 31 May 2024 12:21:28 -0700
Subject: [PATCH 1/4] [lldb] Support reading DW_OP_piece from file address
We received a bug report where someone was trying to print a global
variable without a process. This would succeed in a debug build but fail
in a on optimized build. We traced the issue back to the location being
described by a DW_OP_addr + DW_OP_piece.
The issue is that the DWARF expression evaluator only support reading
pieces from a load address. There's no reason it cannot do the same for
a file address, and indeed, that solves the problem.
I unsuccessfully tried to craft a test case to illustrate the original
example, using a global struct and trying to trick the compiler into
breaking it apart with SROA. Instead I wrote a unit test that uses a
mock target to read memory from.
rdar://127435923
---
lldb/include/lldb/Target/Target.h | 8 +--
lldb/source/Expression/DWARFExpression.cpp| 57 +++---
.../Expression/DWARFExpressionTest.cpp| 60 +++
3 files changed, 100 insertions(+), 25 deletions(-)
diff --git a/lldb/include/lldb/Target/Target.h
b/lldb/include/lldb/Target/Target.h
index 7ad9f33586054..792a4caa76e2d 100644
--- a/lldb/include/lldb/Target/Target.h
+++ b/lldb/include/lldb/Target/Target.h
@@ -1077,9 +1077,9 @@ class Target : public
std::enable_shared_from_this,
// section, then read from the file cache
// 2 - if there is a process, then read from memory
// 3 - if there is no process, then read from the file cache
- size_t ReadMemory(const Address , void *dst, size_t dst_len,
-Status , bool force_live_memory = false,
-lldb::addr_t *load_addr_ptr = nullptr);
+ virtual size_t ReadMemory(const Address , void *dst, size_t dst_len,
+Status , bool force_live_memory = false,
+lldb::addr_t *load_addr_ptr = nullptr);
size_t ReadCStringFromMemory(const Address , std::string _str,
Status , bool force_live_memory = false);
@@ -1615,7 +1615,7 @@ class Target : public
std::enable_shared_from_this,
TargetStats () { return m_stats; }
-private:
+protected:
/// Construct with optional file and arch.
///
/// This member is private. Clients must use
diff --git a/lldb/source/Expression/DWARFExpression.cpp
b/lldb/source/Expression/DWARFExpression.cpp
index c061fd1140fff..326be0d683804 100644
--- a/lldb/source/Expression/DWARFExpression.cpp
+++ b/lldb/source/Expression/DWARFExpression.cpp
@@ -2123,23 +2123,22 @@ bool DWARFExpression::Evaluate(
const Value::ValueType curr_piece_source_value_type =
curr_piece_source_value.GetValueType();
+ Scalar = curr_piece_source_value.GetScalar();
+ const lldb::addr_t addr = scalar.ULongLong(LLDB_INVALID_ADDRESS);
switch (curr_piece_source_value_type) {
case Value::ValueType::Invalid:
return false;
case Value::ValueType::LoadAddress:
if (process) {
if (curr_piece.ResizeData(piece_byte_size) == piece_byte_size) {
-lldb::addr_t load_addr =
-curr_piece_source_value.GetScalar().ULongLong(
-LLDB_INVALID_ADDRESS);
-if (process->ReadMemory(
-load_addr, curr_piece.GetBuffer().GetBytes(),
-piece_byte_size, error) != piece_byte_size) {
+if (process->ReadMemory(addr,
curr_piece.GetBuffer().GetBytes(),
+piece_byte_size,
+error) != piece_byte_size) {
if (error_ptr)
error_ptr->SetErrorStringWithFormat(
"failed to read memory DW_OP_piece(%" PRIu64
-") from 0x%" PRIx64,
-piece_byte_size, load_addr);
+") from load address 0x%" PRIx64,
+piece_byte_size, addr);
return false;
}
} else {
@@ -2153,26 +2152,42 @@ bool DWARFExpression::Evaluate(
}
break;
- case Value::ValueType::FileAddress:
- case Value::ValueType::HostAddress:
-if (error_ptr) {
- lldb::addr_t addr =
curr_piece_source_value.GetScalar().ULongLong(
- LLDB_INVALID_ADDRESS);
+ case Value::ValueType::FileAddress: {
+if (target) {
+ if (curr_piece.ResizeData(piece_byte_size) == piece_byte_size) {
+if (target->ReadMemory(addr, curr_piece.GetBuffer().GetBytes(),
+ piece_byte_size, error,
+
[Lldb-commits] [lldb] [lldb] Support reading DW_OP_piece from file address (PR #94026)
https://github.com/JDevlieghere updated
https://github.com/llvm/llvm-project/pull/94026
>From 72844ebd5cf8f74f6db5d1c52d1f557ca942dbee Mon Sep 17 00:00:00 2001
From: Jonas Devlieghere
Date: Fri, 31 May 2024 12:21:28 -0700
Subject: [PATCH 1/3] [lldb] Support reading DW_OP_piece from file address
We received a bug report where someone was trying to print a global
variable without a process. This would succeed in a debug build but fail
in a on optimized build. We traced the issue back to the location being
described by a DW_OP_addr + DW_OP_piece.
The issue is that the DWARF expression evaluator only support reading
pieces from a load address. There's no reason it cannot do the same for
a file address, and indeed, that solves the problem.
I unsuccessfully tried to craft a test case to illustrate the original
example, using a global struct and trying to trick the compiler into
breaking it apart with SROA. Instead I wrote a unit test that uses a
mock target to read memory from.
rdar://127435923
---
lldb/include/lldb/Target/Target.h | 8 +--
lldb/source/Expression/DWARFExpression.cpp| 57 +++---
.../Expression/DWARFExpressionTest.cpp| 60 +++
3 files changed, 100 insertions(+), 25 deletions(-)
diff --git a/lldb/include/lldb/Target/Target.h
b/lldb/include/lldb/Target/Target.h
index 7ad9f33586054..792a4caa76e2d 100644
--- a/lldb/include/lldb/Target/Target.h
+++ b/lldb/include/lldb/Target/Target.h
@@ -1077,9 +1077,9 @@ class Target : public
std::enable_shared_from_this,
// section, then read from the file cache
// 2 - if there is a process, then read from memory
// 3 - if there is no process, then read from the file cache
- size_t ReadMemory(const Address , void *dst, size_t dst_len,
-Status , bool force_live_memory = false,
-lldb::addr_t *load_addr_ptr = nullptr);
+ virtual size_t ReadMemory(const Address , void *dst, size_t dst_len,
+Status , bool force_live_memory = false,
+lldb::addr_t *load_addr_ptr = nullptr);
size_t ReadCStringFromMemory(const Address , std::string _str,
Status , bool force_live_memory = false);
@@ -1615,7 +1615,7 @@ class Target : public
std::enable_shared_from_this,
TargetStats () { return m_stats; }
-private:
+protected:
/// Construct with optional file and arch.
///
/// This member is private. Clients must use
diff --git a/lldb/source/Expression/DWARFExpression.cpp
b/lldb/source/Expression/DWARFExpression.cpp
index c061fd1140fff..326be0d683804 100644
--- a/lldb/source/Expression/DWARFExpression.cpp
+++ b/lldb/source/Expression/DWARFExpression.cpp
@@ -2123,23 +2123,22 @@ bool DWARFExpression::Evaluate(
const Value::ValueType curr_piece_source_value_type =
curr_piece_source_value.GetValueType();
+ Scalar = curr_piece_source_value.GetScalar();
+ const lldb::addr_t addr = scalar.ULongLong(LLDB_INVALID_ADDRESS);
switch (curr_piece_source_value_type) {
case Value::ValueType::Invalid:
return false;
case Value::ValueType::LoadAddress:
if (process) {
if (curr_piece.ResizeData(piece_byte_size) == piece_byte_size) {
-lldb::addr_t load_addr =
-curr_piece_source_value.GetScalar().ULongLong(
-LLDB_INVALID_ADDRESS);
-if (process->ReadMemory(
-load_addr, curr_piece.GetBuffer().GetBytes(),
-piece_byte_size, error) != piece_byte_size) {
+if (process->ReadMemory(addr,
curr_piece.GetBuffer().GetBytes(),
+piece_byte_size,
+error) != piece_byte_size) {
if (error_ptr)
error_ptr->SetErrorStringWithFormat(
"failed to read memory DW_OP_piece(%" PRIu64
-") from 0x%" PRIx64,
-piece_byte_size, load_addr);
+") from load address 0x%" PRIx64,
+piece_byte_size, addr);
return false;
}
} else {
@@ -2153,26 +2152,42 @@ bool DWARFExpression::Evaluate(
}
break;
- case Value::ValueType::FileAddress:
- case Value::ValueType::HostAddress:
-if (error_ptr) {
- lldb::addr_t addr =
curr_piece_source_value.GetScalar().ULongLong(
- LLDB_INVALID_ADDRESS);
+ case Value::ValueType::FileAddress: {
+if (target) {
+ if (curr_piece.ResizeData(piece_byte_size) == piece_byte_size) {
+if (target->ReadMemory(addr, curr_piece.GetBuffer().GetBytes(),
+ piece_byte_size, error,
+
[Lldb-commits] [lldb] [lldb] Support reading DW_OP_piece from file address (PR #94026)
@@ -768,3 +768,63 @@ TEST(DWARFExpression, ExtensionsDWO) {
testExpressionVendorExtensions(dwo_module_sp, *dwo_dwarf_unit);
}
+
+TEST_F(DWARFExpressionMockProcessTest, DW_OP_piece_file_addr) {
+ struct MockTarget : Target {
labath wrote:
The code I linked to ([this
one](https://github.com/llvm/llvm-project/blob/main/lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h#L73))
shows one way to do it. Basically, you implement the real function in terms on
some other interface -- one which is easier to mock -- and then you mock *that*.
https://github.com/llvm/llvm-project/pull/94026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Support reading DW_OP_piece from file address (PR #94026)
@@ -768,3 +768,63 @@ TEST(DWARFExpression, ExtensionsDWO) {
testExpressionVendorExtensions(dwo_module_sp, *dwo_dwarf_unit);
}
+
+TEST_F(DWARFExpressionMockProcessTest, DW_OP_piece_file_addr) {
+ struct MockTarget : Target {
JDevlieghere wrote:
Very cool, thanks for the suggestion. I had to tweak it a little bit since this
is using an out parameter for the bugger. I couldn't find a declarative way to
do that, but if you know of one I'd love to learn more about it.
https://github.com/llvm/llvm-project/pull/94026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Support reading DW_OP_piece from file address (PR #94026)
https://github.com/JDevlieghere updated
https://github.com/llvm/llvm-project/pull/94026
>From 72844ebd5cf8f74f6db5d1c52d1f557ca942dbee Mon Sep 17 00:00:00 2001
From: Jonas Devlieghere
Date: Fri, 31 May 2024 12:21:28 -0700
Subject: [PATCH 1/2] [lldb] Support reading DW_OP_piece from file address
We received a bug report where someone was trying to print a global
variable without a process. This would succeed in a debug build but fail
in a on optimized build. We traced the issue back to the location being
described by a DW_OP_addr + DW_OP_piece.
The issue is that the DWARF expression evaluator only support reading
pieces from a load address. There's no reason it cannot do the same for
a file address, and indeed, that solves the problem.
I unsuccessfully tried to craft a test case to illustrate the original
example, using a global struct and trying to trick the compiler into
breaking it apart with SROA. Instead I wrote a unit test that uses a
mock target to read memory from.
rdar://127435923
---
lldb/include/lldb/Target/Target.h | 8 +--
lldb/source/Expression/DWARFExpression.cpp| 57 +++---
.../Expression/DWARFExpressionTest.cpp| 60 +++
3 files changed, 100 insertions(+), 25 deletions(-)
diff --git a/lldb/include/lldb/Target/Target.h
b/lldb/include/lldb/Target/Target.h
index 7ad9f33586054..792a4caa76e2d 100644
--- a/lldb/include/lldb/Target/Target.h
+++ b/lldb/include/lldb/Target/Target.h
@@ -1077,9 +1077,9 @@ class Target : public
std::enable_shared_from_this,
// section, then read from the file cache
// 2 - if there is a process, then read from memory
// 3 - if there is no process, then read from the file cache
- size_t ReadMemory(const Address , void *dst, size_t dst_len,
-Status , bool force_live_memory = false,
-lldb::addr_t *load_addr_ptr = nullptr);
+ virtual size_t ReadMemory(const Address , void *dst, size_t dst_len,
+Status , bool force_live_memory = false,
+lldb::addr_t *load_addr_ptr = nullptr);
size_t ReadCStringFromMemory(const Address , std::string _str,
Status , bool force_live_memory = false);
@@ -1615,7 +1615,7 @@ class Target : public
std::enable_shared_from_this,
TargetStats () { return m_stats; }
-private:
+protected:
/// Construct with optional file and arch.
///
/// This member is private. Clients must use
diff --git a/lldb/source/Expression/DWARFExpression.cpp
b/lldb/source/Expression/DWARFExpression.cpp
index c061fd1140fff..326be0d683804 100644
--- a/lldb/source/Expression/DWARFExpression.cpp
+++ b/lldb/source/Expression/DWARFExpression.cpp
@@ -2123,23 +2123,22 @@ bool DWARFExpression::Evaluate(
const Value::ValueType curr_piece_source_value_type =
curr_piece_source_value.GetValueType();
+ Scalar = curr_piece_source_value.GetScalar();
+ const lldb::addr_t addr = scalar.ULongLong(LLDB_INVALID_ADDRESS);
switch (curr_piece_source_value_type) {
case Value::ValueType::Invalid:
return false;
case Value::ValueType::LoadAddress:
if (process) {
if (curr_piece.ResizeData(piece_byte_size) == piece_byte_size) {
-lldb::addr_t load_addr =
-curr_piece_source_value.GetScalar().ULongLong(
-LLDB_INVALID_ADDRESS);
-if (process->ReadMemory(
-load_addr, curr_piece.GetBuffer().GetBytes(),
-piece_byte_size, error) != piece_byte_size) {
+if (process->ReadMemory(addr,
curr_piece.GetBuffer().GetBytes(),
+piece_byte_size,
+error) != piece_byte_size) {
if (error_ptr)
error_ptr->SetErrorStringWithFormat(
"failed to read memory DW_OP_piece(%" PRIu64
-") from 0x%" PRIx64,
-piece_byte_size, load_addr);
+") from load address 0x%" PRIx64,
+piece_byte_size, addr);
return false;
}
} else {
@@ -2153,26 +2152,42 @@ bool DWARFExpression::Evaluate(
}
break;
- case Value::ValueType::FileAddress:
- case Value::ValueType::HostAddress:
-if (error_ptr) {
- lldb::addr_t addr =
curr_piece_source_value.GetScalar().ULongLong(
- LLDB_INVALID_ADDRESS);
+ case Value::ValueType::FileAddress: {
+if (target) {
+ if (curr_piece.ResizeData(piece_byte_size) == piece_byte_size) {
+if (target->ReadMemory(addr, curr_piece.GetBuffer().GetBytes(),
+ piece_byte_size, error,
+
[Lldb-commits] [lldb] [lldb] Support reading DW_OP_piece from file address (PR #94026)
https://github.com/labath commented: Can't say I'm thrilled by the addition of a random virtual method, but I don't want to stand in the way. I'll just note that there is an alternative - in the form of writing a minimal real target in assembly. In fact, I think `test/Shell/SymbolFile/DWARF/x86/DW_OP_piece-struct.s` has all of the boilerplate, so all you'd need is to replace `DW_OP_stack_value` with `DW_OP_addr`. https://github.com/llvm/llvm-project/pull/94026 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Support reading DW_OP_piece from file address (PR #94026)
https://github.com/labath edited https://github.com/llvm/llvm-project/pull/94026 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Support reading DW_OP_piece from file address (PR #94026)
@@ -768,3 +768,63 @@ TEST(DWARFExpression, ExtensionsDWO) {
testExpressionVendorExtensions(dwo_module_sp, *dwo_dwarf_unit);
}
+
+TEST_F(DWARFExpressionMockProcessTest, DW_OP_piece_file_addr) {
+ struct MockTarget : Target {
labath wrote:
You could consider using gmock with [this
kind](https://github.com/llvm/llvm-project/blob/main/lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h#L73)
of a pattern that would enable you to write `EXPECT_CALL(target,
ReadMemory(0x40, 1)).WillOnce(Return(ByMove(std::vector{0x11})));`
https://github.com/llvm/llvm-project/pull/94026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Support reading DW_OP_piece from file address (PR #94026)
@@ -2153,26 +2152,42 @@ bool DWARFExpression::Evaluate(
}
break;
- case Value::ValueType::FileAddress:
- case Value::ValueType::HostAddress:
-if (error_ptr) {
- lldb::addr_t addr =
curr_piece_source_value.GetScalar().ULongLong(
- LLDB_INVALID_ADDRESS);
+ case Value::ValueType::FileAddress: {
labath wrote:
I'm wondering if it would be possible to use a single implementation for these
two. Target::ReadMemory will call Process::ReadMemory if it was a process
available (and the `force_live_memory` precisely so that one can require this
behavior).
https://github.com/llvm/llvm-project/pull/94026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Support reading DW_OP_piece from file address (PR #94026)
@@ -768,3 +768,63 @@ TEST(DWARFExpression, ExtensionsDWO) {
testExpressionVendorExtensions(dwo_module_sp, *dwo_dwarf_unit);
}
+
+TEST_F(DWARFExpressionMockProcessTest, DW_OP_piece_file_addr) {
+ struct MockTarget : Target {
+MockTarget(Debugger , const ArchSpec _arch,
+ const lldb::PlatformSP _sp)
+: Target(debugger, target_arch, platform_sp, true) {}
+
+size_t ReadMemory(const Address , void *dst, size_t dst_len,
+ Status , bool force_live_memory = false,
+ lldb::addr_t *load_addr_ptr = nullptr) override {
+ // We expected to be called in a very specific way.
+ assert(dst_len == 1);
+ assert(addr.GetOffset() == 0x40 || addr.GetOffset() == 0x50);
+
+ if (addr.GetOffset() == 0x40)
+((uint8_t *)dst)[0] = 0x11;
+
+ if (addr.GetOffset() == 0x50)
+((uint8_t *)dst)[0] = 0x22;
+
+ return 1;
+}
+ };
+
+ // Set up a mock process.
+ ArchSpec arch("i386-pc-linux");
+ Platform::SetHostPlatform(
+ platform_linux::PlatformLinux::CreateInstance(true, ));
+ lldb::DebuggerSP debugger_sp = Debugger::CreateInstance();
+ ASSERT_TRUE(debugger_sp);
+ lldb::PlatformSP platform_sp;
+ lldb::TargetSP target_sp =
+ std::make_shared(*debugger_sp, arch, platform_sp);
+ ASSERT_TRUE(target_sp);
+ ASSERT_TRUE(target_sp->GetArchitecture().IsValid());
+
+ ExecutionContext exe_ctx(target_sp, false);
+
+ uint8_t expr[] = {DW_OP_addr, 0x40, 0x0, 0x0, 0x0, DW_OP_piece, 1,
+DW_OP_addr, 0x50, 0x0, 0x0, 0x0, DW_OP_piece, 1};
+ DataExtractor extractor(expr, sizeof(expr), lldb::eByteOrderLittle,
+ /*addr_size*/ 4);
+ Value result;
+ Status status;
+ ASSERT_TRUE(DWARFExpression::Evaluate(
+ _ctx, /*reg_ctx*/ nullptr, /*module_sp*/ {}, extractor,
+ /*unit*/ nullptr, lldb::eRegisterKindLLDB,
+ /*initial_value_ptr*/ nullptr,
+ /*object_address_ptr*/ nullptr, result, ))
+ << status.ToError();
+
+ ASSERT_EQ(result.GetValueType(), Value::ValueType::HostAddress);
+
+ DataBufferHeap = result.GetBuffer();
+ ASSERT_EQ(buf.GetByteSize(), 2U);
+
+ const uint8_t *bytes = buf.GetBytes();
+ EXPECT_EQ(bytes[0], 0x11);
+ EXPECT_EQ(bytes[1], 0x22);
labath wrote:
`ASSERT_THAT(result.GetBuffer().GetData(), testing::ElementsAre(0x11, 0x22));`
(or something along those lines, may need some massaging to compile)
https://github.com/llvm/llvm-project/pull/94026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Support reading DW_OP_piece from file address (PR #94026)
https://github.com/JDevlieghere edited https://github.com/llvm/llvm-project/pull/94026 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Support reading DW_OP_piece from file address (PR #94026)
@@ -768,3 +768,63 @@ TEST(DWARFExpression, ExtensionsDWO) {
testExpressionVendorExtensions(dwo_module_sp, *dwo_dwarf_unit);
}
+
+TEST_F(DWARFExpressionMockProcessTest, DW_OP_piece_file_addr) {
+ struct MockTarget : Target {
+MockTarget(Debugger , const ArchSpec _arch,
+ const lldb::PlatformSP _sp)
+: Target(debugger, target_arch, platform_sp, true) {}
+
+size_t ReadMemory(const Address , void *dst, size_t dst_len,
+ Status , bool force_live_memory = false,
+ lldb::addr_t *load_addr_ptr = nullptr) override {
+ // We expected to be called in a very specific way.
+ assert(dst_len == 1);
+ assert(addr.GetOffset() == 0x40 || addr.GetOffset() == 0x50);
+
+ if (addr.GetOffset() == 0x40)
+((uint8_t *)dst)[0] = 0x11;
+
+ if (addr.GetOffset() == 0x50)
+((uint8_t *)dst)[0] = 0x22;
+
+ return 1;
+}
+ };
+
+ // Set up a mock process.
+ ArchSpec arch("i386-pc-linux");
+ Platform::SetHostPlatform(
+ platform_linux::PlatformLinux::CreateInstance(true, ));
+ lldb::DebuggerSP debugger_sp = Debugger::CreateInstance();
+ ASSERT_TRUE(debugger_sp);
+ lldb::PlatformSP platform_sp;
+ lldb::TargetSP target_sp =
+ std::make_shared(*debugger_sp, arch, platform_sp);
+ ASSERT_TRUE(target_sp);
+ ASSERT_TRUE(target_sp->GetArchitecture().IsValid());
+
+ ExecutionContext exe_ctx(target_sp, false);
+
+ uint8_t expr[] = {DW_OP_addr, 0x40, 0x0, 0x0, 0x0, DW_OP_piece, 1,
+DW_OP_addr, 0x50, 0x0, 0x0, 0x0, DW_OP_piece, 1};
+ DataExtractor extractor(expr, sizeof(expr), lldb::eByteOrderLittle,
+ /*addr_size*/ 4);
+ Value result;
+ Status status;
+ ASSERT_TRUE(DWARFExpression::Evaluate(
+ _ctx, /*reg_ctx*/ nullptr, /*module_sp*/ {}, extractor,
+ /*unit*/ nullptr, lldb::eRegisterKindLLDB,
+ /*initial_value_ptr*/ nullptr,
+ /*object_address_ptr*/ nullptr, result, ))
+ << status.ToError();
+
+ ASSERT_EQ(result.GetValueType(), Value::ValueType::HostAddress);
JDevlieghere wrote:
I can see how this is confusing. This is the **result** of the expression,
which we read in pieces, and because it's non-contiguous, necessarily need to
store in host memory. It's unrelated to the Value representing the `DW_OP_addr`
on the stack in the DWARF expression.
https://github.com/llvm/llvm-project/pull/94026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Support reading DW_OP_piece from file address (PR #94026)
@@ -768,3 +768,63 @@ TEST(DWARFExpression, ExtensionsDWO) {
testExpressionVendorExtensions(dwo_module_sp, *dwo_dwarf_unit);
}
+
+TEST_F(DWARFExpressionMockProcessTest, DW_OP_piece_file_addr) {
+ struct MockTarget : Target {
+MockTarget(Debugger , const ArchSpec _arch,
+ const lldb::PlatformSP _sp)
+: Target(debugger, target_arch, platform_sp, true) {}
+
+size_t ReadMemory(const Address , void *dst, size_t dst_len,
+ Status , bool force_live_memory = false,
+ lldb::addr_t *load_addr_ptr = nullptr) override {
+ // We expected to be called in a very specific way.
+ assert(dst_len == 1);
+ assert(addr.GetOffset() == 0x40 || addr.GetOffset() == 0x50);
+
+ if (addr.GetOffset() == 0x40)
+((uint8_t *)dst)[0] = 0x11;
+
+ if (addr.GetOffset() == 0x50)
+((uint8_t *)dst)[0] = 0x22;
+
+ return 1;
+}
+ };
+
+ // Set up a mock process.
+ ArchSpec arch("i386-pc-linux");
+ Platform::SetHostPlatform(
+ platform_linux::PlatformLinux::CreateInstance(true, ));
+ lldb::DebuggerSP debugger_sp = Debugger::CreateInstance();
+ ASSERT_TRUE(debugger_sp);
+ lldb::PlatformSP platform_sp;
+ lldb::TargetSP target_sp =
+ std::make_shared(*debugger_sp, arch, platform_sp);
+ ASSERT_TRUE(target_sp);
+ ASSERT_TRUE(target_sp->GetArchitecture().IsValid());
+
+ ExecutionContext exe_ctx(target_sp, false);
+
+ uint8_t expr[] = {DW_OP_addr, 0x40, 0x0, 0x0, 0x0, DW_OP_piece, 1,
+DW_OP_addr, 0x50, 0x0, 0x0, 0x0, DW_OP_piece, 1};
+ DataExtractor extractor(expr, sizeof(expr), lldb::eByteOrderLittle,
+ /*addr_size*/ 4);
+ Value result;
+ Status status;
+ ASSERT_TRUE(DWARFExpression::Evaluate(
+ _ctx, /*reg_ctx*/ nullptr, /*module_sp*/ {}, extractor,
+ /*unit*/ nullptr, lldb::eRegisterKindLLDB,
+ /*initial_value_ptr*/ nullptr,
+ /*object_address_ptr*/ nullptr, result, ))
+ << status.ToError();
+
+ ASSERT_EQ(result.GetValueType(), Value::ValueType::HostAddress);
bulbazord wrote:
Trying to understand this test a bit better: It looks like in
DWARFExpression::Evaluate you primarily changed the
`Value::ValueType::FileAddress` case, but this asserts that the result's value
type should be a HostAddress. Is this correct? Or am I missing something?
https://github.com/llvm/llvm-project/pull/94026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Support reading DW_OP_piece from file address (PR #94026)
@@ -2153,26 +2152,41 @@ bool DWARFExpression::Evaluate(
}
break;
- case Value::ValueType::FileAddress:
- case Value::ValueType::HostAddress:
-if (error_ptr) {
- lldb::addr_t addr =
curr_piece_source_value.GetScalar().ULongLong(
- LLDB_INVALID_ADDRESS);
+ case Value::ValueType::FileAddress: {
+if (target) {
+ if (curr_piece.ResizeData(piece_byte_size) == piece_byte_size) {
+if (target->ReadMemory(addr, curr_piece.GetBuffer().GetBytes(),
+ piece_byte_size, error,
+ /*force_live_memory=*/false) !=
+piece_byte_size) {
+ if (error_ptr)
+error_ptr->SetErrorStringWithFormat(
+"failed to read memory DW_OP_piece(%" PRIu64
+") from file address 0x%" PRIx64,
+piece_byte_size, addr);
+ return false;
+}
+ } else {
+if (error_ptr)
JDevlieghere wrote:
Yeah, absolutely.
https://github.com/llvm/llvm-project/pull/94026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Support reading DW_OP_piece from file address (PR #94026)
@@ -2153,26 +2152,41 @@ bool DWARFExpression::Evaluate(
}
break;
- case Value::ValueType::FileAddress:
- case Value::ValueType::HostAddress:
-if (error_ptr) {
- lldb::addr_t addr =
curr_piece_source_value.GetScalar().ULongLong(
- LLDB_INVALID_ADDRESS);
+ case Value::ValueType::FileAddress: {
+if (target) {
+ if (curr_piece.ResizeData(piece_byte_size) == piece_byte_size) {
+if (target->ReadMemory(addr, curr_piece.GetBuffer().GetBytes(),
+ piece_byte_size, error,
+ /*force_live_memory=*/false) !=
+piece_byte_size) {
+ if (error_ptr)
+error_ptr->SetErrorStringWithFormat(
+"failed to read memory DW_OP_piece(%" PRIu64
+") from file address 0x%" PRIx64,
+piece_byte_size, addr);
+ return false;
+}
+ } else {
+if (error_ptr)
adrian-prantl wrote:
We need to convert this function to Expected, too, at some point.
https://github.com/llvm/llvm-project/pull/94026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Support reading DW_OP_piece from file address (PR #94026)
https://github.com/adrian-prantl approved this pull request. https://github.com/llvm/llvm-project/pull/94026 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Support reading DW_OP_piece from file address (PR #94026)
https://github.com/adrian-prantl edited https://github.com/llvm/llvm-project/pull/94026 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Support reading DW_OP_piece from file address (PR #94026)
https://github.com/JDevlieghere updated
https://github.com/llvm/llvm-project/pull/94026
>From 72844ebd5cf8f74f6db5d1c52d1f557ca942dbee Mon Sep 17 00:00:00 2001
From: Jonas Devlieghere
Date: Fri, 31 May 2024 12:21:28 -0700
Subject: [PATCH] [lldb] Support reading DW_OP_piece from file address
We received a bug report where someone was trying to print a global
variable without a process. This would succeed in a debug build but fail
in a on optimized build. We traced the issue back to the location being
described by a DW_OP_addr + DW_OP_piece.
The issue is that the DWARF expression evaluator only support reading
pieces from a load address. There's no reason it cannot do the same for
a file address, and indeed, that solves the problem.
I unsuccessfully tried to craft a test case to illustrate the original
example, using a global struct and trying to trick the compiler into
breaking it apart with SROA. Instead I wrote a unit test that uses a
mock target to read memory from.
rdar://127435923
---
lldb/include/lldb/Target/Target.h | 8 +--
lldb/source/Expression/DWARFExpression.cpp| 57 +++---
.../Expression/DWARFExpressionTest.cpp| 60 +++
3 files changed, 100 insertions(+), 25 deletions(-)
diff --git a/lldb/include/lldb/Target/Target.h
b/lldb/include/lldb/Target/Target.h
index 7ad9f33586054..792a4caa76e2d 100644
--- a/lldb/include/lldb/Target/Target.h
+++ b/lldb/include/lldb/Target/Target.h
@@ -1077,9 +1077,9 @@ class Target : public
std::enable_shared_from_this,
// section, then read from the file cache
// 2 - if there is a process, then read from memory
// 3 - if there is no process, then read from the file cache
- size_t ReadMemory(const Address , void *dst, size_t dst_len,
-Status , bool force_live_memory = false,
-lldb::addr_t *load_addr_ptr = nullptr);
+ virtual size_t ReadMemory(const Address , void *dst, size_t dst_len,
+Status , bool force_live_memory = false,
+lldb::addr_t *load_addr_ptr = nullptr);
size_t ReadCStringFromMemory(const Address , std::string _str,
Status , bool force_live_memory = false);
@@ -1615,7 +1615,7 @@ class Target : public
std::enable_shared_from_this,
TargetStats () { return m_stats; }
-private:
+protected:
/// Construct with optional file and arch.
///
/// This member is private. Clients must use
diff --git a/lldb/source/Expression/DWARFExpression.cpp
b/lldb/source/Expression/DWARFExpression.cpp
index c061fd1140fff..326be0d683804 100644
--- a/lldb/source/Expression/DWARFExpression.cpp
+++ b/lldb/source/Expression/DWARFExpression.cpp
@@ -2123,23 +2123,22 @@ bool DWARFExpression::Evaluate(
const Value::ValueType curr_piece_source_value_type =
curr_piece_source_value.GetValueType();
+ Scalar = curr_piece_source_value.GetScalar();
+ const lldb::addr_t addr = scalar.ULongLong(LLDB_INVALID_ADDRESS);
switch (curr_piece_source_value_type) {
case Value::ValueType::Invalid:
return false;
case Value::ValueType::LoadAddress:
if (process) {
if (curr_piece.ResizeData(piece_byte_size) == piece_byte_size) {
-lldb::addr_t load_addr =
-curr_piece_source_value.GetScalar().ULongLong(
-LLDB_INVALID_ADDRESS);
-if (process->ReadMemory(
-load_addr, curr_piece.GetBuffer().GetBytes(),
-piece_byte_size, error) != piece_byte_size) {
+if (process->ReadMemory(addr,
curr_piece.GetBuffer().GetBytes(),
+piece_byte_size,
+error) != piece_byte_size) {
if (error_ptr)
error_ptr->SetErrorStringWithFormat(
"failed to read memory DW_OP_piece(%" PRIu64
-") from 0x%" PRIx64,
-piece_byte_size, load_addr);
+") from load address 0x%" PRIx64,
+piece_byte_size, addr);
return false;
}
} else {
@@ -2153,26 +2152,42 @@ bool DWARFExpression::Evaluate(
}
break;
- case Value::ValueType::FileAddress:
- case Value::ValueType::HostAddress:
-if (error_ptr) {
- lldb::addr_t addr =
curr_piece_source_value.GetScalar().ULongLong(
- LLDB_INVALID_ADDRESS);
+ case Value::ValueType::FileAddress: {
+if (target) {
+ if (curr_piece.ResizeData(piece_byte_size) == piece_byte_size) {
+if (target->ReadMemory(addr, curr_piece.GetBuffer().GetBytes(),
+ piece_byte_size, error,
+
[Lldb-commits] [lldb] [lldb] Support reading DW_OP_piece from file address (PR #94026)
@@ -1615,7 +1615,7 @@ class Target : public
std::enable_shared_from_this,
TargetStats () { return m_stats; }
-private:
+protected:
JDevlieghere wrote:
If you're wondering, this is so the unit test can access the private
constructor.
https://github.com/llvm/llvm-project/pull/94026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Support reading DW_OP_piece from file address (PR #94026)
https://github.com/JDevlieghere updated
https://github.com/llvm/llvm-project/pull/94026
>From 3aaf98ebce78fd376d33bd0aeb4d4c57762ea63a Mon Sep 17 00:00:00 2001
From: Jonas Devlieghere
Date: Fri, 31 May 2024 12:21:28 -0700
Subject: [PATCH] [lldb] Support reading DW_OP_piece from file address
We received a bug report where someone was trying to print a global
variable without a process. This would succeed in a debug build but fail
in a on optimized build. We traced the issue back to the location being
described by a DW_OP_addr + DW_OP_piece.
The issue is that the DWARF expression evaluator only support reading
pieces from a load address. There's no reason it cannot do the same for
a file address, and indeed, that solves the problem.
I unsuccessfully tried to craft a test case to illustrate the original
example, using a global struct and trying to trick the compiler into
breaking it apart with SROA. Instead I wrote a unit test that uses a
mock target to read memory from.
rdar://127435923
---
lldb/include/lldb/Target/Target.h | 8 +--
lldb/source/Expression/DWARFExpression.cpp| 58 +++---
.../Expression/DWARFExpressionTest.cpp| 60 +++
3 files changed, 100 insertions(+), 26 deletions(-)
diff --git a/lldb/include/lldb/Target/Target.h
b/lldb/include/lldb/Target/Target.h
index 7ad9f33586054..792a4caa76e2d 100644
--- a/lldb/include/lldb/Target/Target.h
+++ b/lldb/include/lldb/Target/Target.h
@@ -1077,9 +1077,9 @@ class Target : public
std::enable_shared_from_this,
// section, then read from the file cache
// 2 - if there is a process, then read from memory
// 3 - if there is no process, then read from the file cache
- size_t ReadMemory(const Address , void *dst, size_t dst_len,
-Status , bool force_live_memory = false,
-lldb::addr_t *load_addr_ptr = nullptr);
+ virtual size_t ReadMemory(const Address , void *dst, size_t dst_len,
+Status , bool force_live_memory = false,
+lldb::addr_t *load_addr_ptr = nullptr);
size_t ReadCStringFromMemory(const Address , std::string _str,
Status , bool force_live_memory = false);
@@ -1615,7 +1615,7 @@ class Target : public
std::enable_shared_from_this,
TargetStats () { return m_stats; }
-private:
+protected:
/// Construct with optional file and arch.
///
/// This member is private. Clients must use
diff --git a/lldb/source/Expression/DWARFExpression.cpp
b/lldb/source/Expression/DWARFExpression.cpp
index c061fd1140fff..4e2942aa3ebf7 100644
--- a/lldb/source/Expression/DWARFExpression.cpp
+++ b/lldb/source/Expression/DWARFExpression.cpp
@@ -2123,23 +2123,22 @@ bool DWARFExpression::Evaluate(
const Value::ValueType curr_piece_source_value_type =
curr_piece_source_value.GetValueType();
+ Scalar = curr_piece_source_value.GetScalar();
+ const lldb::addr_t addr = scalar.ULongLong(LLDB_INVALID_ADDRESS);
switch (curr_piece_source_value_type) {
case Value::ValueType::Invalid:
return false;
case Value::ValueType::LoadAddress:
if (process) {
if (curr_piece.ResizeData(piece_byte_size) == piece_byte_size) {
-lldb::addr_t load_addr =
-curr_piece_source_value.GetScalar().ULongLong(
-LLDB_INVALID_ADDRESS);
-if (process->ReadMemory(
-load_addr, curr_piece.GetBuffer().GetBytes(),
-piece_byte_size, error) != piece_byte_size) {
+if (process->ReadMemory(addr,
curr_piece.GetBuffer().GetBytes(),
+piece_byte_size,
+error) != piece_byte_size) {
if (error_ptr)
error_ptr->SetErrorStringWithFormat(
"failed to read memory DW_OP_piece(%" PRIu64
-") from 0x%" PRIx64,
-piece_byte_size, load_addr);
+") from load address 0x%" PRIx64,
+piece_byte_size, addr);
return false;
}
} else {
@@ -2153,26 +2152,41 @@ bool DWARFExpression::Evaluate(
}
break;
- case Value::ValueType::FileAddress:
- case Value::ValueType::HostAddress:
-if (error_ptr) {
- lldb::addr_t addr =
curr_piece_source_value.GetScalar().ULongLong(
- LLDB_INVALID_ADDRESS);
+ case Value::ValueType::FileAddress: {
+if (target) {
+ if (curr_piece.ResizeData(piece_byte_size) == piece_byte_size) {
+if (target->ReadMemory(addr, curr_piece.GetBuffer().GetBytes(),
+ piece_byte_size, error,
+
[Lldb-commits] [lldb] [lldb] Support reading DW_OP_piece from file address (PR #94026)
llvmbot wrote:
@llvm/pr-subscribers-lldb
Author: Jonas Devlieghere (JDevlieghere)
Changes
We received a bug report where someone was trying to print a global variable
without a process. This would succeed in a debug build but fail in a on
optimized build. We traced the issue back to the location being described by a
DW_OP_addr + DW_OP_piece.
The issue is that the DWARF expression evaluator only support reading pieces
from a load address. There's no reason it cannot do the same for a file
address, and indeed, that solves the problem.
I unsuccessfully tried to craft a test case to illustrate the original example,
using a global struct and trying to trick the compiler into breaking it apart
with SROA. Instead I wrote a unit test that uses a mock target to read memory
from.
rdar://127435923
---
Full diff: https://github.com/llvm/llvm-project/pull/94026.diff
3 Files Affected:
- (modified) lldb/include/lldb/Target/Target.h (+4-4)
- (modified) lldb/source/Expression/DWARFExpression.cpp (+33-13)
- (modified) lldb/unittests/Expression/DWARFExpressionTest.cpp (+60)
``diff
diff --git a/lldb/include/lldb/Target/Target.h
b/lldb/include/lldb/Target/Target.h
index 7ad9f33586054..792a4caa76e2d 100644
--- a/lldb/include/lldb/Target/Target.h
+++ b/lldb/include/lldb/Target/Target.h
@@ -1077,9 +1077,9 @@ class Target : public
std::enable_shared_from_this,
// section, then read from the file cache
// 2 - if there is a process, then read from memory
// 3 - if there is no process, then read from the file cache
- size_t ReadMemory(const Address , void *dst, size_t dst_len,
-Status , bool force_live_memory = false,
-lldb::addr_t *load_addr_ptr = nullptr);
+ virtual size_t ReadMemory(const Address , void *dst, size_t dst_len,
+Status , bool force_live_memory = false,
+lldb::addr_t *load_addr_ptr = nullptr);
size_t ReadCStringFromMemory(const Address , std::string _str,
Status , bool force_live_memory = false);
@@ -1615,7 +1615,7 @@ class Target : public
std::enable_shared_from_this,
TargetStats () { return m_stats; }
-private:
+protected:
/// Construct with optional file and arch.
///
/// This member is private. Clients must use
diff --git a/lldb/source/Expression/DWARFExpression.cpp
b/lldb/source/Expression/DWARFExpression.cpp
index c061fd1140fff..aed550e52c579 100644
--- a/lldb/source/Expression/DWARFExpression.cpp
+++ b/lldb/source/Expression/DWARFExpression.cpp
@@ -2153,21 +2153,41 @@ bool DWARFExpression::Evaluate(
}
break;
- case Value::ValueType::FileAddress:
- case Value::ValueType::HostAddress:
-if (error_ptr) {
- lldb::addr_t addr =
curr_piece_source_value.GetScalar().ULongLong(
- LLDB_INVALID_ADDRESS);
+ case Value::ValueType::FileAddress: {
+lldb::addr_t addr = curr_piece_source_value.GetScalar().ULongLong(
+LLDB_INVALID_ADDRESS);
+if (target) {
+ if (curr_piece.ResizeData(piece_byte_size) == piece_byte_size) {
+if (target->ReadMemory(addr, curr_piece.GetBuffer().GetBytes(),
+ piece_byte_size, error,
+ /*force_live_memory=*/false) !=
+piece_byte_size) {
+ if (error_ptr)
+error_ptr->SetErrorStringWithFormat(
+"failed to read memory DW_OP_piece(%" PRIu64
+") from load address 0x%" PRIx64,
+piece_byte_size, addr);
+ return false;
+}
+ } else {
+if (error_ptr)
+ error_ptr->SetErrorStringWithFormat(
+ "failed to read memory DW_OP_piece(%" PRIu64
+ ") from load address 0x%" PRIx64,
+ piece_byte_size, addr);
+return false;
+ }
+}
+ } break;
+ case Value::ValueType::HostAddress: {
+lldb::addr_t addr = curr_piece_source_value.GetScalar().ULongLong(
+LLDB_INVALID_ADDRESS);
+if (error_ptr)
error_ptr->SetErrorStringWithFormat(
"failed to read memory DW_OP_piece(%" PRIu64
- ") from %s address 0x%" PRIx64,
- piece_byte_size, curr_piece_source_value.GetValueType() ==
- Value::ValueType::FileAddress
- ? "file"
- : "host",
- addr);
-}
-return false;
+ ") from host address 0x%" PRIx64,
+ piece_byte_size, addr);
+ } break;
case Value::ValueType::Scalar: {
[Lldb-commits] [lldb] [lldb] Support reading DW_OP_piece from file address (PR #94026)
https://github.com/JDevlieghere created
https://github.com/llvm/llvm-project/pull/94026
We received a bug report where someone was trying to print a global variable
without a process. This would succeed in a debug build but fail in a on
optimized build. We traced the issue back to the location being described by a
DW_OP_addr + DW_OP_piece.
The issue is that the DWARF expression evaluator only support reading pieces
from a load address. There's no reason it cannot do the same for a file
address, and indeed, that solves the problem.
I unsuccessfully tried to craft a test case to illustrate the original example,
using a global struct and trying to trick the compiler into breaking it apart
with SROA. Instead I wrote a unit test that uses a mock target to read memory
from.
rdar://127435923
>From 0487856604f97d94d1be2502d9c41080064e2a64 Mon Sep 17 00:00:00 2001
From: Jonas Devlieghere
Date: Fri, 31 May 2024 12:21:28 -0700
Subject: [PATCH] [lldb] Support reading DW_OP_piece from file address
We received a bug report where someone was trying to print a global
variable without a process. This would succeed in a debug build but fail
in a on optimized build. We traced the issue back to the location being
described by a DW_OP_addr + DW_OP_piece.
The issue is that the DWARF expression evaluator only support reading
pieces from a load address. There's no reason it cannot do the same for
a file address, and indeed, that solves the problem.
I unsuccessfully tried to craft a test case to illustrate the original
example, using a global struct and trying to trick the compiler into
breaking it apart with SROA. Instead I wrote a unit test that uses a
mock target to read memory from.
rdar://127435923
---
lldb/include/lldb/Target/Target.h | 8 +--
lldb/source/Expression/DWARFExpression.cpp| 46 ++
.../Expression/DWARFExpressionTest.cpp| 60 +++
3 files changed, 97 insertions(+), 17 deletions(-)
diff --git a/lldb/include/lldb/Target/Target.h
b/lldb/include/lldb/Target/Target.h
index 7ad9f33586054..792a4caa76e2d 100644
--- a/lldb/include/lldb/Target/Target.h
+++ b/lldb/include/lldb/Target/Target.h
@@ -1077,9 +1077,9 @@ class Target : public
std::enable_shared_from_this,
// section, then read from the file cache
// 2 - if there is a process, then read from memory
// 3 - if there is no process, then read from the file cache
- size_t ReadMemory(const Address , void *dst, size_t dst_len,
-Status , bool force_live_memory = false,
-lldb::addr_t *load_addr_ptr = nullptr);
+ virtual size_t ReadMemory(const Address , void *dst, size_t dst_len,
+Status , bool force_live_memory = false,
+lldb::addr_t *load_addr_ptr = nullptr);
size_t ReadCStringFromMemory(const Address , std::string _str,
Status , bool force_live_memory = false);
@@ -1615,7 +1615,7 @@ class Target : public
std::enable_shared_from_this,
TargetStats () { return m_stats; }
-private:
+protected:
/// Construct with optional file and arch.
///
/// This member is private. Clients must use
diff --git a/lldb/source/Expression/DWARFExpression.cpp
b/lldb/source/Expression/DWARFExpression.cpp
index c061fd1140fff..aed550e52c579 100644
--- a/lldb/source/Expression/DWARFExpression.cpp
+++ b/lldb/source/Expression/DWARFExpression.cpp
@@ -2153,21 +2153,41 @@ bool DWARFExpression::Evaluate(
}
break;
- case Value::ValueType::FileAddress:
- case Value::ValueType::HostAddress:
-if (error_ptr) {
- lldb::addr_t addr =
curr_piece_source_value.GetScalar().ULongLong(
- LLDB_INVALID_ADDRESS);
+ case Value::ValueType::FileAddress: {
+lldb::addr_t addr = curr_piece_source_value.GetScalar().ULongLong(
+LLDB_INVALID_ADDRESS);
+if (target) {
+ if (curr_piece.ResizeData(piece_byte_size) == piece_byte_size) {
+if (target->ReadMemory(addr, curr_piece.GetBuffer().GetBytes(),
+ piece_byte_size, error,
+ /*force_live_memory=*/false) !=
+piece_byte_size) {
+ if (error_ptr)
+error_ptr->SetErrorStringWithFormat(
+"failed to read memory DW_OP_piece(%" PRIu64
+") from load address 0x%" PRIx64,
+piece_byte_size, addr);
+ return false;
+}
+ } else {
+if (error_ptr)
+ error_ptr->SetErrorStringWithFormat(
+ "failed to read memory DW_OP_piece(%" PRIu64
+ ") from load address 0x%" PRIx64,
+ piece_byte_size, addr);
+return false;
+ }
+}
+
