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

747 ↗

(On Diff #344151)

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

753 ↗

(On Diff #344151)

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

 //===-- Type.cpp ----------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include <stdio.h>
 
 #include "lldb/Core/Module.h"
 #include "lldb/Utility/DataBufferHeap.h"
 #include "lldb/Utility/DataExtractor.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/Scalar.h"
 #include "lldb/Utility/StreamString.h"
 
 #include "lldb/Symbol/CompilerType.h"
 #include "lldb/Symbol/ObjectFile.h"
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();