[PATCH] D53002: Create a new symbol file plugin for cross-platform PDB symbolization

[Pavel Labath via Phabricator via llvm-commits]

labath added a comment.

I can't say I have done a particularly thorough review, but code seems well structured, and it looks like I could make sense of it if I went through the effort of figuring out what various pdb record types mean. These are the things that came to mind while looking at this.

Also, hooray for being able to open pdb files on non-windows.



================
Comment at: lldb/lit/SymbolFile/NativePDB/simple-breakpoints.cpp:1
+// clang-format off
+
----------------
It seems llvm has a special .clang-format file in the test folder which sets `ColumnLimit: 0`. Maybe we should do the same?


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp:37-38
+  if (!ExpectedDBI) {
+    llvm::consumeError(ExpectedDBI.takeError());
+    return nullptr;
+  }
----------------
Maybe propagate this error to the caller and have the caller log the error when ignoring it?


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:53
+
+#include <regex>
+
----------------
Is this actually used?


================
Comment at: lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:78-85
+static bool ShouldUseNativeReader() {
+#if !defined(_WIN32)
+  return true;
+#endif
+  llvm::StringRef use_native = ::getenv("LLDB_USE_NATIVE_PDB_READER");
+  return use_native.equals_lower("on") || use_native.equals_lower("yes") ||
+         use_native.equals_lower("1") || use_native.equals_lower("true");
----------------
I am wondering whether it wouldn't be better to use an lldb setting instead of an env var (plugin.symbol-file.pdb.native ?). For this to work, we'd need to delay to decision of which plugin to use to until `CreateInstance` time, but that shouldn't be too hard.


https://reviews.llvm.org/D53002