[LLDB] Re-land 'Update DIL handling of array subscripting' #154269
Merged
+178
−34
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 attempts to fix the issues with the original PR (llvm#151605), updating the DIL code for handling array subscripting to more closely match and handle all the casees from the original 'frame var' implementation. The first PR did not include special-case code for objc pointers, which apparently caused a test failure on the green-dragon buildbot. Hopefully this PR, which includes the objc pointer special code, fixes that issue.
Member
|
@llvm/pr-subscribers-lldb Author: None (cmtice) ChangesThis attempts to fix the issues with the original PR (#151605), updating the DIL code for handling array subscripting to more closely match and handle all the casees from the original 'frame var' implementation. The first PR did not include special-case code for objc pointers, which apparently caused a test failure on the green-dragon buildbot. Hopefully this PR, which includes the objc pointer special code, fixes that issue. Full diff: https://github.com/llvm/llvm-project/pull/154269.diff 4 Files Affected:
diff --git a/lldb/source/ValueObject/DILEval.cpp b/lldb/source/ValueObject/DILEval.cpp
index 6f28434c646cd..e61b7e33f5fd8 100644
--- a/lldb/source/ValueObject/DILEval.cpp
+++ b/lldb/source/ValueObject/DILEval.cpp
@@ -330,40 +330,135 @@ Interpreter::Visit(const ArraySubscriptNode *node) {
return lhs_or_err;
lldb::ValueObjectSP base = *lhs_or_err;
- // Check to see if 'base' has a synthetic value; if so, try using that.
+ StreamString var_expr_path_strm;
uint64_t child_idx = node->GetIndex();
- if (lldb::ValueObjectSP synthetic = base->GetSyntheticValue()) {
- llvm::Expected<uint32_t> num_children =
- synthetic->GetNumChildren(child_idx + 1);
- if (!num_children)
- return llvm::make_error<DILDiagnosticError>(
- m_expr, toString(num_children.takeError()), node->GetLocation());
- if (child_idx >= *num_children) {
- std::string message = llvm::formatv(
+ lldb::ValueObjectSP child_valobj_sp;
+
+ bool is_incomplete_array = false;
+ CompilerType base_type = base->GetCompilerType().GetNonReferenceType();
+ base->GetExpressionPath(var_expr_path_strm);
+
+ if (base_type.IsPointerType()) {
+ bool is_objc_pointer = true;
+
+ if (base->GetCompilerType().GetMinimumLanguage() != lldb::eLanguageTypeObjC)
+ is_objc_pointer = false;
+ else if (!base->GetCompilerType().IsPointerType())
+ is_objc_pointer = false;
+
+ if (!m_use_synthetic && is_objc_pointer) {
+ std::string err_msg =
+ llvm::formatv("\"(%s) %s\" is an Objective-C pointer, and cannot be "
+ "subscripted",
+ base->GetTypeName().AsCString("<invalid type>"),
+ var_expr_path_strm.GetData());
+ return llvm::make_error<DILDiagnosticError>(m_expr, std::move(err_msg),
+ node->GetLocation());
+ } else if (is_objc_pointer) {
+ lldb::ValueObjectSP synthetic = base->GetSyntheticValue();
+ if (!synthetic || synthetic == base) {
+ std::string err_msg =
+ llvm::formatv("\"(%s) %s\" is not an array type",
+ base->GetTypeName().AsCString("<invalid type>"),
+ var_expr_path_strm.GetData());
+ return llvm::make_error<DILDiagnosticError>(m_expr, std::move(err_msg),
+ node->GetLocation());
+ } else if (static_cast<uint32_t>(child_idx) >=
+ synthetic->GetNumChildrenIgnoringErrors()) {
+ std::string err_msg = llvm::formatv(
+ "array index %ld is not valid for \"(%s) %s\"", child_idx,
+ base->GetTypeName().AsCString("<invalid type>"),
+ var_expr_path_strm.GetData());
+ return llvm::make_error<DILDiagnosticError>(m_expr, std::move(err_msg),
+ node->GetLocation());
+ } else {
+ child_valobj_sp = synthetic->GetChildAtIndex(child_idx);
+ if (!child_valobj_sp) {
+ std::string err_msg = llvm::formatv(
+ "array index %ld is not valid for \"(%s) %s\"", child_idx,
+ base->GetTypeName().AsCString("<invalid type>"),
+ var_expr_path_strm.GetData());
+ return llvm::make_error<DILDiagnosticError>(
+ m_expr, std::move(err_msg), node->GetLocation());
+ }
+ if (m_use_dynamic != lldb::eNoDynamicValues) {
+ if (auto dynamic_sp = child_valobj_sp->GetDynamicValue(m_use_dynamic))
+ child_valobj_sp = std::move(dynamic_sp);
+ }
+ return child_valobj_sp;
+ }
+ }
+
+ child_valobj_sp = base->GetSyntheticArrayMember(child_idx, true);
+ if (!child_valobj_sp) {
+ std::string err_msg = llvm::formatv(
+ "failed to use pointer as array for index {0} for "
+ "\"({1}) {2}\"",
+ child_idx, base->GetTypeName().AsCString("<invalid type>"),
+ var_expr_path_strm.GetData());
+ if (base_type.IsPointerToVoid())
+ err_msg = "subscript of pointer to incomplete type 'void'";
+ return llvm::make_error<DILDiagnosticError>(m_expr, std::move(err_msg),
+ node->GetLocation());
+ }
+ } else if (base_type.IsArrayType(nullptr, nullptr, &is_incomplete_array)) {
+ child_valobj_sp = base->GetChildAtIndex(child_idx);
+ if (!child_valobj_sp && (is_incomplete_array || m_use_synthetic))
+ child_valobj_sp = base->GetSyntheticArrayMember(child_idx, true);
+ if (!child_valobj_sp) {
+ std::string err_msg = llvm::formatv(
"array index {0} is not valid for \"({1}) {2}\"", child_idx,
base->GetTypeName().AsCString("<invalid type>"),
- base->GetName().AsCString());
- return llvm::make_error<DILDiagnosticError>(m_expr, message,
+ var_expr_path_strm.GetData());
+ return llvm::make_error<DILDiagnosticError>(m_expr, std::move(err_msg),
node->GetLocation());
}
- if (lldb::ValueObjectSP child_valobj_sp =
- synthetic->GetChildAtIndex(child_idx))
- return child_valobj_sp;
+ } else if (base_type.IsScalarType()) {
+ child_valobj_sp =
+ base->GetSyntheticBitFieldChild(child_idx, child_idx, true);
+ if (!child_valobj_sp) {
+ std::string err_msg = llvm::formatv(
+ "bitfield range {0}-{1} is not valid for \"({2}) {3}\"", child_idx,
+ child_idx, base->GetTypeName().AsCString("<invalid type>"),
+ var_expr_path_strm.GetData());
+ return llvm::make_error<DILDiagnosticError>(m_expr, std::move(err_msg),
+ node->GetLocation(), 1);
+ }
+ } else {
+ lldb::ValueObjectSP synthetic = base->GetSyntheticValue();
+ if (!m_use_synthetic || !synthetic || synthetic == base) {
+ std::string err_msg =
+ llvm::formatv("\"{0}\" is not an array type",
+ base->GetTypeName().AsCString("<invalid type>"));
+ return llvm::make_error<DILDiagnosticError>(m_expr, std::move(err_msg),
+ node->GetLocation(), 1);
+ }
+ if (static_cast<uint32_t>(child_idx) >=
+ synthetic->GetNumChildrenIgnoringErrors(child_idx + 1)) {
+ std::string err_msg = llvm::formatv(
+ "array index {0} is not valid for \"({1}) {2}\"", child_idx,
+ base->GetTypeName().AsCString("<invalid type>"),
+ var_expr_path_strm.GetData());
+ return llvm::make_error<DILDiagnosticError>(m_expr, std::move(err_msg),
+ node->GetLocation(), 1);
+ }
+ child_valobj_sp = synthetic->GetChildAtIndex(child_idx);
+ if (!child_valobj_sp) {
+ std::string err_msg = llvm::formatv(
+ "array index {0} is not valid for \"({1}) {2}\"", child_idx,
+ base->GetTypeName().AsCString("<invalid type>"),
+ var_expr_path_strm.GetData());
+ return llvm::make_error<DILDiagnosticError>(m_expr, std::move(err_msg),
+ node->GetLocation(), 1);
+ }
}
- auto base_type = base->GetCompilerType().GetNonReferenceType();
- if (!base_type.IsPointerType() && !base_type.IsArrayType())
- return llvm::make_error<DILDiagnosticError>(
- m_expr, "subscripted value is not an array or pointer",
- node->GetLocation());
- if (base_type.IsPointerToVoid())
- return llvm::make_error<DILDiagnosticError>(
- m_expr, "subscript of pointer to incomplete type 'void'",
- node->GetLocation());
-
- if (base_type.IsArrayType()) {
- if (lldb::ValueObjectSP child_valobj_sp = base->GetChildAtIndex(child_idx))
- return child_valobj_sp;
+ if (child_valobj_sp) {
+ if (m_use_dynamic != lldb::eNoDynamicValues) {
+ if (auto dynamic_sp = child_valobj_sp->GetDynamicValue(m_use_dynamic))
+ child_valobj_sp = std::move(dynamic_sp);
+ }
+ return child_valobj_sp;
}
int64_t signed_child_idx = node->GetIndex();
diff --git a/lldb/test/API/commands/frame/var-dil/basics/ArraySubscript/TestFrameVarDILArraySubscript.py b/lldb/test/API/commands/frame/var-dil/basics/ArraySubscript/TestFrameVarDILArraySubscript.py
index 0f56057189395..e3cfb878dd415 100644
--- a/lldb/test/API/commands/frame/var-dil/basics/ArraySubscript/TestFrameVarDILArraySubscript.py
+++ b/lldb/test/API/commands/frame/var-dil/basics/ArraySubscript/TestFrameVarDILArraySubscript.py
@@ -69,17 +69,18 @@ def test_subscript(self):
substrs=["expected 'r_square', got: <'.'"],
)
- # Base should be a "pointer to T" and index should be of an integral type.
- self.expect(
- "frame var 'idx_1[0]'",
- error=True,
- substrs=["subscripted value is not an array or pointer"],
- )
+ # Test accessing bits in scalar types.
+ self.expect_var_path("idx_1[0]", value="1")
+ self.expect_var_path("idx_1[1]", value="0")
+
+ # Bit adcess not valid for a reference.
self.expect(
"frame var 'idx_1_ref[0]'",
error=True,
- substrs=["subscripted value is not an array or pointer"],
+ substrs=["bitfield range 0-0 is not valid"],
)
+
+ # Base should be a "pointer to T" and index should be of an integral type.
self.expect(
"frame var 'int_arr[int_ptr]'",
error=True,
@@ -105,6 +106,8 @@ def test_subscript_synthetic(self):
)
self.runCmd("settings set target.experimental.use-DIL true")
+ self.runCmd("script from myArraySynthProvider import *")
+ self.runCmd("type synth add -l myArraySynthProvider myArray")
# Test synthetic value subscription
self.expect_var_path("vector[1]", value="2")
@@ -113,3 +116,7 @@ def test_subscript_synthetic(self):
error=True,
substrs=["array index 100 is not valid"],
)
+ self.expect(
+ "frame var 'ma_ptr[0]'",
+ substrs=["(myArray) ma_ptr[0] = ([0] = 7, [1] = 8, [2] = 9, [3] = 10)"],
+ )
diff --git a/lldb/test/API/commands/frame/var-dil/basics/ArraySubscript/main.cpp b/lldb/test/API/commands/frame/var-dil/basics/ArraySubscript/main.cpp
index a9a3612dfae5a..03ad3e872ca76 100644
--- a/lldb/test/API/commands/frame/var-dil/basics/ArraySubscript/main.cpp
+++ b/lldb/test/API/commands/frame/var-dil/basics/ArraySubscript/main.cpp
@@ -1,5 +1,11 @@
#include <vector>
+class myArray {
+public:
+ int m_array[4] = {7, 8, 9, 10};
+ int m_arr_size = 4;
+};
+
int main(int argc, char **argv) {
int int_arr[] = {1, 2, 3};
int *int_ptr = int_arr;
@@ -29,5 +35,8 @@ int main(int argc, char **argv) {
std::vector<int> vector = {1, 2, 3};
+ myArray ma;
+ myArray *ma_ptr = &ma;
+
return 0; // Set a breakpoint here
}
diff --git a/lldb/test/API/commands/frame/var-dil/basics/ArraySubscript/myArraySynthProvider.py b/lldb/test/API/commands/frame/var-dil/basics/ArraySubscript/myArraySynthProvider.py
new file mode 100644
index 0000000000000..167899bd3907c
--- /dev/null
+++ b/lldb/test/API/commands/frame/var-dil/basics/ArraySubscript/myArraySynthProvider.py
@@ -0,0 +1,33 @@
+import lldb
+
+
+class myArraySynthProvider:
+ def __init__(self, valobj, dict):
+ self.valobj = valobj
+
+ def num_children(self):
+ size_valobj = self.valobj.GetChildMemberWithName("m_arr_size")
+ if size_valobj:
+ return size_valobj.GetValueAsUnsigned(0)
+ return 0
+
+ def get_child_at_index(self, index):
+ size_valobj = self.valobj.GetChildMemberWithName("m_arr_size")
+ arr = self.valobj.GetChildMemberWithName("m_array")
+ if not size_valobj or not arr:
+ return None
+ max_idx = size_valobj.GetValueAsUnsigned(0)
+ if index >= max_idx:
+ return None
+ return arr.GetChildAtIndex(index)
+
+ def get_child_index(self, name):
+ if name == "[0]":
+ return 0
+ if name == "[1]":
+ return
+ if name == "[2]":
+ return 2
+ if name == "[3]":
+ return 3
+ return -1
|
Collaborator
Does that mean you didn't check if it fixes it? I think we should understand how the test passed without this PR before including that. |
Contributor
Author
I do not have the right machines, etc. to test it. I have asked some friends at Apple to test it for me, but have not heard back yet (but I only just posted the PR). |
Collaborator
I applied this patch to a current TOT checkout I had, and ran the test suite. The only failure I got was: But I'm pretty sure that's not you. |
Contributor
Author
|
Thank you for confirming this fixed the previous test failure (TestDataFormatterObjCNSContainer.py) |
labath
reviewed
Aug 25, 2025
Collaborator
labath
left a comment
labath
left a comment
There was a problem hiding this comment.
Okay, I think I understand now.
The question is about what to do with pointer to data structures with child providers (e.g. std::vector<T>*). The original "frame var" indexed the pointer (meaning v[0] was the same as dereferencing the vector -- handy because "frame var" didn't allow you to insert a (*...) at a random point in the expression). The DIL implementation interpreted it as "returning the first element of the pointed-to vector" (essentially, transparently dereferencing the object). After your first version of the patch changed that to match "frame var", it broke the ObjC test because ObjC actually expected the transparent dereferencing behavior (which sort of makes sense as all ObjC objects are pointers). This version fixes that by taking over the ObjC special-casing code from the original implementation.
This kind of special casing is something we wanted to avoid, but I think it's acceptable for now as it is just taking over existing code. We probably ought to fix it at some point though.
lldb/source/ValueObject/DILEval.cpp
Outdated
|
|
||
| if (!m_use_synthetic && is_objc_pointer) { | ||
| std::string err_msg = | ||
| llvm::formatv("\"(%s) %s\" is an Objective-C pointer, and cannot be " |
Collaborator
There was a problem hiding this comment.
Suggested change
| llvm::formatv("\"(%s) %s\" is an Objective-C pointer, and cannot be " | |
| llvm::formatv("\"({0}) {1}\" is an Objective-C pointer, and cannot be " |
This is the right formatv syntax (throughout the function).
lldb/source/ValueObject/DILEval.cpp
Outdated
| var_expr_path_strm.GetData()); | ||
| return llvm::make_error<DILDiagnosticError>(m_expr, std::move(err_msg), | ||
| node->GetLocation()); | ||
| } else if (is_objc_pointer) { |
Collaborator
There was a problem hiding this comment.
https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return (throughout the function)
- Fix llvm::formatv statements. - Fix else-after-return occurrences.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
labath
approved these changes
Aug 26, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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 attempts to fix the issues with the original PR (#151605), updating the DIL code for handling array subscripting to more closely match and handle all the casees from the original 'frame var' implementation. The first PR did not include special-case code for objc pointers, which apparently caused a test failure on the green-dragon buildbot. Hopefully this PR, which includes the objc pointer special code, fixes that issue.