[LLDB] Add type casting to DIL. #159500
[LLDB] Add type casting to DIL. #159500
Conversation
This adds basic c-style type casting to DIL. It handles basic
built-in types: bool, char, short, int, long, double, float, and void.
It also handles user-defined types (e.g. class names, typedefs, etc.).
It does NOT handle C++ templates at the moment. Not sure if this is
something we would want to add to DIL later or not. DIL will not
be supporting C++-specific-style casts (static_cast<>, reinterpret_cast<>,
or dynamic_cast<>).
This PR adds quite a bit of type-parsing, which must happen in the
parser, to resolve ambiguity ("is this thing I'm looking at a type cast
or just an expression in parentheses?").
I'd be happy to break this up into smaller PRs if someone could suggest
a reasonable way to break it up.
|
@llvm/pr-subscribers-lldb Author: None (cmtice) ChangesThis adds basic c-style type casting to DIL. It handles basic built-in types: bool, char, short, int, long, double, float, and void. It also handles user-defined types (e.g. class names, typedefs, etc.). It does NOT handle C++ templates at the moment. Not sure if this is something we would want to add to DIL later or not. DIL will not be supporting C++-specific-style casts (static_cast<>, reinterpret_cast<>, or dynamic_cast<>). This PR adds quite a bit of type-parsing, which must happen in the parser, to resolve ambiguity ("is this thing I'm looking at a type cast or just an expression in parentheses?"). I'd be happy to break this up into smaller PRs if someone could suggest a reasonable way to break it up. Patch is 61.69 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/159500.diff 11 Files Affected:
diff --git a/lldb/docs/dil-expr-lang.ebnf b/lldb/docs/dil-expr-lang.ebnf
index 67328939ba420..4a66734e75fec 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 = "*" | "&" ;
@@ -41,6 +44,31 @@ 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;
+ | "char"
+ | "bool"
+ | "short"
+ | "int"
+ | "long"
+ | "signed"
+ | "unsigned"
+ | "float"
+ | "double"
+ | "void" ;
+
+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;
diff --git a/lldb/include/lldb/ValueObject/DILAST.h b/lldb/include/lldb/ValueObject/DILAST.h
index 1d10755c46e39..bbaad28d55735 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 {
eArraySubscriptNode,
eBitExtractionNode,
+ eCStyleCastNode,
eErrorNode,
eFloatLiteralNode,
eIdentifierNode,
@@ -28,6 +29,21 @@ enum class NodeKind {
eUnaryOpNode,
};
+/// The C-Style casts allowed by DIL.
+enum class CStyleCastKind {
+ eEnumeration,
+ eNullptr,
+ eReference,
+ eNone,
+};
+
+/// Promotions for C-Style casts in DIL.
+enum class CastPromoKind {
+ eArithmetic,
+ ePointer,
+ eNone,
+};
+
/// The Unary operators recognized by DIL.
enum class UnaryOpKind {
AddrOf, // "&"
@@ -226,6 +242,29 @@ class FloatLiteralNode : public ASTNode {
llvm::APFloat 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
@@ -247,6 +286,8 @@ class Visitor {
Visit(const IntegerLiteralNode *node) = 0;
virtual llvm::Expected<lldb::ValueObjectSP>
Visit(const FloatLiteralNode *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 5a48c2c989f4d..3d4ef021cbc6f 100644
--- a/lldb/include/lldb/ValueObject/DILEval.h
+++ b/lldb/include/lldb/ValueObject/DILEval.h
@@ -58,12 +58,19 @@ class Interpreter : Visitor {
Visit(const IntegerLiteralNode *node) override;
llvm::Expected<lldb::ValueObjectSP>
Visit(const FloatLiteralNode *node) override;
+ llvm::Expected<lldb::ValueObjectSP>
+ Visit(const CStyleCastNode *node) override;
llvm::Expected<CompilerType>
PickIntegerType(lldb::TypeSystemSP type_system,
std::shared_ptr<ExecutionContextScope> ctx,
const IntegerLiteralNode *literal);
+ llvm::Expected<CompilerType>
+ VerifyCStyleCastType(lldb::ValueObjectSP &operand, CompilerType &op_type,
+ CompilerType target_type, CastPromoKind &promo_kind,
+ CStyleCastKind &cast_kind, int location);
+
// 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/DILParser.h b/lldb/include/lldb/ValueObject/DILParser.h
index 90df109337dcf..081711d26439d 100644
--- a/lldb/include/lldb/ValueObject/DILParser.h
+++ b/lldb/include/lldb/ValueObject/DILParser.h
@@ -10,6 +10,7 @@
#define LLDB_VALUEOBJECT_DILPARSER_H
#include "lldb/Target/ExecutionContextScope.h"
+#include "lldb/Target/StackFrame.h"
#include "lldb/Utility/DiagnosticsRendering.h"
#include "lldb/Utility/Status.h"
#include "lldb/ValueObject/DILAST.h"
@@ -31,6 +32,9 @@ enum class ErrorCode : unsigned char {
kUnknown,
};
+llvm::Expected<lldb::TypeSystemSP>
+DILGetTypeSystemFromCU(std::shared_ptr<StackFrame> ctx);
+
// The following is modeled on class OptionParseError.
class DILDiagnosticError
: public llvm::ErrorInfo<DILDiagnosticError, DiagnosticError> {
@@ -55,6 +59,61 @@ class DILDiagnosticError
std::string message() const override { return m_detail.rendered; }
};
+/// TypeDeclaration builds information about the literal type definition as
+/// type is being parsed. It doesn't perform semantic analysis for non-basic
+/// types -- e.g. "char&&&" is a valid type declaration.
+/// NOTE: CV qualifiers are ignored.
+class TypeDeclaration {
+public:
+ enum class TypeSpecifier {
+ kBool,
+ kChar,
+ kDouble,
+ kFloat,
+ kInt,
+ kLong,
+ kLongDouble,
+ kLongLong,
+ kShort,
+ kUnknown,
+ kVoid,
+ };
+
+ enum class SignSpecifier {
+ kUnknown,
+ kSigned,
+ kUnsigned,
+ };
+
+ bool IsEmpty() const { return !m_is_builtin && !m_is_user_type; }
+
+ lldb::BasicType GetBasicType() const;
+
+public:
+ // Indicates user-defined typename (e.g. "MyClass", "MyTmpl<int>").
+ std::string m_user_typename;
+
+ // Basic type specifier ("void", "char", "intr", "float", "long long", etc.).
+ TypeSpecifier m_type_specifier = TypeSpecifier::kUnknown;
+
+ // Signedness specifier ("signed", "unsigned").
+ SignSpecifier m_sign_specifier = SignSpecifier::kUnknown;
+
+ // Does the type declaration includes "int" specifier?
+ // This is different than `type_specifier_` and is used to detect "int"
+ // duplication for types that can be combined with "int" specifier (e.g.
+ // "short int", "long int").
+ bool m_has_int_specifier = false;
+
+ // Indicates whether there was an error during parsing.
+ bool m_has_error = false;
+
+ // Indicates whether this declaration describes a builtin type.
+ bool m_is_builtin = false;
+
+ // Indicates whether this declaration describes a user type.
+ bool m_is_user_type = false;
+}; // class TypeDeclaration
/// Pure recursive descent parser for C++ like expressions.
/// EBNF grammar for the parser is described in lldb/docs/dil-expr-lang.ebnf
@@ -100,10 +159,22 @@ class DILParser {
ASTNodeUP ParseIntegerLiteral();
ASTNodeUP ParseFloatingPointLiteral();
+ ASTNodeUP ParseCastExpression();
+ std::optional<CompilerType> ParseTypeId(bool must_be_type_id = false);
+ void ParseTypeSpecifierSeq(TypeDeclaration *type_decl);
+ bool ParseTypeSpecifier(TypeDeclaration *type_decl);
+ std::string ParseTypeName();
+ CompilerType ResolveTypeDeclarators(CompilerType type,
+ const std::vector<Token> &ptr_operators);
+ bool IsSimpleTypeSpecifierKeyword(Token token) const;
+ bool HandleSimpleTypeSpecifier(TypeDeclaration *type_decl);
+
void BailOut(const std::string &error, uint32_t loc, uint16_t err_len);
void Expect(Token::Kind kind);
+ void ExpectOneOf(std::vector<Token::Kind> kinds_vec);
+
void TentativeParsingRollback(uint32_t saved_idx) {
if (m_error)
llvm::consumeError(std::move(m_error));
@@ -132,4 +203,66 @@ class DILParser {
} // namespace lldb_private::dil
+namespace llvm {
+template <>
+struct format_provider<lldb_private::dil::TypeDeclaration::TypeSpecifier> {
+ static void format(const lldb_private::dil::TypeDeclaration::TypeSpecifier &t,
+ raw_ostream &OS, llvm::StringRef Options) {
+ switch (t) {
+ case lldb_private::dil::TypeDeclaration::TypeSpecifier::kVoid:
+ OS << "void";
+ break;
+ case lldb_private::dil::TypeDeclaration::TypeSpecifier::kBool:
+ OS << "bool";
+ break;
+ case lldb_private::dil::TypeDeclaration::TypeSpecifier::kChar:
+ OS << "char";
+ break;
+ case lldb_private::dil::TypeDeclaration::TypeSpecifier::kInt:
+ OS << "int";
+ break;
+ case lldb_private::dil::TypeDeclaration::TypeSpecifier::kFloat:
+ OS << "float";
+ break;
+ case lldb_private::dil::TypeDeclaration::TypeSpecifier::kShort:
+ OS << "short";
+ break;
+ case lldb_private::dil::TypeDeclaration::TypeSpecifier::kLong:
+ OS << "long";
+ break;
+ case lldb_private::dil::TypeDeclaration::TypeSpecifier::kLongLong:
+ OS << "long long";
+ break;
+ case lldb_private::dil::TypeDeclaration::TypeSpecifier::kDouble:
+ OS << "double";
+ break;
+ case lldb_private::dil::TypeDeclaration::TypeSpecifier::kLongDouble:
+ OS << "long double";
+ break;
+ default:
+ OS << "invalid type specifier";
+ break;
+ }
+ }
+};
+
+template <>
+struct format_provider<lldb_private::dil::TypeDeclaration::SignSpecifier> {
+ static void format(const lldb_private::dil::TypeDeclaration::SignSpecifier &t,
+ raw_ostream &OS, llvm::StringRef Options) {
+ switch (t) {
+ case lldb_private::dil::TypeDeclaration::SignSpecifier::kSigned:
+ OS << "signed";
+ break;
+ case lldb_private::dil::TypeDeclaration::SignSpecifier::kUnsigned:
+ OS << "unsigned";
+ break;
+ default:
+ OS << "invalid sign specifier";
+ break;
+ }
+ }
+};
+} // namespace llvm
+
#endif // LLDB_VALUEOBJECT_DILPARSER_H
diff --git a/lldb/source/ValueObject/DILAST.cpp b/lldb/source/ValueObject/DILAST.cpp
index 70564663a62cd..f8afd9d7bd50a 100644
--- a/lldb/source/ValueObject/DILAST.cpp
+++ b/lldb/source/ValueObject/DILAST.cpp
@@ -46,4 +46,8 @@ llvm::Expected<lldb::ValueObjectSP> FloatLiteralNode::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 c6cf41ee9e9ee..05bcc25d2b9f2 100644
--- a/lldb/source/ValueObject/DILEval.cpp
+++ b/lldb/source/ValueObject/DILEval.cpp
@@ -13,6 +13,7 @@
#include "lldb/Symbol/VariableList.h"
#include "lldb/Target/RegisterContext.h"
#include "lldb/ValueObject/DILAST.h"
+#include "lldb/ValueObject/DILParser.h"
#include "lldb/ValueObject/ValueObject.h"
#include "lldb/ValueObject/ValueObjectRegister.h"
#include "lldb/ValueObject/ValueObjectVariable.h"
@@ -21,6 +22,42 @@
namespace lldb_private::dil {
+lldb::ValueObjectSP
+GetDynamicOrSyntheticValue(lldb::ValueObjectSP in_valobj_sp,
+ lldb::DynamicValueType use_dynamic,
+ bool use_synthetic) {
+ Status error;
+ if (!in_valobj_sp) {
+ error = Status("invalid value object");
+ return in_valobj_sp;
+ }
+ lldb::ValueObjectSP value_sp = in_valobj_sp;
+ Target *target = value_sp->GetTargetSP().get();
+ // If this ValueObject holds an error, then it is valuable for that.
+ if (value_sp->GetError().Fail())
+ return value_sp;
+
+ if (!target)
+ return lldb::ValueObjectSP();
+
+ if (use_dynamic != lldb::eNoDynamicValues) {
+ lldb::ValueObjectSP dynamic_sp = value_sp->GetDynamicValue(use_dynamic);
+ if (dynamic_sp)
+ value_sp = dynamic_sp;
+ }
+
+ if (use_synthetic) {
+ lldb::ValueObjectSP synthetic_sp = value_sp->GetSyntheticValue();
+ if (synthetic_sp)
+ value_sp = synthetic_sp;
+ }
+
+ if (!value_sp)
+ error = Status("invalid value object");
+
+ return value_sp;
+}
+
static lldb::VariableSP DILFindVariable(ConstString name,
VariableList &variable_list) {
lldb::VariableSP exact_match;
@@ -499,16 +536,6 @@ Interpreter::Visit(const BitFieldExtractionNode *node) {
return child_valobj_sp;
}
-static llvm::Expected<lldb::TypeSystemSP>
-GetTypeSystemFromCU(std::shared_ptr<StackFrame> ctx) {
- SymbolContext symbol_context =
- ctx->GetSymbolContext(lldb::eSymbolContextCompUnit);
- lldb::LanguageType language = symbol_context.comp_unit->GetLanguage();
-
- symbol_context = ctx->GetSymbolContext(lldb::eSymbolContextModule);
- return symbol_context.module_sp->GetTypeSystemForLanguage(language);
-}
-
static CompilerType GetBasicType(lldb::TypeSystemSP type_system,
lldb::BasicType basic_type) {
if (type_system)
@@ -559,7 +586,7 @@ Interpreter::PickIntegerType(lldb::TypeSystemSP type_system,
llvm::Expected<lldb::ValueObjectSP>
Interpreter::Visit(const IntegerLiteralNode *node) {
llvm::Expected<lldb::TypeSystemSP> type_system =
- GetTypeSystemFromCU(m_exe_ctx_scope);
+ DILGetTypeSystemFromCU(m_exe_ctx_scope);
if (!type_system)
return type_system.takeError();
@@ -583,7 +610,7 @@ Interpreter::Visit(const IntegerLiteralNode *node) {
llvm::Expected<lldb::ValueObjectSP>
Interpreter::Visit(const FloatLiteralNode *node) {
llvm::Expected<lldb::TypeSystemSP> type_system =
- GetTypeSystemFromCU(m_exe_ctx_scope);
+ DILGetTypeSystemFromCU(m_exe_ctx_scope);
if (!type_system)
return type_system.takeError();
@@ -602,4 +629,238 @@ Interpreter::Visit(const FloatLiteralNode *node) {
"result");
}
+llvm::Expected<CompilerType> Interpreter::VerifyCStyleCastType(
+ lldb::ValueObjectSP &operand, CompilerType &op_type,
+ CompilerType target_type, CastPromoKind &promo_kind,
+ CStyleCastKind &cast_kind, int location) {
+
+ promo_kind = CastPromoKind::eNone;
+ if (op_type.IsReferenceType())
+ op_type = op_type.GetNonReferenceType();
+ if (target_type.IsScalarType()) {
+ if (op_type.IsArrayType()) {
+ // Do array-to-pointer conversion.
+ CompilerType deref_type =
+ op_type.IsReferenceType() ? op_type.GetNonReferenceType() : op_type;
+ CompilerType result_type =
+ deref_type.GetArrayElementType(nullptr).GetPointerType();
+ uint64_t addr = operand->GetLoadAddress();
+ llvm::StringRef name = operand->GetName().GetStringRef();
+ operand = ValueObject::CreateValueObjectFromAddress(
+ name, addr, m_exe_ctx_scope, result_type, /*do_deref=*/false);
+ op_type = result_type;
+ }
+
+ if (op_type.IsPointerType() || op_type.IsNullPtrType()) {
+ // C-style cast from pointer to float/double is not allowed.
+ if (target_type.IsFloat()) {
+ std::string errMsg = llvm::formatv(
+ "C-style cast from {0} to {1} is not allowed",
+ op_type.TypeDescription(), target_type.TypeDescription());
+ return llvm::make_error<DILDiagnosticError>(
+ m_expr, std::move(errMsg), location,
+ op_type.TypeDescription().length());
+ }
+ // Casting pointer to bool is valid. Otherwise check if the result type
+ // is at least as big as the pointer size.
+ uint64_t type_byte_size = 0;
+ uint64_t rhs_type_byte_size = 0;
+ if (auto temp = target_type.GetByteSize(m_exe_ctx_scope.get()))
+ // type_byte_size = temp.value();
+ type_byte_size = *temp;
+ if (auto temp = op_type.GetByteSize(m_exe_ctx_scope.get()))
+ // rhs_type_byte_size = temp.value();
+ rhs_type_byte_size = *temp;
+ if (!target_type.IsBoolean() && type_byte_size < rhs_type_byte_size) {
+ std::string errMsg = llvm::formatv(
+ "cast from pointer to smaller type {0} loses information",
+ target_type.TypeDescription());
+ return llvm::make_error<DILDiagnosticError>(
+ m_expr, std::move(errMsg), location,
+ op_type.TypeDescription().length());
+ }
+ } else if (!op_type.IsScalarType() && !op_type.IsEnumerationType()) {
+ // Otherwise accept only arithmetic types and enums.
+ std::string errMsg = llvm::formatv(
+ "cannot convert {0} to {1} without a conversion operator",
+ op_type.TypeDescription(), target_type.TypeDescription());
+
+ return llvm::make_error<DILDiagnosticError>(
+ m_expr, std::move(errMsg), location,
+ op_type.TypeDescription().length());
+ }
+ promo_kind = CastPromoKind::eArithmetic;
+ } else if (target_type.IsEnumerationType()) {
+ // Cast to enum type.
+ if (!op_type.IsScalarType() && !op_type.IsEnumerationType()) {
+ std::string errMsg = llvm::formatv(
+ "C-style cast from {0} to {1} is not allowed",
+ op_type.TypeDescription(), target_type.TypeDescription());
+
+ return llvm::make_error<DILDiagnosticError>(
+ m_expr, std::move(errMsg), location,
+ op_type.TypeDescription().length());
+ }
+ cast_kind = CStyleCastKind::eEnumeration;
+
+ } else if (target_type.IsPointerType()) {
+ if (!op_type.IsInteger() && !op_type.IsEnumerationType() &&
+ !op_type.IsArrayType() && !op_type.IsPointerType() &&
+ !op_type.IsNullPtrType()) {
+ std::string errMsg = llvm::formatv(
+ "cannot cast from type {0} to pointer type {1}",
+ op_type.TypeDescription(), target_type.TypeDescription());
+
+ return llvm::make_error<DILDiagnosticError>(
+ m_expr, std::move(errMsg), location,
+ op_type.TypeDescription().length());
+ }
+ promo_kind = CastPromoKind::ePointer;
+
+ } else if (target_type.IsNullPtrType()) {
+ // Cast to nullptr type.
+ bool is_signed;
+ if (!target_type.IsNullPtrType() &&
+ (!operand->IsIntegerType(is_signed) ||
+ (is_signed && operand->GetValueAsSigned(0) != 0) ||
+ (!is_signed && operand->GetValueAsUnsigned(0) != 0))) {
+ std::string errMsg = llvm::formatv(
+ "C-style cast from {0} to {1} is not allowed",
+ op_type.TypeDescription(), target_type.TypeDescription());
+
+ return llvm::make_error<DILDiagnosticError>(
+ m_expr, std::move(errMsg), location,
+ op_type.TypeDescription().length());
+ }
+ cast_kind = CStyleCastKind::eNullptr;
+
+ } else if (target_type.IsReferenceType()) {
+ // Cast to a reference type.
+ cast_kind = CStyleCastKind::eReference;
+ } else {
+ // Unsupported cast.
+ std::string errMsg =
+ llvm::formatv("casting of {0} to {1} is not implemented yet",
+ op_type.TypeDescription(), target_type.TypeDescription());
+ return llvm::make_error<DILDiagnosticError>(
+ m_expr, std::move(errMsg), location,
+ op_type.TypeDescription().length());
+ }
+
+ return target_type;
+}
+
+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;
+ CompilerType op_type = operand->GetCompilerType();
+ CStyleCastKind cast_kind = CStyleCastKind::eNone;
+ CastPromoKind promo_kind = CastPromoKind::eNone;
+
+ auto type_or_err =
+ VerifyCStyleCastType(operand, op_type, node->GetType(), promo_kind,
+ cast_kind, node->GetLocation());
+ if (!type_or_err)
+ return type_or_err.takeError();
+
+ CompilerType target_type = *type_or_err;
+ if (op_type.IsReferenceType()) {
+ Status error;
+ operand = operand->Dereference(error);
+ if (error.Fail())
+ return llvm::make_error<D...
[truncated]
|
|
✅ With the latest revision this PR passed the Python code formatter. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
I think we need to get rid of everything that references C++ keywords directly. I don't think we need to parse the builtin type manually, so we can just remove IsSimpleTypeSpecifierKeyword, HandleSimpleTypeSpecifier, and TypeDeclaration class altogether. Instead, parse a sequence of identifiers as a single string and call TypeSystem::GetBuiltinTypeByName to check if such a builtin type exists, it already has pretty much all variations of builtin types in TypeSystemClang. This way other languages can specify their own builtin types as well.
The algorithm of ParseTypeId would be something like this:
ParseTypeSpecifierSeq, that returns a string that is either a builtin type ("unsigned long long") or a user defined type ("ns::myint")- Call
ResolveTypeByNameon the string, which first attempts aGetBuiltinTypeByNameand immediately returns aCompilerTypeif found, then searches in modules for user types - Handle * and & like before
I did a quick test with this logic, seems to be working.
| if (result_type_list.empty()) { | ||
| for (auto type_system_sp : target_sp->GetScratchTypeSystems()) | ||
| if (auto compiler_type = | ||
| type_system_sp->GetBuiltinTypeByName(const_type_name)) | ||
| result_type_list.push_back(compiler_type); | ||
| } |
There was a problem hiding this comment.
This should be the first step before everything else, and use DILGetTypeSystemFromCU as well
auto type_system = DILGetTypeSystemFromCU(ctx_scope);
if (!type_system)
return {};
CompilerType type = type_system.get()->GetBuiltinTypeByName(ConstString(const_type_name));
There was a problem hiding this comment.
I don't think so. ResovleTypeByName is designed to look the type name up in user-defined types first, and only fall back on builtin type names, if the name isn't a user-defined type. As has been pointed out multiple times before, "int", etc. are not reserved words in Swift, so, for example, Swift developers could easily define their own types that should supersede the builtin type names. If we search the builtin type names first we'll never find the user-defined versions (if they exist).
| for (uint32_t i = 0; i < result_type_list.size(); ++i) { | ||
| CompilerType type = result_type_list[i]; | ||
| llvm::StringRef type_name_ref = type.GetTypeName().GetStringRef(); | ||
| ; |
| ; | ||
|
|
||
| if (type_name_ref == name_ref) | ||
| full_match = type; |
There was a problem hiding this comment.
Should we return the type here immediately and not search further?
| // return BuildCStyleCast(type_id.value(), std::move(rhs), | ||
| // token.GetLocation()); |
There was a problem hiding this comment.
| // return BuildCStyleCast(type_id.value(), std::move(rhs), | |
| // token.GetLocation()); |
| threads = lldbutil.continue_to_source_breakpoint( | ||
| self, process,"Set a breakpoint here", lldb.SBFileSpec("main.cpp") | ||
| ) | ||
| self.assertEqual( | ||
| len(threads), 1, "There should be a thread stopped at our breakpoint" | ||
| ) |
There was a problem hiding this comment.
Why do we need to split the test into 2 breakpoints?
There was a problem hiding this comment.
I suppose we don't; I was consolidating multiple tests from the original lldb-eval work, and this is leftover from that. I'll remove it.
| self.expect("frame variable '(int)arr'", error=True, | ||
| substrs=["cast from pointer to smaller type 'int' loses" | ||
| " information"]) |
There was a problem hiding this comment.
Shouldn't this success on a 32-bit system?
There was a problem hiding this comment.
Whoops! Yes, you're correct. I'll fix that.
| // TestCStyleCastBuiltins | ||
| // TestCStyleCastBasicType | ||
| // TestCStyleCastPointer | ||
| // TestCStyleCastNullptrType | ||
| if (false) { // Set a breakpoint here | ||
| } |
There was a problem hiding this comment.
| // TestCStyleCastBuiltins | |
| // TestCStyleCastBasicType | |
| // TestCStyleCastPointer | |
| // TestCStyleCastNullptrType | |
| if (false) { // Set a breakpoint here | |
| } |
| // TestCStyleCastArray | ||
| // TestCStyleCastReference |
There was a problem hiding this comment.
| // TestCStyleCastArray | |
| // TestCStyleCastReference |
| } | ||
|
|
||
| switch (cast_kind) { | ||
| case CStyleCastKind::eEnumeration: { |
There was a problem hiding this comment.
There are so many type checks between this, VerifyCStyleCastType and CastToBasicType, which all seem to check similar things. I haven't looked too much into these yet, but is there any way we can simplify all this? My only idea would be to unify VerifyCStyleCastType and this switch here, since we don't do checks separately like in lldb-eval anymore, while also removing the repeated checks that are done in CastToBasicType (if any). But I'm not sure if this would look any better or not.
There was a problem hiding this comment.
It looks like there were some redundant type checks; I've eliminated them and consolidated the code as much as I can.
| def expect_var_path(self, expr, compare_to_framevar=False, value=None, type=None): | ||
| value_dil = super().expect_var_path(expr, value=value, type=type) | ||
| if compare_to_framevar: | ||
| self.runCmd("settings set target.experimental.use-DIL false") | ||
| value_frv = super().expect_var_path(expr, value=value, type=type) | ||
| self.runCmd("settings set target.experimental.use-DIL true") | ||
| self.assertEqual(value_dil.GetValue(), value_frv.GetValue()) |
There was a problem hiding this comment.
We don't need this anymore, since these new expressions are not handled by the legacy frame var.
There was a problem hiding this comment.
You're right; I'll remove this.
| llvm::Expected<lldb::TypeSystemSP> | ||
| DILGetTypeSystemFromCU(std::shared_ptr<StackFrame> ctx); |
There was a problem hiding this comment.
I was wondering, do we need the DIL prefix here? It just retrieves the type system from the compile unit, nothing specific to DIL itself.
There was a problem hiding this comment.
Fair enough; I'll remove it.
I'm not sure yet; I haven't looked much at the comments in DILParser.cpp yet (I was planning on trying to go through them today). :-) |
Update ResolveTypeByName to return immediately when full match is found. Remove 'DIL' prefix from GetTypeSystemFromCU.
The biggest problem is in your step 2. We can't make GetBuiltinTypeByName the first thing in ResolveTypeByName. Doing that would prevent users from every being able to redefine types with the same names as the builtins, which is something I've been told is allowed in Swift, e.g. |
|
I have been wondering: If people think this PR is too big to review, I could pull most of the type parsing out into a separate PR and have that reviewed by itself... What do folks think? |
Ah, I see, I didn't realize that this logic might not work for other languages. Mainly I suggested to change the way we parse the basic types, using In any case, Swift, if I understand correctly, doesn't really have builtin types, all the types like Need some Swift insight here :) @jimingham @adrian-prantl Swift does have builtin types, they just aren't exposed to the user directly. For instance, this is a swift Int: I don't remember the exact details, but the Builtin module isn't available in most code, it's restricted to the swift standard library or something like that. For instance, if you try to cheat with type inference and do: then when you run this you get: So I think I wasn't supposed to do that... But you can still sort of manipulate them, for instance this - added to doSomething - works:
|
@jimingham |
Following a reviewer suggestion. This eliminates the need for the TypeSpecifier & TypeDeclaration classes which simplifies things a bit.
|
I've simplified the type parsing a bit, as suggested above (for the builtin types), which eliminated ~400 lines of code (woo hoo!). Verified that this still passes all the type-casting tests. |
| | "unsigned" | ||
| | "float" | ||
| | "double" | ||
| | "void" ; |
There was a problem hiding this comment.
I think I have a problem with hardcoding C type names in the grammar itself. I believe this should be an identifier and it should be up to TypeSystem to recognize these. Is that feasible?
There was a problem hiding this comment.
That is in fact the way the (updated) implementation works: Just expecting identifiers and using calls to LLDB's type system to determine if the name is for a builtin type or not. I just forgot to update the grammar here when I changed the implementation to work that way. I will update the grammar (thanks for catching this).
There was a problem hiding this comment.
See DILParser::ParseBuiltinType, around line 396 of DILParser.cpp
4 participants
This adds basic c-style type casting to DIL. It handles basic built-in types: bool, char, short, int, long, double, float, and void. It also handles user-defined types (e.g. class names, typedefs, etc.). It does NOT handle C++ templates at the moment. Not sure if this is something we would want to add to DIL later or not. DIL will not be supporting C++-specific-style casts (static_cast<>, reinterpret_cast<>, or dynamic_cast<>).
This PR adds quite a bit of type-parsing, which must happen in the parser, to resolve ambiguity ("is this thing I'm looking at a type cast or just an expression in parentheses?").
I'd be happy to break this up into smaller PRs if someone could suggest a reasonable way to break it up.