[Lldb-commits] [PATCH] D41428: [lldb] This commit adds support to cache a PDB's global scope and fixes a bug in getting the source file name for a compiland

[Zachary Turner via Phabricator via lldb-commits]

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