[LLDB] Add type casting to DIL, part 1 of 3. #165199
Conversation
This is an alternative to PR 159500, breaking that PR down into three separate PRs, to make it easier to review. This first PR of the three adds the basic framework for doing type casing to the DIL code, but it does not actually do any casting: In this PR the DIL parser only recognizes builtin type names, and the DIL interpreter does not do anything except return the original operand (no casting). The second and third PRs will add most of the type parsing, and do the actual type casting, respectively.
|
@llvm/pr-subscribers-lldb Author: None (cmtice) ChangesThis is an alternative to PR 159500, breaking that PR down into three separate PRs, to make it easier to review. This first PR of the three adds the basic framework for doing type casing to the DIL code, but it does not actually do any casting: In this PR the DIL parser only recognizes builtin type names, and the DIL interpreter does not do anything except return the original operand (no casting). The second and third PRs will add most of the type parsing, and do the actual type casting, respectively. Full diff: https://github.com/llvm/llvm-project/pull/165199.diff 7 Files Affected:
diff --git a/lldb/docs/dil-expr-lang.ebnf b/lldb/docs/dil-expr-lang.ebnf
index 70eda3bf40650..660a69ea51e7f 100644
--- a/lldb/docs/dil-expr-lang.ebnf
+++ b/lldb/docs/dil-expr-lang.ebnf
@@ -3,10 +3,13 @@
(* This is currently a subset of the final DIL Language, matching the current
DIL implementation. *)
-expression = unary_expression ;
+expression = cast_expression;
+
+cast_expression = unary_expression
+ | "(" type_id ")" cast_expression;
unary_expression = postfix_expression
- | unary_operator expression ;
+ | unary_operator cast_expression ;
unary_operator = "*" | "&" ;
@@ -44,10 +47,28 @@ nested_name_specifier = type_name "::"
| namespace_name '::'
| nested_name_specifier identifier "::" ;
+type_id = type_specifier_seq [abstract_declarator] ;
+
+type_specifier_seq = type_specifier [type_specifier];
+
+type_specifier = ["::"] [nested_name_specifier] type_name
+ | builtin_typename ;
+
+nested_name_specifier = type_name "::"
+ | namespace_name "::"
+ | nested_name_specifier identifier "::" ;
+
+abstract_declarator = ptr_operator [abstract_declarator] ;
+
+ptr_operator = "*"
+ | "&";
+
type_name = class_name
| enum_name
| typedef_name;
+builtin_typename = identifier_seq;
+
class_name = identifier ;
enum_name = identifier ;
@@ -56,6 +77,7 @@ typedef_name = identifier ;
namespace_name = identifier ;
-
+identifier_seq = identifier
+ | identifier identifier_seq;
diff --git a/lldb/include/lldb/ValueObject/DILAST.h b/lldb/include/lldb/ValueObject/DILAST.h
index 0f05d753f1b56..fa0368de0c5bf 100644
--- a/lldb/include/lldb/ValueObject/DILAST.h
+++ b/lldb/include/lldb/ValueObject/DILAST.h
@@ -21,6 +21,7 @@ enum class NodeKind {
eArraySubscriptNode,
eBitExtractionNode,
eBooleanLiteralNode,
+ eCStyleCastNode,
eErrorNode,
eFloatLiteralNode,
eIdentifierNode,
@@ -35,6 +36,14 @@ enum class UnaryOpKind {
Deref, // "*"
};
+/// The C-Style casts allowed by DIL.
+enum class CStyleCastKind {
+ eEnumeration,
+ eNullptr,
+ eReference,
+ eNone,
+};
+
/// Forward declaration, for use in DIL AST nodes. Definition is at the very
/// end of this file.
class Visitor;
@@ -244,6 +253,29 @@ class BooleanLiteralNode : public ASTNode {
bool m_value;
};
+class CStyleCastNode : public ASTNode {
+public:
+ CStyleCastNode(uint32_t location, CompilerType type, ASTNodeUP operand,
+ CStyleCastKind kind)
+ : ASTNode(location, NodeKind::eCStyleCastNode), m_type(type),
+ m_operand(std::move(operand)), m_cast_kind(kind) {}
+
+ llvm::Expected<lldb::ValueObjectSP> Accept(Visitor *v) const override;
+
+ CompilerType GetType() const { return m_type; }
+ ASTNode *GetOperand() const { return m_operand.get(); }
+ CStyleCastKind GetCastKind() const { return m_cast_kind; }
+
+ static bool classof(const ASTNode *node) {
+ return node->GetKind() == NodeKind::eCStyleCastNode;
+ }
+
+private:
+ CompilerType m_type;
+ ASTNodeUP m_operand;
+ CStyleCastKind m_cast_kind;
+};
+
/// This class contains one Visit method for each specialized type of
/// DIL AST node. The Visit methods are used to dispatch a DIL AST node to
/// the correct function in the DIL expression evaluator for evaluating that
@@ -267,6 +299,8 @@ class Visitor {
Visit(const FloatLiteralNode *node) = 0;
virtual llvm::Expected<lldb::ValueObjectSP>
Visit(const BooleanLiteralNode *node) = 0;
+ virtual llvm::Expected<lldb::ValueObjectSP>
+ Visit(const CStyleCastNode *node) = 0;
};
} // namespace lldb_private::dil
diff --git a/lldb/include/lldb/ValueObject/DILEval.h b/lldb/include/lldb/ValueObject/DILEval.h
index eab3218ff828f..a7fe3d3fe7461 100644
--- a/lldb/include/lldb/ValueObject/DILEval.h
+++ b/lldb/include/lldb/ValueObject/DILEval.h
@@ -60,6 +60,8 @@ class Interpreter : Visitor {
Visit(const FloatLiteralNode *node) override;
llvm::Expected<lldb::ValueObjectSP>
Visit(const BooleanLiteralNode *node) override;
+ llvm::Expected<lldb::ValueObjectSP>
+ Visit(const CStyleCastNode *node) override;
llvm::Expected<CompilerType>
PickIntegerType(lldb::TypeSystemSP type_system,
diff --git a/lldb/include/lldb/ValueObject/DILParser.h b/lldb/include/lldb/ValueObject/DILParser.h
index d17ed66d9b3ee..2db23749168d4 100644
--- a/lldb/include/lldb/ValueObject/DILParser.h
+++ b/lldb/include/lldb/ValueObject/DILParser.h
@@ -101,6 +101,12 @@ class DILParser {
ASTNodeUP ParseFloatingPointLiteral();
ASTNodeUP ParseBooleanLiteral();
+ ASTNodeUP ParseCastExpression();
+ std::optional<CompilerType> ParseBuiltinType();
+ std::optional<CompilerType> ParseTypeId();
+ CompilerType ResolveTypeDeclarators(CompilerType type,
+ const std::vector<Token> &ptr_operators);
+
void BailOut(const std::string &error, uint32_t loc, uint16_t err_len);
void Expect(Token::Kind kind);
diff --git a/lldb/source/ValueObject/DILAST.cpp b/lldb/source/ValueObject/DILAST.cpp
index 7ed34db6e20df..5922999baaa52 100644
--- a/lldb/source/ValueObject/DILAST.cpp
+++ b/lldb/source/ValueObject/DILAST.cpp
@@ -51,4 +51,8 @@ BooleanLiteralNode::Accept(Visitor *v) const {
return v->Visit(this);
}
+llvm::Expected<lldb::ValueObjectSP> CStyleCastNode::Accept(Visitor *v) const {
+ return v->Visit(this);
+}
+
} // namespace lldb_private::dil
diff --git a/lldb/source/ValueObject/DILEval.cpp b/lldb/source/ValueObject/DILEval.cpp
index a9dbfad298d05..968394e95ab67 100644
--- a/lldb/source/ValueObject/DILEval.cpp
+++ b/lldb/source/ValueObject/DILEval.cpp
@@ -608,4 +608,16 @@ Interpreter::Visit(const BooleanLiteralNode *node) {
return ValueObject::CreateValueObjectFromBool(m_target, value, "result");
}
+llvm::Expected<lldb::ValueObjectSP>
+Interpreter::Visit(const CStyleCastNode *node) {
+ auto operand_or_err = Evaluate(node->GetOperand());
+ if (!operand_or_err)
+ return operand_or_err;
+
+ lldb::ValueObjectSP operand = *operand_or_err;
+ // Don't actually do the cast for now -- that code will be added later.
+ // For now just return the original operand, unchanged.
+ return operand;
+}
+
} // namespace lldb_private::dil
diff --git a/lldb/source/ValueObject/DILParser.cpp b/lldb/source/ValueObject/DILParser.cpp
index 566bcaf81094a..9c4a0cd576b5c 100644
--- a/lldb/source/ValueObject/DILParser.cpp
+++ b/lldb/source/ValueObject/DILParser.cpp
@@ -12,7 +12,9 @@
//===----------------------------------------------------------------------===//
#include "lldb/ValueObject/DILParser.h"
+#include "lldb/Symbol/CompileUnit.h"
#include "lldb/Target/ExecutionContextScope.h"
+#include "lldb/Target/LanguageRuntime.h"
#include "lldb/Utility/DiagnosticsRendering.h"
#include "lldb/ValueObject/DILAST.h"
#include "lldb/ValueObject/DILEval.h"
@@ -80,15 +82,62 @@ ASTNodeUP DILParser::Run() {
// Parse an expression.
//
// expression:
-// unary_expression
+// cast_expression
//
-ASTNodeUP DILParser::ParseExpression() { return ParseUnaryExpression(); }
+ASTNodeUP DILParser::ParseExpression() { return ParseCastExpression(); }
+
+// Parse a cast_expression.
+//
+// cast_expression:
+// unary_expression
+// "(" type_id ")" cast_expression
+
+ASTNodeUP DILParser::ParseCastExpression() {
+ // This can be a C-style cast, try parsing the contents as a type declaration.
+ if (CurToken().Is(Token::l_paren)) {
+ Token token = CurToken();
+ uint32_t loc = token.GetLocation();
+
+ // Enable lexer backtracking, so that we can rollback in case it's not
+ // actually a type declaration.
+
+ // Start tentative parsing (save token location/idx, for possible rollback).
+ uint32_t save_token_idx = m_dil_lexer.GetCurrentTokenIdx();
+
+ // Consume the token only after enabling the backtracking.
+ m_dil_lexer.Advance();
+
+ // Try parsing the type declaration. If the returned value is not valid,
+ // then we should rollback and try parsing the expression.
+ auto type_id = ParseTypeId();
+ if (type_id) {
+ // Successfully parsed the type declaration. Commit the backtracked
+ // tokens and parse the cast_expression.
+
+ if (!type_id.value().IsValid())
+ return std::make_unique<ErrorNode>();
+
+ Expect(Token::r_paren);
+ m_dil_lexer.Advance();
+ auto rhs = ParseCastExpression();
+
+ return std::make_unique<CStyleCastNode>(
+ loc, type_id.value(), std::move(rhs), CStyleCastKind::eNone);
+ }
+
+ // Failed to parse the contents of the parentheses as a type declaration.
+ // Rollback the lexer and try parsing it as unary_expression.
+ TentativeParsingRollback(save_token_idx);
+ }
+
+ return ParseUnaryExpression();
+}
// Parse an unary_expression.
//
// unary_expression:
// postfix_expression
-// unary_operator expression
+// unary_operator cast_expression
//
// unary_operator:
// "&"
@@ -99,7 +148,7 @@ ASTNodeUP DILParser::ParseUnaryExpression() {
Token token = CurToken();
uint32_t loc = token.GetLocation();
m_dil_lexer.Advance();
- auto rhs = ParseExpression();
+ auto rhs = ParseCastExpression();
switch (token.GetKind()) {
case Token::star:
return std::make_unique<UnaryOpNode>(loc, UnaryOpKind::Deref,
@@ -274,6 +323,81 @@ std::string DILParser::ParseNestedNameSpecifier() {
}
}
+// Parse a type_id.
+//
+// type_id:
+// type_specifier_seq [abstract_declarator]
+//
+// type_specifier_seq:
+// type_specifier [type_specifier]
+//
+// type_specifier:
+// ["::"] [nested_name_specifier] type_name // not handled for now!
+// builtin_typename
+//
+std::optional<CompilerType> DILParser::ParseTypeId() {
+ CompilerType type;
+ // For now only allow builtin types -- will expand add to this later.
+ auto maybe_builtin_type = ParseBuiltinType();
+ if (maybe_builtin_type) {
+ type = *maybe_builtin_type;
+ } else
+ return {};
+
+ //
+ // abstract_declarator:
+ // ptr_operator [abstract_declarator]
+ //
+ std::vector<Token> ptr_operators;
+ while (CurToken().IsOneOf({Token::star, Token::amp})) {
+ Token tok = CurToken();
+ ptr_operators.push_back(std::move(tok));
+ m_dil_lexer.Advance();
+ }
+ type = ResolveTypeDeclarators(type, ptr_operators);
+
+ return type;
+}
+
+// Parse a built-in type
+//
+// builtin_typename:
+// identifer_seq
+//
+// identifier_seq
+// identifer [identifier_seq]
+//
+// A built-in type can be a single identifier or a space-separated
+// list of identifiers (e.g. "short" or "long long").
+std::optional<CompilerType> DILParser::ParseBuiltinType() {
+ std::string type_name = "";
+ uint32_t save_token_idx = m_dil_lexer.GetCurrentTokenIdx();
+ bool first_word = true;
+ while (CurToken().GetKind() == Token::identifier) {
+ if (CurToken().GetSpelling() == "const" ||
+ CurToken().GetSpelling() == "volatile")
+ continue;
+ if (!first_word)
+ type_name.push_back(' ');
+ else
+ first_word = false;
+ type_name.append(CurToken().GetSpelling());
+ m_dil_lexer.Advance();
+ }
+
+ if (type_name.size() > 0) {
+ lldb::TargetSP target_sp = m_ctx_scope->CalculateTarget();
+ ConstString const_type_name(type_name.c_str());
+ for (auto type_system_sp : target_sp->GetScratchTypeSystems())
+ if (auto compiler_type =
+ type_system_sp->GetBuiltinTypeByName(const_type_name))
+ return compiler_type;
+ }
+
+ TentativeParsingRollback(save_token_idx);
+ return {};
+}
+
// Parse an id_expression.
//
// id_expression:
@@ -339,6 +463,40 @@ std::string DILParser::ParseUnqualifiedId() {
return identifier;
}
+CompilerType
+DILParser::ResolveTypeDeclarators(CompilerType type,
+ const std::vector<Token> &ptr_operators) {
+ CompilerType bad_type;
+ // Resolve pointers/references.
+ for (Token tk : ptr_operators) {
+ uint32_t loc = tk.GetLocation();
+ if (tk.GetKind() == Token::star) {
+ // Pointers to reference types are forbidden.
+ if (type.IsReferenceType()) {
+ BailOut(llvm::formatv("'type name' declared as a pointer to a "
+ "reference of type {0}",
+ type.TypeDescription()),
+ loc, CurToken().GetSpelling().length());
+ return bad_type;
+ }
+ // Get pointer type for the base type: e.g. int* -> int**.
+ type = type.GetPointerType();
+
+ } else if (tk.GetKind() == Token::amp) {
+ // References to references are forbidden.
+ if (type.IsReferenceType()) {
+ BailOut("type name declared as a reference to a reference", loc,
+ CurToken().GetSpelling().length());
+ return bad_type;
+ }
+ // Get reference type for the base type: e.g. int -> int&.
+ type = type.GetLValueReferenceType();
+ }
+ }
+
+ return type;
+}
+
// Parse an boolean_literal.
//
// boolean_literal:
|
| eEnumeration, | ||
| eNullptr, | ||
| eReference, | ||
| eNone, |
There was a problem hiding this comment.
Can we add a brief oxygen comment to each of these enumerators?
| Deref, // "*" | ||
| }; | ||
|
|
||
| /// The C-Style casts allowed by DIL. |
There was a problem hiding this comment.
Apologies if I'm late to the discussion, but should we be calling it C-style? I know it's inspired by the C-style casts, but Is there going to be some other kind of casting? Maybe just calling it a generic Cast is better?
There was a problem hiding this comment.
We might implement at least static_cast and reinterpret_cast later. Since we are trying to emulate the C-style behavior here, I think it's better to leave it as is to avoid confusion.
There was a problem hiding this comment.
Actually, no I think we reached an agreement not to add those other types of casts to DIL, because they were too language-specific. These comments referring explicitly to C-Style casts are from the implementation that allowed for the other C++ styles of casting, but since we're not planning on adding that to DIL, I will remove the "C Style" references.
|
|
||
| ASTNodeUP DILParser::ParseCastExpression() { | ||
| // This can be a C-style cast, try parsing the contents as a type declaration. | ||
| if (CurToken().Is(Token::l_paren)) { |
There was a problem hiding this comment.
Can we do use early-return style here?
| if (CurToken().Is(Token::l_paren)) { | |
| if (!CurToken().Is(Token::l_paren)) | |
| return ParseUnaryExpression(); | |
| ... |
| // Pointers to reference types are forbidden. | ||
| if (type.IsReferenceType()) { | ||
| BailOut(llvm::formatv("'type name' declared as a pointer to a " | ||
| "reference of type {0}", | ||
| type.TypeDescription()), | ||
| loc, CurToken().GetSpelling().length()); | ||
| return bad_type; | ||
| } |
There was a problem hiding this comment.
Is this too C++ specific? Maybe this should live in TypeSystem?
There was a problem hiding this comment.
Makes me think, should we support references at all for now? Might be wrong, but I'd think casting to a reference type is not a super common use-case. Once we add & as a ptr_operator, we're saying & is the reference syntax for all languages.
Again, if this has already been discussed elsewhere, feel free to ignore
There was a problem hiding this comment.
I guess we could drop cast to &, we don't have an assignment operator yet anyway. The check should stay regardless though, because if DIL cannot handle something, it must return an error rather then an incorrect value.
But in general, this is a C-style cast with C-style syntax, I think it should be expected to be used only with C/C++ code, since DIL will be mostly used automatically without the programmer knowing that DIL will try to evaluate the expression before the compiler.
There was a problem hiding this comment.
Is it hurting anything to leave this in for now?
There was a problem hiding this comment.
But in general, this is a C-style cast with C-style syntax, I think it should be expected to be used only with C/C++ code
But isn't DIL supposed to be the de-facto inspection language? Regardless of what language you're debugging. Yes it follows mostly C/C++ syntax but here you are checking explicitly C++ language semantics, not just syntax.
Maybe I'm incorrectly delineating DIL and the language plugins.
E.g., an alternative would be to say:
if (tk.GetKind() == Token::star)
type = type.GetPointerType();
if (!type) {
BailOut("couldn't get a pointer type to ....");
return {};
}
And the language plugin decide whether it can create a pointer to a reference type.
Wdyt?
There was a problem hiding this comment.
It is the intention of the DIL to be the general introspection language. We should try to make it useable to people who aren't just debugging C but the overall idea is that most people are familiar with C so making the syntax look C-ish is a good way to achieve that goal. Remember that this is not the expression parser, whose job it is to capture all the subtleties of any given language. This is primarily a way to examine data values.
There was a problem hiding this comment.
My concern was that if we have something like:
(lldb) frame var (Foo &*)foo
Why is the DIL parser deciding that this is an invalid cast for all languages? Shouldn't that be the TypeSystem's job? In my mind the DIL parser just consumes tokens and dispatches to the relevant type system to conjure up the types that it thinks it needs to create. And if the TypeSystem doesn't allow making a pointer type to a reference type, then so be it.
Am I being overly pedantic here? I don't mind keeping this here, just wanted to make sure I understand the interaction between TypeSystem and DIL.
There was a problem hiding this comment.
Since the DIL is interpreting: (Typename <operator>) Identifier to mean "re-interpret Identifier as an <operator> variant of "Typename" regardless of how the underlying language would actually spell that operation, similarly * and & can't just mean "pass a * to the language and see what it does with it" since the underlying language may very well not spell it that way.
So if the DIL can convince itself some combination of primitive operations in a cast doesn't make sense in the abstract, it should be free to reject it. But if it just isn't allowed when spelled that way in the languages we know, then it should pass that off to the type system.
There was a problem hiding this comment.
But in general, this is a C-style cast with C-style syntax, I think it should be expected to be used only with C/C++ code, since DIL will be mostly used automatically without the programmer knowing that DIL will try to evaluate the expression before the compiler.
But isn't DIL supposed to be the de-facto inspection language? Regardless of what language you're debugging. Yes it follows mostly C/C++ syntax but here you are checking explicitly C++ language semantics, not just syntax.
It is the intention of the DIL to be the general introspection language. We should try to make it useable to people who aren't just debugging C but the overall idea is that most people are familiar with C so making the syntax look C-ish is a good way to achieve that goal. Remember that this is not the expression parser, whose job it is to capture all the subtleties of any given language. This is primarily a way to examine data values.
@Michael137 @jimingham
What I meant to say is that considering how we're integrating DIL into LLDB, most people won't even know that DIL exists. We just replaced the implementation of frame var with DIL and made people use it without realizing it. And this implementation is used as a first attempt in evaluating expressions when using an IDE via lldb-dap (for example), so people will be just writing expressions in the language that they're debugging.
So, on the one hand, what's the point going out of the way supporting C-style cast on languages with different reference symbol, or no explicit references at all (like Swift, if I understand correctly)? For someone to do this, they would need to know about DIL and to know that it is allowed to replace a reference symbol & with something else specifically in a C-style cast. On the other hand, the debugged code could be a mixture of C++ and Swift objects, so maybe this could be useful to have anyway. I'm really not sure, I thought we could eventually just support different syntax constructions from different languages which underneath could do the same thing, or at least very similar ones. For example, having both a C-style cast and a keyword as for Swift and Rust, which is why I also suggested to keep the name "CStyleCastNode" in the code. This hasn't really come up in any meetings about DIL afaik, so maybe we need to discuss this at some point.
But for now it's probably a good idea anyway to check pointer/reference operators during evaluation, although disallowing reference to pointer and reference to reference is exclusive to this cast. Normally, we can do that, so existing type system functions like GetPointerType won't work here.
There was a problem hiding this comment.
The DIL implements the inspection of values in memory. The operations it supports aren't intended to be "language accurate", for instance we aren't calling overloaded operators, etc. And because of the ways you can name the children provided by synthetic child providers, it may even not describe the objects as they might appear in the source language.
So for that reason, I don't want to give the impression that this IS the expression parser. That's one reason why I think it isn't terribly important that we support each (or even the current) language's way to express cast, dereference, etc. That's just doing work to make an impression that isn't correct.
lldb/source/ValueObject/DILEval.cpp
Outdated
| // Don't actually do the cast for now -- that code will be added later. | ||
| // For now just return the original operand, unchanged. | ||
| return operand; |
There was a problem hiding this comment.
Do we aim to merge this PR separately? Is so, this should return an unimplemented error (or something like that) for now, so that the calling code can fall back to full expression evaluator.
There was a problem hiding this comment.
Yes we plan to merge it separately. I don't think we need to advertise that it doesn't work properly yet (since we're not advertising that this feature is going in at all). So I'm ok with 'type casting' silently doing nothing for now. But if others disagree I could return an error here for now...
Other opinions?
There was a problem hiding this comment.
It's just since frame var implementation is used for expression evaluation in lldb-dap, if a user writes an expression with a cast, but DIL doesn't do it and returns some value, that will be a wrong value the user didn't expect. If DIL returns an error, the calling code will instead use a full expression evaluator. For now we can just return the same parser error as before this patch, to avoid confusing people who might read the error message,
There was a problem hiding this comment.
I'm worried about this terminology. frame var is not and should not be expression evaluation. It's for static inspection of memory, it doesn't do operator overloads in general, and doesn't construct temporary objects or call copy constructors or anything like that. It just looks at static values in memory. We should not give the impression that frame var is the expression evaluator.
This project started because some folks wanted to make it easier to write data formatters (which should be non-code-running for efficiency & stability) using "value path expressions" that required some operations we didn't support in the frame var language. In that use case, presumably the authors know what they are doing and won't confuse DIL with the real language specific expression evaluator.
Even dwim-print requires context in this regard. After all, an -> access of a value that has an -> overload is going to be different in dwim-print. That command works for common cases but really does require the user to know what they are doing, since there will be cases where both frame var and expr return valid but different results.
So it seems to me we should be careful of anything we say or do that confuses the two.
There was a problem hiding this comment.
Even
dwim-printrequires context in this regard. After all, an -> access of a value that has an -> overload is going to be different in dwim-print. That command works for common cases but really does require the user to know what they are doing, since there will be cases where bothframe varandexprreturn valid but different results.
So it seems to me we should be careful of anything we say or do that confuses the two.
I haven't really thought about that. We already replaced old frame var with DIL, which is fine for now since it has basically the same functionality, but will become more of a problem as we add more operators. Compared to dwim-print, lldb-dap doesn't do any checks and just tries to use frame var for expressions, which means this was an existing problem when a -> appeared. Even with the old implementation, the overloaded operator wouldn't be used.
I guess we'll have to think again if this was a correct approach to replace frame var, or if we should just add the same guards dwim-print does to lldb-dap. But I personally really hoped this would just speed up expression evaluation for everyone.
Also, in DIL we could potentially implement overloaded operators, at least with ABI calls, I don't know how limited or accurate this would be though.
There was a problem hiding this comment.
You can implement an overloaded -> operator using a synthetic child provider already if you want to.
But I actually think it's a good thing not a problem to have a "values in memory" view of values and a "how the programming language would present them" view of values. After all, if you're -> operator is misbehaving, or is not letting you look at the value because it doesn't pass some check, you still need some way to just see the dereference in memory.
So long as we are clear about: "frame var shows values in memory" and "expr accesses values as the programming language would" I think this is a useful and understandable split. We also have dwim-print for people that don't want to be bothered with this distinction and happy to let lldb choose the most useful representation for them.
There was a problem hiding this comment.
Sounds reasonable. But I would like to have an option to use DIL for expressions anyway, maybe we can just add another LLDB setting.
There was a problem hiding this comment.
Returning (no pun intended) to the original question here: Is it OK, for now, for this function to return the unchanged operand? Especially because the PR to do the actual type casting will be sent for review as soon as this one lands?
There was a problem hiding this comment.
If offline conversation kuilpd convinced me that this should return an error for now, since it's not actually doing the typecast. Have updated the code to do that.
- change references to C-style casting to just casting - add comments to cast enumeration types - use early-return
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
|
||
| } else if (tk.GetKind() == Token::amp) { | ||
| // References to references are forbidden. | ||
| if (type.IsReferenceType()) { |
There was a problem hiding this comment.
Should we add a FIXME for rvalue references (i.e., &&)?
| CompilerType | ||
| DILParser::ResolveTypeDeclarators(CompilerType type, | ||
| const std::vector<Token> &ptr_operators) { | ||
| CompilerType bad_type; |
There was a problem hiding this comment.
Lets get rid of this and just return {} in the failure branches.
|
|
||
| /// The type casts allowed by DIL. | ||
| enum class CastKind { | ||
| eEnumeration, /// Casting from a scalar to an enumeration type |
There was a problem hiding this comment.
Doxygen syntax is either
/// Casting from a scalar to an enumeration type.
eEnumeration,
or
eEnumeration, ///< Casting from a scalar to an enumeration type.
There was a problem hiding this comment.
Oh! I didn't know that. Thanks! (Fixed).
🐧 Linux x64 Test Results
|
Will replace this with code to do actual casts in next PR.
6 participants
This is an alternative to #159500, breaking that PR down into three separate PRs, to make it easier to review.
This first PR of the three adds the basic framework for doing type casing to the DIL code, but it does not actually do any casting: In this PR the DIL parser only recognizes builtin type names, and the DIL interpreter does not do anything except return the original operand (no casting). The second and third PRs will add most of the type parsing, and do the actual type casting, respectively.