Add missing nullptr checks.

[aprantl]

Summary

GetPersistentExpressionStateForLanguage() can return a nullptr if it cannot construct a typesystem. This patch adds missing nullptr checks at all uses.

Inspired by rdar://problem/58317195

[davide]

Raphael and Jim should look at the expression evaluator bits.

[jingham]

If we can't make a persistent expression state, are we going to be able to do anything useful with expressions? I don't see anything wrong here, but it seems like we should really be putting up a crunchy frog warning and erroring out of "expr"directly if we really can't make a type system.

[aprantl]

In D72413#1810969, @jingham wrote:

If we can't make a persistent expression state, are we going to be able to do anything useful with expressions? I don't see anything wrong here, but it seems like we should really be putting up a crunchy frog warning and erroring out of "expr"directly if we really can't make a type system.

I looked at that for a bit, but there is no obvious place where to early exit without refactoring much more of ClangExpressionDeclMap.

[shafik]

I am curious what prompted this set of changes, they look sensible but I don't see how we can do anything useful if we end up in this state.

Do we currently have a way to end up in this state?

[aprantl]

Do we currently have a way to end up in this state?

Not in llvm.org. This was prompted by a swift-lldb-crash where we can end up with no Swift context after a catastrophic error. I believe that in practice the clang runtimes are always available, but the API returns a nullable pointer so the call sites should take this into account:

PersistentExpressionState *
Target::GetPersistentExpressionStateForLanguage(lldb::LanguageType language) {
  auto type_system_or_err = GetScratchTypeSystemForLanguage(language, true);

  if (auto err = type_system_or_err.takeError()) {
    LLDB_LOG_ERROR(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_TARGET),
                   std::move(err),
                   "Unable to get persistent expression state for language {}",
                   Language::GetNameForLanguageType(language));
    return nullptr;
  }

  return type_system_or_err->GetPersistentExpressionState();
}

[aprantl]

Should we merge this like that, or is there a better way of doing this?

[davide]

In D72413#1812464, @aprantl wrote:

Should we merge this like that, or is there a better way of doing this?

We should merge it like this, IMHO.

[davide]

@teemperor what do you think?

[teemperor]

The C++ expression parser will probably behave incredibly incorrectly without a persistent state but before this patch it just crashed, so I think this is good to go.

[teemperor]

In D72413#1811038, @aprantl wrote:

In D72413#1810969, @jingham wrote:

If we can't make a persistent expression state, are we going to be able to do anything useful with expressions? I don't see anything wrong here, but it seems like we should really be putting up a crunchy frog warning and erroring out of "expr"directly if we really can't make a type system.

I looked at that for a bit, but there is no obvious place where to early exit without refactoring much more of ClangExpressionDeclMap.

FWIW, you could also just add the check to ClangUserExpression::ClangUserExpressionHelper::ResetDeclMap and return a nullptr there. The API is designed to return a nullptr for utility expressions on that don't use the DeclMap. That's the only place in upstream LLDB where we construct a DeclMap with a target, so if there is a check there then the DeclMap doesn't need any further changes and we can just assume there is a persistent state in the rest of the code. But it doesn't really make the code much better, just this patch smaller.

[davide]

In D72413#1813630, @teemperor wrote:

The C++ expression parser will probably behave incredibly incorrectly without a persistent state but before this patch it just crashed, so I think this is good to go.

Should we consider a refactor to ClangExpressionDeclMap that makes this more obvious? [and maybe allows us to not sprinkling these checks everywhere] -- as a followup?

lldb/source/Expression/REPL.cpp

if (code[0] == ':') {
       expr_options.SetColorizeErrors(colorize_err);
       expr_options.SetPoundLine(m_repl_source_path.c_str(),
                                 m_code.GetSize() + 1);
 
       expr_options.SetLanguage(GetLanguage());
 
       PersistentExpressionState *persistent_state =
           m_target.GetPersistentExpressionStateForLanguage(GetLanguage());
+      if (!persistent_state)
+        return;
 
       const size_t var_count_before = persistent_state->GetSize();
 
       const char *expr_prefix = nullptr;
       lldb::ValueObjectSP result_valobj_sp;
       Status error;
       lldb::ModuleSP jit_module_sp;
       lldb::ExpressionResults execution_results =

lldb/source/Expression/UserExpression.cpp

UserExpression::Execute(DiagnosticManager &diagnostic_manager,
                         ExecutionContext &exe_ctx,
                         const EvaluateExpressionOptions &options,
                         lldb::UserExpressionSP &shared_ptr_to_me,
                         lldb::ExpressionVariableSP &result_var) {
   lldb::ExpressionResults expr_result = DoExecute(
       diagnostic_manager, exe_ctx, options, shared_ptr_to_me, result_var);
   Target *target = exe_ctx.GetTargetPtr();
   if (options.GetResultIsInternal() && result_var && target) {
-    target->GetPersistentExpressionStateForLanguage(m_language)
-        ->RemovePersistentVariable(result_var);
+    if (auto *persistent_state =
+            target->GetPersistentExpressionStateForLanguage(m_language))
+      persistent_state->RemovePersistentVariable(result_var);
   }
   return expr_result;
 }

lldb/source/Plugins/ExpressionParser/Clang/ASTResultSynthesizer.cpp

void ASTResultSynthesizer::RecordPersistentDecl(NamedDecl *D) {
   ConstString name_cs(name.str().c_str());
 
   LLDB_LOGF(log, "Recording persistent decl %s\n", name_cs.GetCString());
 
   m_decls.push_back(D);
 }
 
 void ASTResultSynthesizer::CommitPersistentDecls() {
-  PersistentExpressionState *state =
+  auto *state =
       m_target.GetPersistentExpressionStateForLanguage(lldb::eLanguageTypeC);
+  if (!state)
+    return;
+
   auto *persistent_vars = llvm::cast<ClangPersistentVariables>(state);
   ClangASTContext *scratch_ctx = ClangASTContext::GetScratch(m_target);
 
   for (clang::NamedDecl *decl : m_decls) {
     StringRef name = decl->getName();
     ConstString name_cs(name.str().c_str());
 
     Decl *D_scratch = m_target.GetClangASTImporter()->DeportDecl(

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp

 
 void ClangExpressionDeclMap::InstallCodeGenerator(
     clang::ASTConsumer *code_gen) {
   assert(m_parser_vars);
   m_parser_vars->m_code_gen = code_gen;
 }
 
 void ClangExpressionDeclMap::DidParse() {
-  if (m_parser_vars) {
+  if (m_parser_vars && m_parser_vars->m_persistent_vars) {
     for (size_t entity_index = 0, num_entities = m_found_entities.GetSize();
          entity_index < num_entities; ++entity_index) {
       ExpressionVariableSP var_sp(
           m_found_entities.GetVariableAtIndex(entity_index));
       if (var_sp)
         llvm::cast<ClangExpressionVariable>(var_sp.get())
             ->DisableParserVars(GetParserID());
     }
bool ClangExpressionDeclMap::AddPersistentVariable(const NamedDecl *decl,
   if (!user_type.GetOpaqueQualType()) {
     LLDB_LOGF(log, "Persistent variable's type wasn't copied successfully");
     return false;
   }
 
   if (!m_parser_vars->m_target_info.IsValid())
     return false;
 
+  if (!m_parser_vars->m_persistent_vars)
+    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,
               m_parser_vars->m_target_info.address_byte_size)
           .get());
 
bool ClangExpressionDeclMap::AddValueToStruct(const NamedDecl *decl,
 
   if (ClangExpressionVariable::FindVariableInList(m_struct_members, decl,
                                                   GetParserID()))
     return true;
 
   ClangExpressionVariable *var(ClangExpressionVariable::FindVariableInList(
       m_found_entities, decl, GetParserID()));
 
-  if (!var) {
+  if (!var && m_parser_vars->m_persistent_vars) {
     var = ClangExpressionVariable::FindVariableInList(
         *m_parser_vars->m_persistent_vars, decl, GetParserID());
     is_persistent_variable = true;
   }
 
   if (!var)
     return false;
 
clang::NamedDecl *ClangExpressionDeclMap::GetPersistentDecl(ConstString name) {
   if (!m_parser_vars)
     return nullptr;
   Target *target = m_parser_vars->m_exe_ctx.GetTargetPtr();
   if (!target)
     return nullptr;
 
   ClangASTContext::GetScratch(*target);
 
+  if (!m_parser_vars->m_persistent_vars)
+    return nullptr;
   return m_parser_vars->m_persistent_vars->GetPersistentDecl(name);
 }
 
 void ClangExpressionDeclMap::SearchPersistenDecls(NameSearchContext &context,
                                                   const ConstString name,
                                                   unsigned int current_id) {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
 
if (name == g_lldb_local_vars_namespace_cstr) {
       return;
     }
 
     // any other $__lldb names should be weeded out now
     if (name.GetStringRef().startswith("$__lldb"))
       return;
 
     // No ParserVars means we can't do register or variable lookup.
-    if (!m_parser_vars)
+    if (!m_parser_vars || !m_parser_vars->m_persistent_vars)
       return;
 
     ExpressionVariableSP pvar_sp(
         m_parser_vars->m_persistent_vars->GetVariable(name));
 
     if (pvar_sp) {
       AddOneVariable(context, pvar_sp, current_id);
       return;

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

ClangExpressionParser::ClangExpressionParser(
   // 7. Set up the source management objects inside the compiler
   m_compiler->createFileManager();
   if (!m_compiler->hasSourceManager())
     m_compiler->createSourceManager(m_compiler->getFileManager());
   m_compiler->createPreprocessor(TU_Complete);
 
   if (ClangModulesDeclVendor *decl_vendor =
           target_sp->GetClangModulesDeclVendor()) {
-    ClangPersistentVariables *clang_persistent_vars =
-        llvm::cast<ClangPersistentVariables>(
+    if (auto *clang_persistent_vars = llvm::cast<ClangPersistentVariables>(
             target_sp->GetPersistentExpressionStateForLanguage(
-                lldb::eLanguageTypeC));
-    std::unique_ptr<PPCallbacks> pp_callbacks(new LLDBPreprocessorCallbacks(
-        *decl_vendor, *clang_persistent_vars, m_compiler->getSourceManager()));
+                lldb::eLanguageTypeC))) {
+      std::unique_ptr<PPCallbacks> pp_callbacks(
+          new LLDBPreprocessorCallbacks(*decl_vendor, *clang_persistent_vars,
+                                        m_compiler->getSourceManager()));
       m_pp_callbacks =
           static_cast<LLDBPreprocessorCallbacks *>(pp_callbacks.get());
       m_compiler->getPreprocessor().addPPCallbacks(std::move(pp_callbacks));
     }
+  }
 
   // 8. Most of this we get from the CompilerInstance, but we also want to give
   // the context an ExternalASTSource.
 
   auto &PP = m_compiler->getPreprocessor();
   auto &builtin_context = PP.getBuiltinInfo();
   builtin_context.initializeBuiltins(PP.getIdentifierTable(),
                                      m_compiler->getLangOpts());

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp

if (target->GetArchitecture().GetMachine() == llvm::Triple::x86_64) {
       if (lldb::PlatformSP platform_sp = target->GetPlatform()) {
         static ConstString g_platform_ios_simulator("ios-simulator");
         if (platform_sp->GetPluginName() == g_platform_ios_simulator) {
           target_specific_defines = "typedef bool BOOL;\n";
         }
       }
     }
 
-    if (ClangModulesDeclVendor *decl_vendor =
-            target->GetClangModulesDeclVendor()) {
-      ClangPersistentVariables *persistent_vars =
-          llvm::cast<ClangPersistentVariables>(
-              target->GetPersistentExpressionStateForLanguage(
-                  lldb::eLanguageTypeC));
+    ClangModulesDeclVendor *decl_vendor = target->GetClangModulesDeclVendor();
+    auto *persistent_vars = llvm::cast<ClangPersistentVariables>(
+        target->GetPersistentExpressionStateForLanguage(lldb::eLanguageTypeC));
+    if (decl_vendor && persistent_vars) {
       const ClangModulesDeclVendor::ModuleVector &hand_imported_modules =
           persistent_vars->GetHandLoadedClangModules();
       ClangModulesDeclVendor::ModuleVector modules_for_macros;
 
       for (ClangModulesDeclVendor::ModuleID module : hand_imported_modules) {
         modules_for_macros.push_back(module);
       }
 

lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp

if (Target *target = exe_ctx.GetTargetPtr()) {
     return false;
   }
   return true;
 }
 
 static void SetupDeclVendor(ExecutionContext &exe_ctx, Target *target) {
   if (ClangModulesDeclVendor *decl_vendor =
           target->GetClangModulesDeclVendor()) {
+    auto *persistent_state = llvm::cast<ClangPersistentVariables>(
+        target->GetPersistentExpressionStateForLanguage(lldb::eLanguageTypeC));
+    if (!persistent_state)
+      return;
     const ClangModulesDeclVendor::ModuleVector &hand_imported_modules =
-        llvm::cast<ClangPersistentVariables>(
-            target->GetPersistentExpressionStateForLanguage(
-                lldb::eLanguageTypeC))
-            ->GetHandLoadedClangModules();
+        persistent_state->GetHandLoadedClangModules();
     ClangModulesDeclVendor::ModuleVector modules_for_macros;
 
     for (ClangModulesDeclVendor::ModuleID module : hand_imported_modules) {
       modules_for_macros.push_back(module);
     }
 
     if (target->GetEnableAutoImportClangModules()) {
       if (StackFrame *frame = exe_ctx.GetFramePtr()) {
if (m_execution_unit_sp) {
     // If there is more than one external function in the execution unit, it
     // needs to keep living even if it's not top level, because the result
     // could refer to that function.
 
     if (m_execution_unit_sp->GetJittedFunctions().size() > 1) {
       register_execution_unit = true;
     }
 
-    if (register_execution_unit)
-      exe_ctx.GetTargetPtr()
-          ->GetPersistentExpressionStateForLanguage(m_language)
-          ->RegisterExecutionUnit(m_execution_unit_sp);
+    if (register_execution_unit) {
+      if (auto *persistent_state =
+              exe_ctx.GetTargetPtr()->GetPersistentExpressionStateForLanguage(
+                  m_language))
+        persistent_state->RegisterExecutionUnit(m_execution_unit_sp);
+    }
   }
 
   if (generate_debug_info) {
     lldb::ModuleSP jit_module_sp(m_execution_unit_sp->GetJITModule());
 
     if (jit_module_sp) {
       ConstString const_func_name(FunctionName());
       FileSpec jit_file;

lldb/source/Target/ABI.cpp

ValueObjectSP ABI::GetReturnValueObject(Thread &thread, CompilerType &ast_type,
 
   if (persistent) {
     Target &target = *thread.CalculateTarget();
     PersistentExpressionState *persistent_expression_state =
         target.GetPersistentExpressionStateForLanguage(
             ast_type.GetMinimumLanguage());
 
     if (!persistent_expression_state)
-      return ValueObjectSP();
+      return {};
 
     auto prefix = persistent_expression_state->GetPersistentVariablePrefix();
     ConstString persistent_variable_name =
         persistent_expression_state->GetNextPersistentVariableName(target,
                                                                    prefix);
 
     lldb::ValueObjectSP const_valobj_sp;