[LLDB] Update DIL to pass current 'frame var' tests. #145055
Conversation
As a preliminary to making DIL the default implementation for 'frame var', ran check-lldb forcing 'frame var' to always use DIL, and discovered a few failing tests. This fixes most of them. The only two remaining failing tests (once the smart pointer PR is committed) are TestVarPath.py, which fails a test case using a negative array subscript, as DIL does not yet parse negative numbers; and TestDAP_evaluate.py, which now passes a test case that the test says should fail (still investigating this). Changes in this PR: - Sets correct VariableSP, as well as returning ValueObjectSP (needed for several watchpoint tests). - Update error messages, when looking up members, to match what the rest of LLDB expects. Also update appropriate DIL tests to expect the updated error messages. - Update DIL parser to look for and accept "(anonymous namespace)::" at the front of a variable name.
|
@llvm/pr-subscribers-lldb Author: None (cmtice) ChangesAs a preliminary to making DIL the default implementation for 'frame var', ran check-lldb forcing 'frame var' to always use DIL, and discovered a few failing tests. This fixes most of them. The only two remaining failing tests (once the smart pointer PR is committed) are TestVarPath.py, which fails a test case using a negative array subscript, as DIL does not yet parse negative numbers; and TestDAP_evaluate.py, which now passes a test case that the test says should fail (still investigating this). Changes in this PR:
Full diff: https://github.com/llvm/llvm-project/pull/145055.diff 5 Files Affected:
diff --git a/lldb/source/Target/StackFrame.cpp b/lldb/source/Target/StackFrame.cpp
index ab5cd0b27c789..f5a80efc821d5 100644
--- a/lldb/source/Target/StackFrame.cpp
+++ b/lldb/source/Target/StackFrame.cpp
@@ -562,6 +562,7 @@ ValueObjectSP StackFrame::DILGetValueForVariableExpressionPath(
return ValueObjectConstResult::Create(nullptr, std::move(error));
}
+ var_sp = (*valobj_or_error)->GetVariable();
return *valobj_or_error;
}
diff --git a/lldb/source/ValueObject/DILEval.cpp b/lldb/source/ValueObject/DILEval.cpp
index c8cb54aa18a93..2c4d355a2c6b8 100644
--- a/lldb/source/ValueObject/DILEval.cpp
+++ b/lldb/source/ValueObject/DILEval.cpp
@@ -356,8 +356,8 @@ Interpreter::Visit(const MemberOfNode *node) {
if (!m_use_synthetic || !field_obj) {
std::string errMsg = llvm::formatv(
- "no member named '{0}' in {1}", node->GetFieldName(),
- base->GetCompilerType().GetFullyUnqualifiedType().TypeDescription());
+ "\"{0}\" is not a member of \"({1}) {2}\"", node->GetFieldName(),
+ base->GetTypeName().AsCString("<invalid type>"), base->GetName());
return llvm::make_error<DILDiagnosticError>(
m_expr, errMsg, node->GetLocation(), node->GetFieldName().size());
}
@@ -376,9 +376,9 @@ Interpreter::Visit(const MemberOfNode *node) {
CompilerType base_type = base->GetCompilerType();
if (node->GetIsArrow() && base->IsPointerType())
base_type = base_type.GetPointeeType();
- std::string errMsg =
- llvm::formatv("no member named '{0}' in {1}", node->GetFieldName(),
- base_type.GetFullyUnqualifiedType().TypeDescription());
+ std::string errMsg = llvm::formatv(
+ "\"{0}\" is not a member of \"({1}) {2}\"", node->GetFieldName(),
+ base->GetTypeName().AsCString("<invalid type>"), base->GetName());
return llvm::make_error<DILDiagnosticError>(
m_expr, errMsg, node->GetLocation(), node->GetFieldName().size());
}
diff --git a/lldb/source/ValueObject/DILParser.cpp b/lldb/source/ValueObject/DILParser.cpp
index 9667885734f21..37d5bd30fa61f 100644
--- a/lldb/source/ValueObject/DILParser.cpp
+++ b/lldb/source/ValueObject/DILParser.cpp
@@ -178,11 +178,40 @@ ASTNodeUP DILParser::ParsePrimaryExpression() {
}
if (CurToken().Is(Token::l_paren)) {
- m_dil_lexer.Advance();
- auto expr = ParseExpression();
- Expect(Token::r_paren);
- m_dil_lexer.Advance();
- return expr;
+ // Check in case this is an anonynmous namespace
+ if (m_dil_lexer.LookAhead(1).Is(Token::identifier) &&
+ (m_dil_lexer.LookAhead(1).GetSpelling() == "anonymous") &&
+ m_dil_lexer.LookAhead(2).Is(Token::identifier) &&
+ (m_dil_lexer.LookAhead(2).GetSpelling() == "namespace") &&
+ m_dil_lexer.LookAhead(3).Is(Token::r_paren) &&
+ m_dil_lexer.LookAhead(4).Is(Token::coloncolon)) {
+ m_dil_lexer.Advance(4);
+
+ std::string identifier = "(anonymous namespace)";
+ Expect(Token::coloncolon);
+ // Save the source location for the diagnostics message.
+ uint32_t loc = CurToken().GetLocation();
+ m_dil_lexer.Advance();
+ assert(
+ (CurToken().Is(Token::identifier) || CurToken().Is(Token::l_paren)) &&
+ "Expected an identifier or anonymous namespeace, but not found.");
+ std::string identifier2 = ParseNestedNameSpecifier();
+ if (identifier2.empty()) {
+ // There was only an identifer, no more levels of nesting. Or there
+ // was an invalid expression starting with a left parenthesis.
+ Expect(Token::identifier);
+ identifier2 = CurToken().GetSpelling();
+ m_dil_lexer.Advance();
+ }
+ identifier = identifier + "::" + identifier2;
+ return std::make_unique<IdentifierNode>(loc, identifier);
+ } else {
+ m_dil_lexer.Advance();
+ auto expr = ParseExpression();
+ Expect(Token::r_paren);
+ m_dil_lexer.Advance();
+ return expr;
+ }
}
BailOut(llvm::formatv("Unexpected token: {0}", CurToken()),
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
index bb16c1f82489d..c204b75941a2f 100644
--- a/lldb/test/API/commands/frame/var-dil/basics/MemberOf/TestFrameVarDILMemberOf.py
+++ b/lldb/test/API/commands/frame/var-dil/basics/MemberOf/TestFrameVarDILMemberOf.py
@@ -32,7 +32,7 @@ def test_frame_var(self):
self.expect_var_path("sp->r", type="int &")
self.expect("frame variable 'sp->foo'", error=True,
- substrs=["no member named 'foo' in 'Sx *'"])
+ substrs=["\"foo\" is not a member of \"(Sx *) sp\""])
self.expect("frame variable 'sp.x'", error=True,
substrs=["member reference type 'Sx *' is a "
diff --git a/lldb/test/API/commands/frame/var-dil/basics/MemberOfAnonymousMember/TestFrameVarDILMemberOfAnonymousMember.py b/lldb/test/API/commands/frame/var-dil/basics/MemberOfAnonymousMember/TestFrameVarDILMemberOfAnonymousMember.py
index 1bde4706da90f..eb6261aefb5c8 100644
--- a/lldb/test/API/commands/frame/var-dil/basics/MemberOfAnonymousMember/TestFrameVarDILMemberOfAnonymousMember.py
+++ b/lldb/test/API/commands/frame/var-dil/basics/MemberOfAnonymousMember/TestFrameVarDILMemberOfAnonymousMember.py
@@ -28,7 +28,7 @@ def test_frame_var(self):
self.expect_var_path("a.y", value="2")
self.expect("frame variable 'b.x'", error=True,
- substrs=["no member named 'x' in 'B'"])
+ substrs=["\"x\" is not a member of \"(B) 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")
@@ -44,18 +44,18 @@ def test_frame_var(self):
self.expect_var_path("d.w", value="10")
self.expect("frame variable 'e.x'", error=True,
- substrs=["no member named 'x' in 'E'"])
+ substrs=["\"x\" is not a member of \"(E) e\""])
self.expect("frame variable 'f.x'", error=True,
- substrs=["no member named 'x' in 'F'"])
+ substrs=["\"x\" is not a member of \"(F) 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'"])
+ substrs=["\"x\" is not a member of \"(DerivedB) derb\""])
self.expect("frame variable 'derb.y'", error=True,
- substrs=["no member named 'y' in 'DerivedB'"])
+ substrs=["\"y\" is not a member of \"(DerivedB) derb\""])
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")
|
|
✅ With the latest revision this PR passed the Python code formatter. |
|
Question: Should we update DIL to parse/handle negative integers before making the switch? (e.g. to pass the TestVarPath test) |
labath
left a comment
labath
left a comment
There was a problem hiding this comment.
If the test you found the anonymous namespace issue it is explicitly testing the handling of anonymous namespaces, then this is fine. However, if it's doing that as a side-effect of testing something else, then it may be nice to have an explicit test case for it. (Particularly as the anonymous namespace thing can appear in other positions as well (ns1::(anonymous namespace)::foo, (anonymous namespace)::ns::(anonymous namespace)::foo, etc.)
| assert( | ||
| (CurToken().Is(Token::identifier) || CurToken().Is(Token::l_paren)) && | ||
| "Expected an identifier or anonymous namespeace, but not found."); |
There was a problem hiding this comment.
An assert is fine for an internal invariant, but this looks like its checking the next token in the stream. That's can't be right. Or am I misunderstanding something?
There was a problem hiding this comment.
No -- I don't think I am -- I just made lldb crash with this assertion.
There was a problem hiding this comment.
(lldb) frame variable 'lldb::(anonymous namespace)::a'
lldb: lldb/source/ValueObject/DILParser.cpp:235: std::string lldb_private::dil::DILP
arser::ParseNestedNameSpecifier(): Assertion `(CurToken().Is(Token::identifier) || CurToken().Is(Token::l
_paren)) && "Expected an identifier or anonymous namespace, but not found."' failed.
| m_dil_lexer.LookAhead(4).Is(Token::coloncolon)) { | ||
| m_dil_lexer.Advance(4); | ||
|
|
||
| std::string identifier = "(anonymous namespace)"; |
There was a problem hiding this comment.
Instead of basically inlining a part of ParseIdExpression, could we find a way to delegate to it? What would happen if we changed the check on line 172 to say "if next token is :: or identifier OR next 4 tokens are '(anonymous namespace)'" ?
The test I found is lldb-api :: lang/cpp/namespace/TestNamespace.py, which does a basic test of '(anonymous namespace)' at the start of a variable name, but not embedded in the variable name. I agree that we should test it in other positions as well. I will add that to one of the DIL tests. |
- Replace assert statements with appropriate error checks & handling. - Consolidate code checking for 'anonymous namespace'. - Fix various small issues; add checks for 'anonymous namespaces' in multiple different positions.
|
I think I have addressed all your concerns. Please take another look; thanks! |
labath
left a comment
labath
left a comment
There was a problem hiding this comment.
It's better, but it still looks like there's an inlined copy of ParseIdExpression inside ParsePrimaryExpression. I'd be nice to avoid that as it's easy to forget the inlined version when updating ParseIdExpression (and the problem would show up on variables inside anonymous namespaces).
Maybe that's impossible, but if so, I'd like to know why.
| std::string unqualified_id = ParseUnqualifiedId(); | ||
| return std::make_unique<IdentifierNode>(loc, nested_name_specifier + | ||
| unqualified_id); | ||
| } else { |
| m_dil_lexer.Advance(); | ||
| if (!CurToken().Is(Token::identifier) && !CurToken().Is(Token::l_paren)) { | ||
| BailOut( | ||
| "Expected an identifier or anonymous namespeace, but not found.", |
Rearrange code slightyl to ParsePrimaryExpression doesn't duplicate code in ParseIdExpression. Also fix type & return-after-else.
Done. :-) |
3 participants
As a preliminary to making DIL the default implementation for 'frame var', ran check-lldb forcing 'frame var' to always use DIL, and discovered a few failing tests. This fixes most of them. The only two remaining failing tests (once the smart pointer PR is committed) are TestVarPath.py, which fails a test case using a negative array subscript, as DIL does not yet parse negative numbers; and TestDAP_evaluate.py, which now passes a test case that the test says should fail (still investigating this).
Changes in this PR: