[lldb] Fix evaluating expressions without JIT in an object context #145599
Conversation
|
@llvm/pr-subscribers-lldb Author: Igor Kudrin (igorkudrin) ChangesIf a server does not support allocating memory in an inferior process or when debugging a core file, evaluating an expression in the context of a value object results in an error: Such expressions require a live address to be stored in the value object. However, As an unintended bonus, the patch also fixes a FIXME case in Full diff: https://github.com/llvm/llvm-project/pull/145599.diff 6 Files Affected:
diff --git a/lldb/source/Expression/Materializer.cpp b/lldb/source/Expression/Materializer.cpp
index 79c804c6c4214..5f0dcd42289f6 100644
--- a/lldb/source/Expression/Materializer.cpp
+++ b/lldb/source/Expression/Materializer.cpp
@@ -329,22 +329,10 @@ class EntityPersistentVariable : public Materializer::Entity {
return;
}
- lldb::ProcessSP process_sp =
- map.GetBestExecutionContextScope()->CalculateProcess();
- if (!process_sp || !process_sp->CanJIT()) {
- // Allocations are not persistent so persistent variables cannot stay
- // materialized.
-
- m_persistent_variable_sp->m_flags |=
- ExpressionVariable::EVNeedsAllocation;
-
- DestroyAllocation(map, err);
- if (!err.Success())
- return;
- } else if (m_persistent_variable_sp->m_flags &
- ExpressionVariable::EVNeedsAllocation &&
- !(m_persistent_variable_sp->m_flags &
- ExpressionVariable::EVKeepInTarget)) {
+ if (m_persistent_variable_sp->m_flags &
+ ExpressionVariable::EVNeedsAllocation &&
+ !(m_persistent_variable_sp->m_flags &
+ ExpressionVariable::EVKeepInTarget)) {
DestroyAllocation(map, err);
if (!err.Success())
return;
@@ -1086,9 +1074,8 @@ class EntityResultVariable : public Materializer::Entity {
m_delegate->DidDematerialize(ret);
}
- bool can_persist =
- (m_is_program_reference && process_sp && process_sp->CanJIT() &&
- !(address >= frame_bottom && address < frame_top));
+ bool can_persist = m_is_program_reference &&
+ !(address >= frame_bottom && address < frame_top);
if (can_persist && m_keep_in_memory) {
ret->m_live_sp = ValueObjectConstResult::Create(exe_scope, m_type, name,
@@ -1118,7 +1105,9 @@ class EntityResultVariable : public Materializer::Entity {
map.Free(m_temporary_allocation, free_error);
}
} else {
- ret->m_flags |= ExpressionVariable::EVIsLLDBAllocated;
+ ret->m_flags |= m_is_program_reference
+ ? ExpressionVariable::EVIsProgramReference
+ : ExpressionVariable::EVIsLLDBAllocated;
}
m_temporary_allocation = LLDB_INVALID_ADDRESS;
diff --git a/lldb/test/API/functionalities/postmortem/elf-core/expr/TestExpr.py b/lldb/test/API/functionalities/postmortem/elf-core/expr/TestExpr.py
new file mode 100644
index 0000000000000..a4b536ba656e2
--- /dev/null
+++ b/lldb/test/API/functionalities/postmortem/elf-core/expr/TestExpr.py
@@ -0,0 +1,50 @@
+"""
+Test evaluating expressions when debugging core file.
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+@skipIfLLVMTargetMissing("X86")
+class CoreExprTestCase(TestBase):
+
+ def test_result_var(self):
+ """Test that the result variable can be used in subsequent expressions."""
+
+ target = self.dbg.CreateTarget('linux-x86_64.out')
+ process = target.LoadCore('linux-x86_64.core')
+ self.assertTrue(process, PROCESS_IS_VALID)
+
+ self.expect_expr(
+ "outer",
+ result_type="Outer",
+ result_children=[ValueCheck(name="inner", type="Inner")],
+ )
+ self.expect_expr(
+ "$0.inner",
+ result_type="Inner",
+ result_children=[ValueCheck(name="val", type="int", value="5")],
+ )
+ self.expect_expr("$1.val", result_type="int", result_value="5")
+
+
+ def test_context_object(self):
+ """Tests expression evaluation in context of an object."""
+
+ target = self.dbg.CreateTarget('linux-x86_64.out')
+ process = target.LoadCore('linux-x86_64.core')
+ self.assertTrue(process, PROCESS_IS_VALID)
+
+ val_outer = self.expect_expr('outer', result_type='Outer')
+
+ val_inner = val_outer.EvaluateExpression('inner')
+ self.assertTrue(val_inner.IsValid())
+ self.assertEqual('Inner', val_inner.GetDisplayTypeName())
+
+ val_val = val_inner.EvaluateExpression("this->val")
+ self.assertTrue(val_val.IsValid())
+ self.assertEqual('int', val_val.GetDisplayTypeName())
+ self.assertEqual(val_val.GetValueAsSigned(), 5)
diff --git a/lldb/test/API/functionalities/postmortem/elf-core/expr/linux-x86_64.core b/lldb/test/API/functionalities/postmortem/elf-core/expr/linux-x86_64.core
new file mode 100644
index 0000000000000..3bd2916c64e9f
Binary files /dev/null and b/lldb/test/API/functionalities/postmortem/elf-core/expr/linux-x86_64.core differ
diff --git a/lldb/test/API/functionalities/postmortem/elf-core/expr/linux-x86_64.out b/lldb/test/API/functionalities/postmortem/elf-core/expr/linux-x86_64.out
new file mode 100755
index 0000000000000..33e60f025d210
Binary files /dev/null and b/lldb/test/API/functionalities/postmortem/elf-core/expr/linux-x86_64.out differ
diff --git a/lldb/test/API/functionalities/postmortem/elf-core/expr/main.cpp b/lldb/test/API/functionalities/postmortem/elf-core/expr/main.cpp
new file mode 100644
index 0000000000000..f5887559911e6
--- /dev/null
+++ b/lldb/test/API/functionalities/postmortem/elf-core/expr/main.cpp
@@ -0,0 +1,16 @@
+struct Inner {
+ Inner(int val) : val(val) {}
+ int val;
+};
+
+struct Outer {
+ Outer(int val) : inner(val) {}
+ Inner inner;
+};
+
+extern "C" void _start(void)
+{
+ Outer outer(5);
+ char *boom = (char *)0;
+ *boom = 47;
+}
diff --git a/lldb/test/API/lang/cpp/char8_t/TestCxxChar8_t.py b/lldb/test/API/lang/cpp/char8_t/TestCxxChar8_t.py
index 4eb5351eefc82..08f09b317b217 100644
--- a/lldb/test/API/lang/cpp/char8_t/TestCxxChar8_t.py
+++ b/lldb/test/API/lang/cpp/char8_t/TestCxxChar8_t.py
@@ -24,9 +24,7 @@ def test_without_process(self):
self.expect_expr("a", result_type="char8_t", result_summary="0x61 u8'a'")
self.expect_expr("ab", result_type="const char8_t *", result_summary='u8"你好"')
-
- # FIXME: This should work too.
- self.expect("expr abc", substrs=['u8"你好"'], matching=False)
+ self.expect_expr("abc", result_type="char8_t[9]", result_summary='u8"你好"')
@skipIf(compiler="clang", compiler_version=["<", "7.0"])
def test_with_process(self):
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
✅ With the latest revision this PR passed the Python code formatter. |
If a server does not support allocating memory in an inferior process or
when debugging a core file, evaluating an expression in the context of a
value object results in an error:
```
error: <lldb wrapper prefix>:43:1: use of undeclared identifier '$__lldb_class'
43 | $__lldb_class::$__lldb_expr(void *$__lldb_arg)
| ^
```
Such expressions require a live address to be stored in the value
object. However, `EntityResultVariable::Dematerialize()` only sets
`ret->m_live_sp` if JIT is available, even if the address points to the
process memory and no custom allocations were made. Similarly,
`EntityPersistentVariable::Dematerialize()` tries to deallocate memory
based on the same check, resulting in an error if the memory was not
previously allocated in `EntityPersistentVariable::Materialize()`.
As an unintended bonus, the patch also fixes a FIXME case in
`TestCxxChar8_t.py`.
igorkudrin
force-pushed
the
expr-result-no-jit
branch
from
9a56746 to
fac89bb
Compare
|
This makes sense. But the changes that you made are in code that's also used by persistent expression variables - not just persistent results. If you can't allocate memory, you can't make non-scalar persistent expression variables: This is failing in IR interpretation, before it gets to Dematerialization, so your change shouldn't matter for that right now. But we do support scalar persistent expression results in core files currently: (lldb) expr int $my_int = 5 and that should still work after your change. I see no reason why it wouldn't, but it would be good to add some use of persistent expression variables in your test as well as expression result variables to make sure. |
Thank you for the test. It revealed another execution path that had to be fixed. I've updated the patch and added your test. Please take a look. |
| lldb::addr_t IRMemoryMap::Malloc(size_t size, uint8_t alignment, | ||
| uint32_t permissions, AllocationPolicy policy, | ||
| bool zero_memory, Status &error) { | ||
| bool zero_memory, Status &error, |
There was a problem hiding this comment.
Can you put error last here? We generally put the error return last among the out parameters.
There was a problem hiding this comment.
That would mean more intrusive code changes because used_policy would become a required parameter. Maybe it would make more sense to switch to using llvm::Expected<> for this method?
|
That seems like a sensible strategy. I made one trivial comment about the ordering of parameters, but otherwise this LGTM. |
|
It isn't terribly important. We are switching to returning expected so if you think that looks nicer, feel free to do that. But otherwise, this is fine as it. |
jimingham
left a comment
jimingham
left a comment
There was a problem hiding this comment.
This one is ready then.
) This will make changes in #145599 a bit nicer.
This patch fixes updating persistent variables when memory cannot be allocated in an inferior process: ``` > lldb -c test.core (lldb) expr int $i = 5 (lldb) expr $i = 55 (int) $0 = 55 (lldb) expr $i (int) $i = 5 ``` With this patch, the last command prints: ``` (int) $i = 55 ``` The issue was introduced in llvm#145599.
This patch fixes updating persistent variables when memory cannot be allocated in an inferior process: ``` > lldb -c test.core (lldb) expr int $i = 5 (lldb) expr $i = 55 (int) $0 = 55 (lldb) expr $i (int) $i = 5 ``` With this patch, the last command prints: ``` (int) $i = 55 ``` The issue was introduced in #145599.
3 participants
If a server does not support allocating memory in an inferior process or when debugging a core file, evaluating an expression in the context of a value object results in an error:
Such expressions require a live address to be stored in the value object. However,
EntityResultVariable::Dematerialize()only setsret->m_live_spif JIT is available, even if the address points to the process memory and no custom allocations were made. Similarly,EntityPersistentVariable::Dematerialize()tries to deallocate memory based on the same check, resulting in an error if the memory was not previously allocated inEntityPersistentVariable::Materialize().As an unintended bonus, the patch also fixes a FIXME case in
TestCxxChar8_t.py.