[NativePDB] Extend .pdb files search folders

[zloyrobot]

Summary

This patch adds ability to find .pdb files in NT_SYMBOL_PATH folders and in .exe file folder

[stella.stamenova]

lldb/lit/SymbolFile/NativePDB/pdb-file-lookup.cpp

4

Is this not testing finding the PDB file in the _NT_SYMBOL_PATH?

[zloyrobot]

lldb/lit/SymbolFile/NativePDB/pdb-file-lookup.cpp

4

Thanks for feedback, this description is wrong and test checks that we can find PDB file in the _NT_SYMBOL_PATH and doesn't check PDB file in folder containing EXE file.

I'll update test and make it test both cases.

[zloyrobot]

Add test case for searching .pdb file in the same folder as .exe file

[amccarth]

cc: Pavel Labath because I know he's been involved in conversations about how to find symbol files in general (PDB, split DWARF, Breakpad, etc.), especially when doing post-mortem debugging.

I believe there are complications when the debugger host is different than the target host, (e.g., post-mortem debugging a Windows minidump using LLDB on Linux).

I'm not concerned about the details of this patch but whether this approach is going to be compatible with the more general solution being discussed. This is small enough that it's probably not a big deal if it's helpful for now, but I'm going to hold my LGTM until Pavel has a chance to weigh in.

[labath]

The problem I have is that PDB is not following the lldb convention of doing the symbol file search inside a SymbolVendor class (which should really be called a SymbolFinder), but instead they implement their own little symbol vendor privately. There are reasons which make switching that not trivial, but they're not important in the context of this patch, which seems fine to me, as any reasonable future symbol vendor should also support the standard windows search mechanism. So overall, I do not see a reason to not accept this patch.

lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp

110

I find the interface of this function odd. First, the consts on the StringRef argument are useless and should be removed. Secondly, the by-ref return of the magic argument is something that would be nice to avoid. It looks like that could easily be done here by just checking whether the file exists and doing the identify_magic check in the caller (if you want an existing-but-not-pdb file to abort the search), or by checking the signature in this function (if you want to skip past non-pdb files). Also, this patch could use clang-formatting as this line is over the column limit.

111

Llvm policy is to use auto "if and only if it makes the code more readable" http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable. Whether that's the case here is somewhat subjective, but I'd say that none of the uses of auto in this patch are helping readability, as all the types used in this patch are short enough and spelling them out saves the reader from guessing whether ec really is a std::error_code, etc.

[amccarth]

Thanks Pavel!

Please address Pavel's inline comments, and I'll accept this.

[zloyrobot]

lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp

110

I want to skip past non-pdb files. Am I understand correctly that you suggest me to get rid of file_magic parameter and call identify_magic (open and read pdb file) additional time (in caller)?

111

Please note that I moved `auto ec = ...` from original Turner's code

[labath]

lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp

110

In that case, what you could do is get rid of the magic as an argument, and change this function to return a valid result, only if you found the file *and* that files magic number is correct. (note that this is not what the current implementation does)

111

yeah, I know, but that doesn't make it right. :) In fact, based on a random sample, I would guess that about 90% of uses of auto ec in llvm is @zturner's code. :P

TBH, the ec is no the part I'm having problems with the most. If it was just that, I wouldn't mention it, but the fact that you're using it all over the place tells me you weren't aware of llvm's policy regarding auto (which explicitly states "don't 'almost always use auto'"), and I figured it's best to mention that early on.

[zloyrobot]

lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp

110

got it, thanks

111

Thanks for the clarifications, I updated the path

[zloyrobot]

In D60962#1477160, @amccarth wrote:

Thanks Pavel!

Please address Pavel's inline comments, and I'll accept this.

Kind reminder

[labath]

Adrian is on vacation now, but given that he was just waiting until you resolve my comments (which you have), I think we don't have to wait for him.

[zloyrobot]

In D60962#1504921, @labath wrote:

Adrian is on vacation now, but given that he was just waiting until you resolve my comments (which you have), I think we don't have to wait for him.

Thank you for one more review.

[asmith]

Curious what the status of this is? Looks like its been ready for almost one year :)

lldb/lit/SymbolFile/NativePDB/Inputs/pdb-file-lookup.lldbinit

+source list -n main
+quit

lldb/lit/SymbolFile/NativePDB/pdb-file-lookup.cpp

+// clang-format off
+// REQUIRES: lld
+
+// Test that we can find .pdb file in folder containing .exe file.
+// RUN: %build --compiler=clang-cl --nodefaultlib -o %t.exe -- %s 
+// RUN: mkdir -p %t
+// RUN: mv %t.pdb %t/pdb-file-lookup.cpp.tmp.pdb
+// RUN: env _NT_SYMBOL_PATH=%t LLDB_USE_NATIVE_PDB_READER=1 %lldb -f %t.exe -s \
+// RUN:     %p/Inputs/pdb-file-lookup.lldbinit | FileCheck %s
+
+
+int main(int argc, char **argv) {
+  // Here are some comments.
+  return 0;
+}
+
+// CHECK: (lldb) source list -n main
+// CHECK: File: {{.*}}pdb-file-lookup.cpp
+// CHECK:    11
+// CHECK:    12   int main(int argc, char **argv) {
+// CHECK:    13     // Here are some comments.
+// CHECK:    14     return 0;
+// CHECK:    15   }
+// CHECK:    16

lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp

static std::unique_ptr<PDBFile> loadPDBFile(std::string PdbPath,
   if (auto EC = File->parseStreamData()) {
     llvm::consumeError(std::move(EC));
     return nullptr;
   }
 
   return File;
 }
 
+
+static std::string findPdbFile(const llvm::StringRef exe_path, const llvm::StringRef pdb_file, llvm::file_magic &magic) {
+  auto ec = llvm::identify_magic(pdb_file, magic);
+  if (!ec)
+    return pdb_file;
+
+  auto pdb_file_name = llvm::sys::path::filename(pdb_file);
+
+  llvm::SmallString<128> path = exe_path;
+  llvm::sys::path::remove_filename(path);
+  llvm::sys::path::append(path, pdb_file_name);
+
+  ec = llvm::identify_magic(path, magic);
+  if (!ec)
+    return path.str();
+
+  llvm::StringRef nt_symbol_path = ::getenv("_NT_SYMBOL_PATH");
+  llvm::SmallVector<llvm::StringRef, 16> parts;
+  nt_symbol_path.split(parts, ';', -1, false);
+
+  for (auto part_ref : parts)  {
+    if (part_ref.startswith_lower("srv*")) {
+      //Symbol servers is not supported yet
+      continue;
+    }
+
+    path = part_ref;
+    llvm::sys::path::append(path, pdb_file_name);
+
+    ec = llvm::identify_magic(path, magic);
+    if (!ec) {
+      return path.str();
+    }        
+  }
+
+  return {};
+}
 static std::unique_ptr<PDBFile>
 loadMatchingPDBFile(std::string exe_path, llvm::BumpPtrAllocator &allocator) {
   // Try to find a matching PDB for an EXE.
   using namespace llvm::object;
   auto expected_binary = createBinary(exe_path);
 
   // If the file isn't a PE/COFF executable, fail.
   if (!expected_binary) {
     llvm::consumeError(expected_binary.takeError());
     return nullptr;
   }
   OwningBinary<Binary> binary = std::move(*expected_binary);
 
   auto *obj = llvm::dyn_cast<llvm::object::COFFObjectFile>(binary.getBinary());
   if (!obj)
     return nullptr;
   const llvm::codeview::DebugInfo *pdb_info = nullptr;
 
   // If it doesn't have a debug directory, fail.
   llvm::StringRef pdb_file;
   auto ec = obj->getDebugPDBInfo(pdb_info, pdb_file);
   if (ec)
     return nullptr;
 
+  llvm::file_magic magic;
+  auto pdb_full_path = findPdbFile(exe_path, pdb_file, magic);
+
   // if the file doesn't exist, is not a pdb, or doesn't have a matching guid,
   // fail.
-  llvm::file_magic magic;
-  ec = llvm::identify_magic(pdb_file, magic);
-  if (ec || magic != llvm::file_magic::pdb)
+  if (pdb_full_path.empty() || magic != llvm::file_magic::pdb)
     return nullptr;
-  std::unique_ptr<PDBFile> pdb = loadPDBFile(pdb_file, allocator);
+  std::unique_ptr<PDBFile> pdb = loadPDBFile(pdb_full_path, allocator);
   if (!pdb)
     return nullptr;
 
   auto expected_info = pdb->getPDBInfoStream();
   if (!expected_info) {
     llvm::consumeError(expected_info.takeError());
     return nullptr;
   }