zturner added a comment. Since it seems like you're going to be doing some work in here, it would be great if you could update `lldb-test` to dump PDB symbol information. This would allow us to easily test all sorts of things in here. For example, you could find a PDB that returned an empty source file name, and then have the tool dump something like:
Compiland: {foo}, Source File: {bar} then we could just grep this output against FileCheck. ================ Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:367-370 + // For some reason, source file name could be empty, so we will walk through + // all source files of this compiland, and determine the right source file + // if any that is used to generate this compiland based on language + // indicated in compilanddetails language field. ---------------- Is it DIA that is returning the empty name? ================ Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:371 + // indicated in compilanddetails language field. + if (source_file_name.empty()) { + auto details_up = pdb_compiland->findOneChild<PDBSymbolCompilandDetails>(); ---------------- Can you do an early return here? ================ Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:382-389 + (ConstString::Compare(file_extension, ConstString("CPP"), + false) == 0 || + ConstString::Compare(file_extension, ConstString("C"), + false) == 0 || + ConstString::Compare(file_extension, ConstString("CC"), + false) == 0 || + ConstString::Compare(file_extension, ConstString("CXX"), ---------------- This would probably be shorter as: ``` llvm::is_contained({"cpp", "c", "cc", "cxx"}, file_extension.GetStringRef().lower()); ``` ================ Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:683 + + if (cu_sp) { + m_comp_units.insert(std::make_pair(id, cu_sp)); ---------------- Early return here. ================ Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:763 auto prologue = func->findOneChild<PDBSymbolFuncDebugStart>(); - is_prologue = (addr == prologue->getVirtualAddress()); + if (prologue.get()) + is_prologue = (addr == prologue->getVirtualAddress()); ---------------- nit: `.get()` shouldn't be necessary ================ Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:767 auto epilogue = func->findOneChild<PDBSymbolFuncDebugEnd>(); - is_epilogue = (addr == epilogue->getVirtualAddress()); + if (epilogue.get()) + is_epilogue = (addr == epilogue->getVirtualAddress()); ---------------- Same here. ================ Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.h:197 std::unique_ptr<llvm::pdb::IPDBSession> m_session_up; + std::unique_ptr<llvm::pdb::PDBSymbolExe> m_global_scope_up; uint32_t m_cached_compile_unit_count; ---------------- Is this a performance win or just a convenience? I assumed the session itself would cache the global scope, but maybe that assumption was wrong. Repository: rL LLVM https://reviews.llvm.org/D41428 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits