[Lldb-commits] [PATCH] D17363: Add SymbolFilePDB with basic support for line tables.

[Zachary Turner via lldb-commits]

zturner added inline comments.

================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:98-100
@@ +97,5 @@
+{
+    auto global = m_session->getGlobalScope();
+    auto compilands = global->findAllChildren<llvm::PDBSymbolCompiland>();
+    return compilands->getChildCount();
+}
----------------
clayborg wrote:
> Do you want to cache the compile unit count, or is this really cheap to call over and over?
Most of these methods are really cheap.  In fact, the entire process of caching stuff in `SymbolContexts` and whatnot is probably unnecessary for us in theory because while it may be cheaper performance wise to cache it the way we're doing, it's not *that* expensive to just query the file, and when you consider the memory usage of caching it a second time, it's not clear it's a good tradeoff.

That said, there's not really a way around this with the current architecture, so I only cache where necessary (can always add more later if performance becomes an issue)

================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:119
@@ +118,3 @@
+    // Default to C++, currently DebugInfoPDB does not return the language.
+    return lldb::eLanguageTypeC_plus_plus;
+}
----------------
clayborg wrote:
> It will be interesting to see how you solve this one if there is no language in the compile unit. I would be shame to have to iterate through all functions and check for C++... Or look for classes or other things that are defined in the current compile unit. 
Yea, my comment was wrong.  I can fix this, I just didn't realize it at the time.

================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:256-257
@@ +255,4 @@
+        // Locate all compilation units for the given source file.
+        auto compilands =
+            m_session->findCompilandsForSourceFile(file_spec.GetPath(), llvm::PDB_NameSearchFlags::NS_CaseInsensitive);
+
----------------
clayborg wrote:
> You need to still look through all line tables if "check_inlines" is true. Someone might ask for "vector" as the FileSpec (no directory) and 12 for the line table. What we do in DWARF is grab all compile units, get the "LineTable*" from each compile unit, and look to see if "file_spec" is in the support files by asking the support files to find the index for "file_spec" and if the file index is not UINT32_MAX, then we iterate through all line table entries and extract the entries that match. See the SymbolFileDWARF::ResolveSymbolContext() for more details.
So does `check_inlines` really mean `check_header_files`?  I kind of ignored the argument because it wasn't obvious to me what it did based on the name.

What is supposed to happen if someone specifies `vector` as the FileSpec, but vector is #included from 10 different compile units?  Will this return line information from all 10 of those compile units?

I'm not using support files for anything right now because I can't figure out if it makes sense or if we need it.  The PDB tells us, for each compile unit, the list of all source files that contribute to that compile unit.  It sounds like that's what you're using support files for, but in my case I'm comfortable (at least for now) simply querying the PDB every time.

Just to make sure I understand, is it safe to say that:

1. If `check_inlines` is false, `sc_list` should return with exactly 1 `SymbolContext` with `m_comp_unit` set to the main source file?
2. The same for the `check_inlines` is false calse -- we still just have ` SymbolContext` but we add line table entries from all additional files that contribute to the compilation unit?

When will `sc_list`  end up with multiple entries?


http://reviews.llvm.org/D17363