Change SymbolFile::ParseTypes to ParseTypesForCompileUnit

[zturner]

Summary

The function SymbolFile::ParseTypes previously accepted a SymbolContext. This makes it extremely difficult to implement faithfully, because you have to account for all possible combinations of members being set in the SymbolContext. On the other hand, no clients of this function actually care about implementing this function to this strict of a standard. AFAICT, there is actually only 1 client in the entire codebase, and it is the function ParseAllDebugSymbols, which is itself only called for testing purposes when dumping information. At this call-site, the only field it sets is the CompileUnit, meaning that an implementer of a SymbolFile need not worry about any examining or handling any other fields which might be set.

By restricting this API to accept exactly a CompileUnit& and nothing more, we can simplify the life of new SymbolFile plugin implementers by making it clear exactly what the necessary and sufficient set of functionality they need to implement is, while at the same time removing some dead code that tried to handle other types of SymbolContext fields that were never going to be set anyway.

[labath]

Sounds good to me (not clicking accept to see if anyone else has an opinion here).

[amccarth]

So this Patch is effectively NFC, since no caller (not even a test) was using the functionality you've removed. Seems like a nice refactor.

lldb/source/Core/Module.cpp

382 ↗

(On Diff #180766)

I'd argue that this comment is now obsolete.

[clayborg]

So I like the ability to specify any symbol context to parse all types for. If you pass in a symbol context with only a module filled in, we parse all types. If we pass in a symbol context with only the compile unit filled in (no function), we parse all types in the compile unit. If we pass in a symbol context with a function or block, we parse all types in that function or block. Currently it is only being used for compile units, but I don't think we need to change the API.

lldb/source/Plugins/SymbolFile/Symtab/SymbolFileSymtab.cpp

228–238 ↗

(On Diff #180766)

All these functions that just return zero, error, fail, should have default implementations in the SymbolFile class IMHO. Keeps diffs down.

[clayborg]

So there really aren't that many things:

• module only filled out, parse all types
• compile unit only filled out, parse all type from a compile unit
• function filled out, parse all types in a function
• block filled out, parse all types in block, or we can skip this one

[clayborg]

But if everyone else thinks differently, I'll go with the majority. Just my initial thoughts.

[zturner]

In D56462#1351391, @clayborg wrote:

So I like the ability to specify any symbol context to parse all types for. If you pass in a symbol context with only a module filled in, we parse all types. If we pass in a symbol context with only the compile unit filled in (no function), we parse all types in the compile unit. If we pass in a symbol context with a function or block, we parse all types in that function or block. Currently it is only being used for compile units, but I don't think we need to change the API.

I'm actually specifically trying to remove this flexibility in the API because I think it's harmful. In fact as we speak I'm writing up an RFC to send to the mailing list. As someone who just went through implementing a new SymbolFile plugin, it is very difficult to implement these functions faithfully for arbitrary SymbolContexts, and begs for untested code as well as dead code (as evidenced by this patch, there was a bunch of dead code for combinations of SymbolContext fields that no caller actually cared about). So nobody currently even needs this anyway. which means that specifying an arbitrary SymbolContext is a case of YAGNI. If someone comes along one day and needs the ability to parse types in a function or block, they can call ParseTypesForFunction followed by ParseTypesForBlock. This way it is simple and easy for the caller to understand how to use the function, and it is equally simple and easy to understand for the implementer to implement the function. Currently it is complicated for both. For example, does specifying a Module and a Function mean to parse all types for that module or that function? Or does it mean to parse all types for that module and that function.

I would like to remove the ability to specify arbitrary SymbolContext for most of the other SymboFile functions as well (although there are a few where we might still need it).

[jingham]

In D56462#1351394, @clayborg wrote:

But if everyone else thinks differently, I'll go with the majority. Just my initial thoughts.

IIRC, the original point of this ParseTypes function was to have a way for lldb developers to force lldb to parse all the types in some interesting context. The equivalent facility had proved useful in gdb (maint print symbols) to trigger obscure type parsing issues directly. We didn't add a command for it since most often you are debugging lldb with lldb so you can just call it by hand.

When ParseTypes(SymbolContext &sc) gets used in ParseAllDebugSymbols, the calling code was already doing the iteration over CU's to dump all the types in the module. The code to do that iteration is quite simple, so I don't see the need to fold it into the ParseTypes function, and when you want to do the CU iteration by hand while debugging, you can call ParseAllDebugSymbols.

I'm not sure how useful "parsing all the types in a function" is, as types aren't usually defined in functions and so forcing just the ones in the function to get realized NOW isn't all that helpful. This might be more useful if it parsed all the types referred to (i.e. any DW_AT_type), but I don't think that's what it does. It just handles the various DW_TAG_some_kind_of_type DIEs.

So I agree with Zachary that supporting all the possibilities in a SymbolContext seems overly complex to define and not all that helpful. ParseTypesForCompileUnit seems like the right level to do this. It might be nice to have a "Module::ParseAllDebugTypes" for calling by hand, since "Module::ParseAllDebugSymbols" does way more work than needed if you know you are just interested in type parsing. But that's a minor thing.

lldb/trunk/include/lldb/Symbol/SymbolFile.h

public:
   virtual bool
   ParseCompileUnitIsOptimized(const lldb_private::SymbolContext &sc) {
     return false;
   }
   virtual bool
   ParseImportedModules(const SymbolContext &sc,
                        std::vector<ConstString> &imported_modules) = 0;
   virtual size_t ParseFunctionBlocks(const SymbolContext &sc) = 0;
-  virtual size_t ParseTypes(const SymbolContext &sc) = 0;
+  virtual size_t ParseTypesForCompileUnit(CompileUnit &comp_unit) = 0;
   virtual size_t ParseVariablesForContext(const SymbolContext &sc) = 0;
   virtual Type *ResolveTypeUID(lldb::user_id_t type_uid) = 0;
 
 
   /// The characteristics of an array type.
   struct ArrayInfo {
     int64_t first_index;
     llvm::SmallVector<uint64_t, 1> element_orders;

lldb/trunk/include/lldb/Symbol/SymbolVendor.h

public:
 
   virtual bool ParseCompileUnitIsOptimized(const SymbolContext &sc);
 
   virtual bool ParseImportedModules(const SymbolContext &sc,
                                     std::vector<ConstString> &imported_modules);
 
   virtual size_t ParseFunctionBlocks(const SymbolContext &sc);
 
-  virtual size_t ParseTypes(const SymbolContext &sc);
+  virtual size_t ParseTypesForCompileUnit(CompileUnit &comp_unit);
 
   virtual size_t ParseVariablesForContext(const SymbolContext &sc);
 
   virtual Type *ResolveTypeUID(lldb::user_id_t type_uid);
 
   virtual uint32_t ResolveSymbolContext(const Address &so_addr,
                                         lldb::SymbolContextItem resolve_scope,
                                         SymbolContext &sc);

lldb/trunk/source/Core/Module.cpp

if (sc.comp_unit) {
         sc.function = f.get();
         symbols->ParseFunctionBlocks(sc);
         // Parse the variables for this function and all its blocks
         symbols->ParseVariablesForContext(sc);
         return false;
       });
 
       // Parse all types for this compile unit
-      sc.function = nullptr;
-      symbols->ParseTypes(sc);
+      symbols->ParseTypesForCompileUnit(*sc.comp_unit);
     }
   }
 }
 
 void Module::CalculateSymbolContext(SymbolContext *sc) {
   sc->module_sp = shared_from_this();
 }
 

lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h

public:
   ParseCompileUnitIsOptimized(const lldb_private::SymbolContext &sc) override;
 
   bool ParseImportedModules(
       const lldb_private::SymbolContext &sc,
       std::vector<lldb_private::ConstString> &imported_modules) override;
 
   size_t ParseFunctionBlocks(const lldb_private::SymbolContext &sc) override;
 
-  size_t ParseTypes(const lldb_private::SymbolContext &sc) override;
+  size_t
+  ParseTypesForCompileUnit(lldb_private::CompileUnit &comp_unit) override;
 
   size_t
   ParseVariablesForContext(const lldb_private::SymbolContext &sc) override;
 
   lldb_private::Type *ResolveTypeUID(lldb::user_id_t type_uid) override;
   llvm::Optional<ArrayInfo> GetDynamicArrayInfoForUID(
       lldb::user_id_t type_uid,
       const lldb_private::ExecutionContext *exe_ctx) override;

lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

if (function_die) {
       ParseFunctionBlocks(sc, &sc.function->GetBlock(false), function_die,
                           LLDB_INVALID_ADDRESS, 0);
     }
   }
 
   return functions_added;
 }
 
-size_t SymbolFileDWARF::ParseTypes(const SymbolContext &sc) {
+size_t SymbolFileDWARF::ParseTypesForCompileUnit(CompileUnit &comp_unit) {
   ASSERT_MODULE_LOCK(this);
-  // At least a compile unit must be valid
-  assert(sc.comp_unit);
   size_t types_added = 0;
-  DWARFUnit *dwarf_cu = GetDWARFCompileUnit(sc.comp_unit);
+  DWARFUnit *dwarf_cu = GetDWARFCompileUnit(&comp_unit);
   if (dwarf_cu) {
-    if (sc.function) {
-      dw_offset_t function_die_offset = sc.function->GetID();
-      DWARFDIE func_die = dwarf_cu->GetDIE(function_die_offset);
-      if (func_die && func_die.HasChildren()) {
-        types_added = ParseTypes(sc, func_die.GetFirstChild(), true, true);
-      }
-    } else {
-      DWARFDIE dwarf_cu_die = dwarf_cu->DIE();
-      if (dwarf_cu_die && dwarf_cu_die.HasChildren()) {
-        types_added = ParseTypes(sc, dwarf_cu_die.GetFirstChild(), true, true);
-      }
-    }
-  }
+      SymbolContext sc;
+      sc.comp_unit = &comp_unit;
 
   return types_added;
 }
 
 size_t SymbolFileDWARF::ParseVariablesForContext(const SymbolContext &sc) {
   ASSERT_MODULE_LOCK(this);
   if (sc.comp_unit != NULL) {
     DWARFDebugInfo *info = DebugInfo();

lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h

bool ParseCompileUnitSupportFiles(
       const lldb_private::SymbolContext &sc,
       lldb_private::FileSpecList &support_files) override;
   bool
   ParseCompileUnitIsOptimized(const lldb_private::SymbolContext &sc) override;
   bool ParseImportedModules(
       const lldb_private::SymbolContext &sc,
       std::vector<lldb_private::ConstString> &imported_modules) override;
   size_t ParseFunctionBlocks(const lldb_private::SymbolContext &sc) override;
-  size_t ParseTypes(const lldb_private::SymbolContext &sc) override;
+  size_t
+  ParseTypesForCompileUnit(lldb_private::CompileUnit &comp_unit) override;
   size_t
   ParseVariablesForContext(const lldb_private::SymbolContext &sc) override;
 
   lldb_private::Type *ResolveTypeUID(lldb::user_id_t type_uid) override;
   llvm::Optional<ArrayInfo> GetDynamicArrayInfoForUID(
       lldb::user_id_t type_uid,
       const lldb_private::ExecutionContext *exe_ctx) override;
 

lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp

 
 size_t SymbolFileDWARFDebugMap::ParseFunctionBlocks(const SymbolContext &sc) {
   SymbolFileDWARF *oso_dwarf = GetSymbolFile(sc);
   if (oso_dwarf)
     return oso_dwarf->ParseFunctionBlocks(sc);
   return 0;
 }
 
-size_t SymbolFileDWARFDebugMap::ParseTypes(const SymbolContext &sc) {
+size_t
+SymbolFileDWARFDebugMap::ParseTypesForCompileUnit(CompileUnit &comp_unit) {
+  SymbolContext sc;
+  sc.comp_unit = &comp_unit;
   SymbolFileDWARF *oso_dwarf = GetSymbolFile(sc);
   if (oso_dwarf)
-    return oso_dwarf->ParseTypes(sc);
+    return oso_dwarf->ParseTypesForCompileUnit(comp_unit);
   return 0;
 }
 
 size_t
 SymbolFileDWARFDebugMap::ParseVariablesForContext(const SymbolContext &sc) {
   SymbolFileDWARF *oso_dwarf = GetSymbolFile(sc);
   if (oso_dwarf)
     return oso_dwarf->ParseVariablesForContext(sc);

lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h

public:
 
   size_t ParseFunctionBlocks(const SymbolContext &sc) override;
 
   uint32_t FindGlobalVariables(const ConstString &name,
                                const CompilerDeclContext *parent_decl_ctx,
                                uint32_t max_matches,
                                VariableList &variables) override;
 
-  size_t ParseTypes(const SymbolContext &sc) override;
+  size_t
+  ParseTypesForCompileUnit(lldb_private::CompileUnit &comp_unit) override;
   size_t ParseVariablesForContext(const SymbolContext &sc) override;
 
   void AddSymbols(Symtab &symtab) override;
 
   CompilerDecl GetDeclForUID(lldb::user_id_t uid) override;
   CompilerDeclContext GetDeclContextForUID(lldb::user_id_t uid) override;
   CompilerDeclContext GetDeclContextContainingUID(lldb::user_id_t uid) override;
   Type *ResolveTypeUID(lldb::user_id_t type_uid) override;

lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp

if (!type)
       continue;
 
     types.Insert(type);
     ++match_count;
   }
   return match_count;
 }
 
-size_t SymbolFileNativePDB::ParseTypes(const SymbolContext &sc) {
+size_t SymbolFileNativePDB::ParseTypesForCompileUnit(CompileUnit &comp_unit) {
   // Only do the full type scan the first time.
   if (m_done_full_type_scan)
     return 0;
 
   size_t old_count = m_obj_file->GetModule()->GetTypeList()->GetSize();
   LazyRandomTypeCollection &types = m_index->tpi().typeCollection();
 
   // First process the entire TPI stream.

lldb/trunk/source/Plugins/SymbolFile/PDB/SymbolFilePDB.h

bool ParseCompileUnitSupportFiles(
       lldb_private::FileSpecList &support_files) override;
 
   bool ParseImportedModules(
       const lldb_private::SymbolContext &sc,
       std::vector<lldb_private::ConstString> &imported_modules) override;
 
   size_t ParseFunctionBlocks(const lldb_private::SymbolContext &sc) override;
 
-  size_t ParseTypes(const lldb_private::SymbolContext &sc) override;
+  size_t
+  ParseTypesForCompileUnit(lldb_private::CompileUnit &comp_unit) override;
 
   size_t
   ParseVariablesForContext(const lldb_private::SymbolContext &sc) override;
 
   lldb_private::Type *ResolveTypeUID(lldb::user_id_t type_uid) override;
   llvm::Optional<ArrayInfo> GetDynamicArrayInfoForUID(
       lldb::user_id_t type_uid,
       const lldb_private::ExecutionContext *exe_ctx) override;

lldb/trunk/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp

if (!pdb_func_up)
     return 0;
   Block &parent_block = sc.function->GetBlock(false);
   num_added =
       ParseFunctionBlocksForPDBSymbol(sc, pdb_func_up->getVirtualAddress(),
                                       pdb_func_up.get(), &parent_block, true);
   return num_added;
 }
 
-size_t SymbolFilePDB::ParseTypes(const lldb_private::SymbolContext &sc) {
-  lldbassert(sc.module_sp.get());
-  if (!sc.comp_unit)
-    return 0;
+size_t SymbolFilePDB::ParseTypesForCompileUnit(CompileUnit &comp_unit) {
 
   size_t num_added = 0;
-  auto compiland = GetPDBCompilandByUID(sc.comp_unit->GetID());
+  auto compiland = GetPDBCompilandByUID(comp_unit.GetID());
   if (!compiland)
     return 0;
 
   auto ParseTypesByTagFn = [&num_added, this](const PDBSymbol &raw_sym) {
     std::unique_ptr<IPDBEnumSymbols> results;
     PDB_SymType tags_to_search[] = {PDB_SymType::Enum, PDB_SymType::Typedef,
                                     PDB_SymType::UDT};
     for (auto tag : tags_to_search) {
for (auto tag : tags_to_search) {
           // during a TypeList dumping
           type->GetFullCompilerType();
           ++num_added;
         }
       }
     }
   };
 
-  if (sc.function) {
-    auto pdb_func = m_session_up->getConcreteSymbolById<PDBSymbolFunc>(
-        sc.function->GetID());
-    if (!pdb_func)
-      return 0;
-    ParseTypesByTagFn(*pdb_func);
-  } else {
-    ParseTypesByTagFn(*compiland);
 
-    // Also parse global types particularly coming from this compiland.
-    // Unfortunately, PDB has no compiland information for each global type. We
-    // have to parse them all. But ensure we only do this once.
-    static bool parse_all_global_types = false;
-    if (!parse_all_global_types) {
-      ParseTypesByTagFn(*m_global_scope_up);
-      parse_all_global_types = true;
-    }
-  }
   return num_added;
 }
 
 size_t
 SymbolFilePDB::ParseVariablesForContext(const lldb_private::SymbolContext &sc) {
   if (!sc.comp_unit)
     return 0;
 

lldb/trunk/source/Plugins/SymbolFile/Symtab/SymbolFileSymtab.h

bool ParseCompileUnitSupportFiles(
       lldb_private::FileSpecList &support_files) override;
 
   bool ParseImportedModules(
       const lldb_private::SymbolContext &sc,
       std::vector<lldb_private::ConstString> &imported_modules) override;
 
   size_t ParseFunctionBlocks(const lldb_private::SymbolContext &sc) override;
 
-  size_t ParseTypes(const lldb_private::SymbolContext &sc) override;
+  size_t
+  ParseTypesForCompileUnit(lldb_private::CompileUnit &comp_unit) override;
 
   size_t
   ParseVariablesForContext(const lldb_private::SymbolContext &sc) override;
 
   lldb_private::Type *ResolveTypeUID(lldb::user_id_t type_uid) override;
   llvm::Optional<ArrayInfo> GetDynamicArrayInfoForUID(
       lldb::user_id_t type_uid,
       const lldb_private::ExecutionContext *exe_ctx) override;

lldb/trunk/source/Plugins/SymbolFile/Symtab/SymbolFileSymtab.cpp

bool SymbolFileSymtab::ParseImportedModules(
     const SymbolContext &sc, std::vector<ConstString> &imported_modules) {
   return false;
 }
 
 size_t SymbolFileSymtab::ParseFunctionBlocks(const SymbolContext &sc) {
   return 0;
 }
 
-size_t SymbolFileSymtab::ParseTypes(const SymbolContext &sc) { return 0; }
+size_t SymbolFileSymtab::ParseTypesForCompileUnit(CompileUnit &comp_unit) {
+  return 0;
+}
 
 size_t SymbolFileSymtab::ParseVariablesForContext(const SymbolContext &sc) {
   return 0;
 }
 
 Type *SymbolFileSymtab::ResolveTypeUID(lldb::user_id_t type_uid) {
   return NULL;
 }

lldb/trunk/source/Symbol/SymbolVendor.cpp

size_t SymbolVendor::ParseFunctionBlocks(const SymbolContext &sc) {
   if (module_sp) {
     std::lock_guard<std::recursive_mutex> guard(module_sp->GetMutex());
     if (m_sym_file_ap.get())
       return m_sym_file_ap->ParseFunctionBlocks(sc);
   }
   return 0;
 }
 
-size_t SymbolVendor::ParseTypes(const SymbolContext &sc) {
+size_t SymbolVendor::ParseTypesForCompileUnit(CompileUnit &comp_unit) {
   ModuleSP module_sp(GetModule());
   if (module_sp) {
     std::lock_guard<std::recursive_mutex> guard(module_sp->GetMutex());
     if (m_sym_file_ap.get())
-      return m_sym_file_ap->ParseTypes(sc);
+      return m_sym_file_ap->ParseTypesForCompileUnit(comp_unit);
   }
   return 0;
 }
 
 size_t SymbolVendor::ParseVariablesForContext(const SymbolContext &sc) {
   ModuleSP module_sp(GetModule());
   if (module_sp) {
     std::lock_guard<std::recursive_mutex> guard(module_sp->GetMutex());