[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

[Lldb-commits] [PATCH] D60963: Fix dereferencing null pointer

[Mikhail Senkov via Phabricator via lldb-commits]

zloyrobot created this revision.
zloyrobot added reviewers: amccarth, thakis.
zloyrobot added a project: LLDB.
Herald added subscribers: llvm-commits, lldb-commits, erik.pilkington, 
hiraditya.
Herald added a project: LLVM.

All callers of Demangler::parseTagUniqueName check 'Demangler.Error' and assume 
that 'Error is false' means 'return value is not null'


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D60963

Files:
  llvm/lib/Demangle/MicrosoftDemangle.cpp


Index: llvm/lib/Demangle/MicrosoftDemangle.cpp
===
--- llvm/lib/Demangle/MicrosoftDemangle.cpp
+++ llvm/lib/Demangle/MicrosoftDemangle.cpp
@@ -735,11 +735,15 @@
 }
 
 TagTypeNode *Demangler::parseTagUniqueName(StringView &MangledName) {
-  if (!MangledName.consumeFront(".?A"))
+  if (!MangledName.consumeFront(".?A")) {
+Error = true;
 return nullptr;
+  }
   MangledName.consumeFront(".?A");
-  if (MangledName.empty())
+  if (MangledName.empty()) {
+Error = true;
 return nullptr;
+  }
 
   return demangleClassType(MangledName);
 }


Index: llvm/lib/Demangle/MicrosoftDemangle.cpp
===
--- llvm/lib/Demangle/MicrosoftDemangle.cpp
+++ llvm/lib/Demangle/MicrosoftDemangle.cpp
@@ -735,11 +735,15 @@
 }
 
 TagTypeNode *Demangler::parseTagUniqueName(StringView &MangledName) {
-  if (!MangledName.consumeFront(".?A"))
+  if (!MangledName.consumeFront(".?A")) {
+Error = true;
 return nullptr;
+  }
   MangledName.consumeFront(".?A");
-  if (MangledName.empty())
+  if (MangledName.empty()) {
+Error = true;
 return nullptr;
+  }
 
   return demangleClassType(MangledName);
 }
___
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

[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 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] D60963: Fix dereferencing null pointer

[Mikhail Senkov via Phabricator via lldb-commits]

zloyrobot added a comment.

In D60963#1474783 , @thakis wrote:

> test?


Would you please advise me on what kind of test I should add for such fix?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D60963



___
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

[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

[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] D61128: Fix stack unwinding for struct methods

[Mikhail Senkov via Phabricator via lldb-commits]

zloyrobot created this revision.
zloyrobot added reviewers: amccarth, aleksandr.urakov.
zloyrobot added a project: LLDB.
Herald added subscribers: lldb-commits, teemperor.

Repository:
  rLLDB LLDB

https://reviews.llvm.org/D61128

Files:
  lldb/lit/SymbolFile/NativePDB/Inputs/stack_unwinding01.lldbinit
  lldb/lit/SymbolFile/NativePDB/stack_unwinding01.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h

Index: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
===
--- lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
+++ lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
@@ -180,6 +180,9 @@
   lldb::TypeSP CreateArrayType(PdbTypeSymId type_id,
const llvm::codeview::ArrayRecord &ar,
CompilerType ct);
+  lldb::TypeSP CreateFunctionType(PdbTypeSymId type_id,
+  const llvm::codeview::MemberFunctionRecord &pr,
+  CompilerType ct);
   lldb::TypeSP CreateProcedureType(PdbTypeSymId type_id,
const llvm::codeview::ProcedureRecord &pr,
CompilerType ct);
Index: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
===
--- lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -595,6 +595,17 @@
   return array_sp;
 }
 
+
+TypeSP SymbolFileNativePDB::CreateFunctionType(PdbTypeSymId type_id,
+   const MemberFunctionRecord &mfr,
+   CompilerType ct) {
+  Declaration decl;
+  return std::make_shared(
+  toOpaqueUid(type_id), this, ConstString(), 0, nullptr, LLDB_INVALID_UID,
+  lldb_private::Type::eEncodingIsUID, decl, ct,
+  lldb_private::Type::eResolveStateFull);
+}
+
 TypeSP SymbolFileNativePDB::CreateProcedureType(PdbTypeSymId type_id,
 const ProcedureRecord &pr,
 CompilerType ct) {
@@ -655,6 +666,11 @@
 llvm::cantFail(TypeDeserializer::deserializeAs(cvt, pr));
 return CreateProcedureType(type_id, pr, ct);
   }
+  if (cvt.kind() == LF_MFUNCTION) {
+MemberFunctionRecord mfr;
+llvm::cantFail(TypeDeserializer::deserializeAs(cvt, mfr));
+return CreateFunctionType(type_id, mfr, ct);
+  }
 
   return nullptr;
 }
Index: lldb/lit/SymbolFile/NativePDB/stack_unwinding01.cpp
===
--- /dev/null
+++ lldb/lit/SymbolFile/NativePDB/stack_unwinding01.cpp
@@ -0,0 +1,48 @@
+// clang-format off
+// REQUIRES: lld
+
+// RUN: %build --compiler=clang-cl --nodefaultlib -o %t.exe -- %s
+// RUN: env LLDB_USE_NATIVE_PDB_READER=1 %lldb -f %t.exe -s \
+// RUN: %p/Inputs/stack_unwinding01.lldbinit 2>&1 | FileCheck %s
+
+
+struct Struct {
+  void simple_method(int a, int b) {
+ a += 1;
+ simple_method(a, b);
+  }
+};
+
+
+
+int main(int argc, char **argv) {
+  Struct s;
+  s.simple_method(1,2);
+  return 0;
+}
+
+
+// CHECK: (lldb) thread backtrace
+// CHECK-NEXT: * thread #1, stop reason = breakpoint 1.1
+// CHECK-NEXT:   * frame #0: {{.*}} stack_unwinding01.cpp.tmp.exe`Struct::simple_method at stack_unwinding01.cpp:12
+// CHECK-NEXT: frame #1: {{.*}} stack_unwinding01.cpp.tmp.exe`main(argc={{.*}}, argv={{.*}}) at stack_unwinding01.cpp:20
+// CHECK-NEXT: frame #2: {{.*}} kernel32.dll`BaseThreadInitThunk + 34
+// CHECK-NEXT: frame #3: {{.*}} ntdll.dll`RtlUserThreadStart + 52
+
+
+// CHECK: (lldb) thread backtrace
+// CHECK-NEXT: * thread #1, stop reason = breakpoint 1.1
+// CHECK-NEXT:   * frame #0: {{.*}} stack_unwinding01.cpp.tmp.exe`Struct::simple_method at stack_unwinding01.cpp:12
+// CHECK-NEXT: frame #1: {{.*}} stack_unwinding01.cpp.tmp.exe`Struct::simple_method at stack_unwinding01.cpp:12
+// CHECK-NEXT: frame #2: {{.*}} stack_unwinding01.cpp.tmp.exe`main(argc={{.*}}, argv={{.*}}) at stack_unwinding01.cpp:20
+// CHECK-NEXT: frame #3: {{.*}} kernel32.dll`BaseThreadInitThunk + 34
+// CHECK-NEXT: frame #4: {{.*}} ntdll.dll`RtlUserThreadStart + 52
+
+// CHECK: (lldb) thread backtrace
+// CHECK-NEXT: * thread #1, stop reason = breakpoint 1.1
+// CHECK-NEXT:   * frame #0: {{.*}} stack_unwinding01.cpp.tmp.exe`Struct::simple_method at stack_unwinding01.cpp:12
+// CHECK-NEXT: frame #1: {{.*}} stack_unwinding01.cpp.tmp.exe`Struct::simple_method at stack_unwinding01.cpp:12
+// CHECK-NEXT: frame #2: {{.*}} stack_unwinding01.cpp.tmp.exe`Struct::simple_method at stack_unwinding01.cpp:12
+// CHECK-NEXT: frame #3: {{.*}} stack_unwinding01.cpp.tmp.exe`main(argc={{.*}}, argv={{.*}}) at stack_unwinding01.

[Lldb-commits] [PATCH] D61128: Support member function types in PdbAstBuilder

[Mikhail Senkov via Phabricator via lldb-commits]

zloyrobot added a comment.

In D61128#1482765 , @amccarth wrote:

> Thanks for the improved commit message.  Again, sorry about the delay.


No problem, thanks for review!


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D61128



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

[Lldb-commits] [PATCH] D61128: Support member function types in PdbAstBuilder

[Mikhail Senkov via Phabricator via lldb-commits]

zloyrobot added a comment.

In D61128#1483663 , @zloyrobot wrote:

> In D61128#1482765 , @amccarth wrote:
>
> > Thanks for the improved commit message.  Again, sorry about the delay.
>
>
> No problem, thanks for review!


I afraid I don't have commit access to merge this path. Could somebody help me?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D61128



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

[Lldb-commits] [PATCH] D60963: Fix dereferencing null pointer

[Mikhail Senkov via Phabricator via lldb-commits]

zloyrobot added a comment.

kind reminder


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D60963



___
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] D61128: Support member function types in PdbAstBuilder

[Mikhail Senkov via Phabricator via lldb-commits]

zloyrobot added a comment.

In D61128#1499934 , @labath wrote:

> Looks like this is failing on the windows bot 
> http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/4635/steps/test/logs/stdio.
>
> Looking at the test, it seems that you are asserting that you will unwind 
> into a particular offset in ntdll.dll. That sounds like an incredibly fragile 
> assertion, which will only be true for people running the exact same version 
> of ntdll that you happen to have...


My bad, these assertions (ntdll and kernel32) should be removed from test. I 
already asked @aleksandr.urakov to do it.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D61128



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

[Lldb-commits] [PATCH] D61886: Support member functions construction and lookup in PdbAstBuilder

[Mikhail Senkov via Phabricator via lldb-commits]

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

This path implements member function support in 
PdbAstBuilder::GetOrCreateFunctionDecl. It allows to lookup and evaluate struct 
fields inside struct members


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D61886

Files:
  lldb/lit/SymbolFile/NativePDB/Inputs/struct-fields.lldbinit
  lldb/lit/SymbolFile/NativePDB/struct-fields.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.h

Index: lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.h
===
--- lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.h
+++ lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.h
@@ -109,6 +109,9 @@
   clang::VarDecl *CreateVariableDecl(PdbSymUid uid,
  llvm::codeview::CVSymbol sym,
  clang::DeclContext &scope);
+  clang::CXXMethodDecl* LookupOrCreateMethodDecl(clang::CXXRecordDecl* parent,
+  const llvm::codeview::ProcSym& method_sym,
+  llvm::StringRef proc_name);
   clang::DeclContext *
   GetParentDeclContextForSymbol(const llvm::codeview::CVSymbol &sym);
 
Index: lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
===
--- lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
+++ lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
@@ -983,19 +983,39 @@
   return qt;
 }
 
+clang::CXXMethodDecl *
+PdbAstBuilder::LookupOrCreateMethodDecl(clang::CXXRecordDecl* parent,
+const ProcSym& method_sym,
+llvm::StringRef proc_name) {
+  
+  PdbTypeSymId type_id(method_sym.FunctionType);
+  clang::QualType qt = GetOrCreateType(type_id);
+  if (qt.isNull())
+return nullptr;
+
+  CompilerType parent_tag = m_clang.GetTypeForDecl(parent);
+  if (parent_tag.GetCompleteType()) {
+for (clang::CXXMethodDecl *method : parent->methods()) {
+  if (method->getNameAsString() == proc_name && method->getType() == qt)
+return method;
+}
+  }
+
+  PdbTypeSymId type = PdbTypeSymId(method_sym.FunctionType);
+  clang::QualType method_qt = GetOrCreateType(type);
+  CompleteType(method_qt);
+
+  return clang().AddMethodToCXXRecordType(parent_tag.GetOpaqueQualType(),
+proc_name.data(),nullptr, ToCompilerType(method_qt),
+lldb::eAccessPublic,false,false,false,
+false,false,false);
+}
+
 clang::FunctionDecl *
 PdbAstBuilder::GetOrCreateFunctionDecl(PdbCompilandSymId func_id) {
   if (clang::Decl *decl = TryGetDecl(func_id))
 return llvm::dyn_cast(decl);
 
-  clang::DeclContext *parent = GetParentDeclContext(PdbSymUid(func_id));
-  std::string context_name;
-  if (clang::NamespaceDecl *ns = llvm::dyn_cast(parent)) {
-context_name = ns->getQualifiedNameAsString();
-  } else if (clang::TagDecl *tag = llvm::dyn_cast(parent)) {
-context_name = tag->getQualifiedNameAsString();
-  }
-
   CVSymbol cvs = m_index.ReadSymbolRecord(func_id);
   ProcSym proc(static_cast(cvs.kind()));
   llvm::cantFail(SymbolDeserializer::deserializeAs(cvs, proc));
@@ -1005,21 +1025,27 @@
   if (qt.isNull())
 return nullptr;
 
-  clang::StorageClass storage = clang::SC_None;
-  if (proc.Kind == SymbolRecordKind::ProcSym)
-storage = clang::SC_Static;
-
-  const clang::FunctionProtoType *func_type =
-  llvm::dyn_cast(qt);
+  clang::DeclContext *parent = GetParentDeclContext(PdbSymUid(func_id));
+  
+  llvm::StringRef proc_name = proc.Name;
+  if (clang::NamedDecl *ns = llvm::dyn_cast(parent)) {
+proc_name.consume_front(ns->getQualifiedNameAsString());
+proc_name.consume_front("::");
+  }
 
-  CompilerType func_ct = ToCompilerType(qt);
+  clang::FunctionDecl *function_decl = nullptr;
+  if (clang::CXXRecordDecl *tag = llvm::dyn_cast(parent))
+function_decl = LookupOrCreateMethodDecl(tag, proc, proc_name);
 
-  llvm::StringRef proc_name = proc.Name;
-  proc_name.consume_front(context_name);
-  proc_name.consume_front("::");
+  if (function_decl == nullptr) {
+clang::StorageClass storage = clang::SC_None;
+if (proc.Kind == SymbolRecordKind::ProcSym)
+  storage = clang::SC_Static;
 
-  clang::FunctionDecl *function_decl = m_clang.CreateFunctionDeclaration(
+CompilerType func_ct = ToCompilerType(qt);
+function_decl = m_clang.CreateFunctionDeclaration(
   parent, proc_name.str().c_str(), func_ct, storage, false);
+  }
 
   lldbassert(m_uid_to_decl.count(toOpaqueUid(func_id)) == 0);
   m_uid_to_decl[toOpaqueUid(func_id)] = function_decl;
@@ -1028,7 +1054,9 @@
   status.uid = toOpaqueUid(func_id);
   m_decl_to_status.insert({function_decl, status});
 
-  CreateFunctionParam

[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