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

[Zachary Turner via lldb-commits]

zturner added a comment.

Added a few notes to clarify things that might not be obvious upon a first look at the patch.


================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:81
@@ +80,3 @@
+        std::string exePath = m_obj_file->GetFileSpec().GetPath();
+        auto error = llvm::loadDataForEXE(llvm::PDB_ReaderType::DIA, llvm::StringRef(exePath), m_session);
+        if (error != llvm::PDB_ErrorCode::Success)
----------------
Note that this entire plugin is supposed to compile and be registered on every platform, including platforms that don't support PDB (i.e. which, for now, is all non-Windows platforms).  But this function will simply return an error in those case, and return `0` for the abilities.

Later, if and when PDB reading support is implemented in such a way as to not require Windows, we need only change the first argument and to `loadDataForExe` and we will have PDB support on non windows platforms with no other changes required.

================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:118-119
@@ +117,4 @@
+{
+    // Default to C++, currently DebugInfoPDB does not return the language.
+    return lldb::eLanguageTypeC_plus_plus;
+}
----------------
This is a mistake, it does contain the language.  I will need to implement this, although for all intents and purposes it doesn't matter because it's C++ in all cases we care about anyway.

================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:374-375
@@ +373,4 @@
+
+    // This frequently (i.e. in all cases I've seen) returns null.  Hopefully
+    // LLDB can handle null values for this.
+    FileSpec path(cu->getSourceFileName(), false);
----------------
This comment is no longer valid.  It returns the source file name.  Pretend you didn't see this.

================
Comment at: unittests/SymbolFile/PDB/Inputs/test-dwarf.cpp:1-2
@@ +1,3 @@
+// Compile with "cl /c /Zi /GR- test.cpp"
+// Link with "link test.obj /debug /nodefaultlib /entry:main /out:test.exe"
+
----------------
Need to fix this comment to include the clang++ command line instead of the cl command line.

================
Comment at: unittests/SymbolFile/PDB/SymbolFilePDBTests.cpp:94
@@ +93,3 @@
+#else
+#define REQUIRES_DIA_SDK(TestName) DISABLED_##TestName
+#endif
----------------
This will cause the 2 PDB-specific tests to run only on Windows.


http://reviews.llvm.org/D17363