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

[Greg Clayton via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

So the main issue is to fix the:

  uint32_t
  SymbolFilePDB::ResolveSymbolContext(const lldb_private::FileSpec &file_spec, uint32_t line, bool check_inlines,
                                      uint32_t resolve_scope, lldb_private::SymbolContextList &sc_list)

So that it searches all compile units when "check_inlines" is true. See inlined comments.

Feel free to add default implementations of any SymbolFile functions that you overrode that aren't doing anything. Especially for things you don't plan to implement. SymbolFileSymtab can them rely on these new default implementations.


================
Comment at: source/Initialization/SystemInitializerCommon.cpp:97-98
@@ -95,2 +96,4 @@
     }
+
+    ::CoInitializeEx(nullptr, COINIT_MULTITHREADED);
 #endif
----------------
Should we make a SBHostOS::Initialize() and move this code and the windows specific #include statements into that file? Not sure how ```#if defined()``` up this file is, but it is something to think about.

================
Comment at: source/Initialization/SystemInitializerCommon.cpp:202-204
@@ -198,1 +201,5 @@
+
+#if defined(_MSC_VER)
+    ::CoUninitialize();
+#endif
 }
----------------
Call Host::Terminate() and move ::CoUninitialize(); over into the windows specific Host::Terminate()?

================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:82-83
@@ +81,4 @@
+        auto error = llvm::loadDataForEXE(llvm::PDB_ReaderType::DIA, llvm::StringRef(exePath), m_session);
+        if (error != llvm::PDB_ErrorCode::Success)
+            return 0;
+    }
----------------
how about reversing this just to be safe:

```
if (error == llvm::PDB_ErrorCode::Success)
    return CompileUnits | LineTables;
```

================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:82-84
@@ +81,5 @@
+        auto error = llvm::loadDataForEXE(llvm::PDB_ReaderType::DIA, llvm::StringRef(exePath), m_session);
+        if (error != llvm::PDB_ErrorCode::Success)
+            return 0;
+    }
+    return CompileUnits | LineTables;
----------------
clayborg wrote:
> how about reversing this just to be safe:
> 
> ```
> if (error == llvm::PDB_ErrorCode::Success)
>     return CompileUnits | LineTables;
> ```
how about reversing this just to be safe:

```
if (error == llvm::PDB_ErrorCode::Success)
    return CompileUnits | LineTables;
```

Do you also need to clear m_session in the else clause?

================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:85
@@ +84,3 @@
+    }
+    return CompileUnits | LineTables;
+}
----------------
If you reverse logic above, then this becomes:

```
return 0;
```

================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:92
@@ +91,3 @@
+    lldb::addr_t obj_load_address = m_obj_file->GetFileOffset();
+    m_session->setLoadAddress(obj_load_address);
+}
----------------
Do you need to check m_session?

================
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();
+}
----------------
Do you want to cache the compile unit count, or is this really cheap to call over and over?

================
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;
+}
----------------
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. 

================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:146
@@ +145,3 @@
+
+        uint32_t va = line->getVirtualAddress();
+        uint32_t lno = line->getLineNumber();
----------------
Is this really a 32 bit value for 32 and 64 bit binaries?

================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:162-236
@@ +161,77 @@
+
+bool
+SymbolFilePDB::ParseCompileUnitDebugMacros(const lldb_private::SymbolContext &sc)
+{
+    // PDB doesn't contain information about macros
+    return false;
+}
+
+bool
+SymbolFilePDB::ParseCompileUnitSupportFiles(const lldb_private::SymbolContext &sc,
+                                            lldb_private::FileSpecList &support_files)
+{
+    // TODO: What are support files?
+    return false;
+}
+
+bool
+SymbolFilePDB::ParseImportedModules(const lldb_private::SymbolContext &sc,
+                                    std::vector<lldb_private::ConstString> &imported_modules)
+{
+    // PDB does not yet support module debug info
+    return false;
+}
+
+size_t
+SymbolFilePDB::ParseFunctionBlocks(const lldb_private::SymbolContext &sc)
+{
+    // TODO: Implement this
+    return size_t();
+}
+
+size_t
+SymbolFilePDB::ParseTypes(const lldb_private::SymbolContext &sc)
+{
+    // TODO: Implement this
+    return size_t();
+}
+
+size_t
+SymbolFilePDB::ParseVariablesForContext(const lldb_private::SymbolContext &sc)
+{
+    // TODO: Implement this
+    return size_t();
+}
+
+lldb_private::Type *
+SymbolFilePDB::ResolveTypeUID(lldb::user_id_t type_uid)
+{
+    return nullptr;
+}
+
+bool
+SymbolFilePDB::CompleteType(lldb_private::CompilerType &compiler_type)
+{
+    // TODO: Implement this
+    return false;
+}
+
+lldb_private::CompilerDecl
+SymbolFilePDB::GetDeclForUID(lldb::user_id_t uid)
+{
+    return lldb_private::CompilerDecl();
+}
+
+lldb_private::CompilerDeclContext
+SymbolFilePDB::GetDeclContextForUID(lldb::user_id_t uid)
+{
+    return lldb_private::CompilerDeclContext();
+}
+
+lldb_private::CompilerDeclContext
+SymbolFilePDB::GetDeclContextContainingUID(lldb::user_id_t uid)
+{
+    return lldb_private::CompilerDeclContext();
+}
+
+void
----------------
Feel free to add default implementations to the base SymbolFile class if you don't plan to add your own versions of any functions. We have this same kind of code over in SymbolFileSymtab and I would love to get rid of 100 different "return invalid value" functions.

================
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);
+
----------------
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.

================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.h:182
@@ +181,3 @@
+
+    std::unique_ptr<llvm::IPDBSession> m_session;
+};
----------------
Please rename to "m_session_up" to indicate it is a unique_ptr.


http://reviews.llvm.org/D17363