vsk created this revision.
vsk added reviewers: labath, zturner, davide, aprantl, lhames.
LLDB CompilerTypes may be invalid, i.e they should be checked for
validity before use.
Compared to a more typical model in which only valid types exist [1],
this has a few disadvantages:
- The debugger is guaranteed to operate on invalid types as if they were valid,
leading to unexpected or unpredictable results. Because type validity checks
are not mandatory, they can and will be skipped.
E.g in this patch I chose a random function which returns a type. Its callers
checked for invalid results inconsistently, hiding a bug.
- Operations on types have high complexity, because each operation has fallback
paths to deal with invalid types [2]. Beyond making code hard to read, this
results in redundant double-checking of type validity.
Going forward, we should transition to a model in which CompilerTypes
are either valid or do not exist. This is a first, incremental step
towards that goal. Ultimately we should delete "Type::IsValid()".
[1] See llvm::Type, clang::Type, swift::Type, etc. All of these either
exist and are valid, or do not exist.
[2] See the methods in CompilerType. Each of these branch on the type's
validity.
[3] For an overview of the principles and advantages of llvm::Error, see
Lang Hames's talk: https://www.youtube.com/watch?v=Wq8fNK98WGw.
https://reviews.llvm.org/D43912
Files:
include/lldb/Symbol/CompilerType.h
source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
source/Symbol/CompilerType.cpp
Index: source/Symbol/CompilerType.cpp
===================================================================
--- source/Symbol/CompilerType.cpp
+++ source/Symbol/CompilerType.cpp
@@ -1080,3 +1080,5 @@
return lhs.GetTypeSystem() != rhs.GetTypeSystem() ||
lhs.GetOpaqueQualType() != rhs.GetOpaqueQualType();
}
+
+char lldb_private::InvalidType::ID;
Index: source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
===================================================================
--- source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
+++ source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
@@ -625,10 +625,11 @@
/// The type as it appears in the source context.
///
/// @return
- /// Returns the moved type, or an empty type if there was a problem.
+ /// Returns the moved type, or an error.
//------------------------------------------------------------------
- TypeFromUser DeportType(ClangASTContext &target, ClangASTContext &source,
- TypeFromParser parser_type);
+ llvm::Expected<TypeFromUser> DeportType(ClangASTContext &target,
+ ClangASTContext &source,
+ TypeFromParser parser_type);
ClangASTContext *GetClangASTContext();
};
Index: source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
===================================================================
--- source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
+++ source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
@@ -277,9 +277,10 @@
return ret;
}
-TypeFromUser ClangExpressionDeclMap::DeportType(ClangASTContext &target,
- ClangASTContext &source,
- TypeFromParser parser_type) {
+llvm::Expected<TypeFromUser>
+ClangExpressionDeclMap::DeportType(ClangASTContext &target,
+ ClangASTContext &source,
+ TypeFromParser parser_type) {
assert (&target == m_target->GetScratchClangASTContext());
assert ((TypeSystem*)&source == parser_type.GetTypeSystem());
assert (source.getASTContext() == m_ast_context);
@@ -302,10 +303,10 @@
source_file,
clang::QualType::getFromOpaquePtr(parser_type.GetOpaqueQualType()));
return TypeFromUser(exported_type.getAsOpaquePtr(), &target);
- } else {
- lldbassert(0 && "No mechanism for deporting a type!");
- return TypeFromUser();
}
+
+ lldbassert(0 && "No mechanism for deporting a type!");
+ return llvm::make_error<InvalidType>("Could not deport type from context");
}
bool ClangExpressionDeclMap::AddPersistentVariable(const NamedDecl *decl,
@@ -315,6 +316,7 @@
bool is_lvalue) {
assert(m_parser_vars.get());
+ Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
ClangASTContext *ast =
llvm::dyn_cast_or_null<ClangASTContext>(parser_type.GetTypeSystem());
if (ast == nullptr)
@@ -328,8 +330,18 @@
if (target == nullptr)
return false;
- TypeFromUser user_type =
+ llvm::Expected<TypeFromUser> user_type_or_err =
DeportType(*target->GetScratchClangASTContext(), *ast, parser_type);
+ if (llvm::Error E = user_type_or_err.takeError()) {
+ std::string Reason = llvm::toString(std::move(E));
+ if (log)
+ log->Printf("%s", Reason.c_str());
+ return false;
+ }
+
+ TypeFromUser &user_type = *user_type_or_err;
+ assert(user_type.IsValid() &&
+ "Persistent variable's type wasn't copied successfully");
uint32_t offset = m_parser_vars->m_materializer->AddResultVariable(
user_type, is_lvalue, m_keep_result_in_memory, m_result_delegate, err);
@@ -358,22 +370,26 @@
return true;
}
- Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
ExecutionContext &exe_ctx = m_parser_vars->m_exe_ctx;
Target *target = exe_ctx.GetTargetPtr();
if (target == NULL)
return false;
ClangASTContext *context(target->GetScratchClangASTContext());
- TypeFromUser user_type = DeportType(*context, *ast, parser_type);
-
- if (!user_type.GetOpaqueQualType()) {
+ llvm::Expected<TypeFromUser> user_type_or_err =
+ DeportType(*context, *ast, parser_type);
+ if (llvm::Error E = user_type_or_err.takeError()) {
+ std::string Reason = llvm::toString(std::move(E));
if (log)
- log->Printf("Persistent variable's type wasn't copied successfully");
+ log->Printf("%s", Reason.c_str());
return false;
}
+ TypeFromUser &user_type = *user_type_or_err;
+ assert(user_type.IsValid() &&
+ "Persistent variable's type wasn't copied successfully");
+
if (!m_parser_vars->m_target_info.IsValid())
return false;
Index: include/lldb/Symbol/CompilerType.h
===================================================================
--- include/lldb/Symbol/CompilerType.h
+++ include/lldb/Symbol/CompilerType.h
@@ -21,6 +21,7 @@
#include "lldb/Core/ClangForward.h"
#include "lldb/lldb-private.h"
#include "llvm/ADT/APSInt.h"
+#include "llvm/Support/Error.h"
namespace lldb_private {
@@ -441,6 +442,23 @@
CompilerType type;
};
+/// A force-checked error used to describe type construction failures.
+class InvalidType : public llvm::ErrorInfo<InvalidType> {
+public:
+ static char ID;
+ std::string Reason;
+
+ InvalidType(llvm::StringRef Reason) : Reason(Reason) {}
+
+ void log(llvm::raw_ostream &OS) const override {
+ OS << "Invalid type: " << Reason;
+ }
+
+ std::error_code convertToErrorCode() const override {
+ return std::make_error_code(std::errc::invalid_argument);
+ }
+};
+
} // namespace lldb_private
#endif // liblldb_CompilerType_h_
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
