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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits