[LLDB] Add field member operators to DIL #138093
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
Add the arrow and period operators, allowing DIL to find and access member fields.
|
@llvm/pr-subscribers-lldb Author: None (cmtice) ChangesAdd the arrow and period operators, allowing DIL to find and access member fields. Patch is 28.98 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/138093.diff 18 Files Affected:
diff --git a/lldb/docs/dil-expr-lang.ebnf b/lldb/docs/dil-expr-lang.ebnf
index c8bf4231b3e4a..580626862c005 100644
--- a/lldb/docs/dil-expr-lang.ebnf
+++ b/lldb/docs/dil-expr-lang.ebnf
@@ -5,13 +5,17 @@
expression = unary_expression ;
-unary_expression = unary_operator expression
- | primary_expression ;
+unary_expression = postfix_expression
+ | unary_operator expression ;
unary_operator = "*" | "&" ;
+postfix_expresson = primary_expression
+ | postfix_expression "." id_expression
+ | postfix_expression "->" id_expression ;
+
primary_expression = id_expression
- | "(" expression ")";
+ | "(" expression ")" ;
id_expression = unqualified_id
| qualified_id
diff --git a/lldb/include/lldb/ValueObject/DILAST.h b/lldb/include/lldb/ValueObject/DILAST.h
index fe3827ef0516a..a74a12bd8be9d 100644
--- a/lldb/include/lldb/ValueObject/DILAST.h
+++ b/lldb/include/lldb/ValueObject/DILAST.h
@@ -20,6 +20,7 @@ namespace lldb_private::dil {
enum class NodeKind {
eErrorNode,
eIdentifierNode,
+ eMemberOfNode,
eUnaryOpNode,
};
@@ -88,6 +89,29 @@ class IdentifierNode : public ASTNode {
std::string m_name;
};
+class MemberOfNode : public ASTNode {
+public:
+ MemberOfNode(uint32_t location, ASTNodeUP base, bool is_arrow,
+ ConstString name)
+ : ASTNode(location, NodeKind::eMemberOfNode), m_base(std::move(base)),
+ m_is_arrow(is_arrow), m_field_name(name) { }
+
+ llvm::Expected<lldb::ValueObjectSP> Accept(Visitor *v) const override;
+
+ ASTNode *base() const { return m_base.get(); }
+ bool IsArrow() const { return m_is_arrow; }
+ ConstString FieldName() const { return m_field_name; }
+
+ static bool classof(const ASTNode *node) {
+ return node->GetKind() == NodeKind::eMemberOfNode;
+ }
+
+private:
+ ASTNodeUP m_base;
+ bool m_is_arrow;
+ ConstString m_field_name;
+};
+
class UnaryOpNode : public ASTNode {
public:
UnaryOpNode(uint32_t location, UnaryOpKind kind, ASTNodeUP operand)
@@ -118,6 +142,8 @@ class Visitor {
virtual llvm::Expected<lldb::ValueObjectSP>
Visit(const IdentifierNode *node) = 0;
virtual llvm::Expected<lldb::ValueObjectSP>
+ Visit(const MemberOfNode *node) = 0;
+ virtual llvm::Expected<lldb::ValueObjectSP>
Visit(const UnaryOpNode *node) = 0;
};
diff --git a/lldb/include/lldb/ValueObject/DILEval.h b/lldb/include/lldb/ValueObject/DILEval.h
index b1dd3fdb49739..053daffaa41f2 100644
--- a/lldb/include/lldb/ValueObject/DILEval.h
+++ b/lldb/include/lldb/ValueObject/DILEval.h
@@ -49,8 +49,16 @@ class Interpreter : Visitor {
private:
llvm::Expected<lldb::ValueObjectSP>
Visit(const IdentifierNode *node) override;
+ llvm::Expected<lldb::ValueObjectSP> Visit(const MemberOfNode *node) override;
llvm::Expected<lldb::ValueObjectSP> Visit(const UnaryOpNode *node) override;
+ lldb::ValueObjectSP EvaluateMemberOf(lldb::ValueObjectSP value,
+ const std::vector<uint32_t> &path,
+ bool use_synthetic, bool is_dynamic);
+
+ lldb::ValueObjectSP FindMemberWithName(lldb::ValueObjectSP base,
+ ConstString name, bool is_arrow);
+
// Used by the interpreter to create objects, perform casts, etc.
lldb::TargetSP m_target;
llvm::StringRef m_expr;
diff --git a/lldb/include/lldb/ValueObject/DILLexer.h b/lldb/include/lldb/ValueObject/DILLexer.h
index 3508b8b7a85c6..9475519a43d2a 100644
--- a/lldb/include/lldb/ValueObject/DILLexer.h
+++ b/lldb/include/lldb/ValueObject/DILLexer.h
@@ -25,10 +25,12 @@ class Token {
public:
enum Kind {
amp,
+ arrow,
coloncolon,
eof,
identifier,
l_paren,
+ period,
r_paren,
star,
};
diff --git a/lldb/include/lldb/ValueObject/DILParser.h b/lldb/include/lldb/ValueObject/DILParser.h
index f5c00b1040ef4..c62f8908290f5 100644
--- a/lldb/include/lldb/ValueObject/DILParser.h
+++ b/lldb/include/lldb/ValueObject/DILParser.h
@@ -84,6 +84,7 @@ class DILParser {
ASTNodeUP ParseExpression();
ASTNodeUP ParseUnaryExpression();
+ ASTNodeUP ParsePostfixExpression();
ASTNodeUP ParsePrimaryExpression();
std::string ParseNestedNameSpecifier();
diff --git a/lldb/source/ValueObject/DILAST.cpp b/lldb/source/ValueObject/DILAST.cpp
index ea847587501ee..2814d96b94e6e 100644
--- a/lldb/source/ValueObject/DILAST.cpp
+++ b/lldb/source/ValueObject/DILAST.cpp
@@ -19,6 +19,10 @@ llvm::Expected<lldb::ValueObjectSP> IdentifierNode::Accept(Visitor *v) const {
return v->Visit(this);
}
+llvm::Expected<lldb::ValueObjectSP> MemberOfNode::Accept(Visitor *v) const {
+ return v->Visit(this);
+}
+
llvm::Expected<lldb::ValueObjectSP> UnaryOpNode::Accept(Visitor *v) const {
return v->Visit(this);
}
diff --git a/lldb/source/ValueObject/DILEval.cpp b/lldb/source/ValueObject/DILEval.cpp
index 15a66d4866305..1f1ad7161f42e 100644
--- a/lldb/source/ValueObject/DILEval.cpp
+++ b/lldb/source/ValueObject/DILEval.cpp
@@ -272,4 +272,204 @@ Interpreter::Visit(const UnaryOpNode *node) {
m_expr, "invalid ast: unexpected binary operator", node->GetLocation());
}
+lldb::ValueObjectSP
+Interpreter::EvaluateMemberOf(lldb::ValueObjectSP value,
+ const std::vector<uint32_t> &path,
+ bool use_synthetic, bool is_dynamic) {
+ lldb::ValueObjectSP member_val_sp = value;
+
+ lldb::DynamicValueType use_dynamic =
+ (!is_dynamic) ? lldb::eNoDynamicValues : lldb::eDynamicDontRunTarget;
+ // Walk the path from the base value to the value that contains the requested field.
+ for (uint32_t idx : path) {
+ member_val_sp = member_val_sp->GetChildAtIndex(idx, /*can_create*/ true);
+ }
+ // If that didn't work, try it with the dynamic value.
+ if (!member_val_sp && is_dynamic) {
+ lldb::ValueObjectSP dyn_val_sp = value->GetDynamicValue(use_dynamic);
+ if (dyn_val_sp) {
+ for (uint32_t idx : path) {
+ dyn_val_sp = dyn_val_sp->GetChildAtIndex(idx, true);
+ }
+ member_val_sp = dyn_val_sp;
+ }
+ }
+ assert(member_val_sp && "invalid ast: invalid member access");
+
+ return member_val_sp;
+}
+
+static bool GetFieldIndex(CompilerType type, const std::string &name,
+ std::vector<uint32_t> *idx_path) {
+ bool found = false;
+ uint32_t num_fields = type.GetNumFields();
+ for (uint32_t i = 0; i < num_fields; ++i) {
+ uint64_t bit_offset = 0;
+ uint32_t bitfield_bit_size = 0;
+ bool is_bitfield = false;
+ std::string name_sstr;
+ CompilerType field_type(type.GetFieldAtIndex(
+ i, name_sstr, &bit_offset, &bitfield_bit_size, &is_bitfield));
+ auto field_name =
+ name_sstr.length() == 0 ? std::optional<std::string>() : name_sstr;
+ if (field_type.IsValid() && name_sstr == name) {
+ idx_path->push_back(i + type.GetNumberOfNonEmptyBaseClasses());
+ found = true;
+ break;
+ } else if (field_type.IsAnonymousType()) {
+ found = GetFieldIndex(field_type, name, idx_path);
+ if (found) {
+ idx_path->push_back(i + type.GetNumberOfNonEmptyBaseClasses());
+ break;
+ }
+ }
+ }
+ return found;
+}
+
+static bool SearchBaseClassesForField(lldb::ValueObjectSP base_sp,
+ CompilerType base_type,
+ const std::string &name,
+ std::vector<uint32_t> *idx_path,
+ bool use_synthetic, bool is_dynamic) {
+ bool found = false;
+ uint32_t num_non_empty_bases = 0;
+ uint32_t num_direct_bases = base_type.GetNumDirectBaseClasses();
+ for (uint32_t i = 0; i < num_direct_bases; ++i) {
+ uint32_t bit_offset;
+ CompilerType base_class =
+ base_type.GetDirectBaseClassAtIndex(i, &bit_offset);
+ std::vector<uint32_t> field_idx_path;
+ if (GetFieldIndex(base_class, name, &field_idx_path)) {
+ for (uint32_t j : field_idx_path)
+ idx_path->push_back(j + base_class.GetNumberOfNonEmptyBaseClasses());
+ idx_path->push_back(i);
+ return true;
+ }
+
+ found = SearchBaseClassesForField(base_sp, base_class, name, idx_path,
+ use_synthetic, is_dynamic);
+ if (found) {
+ idx_path->push_back(i);
+ return true;
+ }
+
+ if (base_class.GetNumFields() > 0)
+ num_non_empty_bases += 1;
+ }
+ return false;
+}
+
+lldb::ValueObjectSP Interpreter::FindMemberWithName(lldb::ValueObjectSP base,
+ ConstString name,
+ bool is_arrow) {
+ bool is_synthetic = false;
+ bool is_dynamic = true;
+ // See if GetChildMemberWithName works.
+ lldb::ValueObjectSP field_obj =
+ base->GetChildMemberWithName(name.GetStringRef());
+ if (field_obj && field_obj->GetName() == name)
+ return field_obj;
+
+ // Check for synthetic member.
+ lldb::ValueObjectSP child_sp = base->GetSyntheticValue();
+ if (child_sp) {
+ is_synthetic = true;
+ field_obj = child_sp->GetChildMemberWithName(name);
+ if (field_obj && field_obj->GetName() == name)
+ return field_obj;
+ }
+
+ // Check indices of immediate member fields of base's type.
+ CompilerType base_type = base->GetCompilerType();
+ std::vector<uint32_t> field_idx_path;
+ if (GetFieldIndex(base_type, name.GetString(), &field_idx_path)) {
+ std::reverse(field_idx_path.begin(), field_idx_path.end());
+ // Traverse the path & verify the final object is correct.
+ field_obj = base;
+ for (uint32_t i : field_idx_path)
+ field_obj = field_obj->GetChildAtIndex(i, true);
+ if (field_obj && field_obj->GetName() == name)
+ return field_obj;
+ }
+
+ // Go through base classes and look for field there.
+ std::vector<uint32_t> base_class_idx_path;
+ bool found =
+ SearchBaseClassesForField(base, base_type, name.GetString(),
+ &base_class_idx_path, is_synthetic, is_dynamic);
+ if (found && !base_class_idx_path.empty()) {
+ std::reverse(base_class_idx_path.begin(), base_class_idx_path.end());
+ field_obj =
+ EvaluateMemberOf(base, base_class_idx_path, is_synthetic, is_dynamic);
+ if (field_obj && field_obj->GetName() == name)
+ return field_obj;
+ }
+
+ // Field not found.
+ return lldb::ValueObjectSP();
+}
+
+llvm::Expected<lldb::ValueObjectSP>
+Interpreter::Visit(const MemberOfNode *node) {
+ Status error;
+ auto base_or_err = Evaluate(node->base());
+ if (!base_or_err) {
+ return base_or_err;
+ }
+ lldb::ValueObjectSP base = *base_or_err;
+
+ // Perform basic type checking.
+ CompilerType base_type = base->GetCompilerType();
+ // When using an arrow, make sure the base is a pointer or array type.
+ // When using a period, make sure the base type is NOT a pointer type.
+ if (node->IsArrow() && !base_type.IsPointerType() &&
+ !base_type.IsArrayType()) {
+ lldb::ValueObjectSP deref_sp = base->Dereference(error);
+ if (error.Success()) {
+ base = deref_sp;
+ base_type = deref_sp->GetCompilerType().GetPointerType();
+ } else {
+ std::string errMsg =
+ llvm::formatv("member reference type {0} is not a pointer; "
+ "did you mean to use '.'?",
+ base_type.TypeDescription());
+ return llvm::make_error<DILDiagnosticError>(
+ m_expr, errMsg, node->GetLocation(), node->FieldName().GetLength());
+ }
+ } else if (!node->IsArrow() && base_type.IsPointerType()) {
+ std::string errMsg =
+ llvm::formatv("member reference type {0} is a pointer; "
+ "did you mean to use '->'?",
+ base_type.TypeDescription());
+ return llvm::make_error<DILDiagnosticError>(
+ m_expr, errMsg, node->GetLocation(), node->FieldName().GetLength());
+ }
+
+ // User specified array->elem; need to get to element[0] to look for fields.
+ if (node->IsArrow() && base_type.IsArrayType())
+ base = base->GetChildAtIndex(0);
+
+ // Now look for the member with the specified name.
+ lldb::ValueObjectSP field_obj =
+ FindMemberWithName(base, node->FieldName(), node->IsArrow());
+ if (field_obj) {
+ if (field_obj->GetCompilerType().IsReferenceType()) {
+ lldb::ValueObjectSP tmp_obj = field_obj->Dereference(error);
+ if (error.Fail())
+ return error.ToError();
+ return tmp_obj;
+ }
+ return field_obj;
+ }
+
+ if (node->IsArrow() && base_type.IsPointerType())
+ base_type = base_type.GetPointeeType();
+ std::string errMsg = llvm::formatv(
+ "no member named '{0}' in {1}", node->FieldName().GetStringRef(),
+ base_type.GetFullyUnqualifiedType().TypeDescription());
+ return llvm::make_error<DILDiagnosticError>(
+ m_expr, errMsg, node->GetLocation(), node->FieldName().GetLength());
+}
+
} // namespace lldb_private::dil
diff --git a/lldb/source/ValueObject/DILLexer.cpp b/lldb/source/ValueObject/DILLexer.cpp
index b9c2e7971e3b4..1610b4e87d7fb 100644
--- a/lldb/source/ValueObject/DILLexer.cpp
+++ b/lldb/source/ValueObject/DILLexer.cpp
@@ -21,6 +21,8 @@ llvm::StringRef Token::GetTokenName(Kind kind) {
switch (kind) {
case Kind::amp:
return "amp";
+ case Kind::arrow:
+ return "arrow";
case Kind::coloncolon:
return "coloncolon";
case Kind::eof:
@@ -29,6 +31,8 @@ llvm::StringRef Token::GetTokenName(Kind kind) {
return "identifier";
case Kind::l_paren:
return "l_paren";
+ case Kind::period:
+ return "period";
case Kind::r_paren:
return "r_paren";
case Token::star:
@@ -86,8 +90,9 @@ llvm::Expected<Token> DILLexer::Lex(llvm::StringRef expr,
return Token(Token::identifier, maybe_word->str(), position);
constexpr std::pair<Token::Kind, const char *> operators[] = {
- {Token::amp, "&"}, {Token::coloncolon, "::"}, {Token::l_paren, "("},
- {Token::r_paren, ")"}, {Token::star, "*"},
+ {Token::amp, "&"}, {Token::arrow, "->"}, {Token::coloncolon, "::"},
+ {Token::l_paren, "("}, {Token::period, "."}, {Token::r_paren, ")"},
+ {Token::star, "*"},
};
for (auto [kind, str] : operators) {
if (remainder.consume_front(str))
diff --git a/lldb/source/ValueObject/DILParser.cpp b/lldb/source/ValueObject/DILParser.cpp
index 2c78eae8cf6bf..9c5bc71775fb2 100644
--- a/lldb/source/ValueObject/DILParser.cpp
+++ b/lldb/source/ValueObject/DILParser.cpp
@@ -79,15 +79,15 @@ ASTNodeUP DILParser::Run() {
// Parse an expression.
//
// expression:
-// primary_expression
+// unary_expression
//
ASTNodeUP DILParser::ParseExpression() { return ParseUnaryExpression(); }
// Parse an unary_expression.
//
// unary_expression:
+// postfix_expression
// unary_operator expression
-// primary_expression
//
// unary_operator:
// "&"
@@ -111,7 +111,28 @@ ASTNodeUP DILParser::ParseUnaryExpression() {
llvm_unreachable("invalid token kind");
}
}
- return ParsePrimaryExpression();
+ return ParsePostfixExpression();
+}
+// Parse a postfix_expression.
+//
+// postfix_expression:
+// primary_expression
+// postfix_expression "." id_expression
+// postfix_expression "->" id_expression
+//
+ASTNodeUP DILParser::ParsePostfixExpression() {
+ ASTNodeUP lhs = ParsePrimaryExpression();
+ while (CurToken().IsOneOf({Token::period, Token::arrow})) {
+ Token token = CurToken();
+ m_dil_lexer.Advance();
+ Token member_token = CurToken();
+ std::string member_id = ParseIdExpression();
+ lhs = std::make_unique<MemberOfNode>(member_token.GetLocation(),
+ std::move(lhs),
+ token.GetKind() == Token::arrow,
+ ConstString(member_id));
+ }
+ return lhs;
}
// Parse a primary_expression.
diff --git a/lldb/test/API/commands/frame/var-dil/basics/MemberOf/Makefile b/lldb/test/API/commands/frame/var-dil/basics/MemberOf/Makefile
new file mode 100644
index 0000000000000..99998b20bcb05
--- /dev/null
+++ b/lldb/test/API/commands/frame/var-dil/basics/MemberOf/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
diff --git a/lldb/test/API/commands/frame/var-dil/basics/MemberOf/TestFrameVarDILMemberOf.py b/lldb/test/API/commands/frame/var-dil/basics/MemberOf/TestFrameVarDILMemberOf.py
new file mode 100644
index 0000000000000..ff942c88bf183
--- /dev/null
+++ b/lldb/test/API/commands/frame/var-dil/basics/MemberOf/TestFrameVarDILMemberOf.py
@@ -0,0 +1,47 @@
+"""
+Make sure 'frame var' using DIL parser/evaultor works for local variables.
+"""
+
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from lldbsuite.test import lldbutil
+
+import os
+import shutil
+import time
+
+class TestFrameVarDILMemberOf(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.expect("settings set target.experimental.use-DIL true",
+ substrs=[""])
+ self.expect_var_path("s.x", value="1")
+ self.expect_var_path("s.r", value="2")
+ self.expect_var_path("sr.x", value="1")
+ self.expect_var_path("sr.r", value="2")
+ self.expect_var_path("sp->x", value="1")
+ self.expect_var_path("sp->r", value="2")
+ self.expect_var_path("sarr->x", value="5");
+ self.expect_var_path("sarr->r", value="2")
+
+ self.expect("frame variable 'sp->foo'", error=True,
+ substrs=["no member named 'foo' in 'Sx'"])
+
+ self.expect("frame variable 'sp.x'", error=True,
+ substrs=["member reference type 'Sx *' is a "
+ "pointer; did you mean to use '->'"])
+ self.expect("frame variable 'sarr.x'", error=True,
+ substrs=["no member named 'x' in 'Sx[2]'"])
+
+ # Test for record typedefs.
+ self.expect_var_path("sa.x", value="3")
+ self.expect_var_path("sa.y", value="'\\x04'")
diff --git a/lldb/test/API/commands/frame/var-dil/basics/MemberOf/main.cpp b/lldb/test/API/commands/frame/var-dil/basics/MemberOf/main.cpp
new file mode 100644
index 0000000000000..dace888bef4dc
--- /dev/null
+++ b/lldb/test/API/commands/frame/var-dil/basics/MemberOf/main.cpp
@@ -0,0 +1,59 @@
+int
+main(int argc, char**argv)
+{
+ int x = 2;
+ struct Sx {
+ int x;
+ int& r;
+ char y;
+ } s{1, x, 2};
+
+ Sx& sr = s;
+ Sx* sp = &s;
+
+ Sx sarr[2] = {{5, x, 2}, {1, x, 3}};
+
+ using SxAlias = Sx;
+ SxAlias sa{3, x, 4};
+
+ return 0; // Set a breakpoint here
+}
+
+/*
+ EXPECT_THAT(Eval("s.x"), IsEqual("1"));
+ EXPECT_THAT(Eval("s.r"), IsEqual("2"));
+ EXPECT_THAT(Eval("s.r + 1"), IsEqual("3"));
+ EXPECT_THAT(Eval("sr.x"), IsEqual("1"));
+ EXPECT_THAT(Eval("sr.r"), IsEqual("2"));
+ EXPECT_THAT(Eval("sr.r + 1"), IsEqual("3"));
+ EXPECT_THAT(Eval("sp->x"), IsEqual("1"));
+ EXPECT_THAT(Eval("sp->r"), IsEqual("2"));
+ EXPECT_THAT(Eval("sp->r + 1"), IsEqual("3"));
+ EXPECT_THAT(Eval("sarr->x"), IsEqual("5"));
+ EXPECT_THAT(Eval("sarr->r"), IsEqual("2"));
+ EXPECT_THAT(Eval("sarr->r + 1"), IsEqual("3"));
+ EXPECT_THAT(Eval("(sarr + 1)->x"), IsEqual("1"));
+
+ EXPECT_THAT(
+ Eval("sp->4"),
+ IsError(
+ "<expr>:1:5: expected 'identifier', got: <'4' (numeric_constant)>\n"
+ "sp->4\n"
+ " ^"));
+ EXPECT_THAT(Eval("sp->foo"), IsError("no member named 'foo' in 'Sx'"));
+ EXPECT_THAT(
+ Eval("sp->r / (void*)0"),
+ IsError("invalid operands to binary expression ('int' and 'void *')"));
+
+ EXPECT_THAT(Eval("sp.x"), IsError("member reference type 'Sx *' is a "
+ "pointer; did you mean to use '->'"));
+ EXPECT_THAT(
+ Eval("sarr.x"),
+ IsError(
+ "member reference base type 'Sx[2]' is not a structure or union"));
+
+ // Test for record typedefs.
+ EXPECT_THAT(Eval("sa.x"), IsEqual("3"));
+ EXPECT_THAT(Eval("sa.y"), IsEqual("'\\x04'"));
+
+*/
diff --git a/lldb/test/API/commands/frame/var-dil...
[truncated]
|
You can test this locally with the following command:darker --check --diff -r HEAD~1...HEAD lldb/test/API/commands/frame/var-dil/basics/MemberOf/TestFrameVarDILMemberOf.py lldb/test/API/commands/frame/var-dil/basics/MemberOfAnonymousMember/TestFrameVarDILMemberOfAnonymousMember.py lldb/test/API/commands/frame/var-dil/basics/MemberOfInheritance/TestFrameVarDILMemberOfInheritance.pyView the diff from darker here.--- MemberOf/TestFrameVarDILMemberOf.py 2025-05-22 20:03:49.000000 +0000
+++ MemberOf/TestFrameVarDILMemberOf.py 2025-05-22 20:07:20.201670 +0000
@@ -9,35 +9,43 @@
import os
import shutil
import time
+
class TestFrameVarDILMemberOf(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"))
+ lldbutil.run_to_source_breakpoint(
+ self, "Set a breakpoint here", lldb.SBFileSpec("main.cpp")
+ )
- self.expect("settings set target.experimental.use-DIL true",
- substrs=[""])
+ self.expect("settings set target.experimental.use-DIL true", substrs=[""])
self.expect_var_path("s.x", value="1")
self.expect_var_path("s.r", type="int &")
self.expect_var_path("sr.x", value="1")
self.expect_var_path("sr.r", type="int &")
self.expect_var_path("sp->x", value="1")
self.expect_var_path("sp->r", type="int &")
- self.expect("frame variable 'sp->foo'", error=True,
- substrs=["no member named 'foo' in 'Sx *'"])
+ self.expect(
+ "frame variable 'sp->foo'",
+ error=True,
+ substrs=["no member named 'foo' in 'Sx *'"],
+ )
- self.expect("frame variable 'sp.x'", error=True,
- substrs=["member reference type 'Sx *' is a "
- "pointer; did you mean to use '->'"])
+ self.expect(
+ "frame variable 'sp.x'",
+ error=True,
+ substrs=[
+ "member reference type 'Sx *' is a " "pointer; did you mean to use '->'"
+ ],
+ )
# Test for record typedefs.
self.expect_var_path("sa.x", value="3")
self.expect_var_path("sa.y", value="'\\x04'")
--- MemberOfAnonymousMember/TestFrameVarDILMemberOfAnonymousMember.py 2025-05-22 20:03:49.000000 +0000
+++ MemberOfAnonymousMember/TestFrameVarDILMemberOfAnonymousMember.py 2025-05-22 20:07:20.227505 +0000
@@ -9,29 +9,31 @@
import os
import shutil
import time
+
class TestFrameVarDILMemberOfAnonymousMember(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"))
+ lldbutil.run_to_source_breakpoint(
+ self, "Set a breakpoint here", lldb.SBFileSpec("main.cpp")
+ )
- self.expect("settings set target.experimental.use-DIL true",
- substrs=[""])
+ self.expect("settings set target.experimental.use-DIL true", substrs=[""])
self.expect_var_path("a.x", value="1")
self.expect_var_path("a.y", value="2")
- self.expect("frame variable 'b.x'", error=True,
- substrs=["no member named 'x' in 'B'"])
- #self.expect_var_path("b.y", value="0")
+ self.expect(
+ "frame variable 'b.x'", error=True, substrs=["no member named 'x' in 'B'"]
+ )
+ # self.expect_var_path("b.y", value="0")
self.expect_var_path("b.z", value="3")
self.expect_var_path("b.w", value="4")
self.expect_var_path("b.a.x", value="1")
self.expect_var_path("b.a.y", value="2")
@@ -41,22 +43,30 @@
self.expect_var_path("d.x", value="7")
self.expect_var_path("d.y", value="8")
self.expect_var_path("d.z", value="9")
self.expect_var_path("d.w", value="10")
- self.expect("frame variable 'e.x'", error=True,
- substrs=["no member named 'x' in 'E'"])
- self.expect("frame variable 'f.x'", error=True,
- substrs=["no member named 'x' in 'F'"])
+ self.expect(
+ "frame variable 'e.x'", error=True, substrs=["no member named 'x' in 'E'"]
+ )
+ self.expect(
+ "frame variable 'f.x'", error=True, substrs=["no member named 'x' in 'F'"]
+ )
self.expect_var_path("f.named_field.x", value="12")
self.expect_var_path("unnamed_derived.y", value="2")
self.expect_var_path("unnamed_derived.z", value="13")
- self.expect("frame variable 'derb.x'", error=True,
- substrs=["no member named 'x' in 'DerivedB'"])
- self.expect("frame variable 'derb.y'", error=True,
- substrs=["no member named 'y' in 'DerivedB'"])
+ self.expect(
+ "frame variable 'derb.x'",
+ error=True,
+ substrs=["no member named 'x' in 'DerivedB'"],
+ )
+ self.expect(
+ "frame variable 'derb.y'",
+ error=True,
+ substrs=["no member named 'y' in 'DerivedB'"],
+ )
self.expect_var_path("derb.w", value="14")
self.expect_var_path("derb.k", value="15")
self.expect_var_path("derb.a.x", value="1")
self.expect_var_path("derb.a.y", value="2")
--- MemberOfInheritance/TestFrameVarDILMemberOfInheritance.py 2025-05-22 20:03:49.000000 +0000
+++ MemberOfInheritance/TestFrameVarDILMemberOfInheritance.py 2025-05-22 20:07:20.248298 +0000
@@ -9,23 +9,24 @@
import os
import shutil
import time
+
class TestFrameVarDILMemberOfInheritance(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"))
+ lldbutil.run_to_source_breakpoint(
+ self, "Set a breakpoint here", lldb.SBFileSpec("main.cpp")
+ )
- self.expect("settings set target.experimental.use-DIL true",
- substrs=[""])
+ self.expect("settings set target.experimental.use-DIL true", substrs=[""])
self.expect_var_path("a.a_", value="1")
self.expect_var_path("b.b_", value="2")
self.expect_var_path("c.a_", value="1")
self.expect_var_path("c.b_", value="2")
self.expect_var_path("c.c_", value="3")
|
You can test this locally with the following command:git-clang-format --diff HEAD~1 HEAD --extensions cpp,h -- lldb/test/API/commands/frame/var-dil/basics/MemberOf/main.cpp lldb/test/API/commands/frame/var-dil/basics/MemberOfAnonymousMember/main.cpp lldb/test/API/commands/frame/var-dil/basics/MemberOfInheritance/main.cpp lldb/include/lldb/ValueObject/DILAST.h lldb/include/lldb/ValueObject/DILEval.h lldb/include/lldb/ValueObject/DILLexer.h lldb/include/lldb/ValueObject/DILParser.h lldb/source/Target/StackFrame.cpp lldb/source/ValueObject/DILAST.cpp lldb/source/ValueObject/DILEval.cpp lldb/source/ValueObject/DILLexer.cpp lldb/source/ValueObject/DILParser.cppView the diff from clang-format here.diff --git a/lldb/test/API/commands/frame/var-dil/basics/MemberOf/main.cpp b/lldb/test/API/commands/frame/var-dil/basics/MemberOf/main.cpp
index 80e8c064b..4bb46d9ce 100644
--- a/lldb/test/API/commands/frame/var-dil/basics/MemberOf/main.cpp
+++ b/lldb/test/API/commands/frame/var-dil/basics/MemberOf/main.cpp
@@ -1,15 +1,13 @@
-int
-main(int argc, char**argv)
-{
+int main(int argc, char **argv) {
int x = 2;
struct Sx {
int x;
- int& r;
+ int &r;
char y;
} s{1, x, 2};
- Sx& sr = s;
- Sx* sp = &s;
+ Sx &sr = s;
+ Sx *sp = &s;
using SxAlias = Sx;
SxAlias sa{3, x, 4};
diff --git a/lldb/test/API/commands/frame/var-dil/basics/MemberOfAnonymousMember/main.cpp b/lldb/test/API/commands/frame/var-dil/basics/MemberOfAnonymousMember/main.cpp
index 6237523ac..c787f29f9 100644
--- a/lldb/test/API/commands/frame/var-dil/basics/MemberOfAnonymousMember/main.cpp
+++ b/lldb/test/API/commands/frame/var-dil/basics/MemberOfAnonymousMember/main.cpp
@@ -1,5 +1,4 @@
-int main(int argc, char** argv)
-{
+int main(int argc, char **argv) {
struct A {
struct {
int x = 1;
@@ -22,7 +21,7 @@ int main(int argc, char** argv)
int x = 5;
};
class {
- public:
+ public:
int y = 6;
};
} c;
diff --git a/lldb/test/API/commands/frame/var-dil/basics/MemberOfInheritance/main.cpp b/lldb/test/API/commands/frame/var-dil/basics/MemberOfInheritance/main.cpp
index 4d29af1b7..27177f3f0 100644
--- a/lldb/test/API/commands/frame/var-dil/basics/MemberOfInheritance/main.cpp
+++ b/lldb/test/API/commands/frame/var-dil/basics/MemberOfInheritance/main.cpp
@@ -1,5 +1,4 @@
-int main(int argc, char** argv)
-{
+int main(int argc, char **argv) {
struct A {
int a_;
} a{1};
@@ -38,7 +37,7 @@ int main(int argc, char** argv)
} bat;
bat.weight_ = 10;
- // Empty bases example.
+ // Empty bases example.
struct IPlugin {
virtual ~IPlugin() {}
};
@@ -74,14 +73,14 @@ int main(int argc, char** argv)
struct Mixin {};
struct Parent : private Mixin, public Base {
int z;
- virtual void Do(){};
+ virtual void Do() {};
};
Parent obj;
obj.x = 1;
obj.y = 2;
obj.z = 3;
- Base* parent_base = &obj;
- Parent* parent = &obj;
+ Base *parent_base = &obj;
+ Parent *parent = &obj;
return 0; // Set a breakpoint here
}
|
labath
reviewed
May 2, 2025
There was a problem hiding this comment.
A very predictable question. Why is the implementation of this more complicated than base->GetChildMemberWithName(name). I think it shouldn't be.
To avoid repeating the same points over and over, I suggest going through the discussion on the dereference/addressof patch. If you still have questions after that, let me know.
|
|
||
| ASTNode *base() const { return m_base.get(); } | ||
| bool IsArrow() const { return m_is_arrow; } | ||
| ConstString FieldName() const { return m_field_name; } |
There was a problem hiding this comment.
Let's not use ConstString here. All this does here is increase lldb memory consumption.
Comment on lines
101
to
102
| ASTNode *base() const { return m_base.get(); } | ||
| bool IsArrow() const { return m_is_arrow; } |
There was a problem hiding this comment.
Let's be consistent with the names here. I see we've used GetFoo elsewhere.
@kuilpd, I missed this in the UnaryOpNode (because it was self-consistent at least, but I think that one should also use the GetFoo scheme.
lldb/source/ValueObject/DILEval.cpp
Outdated
| lldb::ValueObjectSP | ||
| Interpreter::EvaluateMemberOf(lldb::ValueObjectSP value, | ||
| const std::vector<uint32_t> &path, | ||
| bool use_synthetic, bool is_dynamic) { |
There was a problem hiding this comment.
use_dynamic was passed in the the parser as a lldb::DynamicValueType. Why did it turn into a bool here?
There was a problem hiding this comment.
Removed this function so comment doesn't matter now.
ValueObject::GetChildMemberWithName. Also minor code cleanups requested by reviewer.
GetChildMemberWithName didn't handle everything correctly. I've fixed part of that now. There's still a bug in the code that searches for fields in base classes, which I want to try to find & fix. I've removed all the special handling code from this PR. Please review again. Thanks! |
labath
reviewed
May 12, 2025
|
|
||
| ASTNode *GetBase() const { return m_base.get(); } | ||
| bool GetIsArrow() const { return m_is_arrow; } | ||
| std::string GetFieldName() const { return m_field_name; } |
There was a problem hiding this comment.
Suggested change
| std::string GetFieldName() const { return m_field_name; } | |
| llvm::StringRef GetFieldName() const { return m_field_name; } |
lldb/source/ValueObject/DILEval.cpp
Outdated
| @@ -272,4 +272,66 @@ Interpreter::Visit(const UnaryOpNode *node) { | |||
| m_expr, "invalid ast: unexpected binary operator", node->GetLocation()); | |||
| } | |||
|
|
|||
| llvm::Expected<lldb::ValueObjectSP> | |||
| Interpreter::Visit(const MemberOfNode *node) { | |||
| Status error; | |||
There was a problem hiding this comment.
Declare it where it's being used to make it clear what's its scope and contents.
lldb/source/ValueObject/DILEval.cpp
Outdated
Comment on lines
279
to
281
| if (!base_or_err) { | ||
| return base_or_err; | ||
| } |
There was a problem hiding this comment.
lldb/source/ValueObject/DILEval.cpp
Outdated
Comment on lines
288
to
289
| if (node->GetIsArrow() && !base_type.IsPointerType() && | ||
| !base_type.IsArrayType()) { |
There was a problem hiding this comment.
I think we should drop the type checks and perform the dereference unconditionally.
There was a problem hiding this comment.
Why? The current "frame var" implementation does perform these checks and return these errors...
There was a problem hiding this comment.
Because we want to let the values (and their type systems) decide whether they are dereferencable or not. That's what we did for the implementation of the * operator.
I could be convinced by the "status quo" argument, but this doesn't look like an equivalent implementation to that (for one, you're adding support for dereferencing arrays), which is why I want to turn this around: if * does not check for types, then why should -> (which is defined as (*x).) do that?
What I can imagine is doing something on the error handling path -- if dereferencing failed, we look at the value to provide a better error message about why it failed. However, even in this case, I'd try to avoid looking at the type too much and instead try to reuse the error message from the Dereference operation (changing it, if needed) so that we can be sure it makes sense regardless of how the value depends to implement dereferencing.
lldb/source/ValueObject/DILEval.cpp
Outdated
| return llvm::make_error<DILDiagnosticError>( | ||
| m_expr, errMsg, node->GetLocation(), node->GetFieldName().size()); | ||
| } | ||
| } else if (!node->GetIsArrow() && base_type.IsPointerType()) { |
There was a problem hiding this comment.
Same question/response as before.
lldb/source/ValueObject/DILEval.cpp
Outdated
Comment on lines
312
to
313
| if (node->GetIsArrow() && base_type.IsArrayType()) | ||
| base = base->GetChildAtIndex(0); |
There was a problem hiding this comment.
This probably shouldn't exist
lldb/source/ValueObject/DILEval.cpp
Outdated
Comment on lines
319
to
320
| if (field_obj->GetCompilerType().IsReferenceType()) { | ||
| lldb::ValueObjectSP tmp_obj = field_obj->Dereference(error); |
There was a problem hiding this comment.
Why is this necessary? GetChildMemberWithName seems to work just fine on references:
(lldb) script lldb.frame.FindVariable("b")
(A &) b = 0x00007fffffffd7e8: {
X = 42
x = 0x00007fffffffd7e8
}
(lldb) script lldb.frame.FindVariable("b").GetChildMemberWithName("X")
(int) X = 42
There was a problem hiding this comment.
We can remove it; I'll do that.
Comment on lines
22
to
59
| /* | ||
| EXPECT_THAT(Eval("s.x"), IsEqual("1")); | ||
| EXPECT_THAT(Eval("s.r"), IsEqual("2")); | ||
| EXPECT_THAT(Eval("s.r + 1"), IsEqual("3")); | ||
| EXPECT_THAT(Eval("sr.x"), IsEqual("1")); | ||
| EXPECT_THAT(Eval("sr.r"), IsEqual("2")); | ||
| EXPECT_THAT(Eval("sr.r + 1"), IsEqual("3")); | ||
| EXPECT_THAT(Eval("sp->x"), IsEqual("1")); | ||
| EXPECT_THAT(Eval("sp->r"), IsEqual("2")); | ||
| EXPECT_THAT(Eval("sp->r + 1"), IsEqual("3")); | ||
| EXPECT_THAT(Eval("sarr->x"), IsEqual("5")); | ||
| EXPECT_THAT(Eval("sarr->r"), IsEqual("2")); | ||
| EXPECT_THAT(Eval("sarr->r + 1"), IsEqual("3")); | ||
| EXPECT_THAT(Eval("(sarr + 1)->x"), IsEqual("1")); | ||
|
|
||
| EXPECT_THAT( | ||
| Eval("sp->4"), | ||
| IsError( | ||
| "<expr>:1:5: expected 'identifier', got: <'4' (numeric_constant)>\n" | ||
| "sp->4\n" | ||
| " ^")); | ||
| EXPECT_THAT(Eval("sp->foo"), IsError("no member named 'foo' in 'Sx'")); | ||
| EXPECT_THAT( | ||
| Eval("sp->r / (void*)0"), | ||
| IsError("invalid operands to binary expression ('int' and 'void *')")); | ||
|
|
||
| EXPECT_THAT(Eval("sp.x"), IsError("member reference type 'Sx *' is a " | ||
| "pointer; did you mean to use '->'")); | ||
| EXPECT_THAT( | ||
| Eval("sarr.x"), | ||
| IsError( | ||
| "member reference base type 'Sx[2]' is not a structure or union")); | ||
|
|
||
| // Test for record typedefs. | ||
| EXPECT_THAT(Eval("sa.x"), IsEqual("3")); | ||
| EXPECT_THAT(Eval("sa.y"), IsEqual("'\\x04'")); | ||
|
|
||
| */ |
There was a problem hiding this comment.
Whoops; notes to self re original tests in lldb-eval. Will remove that.
- Formatting issues. - Update the evalution of MemberOf to NOT automatically deref references - Update tests to match. - Change GetFieldName to return a StringRef instead of a std::string.
- Remove code that automatically dereferenced result. - Update member-of logic to more closely match current frame var implementation.
|
I believe I have addressed all the review comments so far. Please take another look. Thanks! |
labath
reviewed
May 22, 2025
Comment on lines
109
to
112
| bool GetCheckPtrVsMember() const { return m_check_ptr_vs_member; } | ||
| bool GetFragileIvar() const { return m_fragile_ivar; } | ||
| bool GetSynthChild() const { return m_use_synth_child; } | ||
| lldb::DynamicValueType GetUseDynamic() const { return m_use_dynamic; } |
There was a problem hiding this comment.
I think we discussed this (briefly) at one of the previous PRs, and I think the conclusion was that things like these should be a property of the interpreter rather than of a specific node.
I can sort of imagine a world some parts of an expression are evaluated using dynamic types and some aren't, but I don't think it's your intention to create that world. (And if it is, then these properties (at least some of them), should be other nodes as well, as e.g. [] also needs to know whether it should be looking at synthetic children).
labath
approved these changes
May 23, 2025
|
@cmtice I'm getting: Would you mind taking a look? Should I just remove these variables, adjust the constructor, and the callers of the constructor? Thanks! |
I'll try to get a fix for this asap. |
I think this should fix the problem: #141259 . Kazu, could you verify that it does please? |
sivan-shani
pushed a commit
to sivan-shani/llvm-project
that referenced
this pull request
Jun 3, 2025
Add the arrow and period operators, allowing DIL to find and access member fields.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add the arrow and period operators, allowing DIL to find and access member fields.