[Symbol] Add InvalidTypeError, a force-checked recoverable error

[vsk]

Summary

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:

1. 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.

2. 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.

[aprantl]

Going forward, we should transition to a model in which CompilerTypes are either valid or do not exist.

I don't understand very well how the LLDB type system works so excuse my naive questions: Does this account for lazyness? I.e., could there be value in having an unverified type that might be sufficient for what LLDB is trying to do with it, where validating it (which may involve recursively materializing all of its children) might fail? I could imagine that for some use-cases just knowing the size of a type would be sufficient.
I guess what I'm trying to say is: Are there code paths in LLDB that do something useful with a type where type.IsValid()==false ?

[JDevlieghere]

include/lldb/Symbol/CompilerType.h

446

I think this should be called InvalidTypeError?

[labath]

I agree that we should start using this in a lot more places. I think you should be able to shorten the logging code a bit by using the LLDB_LOG_ERROR macro, which I've created for this purpose.

source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp

334–339

How about:

if (!user_type_or_err) {
  LLDB_LOG_ERROR(log, user_type_or_err.takeError(), "{0}");
  return false;
}

[zturner]

source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp

375–380

I wonder if we need a macro like

#define RETURN_IF_UNEXPECTED(item, ret_val, log_categories)           \
  if (auto E = item.takeError()) {                                    \
    std::string Reason = llvm::toString(std::move(E));                \
    Log *log(lldb_private::GetLogIfAllCategoriesSet(log_categories)); \
    if (log) \
      log->Printf("%s", Reason.c_str());
    return ret_val;
  }

then we say:

RETURN_IF_UNEXPECTED(user_type_or_err, false, LIBLLDB_LOG_EXPRESSIONS);

I don't have too strong of an opinion, but as this pattern becomes more pervasive, it's annoying to have to write out 5-6 lines of code every time you call a function that returns an Expected<T>.

[jingham]

In D43912#1022916, @aprantl wrote:

Going forward, we should transition to a model in which CompilerTypes are either valid or do not exist.

I don't understand very well how the LLDB type system works so excuse my naive questions: Does this account for lazyness? I.e., could there be value in having an unverified type that might be sufficient for what LLDB is trying to do with it, where validating it (which may involve recursively materializing all of its children) might fail? I could imagine that for some use-cases just knowing the size of a type would be sufficient.
I guess what I'm trying to say is: Are there code paths in LLDB that do something useful with a type where type.IsValid()==false ?

No, I don't think so. Laziness might make a type go from valid (we only got the forward type) to invalid if the attempt to complete it fails for some reason (base class missing, for instance). But a not yet fully completed type will call itself valid, not invalid.

[vsk]

• Address some review feedback.

source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp

334–339

I think it'd be even more convenient to include the return into the macro, as Zachary suggested.

375–380

Thanks for the suggestion. I've introduced a macro which is pretty similar to what you've shown here.

[vsk]

In D43912#1023078, @jingham wrote:

In D43912#1022916, @aprantl wrote:

Going forward, we should transition to a model in which CompilerTypes are either valid or do not exist.

I don't understand very well how the LLDB type system works so excuse my naive questions: Does this account for lazyness? I.e., could there be value in having an unverified type that might be sufficient for what LLDB is trying to do with it, where validating it (which may involve recursively materializing all of its children) might fail? I could imagine that for some use-cases just knowing the size of a type would be sufficient.
I guess what I'm trying to say is: Are there code paths in LLDB that do something useful with a type where type.IsValid()==false ?

If we discover code like this, transitioning the code into a model where types are force-checked will require caution, but would still be possible/desirable. For example, we could refactor the operations on invalid type s.t they use a different type, i.e one without the same validity requirement as CompilerType.

No, I don't think so. Laziness might make a type go from valid (we only got the forward type) to invalid if the attempt to complete it fails for some reason (base class missing, for instance). But a not yet fully completed type will call itself valid, not invalid.

Thanks for the explanation Jim!

[jingham]

The problem with your RETURN_IF_UNEXPECTED macro is that it make it impossible to put a breakpoint only on the "type was invalid case." To catch that happening while debugging you'd have to write a conditional breakpoint that also checks the error.

That seems like something you would really want to be able to do. If you leave the return outside the macro, then you can break on the return. IMHO that's worth having to write

if (CHECK_IF_ERROR)

return;

[labath]

include/lldb/Utility/Log.h

277–282

it looks like this could be just handled by delegating to the LLDB_LOG_ERROR macro.

source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp

334–339

Yeah, I'm fine with the new macro as well. I see it mostly as a transitionary measure. Once most of the code uses llvm::Error, we should be able to just propagate that (or do something interesting with the error).

[jingham]

The phase where we are converting everything over seems exactly when we want to be able to easily stop on the error cases when debugging, which using a macro that contains the return makes impossible.

[vsk]

The problem with your RETURN_IF_UNEXPECTED macro is that it make it impossible to put a breakpoint only on the "type was invalid case." To catch that happening while debugging you'd have to write a conditional breakpoint that also checks the error.

Yes, we'd need a conditional breakpoint if more than one InvalidTypeError instance is created (otherwise we'd just break on the constructor). While I hope we never have to deal with that, I'm happy to keep things more explicit for now.

include/lldb/Utility/Log.h

277–282

I missed this earlier because I mistakenly named two variables error_private. At any rate, there's an objection to having a return_if macro so I'm setting it aside.

[jingham]

Asking the person trying to catch a bad CompilerType to understand how the error mechanism works to effect this goal seems a little much, especially 'cause they are going to be focused on the return which your macro would make tantalizingly out of my reach. OTOH, if you put a comment before the macro saying "To catch this error return case, do X" I have no objections to having the macro.

[zturner]

I'm also ok with not having the macro fwiw, just an idea to reduce boilerplate.

[vsk]

• While playing around with InvalidTypeError, I found that it's useful to be able to pass a log category mask to the error-logging macros. I've implemented support for that.

• I also found that the RETURN_IF_UNEXPECTED macro is very convenient in practice. There seems to be no objection to it provided that I document how to set breakpoints for failure cases. I've brought the macro back.

Thanks for your feedback! PTAL.

[labath]

include/lldb/Utility/Log.h

259–260

The thing to remember here is that we have multiple log channels. So the unsigned value must be a category from the "lldb" channel. That is by far the most prevalent channel, so it's probably fine that we don't have a shorthand way of specifying the other channels, but it should be called out explicitly.

[vsk]

• Clarify that the log_categories macro argument is for logging to the "lldb" channel.

[labath]

Thanks. The logging stuff looks good to me. I know very little about the TypeFromUser class and friends, so someone else should probably ack that.

[vsk]

I don't think this got a lot of buy-in. Let's revisit this if the need to revamp CompilerType becomes more pressing.

include/lldb/Symbol/CompilerType.h

 #include <string>
 #include <vector>
 
 // Other libraries and framework includes
 // Project includes
 #include "lldb/Core/ClangForward.h"
 #include "lldb/lldb-private.h"
 #include "llvm/ADT/APSInt.h"
+#include "llvm/Support/Error.h"
 
 namespace lldb_private {
 
 class DataExtractor;
 
 //----------------------------------------------------------------------
 // A class that can carry around a clang ASTContext and a opaque clang
 // QualType. A clang::QualType can be easily reconstructed from an

 bool operator==(const CompilerType &lhs, const CompilerType &rhs);
 bool operator!=(const CompilerType &lhs, const CompilerType &rhs);
 
 struct CompilerType::IntegralTemplateArgument {
   llvm::APSInt value;
   CompilerType type;
 };
 
+/// A force-checked error used to describe type construction failures.
+class InvalidTypeError : public llvm::ErrorInfo<InvalidTypeError> {
+public:
+  static char ID;
+  std::string Reason;
+
+  InvalidTypeError(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_

include/lldb/Utility/Log.h

static uint32_t GetFlags(llvm::raw_ostream &stream, const ChannelMap::value_type &entry,
                            llvm::ArrayRef<const char *> categories);
 
   static void DisableLoggingChild();
 
   Log(const Log &) = delete;
   void operator=(const Log &) = delete;
 };
 
+// Helper utilities for macros which consume and log errors. These are not
+// meant to be used directly.
+template <typename... Args>
+void _takeAndLogError(const char *file, const char *func, llvm::Error E,
+                      Log *log, Args &&... args) {
+  if (E && log)
+    log->FormatError(std::move(E), file, func, std::forward<Args>(args)...);
+  else
+    llvm::consumeError(std::move(E));
+}
+template <typename... Args>
+void _takeAndLogError(const char *file, const char *func, llvm::Error E,
+                      uint32_t log_categories, Args &&... args) {
+  Log *log(lldb_private::GetLogIfAllCategoriesSet(log_categories));
+  _takeAndLogError(file, func, std::move(E), log, std::forward<Args>(args)...);
+}
+
 } // namespace lldb_private
 
 #define LLDB_LOG(log, ...)                                                     \
   do {                                                                         \
     ::lldb_private::Log *log_private = (log);                                  \
     if (log_private)                                                           \
       log_private->Format(__FILE__, __func__, __VA_ARGS__);                    \
   } while (0)
 
 #define LLDB_LOGV(log, ...)                                                    \
   do {                                                                         \
     ::lldb_private::Log *log_private = (log);                                  \
     if (log_private && log_private->GetVerbose())                              \
       log_private->Format(__FILE__, __func__, __VA_ARGS__);                    \
   } while (0)
 
 // Write message to log, if error is set. In the log message refer to the error
 // with {0}. Error is cleared regardless of whether logging is enabled.
+//
+// For convenience, log may either be a Log instance or an unsigned value
+// specifying log categories in the "lldb" log channel.
 #define LLDB_LOG_ERROR(log, error, ...)                                        \
+  ::lldb_private::_takeAndLogError(__FILE__, __func__, error, log, __VA_ARGS__)
+
+// If item contains an unexpected result, log the error and return the given
+// return value.
+//
+// For convenience, log may either be a Log instance or an unsigned value
+// specifying log categories in the "lldb" log channel.
+//
+// To set a breakpoint on the failure case, break on the constructor of the
+// appropriate ErrorInfo. Set a breakpoint on ErrorInfoBase to catch all errors.
+#define RETURN_IF_UNEXPECTED(item, ret_val, log)                               \
   do {                                                                         \
-    ::lldb_private::Log *log_private = (log);                                  \
-    ::llvm::Error error_private = (error);                                     \
-    if (log_private && error_private) {                                        \
-      log_private->FormatError(::std::move(error_private), __FILE__, __func__, \
-                               __VA_ARGS__);                                   \
-    } else                                                                     \
-      ::llvm::consumeError(::std::move(error_private));                        \
+    if (::llvm::Error error_private = (item).takeError()) {                    \
+      LLDB_LOG_ERROR(log, std::move(error_private), "{0}");                    \
+      return ret_val;                                                          \
+    }                                                                          \
   } while (0)
 
 #endif // LLDB_UTILITY_LOG_H

source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h

private:
   /// @param[in] target
   ///     The ClangASTContext to move to.
   /// @param[in] source
   ///     The ClangASTContext to move from.  This is assumed to be going away.
   /// @param[in] parser_type
   ///     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,
+  llvm::Expected<TypeFromUser> DeportType(ClangASTContext &target,
+                                          ClangASTContext &source,
                                           TypeFromParser parser_type);
 
   ClangASTContext *GetClangASTContext();
 };
 
 } // namespace lldb_private
 
 #endif // liblldb_ClangExpressionDeclMap_h_

source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp

static clang::QualType ExportAllDeclaredTypes(
   merger.AddSources(importer_source);
   clang::ASTImporter &exporter = merger.ImporterForOrigin(source);
   CompleteAllDeclContexts(exporter, file, root);
   clang::QualType ret = exporter.Import(root);
   merger.RemoveSources(importer_source);
   return ret;
 }
 
-TypeFromUser ClangExpressionDeclMap::DeportType(ClangASTContext &target,
-                                                ClangASTContext &source,
-                                                TypeFromParser parser_type) {
+llvm::Expected<TypeFromUser>
+ClangExpressionDeclMap::DeportType(ClangASTContext &target,
   assert (&target == m_target->GetScratchClangASTContext());
   assert ((TypeSystem*)&source == parser_type.GetTypeSystem());
   assert (source.getASTContext() == m_ast_context);
   
   if (m_ast_importer_sp) {
     return TypeFromUser(m_ast_importer_sp->DeportType(
                             target.getASTContext(), source.getASTContext(),
                             parser_type.GetOpaqueQualType()),
                         &target);
   } else if (m_merger_up) {
     clang::FileID source_file =
         source.getASTContext()->getSourceManager().getFileID(
             source.getASTContext()->getTranslationUnitDecl()->getLocation());
     auto scratch_ast_context = static_cast<ClangASTContextForExpressions*>(
         m_target->GetScratchClangASTContext());
     clang::QualType exported_type = ExportAllDeclaredTypes(
         scratch_ast_context->GetMergerUnchecked(),
         *source.getASTContext(), *source.getFileManager(),
         m_merger_up->GetOrigins(),
         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<InvalidTypeError>("Could not deport type");
 }
 
 bool ClangExpressionDeclMap::AddPersistentVariable(const NamedDecl *decl,
                                                    const ConstString &name,
                                                    TypeFromParser parser_type,
                                                    bool is_result,
                                                    bool is_lvalue) {
   assert(m_parser_vars.get());
 
   ClangASTContext *ast =
       llvm::dyn_cast_or_null<ClangASTContext>(parser_type.GetTypeSystem());
   if (ast == nullptr)
     return false;
 
   if (m_parser_vars->m_materializer && is_result) {
     Status err;
 
     ExecutionContext &exe_ctx = m_parser_vars->m_exe_ctx;
     Target *target = exe_ctx.GetTargetPtr();
     if (target == nullptr)
       return false;
 
-    TypeFromUser user_type =
+    llvm::Expected<TypeFromUser> user_type_or_err =
         DeportType(*target->GetScratchClangASTContext(), *ast, parser_type);
+    RETURN_IF_UNEXPECTED(user_type_or_err, false, LIBLLDB_LOG_EXPRESSIONS);
+    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);
 
     ClangExpressionVariable *var = new ClangExpressionVariable(
         exe_ctx.GetBestExecutionContextScope(), name, user_type,
         m_parser_vars->m_target_info.byte_order,
         m_parser_vars->m_target_info.address_byte_size);
 
     m_found_entities.AddNewlyConstructedVariable(var);
bool ClangExpressionDeclMap::AddPersistentVariable(const NamedDecl *decl,
   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()) {
-    if (log)
-      log->Printf("Persistent variable's type wasn't copied successfully");
-    return false;
-  }
+  llvm::Expected<TypeFromUser> user_type_or_err =
+      DeportType(*context, *ast, parser_type);
+  RETURN_IF_UNEXPECTED(user_type_or_err, false, log);
+  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;
 
   ClangExpressionVariable *var = llvm::cast<ClangExpressionVariable>(
       m_parser_vars->m_persistent_vars
           ->CreatePersistentVariable(
               exe_ctx.GetBestExecutionContextScope(), name, user_type,
               m_parser_vars->m_target_info.byte_order,

source/Symbol/CompilerType.cpp

return lhs.GetTypeSystem() == rhs.GetTypeSystem() &&
          lhs.GetOpaqueQualType() == rhs.GetOpaqueQualType();
 }
 
 bool lldb_private::operator!=(const lldb_private::CompilerType &lhs,
                               const lldb_private::CompilerType &rhs) {
   return lhs.GetTypeSystem() != rhs.GetTypeSystem() ||
          lhs.GetOpaqueQualType() != rhs.GetOpaqueQualType();
 }
+
+char lldb_private::InvalidTypeError::ID;