[LLDB] Update DIL to handle smart pointers; add more tests. #143786
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
This updates the DIL implementation to handle smart pointers (accessing field members and dereferencing) in the same way the current 'frame variable' implementation does. It also adds tests for handling smart pointers, as well as some additional DIL tests.
Member
|
@llvm/pr-subscribers-lldb Author: None (cmtice) ChangesThis updates the DIL implementation to handle smart pointers (accessing field members and dereferencing) in the same way the current 'frame variable' implementation does. It also adds tests for handling smart pointers, as well as some additional DIL tests. Patch is 20.27 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/143786.diff 21 Files Affected:
diff --git a/lldb/source/ValueObject/DILEval.cpp b/lldb/source/ValueObject/DILEval.cpp
index c8cb54aa18a93..adc34e25766b3 100644
--- a/lldb/source/ValueObject/DILEval.cpp
+++ b/lldb/source/ValueObject/DILEval.cpp
@@ -253,6 +253,12 @@ Interpreter::Visit(const UnaryOpNode *node) {
rhs = dynamic_rhs;
lldb::ValueObjectSP child_sp = rhs->Dereference(error);
+ if (!child_sp && m_use_synthetic) {
+ if (lldb::ValueObjectSP synth_obj_sp = rhs->GetSyntheticValue()) {
+ error.Clear();
+ child_sp = synth_obj_sp->Dereference(error);
+ }
+ }
if (error.Fail())
return llvm::make_error<DILDiagnosticError>(m_expr, error.AsCString(),
node->GetLocation());
@@ -280,6 +286,7 @@ Interpreter::Visit(const MemberOfNode *node) {
auto base_or_err = Evaluate(node->GetBase());
if (!base_or_err)
return base_or_err;
+ bool expr_is_ptr = node->GetIsArrow();
lldb::ValueObjectSP base = *base_or_err;
// Perform some basic type & correctness checking.
@@ -319,11 +326,11 @@ Interpreter::Visit(const MemberOfNode *node) {
return llvm::make_error<DILDiagnosticError>(
m_expr, errMsg, node->GetLocation(), node->GetFieldName().size());
}
+ expr_is_ptr = false;
}
}
if (m_check_ptr_vs_member) {
- bool expr_is_ptr = node->GetIsArrow();
bool base_is_ptr = base->IsPointerType();
if (expr_is_ptr != base_is_ptr) {
diff --git a/lldb/test/API/commands/frame/var-dil/basics/BitField/Makefile b/lldb/test/API/commands/frame/var-dil/basics/BitField/Makefile
new file mode 100644
index 0000000000000..99998b20bcb05
--- /dev/null
+++ b/lldb/test/API/commands/frame/var-dil/basics/BitField/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
diff --git a/lldb/test/API/commands/frame/var-dil/basics/BitField/TestFrameVarDILBitField.py b/lldb/test/API/commands/frame/var-dil/basics/BitField/TestFrameVarDILBitField.py
new file mode 100644
index 0000000000000..8b08ec7f62d5c
--- /dev/null
+++ b/lldb/test/API/commands/frame/var-dil/basics/BitField/TestFrameVarDILBitField.py
@@ -0,0 +1,38 @@
+"""
+Make sure 'frame var' using DIL parser/evaluator works for bit fields.
+"""
+
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from lldbsuite.test import lldbutil
+
+import os
+import shutil
+import time
+
+class TestFrameVarDILBitField(TestBase):
+ # If your test case doesn't stress debug info, then
+ # set this to true. That way it won't be run once for
+ # each debug info format.
+ NO_DEBUG_INFO_TESTCASE = True
+
+ def test_frame_var(self):
+ self.build()
+ lldbutil.run_to_source_breakpoint(self, "Set a breakpoint here",
+ lldb.SBFileSpec("main.cpp"))
+
+ self.runCmd("settings set target.experimental.use-DIL true")
+ self.expect_var_path("bf.a", value="1023")
+ self.expect_var_path("bf.b", value="9")
+ self.expect_var_path("bf.c", value="false")
+ self.expect_var_path("bf.d", value="true")
+
+ self.expect_var_path("abf.a", value="1023")
+ self.expect_var_path("abf.b", value="'\\x0f'")
+ self.expect_var_path("abf.c", value="3")
+
+ # Perform an operation to ensure we actually read the value.
+ # Address-of is not allowed for bit-fields.
+ self.expect("frame variable '&bf.a'", error=True,
+ substrs=["'bf.a' doesn't have a valid address"])
diff --git a/lldb/test/API/commands/frame/var-dil/basics/BitField/main.cpp b/lldb/test/API/commands/frame/var-dil/basics/BitField/main.cpp
new file mode 100644
index 0000000000000..bd9b23a44ebbe
--- /dev/null
+++ b/lldb/test/API/commands/frame/var-dil/basics/BitField/main.cpp
@@ -0,0 +1,44 @@
+#include <cstdint>
+
+int
+main(int argc, char **argv)
+{
+ enum BitFieldEnum : uint32_t { kZero, kOne };
+
+ struct BitFieldStruct {
+ uint16_t a : 10;
+ uint32_t b : 4;
+ bool c : 1;
+ bool d : 1;
+ int32_t e : 32;
+ uint32_t f : 32;
+ uint32_t g : 31;
+ uint64_t h : 31;
+ uint64_t i : 33;
+ BitFieldEnum j : 10;
+ };
+
+ BitFieldStruct bf;
+ bf.a = 0b1111111111;
+ bf.b = 0b1001;
+ bf.c = 0b0;
+ bf.d = 0b1;
+ bf.e = 0b1;
+ bf.f = 0b1;
+ bf.g = 0b1;
+ bf.h = 0b1;
+ bf.i = 0b1;
+ bf.j = BitFieldEnum::kOne;
+
+ struct AlignedBitFieldStruct {
+ uint16_t a : 10;
+ uint8_t b : 4;
+ unsigned char : 0;
+ uint16_t c : 2;
+ };
+
+ uint32_t data = ~0;
+ AlignedBitFieldStruct abf = (AlignedBitFieldStruct&)data;
+
+ return 0; // Set a breakpoint here
+}
diff --git a/lldb/test/API/commands/frame/var-dil/basics/Indirection/Makefile b/lldb/test/API/commands/frame/var-dil/basics/Indirection/Makefile
new file mode 100644
index 0000000000000..99998b20bcb05
--- /dev/null
+++ b/lldb/test/API/commands/frame/var-dil/basics/Indirection/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
diff --git a/lldb/test/API/commands/frame/var-dil/basics/Indirection/TestFrameVarDILIndirection.py b/lldb/test/API/commands/frame/var-dil/basics/Indirection/TestFrameVarDILIndirection.py
new file mode 100644
index 0000000000000..13acc4232bd7c
--- /dev/null
+++ b/lldb/test/API/commands/frame/var-dil/basics/Indirection/TestFrameVarDILIndirection.py
@@ -0,0 +1,36 @@
+"""
+Make sure 'frame var' using DIL parser/evaluator works for indirection.
+"""
+
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from lldbsuite.test import lldbutil
+
+import os
+import shutil
+import time
+
+class TestFrameVarDILIndirection(TestBase):
+ # If your test case doesn't stress debug info, then
+ # set this to true. That way it won't be run once for
+ # each debug info format.
+ NO_DEBUG_INFO_TESTCASE = True
+
+ def test_frame_var(self):
+ self.build()
+ lldbutil.run_to_source_breakpoint(self, "Set a breakpoint here",
+ lldb.SBFileSpec("main.cpp"))
+
+ self.runCmd("settings set target.experimental.use-DIL true")
+ self.expect_var_path("*p", value="1")
+ self.expect_var_path("p", type="int *")
+ self.expect_var_path("*my_p", value="1")
+ self.expect_var_path("my_p", type="myp")
+ self.expect_var_path("*my_pr", type="int *")
+ self.expect_var_path("my_pr", type="mypr")
+
+ self.expect("frame variable '*1'", error=True,
+ substrs=["Unexpected token: <'1' (numeric_constant)>"])
+ self.expect("frame variable '*val'", error=True,
+ substrs=["dereference failed: not a pointer, reference or array type: (int) val"])
diff --git a/lldb/test/API/commands/frame/var-dil/basics/Indirection/main.cpp b/lldb/test/API/commands/frame/var-dil/basics/Indirection/main.cpp
new file mode 100644
index 0000000000000..827968a71519a
--- /dev/null
+++ b/lldb/test/API/commands/frame/var-dil/basics/Indirection/main.cpp
@@ -0,0 +1,14 @@
+int
+main(int argc, char **argv)
+{
+ int val = 1;
+ int* p = &val;
+
+ typedef int* myp;
+ myp my_p = &val;
+
+ typedef int*& mypr;
+ mypr my_pr = p;
+
+ return 0; // Set a breakpoint here
+}
diff --git a/lldb/test/API/commands/frame/var-dil/basics/PointerDereference/Makefile b/lldb/test/API/commands/frame/var-dil/basics/PointerDereference/Makefile
new file mode 100644
index 0000000000000..99998b20bcb05
--- /dev/null
+++ b/lldb/test/API/commands/frame/var-dil/basics/PointerDereference/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
diff --git a/lldb/test/API/commands/frame/var-dil/basics/PointerDereference/TestFrameVarDILPointerDereference.py b/lldb/test/API/commands/frame/var-dil/basics/PointerDereference/TestFrameVarDILPointerDereference.py
new file mode 100644
index 0000000000000..7f5acc26dd40b
--- /dev/null
+++ b/lldb/test/API/commands/frame/var-dil/basics/PointerDereference/TestFrameVarDILPointerDereference.py
@@ -0,0 +1,30 @@
+"""
+Test DIL pointer dereferencing.
+"""
+
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from lldbsuite.test import lldbutil
+
+import os
+import shutil
+import time
+
+class TestFrameVarDILPointerDereference(TestBase):
+ NO_DEBUG_INFO_TESTCASE = True
+
+ def test_frame_var(self):
+ self.build()
+ lldbutil.run_to_source_breakpoint(
+ self, "Set a breakpoint here", lldb.SBFileSpec("main.cpp")
+ )
+
+ self.runCmd("settings set target.experimental.use-DIL true")
+ self.expect_var_path("*p_int0", value="0")
+ self.expect_var_path("*cp_int5", value="5")
+ self.expect_var_path("&pp_void0[2]", type="void **")
+ self.expect_var_path("**pp_int0", value="0")
+ self.expect_var_path("&**pp_int0", type="int *")
+ self.expect("frame variable '&p_void[0]'", error=True,
+ substrs=["subscript of pointer to incomplete type 'void'"])
diff --git a/lldb/test/API/commands/frame/var-dil/basics/PointerDereference/main.cpp b/lldb/test/API/commands/frame/var-dil/basics/PointerDereference/main.cpp
new file mode 100644
index 0000000000000..719fa81b60830
--- /dev/null
+++ b/lldb/test/API/commands/frame/var-dil/basics/PointerDereference/main.cpp
@@ -0,0 +1,32 @@
+#include <cstddef>
+
+int main (int argc, char **argv) {
+ int* p_null = nullptr;
+ const char* p_char1 = "hello";
+
+ typedef const char* my_char_ptr;
+ my_char_ptr my_p_char1 = p_char1;
+
+ int offset = 5;
+ int array[10];
+ array[0] = 0;
+ array[offset] = offset;
+
+ int(&array_ref)[10] = array;
+
+ int* p_int0 = &array[0];
+ int** pp_int0 = &p_int0;
+ const int* cp_int0 = &array[0];
+ const int* cp_int5 = &array[offset];
+
+ typedef int* td_int_ptr_t;
+ td_int_ptr_t td_int_ptr0 = &array[0];
+
+ void* p_void = (void*)p_char1;
+ void** pp_void0 = &p_void;
+ void** pp_void1 = pp_void0 + 1;
+
+ std::nullptr_t std_nullptr_t = nullptr;
+
+ return 0; // Set a breakpoint here
+}
diff --git a/lldb/test/API/commands/frame/var-dil/basics/QualifiedId/Makefile b/lldb/test/API/commands/frame/var-dil/basics/QualifiedId/Makefile
new file mode 100644
index 0000000000000..99998b20bcb05
--- /dev/null
+++ b/lldb/test/API/commands/frame/var-dil/basics/QualifiedId/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
diff --git a/lldb/test/API/commands/frame/var-dil/basics/QualifiedId/TestFrameVarDILQualifiedId.py b/lldb/test/API/commands/frame/var-dil/basics/QualifiedId/TestFrameVarDILQualifiedId.py
new file mode 100644
index 0000000000000..3d91198ea3ab9
--- /dev/null
+++ b/lldb/test/API/commands/frame/var-dil/basics/QualifiedId/TestFrameVarDILQualifiedId.py
@@ -0,0 +1,29 @@
+"""
+Make sure 'frame var' using DIL parser/evaluator works for namespaces.
+"""
+
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from lldbsuite.test import lldbutil
+
+import os
+import shutil
+import time
+
+class TestFrameVarDILQualifiedId(TestBase):
+ # If your test case doesn't stress debug info, then
+ # set this to true. That way it won't be run once for
+ # each debug info format.
+ NO_DEBUG_INFO_TESTCASE = True
+
+ def test_frame_var(self):
+ self.build()
+ lldbutil.run_to_source_breakpoint(self, "Set a breakpoint here",
+ lldb.SBFileSpec("main.cpp"))
+
+ self.runCmd("settings set target.experimental.use-DIL true")
+ self.expect_var_path("::ns::i", value="1")
+ self.expect_var_path("ns::i", value="1")
+ self.expect_var_path("::ns::ns::i", value="2")
+ self.expect_var_path("ns::ns::i", value="2")
diff --git a/lldb/test/API/commands/frame/var-dil/basics/QualifiedId/main.cpp b/lldb/test/API/commands/frame/var-dil/basics/QualifiedId/main.cpp
new file mode 100644
index 0000000000000..3316d0b807868
--- /dev/null
+++ b/lldb/test/API/commands/frame/var-dil/basics/QualifiedId/main.cpp
@@ -0,0 +1,18 @@
+namespace ns {
+
+int i = 1;
+
+namespace ns {
+
+int i = 2;
+
+} // namespace ns
+
+} // namespace ns
+
+int
+main(int argc, char **argv)
+{
+
+ return 0; // Set a breakpoint here
+}
diff --git a/lldb/test/API/commands/frame/var-dil/basics/SharedPtr/Makefile b/lldb/test/API/commands/frame/var-dil/basics/SharedPtr/Makefile
new file mode 100644
index 0000000000000..af1baa7931b39
--- /dev/null
+++ b/lldb/test/API/commands/frame/var-dil/basics/SharedPtr/Makefile
@@ -0,0 +1,6 @@
+CXX_SOURCES := main.cpp
+CXXFLAGS_EXTRAS := -std=c++14
+
+USE_LIBCPP := 1
+
+include Makefile.rules
diff --git a/lldb/test/API/commands/frame/var-dil/basics/SharedPtr/TestFrameVarDILSharedPtr.py b/lldb/test/API/commands/frame/var-dil/basics/SharedPtr/TestFrameVarDILSharedPtr.py
new file mode 100644
index 0000000000000..94fe0340dc98c
--- /dev/null
+++ b/lldb/test/API/commands/frame/var-dil/basics/SharedPtr/TestFrameVarDILSharedPtr.py
@@ -0,0 +1,36 @@
+"""
+Make sure 'frame var' using DIL parser/evaluator works for shared/weak pointers.
+"""
+
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from lldbsuite.test import lldbutil
+
+import os
+import shutil
+import time
+
+class TestFrameVarDILSharedPtr(TestBase):
+ # If your test case doesn't stress debug info, then
+ # set this to true. That way it won't be run once for
+ # each debug info format.
+ NO_DEBUG_INFO_TESTCASE = True
+
+ def test_frame_var(self):
+ self.build()
+ lldbutil.run_to_source_breakpoint(self, "Set a breakpoint here",
+ lldb.SBFileSpec("main.cpp"))
+
+ self.runCmd("settings set target.experimental.use-DIL true")
+ self.expect_var_path("ptr_node.__ptr_",
+ type="std::shared_ptr<NodeS>::element_type *")
+ self.expect_var_path("ptr_node.__ptr_->value", value="1")
+ self.expect_var_path("ptr_node.__ptr_->next.__ptr_->value", value="2")
+
+ self.expect_var_path("ptr_int.__ptr_",
+ type="std::shared_ptr<int>::element_type *")
+ self.expect_var_path("*ptr_int.__ptr_", value="1")
+ self.expect_var_path("ptr_int_weak.__ptr_",
+ type="std::weak_ptr<int>::element_type *")
+ self.expect_var_path("*ptr_int_weak.__ptr_", value="1")
diff --git a/lldb/test/API/commands/frame/var-dil/basics/SharedPtr/TestFrameVarDILSharedPtrDeref.py b/lldb/test/API/commands/frame/var-dil/basics/SharedPtr/TestFrameVarDILSharedPtrDeref.py
new file mode 100644
index 0000000000000..32b389bec8fa4
--- /dev/null
+++ b/lldb/test/API/commands/frame/var-dil/basics/SharedPtr/TestFrameVarDILSharedPtrDeref.py
@@ -0,0 +1,29 @@
+"""
+Make sure 'frame var' using DIL parser/evaluator works for shared pointer deref.
+"""
+
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from lldbsuite.test import lldbutil
+
+import os
+import shutil
+import time
+
+class TestFrameVarDILSharedPtrDeref(TestBase):
+ # If your test case doesn't stress debug info, then
+ # set this to true. That way it won't be run once for
+ # each debug info format.
+ NO_DEBUG_INFO_TESTCASE = True
+
+ def test_frame_var(self):
+ self.build()
+ lldbutil.run_to_source_breakpoint(self, "Set a breakpoint here",
+ lldb.SBFileSpec("main.cpp"))
+
+ self.runCmd("settings set target.experimental.use-DIL true")
+ self.expect_var_path("ptr_node->value", value="1")
+ self.expect_var_path("ptr_node->next->value", value="2")
+ self.expect_var_path("(*ptr_node).value", value="1")
+ self.expect_var_path("(*(*ptr_node).next).value", value="2")
diff --git a/lldb/test/API/commands/frame/var-dil/basics/SharedPtr/main.cpp b/lldb/test/API/commands/frame/var-dil/basics/SharedPtr/main.cpp
new file mode 100644
index 0000000000000..642f5172a37f6
--- /dev/null
+++ b/lldb/test/API/commands/frame/var-dil/basics/SharedPtr/main.cpp
@@ -0,0 +1,26 @@
+#include <memory>
+
+int
+main(int argc, char **argv)
+{
+
+ struct NodeS {
+ std::shared_ptr<NodeS> next;
+ int value;
+ };
+ auto ptr_node = std::shared_ptr<NodeS>(new NodeS{nullptr, 2});
+ ptr_node = std::shared_ptr<NodeS>(new NodeS{std::move(ptr_node), 1});
+
+ std::shared_ptr<char> ptr_null;
+ auto ptr_int = std::make_shared<int>(1);
+ auto ptr_float = std::make_shared<float>(1.1f);
+
+ std::weak_ptr<int> ptr_int_weak = ptr_int;
+
+ std::shared_ptr<void> ptr_void = ptr_int;
+
+ // TestSharedPtr
+ // TestSharedPtrDeref
+ // TestSharedPtrCompare
+ return 0; // Set a breakpoint here
+}
diff --git a/lldb/test/API/commands/frame/var-dil/basics/UniquePtr/Makefile b/lldb/test/API/commands/frame/var-dil/basics/UniquePtr/Makefile
new file mode 100644
index 0000000000000..af1baa7931b39
--- /dev/null
+++ b/lldb/test/API/commands/frame/var-dil/basics/UniquePtr/Makefile
@@ -0,0 +1,6 @@
+CXX_SOURCES := main.cpp
+CXXFLAGS_EXTRAS := -std=c++14
+
+USE_LIBCPP := 1
+
+include Makefile.rules
diff --git a/lldb/test/API/commands/frame/var-dil/basics/UniquePtr/TestFrameVarDILUniquePtr.py b/lldb/test/API/commands/frame/var-dil/basics/UniquePtr/TestFrameVarDILUniquePtr.py
new file mode 100644
index 0000000000000..b7f9627f8a8c0
--- /dev/null
+++ b/lldb/test/API/commands/frame/var-dil/basics/UniquePtr/TestFrameVarDILUniquePtr.py
@@ -0,0 +1,28 @@
+"""
+Make sure 'frame var' using DIL parser/evaluator works for unique pointers.
+"""
+
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from lldbsuite.test import lldbutil
+
+import os
+import shutil
+import time
+
+class TestFrameVarDILUniquePtr(TestBase):
+ # If your test case doesn't stress debug info, then
+ # set this to true. That way it won't be run once for
+ # each debug info format.
+ NO_DEBUG_INFO_TESTCASE = True
+
+ def test_frame_var(self):
+ self.build()
+ lldbutil.run_to_source_breakpoint(self, "Set a breakpoint here",
+ lldb.SBFileSpec("main.cpp"))
+
+ self.runCmd("settings set target.experimental.use-DIL true")
+ self.expect_var_path("ptr_node.__ptr_", type="NodeU *")
+ self.expect_var_path("ptr_node.__ptr_->value", value="1")
+ self.expect_var_path("ptr_node.__ptr_->next.__ptr_->value", value="2")
diff --git a/lldb/test/API/commands/frame/var-dil/basics/UniquePtr/TestFrameVarDILUniquePtrDeref.py b/lldb/test/API/commands/frame/var-dil/basics/UniquePtr/TestFrameVarDILUniquePtrDeref.py
new file mode 100644
index 0000000000000..065fb9df33444
--- /dev/null
+++ b/lldb/test/API/commands/frame/var-dil/basics/UniquePtr/TestFrameVarDILUniquePtrDeref.py
@@ -0,0 +1,29 @@
+"""
+Make sure 'frame var' using DIL parser/evaluator works for unique pointer derefs.
+"""
+
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from lldbsuite.test import lldbutil
+
+import os
+import shutil
+import time
+
+class TestFrameVarDILUniquePtrDeref(TestBase):
+ # If your test case doesn't stress debug info, then
+ # set this to true. That way it won't be run once for
+ # each debug info format.
+ NO_DEBUG_INFO_TESTCASE = True
+
+ def test_frame_var(self):
+ self.build()
+ lldbutil.run_to_source_breakpoint(self, "Set a breakpoint here",
+ lldb.SBFileSpec("main.cpp"))
+
+ self.runCmd("settings set target.experimental.use-DIL true")
+ self.expect_var_path("ptr_node->value", value="1")
+ self.expect_var_path("ptr_node->next->value", value="2")
+ self.expect_var_path("(*ptr_node).value", value="1")
+ self.expect_var_path("(*(*ptr_node).next).value", value="2")
diff --git a/lldb/test/API/commands/frame/var-dil/basics/UniquePtr/main.cpp b/lldb/test/API/commands/frame/var-dil/basics/UniquePtr/main.cpp
new file mode 100644
index 0000000000000..f20f6f116a084
--- /dev/null
+++ b/lldb/test/API/commands/frame/var-dil/basics/UniquePtr/main.cpp
@@ -0,0 +1,25 @@
+#include <memory>
+
+int
+main(int argc, char **argv)
+{
+
+ struct NodeU {
+ std::unique_ptr<NodeU> next;
+ int value;
+ };
+ auto ptr_node = std::unique_ptr<NodeU>(new NodeU{nullptr, 2});
+ ptr_node = std::unique_ptr<NodeU>(new NodeU{std::move(ptr_node), 1});
+
+ std::unique_ptr<char> ptr_null;
+ auto ptr_int = std::make_unique<int>(1);
+ auto ptr_float = std::make_unique<float>(1.1f)...
[truncated]
|
|
✅ With the latest revision this PR passed the Python code formatter. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
labath
reviewed
Jun 12, 2025
Collaborator
labath
left a comment
labath
left a comment
There was a problem hiding this comment.
The code looks good, but it seems that you need to reformat it (both c++ and python).
Instead of hardcoding libc++ implementation details, it would be better to write your own "smart" pointer. libc++ changes the internal representation fairly frequently, and we shouldn't rely on it where it isn't necessary. If this test is meant to stay here ~forever, I think that's what we should do, but I'm not sure what is the long-term future of these tests.
Have you thought about what happens to them when we flip the flag (and maybe after the old implementation is removed)? I would expect that most of this functionality is already tested elsewhere, so I have a feeling that some sort of deduplication would be in order. And since you've found this issue by running the test suite, I assume that this is tested elsewhere (maybe inside the std::shared_ptr formatter test?)..
Test synthetic format provider similar to LibCxx smart pointers, but independent of actual smart pointer implementation (so not brittle test).
Contributor
Author
|
I think I have addressed all the review comments; please take another look. |
labath
reviewed
Jun 23, 2025
Collaborator
labath
left a comment
labath
left a comment
There was a problem hiding this comment.
The code looks good. I just have issues with the test(s). Please, see inline comments.
| // Fake smart pointer definition. | ||
| class smart_ptr { | ||
| public: | ||
| NodeS *__ptr_; |
Collaborator
There was a problem hiding this comment.
This is technically a reserved identifier. Let's just use ptr.
Comment on lines
18
to
21
| value_type = internal_child.GetType().GetPointerType() | ||
| cast_ptr_sp = internal_child.Cast(value_type) | ||
| value = internal_child.Dereference() | ||
| return value |
Collaborator
There was a problem hiding this comment.
The cast, is a noop, right? Any reason to not remove it?
Suggested change
| value_type = internal_child.GetType().GetPointerType() | |
| cast_ptr_sp = internal_child.Cast(value_type) | |
| value = internal_child.Dereference() | |
| return value | |
| return internal_child.Dereference() |
Collaborator
There was a problem hiding this comment.
I have a bit of an issue with the test name and location, I think it sets up the wrong expectations. The DIL should be language agnostic, so it shouldn't work (only) with (c++) smart pointers. Even in c++ you have non-pointer types that can be "dereferenced" (std::optional, for one).
The DIL should work with with any type which provides a synthetic dereference operator, so if we're going to have a folder for all DIL tests, this test (which uses a type with a synthetic deref op) should be there. And to drive the point home, I think it ought to be called something like TestSyntheticDeref.py. I don't have an issue with the name smart_ptr in main.cpp, but I would like to the python test code to reflect the genericness of this feature (in the same way that the actual implementation does not reference any particular type).
OTOH, I believe tests for std::shared/unique_ptr would be better off in test/API/functionalities/data-formatter/data-formatter-stl, where we that operations on that specific type work. Then you can compare them with the existing test for these types (and their data formatters). If your testing something not covered by the existing test, just add it there. Otherwise, delete it
Contributor
Author
There was a problem hiding this comment.
I've tried to move most of the tests around as you suggested. I have kept the dil smart pointer tests separate from the existing stl tests (although I moved them over to the data-formattter-stl/libcxx tree). They do test functionality that the existing tests don't.
labath
reviewed
Jun 23, 2025
Comment on lines
+25
to
+29
| self.expect_var_path("*p_int0", value="0") | ||
| self.expect_var_path("*cp_int5", value="5") | ||
| self.expect_var_path("&pp_void0[2]", type="void **") | ||
| self.expect_var_path("**pp_int0", value="0") | ||
| self.expect_var_path("&**pp_int0", type="int *") |
Collaborator
There was a problem hiding this comment.
Since the goal here is to explicitly check handling pointer arithmetic, you may want to add additional checks for the values you get this way. expect_var_path returns the SBValue it has located, so you could do something like:
pp_void0_2_got = self.expect_var_path("&pp_void0[2]", type="void **")
pp_void0_2_exp = self.expect_var_path("pp_void0_2", type="void **") # Initialized in C++ code to point to the same value
self.assertEqual(pp_void0_2_got.GetValueAsAddress(), pp_void0_2_exp.GetValueAsAddress())
Contributor
Author
There was a problem hiding this comment.
Thanks for the suggestion! Done.
- Moved DIL SharedPtr and UniquePtr tests to API/functionalities/data-formatter/data-formatter-stl/libcxx/dil_shared_ptr and API/functionalities/data-formatter/data-formatter-stl/libcxx/dil_unique_ptr - Moved my new test to API/command/frame/var-dil/basics/SyntheticDereference. - Changed 'smart_ptr' to 'wrap_ptr' in new test. - Changed internal field name from '__ptr_' to 'ptr. - Remove dead code from test. - Update pointer dereference test to test some expected vs actual addresses, as suggested.
labath
approved these changes
Jun 24, 2025
Collaborator
labath
left a comment
labath
left a comment
There was a problem hiding this comment.
Thanks. Could you also merge them into the existing tests (as a separate test method)? The existing tests are of the same type ("break and inspect a bunch of variables") so it should be easy enough and this way one can see all tests for a type in single place.
Comment on lines
20
to
22
| // TestSharedPtr | ||
| // TestSharedPtrDeref | ||
| // TestSharedPtrCompare |
Comment on lines
21
to
23
| // TestUniquePtr | ||
| // TestUniquePtrDeref | ||
| // TestUniquePtrCompare |
Contributor
Author
Done? I think? |
labath
reviewed
Jun 27, 2025
Collaborator
labath
left a comment
labath
left a comment
There was a problem hiding this comment.
I meant adding a new test method (def test_something(self): self.build(); self.run(); your code), but this is fine too, especially if the switch to the new implementation is imminent.
labath
approved these changes
Jun 27, 2025
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 updates the DIL implementation to handle smart pointers (accessing field members and dereferencing) in the same way the current 'frame variable' implementation does. It also adds tests for handling smart pointers, as well as some additional DIL tests.