[Lldb-commits] [PATCH] D60962: [NativePDB] Extend .pdb files search folders

[Aaron Smith via Phabricator via lldb-commits]

asmith added a comment.
Herald added a subscriber: JDevlieghere.

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


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60962/new/

https://reviews.llvm.org/D60962



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D60962: [NativePDB] Extend .pdb files search folders

[Mikhail Senkov via Phabricator via lldb-commits]

zloyrobot added a comment.

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.


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60962/new/

https://reviews.llvm.org/D60962



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D60962: [NativePDB] Extend .pdb files search folders

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.

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.


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60962/new/

https://reviews.llvm.org/D60962



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D60962: [NativePDB] Extend .pdb files search folders

[Mikhail Senkov via Phabricator via lldb-commits]

zloyrobot added a comment.

In D60962#1477160 , @amccarth wrote:

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


Kind reminder


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60962/new/

https://reviews.llvm.org/D60962



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D60962: [NativePDB] Extend .pdb files search folders

[Mikhail Senkov via Phabricator via lldb-commits]

zloyrobot marked 2 inline comments as done.
zloyrobot added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:110
+
+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);

labath wrote:
> zloyrobot wrote:
> > labath wrote:
> > > I find the interface of this function odd. First, the `const`s 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.
> > 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)? 
> 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)
got it, thanks



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:111
+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)

labath wrote:
> zloyrobot wrote:
> > labath wrote:
> > > Llvm policy is to use `auto` "if and only if it makes the 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.
> > Please note that I moved  ```auto ec = ...```  from original Turner's code 
> 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. 
Thanks for the clarifications, I updated the path


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60962/new/

https://reviews.llvm.org/D60962



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D60962: [NativePDB] Extend .pdb files search folders

[Mikhail Senkov via Phabricator via lldb-commits]

zloyrobot updated this revision to Diff 196614.

Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60962/new/

https://reviews.llvm.org/D60962

Files:
  lldb/lit/SymbolFile/NativePDB/Inputs/pdb-file-lookup.lldbinit
  lldb/lit/SymbolFile/NativePDB/pdb-file-lookup.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp

Index: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
===
--- lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -106,6 +106,45 @@
   return File;
 }
 
+
+static std::string findPdbFile(llvm::StringRef exe_path, 
+   llvm::StringRef pdb_file) {
+  llvm::file_magic magic;
+  std::error_code ec = llvm::identify_magic(pdb_file, magic);
+  if (!ec && magic == llvm::file_magic::pdb)
+return pdb_file;
+
+  llvm::StringRef 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 && magic == llvm::file_magic::pdb)
+return path.str();
+
+  llvm::StringRef nt_symbol_path = ::getenv("_NT_SYMBOL_PATH");
+  llvm::SmallVector parts;
+  nt_symbol_path.split(parts, ';', -1, false);
+
+  for (llvm::StringRef 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 && magic == llvm::file_magic::pdb) {
+  return path.str();
+}
+  }
+
+  return {};
+}
 static std::unique_ptr
 loadMatchingPDBFile(std::string exe_path, llvm::BumpPtrAllocator &allocator) {
   // Try to find a matching PDB for an EXE.
@@ -130,13 +169,13 @@
   if (ec)
 return nullptr;
 
+  auto pdb_full_path = findPdbFile(exe_path, pdb_file);
+
   // 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())
 return nullptr;
-  std::unique_ptr pdb = loadPDBFile(pdb_file, allocator);
+  std::unique_ptr pdb = loadPDBFile(pdb_full_path, allocator);
   if (!pdb)
 return nullptr;
 
Index: lldb/lit/SymbolFile/NativePDB/pdb-file-lookup.cpp
===
--- /dev/null
+++ lldb/lit/SymbolFile/NativePDB/pdb-file-lookup.cpp
@@ -0,0 +1,31 @@
+// clang-format off
+// REQUIRES: lld
+
+// Test that we can find .pdb file in _NT_SYMBOL_PATH folder. 
+// 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
+//
+// Test that we can find .pdb file in folder containing .exe file.
+// RUN: mv %t.exe %t/pdb-file-lookup.cpp.tmp.exe
+// RUN: env LLDB_USE_NATIVE_PDB_READER=1 %lldb -f \
+// RUN: %t/pdb-file-lookup.cpp.tmp.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:18
+// CHECK:19   int main(int argc, char **argv) {
+// CHECK:20 // Here are some comments.
+// CHECK:21 return 0;
+// CHECK:22   }
+// CHECK:23
Index: lldb/lit/SymbolFile/NativePDB/Inputs/pdb-file-lookup.lldbinit
===
--- /dev/null
+++ lldb/lit/SymbolFile/NativePDB/Inputs/pdb-file-lookup.lldbinit
@@ -0,0 +1,2 @@
+source list -n main
+quit
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D60962: [NativePDB] Extend .pdb files search folders

[Pavel Labath via Phabricator via lldb-commits]

labath added a subscriber: zturner.
labath added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:110
+
+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);

zloyrobot wrote:
> labath wrote:
> > I find the interface of this function odd. First, the `const`s 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.
> 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)? 
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)



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:111
+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)

zloyrobot wrote:
> labath wrote:
> > Llvm policy is to use `auto` "if and only if it makes the 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.
> Please note that I moved  ```auto ec = ...```  from original Turner's code 
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. 


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60962/new/

https://reviews.llvm.org/D60962



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D60962: [NativePDB] Extend .pdb files search folders

[Mikhail Senkov via Phabricator via lldb-commits]

zloyrobot marked 2 inline comments as done.
zloyrobot added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:110
+
+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);

labath wrote:
> I find the interface of this function odd. First, the `const`s 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.
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)? 



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:111
+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)

labath wrote:
> Llvm policy is to use `auto` "if and only if it makes the 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.
Please note that I moved  ```auto ec = ...```  from original Turner's code 


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60962/new/

https://reviews.llvm.org/D60962



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D60962: [NativePDB] Extend .pdb files search folders

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

Thanks Pavel!

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


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60962/new/

https://reviews.llvm.org/D60962



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D60962: [NativePDB] Extend .pdb files search folders

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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.




Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:110
+
+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);

I find the interface of this function odd. First, the `const`s 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.



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:111
+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)

Llvm policy is to use `auto` "if and only if it makes the 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.


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60962/new/

https://reviews.llvm.org/D60962



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D60962: [NativePDB] Extend .pdb files search folders

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

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.


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60962/new/

https://reviews.llvm.org/D60962



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D60962: [NativePDB] Extend .pdb files search folders

[Mikhail Senkov via Phabricator via lldb-commits]

zloyrobot updated this revision to Diff 196232.

Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60962/new/

https://reviews.llvm.org/D60962

Files:
  lldb/lit/SymbolFile/NativePDB/Inputs/pdb-file-lookup.lldbinit
  lldb/lit/SymbolFile/NativePDB/pdb-file-lookup.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp

Index: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
===
--- lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -106,6 +106,43 @@
   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 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
 loadMatchingPDBFile(std::string exe_path, llvm::BumpPtrAllocator &allocator) {
   // Try to find a matching PDB for an EXE.
@@ -130,13 +167,14 @@
   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 pdb = loadPDBFile(pdb_file, allocator);
+  std::unique_ptr pdb = loadPDBFile(pdb_full_path, allocator);
   if (!pdb)
 return nullptr;
 
Index: lldb/lit/SymbolFile/NativePDB/pdb-file-lookup.cpp
===
--- /dev/null
+++ lldb/lit/SymbolFile/NativePDB/pdb-file-lookup.cpp
@@ -0,0 +1,31 @@
+// clang-format off
+// REQUIRES: lld
+
+// Test that we can find .pdb file in _NT_SYMBOL_PATH folder. 
+// 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
+//
+// Test that we can find .pdb file in folder containing .exe file.
+// RUN: mv %t.exe %t/pdb-file-lookup.cpp.tmp.exe
+// RUN: env LLDB_USE_NATIVE_PDB_READER=1 %lldb -f \
+// RUN: %t/pdb-file-lookup.cpp.tmp.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:18
+// CHECK:19   int main(int argc, char **argv) {
+// CHECK:20 // Here are some comments.
+// CHECK:21 return 0;
+// CHECK:22   }
+// CHECK:23
Index: lldb/lit/SymbolFile/NativePDB/Inputs/pdb-file-lookup.lldbinit
===
--- /dev/null
+++ lldb/lit/SymbolFile/NativePDB/Inputs/pdb-file-lookup.lldbinit
@@ -0,0 +1,2 @@
+source list -n main
+quit
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D60962: [NativePDB] Extend .pdb files search folders

[Mikhail Senkov via Phabricator via lldb-commits]

zloyrobot updated this revision to Diff 196226.
zloyrobot added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60962/new/

https://reviews.llvm.org/D60962

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


Index: lldb/lit/SymbolFile/NativePDB/pdb-file-lookup.cpp
===
--- /dev/null
+++ lldb/lit/SymbolFile/NativePDB/pdb-file-lookup.cpp
@@ -0,0 +1,31 @@
+// clang-format off
+// REQUIRES: lld
+
+// Test that we can find .pdb file in _NT_SYMBOL_PATH folder. 
+// 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
+//
+// Test that we can find .pdb file in folder containing .exe file.
+// RUN: mv %t.exe %t/pdb-file-lookup.cpp.tmp.exe
+// RUN: env LLDB_USE_NATIVE_PDB_READER=1 %lldb -f \
+// RUN: %t/pdb-file-lookup.cpp.tmp.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:18
+// CHECK:19   int main(int argc, char **argv) {
+// CHECK:20 // Here are some comments.
+// CHECK:21 return 0;
+// CHECK:22   }
+// CHECK:23


Index: lldb/lit/SymbolFile/NativePDB/pdb-file-lookup.cpp
===
--- /dev/null
+++ lldb/lit/SymbolFile/NativePDB/pdb-file-lookup.cpp
@@ -0,0 +1,31 @@
+// clang-format off
+// REQUIRES: lld
+
+// Test that we can find .pdb file in _NT_SYMBOL_PATH folder. 
+// 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
+//
+// Test that we can find .pdb file in folder containing .exe file.
+// RUN: mv %t.exe %t/pdb-file-lookup.cpp.tmp.exe
+// RUN: env LLDB_USE_NATIVE_PDB_READER=1 %lldb -f \
+// RUN: %t/pdb-file-lookup.cpp.tmp.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:18
+// CHECK:19   int main(int argc, char **argv) {
+// CHECK:20 // Here are some comments.
+// CHECK:21 return 0;
+// CHECK:22   }
+// CHECK:23
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D60962: [NativePDB] Extend .pdb files search folders

[Mikhail Senkov via Phabricator via lldb-commits]

zloyrobot added inline comments.



Comment at: lldb/lit/SymbolFile/NativePDB/pdb-file-lookup.cpp:4
+
+// Test that we can find .pdb file in folder containing .exe file.
+// RUN: %build --compiler=clang-cl --nodefaultlib -o %t.exe -- %s 

stella.stamenova wrote:
> Is this not testing finding the PDB file in the _NT_SYMBOL_PATH?
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.


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60962/new/

https://reviews.llvm.org/D60962



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D60962: [NativePDB] Extend .pdb files search folders

[Stella Stamenova via Phabricator via lldb-commits]

stella.stamenova added inline comments.



Comment at: lldb/lit/SymbolFile/NativePDB/pdb-file-lookup.cpp:4
+
+// Test that we can find .pdb file in folder containing .exe file.
+// RUN: %build --compiler=clang-cl --nodefaultlib -o %t.exe -- %s 

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


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60962/new/

https://reviews.llvm.org/D60962



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D60962: [NativePDB] Extend .pdb files search folders

[Mikhail Senkov via Phabricator via lldb-commits]

zloyrobot created this revision.
zloyrobot added reviewers: amccarth, asmith, stella.stamenova.
zloyrobot added a project: LLDB.
Herald added subscribers: lldb-commits, teemperor.

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


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D60962

Files:
  lldb/lit/SymbolFile/NativePDB/Inputs/pdb-file-lookup.lldbinit
  lldb/lit/SymbolFile/NativePDB/pdb-file-lookup.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp

Index: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
===
--- lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -106,6 +106,43 @@
   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 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
 loadMatchingPDBFile(std::string exe_path, llvm::BumpPtrAllocator &allocator) {
   // Try to find a matching PDB for an EXE.
@@ -130,13 +167,14 @@
   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 pdb = loadPDBFile(pdb_file, allocator);
+  std::unique_ptr pdb = loadPDBFile(pdb_full_path, allocator);
   if (!pdb)
 return nullptr;
 
Index: lldb/lit/SymbolFile/NativePDB/pdb-file-lookup.cpp
===
--- /dev/null
+++ lldb/lit/SymbolFile/NativePDB/pdb-file-lookup.cpp
@@ -0,0 +1,24 @@
+// 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
Index: lldb/lit/SymbolFile/NativePDB/Inputs/pdb-file-lookup.lldbinit
===
--- /dev/null
+++ lldb/lit/SymbolFile/NativePDB/Inputs/pdb-file-lookup.lldbinit
@@ -0,0 +1,2 @@
+source list -n main
+quit
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits