Add null-pointer checks when accessing a TypeSystem's SymbolFile

[augusto2112]

Summary

A type system is not guaranteed to have a symbol file. This patch adds null-pointer checks so we don't crash when trying to access a type system's symbol file.

[DavidSpickett]

General question, is it possible to write a test for this? (I've not looked into this area of code before)

[teemperor]

TypeSystemClang can be unit-tested just fine these days. It should also be possible to test Type (You can just model a random 'int' type from a TypeSystemClang instance).

[augusto2112]

Add GetExeModuleWhenMissingSymbolFile tesst.

[augusto2112]

@teemperor let me know if this test is OK, or if there's a better way to test this :)

[teemperor]

LGTM, just some nits and please address the clang-format issue.

lldb/unittests/Symbol/TestTypeSystemClang.cpp

746

Can you make that CompilerType? (Just because it's not clear what this returns, e.g. Type or CompilerType)

752

Can you also spell out the auto type and check that this is a nullptr? EXPECT_EQ(module, nullptr); Right now this could return some random pointer and pass.

[augusto2112]

Update test

lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp

return std::make_shared<lldb_private::Type>(
         type.getSymIndexId(), m_ast.GetSymbolFile(), ConstString(name), bytes,
         nullptr, LLDB_INVALID_UID, lldb_private::Type::eEncodingIsUID, decl,
         ast_enum, lldb_private::Type::ResolveState::Full);
   } break;
   case PDB_SymType::Typedef: {
     auto type_def = llvm::dyn_cast<PDBSymbolTypeTypedef>(&type);
     assert(type_def);
 
+    SymbolFile *symbol_file = m_ast.GetSymbolFile();
+    if (!symbol_file)
+      return nullptr;
+
     lldb_private::Type *target_type =
-        m_ast.GetSymbolFile()->ResolveTypeUID(type_def->getTypeId());
+        symbol_file->ResolveTypeUID(type_def->getTypeId());
     if (!target_type)
       return nullptr;
 
     std::string name =
         std::string(MSVCUndecoratedNameParser::DropScope(type_def->getName()));
     auto decl_ctx = GetDeclContextContainingSymbol(type);
 
     // Check if such a typedef already exists in the current context
case PDB_SymType::FunctionSig: {
     bool is_variadic = func_sig->isCVarArgs();
     // Drop last variadic argument.
     if (is_variadic)
       --num_args;
     for (uint32_t arg_idx = 0; arg_idx < num_args; arg_idx++) {
       auto arg = arg_enum->getChildAtIndex(arg_idx);
       if (!arg)
         break;
+
+      SymbolFile *symbol_file = m_ast.GetSymbolFile();
+      if (!symbol_file)
+        return nullptr;
+
       lldb_private::Type *arg_type =
-          m_ast.GetSymbolFile()->ResolveTypeUID(arg->getSymIndexId());
+          symbol_file->ResolveTypeUID(arg->getSymIndexId());
       // If there's some error looking up one of the dependent types of this
       // function signature, bail.
       if (!arg_type)
         return nullptr;
       CompilerType arg_ast_type = arg_type->GetFullCompilerType();
       arg_list.push_back(arg_ast_type);
     }
     lldbassert(arg_list.size() <= num_args);
 
     auto pdb_return_type = func_sig->getReturnType();
+    SymbolFile *symbol_file = m_ast.GetSymbolFile();
+    if (!symbol_file)
+      return nullptr;
+
     lldb_private::Type *return_type =
-        m_ast.GetSymbolFile()->ResolveTypeUID(pdb_return_type->getSymIndexId());
+        symbol_file->ResolveTypeUID(pdb_return_type->getSymIndexId());
     // If there's some error looking up one of the dependent types of this
     // function signature, bail.
     if (!return_type)
       return nullptr;
     CompilerType return_ast_type = return_type->GetFullCompilerType();
     uint32_t type_quals = 0;
     if (func_sig->isConstType())
       type_quals |= clang::Qualifiers::Const;
case PDB_SymType::ArrayType: {
     auto array_type = llvm::dyn_cast<PDBSymbolTypeArray>(&type);
     assert(array_type);
     uint32_t num_elements = array_type->getCount();
     uint32_t element_uid = array_type->getElementTypeId();
     llvm::Optional<uint64_t> bytes;
     if (uint64_t size = array_type->getLength())
       bytes = size;
 
+    SymbolFile *symbol_file = m_ast.GetSymbolFile();
+    if (!symbol_file)
+      return nullptr;
+
     // If array rank > 0, PDB gives the element type at N=0. So element type
     // will parsed in the order N=0, N=1,..., N=rank sequentially.
-    lldb_private::Type *element_type =
-        m_ast.GetSymbolFile()->ResolveTypeUID(element_uid);
+    lldb_private::Type *element_type = symbol_file->ResolveTypeUID(element_uid);
     if (!element_type)
       return nullptr;
 
     CompilerType element_ast_type = element_type->GetForwardCompilerType();
     // If element type is UDT, it needs to be complete.
     if (TypeSystemClang::IsCXXClassType(element_ast_type) &&
         !element_ast_type.GetCompleteType()) {
       if (TypeSystemClang::StartTagDeclarationDefinition(element_ast_type)) {
case PDB_SymType::BuiltinType: {
     return std::make_shared<lldb_private::Type>(
         builtin_type->getSymIndexId(), m_ast.GetSymbolFile(), type_name, bytes,
         nullptr, LLDB_INVALID_UID, lldb_private::Type::eEncodingIsUID, decl,
         builtin_ast_type, lldb_private::Type::ResolveState::Full);
   } break;
   case PDB_SymType::PointerType: {
     auto *pointer_type = llvm::dyn_cast<PDBSymbolTypePointer>(&type);
     assert(pointer_type);
-    Type *pointee_type = m_ast.GetSymbolFile()->ResolveTypeUID(
+
+    SymbolFile *symbol_file = m_ast.GetSymbolFile();
+    if (!symbol_file)
+      return nullptr;
+
+    Type *pointee_type = symbol_file->ResolveTypeUID(
         pointer_type->getPointeeType()->getSymIndexId());
     if (!pointee_type)
       return nullptr;
 
     if (pointer_type->isPointerToDataMember() ||
         pointer_type->isPointerToMemberFunction()) {
       auto class_parent_uid = pointer_type->getRawSymbol().getClassParentId();
-      auto class_parent_type =
-          m_ast.GetSymbolFile()->ResolveTypeUID(class_parent_uid);
+      auto class_parent_type = symbol_file->ResolveTypeUID(class_parent_uid);
       assert(class_parent_type);
 
       CompilerType pointer_ast_type;
       pointer_ast_type = TypeSystemClang::CreateMemberPointerType(
           class_parent_type->GetLayoutCompilerType(),
           pointee_type->GetForwardCompilerType());
       assert(pointer_ast_type);
 

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp

CompilerType TypeSystemClang::DeclGetFunctionArgumentType(void *opaque_decl,
   return CompilerType();
 }
 
 // CompilerDeclContext functions
 
 std::vector<CompilerDecl> TypeSystemClang::DeclContextFindDeclByName(
     void *opaque_decl_ctx, ConstString name, const bool ignore_using_decls) {
   std::vector<CompilerDecl> found_decls;
-  if (opaque_decl_ctx) {
+  SymbolFile *symbol_file = GetSymbolFile();
+  if (opaque_decl_ctx && symbol_file) {
     DeclContext *root_decl_ctx = (DeclContext *)opaque_decl_ctx;
     std::set<DeclContext *> searched;
     std::multimap<DeclContext *, DeclContext *> search_queue;
-    SymbolFile *symbol_file = GetSymbolFile();
 
     for (clang::DeclContext *decl_context = root_decl_ctx;
          decl_context != nullptr && found_decls.empty();
          decl_context = decl_context->getParent()) {
       search_queue.insert(std::make_pair(decl_context, decl_context));
 
       for (auto it = search_queue.find(decl_context); it != search_queue.end();
            it++) {

 // scope. Ideally we'd like to treat the file scope as an additional scope just
 // below the global scope.  More work needs to be done to recognise that, if
 // the decl we're trying to look up is static, we should compare its source
 // file with that of the current scope and return a lower number for it.
 uint32_t TypeSystemClang::CountDeclLevels(clang::DeclContext *frame_decl_ctx,
                                           clang::DeclContext *child_decl_ctx,
                                           ConstString *child_name,
                                           CompilerType *child_type) {
-  if (frame_decl_ctx) {
+  SymbolFile *symbol_file = GetSymbolFile();
+  if (frame_decl_ctx && symbol_file) {
     std::set<DeclContext *> searched;
     std::multimap<DeclContext *, DeclContext *> search_queue;
-    SymbolFile *symbol_file = GetSymbolFile();
 
     // Get the lookup scope for the decl we're trying to find.
     clang::DeclContext *parent_decl_ctx = child_decl_ctx->getParent();
 
     // Look for it in our scope's decl context and its parents.
     uint32_t level = 0;
     for (clang::DeclContext *decl_ctx = frame_decl_ctx; decl_ctx != nullptr;
          decl_ctx = decl_ctx->getParent()) {

lldb/source/Symbol/Type.cpp

ModuleSP Type::GetModule() {
   if (m_symbol_file)
     return m_symbol_file->GetObjectFile()->GetModule();
   return ModuleSP();
 }
 
 ModuleSP Type::GetExeModule() {
   if (m_compiler_type) {
     SymbolFile *symbol_file = m_compiler_type.GetTypeSystem()->GetSymbolFile();
+    if (symbol_file)
       return symbol_file->GetObjectFile()->GetModule();
   }
   return ModuleSP();
 }
 
 TypeAndOrName::TypeAndOrName(TypeSP &in_type_sp) {
   if (in_type_sp) {
     m_compiler_type = in_type_sp->GetForwardCompilerType();
     m_type_name = in_type_sp->GetName();

lldb/unittests/Symbol/TestTypeSystemClang.cpp

EXPECT_EQ(
       ScratchTypeSystemClang::DefaultAST,
       ScratchTypeSystemClang::InferIsolatedASTKindFromLangOpts(lang_opts));
 
   lang_opts.Modules = true;
   EXPECT_EQ(
       ScratchTypeSystemClang::IsolatedASTKind::CppModules,
       ScratchTypeSystemClang::InferIsolatedASTKindFromLangOpts(lang_opts));
 }
+
+TEST_F(TestTypeSystemClang, GetExeModuleWhenMissingSymbolFile) {
+  CompilerType compiler_type = m_ast->GetBasicTypeFromAST(lldb::eBasicTypeInt);
+  lldb_private::Type t(0, nullptr, ConstString("MyType"), llvm::None, nullptr,
+                       0, {}, {}, compiler_type,
+                       lldb_private::Type::ResolveState::Full);
+  // Test that getting the execution module when no type system is present
+  // is handled gracefully.
+  ModuleSP module = t.GetExeModule();
+  EXPECT_EQ(module.get(), nullptr);
+}
+