[Lldb-commits] [PATCH] D57751: minidump: Add ability to attach (breakpad) symbol files to placeholder modules

[Leonard Mosescu via Phabricator via lldb-commits]

lemo accepted this revision.
lemo added a comment.

Looks good - a few comments inline.




Comment at: include/lldb/Core/Module.h:178
+module_sp->m_file = module_sp->m_objfile_sp->GetFileSpec();
+return std::move(module_sp);
   }

nit: return std::move(x) is almost never a good idea. It's not needed, verbose 
and most importantly it actually inhibits NRVO so it's likely less efficient. 



Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:51
 public:
-  PlaceholderModule(const ModuleSpec _spec) :
-Module(module_spec.GetFileSpec(), module_spec.GetArchitecture()) {
-if (module_spec.GetUUID().IsValid())
-  SetUUID(module_spec.GetUUID());
-  }
-
-  // Creates a synthetic module section covering the whole module image (and
-  // sets the section load address as well)
-  void CreateImageSection(const MinidumpModule *module, Target& target) {
-const ConstString section_name(".module_image");
-lldb::SectionSP section_sp(new Section(
-shared_from_this(), // Module to which this section belongs.
-nullptr,// ObjectFile
-0,  // Section ID.
-section_name,   // Section name.
-eSectionTypeContainer,  // Section type.
-module->base_of_image,  // VM address.
-module->size_of_image,  // VM size in bytes of this section.
-0,  // Offset of this section in the file.
-module->size_of_image,  // Size of the section as found in the file.
-12, // Alignment of the section (log2)
-0,  // Flags for this section.
-1));// Number of host bytes per target byte
-section_sp->SetPermissions(ePermissionsExecutable | ePermissionsReadable);
-GetSectionList()->AddSection(section_sp);
-target.GetSectionLoadList().SetSectionLoadAddress(
-section_sp, module->base_of_image);
+  PlaceholderObjectFile(const lldb::ModuleSP _sp,
+const ModuleSpec _spec, lldb::offset_t base,

Can we avoid passing both the module_sp and module_spec? I try to avoid 
interfaces which allow for inconsistent states, ex: what if module_sp and 
module_spec describe completely different modules?

At very least I'd add a few asserts, but if we can change the interface to 
avoid it, even better.



Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:52
-//--
-class PlaceholderModule : public Module {
 public:

Nice - so we can do without this in all scenarios? What about PDBs?



Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:66
+  Strata CalculateStrata() override { return eStrataUnknown; }
+  void Dump(Stream *s) override {}
+  uint32_t GetDependentModules(FileSpecList _list) override { return 0; }

should we dump something meaningful? I don't remember the exact command but I 
think this surfaces in user output


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

https://reviews.llvm.org/D57751



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

[Lldb-commits] [PATCH] D56595: SymbolFileBreakpad: Add line table support

[Leonard Mosescu via Phabricator via lldb-commits]

lemo accepted this revision.
lemo added a comment.
This revision is now accepted and ready to land.

The latest version looks good to me. Please update the description (it still 
says it uses the one CU per symbols file)




Comment at: include/lldb/Core/FileSpecList.h:72
+  /// Move-assignment operator.
+  FileSpecList =(FileSpecList &) = default;
 

nit: if you have move assignment-operator also add the move constructor


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

https://reviews.llvm.org/D56595



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

[Lldb-commits] [PATCH] D57037: BreakpadRecords: Address post-commit feedback

[Leonard Mosescu via Phabricator via lldb-commits]

lemo accepted this revision.
lemo added a comment.
This revision is now accepted and ready to land.

Looks good - the all-zero UUID case is a bit unfortunate but it seems we have 
to handle it (like Greg suggested)




Comment at: source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp:69
+  llvm::StringRef chunk = str.take_front(hex_digits());
+  uintmax_t t;
+  if (!to_integer(chunk, t, 16))

= 0; ?



Comment at: source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h:48
 
 class ModuleRecord : public Record {
 public:

coding-convention-wise: should these definitions use struct instead of class?



Comment at: source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h:59
 
-bool operator==(const ModuleRecord , const ModuleRecord );
+bool operator==(const ModuleRecord , const ModuleRecord ) {
+  return L.OS == R.OS && L.Arch == R.Arch && L.ID == R.ID;

const method qualifier?



Comment at: source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h:72
 
 inline bool operator==(const InfoRecord , const InfoRecord ) {
+  return L.ID == R.ID;

ditto


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

https://reviews.llvm.org/D57037



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

[Lldb-commits] [PATCH] D56844: Breakpad: Extract parsing code into a separate file

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added a comment.

Looks good. A few questions/suggestions inline.




Comment at: lldb/trunk/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp:74
+  static_assert(sizeof(data) == 20, "");
+  // The textual module id encoding should be between 33 and 40 bytes long,
+  // depending on the size of the age field, which is of variable length.

the comment is great, but I think we should still have symbolic constants for 
all these magic values



Comment at: lldb/trunk/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h:34
+
+  ~Record() = default;
+

should this be virtual? (even though the class doesn't have other virtual 
members, the class hierarchy introduces the possibility for consumers to treat 
them a polymorphic types - ex. storing as Record* and use the kind type to 
figure out the concrete type)



Comment at: lldb/trunk/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h:40
+private:
+  Kind TheKind;
+};

Just curious, what is the definitive convention for naming data members? A lot 
of LLDB code uses the m_camelCase convention.



Comment at: lldb/trunk/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h:84
+
+class PublicRecord : public Record {
+public:

most of these types are just plain data containers, so why not make them 
structs? (and avoid all the boilerplate associated with public accessors, 
repetitive constructors, ...)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56844



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

[Lldb-commits] [PATCH] D56293: Use the minidump exception record if present

[Leonard Mosescu via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rLLDB350546: Use the minidump exception record if present 
(authored by lemo, committed by ).

Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D56293

Files:
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.cpp
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.dmp
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.lldbinit
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.pdb
  lit/Minidump/Windows/Sigsegv/sigsegv.test
  source/Plugins/Process/minidump/MinidumpParser.cpp
  source/Plugins/Process/minidump/MinidumpParser.h
  source/Plugins/Process/minidump/ProcessMinidump.cpp

Index: source/Plugins/Process/minidump/MinidumpParser.h
===
--- source/Plugins/Process/minidump/MinidumpParser.h
+++ source/Plugins/Process/minidump/MinidumpParser.h
@@ -58,6 +58,9 @@
 
   llvm::ArrayRef GetThreads();
 
+  llvm::ArrayRef
+  GetThreadContext(const MinidumpLocationDescriptor );
+
   llvm::ArrayRef GetThreadContext(const MinidumpThread );
 
   llvm::ArrayRef GetThreadContextWow64(const MinidumpThread );
Index: source/Plugins/Process/minidump/MinidumpParser.cpp
===
--- source/Plugins/Process/minidump/MinidumpParser.cpp
+++ source/Plugins/Process/minidump/MinidumpParser.cpp
@@ -107,11 +107,15 @@
 }
 
 llvm::ArrayRef
-MinidumpParser::GetThreadContext(const MinidumpThread ) {
-  if (td.thread_context.rva + td.thread_context.data_size > GetData().size())
+MinidumpParser::GetThreadContext(const MinidumpLocationDescriptor ) {
+  if (location.rva + location.data_size > GetData().size())
 return {};
+  return GetData().slice(location.rva, location.data_size);
+}
 
-  return GetData().slice(td.thread_context.rva, td.thread_context.data_size);
+llvm::ArrayRef
+MinidumpParser::GetThreadContext(const MinidumpThread ) {
+  return GetThreadContext(td.thread_context);
 }
 
 llvm::ArrayRef
Index: source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -34,6 +34,7 @@
 #include "llvm/Support/Threading.h"
 
 #include "Plugins/Process/Utility/StopInfoMachException.h"
+
 // C includes
 // C++ includes
 
@@ -80,7 +81,7 @@
 section_sp, module->base_of_image);
   }
 
-  ObjectFile *GetObjectFile() override { return nullptr; }
+ObjectFile *GetObjectFile() override { return nullptr; }
 
   SectionList *GetSectionList() override {
 return Module::GetUnifiedSectionList();
@@ -305,19 +306,22 @@
 
 bool ProcessMinidump::UpdateThreadList(ThreadList _thread_list,
ThreadList _thread_list) {
-  uint32_t num_threads = 0;
-  if (m_thread_list.size() > 0)
-num_threads = m_thread_list.size();
+  for (const MinidumpThread& thread : m_thread_list) {
+MinidumpLocationDescriptor context_location = thread.thread_context;
+
+// If the minidump contains an exception context, use it
+if (m_active_exception != nullptr &&
+m_active_exception->thread_id == thread.thread_id) {
+  context_location = m_active_exception->thread_context;
+}
 
-  for (lldb::tid_t tid = 0; tid < num_threads; ++tid) {
 llvm::ArrayRef context;
 if (!m_is_wow64)
-  context = m_minidump_parser.GetThreadContext(m_thread_list[tid]);
+  context = m_minidump_parser.GetThreadContext(context_location);
 else
-  context = m_minidump_parser.GetThreadContextWow64(m_thread_list[tid]);
+  context = m_minidump_parser.GetThreadContextWow64(thread);
 
-lldb::ThreadSP thread_sp(
-new ThreadMinidump(*this, m_thread_list[tid], context));
+lldb::ThreadSP thread_sp(new ThreadMinidump(*this, thread, context));
 new_thread_list.AddThread(thread_sp);
   }
   return new_thread_list.GetSize(false) > 0;
@@ -549,9 +553,9 @@
 APPEND_OPT(m_dump_linux_all);
 m_option_group.Finalize();
   }
-  
+
   ~CommandObjectProcessMinidumpDump() {}
-  
+
   Options *GetOptions() override { return _option_group; }
 
   bool DoExecute(Args , CommandReturnObject ) override {
@@ -563,7 +567,7 @@
   return false;
 }
 SetDefaultOptionsIfNoneAreSet();
-
+
 ProcessMinidump *process = static_cast(
 m_interpreter.GetExecutionContext().GetProcessPtr());
 result.SetStatus(eReturnStatusSuccessFinishResult);
@@ -635,7 +639,7 @@
 LoadSubCommand("dump",
 CommandObjectSP(new CommandObjectProcessMinidumpDump(interpreter)));
   }
-  
+
   ~CommandObjectMultiwordProcessMinidump() {}
 };
 
Index: lit/Minidump/Windows/Sigsegv/sigsegv.test
===
--- lit/Minidump/Windows/Sigsegv/sigsegv.test
+++ lit/Minidump/Windows/Sigsegv/sigsegv.test
@@ -0,0 +1,13 @@
+// RUN: cd 

[Lldb-commits] [PATCH] D56293: Use the minidump exception record if present

[Leonard Mosescu via Phabricator via lldb-commits]

lemo updated this revision to Diff 180515.
lemo added a comment.

Incorporating Pavel's suggestions (range-based loop, explicit type instead of 
auto)


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

https://reviews.llvm.org/D56293

Files:
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.cpp
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.dmp
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.lldbinit
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.pdb
  lit/Minidump/Windows/Sigsegv/sigsegv.test
  source/Plugins/Process/minidump/MinidumpParser.cpp
  source/Plugins/Process/minidump/MinidumpParser.h
  source/Plugins/Process/minidump/ProcessMinidump.cpp

Index: source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -34,6 +34,7 @@
 #include "llvm/Support/Threading.h"
 
 #include "Plugins/Process/Utility/StopInfoMachException.h"
+
 // C includes
 // C++ includes
 
@@ -80,7 +81,7 @@
 section_sp, module->base_of_image);
   }
 
-  ObjectFile *GetObjectFile() override { return nullptr; }
+ObjectFile *GetObjectFile() override { return nullptr; }
 
   SectionList *GetSectionList() override {
 return Module::GetUnifiedSectionList();
@@ -305,19 +306,22 @@
 
 bool ProcessMinidump::UpdateThreadList(ThreadList _thread_list,
ThreadList _thread_list) {
-  uint32_t num_threads = 0;
-  if (m_thread_list.size() > 0)
-num_threads = m_thread_list.size();
+  for (const MinidumpThread& thread : m_thread_list) {
+MinidumpLocationDescriptor context_location = thread.thread_context;
+
+// If the minidump contains an exception context, use it
+if (m_active_exception != nullptr &&
+m_active_exception->thread_id == thread.thread_id) {
+  context_location = m_active_exception->thread_context;
+}
 
-  for (lldb::tid_t tid = 0; tid < num_threads; ++tid) {
 llvm::ArrayRef context;
 if (!m_is_wow64)
-  context = m_minidump_parser.GetThreadContext(m_thread_list[tid]);
+  context = m_minidump_parser.GetThreadContext(context_location);
 else
-  context = m_minidump_parser.GetThreadContextWow64(m_thread_list[tid]);
+  context = m_minidump_parser.GetThreadContextWow64(thread);
 
-lldb::ThreadSP thread_sp(
-new ThreadMinidump(*this, m_thread_list[tid], context));
+lldb::ThreadSP thread_sp(new ThreadMinidump(*this, thread, context));
 new_thread_list.AddThread(thread_sp);
   }
   return new_thread_list.GetSize(false) > 0;
@@ -549,9 +553,9 @@
 APPEND_OPT(m_dump_linux_all);
 m_option_group.Finalize();
   }
-  
+
   ~CommandObjectProcessMinidumpDump() {}
-  
+
   Options *GetOptions() override { return _option_group; }
 
   bool DoExecute(Args , CommandReturnObject ) override {
@@ -563,7 +567,7 @@
   return false;
 }
 SetDefaultOptionsIfNoneAreSet();
-
+
 ProcessMinidump *process = static_cast(
 m_interpreter.GetExecutionContext().GetProcessPtr());
 result.SetStatus(eReturnStatusSuccessFinishResult);
@@ -635,7 +639,7 @@
 LoadSubCommand("dump",
 CommandObjectSP(new CommandObjectProcessMinidumpDump(interpreter)));
   }
-  
+
   ~CommandObjectMultiwordProcessMinidump() {}
 };
 
Index: source/Plugins/Process/minidump/MinidumpParser.h
===
--- source/Plugins/Process/minidump/MinidumpParser.h
+++ source/Plugins/Process/minidump/MinidumpParser.h
@@ -58,6 +58,9 @@
 
   llvm::ArrayRef GetThreads();
 
+  llvm::ArrayRef
+  GetThreadContext(const MinidumpLocationDescriptor );
+
   llvm::ArrayRef GetThreadContext(const MinidumpThread );
 
   llvm::ArrayRef GetThreadContextWow64(const MinidumpThread );
Index: source/Plugins/Process/minidump/MinidumpParser.cpp
===
--- source/Plugins/Process/minidump/MinidumpParser.cpp
+++ source/Plugins/Process/minidump/MinidumpParser.cpp
@@ -107,11 +107,15 @@
 }
 
 llvm::ArrayRef
-MinidumpParser::GetThreadContext(const MinidumpThread ) {
-  if (td.thread_context.rva + td.thread_context.data_size > GetData().size())
+MinidumpParser::GetThreadContext(const MinidumpLocationDescriptor ) {
+  if (location.rva + location.data_size > GetData().size())
 return {};
+  return GetData().slice(location.rva, location.data_size);
+}
 
-  return GetData().slice(td.thread_context.rva, td.thread_context.data_size);
+llvm::ArrayRef
+MinidumpParser::GetThreadContext(const MinidumpThread ) {
+  return GetThreadContext(td.thread_context);
 }
 
 llvm::ArrayRef
Index: lit/Minidump/Windows/Sigsegv/sigsegv.test
===
--- lit/Minidump/Windows/Sigsegv/sigsegv.test
+++ lit/Minidump/Windows/Sigsegv/sigsegv.test
@@ -0,0 +1,13 @@
+// RUN: cd %p/Inputs
+// RUN: env LLDB_USE_NATIVE_PDB_READER=1 \
+// RUN:   

[Lldb-commits] [PATCH] D56293: Use the minidump exception record if present

[Leonard Mosescu via Phabricator via lldb-commits]

lemo updated this revision to Diff 180263.
lemo added a comment.

Removed sigsegv.exe from the test inputs


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

https://reviews.llvm.org/D56293

Files:
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.cpp
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.dmp
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.lldbinit
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.pdb
  lit/Minidump/Windows/Sigsegv/sigsegv.test
  source/Plugins/Process/minidump/MinidumpParser.cpp
  source/Plugins/Process/minidump/MinidumpParser.h
  source/Plugins/Process/minidump/ProcessMinidump.cpp

Index: source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -34,6 +34,7 @@
 #include "llvm/Support/Threading.h"
 
 #include "Plugins/Process/Utility/StopInfoMachException.h"
+
 // C includes
 // C++ includes
 
@@ -80,7 +81,7 @@
 section_sp, module->base_of_image);
   }
 
-  ObjectFile *GetObjectFile() override { return nullptr; }
+ObjectFile *GetObjectFile() override { return nullptr; }
 
   SectionList *GetSectionList() override {
 return Module::GetUnifiedSectionList();
@@ -309,15 +310,23 @@
   if (m_thread_list.size() > 0)
 num_threads = m_thread_list.size();
 
-  for (lldb::tid_t tid = 0; tid < num_threads; ++tid) {
+  for (size_t t_index = 0; t_index < num_threads; ++t_index) {
 llvm::ArrayRef context;
+const auto& thread = m_thread_list[t_index];
+auto context_location = thread.thread_context;
+
+// If the minidump contains an exception context, use it
+if (m_active_exception != nullptr &&
+m_active_exception->thread_id == thread.thread_id) {
+  context_location = m_active_exception->thread_context;
+}
+
 if (!m_is_wow64)
-  context = m_minidump_parser.GetThreadContext(m_thread_list[tid]);
+  context = m_minidump_parser.GetThreadContext(context_location);
 else
-  context = m_minidump_parser.GetThreadContextWow64(m_thread_list[tid]);
+  context = m_minidump_parser.GetThreadContextWow64(thread);
 
-lldb::ThreadSP thread_sp(
-new ThreadMinidump(*this, m_thread_list[tid], context));
+lldb::ThreadSP thread_sp(new ThreadMinidump(*this, thread, context));
 new_thread_list.AddThread(thread_sp);
   }
   return new_thread_list.GetSize(false) > 0;
@@ -549,9 +558,9 @@
 APPEND_OPT(m_dump_linux_all);
 m_option_group.Finalize();
   }
-  
+
   ~CommandObjectProcessMinidumpDump() {}
-  
+
   Options *GetOptions() override { return _option_group; }
 
   bool DoExecute(Args , CommandReturnObject ) override {
@@ -563,7 +572,7 @@
   return false;
 }
 SetDefaultOptionsIfNoneAreSet();
-
+
 ProcessMinidump *process = static_cast(
 m_interpreter.GetExecutionContext().GetProcessPtr());
 result.SetStatus(eReturnStatusSuccessFinishResult);
@@ -635,7 +644,7 @@
 LoadSubCommand("dump",
 CommandObjectSP(new CommandObjectProcessMinidumpDump(interpreter)));
   }
-  
+
   ~CommandObjectMultiwordProcessMinidump() {}
 };
 
Index: source/Plugins/Process/minidump/MinidumpParser.h
===
--- source/Plugins/Process/minidump/MinidumpParser.h
+++ source/Plugins/Process/minidump/MinidumpParser.h
@@ -58,6 +58,9 @@
 
   llvm::ArrayRef GetThreads();
 
+  llvm::ArrayRef
+  GetThreadContext(const MinidumpLocationDescriptor );
+
   llvm::ArrayRef GetThreadContext(const MinidumpThread );
 
   llvm::ArrayRef GetThreadContextWow64(const MinidumpThread );
Index: source/Plugins/Process/minidump/MinidumpParser.cpp
===
--- source/Plugins/Process/minidump/MinidumpParser.cpp
+++ source/Plugins/Process/minidump/MinidumpParser.cpp
@@ -107,11 +107,15 @@
 }
 
 llvm::ArrayRef
-MinidumpParser::GetThreadContext(const MinidumpThread ) {
-  if (td.thread_context.rva + td.thread_context.data_size > GetData().size())
+MinidumpParser::GetThreadContext(const MinidumpLocationDescriptor ) {
+  if (location.rva + location.data_size > GetData().size())
 return {};
+  return GetData().slice(location.rva, location.data_size);
+}
 
-  return GetData().slice(td.thread_context.rva, td.thread_context.data_size);
+llvm::ArrayRef
+MinidumpParser::GetThreadContext(const MinidumpThread ) {
+  return GetThreadContext(td.thread_context);
 }
 
 llvm::ArrayRef
Index: lit/Minidump/Windows/Sigsegv/sigsegv.test
===
--- lit/Minidump/Windows/Sigsegv/sigsegv.test
+++ lit/Minidump/Windows/Sigsegv/sigsegv.test
@@ -0,0 +1,13 @@
+// RUN: cd %p/Inputs
+// RUN: env LLDB_USE_NATIVE_PDB_READER=1 \
+// RUN:   %lldb -c sigsegv.dmp -s sigsegv.lldbinit | FileCheck %s
+
+CHECK: * thread #1, stop reason = Exception 0xc005 encountered at address 0x7ff7a13110d9
+CHECK:   * frame 

[Lldb-commits] [PATCH] D56293: Use the minidump exception record if present

[Leonard Mosescu via Phabricator via lldb-commits]

lemo created this revision.
lemo added reviewers: labath, zturner.
lemo added a project: LLDB.
Herald added subscribers: jfb, abidh.

If the minidump contains a saved exception record use it automatically.

(This patch is cherry picked from the larger https://reviews.llvm.org/D55142)


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D56293

Files:
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.cpp
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.dmp
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.exe
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.lldbinit
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.pdb
  lit/Minidump/Windows/Sigsegv/sigsegv.test
  source/Plugins/Process/minidump/MinidumpParser.cpp
  source/Plugins/Process/minidump/MinidumpParser.h
  source/Plugins/Process/minidump/ProcessMinidump.cpp

Index: source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -34,6 +34,7 @@
 #include "llvm/Support/Threading.h"
 
 #include "Plugins/Process/Utility/StopInfoMachException.h"
+
 // C includes
 // C++ includes
 
@@ -80,7 +81,7 @@
 section_sp, module->base_of_image);
   }
 
-  ObjectFile *GetObjectFile() override { return nullptr; }
+ObjectFile *GetObjectFile() override { return nullptr; }
 
   SectionList *GetSectionList() override {
 return Module::GetUnifiedSectionList();
@@ -309,15 +310,23 @@
   if (m_thread_list.size() > 0)
 num_threads = m_thread_list.size();
 
-  for (lldb::tid_t tid = 0; tid < num_threads; ++tid) {
+  for (size_t t_index = 0; t_index < num_threads; ++t_index) {
 llvm::ArrayRef context;
+const auto& thread = m_thread_list[t_index];
+auto context_location = thread.thread_context;
+
+// If the minidump contains an exception context, use it
+if (m_active_exception != nullptr &&
+m_active_exception->thread_id == thread.thread_id) {
+  context_location = m_active_exception->thread_context;
+}
+
 if (!m_is_wow64)
-  context = m_minidump_parser.GetThreadContext(m_thread_list[tid]);
+  context = m_minidump_parser.GetThreadContext(context_location);
 else
-  context = m_minidump_parser.GetThreadContextWow64(m_thread_list[tid]);
+  context = m_minidump_parser.GetThreadContextWow64(thread);
 
-lldb::ThreadSP thread_sp(
-new ThreadMinidump(*this, m_thread_list[tid], context));
+lldb::ThreadSP thread_sp(new ThreadMinidump(*this, thread, context));
 new_thread_list.AddThread(thread_sp);
   }
   return new_thread_list.GetSize(false) > 0;
@@ -549,9 +558,9 @@
 APPEND_OPT(m_dump_linux_all);
 m_option_group.Finalize();
   }
-  
+
   ~CommandObjectProcessMinidumpDump() {}
-  
+
   Options *GetOptions() override { return _option_group; }
 
   bool DoExecute(Args , CommandReturnObject ) override {
@@ -563,7 +572,7 @@
   return false;
 }
 SetDefaultOptionsIfNoneAreSet();
-
+
 ProcessMinidump *process = static_cast(
 m_interpreter.GetExecutionContext().GetProcessPtr());
 result.SetStatus(eReturnStatusSuccessFinishResult);
@@ -635,7 +644,7 @@
 LoadSubCommand("dump",
 CommandObjectSP(new CommandObjectProcessMinidumpDump(interpreter)));
   }
-  
+
   ~CommandObjectMultiwordProcessMinidump() {}
 };
 
Index: source/Plugins/Process/minidump/MinidumpParser.h
===
--- source/Plugins/Process/minidump/MinidumpParser.h
+++ source/Plugins/Process/minidump/MinidumpParser.h
@@ -58,6 +58,9 @@
 
   llvm::ArrayRef GetThreads();
 
+  llvm::ArrayRef
+  GetThreadContext(const MinidumpLocationDescriptor );
+
   llvm::ArrayRef GetThreadContext(const MinidumpThread );
 
   llvm::ArrayRef GetThreadContextWow64(const MinidumpThread );
Index: source/Plugins/Process/minidump/MinidumpParser.cpp
===
--- source/Plugins/Process/minidump/MinidumpParser.cpp
+++ source/Plugins/Process/minidump/MinidumpParser.cpp
@@ -107,11 +107,15 @@
 }
 
 llvm::ArrayRef
-MinidumpParser::GetThreadContext(const MinidumpThread ) {
-  if (td.thread_context.rva + td.thread_context.data_size > GetData().size())
+MinidumpParser::GetThreadContext(const MinidumpLocationDescriptor ) {
+  if (location.rva + location.data_size > GetData().size())
 return {};
+  return GetData().slice(location.rva, location.data_size);
+}
 
-  return GetData().slice(td.thread_context.rva, td.thread_context.data_size);
+llvm::ArrayRef
+MinidumpParser::GetThreadContext(const MinidumpThread ) {
+  return GetThreadContext(td.thread_context);
 }
 
 llvm::ArrayRef
Index: lit/Minidump/Windows/Sigsegv/sigsegv.test
===
--- lit/Minidump/Windows/Sigsegv/sigsegv.test
+++ lit/Minidump/Windows/Sigsegv/sigsegv.test
@@ -0,0 +1,13 @@
+// RUN: cd %p/Inputs
+// RUN: env 

[Lldb-commits] [PATCH] D55727: Add "dump" command as a custom "process plugin" subcommand when ProcessMinidump is used.

[Leonard Mosescu via Phabricator via lldb-commits]

lemo accepted this revision.
lemo added a comment.
This revision is now accepted and ready to land.

I love the idea of custom dump commands, thanks Greg.

The changes look good to me (I agree with Zach suggestion to add lit tests)




Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:706
+  }
+  return "???";
+}

use a more explicit text, ex "unknown stream type" ?



Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:495
+public:
+#define INIT_BOOL(VAR, LONG, SHORT, DESC) \
+VAR(LLDB_OPT_SET_1, false, LONG, SHORT, DESC, false, true)

I'm not a fan of "embedded" macro definitions - it may suggests that they are 
scope bound why they are not. I'd move them above the class definition.


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

https://reviews.llvm.org/D55727



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

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Leonard Mosescu via Phabricator via lldb-commits]

lemo marked an inline comment as done.
lemo added inline comments.



Comment at: source/Commands/CommandObjectTarget.cpp:4246
   if (symbol_file) {
-ObjectFile *object_file = symbol_file->GetObjectFile();
 

note I had to bypass this check: we don't (yet) have a ObjectFilePDB so the 
SymbolFileNativePDB always points to the associated PE binary. 

the check itself seems valuable as a diagnostic but not strictly required. 
Should I add a TODO comment and/or open a bug to revisit this?


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

https://reviews.llvm.org/D55142



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

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Leonard Mosescu via Phabricator via lldb-commits]

lemo updated this revision to Diff 177898.

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

https://reviews.llvm.org/D55142

Files:
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.cpp
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.dmp
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.lldbinit
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.pdb
  lit/Minidump/Windows/Sigsegv/sigsegv.test
  source/Commands/CommandObjectTarget.cpp
  source/Plugins/Process/minidump/MinidumpParser.cpp
  source/Plugins/Process/minidump/MinidumpParser.h
  source/Plugins/Process/minidump/ProcessMinidump.cpp
  source/Plugins/SymbolFile/NativePDB/CompileUnitIndex.cpp
  source/Plugins/SymbolFile/NativePDB/CompileUnitIndex.h
  source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp
  source/Plugins/SymbolFile/NativePDB/PdbIndex.h
  source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp

Index: source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
===
--- source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -30,6 +30,7 @@
 #include "lldb/Symbol/SymbolVendor.h"
 #include "lldb/Symbol/Variable.h"
 #include "lldb/Symbol/VariableList.h"
+#include "lldb/Utility/UUID.h"
 
 #include "llvm/DebugInfo/CodeView/CVRecord.h"
 #include "llvm/DebugInfo/CodeView/CVTypeVisitor.h"
@@ -102,33 +103,57 @@
 }
 
 static std::unique_ptr
-loadMatchingPDBFile(std::string exe_path, llvm::BumpPtrAllocator ) {
-  // Try to find a matching PDB for an EXE.
+loadMatchingPDBFile(lldb_private::ObjectFile& obj_file,
+llvm::BumpPtrAllocator ) {
   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 = std::move(*expected_binary);
+  // Try to find a matching PDB for an EXE.
+  std::string pdb_file;
+  llvm::codeview::GUID guid;
 
-  auto *obj = llvm::dyn_cast(binary.getBinary());
-  if (!obj)
-return nullptr;
-  const llvm::codeview::DebugInfo *pdb_info = nullptr;
+  const auto module = obj_file.GetModule();
+  const auto symbol_file_filespec = module->GetSymbolFileFileSpec();
+  if (symbol_file_filespec.IsResolved()) {
+// If we have the symbol file filespec set, use it
+pdb_file = symbol_file_filespec.GetPath();
+
+// Extract the debug GUID from the ObjectFile
+lldb_private::UUID uuid;
+obj_file.GetUUID();
+auto uuid_bytes = uuid.GetBytes();
+if (uuid_bytes.size() != sizeof(guid) + 4) // CvRecordPdb70
+  return nullptr;
+memcpy(, uuid_bytes.data(), sizeof(guid));
+  } else {
+// Use the PDB path embedded in the binary
+auto expected_binary = createBinary(obj_file.GetFileSpec().GetPath());
+if (!expected_binary) {
+  llvm::consumeError(expected_binary.takeError());
+  return 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;
+OwningBinary binary = std::move(*expected_binary);
+
+auto *obj = llvm::dyn_cast(binary.getBinary());
+if (!obj)
+  return nullptr;
+
+const llvm::codeview::DebugInfo *pdb_info = nullptr;
+llvm::StringRef pdb_file_name_ref;
+if (obj->getDebugPDBInfo(pdb_info, pdb_file_name_ref)) {
+  // If it doesn't have a debug directory, fail.
+  return nullptr;
+}
+pdb_file = pdb_file_name_ref;
+
+// Extract the debug GUID from the debug directory
+memcpy(, pdb_info->PDB70.Signature, sizeof(guid));
+  }
 
   // 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);
+  auto ec = llvm::identify_magic(pdb_file, magic);
   if (ec || magic != llvm::file_magic::pdb)
 return nullptr;
   std::unique_ptr pdb = loadPDBFile(pdb_file, allocator);
@@ -140,11 +165,11 @@
 llvm::consumeError(expected_info.takeError());
 return nullptr;
   }
-  llvm::codeview::GUID guid;
-  memcpy(, pdb_info->PDB70.Signature, 16);
 
+  // TODO: we need to compare the age, in addition to the GUID
   if (expected_info->getGuid() != guid)
 return nullptr;
+
   return pdb;
 }
 
@@ -521,7 +546,7 @@
   if (!m_index) {
 // Lazily load and match the PDB file, but only do this once.
 std::unique_ptr file_up =
-loadMatchingPDBFile(m_obj_file->GetFileSpec().GetPath(), m_allocator);
+loadMatchingPDBFile(*m_obj_file, m_allocator);
 
 if (!file_up) {
   auto module_sp = m_obj_file->GetModule();
@@ -1459,11 +1484,10 @@
 llvm::Optional modi = m_index->GetModuleIndexForVa(file_addr);
 if (!modi)
   return 0;
-CompilandIndexItem *cci = m_index->compilands().GetCompiland(*modi);
-if (!cci)
+if 

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added a comment.

> How large is the PDB file here?

~100kb


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

https://reviews.llvm.org/D55142



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

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Leonard Mosescu via Phabricator via lldb-commits]

lemo updated this revision to Diff 177576.

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

https://reviews.llvm.org/D55142

Files:
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.cpp
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.dmp
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.lldbinit
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.pdb
  lit/Minidump/Windows/Sigsegv/sigsegv.test
  source/Plugins/Process/minidump/MinidumpParser.cpp
  source/Plugins/Process/minidump/MinidumpParser.h
  source/Plugins/Process/minidump/ProcessMinidump.cpp
  source/Plugins/SymbolFile/NativePDB/CompileUnitIndex.cpp
  source/Plugins/SymbolFile/NativePDB/CompileUnitIndex.h
  source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp
  source/Plugins/SymbolFile/NativePDB/PdbIndex.h
  source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp

Index: source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
===
--- source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -30,6 +30,7 @@
 #include "lldb/Symbol/SymbolVendor.h"
 #include "lldb/Symbol/Variable.h"
 #include "lldb/Symbol/VariableList.h"
+#include "lldb/Utility/UUID.h"
 
 #include "llvm/DebugInfo/CodeView/CVRecord.h"
 #include "llvm/DebugInfo/CodeView/CVTypeVisitor.h"
@@ -102,33 +103,59 @@
 }
 
 static std::unique_ptr
-loadMatchingPDBFile(std::string exe_path, llvm::BumpPtrAllocator ) {
-  // Try to find a matching PDB for an EXE.
+loadMatchingPDBFile(lldb_private::ObjectFile& obj_file,
+llvm::BumpPtrAllocator ) {
   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 = std::move(*expected_binary);
+  // Try to find a matching PDB for an EXE.
+  std::string pdb_file;
+  llvm::codeview::GUID guid;
 
-  auto *obj = llvm::dyn_cast(binary.getBinary());
-  if (!obj)
-return nullptr;
-  const llvm::codeview::DebugInfo *pdb_info = nullptr;
+  auto expected_binary = createBinary(obj_file.GetFileSpec().GetPath());
+  if (expected_binary) {
+OwningBinary binary = std::move(*expected_binary);
 
-  // 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;
+auto *obj = llvm::dyn_cast(binary.getBinary());
+if (!obj)
+  return nullptr;
+
+const llvm::codeview::DebugInfo *pdb_info = nullptr;
+llvm::StringRef pdb_file_name_ref;
+if (obj->getDebugPDBInfo(pdb_info, pdb_file_name_ref)) {
+  // If it doesn't have a debug directory, fail.
+  return nullptr;
+}
+pdb_file = pdb_file_name_ref;
+
+// Extract the debug GUID from the debug directory
+memcpy(, pdb_info->PDB70.Signature, sizeof(guid));
+  } else {
+// If the file isn't a PE/COFF executable, look for the PDB in the
+// current directory. This provides a basic solution for debugging minidumps
+// although it's only a stop-gap (until we implement a SymbolVendor)
+//
+// TODO: Implement a SymbolVendor for PDBs.
+//
+llvm::consumeError(expected_binary.takeError());
+pdb_file = obj_file.GetFileSpec()
+   .GetFileNameStrippingExtension()
+   .GetStringRef()
+   .str();
+pdb_file += ".pdb";
+
+// Extract the debug GUID from the ObjectFile
+lldb_private::UUID uuid;
+obj_file.GetUUID();
+auto uuid_bytes = uuid.GetBytes();
+if (uuid_bytes.size() != sizeof(guid) + 4) // CvRecordPdb70
+  return nullptr;
+memcpy(, uuid_bytes.data(), sizeof(guid));
+  }
 
   // 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);
+  auto ec = llvm::identify_magic(pdb_file, magic);
   if (ec || magic != llvm::file_magic::pdb)
 return nullptr;
   std::unique_ptr pdb = loadPDBFile(pdb_file, allocator);
@@ -140,11 +167,11 @@
 llvm::consumeError(expected_info.takeError());
 return nullptr;
   }
-  llvm::codeview::GUID guid;
-  memcpy(, pdb_info->PDB70.Signature, 16);
 
+  // TODO: we need to compare the age, in addition to the GUID
   if (expected_info->getGuid() != guid)
 return nullptr;
+
   return pdb;
 }
 
@@ -521,7 +548,7 @@
   if (!m_index) {
 // Lazily load and match the PDB file, but only do this once.
 std::unique_ptr file_up =
-loadMatchingPDBFile(m_obj_file->GetFileSpec().GetPath(), m_allocator);
+loadMatchingPDBFile(*m_obj_file, m_allocator);
 
 if (!file_up) {
   auto module_sp = m_obj_file->GetModule();
@@ -1459,11 +1486,10 @@
 llvm::Optional modi = m_index->GetModuleIndexForVa(file_addr);
 if (!modi)
   return 0;
-CompilandIndexItem *cci = 

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added inline comments.



Comment at: source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:171-174
+  // TODO: we need to compare the age, in addition to the GUID
   if (expected_info->getGuid() != guid)
 return nullptr;
+

labath wrote:
> Mainly out of curiosity, what's the complication here? The llvm interface 
> does not provide the means to retrieve the age?
Yes, the interface doesn't expose the age, although that's not not really the 
problem (the age is a detail - part of the generic "Debug ID")

We just need to make the handling of PE/COFF & VC UUIDs more consistent: right 
now most places expect a stripped version (just the GUID). This is wrong since 
a correct PDB match should take the age into account, but it's outside the 
scope of this change.


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

https://reviews.llvm.org/D55142



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

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Leonard Mosescu via Phabricator via lldb-commits]

lemo updated this revision to Diff 177329.
lemo marked 3 inline comments as done.
lemo added a comment.

Incorporating feedback + adding a test case


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

https://reviews.llvm.org/D55142

Files:
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.cpp
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.dmp
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.lldbinit
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.pdb
  lit/Minidump/Windows/Sigsegv/sigsegv.test
  source/Plugins/Process/minidump/MinidumpParser.cpp
  source/Plugins/Process/minidump/MinidumpParser.h
  source/Plugins/Process/minidump/ProcessMinidump.cpp
  source/Plugins/SymbolFile/NativePDB/CompileUnitIndex.cpp
  source/Plugins/SymbolFile/NativePDB/CompileUnitIndex.h
  source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp
  source/Plugins/SymbolFile/NativePDB/PdbIndex.h
  source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp

Index: source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
===
--- source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -30,6 +30,7 @@
 #include "lldb/Symbol/SymbolVendor.h"
 #include "lldb/Symbol/Variable.h"
 #include "lldb/Symbol/VariableList.h"
+#include "lldb/Utility/UUID.h"
 
 #include "llvm/DebugInfo/CodeView/CVRecord.h"
 #include "llvm/DebugInfo/CodeView/CVTypeVisitor.h"
@@ -102,33 +103,59 @@
 }
 
 static std::unique_ptr
-loadMatchingPDBFile(std::string exe_path, llvm::BumpPtrAllocator ) {
-  // Try to find a matching PDB for an EXE.
+loadMatchingPDBFile(lldb_private::ObjectFile& obj_file,
+llvm::BumpPtrAllocator ) {
   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 = std::move(*expected_binary);
+  // Try to find a matching PDB for an EXE.
+  std::string pdb_file;
+  llvm::codeview::GUID guid;
 
-  auto *obj = llvm::dyn_cast(binary.getBinary());
-  if (!obj)
-return nullptr;
-  const llvm::codeview::DebugInfo *pdb_info = nullptr;
+  auto expected_binary = createBinary(obj_file.GetFileSpec().GetPath());
+  if (expected_binary) {
+OwningBinary binary = std::move(*expected_binary);
 
-  // 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;
+auto *obj = llvm::dyn_cast(binary.getBinary());
+if (!obj)
+  return nullptr;
+
+const llvm::codeview::DebugInfo *pdb_info = nullptr;
+llvm::StringRef pdb_file_name_ref;
+if (obj->getDebugPDBInfo(pdb_info, pdb_file_name_ref)) {
+  // If it doesn't have a debug directory, fail.
+  return nullptr;
+}
+pdb_file = pdb_file_name_ref;
+
+// Extract the debug GUID from the debug directory
+memcpy(, pdb_info->PDB70.Signature, sizeof(guid));
+  } else {
+// If the file isn't a PE/COFF executable, look for the PDB in the
+// current directory. This provides a basic solution for debugging minidumps
+// although it's only a stop-gap (until we implement a SymbolVendor)
+//
+// TODO: Implement a SymbolVendor for PDBs.
+//
+llvm::consumeError(expected_binary.takeError());
+pdb_file = obj_file.GetFileSpec()
+   .GetFileNameStrippingExtension()
+   .GetStringRef()
+   .str();
+pdb_file += ".pdb";
+
+// Extract the debug GUID from the ObjectFile
+lldb_private::UUID uuid;
+obj_file.GetUUID();
+auto uuid_bytes = uuid.GetBytes();
+if (uuid_bytes.size() != sizeof(guid) + 4) // CvRecordPdb70
+  return nullptr;
+memcpy(, uuid_bytes.data(), sizeof(guid));
+  }
 
   // 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);
+  auto ec = llvm::identify_magic(pdb_file, magic);
   if (ec || magic != llvm::file_magic::pdb)
 return nullptr;
   std::unique_ptr pdb = loadPDBFile(pdb_file, allocator);
@@ -140,11 +167,11 @@
 llvm::consumeError(expected_info.takeError());
 return nullptr;
   }
-  llvm::codeview::GUID guid;
-  memcpy(, pdb_info->PDB70.Signature, 16);
 
+  // TODO: we need to compare the age, in addition to the GUID
   if (expected_info->getGuid() != guid)
 return nullptr;
+
   return pdb;
 }
 
@@ -521,7 +548,7 @@
   if (!m_index) {
 // Lazily load and match the PDB file, but only do this once.
 std::unique_ptr file_up =
-loadMatchingPDBFile(m_obj_file->GetFileSpec().GetPath(), m_allocator);
+loadMatchingPDBFile(*m_obj_file, m_allocator);
 
 if (!file_up) {
   auto module_sp = m_obj_file->GetModule();
@@ -1459,11 +1486,10 @@
 llvm::Optional modi = 

[Lldb-commits] [PATCH] D55356: Add a method to get the "base" file address of an object file

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added inline comments.



Comment at: include/lldb/Symbol/ObjectFile.h:569
 
+  /// Returns the base file address of an object file (also known as the
+  /// preferred load address or image base address). This is typically the file

"file address" can mean an offset within a container file.

to avoid any confusion I'd use "base address" (or "image base address")


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

https://reviews.llvm.org/D55356



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

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Leonard Mosescu via Phabricator via lldb-commits]

lemo updated this revision to Diff 176853.
lemo marked an inline comment as done.

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

https://reviews.llvm.org/D55142

Files:
  source/Plugins/Process/minidump/MinidumpParser.cpp
  source/Plugins/Process/minidump/MinidumpParser.h
  source/Plugins/Process/minidump/ProcessMinidump.cpp
  source/Plugins/SymbolFile/NativePDB/CompileUnitIndex.cpp
  source/Plugins/SymbolFile/NativePDB/CompileUnitIndex.h
  source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp
  source/Plugins/SymbolFile/NativePDB/PdbIndex.h
  source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp

Index: source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
===
--- source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -30,6 +30,7 @@
 #include "lldb/Symbol/SymbolVendor.h"
 #include "lldb/Symbol/Variable.h"
 #include "lldb/Symbol/VariableList.h"
+#include "lldb/Utility/UUID.h"
 
 #include "llvm/DebugInfo/CodeView/CVRecord.h"
 #include "llvm/DebugInfo/CodeView/CVTypeVisitor.h"
@@ -102,33 +103,59 @@
 }
 
 static std::unique_ptr
-loadMatchingPDBFile(std::string exe_path, llvm::BumpPtrAllocator ) {
-  // Try to find a matching PDB for an EXE.
+loadMatchingPDBFile(lldb_private::ObjectFile *obj_file,
+llvm::BumpPtrAllocator ) {
   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 = std::move(*expected_binary);
+  // Try to find a matching PDB for an EXE.
+  std::string pdb_file;
+  llvm::codeview::GUID guid;
 
-  auto *obj = llvm::dyn_cast(binary.getBinary());
-  if (!obj)
-return nullptr;
-  const llvm::codeview::DebugInfo *pdb_info = nullptr;
+  auto expected_binary = createBinary(obj_file->GetFileSpec().GetPath());
+  if (expected_binary) {
+OwningBinary binary = std::move(*expected_binary);
 
-  // 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;
+auto *obj = llvm::dyn_cast(binary.getBinary());
+if (!obj)
+  return nullptr;
+
+const llvm::codeview::DebugInfo *pdb_info = nullptr;
+llvm::StringRef pdb_file_name_ref;
+if (obj->getDebugPDBInfo(pdb_info, pdb_file_name_ref)) {
+  // If it doesn't have a debug directory, fail.
+  return nullptr;
+}
+pdb_file = pdb_file_name_ref;
+
+// Extract the debug GUID from the debug directory
+memcpy(, pdb_info->PDB70.Signature, sizeof(guid));
+  } else {
+// If the file isn't a PE/COFF executable, look for the PDB in the
+// current directory. This provides a basic solution for debugging minidumps
+// although it's only a stop-gap (until we implement a SymbolVendor)
+//
+// TODO: Implement a SymbolVendor for PDBs.
+//
+llvm::consumeError(expected_binary.takeError());
+pdb_file = obj_file->GetFileSpec()
+   .GetFileNameStrippingExtension()
+   .GetStringRef()
+   .str();
+pdb_file += ".pdb";
+
+// Extract the debug GUID from the ObjectFile
+lldb_private::UUID uuid;
+obj_file->GetUUID();
+auto uuid_bytes = uuid.GetBytes();
+if (uuid_bytes.size() != sizeof(llvm::codeview::GUID) + 4) // CvRecordPdb70
+  return nullptr;
+memcpy(, uuid_bytes.data(), sizeof(guid));
+  }
 
   // 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);
+  auto ec = llvm::identify_magic(pdb_file, magic);
   if (ec || magic != llvm::file_magic::pdb)
 return nullptr;
   std::unique_ptr pdb = loadPDBFile(pdb_file, allocator);
@@ -140,11 +167,11 @@
 llvm::consumeError(expected_info.takeError());
 return nullptr;
   }
-  llvm::codeview::GUID guid;
-  memcpy(, pdb_info->PDB70.Signature, 16);
 
+  // TODO: we need to compare the age, in addition to the GUID
   if (expected_info->getGuid() != guid)
 return nullptr;
+
   return pdb;
 }
 
@@ -521,7 +548,7 @@
   if (!m_index) {
 // Lazily load and match the PDB file, but only do this once.
 std::unique_ptr file_up =
-loadMatchingPDBFile(m_obj_file->GetFileSpec().GetPath(), m_allocator);
+loadMatchingPDBFile(m_obj_file, m_allocator);
 
 if (!file_up) {
   auto module_sp = m_obj_file->GetModule();
@@ -1459,11 +1486,10 @@
 llvm::Optional modi = m_index->GetModuleIndexForVa(file_addr);
 if (!modi)
   return 0;
-CompilandIndexItem *cci = m_index->compilands().GetCompiland(*modi);
-if (!cci)
+if (!m_index->compilands().HasCompilandInfo(*modi))
   return 0;
-
-sc.comp_unit = GetOrCreateCompileUnit(*cci).get();
+

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Leonard Mosescu via Phabricator via lldb-commits]

lemo marked 9 inline comments as done.
lemo added a comment.

In D55142#1316153 , @labath wrote:

> I don't see any tests :(.
>  Also, the three bullet points in the description sound like rather 
> independent pieces of functionality. Would it be possible to split them up 
> into separate patches? That would make things easier to review, particularly 
> for those who don't look at this code very often :).


I agree with both comments. The intention is to add some tests but I wanted to 
get the review out early to surface concerns, if any. I also needed more time 
to investigate a few complex failures uncovered by this change (ex. 
https://bugs.llvm.org/show_bug.cgi?id=39882 and 
https://bugs.llvm.org/show_bug.cgi?id=39897)

Yes, this change can also be split in three parts: the reason it's bundled up 
in this review is that all three parts are required to enable the basic 
functionality (and overall it's a relatively small change). Maybe it was better 
if I sent out the parts separately, but right now I'd like to preserve the 
continuity in the review comments. 
I'm about to send out a new revision and once this review satisfies all the 
comments I'll split it out and send individual reviews.




Comment at: source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp:132-135
+if (so.segment == 0) {
+  lldbassert(so.offset == 0);
+  continue;
+}

zturner wrote:
> What kind of symbols have a segment and offset of 0?
68 | S_LDATA32 [size = 44] `trace_event_unique_atomic140`
 type = 0x0013 (__int64), addr = :
   112 | S_LDATA32 [size = 44] `trace_event_unique_atomic171`
 type = 0x0013 (__int64), addr = :
   156 | S_LDATA32 [size = 44] `trace_event_unique_atomic196`
 type = 0x0013 (__int64), addr = :
...

Tracked by : https://bugs.llvm.org/show_bug.cgi?id=39882




Comment at: source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp:150-157
 // The odds of an error in some function such as GetSegmentAndOffset or
 // MakeVirtualAddress are much higher than the odds of encountering bad
 // debug info, so assert that this item was inserted in the map as opposed
 // to having already been there.
-lldbassert(insert_result.second);
+//
+// TODO: revisit this check since it fires for apparently valid PDBs
+//

zturner wrote:
> If we're going to comment it out, then just delete the code IMO.
> 
> Do you have some llvm-pdbutil output that demonstrates this on a valid PDB?  
> I guess we'd be looking for 2 symbols with the same Virtual Address.  Seems 
> odd to have that, but maybe an example of where it's happening would make it 
> clear whether this is actually a valid case.
I spent more time to investigate this and opened a bug to track it: 
https://bugs.llvm.org/show_bug.cgi?id=39897

With very large PDBs it's not easy to have a definitive verdict so a 2nd 
opinion is welcome, but as I noted in the bug is seems that ICF is one case 
where we can end up with multiple symbols at the same address. This means that 
the data structure needs to be revisited.

For now, I think it's better to suppress the assert so we can exercise the rest 
of the native PDB reader (I updated the comment to point to the new bug I just 
opened)



Comment at: source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:106
 static std::unique_ptr
-loadMatchingPDBFile(std::string exe_path, llvm::BumpPtrAllocator ) {
-  // Try to find a matching PDB for an EXE.
+loadMatchingPDBFile(lldb_private::ObjectFile *obj_file,
+llvm::BumpPtrAllocator ) {

zturner wrote:
> Perhaps `obj_file` should be a reference just to clarify that it can't be 
> null.
That would make sense. Unfortunately, obj_file can't be const 
(ObjectFile::GetUUID() is non const and it's not easy to surgically fix it)

So we'd have to pass by non-const ref, which would read "out-parameter", which 
IMO is more confusing than the non-null part is worth.



Comment at: source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:114-115
 
-  auto *obj = llvm::dyn_cast(binary.getBinary());
-  if (!obj)
-return nullptr;
-  const llvm::codeview::DebugInfo *pdb_info = nullptr;
+  auto expected_binary = createBinary(obj_file->GetFileSpec().GetPath());
+  if (expected_binary) {
+OwningBinary binary = std::move(*expected_binary);

zturner wrote:
> Instead of trying to load it as a PE/COFF fail and then trying something else 
> if it fails (hoping that it's a minidump), perhaps we could use 
> `llvm::identify_magic()` to figure out what it is actually is first.  That 
> function currently cannot detect a Windows minidump, but we coudl teach it to 
> support that.  I think that would make this code more logical.  
> 
> ```
> if (type == exe) {
> } else if (type == minidump) {
> } else {
>   // actual error
> }
> ```
We 

[Lldb-commits] [PATCH] D55214: Introduce ObjectFileBreakpad

[Leonard Mosescu via Phabricator via lldb-commits]

lemo accepted this revision.
lemo added a comment.

Looks like a great start




Comment at: source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp:53
+  static_assert(sizeof(data) == 20, "");
+  if (str.size() < 33 || str.size() > 40)
+return UUID();

these magic integer literals make it hard to follow the intent - what's special 
about 33, 40, 8, 16, ... ? (symbolic constants might help)



Comment at: source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.h:98
+
+private:
+  struct Header {

Nit: I personally prefer not to mix data, type and function members in the same 
"access" section - is there an LLVM/LLDB guideline which requires everything in 
the same place?

If not, can you please add a private section for the destructor, followed by a 
section for the private data members?


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

https://reviews.llvm.org/D55214



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

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Leonard Mosescu via Phabricator via lldb-commits]

lemo created this revision.
lemo added reviewers: labath, zturner, amccarth.
lemo added a project: LLDB.
Herald added subscribers: jfb, abidh, arphaman.

A few changes required to enable the use of the native PDB reader when 
debugging minidumps:

1. Consume PDBs even if the module binary is not available Implementing a 
PlaceholderObjectFile to complement the existing PlaceholderModule

2. Use the minidump exception record if present

3. Relax the PDB file search As noted in the comments this is a stop-gap until 
we add a proper SymbolVendor

Overall this is a modest step towards improving the minidump debugging 
experience. But more importantly,
it enables the basic consumption of PDBs using the NativePDB(TM) reader as a 
foundation to add more functionality incrementally.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D55142

Files:
  source/Plugins/Process/minidump/MinidumpParser.cpp
  source/Plugins/Process/minidump/MinidumpParser.h
  source/Plugins/Process/minidump/ProcessMinidump.cpp
  source/Plugins/SymbolFile/NativePDB/CompileUnitIndex.cpp
  source/Plugins/SymbolFile/NativePDB/CompileUnitIndex.h
  source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp
  source/Plugins/SymbolFile/NativePDB/PdbIndex.h
  source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp

Index: source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
===
--- source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -30,6 +30,7 @@
 #include "lldb/Symbol/SymbolVendor.h"
 #include "lldb/Symbol/Variable.h"
 #include "lldb/Symbol/VariableList.h"
+#include "lldb/Utility/UUID.h"
 
 #include "llvm/DebugInfo/CodeView/CVRecord.h"
 #include "llvm/DebugInfo/CodeView/CVTypeVisitor.h"
@@ -102,33 +103,53 @@
 }
 
 static std::unique_ptr
-loadMatchingPDBFile(std::string exe_path, llvm::BumpPtrAllocator ) {
-  // Try to find a matching PDB for an EXE.
+loadMatchingPDBFile(lldb_private::ObjectFile *obj_file,
+llvm::BumpPtrAllocator ) {
   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 = std::move(*expected_binary);
+  // Try to find a matching PDB for an EXE.
+  std::string pdb_file;
+  llvm::codeview::GUID guid;
 
-  auto *obj = llvm::dyn_cast(binary.getBinary());
-  if (!obj)
-return nullptr;
-  const llvm::codeview::DebugInfo *pdb_info = nullptr;
+  auto expected_binary = createBinary(obj_file->GetFileSpec().GetPath());
+  if (expected_binary) {
+OwningBinary binary = std::move(*expected_binary);
 
-  // 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;
+auto *obj = llvm::dyn_cast(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_name_ref;
+if (obj->getDebugPDBInfo(pdb_info, pdb_file_name_ref))
+  return nullptr;
+pdb_file = pdb_file_name_ref;
+
+// Extract the debug GUID from the debug directory
+memcpy(, pdb_info->PDB70.Signature, sizeof(guid));
+  } else {
+// TODO: If the file isn't a PE/COFF executable, look for the PDB in the
+//  current directly. This provides a basic solution for debugging minidumps
+//  although it's only a stop-gap until we implement a proper SymbolVendor
+//  for PDBs.
+llvm::consumeError(expected_binary.takeError());
+pdb_file =
+obj_file->GetFileSpec().GetFileNameStrippingExtension().GetCString();
+pdb_file += ".pdb";
+
+// Extract the debug GUID from the ObjectFile
+lldb_private::UUID uuid;
+obj_file->GetUUID();
+auto uuid_bytes = uuid.GetBytes();
+lldbassert(uuid_bytes.size() == sizeof(llvm::codeview::GUID) + 4);
+memcpy(, uuid_bytes.data(), sizeof(guid));
+  }
 
   // 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);
+  auto ec = llvm::identify_magic(pdb_file, magic);
   if (ec || magic != llvm::file_magic::pdb)
 return nullptr;
   std::unique_ptr pdb = loadPDBFile(pdb_file, allocator);
@@ -140,11 +161,11 @@
 llvm::consumeError(expected_info.takeError());
 return nullptr;
   }
-  llvm::codeview::GUID guid;
-  memcpy(, pdb_info->PDB70.Signature, 16);
 
+  // TODO: we need to compare the age, in addition to the GUID
   if (expected_info->getGuid() != guid)
 return nullptr;
+
   return pdb;
 }
 
@@ -521,7 +542,7 @@
   if (!m_index) {
 // Lazily load and match the PDB file, but only do this once.
 std::unique_ptr file_up =
-

[Lldb-commits] [PATCH] D54692: [Driver] Use libOption with tablegen.

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added a comment.

I noticed a small problem, this change breaks "lldb -c ". The inline 
comment explains the root cause.

Thanks




Comment at: lldb/trunk/tools/driver/Driver.cpp:310
+  if (args.hasArg(OPT_core)) {
+SBFileSpec file(optarg);
+if (file.Exists()) {

there's a small bug in here: optarg is the global definition, I assume the 
intention was to use the getValue() instead. as is, it breaks "lldb -c 
"

it would be nice to get rid the global optarg as well since it masks these kind 
of mistakes.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54692



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

[Lldb-commits] [PATCH] D54452: [NativePDB] Add support for handling S_CONSTANT records

[Leonard Mosescu via Phabricator via lldb-commits]

lemo accepted this revision.
lemo added inline comments.
This revision is now accepted and ready to land.



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:329
 
+std::pair GetIntegralTypeInfo(TypeIndex ti, TpiStream ) {
+  if (ti.isSimple()) {

Just a suggestion: I'm not a big fan of returning std::pair<>. I'd use a simple 
struct instead.


https://reviews.llvm.org/D54452



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

[Lldb-commits] [PATCH] D54241: Fix bug in printing ValueObjects and in PE/COFF Plugin

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added a comment.

Shouldn't we also handle the general case where raw size < virtual size? (which 
would naturally subsume this fix)




Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:1073
+
+  if (section_offset >= section->GetByteSize())
+return 0;

shouldn't this be a hard error instead of returning 0?





Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:1077
+  size_t bytes_avail = section->GetByteSize() - section_offset;
+  size_t read_size = std::min(dst_len, bytes_avail);
+  ::memset(dst, 0, read_size);

do we really need to return the available "bytes" if the full read size can't 
be fullfiled? (ie. shouldn't that be a hard error instead?)


https://reviews.llvm.org/D54241



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

[Lldb-commits] [PATCH] D53731: [NativePDB] Add the ability to display global variables

[Leonard Mosescu via Phabricator via lldb-commits]

lemo accepted this revision.
lemo added a comment.
This revision is now accepted and ready to land.

looks good. a few comments inline.




Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbSymUid.h:46
+   // due to the debug magic at the beginning of the 
stream.
+  uint64_t global : 1; // True if this is from the globals stream.
+  uint64_t modi : 16;  // For non-global, this is the 0-based index of module.

30 + 1 != 32 - what's going on?



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbSymUid.h:135
+  static PdbSymUid makeGlobalVariableUid(uint32_t offset) {
+PdbSymUid uid;
+uid.m_uid.cu_sym.modi = 0;

 = {}; ? (to avoid uninitialized bits)



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:892
 
+static DWARFExpression MakeGlobalLocationExpression(uint16_t section,
+uint32_t offset,

assert that section > 0 ? (as precondition)



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:913
+
+  uint32_t section_idx = section - 1;
+  if (section_idx >= section_list->GetSize())

comment explaining the - 1 ?



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:936
+  lldb::ValueType scope = eValueTypeInvalid;
+  TypeIndex ti;
+  llvm::StringRef name;

pls explicitly initialize all the locals



Comment at: 
lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:1406
+  TypeSP type_sp = CreateAndCacheType(uid);
+  return &*type_sp;
 }

type_sp->get() seems cleaner / more readable


https://reviews.llvm.org/D53731



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

[Lldb-commits] [PATCH] D53094: [pecoff] Implement ObjectFilePECOFF::GetDependedModules()

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added a comment.

drive by CR notes:

1. does this handle forwarding imports? (it doesn't seem to from a quick glance 
at the code)
2. can you please add, or extend the existing test to cover the transitive 
case? A simple dag would suffice (ex. make both dllA and dllB implicitly import 
dllC)




Comment at: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h:219
 coff_data_dir_import_table = 1,
+coff_data_dir_resource_table,
+coff_data_dir_exception_table,

nit: either explicitly assign values to all or none


https://reviews.llvm.org/D53094



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

[Lldb-commits] [PATCH] D53511: [NativePDB] Support type lookup by name

[Leonard Mosescu via Phabricator via lldb-commits]

lemo accepted this revision.
lemo added a comment.
This revision is now accepted and ready to land.

Looks good to me, minor notes inline.




Comment at: lldb/lit/SymbolFile/NativePDB/tag-types.cpp:131
+
+int main(int argc, char **argv) {
+  Struct S;

zturner wrote:
> lemo wrote:
> > a few suggestions for additional things to cover:
> > - local classes
> > - lambdas
> > - instantiating class and function templates
> >- explicit specializations
> >- for classes, partial specializations
> > - namespaces
> > - anonymous namespace
> Some of these I could probably handle right now.  I tried to keep the scope 
> of the patch to "types which would be named", because it makes it easy to 
> write tests (the test can just do lookup by name, basically a WinDbg `dt` 
> test.).  That makes local classes, lambdas, anonymous namespace, and anything 
> to do with functions out of scope.
k. how about more basic stuff like?
- pointer to pointer
- const/volatile




Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:313
+  case SimpleTypeKind::SByte:
+return "signed chr";
+  case SimpleTypeKind::Character16:

char



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:335
+  case SimpleTypeKind::Int64Quad:
+return "__int64";
+  case SimpleTypeKind::Int32:

int64_t ?



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:344
+  case SimpleTypeKind::UInt64Quad:
+return "unsigned __int64";
+  case SimpleTypeKind::HResult:

unsigned int64_t ?



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:660
+ct = ct.GetPointerType();
+uint32_t pointer_size = 4;
+switch (ti.getSimpleMode()) {

minor: = 0; ? 
(to make any potential mistakes more obvious)



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:687
+  CompilerType ct = m_clang->GetBasicType(bt);
+  size_t size = GetTypeSizeForSimpleKind(ti.getSimpleKind());
+

assert size > 0 ?



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp:189
+}
\ No newline at end of file


nit: add empty line at the end of file




Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h:26
+namespace lldb_private {
+class Type;
+class CompilerType;

nit: vertical space between { and the next line


https://reviews.llvm.org/D53511



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

[Lldb-commits] [PATCH] D53511: [NativePDB] Support type lookup by name

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added a comment.

Nice :)




Comment at: lldb/lit/SymbolFile/NativePDB/tag-types.cpp:87
+// Test single inheritance.
+class Derived : public Class {
+public:

- at least one virtual function + override?
- at least one method returning void?



Comment at: lldb/lit/SymbolFile/NativePDB/tag-types.cpp:94
+  // infinite cycle.
+  Derived 
+

pointer to itself too?



Comment at: lldb/lit/SymbolFile/NativePDB/tag-types.cpp:110
+
+// Test multiple inheritance, as well as protected and private inheritance.
+class Derived2 : protected Class, private Struct {

just an idea - add a virtual inheritance variation?



Comment at: lldb/lit/SymbolFile/NativePDB/tag-types.cpp:131
+
+int main(int argc, char **argv) {
+  Struct S;

a few suggestions for additional things to cover:
- local classes
- lambdas
- instantiating class and function templates
   - explicit specializations
   - for classes, partial specializations
- namespaces
- anonymous namespace


https://reviews.llvm.org/D53511



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

[Lldb-commits] [PATCH] D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added a comment.

Hi Greg, looking at request_evaluate() I noticed that it will evaluate the
string as a lldb command if prefixed by ` .

This is a great feature (it allows building REPL consoles on top of DAP),
but I'm curious how you picked up this convention? For example I believe
that the gdb DAP uses -exec 'command' instead.


Repository:
  rL LLVM

https://reviews.llvm.org/D50365



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

[Lldb-commits] [PATCH] D51604: Terminate debugger if an assert was hit

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added inline comments.



Comment at: source/Utility/LLDBAssert.cpp:31
+"log, and as many details as possible\n";
+  exit(1);
 }

abort() may be a better choice here (exit() path calls a lot of shutdown code 
and it's not safe in a number of cases)


https://reviews.llvm.org/D51604



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

[Lldb-commits] [PATCH] D51442: Don't include the Age in the UUID for CvRecordPdb70 UUID records in minidump files

[Leonard Mosescu via Phabricator via lldb-commits]

lemo accepted this revision.
lemo added a comment.
This revision is now accepted and ready to land.

Looks good (with one inline request for a comment)




Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:85
+  auto arch = GetArchitecture();
+  if (arch.GetTriple().getVendor() == llvm::Triple::Apple)
+return UUID::fromData(pdb70_uuid->Uuid, sizeof(pdb70_uuid->Uuid));

please add a comment describing the situation (breakpad producing debug ids 
which don't match the binaries)


https://reviews.llvm.org/D51442



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

[Lldb-commits] [PATCH] D51442: Don't include the Age in the UUID for CvRecordPdb70 UUID records in minidump files

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added a subscriber: zturner.
lemo added a comment.

I'm curious too: where did the PDB70 age create matching problems?

On a related note, I just noticed that ObjectFilePECOFF::GetUUID() doesn't
have a real implementation (just returns false). How do we extract module
UUID for PE/COFF files?


https://reviews.llvm.org/D51442



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

[Lldb-commits] [PATCH] D51176: Restrict the set of plugins used for ProcessMinidump

[Leonard Mosescu via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rLLDB340578: Restrict the set of plugins used for 
ProcessMinidump (authored by lemo, committed by ).

Repository:
  rLLDB LLDB

https://reviews.llvm.org/D51176

Files:
  source/Plugins/Process/minidump/ProcessMinidump.cpp
  source/Plugins/Process/minidump/ProcessMinidump.h


Index: source/Plugins/Process/minidump/ProcessMinidump.h
===
--- source/Plugins/Process/minidump/ProcessMinidump.h
+++ source/Plugins/Process/minidump/ProcessMinidump.h
@@ -55,7 +55,7 @@
 
   Status DoLoadCore() override;
 
-  DynamicLoader *GetDynamicLoader() override;
+  DynamicLoader *GetDynamicLoader() override { return nullptr; }
 
   ConstString GetPluginName() override;
 
@@ -102,6 +102,8 @@
 
   void ReadModuleList();
 
+  JITLoaderList () override;
+
 private:
   FileSpec m_core_file;
   llvm::ArrayRef m_thread_list;
Index: source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -16,7 +16,7 @@
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/Section.h"
-#include "lldb/Target/DynamicLoader.h"
+#include "lldb/Target/JITLoaderList.h"
 #include "lldb/Target/MemoryRegionInfo.h"
 #include "lldb/Target/SectionLoadList.h"
 #include "lldb/Target/Target.h"
@@ -201,12 +201,6 @@
   return error;
 }
 
-DynamicLoader *ProcessMinidump::GetDynamicLoader() {
-  if (m_dyld_ap.get() == nullptr)
-m_dyld_ap.reset(DynamicLoader::FindPlugin(this, nullptr));
-  return m_dyld_ap.get();
-}
-
 ConstString ProcessMinidump::GetPluginName() { return GetPluginNameStatic(); }
 
 uint32_t ProcessMinidump::GetPluginVersion() { return 1; }
@@ -401,3 +395,14 @@
   }
   return true;
 }
+
+// For minidumps there's no runtime generated code so we don't need 
JITLoader(s)
+// Avoiding them will also speed up minidump loading since JITLoaders normally
+// try to set up symbolic breakpoints, which in turn may force loading more
+// debug information than needed.
+JITLoaderList ::GetJITLoaders() {
+  if (!m_jit_loaders_ap) {
+m_jit_loaders_ap = llvm::make_unique();
+  }
+  return *m_jit_loaders_ap;
+}


Index: source/Plugins/Process/minidump/ProcessMinidump.h
===
--- source/Plugins/Process/minidump/ProcessMinidump.h
+++ source/Plugins/Process/minidump/ProcessMinidump.h
@@ -55,7 +55,7 @@
 
   Status DoLoadCore() override;
 
-  DynamicLoader *GetDynamicLoader() override;
+  DynamicLoader *GetDynamicLoader() override { return nullptr; }
 
   ConstString GetPluginName() override;
 
@@ -102,6 +102,8 @@
 
   void ReadModuleList();
 
+  JITLoaderList () override;
+
 private:
   FileSpec m_core_file;
   llvm::ArrayRef m_thread_list;
Index: source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -16,7 +16,7 @@
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/Section.h"
-#include "lldb/Target/DynamicLoader.h"
+#include "lldb/Target/JITLoaderList.h"
 #include "lldb/Target/MemoryRegionInfo.h"
 #include "lldb/Target/SectionLoadList.h"
 #include "lldb/Target/Target.h"
@@ -201,12 +201,6 @@
   return error;
 }
 
-DynamicLoader *ProcessMinidump::GetDynamicLoader() {
-  if (m_dyld_ap.get() == nullptr)
-m_dyld_ap.reset(DynamicLoader::FindPlugin(this, nullptr));
-  return m_dyld_ap.get();
-}
-
 ConstString ProcessMinidump::GetPluginName() { return GetPluginNameStatic(); }
 
 uint32_t ProcessMinidump::GetPluginVersion() { return 1; }
@@ -401,3 +395,14 @@
   }
   return true;
 }
+
+// For minidumps there's no runtime generated code so we don't need JITLoader(s)
+// Avoiding them will also speed up minidump loading since JITLoaders normally
+// try to set up symbolic breakpoints, which in turn may force loading more
+// debug information than needed.
+JITLoaderList ::GetJITLoaders() {
+  if (!m_jit_loaders_ap) {
+m_jit_loaders_ap = llvm::make_unique();
+  }
+  return *m_jit_loaders_ap;
+}
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D51176: Restrict the set of plugins used for ProcessMinidump

[Leonard Mosescu via Phabricator via lldb-commits]

lemo marked an inline comment as done.
lemo added a comment.

In https://reviews.llvm.org/D51176#1211433, @jingham wrote:

> The other option would be to move the code that populates the section load 
> list into the mini dump dynamic loader.  That has the benefit of keeping this 
> consistent with the other process plugins, but OTOH is just moving code 
> around...


It's an interesting idea, thanks. I'd like to keep the fix small and simple for 
now, but it's worth considering if the current minidump loading path will need 
more flexibility.


https://reviews.llvm.org/D51176



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

[Lldb-commits] [PATCH] D51176: Restrict the set of plugins used for ProcessMinidump

[Leonard Mosescu via Phabricator via lldb-commits]

lemo updated this revision to Diff 162270.
lemo added a comment.

Added the comment requested by zturner


https://reviews.llvm.org/D51176

Files:
  source/Plugins/Process/minidump/ProcessMinidump.cpp
  source/Plugins/Process/minidump/ProcessMinidump.h


Index: source/Plugins/Process/minidump/ProcessMinidump.h
===
--- source/Plugins/Process/minidump/ProcessMinidump.h
+++ source/Plugins/Process/minidump/ProcessMinidump.h
@@ -55,7 +55,7 @@
 
   Status DoLoadCore() override;
 
-  DynamicLoader *GetDynamicLoader() override;
+  DynamicLoader *GetDynamicLoader() override { return nullptr; }
 
   ConstString GetPluginName() override;
 
@@ -102,6 +102,8 @@
 
   void ReadModuleList();
 
+  JITLoaderList () override;
+
 private:
   FileSpec m_core_file;
   llvm::ArrayRef m_thread_list;
Index: source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -16,7 +16,7 @@
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/Section.h"
-#include "lldb/Target/DynamicLoader.h"
+#include "lldb/Target/JITLoaderList.h"
 #include "lldb/Target/MemoryRegionInfo.h"
 #include "lldb/Target/SectionLoadList.h"
 #include "lldb/Target/Target.h"
@@ -201,12 +201,6 @@
   return error;
 }
 
-DynamicLoader *ProcessMinidump::GetDynamicLoader() {
-  if (m_dyld_ap.get() == nullptr)
-m_dyld_ap.reset(DynamicLoader::FindPlugin(this, nullptr));
-  return m_dyld_ap.get();
-}
-
 ConstString ProcessMinidump::GetPluginName() { return GetPluginNameStatic(); }
 
 uint32_t ProcessMinidump::GetPluginVersion() { return 1; }
@@ -401,3 +395,14 @@
   }
   return true;
 }
+
+// For minidumps there's no runtime generated code so we don't need 
JITLoader(s)
+// Avoiding them will also speed up minidump loading since JITLoaders normally
+// try to set up symbolic breakpoints, which in turn may force loading more
+// debug information than needed.
+JITLoaderList ::GetJITLoaders() {
+  if (!m_jit_loaders_ap) {
+m_jit_loaders_ap = llvm::make_unique();
+  }
+  return *m_jit_loaders_ap;
+}


Index: source/Plugins/Process/minidump/ProcessMinidump.h
===
--- source/Plugins/Process/minidump/ProcessMinidump.h
+++ source/Plugins/Process/minidump/ProcessMinidump.h
@@ -55,7 +55,7 @@
 
   Status DoLoadCore() override;
 
-  DynamicLoader *GetDynamicLoader() override;
+  DynamicLoader *GetDynamicLoader() override { return nullptr; }
 
   ConstString GetPluginName() override;
 
@@ -102,6 +102,8 @@
 
   void ReadModuleList();
 
+  JITLoaderList () override;
+
 private:
   FileSpec m_core_file;
   llvm::ArrayRef m_thread_list;
Index: source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -16,7 +16,7 @@
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/Section.h"
-#include "lldb/Target/DynamicLoader.h"
+#include "lldb/Target/JITLoaderList.h"
 #include "lldb/Target/MemoryRegionInfo.h"
 #include "lldb/Target/SectionLoadList.h"
 #include "lldb/Target/Target.h"
@@ -201,12 +201,6 @@
   return error;
 }
 
-DynamicLoader *ProcessMinidump::GetDynamicLoader() {
-  if (m_dyld_ap.get() == nullptr)
-m_dyld_ap.reset(DynamicLoader::FindPlugin(this, nullptr));
-  return m_dyld_ap.get();
-}
-
 ConstString ProcessMinidump::GetPluginName() { return GetPluginNameStatic(); }
 
 uint32_t ProcessMinidump::GetPluginVersion() { return 1; }
@@ -401,3 +395,14 @@
   }
   return true;
 }
+
+// For minidumps there's no runtime generated code so we don't need JITLoader(s)
+// Avoiding them will also speed up minidump loading since JITLoaders normally
+// try to set up symbolic breakpoints, which in turn may force loading more
+// debug information than needed.
+JITLoaderList ::GetJITLoaders() {
+  if (!m_jit_loaders_ap) {
+m_jit_loaders_ap = llvm::make_unique();
+  }
+  return *m_jit_loaders_ap;
+}
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D51176: Restrict the set of plugins used for ProcessMinidump

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added subscribers: bgianfo, clayborg, zturner.
lemo added a comment.

It's an interesting idea, thanks! I don't object moving code around if
there's a strong case for it, but I'd like to keep the fix small and simple
for now, but it's worth considering if the current minidump loading path
will need more flexibility.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D51176



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

[Lldb-commits] [PATCH] D51176: Restrict the set of plugins used for ProcessMinidump

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added a comment.

In https://reviews.llvm.org/D51176#1211358, @clayborg wrote:

> So this might actually work. Take a look around and see what else the dynamic 
> loader is used for and make sure that they won't be needed for anything else. 
> If not, this fix should work, but we should document it.


I took another look and I don't see anything where the dynamic loader is 
required in the context of loading minidumps.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D51176



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

[Lldb-commits] [PATCH] D51176: Restrict the set of plugins used for ProcessMinidump

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added a comment.

In https://reviews.llvm.org/D51176#1211307, @clayborg wrote:

> Dynamic loaders will fill out the section load list in the target that saids 
> "__TEXT" from "/tmp/a.out" was loaded at address 0x120200. So yes they 
> are needed. If the process minidump is manually setting the section load list 
> itself, then there really is no need for a dynamic loader and this fix might 
> work.


That was my original thought: for minidumps we don't really have "load 
addresses", the memory map is recorded in the minidump and that's what we use 
(see PlaceholderModule::CreateImageSection)


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D51176



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

[Lldb-commits] [PATCH] D51176: Restrict the set of plugins used for ProcessMinidump

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added a comment.

> That's not changing the module list, that's changing the loaded section 
> information. It is the dynamic loader's job to figure out what got loaded 
> where, plus your stack trace show this happening after we've selected a 
> plugin, not in the process of negotiating for the plugin. Clearing the 
> section load list before setting to work seems like an appropriate thing for 
> a selected plugin to do.

Correct, sorry - I meant the loaded sections list. This reset is problematic 
for minidumps since we build the loaded sections list while loading the 
minidump.

Is the dynamic loader relevant to loading the minidumps in any way? I'm open to 
any other ides on how to avoid this conflict between the minidump loading and 
the dynamic loader, although I'd strongly prefer to minimize the code path to 
the strictly minimum required. For example this dynamic loader issue is 
particularly unfortunate since it only happens for macOS minidumps. So one 
immediate consequence at least is that it complicates the test matrix (BTW, we 
should probably have at least one macOS & iOS minidumps for LLDB tests, what do 
you think?)


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D51176



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

[Lldb-commits] [PATCH] D51176: Restrict the set of plugins used for ProcessMinidump

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added a comment.

> Dynamic loaders are needed for loading breakpad minidumps that are for MacOSX 
> and iOS processes. They should also be needed for loading any minidumps that 
> have stack traces.

Thanks. I just validated the change against a macOS minidump and everything 
works fine in my local test. What parts of the dynamic loader is relevant to 
minidump loading? (ie. anything specific I should pay attention to?)

> No dynamic loader plug-ins should be affecting the module list during the 
> plug-in loading/selection, if that is happening, that is a bug and it should 
> be fixed.

I agree. Although that's outside the scope of this change if I'm right in that 
we can avoid the dynamic loader plugins completely. Here's an example where the 
DynamicLoaderDarwin is misbehaving
(note the DynamicLoaderDarwin::PrivateInitialize() call to 
Target::ClearAllLoadedSections())

lldb_private::SectionLoadList::Clear(lldb_private::SectionLoadList * this) 
(lldb/source/Target/SectionLoadList.cpp:47)
lldb_private::SectionLoadList::~SectionLoadList(lldb_private::SectionLoadList * 
this) (lldb/include/lldb/Target/SectionLoadList.h:39)
std::_Sp_counted_ptr::_M_dispose(std::_Sp_counted_ptr * this) (libstdcxx/include/bits/shared_ptr_base.h:371)
std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release(std::_Sp_counted_base<__gnu_cxx::_S_atomic>
 * this) (libstdcxx/include/bits/shared_ptr_base.h:149)
std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count(std::__shared_count<__gnu_cxx::_S_atomic>
 * this) (libstdcxx/include/bits/shared_ptr_base.h:664)
std::__shared_ptr::~__shared_ptr(std::__shared_ptr * this) (libstdcxx/include/bits/shared_ptr_base.h:912)
std::shared_ptr::~shared_ptr(std::shared_ptr
 * this) (libstdcxx/include/bits/shared_ptr_base.h:342)
std::pair 
>::~pair(std::pair > * this) 
(libstdcxx/include/bits/stl_pair.h:96)
__gnu_cxx::new_allocator > > 
>::destroy > 
>(__gnu_cxx::new_allocator > > > * this, std::pair > * __p) 
(libstdcxx/include/ext/new_allocator.h:165)
std::allocator_traits > > > 
>::destroy > 
>(std::allocator_traits > > > 
>::allocator_type & __a, std::pair > * __p) 
(libstdcxx/include/bits/alloc_traits.h:539)
std::_Rb_tree >, 
std::_Select1st > >, std::map, std::less, 
std::allocator > > 
>::_ConstCompare >, std::allocator > > 
>::_M_destroy_node(std::_Rb_tree >, 
std::_Select1st > >, std::map, std::less, 
std::allocator > > 
>::_ConstCompare >, std::allocator > > > * this, 
std::_Rb_tree >, 
std::_Select1st > >, std::map, std::less, 
std::allocator > > 
>::_ConstCompare >, std::allocator > > >::_Link_type 
__p) (libstdcxx/include/bits/stl_tree.h:435)
std::_Rb_tree >, 
std::_Select1st > >, std::map, std::less, 
std::allocator > > 
>::_ConstCompare >, std::allocator > > 
>::_M_erase(std::_Rb_tree >, 
std::_Select1st > >, std::map, std::less, 
std::allocator > > 
>::_ConstCompare >, std::allocator > > > * this, 
std::_Rb_tree >, 
std::_Select1st > >, std::map, std::less, 
std::allocator > > 
>::_ConstCompare >, std::allocator > > >::_Link_type 
__x) (libstdcxx/include/bits/stl_tree.h:1283)
std::_Rb_tree >, 
std::_Select1st > >, std::map, std::less, 
std::allocator > > 
>::_ConstCompare >, std::allocator > > 
>::clear(std::_Rb_tree >, 
std::_Select1st > >, std::map, std::less, 
std::allocator > > 
>::_ConstCompare >, std::allocator > > > * this) 
(libstdcxx/include/bits/stl_tree.h:944)
std::map, 
std::less, std::allocator > > >::clear(std::map, std::less, 
std::allocator > > > * this) 
(libstdcxx/include/bits/stl_map.h:862)
lldb_private::SectionLoadHistory::Clear(lldb_private::SectionLoadHistory * 
this) (lldb/source/Target/SectionLoadHistory.cpp:29)
lldb_private::Target::ClearAllLoadedSections(lldb_private::Target * this) 
(lldb/source/Target/Target.cpp:2952)
lldb_private::DynamicLoaderDarwin::PrivateInitialize(lldb_private::DynamicLoaderDarwin
 * this, lldb_private::Process * process) 
(lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp:768)
lldb_private::DynamicLoaderDarwin::DidAttach(lldb_private::DynamicLoaderDarwin 
* this) 
(lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp:73)
lldb_private::Process::LoadCore(lldb_private::Process * this) 
(lldb/source/Target/Process.cpp:2847)
lldb::SBTarget::LoadCore(lldb::SBTarget * this, const char * core_file, 
lldb::SBError & error) (lldb/source/API/SBTarget.cpp:218)


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D51176



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

[Lldb-commits] [PATCH] D51176: Restrict the set of plugins used for ProcessMinidump

[Leonard Mosescu via Phabricator via lldb-commits]

lemo created this revision.
lemo added reviewers: clayborg, zturner.
lemo added a project: LLDB.
Herald added a subscriber: abidh.

1. The dynamic loaders should not be needed for loading minidumps and they may 
create problems (ex. the macOS loader resets the list of loaded modules)
2. In general, the extra plugins can do extraneous work which hurts performance 
(ex. trying to set up implicit breakpoints, which in turn will trigger extra 
symbol loading)


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D51176

Files:
  source/Plugins/Process/minidump/ProcessMinidump.cpp
  source/Plugins/Process/minidump/ProcessMinidump.h


Index: source/Plugins/Process/minidump/ProcessMinidump.h
===
--- source/Plugins/Process/minidump/ProcessMinidump.h
+++ source/Plugins/Process/minidump/ProcessMinidump.h
@@ -55,7 +55,7 @@
 
   Status DoLoadCore() override;
 
-  DynamicLoader *GetDynamicLoader() override;
+  DynamicLoader *GetDynamicLoader() override { return nullptr; }
 
   ConstString GetPluginName() override;
 
@@ -102,6 +102,8 @@
 
   void ReadModuleList();
 
+  JITLoaderList () override;
+
 private:
   FileSpec m_core_file;
   llvm::ArrayRef m_thread_list;
Index: source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -16,7 +16,7 @@
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/Section.h"
-#include "lldb/Target/DynamicLoader.h"
+#include "lldb/Target/JITLoaderList.h"
 #include "lldb/Target/MemoryRegionInfo.h"
 #include "lldb/Target/SectionLoadList.h"
 #include "lldb/Target/Target.h"
@@ -201,12 +201,6 @@
   return error;
 }
 
-DynamicLoader *ProcessMinidump::GetDynamicLoader() {
-  if (m_dyld_ap.get() == nullptr)
-m_dyld_ap.reset(DynamicLoader::FindPlugin(this, nullptr));
-  return m_dyld_ap.get();
-}
-
 ConstString ProcessMinidump::GetPluginName() { return GetPluginNameStatic(); }
 
 uint32_t ProcessMinidump::GetPluginVersion() { return 1; }
@@ -401,3 +395,10 @@
   }
   return true;
 }
+
+JITLoaderList ::GetJITLoaders() {
+  if (!m_jit_loaders_ap) {
+m_jit_loaders_ap = llvm::make_unique();
+  }
+  return *m_jit_loaders_ap;
+}


Index: source/Plugins/Process/minidump/ProcessMinidump.h
===
--- source/Plugins/Process/minidump/ProcessMinidump.h
+++ source/Plugins/Process/minidump/ProcessMinidump.h
@@ -55,7 +55,7 @@
 
   Status DoLoadCore() override;
 
-  DynamicLoader *GetDynamicLoader() override;
+  DynamicLoader *GetDynamicLoader() override { return nullptr; }
 
   ConstString GetPluginName() override;
 
@@ -102,6 +102,8 @@
 
   void ReadModuleList();
 
+  JITLoaderList () override;
+
 private:
   FileSpec m_core_file;
   llvm::ArrayRef m_thread_list;
Index: source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -16,7 +16,7 @@
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/Section.h"
-#include "lldb/Target/DynamicLoader.h"
+#include "lldb/Target/JITLoaderList.h"
 #include "lldb/Target/MemoryRegionInfo.h"
 #include "lldb/Target/SectionLoadList.h"
 #include "lldb/Target/Target.h"
@@ -201,12 +201,6 @@
   return error;
 }
 
-DynamicLoader *ProcessMinidump::GetDynamicLoader() {
-  if (m_dyld_ap.get() == nullptr)
-m_dyld_ap.reset(DynamicLoader::FindPlugin(this, nullptr));
-  return m_dyld_ap.get();
-}
-
 ConstString ProcessMinidump::GetPluginName() { return GetPluginNameStatic(); }
 
 uint32_t ProcessMinidump::GetPluginVersion() { return 1; }
@@ -401,3 +395,10 @@
   }
   return true;
 }
+
+JITLoaderList ::GetJITLoaders() {
+  if (!m_jit_loaders_ap) {
+m_jit_loaders_ap = llvm::make_unique();
+  }
+  return *m_jit_loaders_ap;
+}
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added a subscriber: zturner.
lemo added a comment.

> The LLDB MI plugin didn't work very well as was quite flaky when I tested
>  it a while back.

Just curious, what was the flaky part, the debug adapter or the LLDB MI
interface?


https://reviews.llvm.org/D50365



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

[Lldb-commits] [PATCH] D50274: Misc module/dwarf logging improvements

[Leonard Mosescu via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL339161: Misc module/dwarf logging improvements (authored by 
lemo, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D50274?vs=159432=159551#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D50274

Files:
  lldb/trunk/lit/Modules/compressed-sections.yaml
  lldb/trunk/source/Core/Module.cpp
  lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.cpp
  lldb/trunk/source/Target/Process.cpp

Index: lldb/trunk/source/Core/Module.cpp
===
--- lldb/trunk/source/Core/Module.cpp
+++ lldb/trunk/source/Core/Module.cpp
@@ -162,9 +162,13 @@
   // fill any ivars in so we don't accidentally grab the wrong file later since
   // they don't match...
   ModuleSpec matching_module_spec;
-  if (modules_specs.FindMatchingModuleSpec(module_spec, matching_module_spec) ==
-  0)
+  if (!modules_specs.FindMatchingModuleSpec(module_spec,
+matching_module_spec)) {
+if (log) {
+  log->Printf("Found local object file but the specs didn't match");
+}
 return;
+  }
 
   if (module_spec.GetFileSpec())
 m_mod_time = FileSystem::GetModificationTime(module_spec.GetFileSpec());
Index: lldb/trunk/source/Target/Process.cpp
===
--- lldb/trunk/source/Target/Process.cpp
+++ lldb/trunk/source/Target/Process.cpp
@@ -5842,7 +5842,7 @@
   // that loaded.
 
   // Iterate over a copy of this language runtime list in case the language
-  // runtime ModulesDidLoad somehow causes the language riuntime to be
+  // runtime ModulesDidLoad somehow causes the language runtime to be
   // unloaded.
   LanguageRuntimeCollection language_runtimes(m_language_runtimes);
   for (const auto  : language_runtimes) {
@@ -6095,15 +6095,15 @@
   // For each StructuredDataPlugin, if the plugin handles any of the types in
   // the supported_type_names, map that type name to that plugin. Stop when
   // we've consumed all the type names.
-  // FIXME: should we return an error if there are type names nobody 
+  // FIXME: should we return an error if there are type names nobody
   // supports?
   for (uint32_t plugin_index = 0; !const_type_names.empty(); plugin_index++) {
 auto create_instance =
PluginManager::GetStructuredDataPluginCreateCallbackAtIndex(
plugin_index);
 if (!create_instance)
   break;
-  
+
 // Create the plugin.
 StructuredDataPluginSP plugin_sp = (*create_instance)(*this);
 if (!plugin_sp) {
Index: lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -365,6 +365,12 @@
   // This enables most LLDB functionality involving address-to-module
   // translations (ex. identifing the module for a stack frame PC) and
   // modules/sections commands (ex. target modules list, ...)
+  if (log) {
+log->Printf("Unable to locate the matching object file, creating a "
+"placeholder module for: %s",
+name.getValue().c_str());
+  }
+
   auto placeholder_module =
   std::make_shared(module_spec);
   placeholder_module->CreateImageSection(module, GetTarget());
Index: lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===
--- lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -1390,7 +1390,7 @@
 arch_spec.GetTriple().getOS() == llvm::Triple::OSType::UnknownOS)
   // In case of MIPSR6, the LLDB_NT_OWNER_GNU note is missing for some
   // cases (e.g. compile with -nostdlib) Hence set OS to Linux
-  arch_spec.GetTriple().setOS(llvm::Triple::OSType::Linux); 
+  arch_spec.GetTriple().setOS(llvm::Triple::OSType::Linux);
   }
 }
 
@@ -1494,7 +1494,7 @@
 const uint32_t sub_type = subTypeFromElfHeader(header);
 arch_spec.SetArchitecture(eArchTypeELF, header.e_machine, sub_type,
   header.e_ident[EI_OSABI]);
-
+
 // Validate if it is ok to remove GetOsFromOSABI. Note, that now the OS is
 // determined based on EI_OSABI flag and the info extracted from ELF notes
 // (see RefineModuleDetailsFromNote). However in some cases that still
@@ -3385,8 +3385,6 @@
   if (section->GetObjectFile() != this)
 return section->GetObjectFile()->ReadSectionData(section, section_data);
 
-  Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_MODULES);
-
   size_t result = 

[Lldb-commits] [PATCH] D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added subscribers: clayborg, jingham, jasonmolenda, labath, lemo.
lemo added a comment.

Really cool! Are you planning to add some documentation for it? (set up
instructions, etc...)


https://reviews.llvm.org/D50365



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

[Lldb-commits] [PATCH] D50274: Misc module/dwarf logging improvements

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added inline comments.



Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3413
+section->GetName().GetCString(),
+llvm::toString(Decompressor.takeError()).c_str());
+return 0;

labath wrote:
> lemo wrote:
> > labath wrote:
> > > This needs to be `std::move(Error)`. If you built with asserts enabled 
> > > and hit this line, you would crash because you did not consume the 
> > > `Error` object.
> > Can you please elaborate? std::move is just a cast (and the result of 
> > Error::takeValue() is already an rvalue - the error object has been already 
> > moved into a temporary Error instance)
> The llvm manual 
>  says
> ```
> All Error instances, whether success or failure, must be either checked or 
> moved from (via std::move or a return) before they are destructed. 
> Accidentally discarding an unchecked error will cause a program abort at the 
> point where the unchecked value’s destructor is run, making it easy to 
> identify and fix violations of this rule.
> ...
> Success values are considered checked once they have been tested (by invoking 
> the boolean conversion operator).
> ...
> Failure values are considered checked once a handler for the error type has 
> been activated.
> ```
> The error object created on line 3407 (in the if-declaration) is neither 
> moved from nor has it's handler invoked. You only invoke it's bool operator, 
> which is not enough for it to be considered "checked" if it is in the 
> "failure" state. This means it will assert once it's destructor is executed. 
> By writing `llvm::toString(std::move(Error))`, you will "move" from the 
> object, thereby clearing it. (It also makes sense to print out the error that 
> you have just checked instead of some error from a previous step.)
> 
> Try this pattern out on a toy program to see what happens:
> ```
> if (Error E = make_error("This is an error", 
> inconvertibleErrorCode()))
>   outs() << "I encountered an error but I am not handling it";
> ```
Thanks. I see, I was looking at the previous block. 

> By writing llvm::toString(std::move(Error)), you will "move" from the object, 
> thereby clearing it.

It's a nice contract, although the "move" part was not the problem nor the 
solution in itself (I took a close look at the Error class, it doesn't matter 
how much you move it, someone has to eventually call handleErrors() on it. 
Conveniently, llvm::toString() does it)


https://reviews.llvm.org/D50274



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

[Lldb-commits] [PATCH] D50274: Misc module/dwarf logging improvements

[Leonard Mosescu via Phabricator via lldb-commits]

lemo updated this revision to Diff 159432.
lemo marked an inline comment as done.
lemo added a comment.

Updated the LIT file too


https://reviews.llvm.org/D50274

Files:
  lit/Modules/compressed-sections.yaml
  source/Core/Module.cpp
  source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  source/Plugins/Process/minidump/ProcessMinidump.cpp
  source/Target/Process.cpp

Index: source/Target/Process.cpp
===
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -5842,7 +5842,7 @@
   // that loaded.
 
   // Iterate over a copy of this language runtime list in case the language
-  // runtime ModulesDidLoad somehow causes the language riuntime to be
+  // runtime ModulesDidLoad somehow causes the language runtime to be
   // unloaded.
   LanguageRuntimeCollection language_runtimes(m_language_runtimes);
   for (const auto  : language_runtimes) {
@@ -6095,15 +6095,15 @@
   // For each StructuredDataPlugin, if the plugin handles any of the types in
   // the supported_type_names, map that type name to that plugin. Stop when
   // we've consumed all the type names.
-  // FIXME: should we return an error if there are type names nobody 
+  // FIXME: should we return an error if there are type names nobody
   // supports?
   for (uint32_t plugin_index = 0; !const_type_names.empty(); plugin_index++) {
 auto create_instance =
PluginManager::GetStructuredDataPluginCreateCallbackAtIndex(
plugin_index);
 if (!create_instance)
   break;
-  
+
 // Create the plugin.
 StructuredDataPluginSP plugin_sp = (*create_instance)(*this);
 if (!plugin_sp) {
Index: source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -357,6 +357,12 @@
   // This enables most LLDB functionality involving address-to-module
   // translations (ex. identifing the module for a stack frame PC) and
   // modules/sections commands (ex. target modules list, ...)
+  if (log) {
+log->Printf("Unable to locate the matching object file, creating a "
+"placeholder module for: %s",
+name.getValue().c_str());
+  }
+
   auto placeholder_module =
   std::make_shared(module_spec);
   placeholder_module->CreateImageSection(module, GetTarget());
Index: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===
--- source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -1390,7 +1390,7 @@
 arch_spec.GetTriple().getOS() == llvm::Triple::OSType::UnknownOS)
   // In case of MIPSR6, the LLDB_NT_OWNER_GNU note is missing for some
   // cases (e.g. compile with -nostdlib) Hence set OS to Linux
-  arch_spec.GetTriple().setOS(llvm::Triple::OSType::Linux); 
+  arch_spec.GetTriple().setOS(llvm::Triple::OSType::Linux);
   }
 }
 
@@ -1494,7 +1494,7 @@
 const uint32_t sub_type = subTypeFromElfHeader(header);
 arch_spec.SetArchitecture(eArchTypeELF, header.e_machine, sub_type,
   header.e_ident[EI_OSABI]);
-
+
 // Validate if it is ok to remove GetOsFromOSABI. Note, that now the OS is
 // determined based on EI_OSABI flag and the info extracted from ELF notes
 // (see RefineModuleDetailsFromNote). However in some cases that still
@@ -3385,8 +3385,6 @@
   if (section->GetObjectFile() != this)
 return section->GetObjectFile()->ReadSectionData(section, section_data);
 
-  Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_MODULES);
-
   size_t result = ObjectFile::ReadSectionData(section, section_data);
   if (result == 0 || !section->Test(SHF_COMPRESSED))
 return result;
@@ -3397,20 +3395,27 @@
size_t(section_data.GetByteSize())},
   GetByteOrder() == eByteOrderLittle, GetAddressByteSize() == 8);
   if (!Decompressor) {
-LLDB_LOG_ERROR(log, Decompressor.takeError(),
-   "Unable to initialize decompressor for section {0}",
-   section->GetName());
-return result;
+GetModule()->ReportWarning(
+"Unable to initialize decompressor for section '%s': %s",
+section->GetName().GetCString(),
+llvm::toString(Decompressor.takeError()).c_str());
+section_data.Clear();
+return 0;
   }
+
   auto buffer_sp =
   std::make_shared(Decompressor->getDecompressedSize(), 0);
-  if (auto Error = Decompressor->decompress(
+  if (auto error = Decompressor->decompress(
   {reinterpret_cast(buffer_sp->GetBytes()),
size_t(buffer_sp->GetByteSize())})) {
-LLDB_LOG_ERROR(log, std::move(Error), "Decompression of section {0} failed",
-   section->GetName());
-return result;
+

[Lldb-commits] [PATCH] D50274: Misc module/dwarf logging improvements

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added inline comments.



Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3413
+section->GetName().GetCString(),
+llvm::toString(Decompressor.takeError()).c_str());
+return 0;

labath wrote:
> This needs to be `std::move(Error)`. If you built with asserts enabled and 
> hit this line, you would crash because you did not consume the `Error` object.
Can you please elaborate? std::move is just a cast (and the result of 
Error::takeValue() is already an rvalue - the error object has been already 
moved into a temporary Error instance)


https://reviews.llvm.org/D50274



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

[Lldb-commits] [PATCH] D50274: Misc module/dwarf logging improvements

[Leonard Mosescu via Phabricator via lldb-commits]

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



Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3413
+section->GetName().GetCString());
+return 0;
   }

labath wrote:
> `lit/Modules/compressed-sections.yaml` test will need to be updated to 
> account for the return 0.
compressed-sections.yaml seems to be gated by REQUIRES: zlib, so it would not 
hit the 0-length path, am I wrong?


https://reviews.llvm.org/D50274



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

[Lldb-commits] [PATCH] D50274: Misc module/dwarf logging improvements

[Leonard Mosescu via Phabricator via lldb-commits]

lemo updated this revision to Diff 159368.
lemo added a comment.

Incorporating feedback, thanks.


https://reviews.llvm.org/D50274

Files:
  source/Core/Module.cpp
  source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  source/Plugins/Process/minidump/ProcessMinidump.cpp
  source/Target/Process.cpp

Index: source/Target/Process.cpp
===
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -5842,7 +5842,7 @@
   // that loaded.
 
   // Iterate over a copy of this language runtime list in case the language
-  // runtime ModulesDidLoad somehow causes the language riuntime to be
+  // runtime ModulesDidLoad somehow causes the language runtime to be
   // unloaded.
   LanguageRuntimeCollection language_runtimes(m_language_runtimes);
   for (const auto  : language_runtimes) {
@@ -6095,15 +6095,15 @@
   // For each StructuredDataPlugin, if the plugin handles any of the types in
   // the supported_type_names, map that type name to that plugin. Stop when
   // we've consumed all the type names.
-  // FIXME: should we return an error if there are type names nobody 
+  // FIXME: should we return an error if there are type names nobody
   // supports?
   for (uint32_t plugin_index = 0; !const_type_names.empty(); plugin_index++) {
 auto create_instance =
PluginManager::GetStructuredDataPluginCreateCallbackAtIndex(
plugin_index);
 if (!create_instance)
   break;
-  
+
 // Create the plugin.
 StructuredDataPluginSP plugin_sp = (*create_instance)(*this);
 if (!plugin_sp) {
Index: source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -357,6 +357,12 @@
   // This enables most LLDB functionality involving address-to-module
   // translations (ex. identifing the module for a stack frame PC) and
   // modules/sections commands (ex. target modules list, ...)
+  if (log) {
+log->Printf("Unable to locate the matching object file, creating a "
+"placeholder module for: %s",
+name.getValue().c_str());
+  }
+
   auto placeholder_module =
   std::make_shared(module_spec);
   placeholder_module->CreateImageSection(module, GetTarget());
Index: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===
--- source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -1390,7 +1390,7 @@
 arch_spec.GetTriple().getOS() == llvm::Triple::OSType::UnknownOS)
   // In case of MIPSR6, the LLDB_NT_OWNER_GNU note is missing for some
   // cases (e.g. compile with -nostdlib) Hence set OS to Linux
-  arch_spec.GetTriple().setOS(llvm::Triple::OSType::Linux); 
+  arch_spec.GetTriple().setOS(llvm::Triple::OSType::Linux);
   }
 }
 
@@ -1494,7 +1494,7 @@
 const uint32_t sub_type = subTypeFromElfHeader(header);
 arch_spec.SetArchitecture(eArchTypeELF, header.e_machine, sub_type,
   header.e_ident[EI_OSABI]);
-
+
 // Validate if it is ok to remove GetOsFromOSABI. Note, that now the OS is
 // determined based on EI_OSABI flag and the info extracted from ELF notes
 // (see RefineModuleDetailsFromNote). However in some cases that still
@@ -3385,8 +3385,6 @@
   if (section->GetObjectFile() != this)
 return section->GetObjectFile()->ReadSectionData(section, section_data);
 
-  Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_MODULES);
-
   size_t result = ObjectFile::ReadSectionData(section, section_data);
   if (result == 0 || !section->Test(SHF_COMPRESSED))
 return result;
@@ -3397,20 +3395,25 @@
size_t(section_data.GetByteSize())},
   GetByteOrder() == eByteOrderLittle, GetAddressByteSize() == 8);
   if (!Decompressor) {
-LLDB_LOG_ERROR(log, Decompressor.takeError(),
-   "Unable to initialize decompressor for section {0}",
-   section->GetName());
-return result;
+GetModule()->ReportWarning(
+"Unable to initialize decompressor for section '%s': %s",
+section->GetName().GetCString(),
+llvm::toString(Decompressor.takeError()).c_str());
+return 0;
   }
+
   auto buffer_sp =
   std::make_shared(Decompressor->getDecompressedSize(), 0);
   if (auto Error = Decompressor->decompress(
   {reinterpret_cast(buffer_sp->GetBytes()),
size_t(buffer_sp->GetByteSize())})) {
-LLDB_LOG_ERROR(log, std::move(Error), "Decompression of section {0} failed",
-   section->GetName());
-return result;
+GetModule()->ReportWarning(
+"Decompression of section '%s' failed: %s",
+section->GetName().GetCString(),
+

[Lldb-commits] [PATCH] D50290: Fix a bug in VMRange

[Leonard Mosescu via Phabricator via lldb-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL338949: Fix a bug in VMRange (authored by lemo, committed by 
).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D50290?vs=159147=159156#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D50290

Files:
  lldb/trunk/include/lldb/Utility/VMRange.h
  lldb/trunk/source/Utility/VMRange.cpp


Index: lldb/trunk/source/Utility/VMRange.cpp
===
--- lldb/trunk/source/Utility/VMRange.cpp
+++ lldb/trunk/source/Utility/VMRange.cpp
@@ -24,14 +24,16 @@
 
 bool VMRange::ContainsValue(const VMRange::collection ,
 lldb::addr_t value) {
-  ValueInRangeUnaryPredicate in_range_predicate(value);
-  return llvm::find_if(coll, in_range_predicate) != coll.end();
+  return llvm::find_if(coll, [&](const VMRange ) {
+   return r.Contains(value);
+ }) != coll.end();
 }
 
 bool VMRange::ContainsRange(const VMRange::collection ,
 const VMRange ) {
-  RangeInRangeUnaryPredicate in_range_predicate(range);
-  return llvm::find_if(coll, in_range_predicate) != coll.end();
+  return llvm::find_if(coll, [&](const VMRange ) {
+   return r.Contains(range);
+ }) != coll.end();
 }
 
 void VMRange::Dump(Stream *s, lldb::addr_t offset, uint32_t addr_width) const {
Index: lldb/trunk/include/lldb/Utility/VMRange.h
===
--- lldb/trunk/include/lldb/Utility/VMRange.h
+++ lldb/trunk/include/lldb/Utility/VMRange.h
@@ -87,24 +87,6 @@
   void Dump(Stream *s, lldb::addr_t base_addr = 0,
 uint32_t addr_width = 8) const;
 
-  class ValueInRangeUnaryPredicate {
-  public:
-ValueInRangeUnaryPredicate(lldb::addr_t value) : _value(value) {}
-bool operator()(const VMRange ) const {
-  return range.Contains(_value);
-}
-lldb::addr_t _value;
-  };
-
-  class RangeInRangeUnaryPredicate {
-  public:
-RangeInRangeUnaryPredicate(VMRange range) : _range(range) {}
-bool operator()(const VMRange ) const {
-  return range.Contains(_range);
-}
-const VMRange &_range;
-  };
-
   static bool ContainsValue(const VMRange::collection ,
 lldb::addr_t value);
 


Index: lldb/trunk/source/Utility/VMRange.cpp
===
--- lldb/trunk/source/Utility/VMRange.cpp
+++ lldb/trunk/source/Utility/VMRange.cpp
@@ -24,14 +24,16 @@
 
 bool VMRange::ContainsValue(const VMRange::collection ,
 lldb::addr_t value) {
-  ValueInRangeUnaryPredicate in_range_predicate(value);
-  return llvm::find_if(coll, in_range_predicate) != coll.end();
+  return llvm::find_if(coll, [&](const VMRange ) {
+   return r.Contains(value);
+ }) != coll.end();
 }
 
 bool VMRange::ContainsRange(const VMRange::collection ,
 const VMRange ) {
-  RangeInRangeUnaryPredicate in_range_predicate(range);
-  return llvm::find_if(coll, in_range_predicate) != coll.end();
+  return llvm::find_if(coll, [&](const VMRange ) {
+   return r.Contains(range);
+ }) != coll.end();
 }
 
 void VMRange::Dump(Stream *s, lldb::addr_t offset, uint32_t addr_width) const {
Index: lldb/trunk/include/lldb/Utility/VMRange.h
===
--- lldb/trunk/include/lldb/Utility/VMRange.h
+++ lldb/trunk/include/lldb/Utility/VMRange.h
@@ -87,24 +87,6 @@
   void Dump(Stream *s, lldb::addr_t base_addr = 0,
 uint32_t addr_width = 8) const;
 
-  class ValueInRangeUnaryPredicate {
-  public:
-ValueInRangeUnaryPredicate(lldb::addr_t value) : _value(value) {}
-bool operator()(const VMRange ) const {
-  return range.Contains(_value);
-}
-lldb::addr_t _value;
-  };
-
-  class RangeInRangeUnaryPredicate {
-  public:
-RangeInRangeUnaryPredicate(VMRange range) : _range(range) {}
-bool operator()(const VMRange ) const {
-  return range.Contains(_range);
-}
-const VMRange &_range;
-  };
-
   static bool ContainsValue(const VMRange::collection ,
 lldb::addr_t value);
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D50290: Fix a bug in VMRange

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added subscribers: bgianfo, labath, penryu, teemperor, zturner.
lemo added a comment.

Thanks Zach. I can't find llvm::contains() though, any pointers to it?


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D50290



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

[Lldb-commits] [PATCH] D49415: Add unit tests for VMRange

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added subscribers: teemperor, lemo.
lemo added a comment.

The new test cases did catch a real bug:

[ RUN  ] VMRange.CollectionContains
llvm/src/tools/lldb/unittests/Utility/VMRangeTest.cpp:146: Failure
Value of: VMRange::ContainsRange(collection, VMRange(0x100, 0x104))

  Actual: false

Expected: true

class RangeInRangeUnaryPredicate {
public:
RangeInRangeUnaryPredicate(VMRange range) : _range(range) {} // note that
_range binds to a temporary!
bool operator()(const VMRange ) const {
return range.Contains(_range);
}
const VMRange &_range;
};

I just sent out a review for the fix (https://reviews.llvm.org/D50290)


Repository:
  rL LLVM

https://reviews.llvm.org/D49415



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

[Lldb-commits] [PATCH] D50290: Fix a bug in VMRange

[Leonard Mosescu via Phabricator via lldb-commits]

lemo created this revision.
lemo added reviewers: zturner, labath, teemperor.
lemo added a project: LLDB.

I noticed a suspicious failure:

[ RUN  ] VMRange.CollectionContains
llvm/src/tools/lldb/unittests/Utility/VMRangeTest.cpp:146: Failure
Value of: VMRange::ContainsRange(collection, VMRange(0x100, 0x104))

  Actual: false

Expected: true

Looking at the code, it is a very real bug:

  class RangeInRangeUnaryPredicate {
  public:
RangeInRangeUnaryPredicate(VMRange range) : _range(range) {} // note that 
_range binds to a temporary!
bool operator()(const VMRange ) const {
  return range.Contains(_range);
}
const VMRange &_range;
  };

This change fixes the bug.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D50290

Files:
  include/lldb/Utility/VMRange.h
  source/Utility/VMRange.cpp


Index: source/Utility/VMRange.cpp
===
--- source/Utility/VMRange.cpp
+++ source/Utility/VMRange.cpp
@@ -24,14 +24,16 @@
 
 bool VMRange::ContainsValue(const VMRange::collection ,
 lldb::addr_t value) {
-  ValueInRangeUnaryPredicate in_range_predicate(value);
-  return llvm::find_if(coll, in_range_predicate) != coll.end();
+  return llvm::find_if(coll, [&](const VMRange ) {
+   return r.Contains(value);
+ }) != coll.end();
 }
 
 bool VMRange::ContainsRange(const VMRange::collection ,
 const VMRange ) {
-  RangeInRangeUnaryPredicate in_range_predicate(range);
-  return llvm::find_if(coll, in_range_predicate) != coll.end();
+  return llvm::find_if(coll, [&](const VMRange ) {
+   return r.Contains(range);
+ }) != coll.end();
 }
 
 void VMRange::Dump(Stream *s, lldb::addr_t offset, uint32_t addr_width) const {
Index: include/lldb/Utility/VMRange.h
===
--- include/lldb/Utility/VMRange.h
+++ include/lldb/Utility/VMRange.h
@@ -87,24 +87,6 @@
   void Dump(Stream *s, lldb::addr_t base_addr = 0,
 uint32_t addr_width = 8) const;
 
-  class ValueInRangeUnaryPredicate {
-  public:
-ValueInRangeUnaryPredicate(lldb::addr_t value) : _value(value) {}
-bool operator()(const VMRange ) const {
-  return range.Contains(_value);
-}
-lldb::addr_t _value;
-  };
-
-  class RangeInRangeUnaryPredicate {
-  public:
-RangeInRangeUnaryPredicate(VMRange range) : _range(range) {}
-bool operator()(const VMRange ) const {
-  return range.Contains(_range);
-}
-const VMRange &_range;
-  };
-
   static bool ContainsValue(const VMRange::collection ,
 lldb::addr_t value);
 


Index: source/Utility/VMRange.cpp
===
--- source/Utility/VMRange.cpp
+++ source/Utility/VMRange.cpp
@@ -24,14 +24,16 @@
 
 bool VMRange::ContainsValue(const VMRange::collection ,
 lldb::addr_t value) {
-  ValueInRangeUnaryPredicate in_range_predicate(value);
-  return llvm::find_if(coll, in_range_predicate) != coll.end();
+  return llvm::find_if(coll, [&](const VMRange ) {
+   return r.Contains(value);
+ }) != coll.end();
 }
 
 bool VMRange::ContainsRange(const VMRange::collection ,
 const VMRange ) {
-  RangeInRangeUnaryPredicate in_range_predicate(range);
-  return llvm::find_if(coll, in_range_predicate) != coll.end();
+  return llvm::find_if(coll, [&](const VMRange ) {
+   return r.Contains(range);
+ }) != coll.end();
 }
 
 void VMRange::Dump(Stream *s, lldb::addr_t offset, uint32_t addr_width) const {
Index: include/lldb/Utility/VMRange.h
===
--- include/lldb/Utility/VMRange.h
+++ include/lldb/Utility/VMRange.h
@@ -87,24 +87,6 @@
   void Dump(Stream *s, lldb::addr_t base_addr = 0,
 uint32_t addr_width = 8) const;
 
-  class ValueInRangeUnaryPredicate {
-  public:
-ValueInRangeUnaryPredicate(lldb::addr_t value) : _value(value) {}
-bool operator()(const VMRange ) const {
-  return range.Contains(_value);
-}
-lldb::addr_t _value;
-  };
-
-  class RangeInRangeUnaryPredicate {
-  public:
-RangeInRangeUnaryPredicate(VMRange range) : _range(range) {}
-bool operator()(const VMRange ) const {
-  return range.Contains(_range);
-}
-const VMRange &_range;
-  };
-
   static bool ContainsValue(const VMRange::collection ,
 lldb::addr_t value);
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D50274: Misc module/dwarf logging improvements

[Leonard Mosescu via Phabricator via lldb-commits]

lemo created this revision.
lemo added reviewers: labath, clayborg.
lemo added a project: LLDB.
Herald added subscribers: JDevlieghere, arichardson, aprantl, emaste.
Herald added a reviewer: espindola.

This change improves the logging for the lldb.module category to note a few 
interesting cases:

1. Local object file found, but specs not matching
2. Local object file not found, using a placeholder module

The logging for failing to load compressed dwarf symbols is also improved.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D50274

Files:
  source/Core/Module.cpp
  source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  source/Plugins/Process/minidump/ProcessMinidump.cpp
  source/Target/Process.cpp

Index: source/Target/Process.cpp
===
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -5842,7 +5842,7 @@
   // that loaded.
 
   // Iterate over a copy of this language runtime list in case the language
-  // runtime ModulesDidLoad somehow causes the language riuntime to be
+  // runtime ModulesDidLoad somehow causes the language runtime to be
   // unloaded.
   LanguageRuntimeCollection language_runtimes(m_language_runtimes);
   for (const auto  : language_runtimes) {
@@ -6095,15 +6095,15 @@
   // For each StructuredDataPlugin, if the plugin handles any of the types in
   // the supported_type_names, map that type name to that plugin. Stop when
   // we've consumed all the type names.
-  // FIXME: should we return an error if there are type names nobody 
+  // FIXME: should we return an error if there are type names nobody
   // supports?
   for (uint32_t plugin_index = 0; !const_type_names.empty(); plugin_index++) {
 auto create_instance =
PluginManager::GetStructuredDataPluginCreateCallbackAtIndex(
plugin_index);
 if (!create_instance)
   break;
-  
+
 // Create the plugin.
 StructuredDataPluginSP plugin_sp = (*create_instance)(*this);
 if (!plugin_sp) {
Index: source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -357,6 +357,12 @@
   // This enables most LLDB functionality involving address-to-module
   // translations (ex. identifing the module for a stack frame PC) and
   // modules/sections commands (ex. target modules list, ...)
+  if (log) {
+log->Printf("Unable to locate the matching object file, creating a "
+"placeholder module for: %s",
+name.getValue().c_str());
+  }
+
   auto placeholder_module =
   std::make_shared(module_spec);
   placeholder_module->CreateImageSection(module, GetTarget());
Index: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===
--- source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -1390,7 +1390,7 @@
 arch_spec.GetTriple().getOS() == llvm::Triple::OSType::UnknownOS)
   // In case of MIPSR6, the LLDB_NT_OWNER_GNU note is missing for some
   // cases (e.g. compile with -nostdlib) Hence set OS to Linux
-  arch_spec.GetTriple().setOS(llvm::Triple::OSType::Linux); 
+  arch_spec.GetTriple().setOS(llvm::Triple::OSType::Linux);
   }
 }
 
@@ -1494,7 +1494,7 @@
 const uint32_t sub_type = subTypeFromElfHeader(header);
 arch_spec.SetArchitecture(eArchTypeELF, header.e_machine, sub_type,
   header.e_ident[EI_OSABI]);
-
+
 // Validate if it is ok to remove GetOsFromOSABI. Note, that now the OS is
 // determined based on EI_OSABI flag and the info extracted from ELF notes
 // (see RefineModuleDetailsFromNote). However in some cases that still
@@ -3397,19 +3397,20 @@
size_t(section_data.GetByteSize())},
   GetByteOrder() == eByteOrderLittle, GetAddressByteSize() == 8);
   if (!Decompressor) {
-LLDB_LOG_ERROR(log, Decompressor.takeError(),
-   "Unable to initialize decompressor for section {0}",
-   section->GetName());
-return result;
+GetModule()->ReportWarning(
+"Unable to initialize decompressor for section '%s'",
+section->GetName().GetCString());
+return 0;
   }
   auto buffer_sp =
   std::make_shared(Decompressor->getDecompressedSize(), 0);
   if (auto Error = Decompressor->decompress(
   {reinterpret_cast(buffer_sp->GetBytes()),
size_t(buffer_sp->GetByteSize())})) {
-LLDB_LOG_ERROR(log, std::move(Error), "Decompression of section {0} failed",
-   section->GetName());
-return result;
+GetModule()->ReportWarning(
+"Decompression of section '%s' failed",
+section->GetName().GetCString());
+return 0;
   }
   

[Lldb-commits] [PATCH] D49750: Add support for ARM and ARM64 breakpad generated minidump files.

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added a comment.

Thanks Greg, looks good to me (a couple of inline comments left at your 
discretion)




Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:15
 // Other libraries and framework includes
+//#include "lldb/Core/Architecture.h"
 #include "lldb/Core/Module.h"

it this set for removal?



Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp:195
+// ARM general purpose registers.
+const uint32_t g_gpr_regnums[] = {
+  reg_r0,  reg_r1,  reg_r2,  reg_r3,  reg_r4, reg_r5, reg_r6, reg_r7,

clayborg wrote:
> lemo wrote:
> > use std::array for these kind of static arrays? (debug bounds checks, easy 
> > access to the static size, ...)
> Tried it but it introduces a global constructor. We try to avoid those.
We shouldn't have a dynamic initializer: that's strange, if that's the case we 
have a compiler bug on our hands. A quick experiment indicates that even with 
-O0 recent clang/llvm do the right thing: https://godbolt.org/g/NMUFLP

Is the problem only with arrays of RegisterInfo structs? If that's the case the 
cause is RegisterInfo itself and std::array should not make a difference (ie. 
we'd see the dynamic initializer even with plain C arrays)


https://reviews.llvm.org/D49750



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

[Lldb-commits] [PATCH] D49750: Add support for ARM and ARM64 breakpad generated minidump files.

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added subscribers: clayborg, labath, markmentovai, t.p.northover, zturner.
lemo added a comment.

FYI, Breakpad & Crashpad will start generating the Microsoft flavor of ARM
minidumps soon.


https://reviews.llvm.org/D49750



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

[Lldb-commits] [PATCH] D49685: LLDB does not respect platform sysroot when loading core on Linux

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added a comment.

> I never *ran LLDB tests*, not sure where they are and what they are.

I hope you're planning to look into this before submitting the change :)


https://reviews.llvm.org/D49685



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

[Lldb-commits] [PATCH] D49750: Add support for ARM and ARM64 breakpad generated minidump files.

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added inline comments.



Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp:51
+  reg_fpscr,
+  reg_d0,   reg_d1,  reg_d2,  reg_d3,  reg_d4,  reg_d5,  reg_d6,  reg_d7,
+  reg_d8,   reg_d9, reg_d10, reg_d11, reg_d12, reg_d13, reg_d14, reg_d15,

Pavel's comment reminded me: what about the S registers (32bit fp) and Q 
registers (128bit Neon)?


https://reviews.llvm.org/D49750



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

[Lldb-commits] [PATCH] D49750: Add support for ARM and ARM64 breakpad generated minidump files.

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added inline comments.



Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:216
+  if (csd_version.find("Linux") != std::string::npos)
+triple.setOS(llvm::Triple::OSType::Linux);
+  break;

clayborg wrote:
> I have run into some minidump files that don't have linux set corrreclty as 
> the OS, but they do have "linux" in the description. So this is to handle 
> those cases. I have told the people that are generating these about the issue 
> and they will fix it, but we have minidump files out in the wild that don't 
> set linux as the OS correctly.
1. any idea where this minidumps originate from?
2. should we raise some kind of signal when we go down this path? print an 
warning or at least verbose logging?



Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:179
+auto platform_sp = target.GetPlatform();
+if (platform_sp && !platform_sp->IsCompatibleArchitecture(arch, false, 
nullptr)) {
+  ArchSpec platform_arch;

clayborg wrote:
> lemo wrote:
> > shouldn't this be a check resulting in an error? why do we need to make 
> > this implicit adjustment here?
> By default the "host" platform is selected and it was incorrectly being used 
> along with _any_ triple. So we need to adjust the platform if the host 
> platform isn't compatible. The platform being set correctly helps with many 
> things during a debug session like finding symbols and much more.
Nice catch/fix in that case!

Just curious: It still seems a bit surprising to me to have the target mutated 
by the process object - is that how it's normally done in other cases?



https://reviews.llvm.org/D49750



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

[Lldb-commits] [PATCH] D49750: Add support for ARM and ARM64 breakpad generated minidump files.

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added a comment.

Looks really good, a few comments inline.

This may not big a big deal, but the part about FP (and Apple vs. non-Apple) is 
confusing: the FP is a pretty weak convention, and in some ABIs is not actually 
"fixed" (ex. FP can be either https://reviews.llvm.org/source/openmp/ or 
https://reviews.llvm.org/source/libunwind/, which in turn can be used as GPRs). 
If Apple sticks to a strict usage it makes sense to name it but then maybe we 
should just not name any FP for non-Apple?




Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:149-150
 ArchSpec MinidumpParser::GetArchitecture() {
-  ArchSpec arch_spec;
+  if (m_arch.IsValid())
+return m_arch;
   const MinidumpSystemInfo *system_info = GetSystemInfo();

instead of useing m_arch as a cache I'd explicitly initialize it in 
MinidumpParser::Initialize() 



Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:212-216
+  std::string csd_version;
+  if (auto s = GetMinidumpString(system_info->csd_version_rva))
+csd_version = *s;
+  if (csd_version.find("Linux") != std::string::npos)
+triple.setOS(llvm::Triple::OSType::Linux);

why is this needed when we have MinidumpOSPlatform::Linux ?



Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:179
+auto platform_sp = target.GetPlatform();
+if (platform_sp && !platform_sp->IsCompatibleArchitecture(arch, false, 
nullptr)) {
+  ArchSpec platform_arch;

shouldn't this be a check resulting in an error? why do we need to make this 
implicit adjustment here?



Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp:192
+
+static size_t k_num_reg_infos = llvm::array_lengthof(g_reg_infos);
+

constexpr?



Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp:195
+// ARM general purpose registers.
+const uint32_t g_gpr_regnums[] = {
+  reg_r0,  reg_r1,  reg_r2,  reg_r3,  reg_r4, reg_r5, reg_r6, reg_r7,

use std::array for these kind of static arrays? (debug bounds checks, easy 
access to the static size, ...)



Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp:225
+  m_regs.context_flags = data.GetU32();
+  for (unsigned i=0; i<16; ++i)
+m_regs.r[i] = data.GetU32();

symbolic constants or use the array size?



Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp:231
+m_regs.d[i] = data.GetU64();
+  assert(k_num_regs == k_num_reg_infos_apple);
+  assert(k_num_regs == k_num_reg_infos);

lldb_assert ?



Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp:242
+}
+size_t RegisterContextMinidump_ARM::GetRegisterCount() {
+  return k_num_regs;

not a big deal, but this kind of accessors for constants can be constexpr 
themselves



Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp:248
+RegisterContextMinidump_ARM::GetRegisterInfoAtIndex(size_t reg) {
+  if (reg < k_num_reg_infos)
+return _reg_infos[reg];

failfast if out of bounds? who'd pass an invalid index and expect a meaninful 
result?
(btw, std::array would provide the debug checks if that's all that we want)



Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp:281
+bool RegisterContextMinidump_ARM::WriteRegister(
+const RegisterInfo *reg_info, const RegisterValue _value) {
+  return false;

remove unused parameter name or [[maybe_unused]]



Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_ARM.h:82
+Context m_regs;
+RegisterInfo *m_reg_infos;
+size_t m_num_reg_infos;

 = nullptr;



Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_ARM.h:83
+RegisterInfo *m_reg_infos;
+size_t m_num_reg_infos;
+};

= 0;



Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.h:37
+  
+  ~RegisterContextMinidump_ARM64() override {}
+  

=default instead of {} ?


https://reviews.llvm.org/D49750



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

[Lldb-commits] [PATCH] D49685: LLDB does not respect platform sysroot when loading core on Linux

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added subscribers: clayborg, EugeneBi, labath, lemo.
lemo added a comment.

> The problem is that shared libraries differ on these machines and
>  LLDB either fails to load some libraries *or loads wrong ones*.

Not finding the modules is not surprising but the latter (loading the wrong
modules) is a bit concerning. Do you know why the module build-id is not
used when searching for the local binary?


Repository:
  rL LLVM

https://reviews.llvm.org/D49685



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

[Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added a comment.

Great timing! ARM support would be most welcome. Are you planning to
support the Breakpad flavor or ARM minidumps or the Microsoft one? (Mark
Mentovai just reminded me that the ARM support was added independently and
some of the structures are different)

Regarding the invalid minidumps, I used my favorite hex editor
https://www.sweetscape.com/010editor/ to manually corrupt the minidump
streams directory.


Repository:
  rL LLVM

https://reviews.llvm.org/D49202



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

[Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added subscribers: amccarth, bgianfo, labath, penryu.
lemo added a comment.

The problem is not returning an error from Minidump::Create() - if that was
the case this could easily be improved indeed. The two-phase initialization
is a consequence of the LLDB plugin lookup:

1. Target::CreateProcess() calls Process::FindPlugin()
2. ProcessMinidump::CreateInstance() then has to inspect the core file to

see if it's a minidump
2b. ... if it is a minidump, we need to create a ProcessMinidump (which
calls MinidumpParser::Create())

3. once the plugin is selected, Process::LoadCore() is finally called and

this the earliest we can do minidump-specific error checking

Note that at step 2b. we don't have a way to propagate the error since
we're just doing the plugin lookup (the most we can pass on the lookup to
the rest of the plugins). We can't easily defer the
MinidumpParser::Create() as step 2b either since that only morphs into a
different kind of two-stage initialization (having a ProcessMinidump w/o a
parser).

I agree that it would be nicer with a one step initialization but overall
changing the LLDB plugin lookup is too intrusive for the relatively small
benefit. If you have any suggestions I'd love to hear them.


Repository:
  rL LLVM

https://reviews.llvm.org/D49202



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

[Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

[Leonard Mosescu via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL336918: Restructure the minidump loading path and add early 
 explicit consistency… (authored by lemo, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D49202?vs=155080=155212#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D49202

Files:
  lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp
  lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.h
  lldb/trunk/source/Plugins/Process/minidump/MinidumpTypes.cpp
  lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.cpp
  lldb/trunk/unittests/Process/minidump/CMakeLists.txt
  lldb/trunk/unittests/Process/minidump/Inputs/bad_duplicate_streams.dmp
  lldb/trunk/unittests/Process/minidump/Inputs/bad_overlapping_streams.dmp
  lldb/trunk/unittests/Process/minidump/MinidumpParserTest.cpp

Index: lldb/trunk/unittests/Process/minidump/CMakeLists.txt
===
--- lldb/trunk/unittests/Process/minidump/CMakeLists.txt
+++ lldb/trunk/unittests/Process/minidump/CMakeLists.txt
@@ -17,6 +17,8 @@
linux-x86_64.dmp
linux-x86_64_not_crashed.dmp
fizzbuzz_no_heap.dmp
-   fizzbuzz_wow64.dmp)
+   fizzbuzz_wow64.dmp
+   bad_duplicate_streams.dmp
+   bad_overlapping_streams.dmp)
 
 add_unittest_inputs(LLDBMinidumpTests "${test_inputs}")
Index: lldb/trunk/unittests/Process/minidump/MinidumpParserTest.cpp
===
--- lldb/trunk/unittests/Process/minidump/MinidumpParserTest.cpp
+++ lldb/trunk/unittests/Process/minidump/MinidumpParserTest.cpp
@@ -38,16 +38,32 @@
 
 class MinidumpParserTest : public testing::Test {
 public:
-  void SetUpData(const char *minidump_filename,
- uint64_t load_size = UINT64_MAX) {
+  void SetUpData(const char *minidump_filename) {
 std::string filename = GetInputFilePath(minidump_filename);
-auto BufferPtr = DataBufferLLVM::CreateSliceFromPath(filename, load_size, 0);
+auto BufferPtr = DataBufferLLVM::CreateSliceFromPath(filename, -1, 0);
+ASSERT_NE(BufferPtr, nullptr);
+llvm::Optional optional_parser =
+MinidumpParser::Create(BufferPtr);
+ASSERT_TRUE(optional_parser.hasValue());
+parser.reset(new MinidumpParser(optional_parser.getValue()));
+ASSERT_GT(parser->GetData().size(), 0UL);
+auto result = parser->Initialize();
+ASSERT_TRUE(result.Success()) << result.AsCString();
+  }
+
+  void InvalidMinidump(const char *minidump_filename, uint64_t load_size) {
+std::string filename = GetInputFilePath(minidump_filename);
+auto BufferPtr =
+DataBufferLLVM::CreateSliceFromPath(filename, load_size, 0);
+ASSERT_NE(BufferPtr, nullptr);
 
 llvm::Optional optional_parser =
 MinidumpParser::Create(BufferPtr);
 ASSERT_TRUE(optional_parser.hasValue());
 parser.reset(new MinidumpParser(optional_parser.getValue()));
 ASSERT_GT(parser->GetData().size(), 0UL);
+auto result = parser->Initialize();
+ASSERT_TRUE(result.Fail());
   }
 
   std::unique_ptr parser;
@@ -68,12 +84,15 @@
   EXPECT_EQ(1232UL, context.size());
 }
 
-TEST_F(MinidumpParserTest, GetThreadsTruncatedFile) {
-  SetUpData("linux-x86_64.dmp", 200);
-  llvm::ArrayRef thread_list;
+TEST_F(MinidumpParserTest, TruncatedMinidumps) {
+  InvalidMinidump("linux-x86_64.dmp", 32);
+  InvalidMinidump("linux-x86_64.dmp", 100);
+  InvalidMinidump("linux-x86_64.dmp", 20 * 1024);
+}
 
-  thread_list = parser->GetThreads();
-  ASSERT_EQ(0UL, thread_list.size());
+TEST_F(MinidumpParserTest, IllFormedMinidumps) {
+  InvalidMinidump("bad_duplicate_streams.dmp", -1);
+  InvalidMinidump("bad_overlapping_streams.dmp", -1);
 }
 
 TEST_F(MinidumpParserTest, GetArchitecture) {
Index: lldb/trunk/source/Plugins/Process/minidump/MinidumpTypes.cpp
===
--- lldb/trunk/source/Plugins/Process/minidump/MinidumpTypes.cpp
+++ lldb/trunk/source/Plugins/Process/minidump/MinidumpTypes.cpp
@@ -33,9 +33,6 @@
   version != MinidumpHeaderConstants::Version)
 return nullptr;
 
-  // TODO check for max number of streams ?
-  // TODO more sanity checks ?
-
   return header;
 }
 
Index: lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.h
===
--- lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.h
+++ lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.h
@@ -89,13 +89,15 @@
 
   llvm::Optional GetMemoryRegionInfo(lldb::addr_t);
 
+  // Perform consistency checks and initialize internal data structures
+  Status Initialize();
+
+private:
+  MinidumpParser(const lldb::DataBufferSP _buf_sp);
+
 private:
   lldb::DataBufferSP m_data_sp;
   llvm::DenseMap m_directory_map;
-
-  MinidumpParser(
-  const lldb::DataBufferSP _buf_sp,
-  llvm::DenseMap &_map);
 };
 
 } // end namespace minidump

[Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added a comment.

Thanks Adrian for the thorough review.




Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:52
 ASSERT_GT(parser->GetData().size(), 0UL);
+auto result = parser->Initialize();
+if (expect_failure)

amccarth wrote:
> lemo wrote:
> > amccarth wrote:
> > > lemo wrote:
> > > > amccarth wrote:
> > > > > I would rather see this function return the result of the Initialize 
> > > > > and let the individual tests check the expectation explicitly.
> > > > > 
> > > > > I know that will lead to a little bit of duplication in the tests, 
> > > > > but it will make the individual tests easier to understand in 
> > > > > isolation.  It also makes it possible for each test to decide whether 
> > > > > to ASSERT or EXPECT.  And it eliminates the need for the bool 
> > > > > parameter which is hard to decipher at the call sites.
> > > > Good idea, although gunit doesn't let us ASSERT in non-void returning 
> > > > functions.
> > > > 
> > > > I agree that the bool parameter is ugly, so I created a separate 
> > > > TruncateMinidump() helper (which cleans up the SetUpData since the 
> > > > load_size was only used for truncation)
> > > This isn't quit what I meant.  I'd rather not have the ASSERTs in helper 
> > > functions at all (regardless of return type).  The helpers should return 
> > > a status and the actual test code should do whatever ASSERT or EXPECT is 
> > > appropriate.
> > So how would we handle the existing checks in SetupData()?
> SetUpData would just return an error status instead of ASSERTing.  The actual 
> ASSERTs would be placed in the tests that call SetUpData.  As I said, that 
> would add some duplication, because individual tests would have to ASSERT (or 
> EXPECT) on the result of SetUpData, but it makes the tests easier to read and 
> maintain the tests.
> 
> Since there are other tests using SetUpData, they would have to be updated, 
> so maybe you want to declare this suggestion as out-of-scope for this patch.  
> Anyway, I'm happy that you eliminated the boolean argument.
I think I understand the idea and I agree it's tempting. The problem is that 
not all the checks in SetupData() are status based, so there's no direct 
mapping from each operation to a return value. 

It doesn't seem terribly bad to let the helper encapsulate some of the checks.


https://reviews.llvm.org/D49202



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

[Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

[Leonard Mosescu via Phabricator via lldb-commits]

lemo updated this revision to Diff 155080.
Herald added a subscriber: mgorny.

https://reviews.llvm.org/D49202

Files:
  source/Plugins/Process/minidump/MinidumpParser.cpp
  source/Plugins/Process/minidump/MinidumpParser.h
  source/Plugins/Process/minidump/MinidumpTypes.cpp
  source/Plugins/Process/minidump/ProcessMinidump.cpp
  unittests/Process/minidump/CMakeLists.txt
  unittests/Process/minidump/Inputs/bad_duplicate_streams.dmp
  unittests/Process/minidump/Inputs/bad_overlapping_streams.dmp
  unittests/Process/minidump/MinidumpParserTest.cpp

Index: unittests/Process/minidump/MinidumpParserTest.cpp
===
--- unittests/Process/minidump/MinidumpParserTest.cpp
+++ unittests/Process/minidump/MinidumpParserTest.cpp
@@ -38,16 +38,32 @@
 
 class MinidumpParserTest : public testing::Test {
 public:
-  void SetUpData(const char *minidump_filename,
- uint64_t load_size = UINT64_MAX) {
+  void SetUpData(const char *minidump_filename) {
 std::string filename = GetInputFilePath(minidump_filename);
-auto BufferPtr = DataBufferLLVM::CreateSliceFromPath(filename, load_size, 0);
+auto BufferPtr = DataBufferLLVM::CreateSliceFromPath(filename, -1, 0);
+ASSERT_NE(BufferPtr, nullptr);
+llvm::Optional optional_parser =
+MinidumpParser::Create(BufferPtr);
+ASSERT_TRUE(optional_parser.hasValue());
+parser.reset(new MinidumpParser(optional_parser.getValue()));
+ASSERT_GT(parser->GetData().size(), 0UL);
+auto result = parser->Initialize();
+ASSERT_TRUE(result.Success()) << result.AsCString();
+  }
+
+  void InvalidMinidump(const char *minidump_filename, uint64_t load_size) {
+std::string filename = GetInputFilePath(minidump_filename);
+auto BufferPtr =
+DataBufferLLVM::CreateSliceFromPath(filename, load_size, 0);
+ASSERT_NE(BufferPtr, nullptr);
 
 llvm::Optional optional_parser =
 MinidumpParser::Create(BufferPtr);
 ASSERT_TRUE(optional_parser.hasValue());
 parser.reset(new MinidumpParser(optional_parser.getValue()));
 ASSERT_GT(parser->GetData().size(), 0UL);
+auto result = parser->Initialize();
+ASSERT_TRUE(result.Fail());
   }
 
   std::unique_ptr parser;
@@ -68,12 +84,15 @@
   EXPECT_EQ(1232UL, context.size());
 }
 
-TEST_F(MinidumpParserTest, GetThreadsTruncatedFile) {
-  SetUpData("linux-x86_64.dmp", 200);
-  llvm::ArrayRef thread_list;
+TEST_F(MinidumpParserTest, TruncatedMinidumps) {
+  InvalidMinidump("linux-x86_64.dmp", 32);
+  InvalidMinidump("linux-x86_64.dmp", 100);
+  InvalidMinidump("linux-x86_64.dmp", 20 * 1024);
+}
 
-  thread_list = parser->GetThreads();
-  ASSERT_EQ(0UL, thread_list.size());
+TEST_F(MinidumpParserTest, IllFormedMinidumps) {
+  InvalidMinidump("bad_duplicate_streams.dmp", -1);
+  InvalidMinidump("bad_overlapping_streams.dmp", -1);
 }
 
 TEST_F(MinidumpParserTest, GetArchitecture) {
Index: unittests/Process/minidump/CMakeLists.txt
===
--- unittests/Process/minidump/CMakeLists.txt
+++ unittests/Process/minidump/CMakeLists.txt
@@ -17,6 +17,8 @@
linux-x86_64.dmp
linux-x86_64_not_crashed.dmp
fizzbuzz_no_heap.dmp
-   fizzbuzz_wow64.dmp)
+   fizzbuzz_wow64.dmp
+   bad_duplicate_streams.dmp
+   bad_overlapping_streams.dmp)
 
 add_unittest_inputs(LLDBMinidumpTests "${test_inputs}")
Index: source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -105,7 +105,7 @@
   if (!DataPtr)
 return nullptr;
 
-  assert(DataPtr->GetByteSize() == header_size);
+  lldbassert(DataPtr->GetByteSize() == header_size);
 
   // first, only try to parse the header, beacuse we need to be fast
   llvm::ArrayRef HeaderBytes = DataPtr->GetData();
@@ -164,10 +164,29 @@
 Status ProcessMinidump::DoLoadCore() {
   Status error;
 
+  // Minidump parser initialization & consistency checks
+  error = m_minidump_parser.Initialize();
+  if (error.Fail())
+return error;
+
+  // Do we support the minidump's architecture?
+  ArchSpec arch = GetArchitecture();
+  switch (arch.GetMachine()) {
+  case llvm::Triple::x86:
+  case llvm::Triple::x86_64:
+// supported
+break;
+
+  default:
+error.SetErrorStringWithFormat("unsupported minidump architecture: %s",
+   arch.GetArchitectureName());
+return error;
+  }
+
   m_thread_list = m_minidump_parser.GetThreads();
   m_active_exception = m_minidump_parser.GetExceptionStream();
   ReadModuleList();
-  GetTarget().SetArchitecture(GetArchitecture());
+  GetTarget().SetArchitecture(arch);
 
   llvm::Optional pid = m_minidump_parser.GetPid();
   if (!pid) {
Index: source/Plugins/Process/minidump/MinidumpTypes.cpp
===
--- 

[Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

[Leonard Mosescu via Phabricator via lldb-commits]

lemo updated this revision to Diff 155076.
lemo added a comment.

Adding a few ill-formed minidumps for testing the new checks


https://reviews.llvm.org/D49202

Files:
  source/Plugins/Process/minidump/MinidumpParser.cpp
  source/Plugins/Process/minidump/MinidumpParser.h
  source/Plugins/Process/minidump/MinidumpTypes.cpp
  source/Plugins/Process/minidump/ProcessMinidump.cpp
  unittests/Process/minidump/Inputs/bad_duplicate_streams.dmp
  unittests/Process/minidump/Inputs/bad_overlapping_streams.dmp
  unittests/Process/minidump/MinidumpParserTest.cpp

Index: unittests/Process/minidump/MinidumpParserTest.cpp
===
--- unittests/Process/minidump/MinidumpParserTest.cpp
+++ unittests/Process/minidump/MinidumpParserTest.cpp
@@ -38,16 +38,30 @@
 
 class MinidumpParserTest : public testing::Test {
 public:
-  void SetUpData(const char *minidump_filename,
- uint64_t load_size = UINT64_MAX) {
+  void SetUpData(const char *minidump_filename) {
 std::string filename = GetInputFilePath(minidump_filename);
-auto BufferPtr = DataBufferLLVM::CreateSliceFromPath(filename, load_size, 0);
+auto BufferPtr = DataBufferLLVM::CreateSliceFromPath(filename, -1, 0);
+llvm::Optional optional_parser =
+MinidumpParser::Create(BufferPtr);
+ASSERT_TRUE(optional_parser.hasValue());
+parser.reset(new MinidumpParser(optional_parser.getValue()));
+ASSERT_GT(parser->GetData().size(), 0UL);
+auto result = parser->Initialize();
+ASSERT_TRUE(result.Success()) << result.AsCString();
+  }
+
+  void InvalidMinidump(const char *minidump_filename, uint64_t load_size) {
+std::string filename = GetInputFilePath(minidump_filename);
+auto BufferPtr =
+DataBufferLLVM::CreateSliceFromPath(filename, load_size, 0);
 
 llvm::Optional optional_parser =
 MinidumpParser::Create(BufferPtr);
 ASSERT_TRUE(optional_parser.hasValue());
 parser.reset(new MinidumpParser(optional_parser.getValue()));
 ASSERT_GT(parser->GetData().size(), 0UL);
+auto result = parser->Initialize();
+ASSERT_TRUE(result.Fail());
   }
 
   std::unique_ptr parser;
@@ -68,12 +82,15 @@
   EXPECT_EQ(1232UL, context.size());
 }
 
-TEST_F(MinidumpParserTest, GetThreadsTruncatedFile) {
-  SetUpData("linux-x86_64.dmp", 200);
-  llvm::ArrayRef thread_list;
+TEST_F(MinidumpParserTest, TruncatedMinidumps) {
+  InvalidMinidump("linux-x86_64.dmp", 32);
+  InvalidMinidump("linux-x86_64.dmp", 100);
+  InvalidMinidump("linux-x86_64.dmp", 20 * 1024);
+}
 
-  thread_list = parser->GetThreads();
-  ASSERT_EQ(0UL, thread_list.size());
+TEST_F(MinidumpParserTest, IllFormedMinidumps) {
+  InvalidMinidump("bad_duplicate_streams.dmp", -1);
+  InvalidMinidump("bad_overlapping_streams.dmp", -1);
 }
 
 TEST_F(MinidumpParserTest, GetArchitecture) {
Index: source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -105,7 +105,7 @@
   if (!DataPtr)
 return nullptr;
 
-  assert(DataPtr->GetByteSize() == header_size);
+  lldbassert(DataPtr->GetByteSize() == header_size);
 
   // first, only try to parse the header, beacuse we need to be fast
   llvm::ArrayRef HeaderBytes = DataPtr->GetData();
@@ -164,10 +164,29 @@
 Status ProcessMinidump::DoLoadCore() {
   Status error;
 
+  // Minidump parser initialization & consistency checks
+  error = m_minidump_parser.Initialize();
+  if (error.Fail())
+return error;
+
+  // Do we support the minidump's architecture?
+  ArchSpec arch = GetArchitecture();
+  switch (arch.GetMachine()) {
+  case llvm::Triple::x86:
+  case llvm::Triple::x86_64:
+// supported
+break;
+
+  default:
+error.SetErrorStringWithFormat("unsupported minidump architecture: %s",
+   arch.GetArchitectureName());
+return error;
+  }
+
   m_thread_list = m_minidump_parser.GetThreads();
   m_active_exception = m_minidump_parser.GetExceptionStream();
   ReadModuleList();
-  GetTarget().SetArchitecture(GetArchitecture());
+  GetTarget().SetArchitecture(arch);
 
   llvm::Optional pid = m_minidump_parser.GetPid();
   if (!pid) {
Index: source/Plugins/Process/minidump/MinidumpTypes.cpp
===
--- source/Plugins/Process/minidump/MinidumpTypes.cpp
+++ source/Plugins/Process/minidump/MinidumpTypes.cpp
@@ -33,9 +33,6 @@
   version != MinidumpHeaderConstants::Version)
 return nullptr;
 
-  // TODO check for max number of streams ?
-  // TODO more sanity checks ?
-
   return header;
 }
 
Index: source/Plugins/Process/minidump/MinidumpParser.h
===
--- source/Plugins/Process/minidump/MinidumpParser.h
+++ source/Plugins/Process/minidump/MinidumpParser.h
@@ -89,14 +89,16 @@
 
   llvm::Optional 

[Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added a comment.

Regarding test for the other checks, I'll try to fabricate a few invalid 
minidumps (although it would obviously provide limited coverage)




Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:52
 ASSERT_GT(parser->GetData().size(), 0UL);
+auto result = parser->Initialize();
+if (expect_failure)

amccarth wrote:
> lemo wrote:
> > amccarth wrote:
> > > I would rather see this function return the result of the Initialize and 
> > > let the individual tests check the expectation explicitly.
> > > 
> > > I know that will lead to a little bit of duplication in the tests, but it 
> > > will make the individual tests easier to understand in isolation.  It 
> > > also makes it possible for each test to decide whether to ASSERT or 
> > > EXPECT.  And it eliminates the need for the bool parameter which is hard 
> > > to decipher at the call sites.
> > Good idea, although gunit doesn't let us ASSERT in non-void returning 
> > functions.
> > 
> > I agree that the bool parameter is ugly, so I created a separate 
> > TruncateMinidump() helper (which cleans up the SetUpData since the 
> > load_size was only used for truncation)
> This isn't quit what I meant.  I'd rather not have the ASSERTs in helper 
> functions at all (regardless of return type).  The helpers should return a 
> status and the actual test code should do whatever ASSERT or EXPECT is 
> appropriate.
So how would we handle the existing checks in SetupData()?


https://reviews.llvm.org/D49202



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

[Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

[Leonard Mosescu via Phabricator via lldb-commits]

lemo updated this revision to Diff 155069.
lemo marked 9 inline comments as done.
lemo added a comment.

Incorporating CR feedback


https://reviews.llvm.org/D49202

Files:
  source/Plugins/Process/minidump/MinidumpParser.cpp
  source/Plugins/Process/minidump/MinidumpParser.h
  source/Plugins/Process/minidump/MinidumpTypes.cpp
  source/Plugins/Process/minidump/ProcessMinidump.cpp
  unittests/Process/minidump/MinidumpParserTest.cpp

Index: unittests/Process/minidump/MinidumpParserTest.cpp
===
--- unittests/Process/minidump/MinidumpParserTest.cpp
+++ unittests/Process/minidump/MinidumpParserTest.cpp
@@ -38,16 +38,30 @@
 
 class MinidumpParserTest : public testing::Test {
 public:
-  void SetUpData(const char *minidump_filename,
- uint64_t load_size = UINT64_MAX) {
+  void SetUpData(const char *minidump_filename) {
 std::string filename = GetInputFilePath(minidump_filename);
-auto BufferPtr = DataBufferLLVM::CreateSliceFromPath(filename, load_size, 0);
+auto BufferPtr = DataBufferLLVM::CreateSliceFromPath(filename, -1, 0);
+llvm::Optional optional_parser =
+MinidumpParser::Create(BufferPtr);
+ASSERT_TRUE(optional_parser.hasValue());
+parser.reset(new MinidumpParser(optional_parser.getValue()));
+ASSERT_GT(parser->GetData().size(), 0UL);
+auto result = parser->Initialize();
+ASSERT_TRUE(result.Success()) << result.AsCString();
+  }
+
+  void TruncateMinidump(const char *minidump_filename, uint64_t load_size) {
+std::string filename = GetInputFilePath(minidump_filename);
+auto BufferPtr =
+DataBufferLLVM::CreateSliceFromPath(filename, load_size, 0);
 
 llvm::Optional optional_parser =
 MinidumpParser::Create(BufferPtr);
 ASSERT_TRUE(optional_parser.hasValue());
 parser.reset(new MinidumpParser(optional_parser.getValue()));
 ASSERT_GT(parser->GetData().size(), 0UL);
+auto result = parser->Initialize();
+ASSERT_TRUE(result.Fail());
   }
 
   std::unique_ptr parser;
@@ -68,12 +82,10 @@
   EXPECT_EQ(1232UL, context.size());
 }
 
-TEST_F(MinidumpParserTest, GetThreadsTruncatedFile) {
-  SetUpData("linux-x86_64.dmp", 200);
-  llvm::ArrayRef thread_list;
-
-  thread_list = parser->GetThreads();
-  ASSERT_EQ(0UL, thread_list.size());
+TEST_F(MinidumpParserTest, TruncatedMinidumps) {
+  TruncateMinidump("linux-x86_64.dmp", 32);
+  TruncateMinidump("linux-x86_64.dmp", 100);
+  TruncateMinidump("linux-x86_64.dmp", 20 * 1024);
 }
 
 TEST_F(MinidumpParserTest, GetArchitecture) {
Index: source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -105,7 +105,7 @@
   if (!DataPtr)
 return nullptr;
 
-  assert(DataPtr->GetByteSize() == header_size);
+  lldbassert(DataPtr->GetByteSize() == header_size);
 
   // first, only try to parse the header, beacuse we need to be fast
   llvm::ArrayRef HeaderBytes = DataPtr->GetData();
@@ -164,10 +164,29 @@
 Status ProcessMinidump::DoLoadCore() {
   Status error;
 
+  // Minidump parser initialization & consistency checks
+  error = m_minidump_parser.Initialize();
+  if (error.Fail())
+return error;
+
+  // Do we support the minidump's architecture?
+  ArchSpec arch = GetArchitecture();
+  switch (arch.GetMachine()) {
+  case llvm::Triple::x86:
+  case llvm::Triple::x86_64:
+// supported
+break;
+
+  default:
+error.SetErrorStringWithFormat("unsupported minidump architecture: %s",
+   arch.GetArchitectureName());
+return error;
+  }
+
   m_thread_list = m_minidump_parser.GetThreads();
   m_active_exception = m_minidump_parser.GetExceptionStream();
   ReadModuleList();
-  GetTarget().SetArchitecture(GetArchitecture());
+  GetTarget().SetArchitecture(arch);
 
   llvm::Optional pid = m_minidump_parser.GetPid();
   if (!pid) {
Index: source/Plugins/Process/minidump/MinidumpTypes.cpp
===
--- source/Plugins/Process/minidump/MinidumpTypes.cpp
+++ source/Plugins/Process/minidump/MinidumpTypes.cpp
@@ -33,9 +33,6 @@
   version != MinidumpHeaderConstants::Version)
 return nullptr;
 
-  // TODO check for max number of streams ?
-  // TODO more sanity checks ?
-
   return header;
 }
 
Index: source/Plugins/Process/minidump/MinidumpParser.h
===
--- source/Plugins/Process/minidump/MinidumpParser.h
+++ source/Plugins/Process/minidump/MinidumpParser.h
@@ -89,14 +89,16 @@
 
   llvm::Optional GetMemoryRegionInfo(lldb::addr_t);
 
+  // Perform consistency checks and initialize internal data structures
+  Status Initialize();
+
+private:
+  MinidumpParser(const lldb::DataBufferSP _buf_sp);
+
 private:
   lldb::DataBufferSP m_data_sp;
-  const MinidumpHeader *m_header;
+  const 

[Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added inline comments.



Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:23
 #include 
+#include 
+#include 

amccarth wrote:
> Why add ``?  It looks like your new map is just a vector.
Good catch



Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:558
+  return result;
+}

amccarth wrote:
> This is a long function (which, unfortunately, is not unusual in LLVM/LLDB), 
> but the comments here are really good, so I'm OK with it.
I agree size-wise it's borderline. It's also a bit smelly in that it does two 
things (checks + initialization).

I originally had a separate "ConsistencyCheck", but there was significant 
duplication (the directory traversal) and the combined version won by a tiny 
margin :) If this grows with more checks I'd be happy to revisit and refactor 
it. 




Comment at: source/Plugins/Process/minidump/MinidumpParser.h:93
+  // Perform consistency checks and initialize internal data structures
+  Status Initialize();
+

amccarth wrote:
> I'm not a big fan of 2-step initialization, but that seems to be the way of 
> LLDB. :-(
Agreed. I don't see how to avoid this w/o much bigger changes though (we don't 
have a chance to return meaningful errors during process creation).



Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:172
+
+  // Do we support the minidump's architecture?
+  ArchSpec arch = GetArchitecture();

amccarth wrote:
> Should the architecture check be in the MinidumpParser::Initialize with the 
> other checks?
> 
> I don't know the answer; I'm just asking for your thinking about this.
Good question, here's my take: the checks are for consistency and a minidump 
with a currently unsupported architecture is a valid minidump. 

So I think it's better to have this check external since the architecture 
support is not a minidump parser concern. WDYT?



Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:193
   if (!pid) {
-error.SetErrorString("failed to parse PID");
+error.SetErrorString("Failed to parse PID");
 return error;

amccarth wrote:
> I realize nothing is perfectly consistent, but I think LLVM tends to start 
> error strings with a lowercase letter (except for proper nouns).  Can you 
> revert this case change and make sure your new error strings follow that 
> pattern?
Sigh, sure. I was hoping it's the other way around.



Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:52
 ASSERT_GT(parser->GetData().size(), 0UL);
+auto result = parser->Initialize();
+if (expect_failure)

amccarth wrote:
> I would rather see this function return the result of the Initialize and let 
> the individual tests check the expectation explicitly.
> 
> I know that will lead to a little bit of duplication in the tests, but it 
> will make the individual tests easier to understand in isolation.  It also 
> makes it possible for each test to decide whether to ASSERT or EXPECT.  And 
> it eliminates the need for the bool parameter which is hard to decipher at 
> the call sites.
Good idea, although gunit doesn't let us ASSERT in non-void returning functions.

I agree that the bool parameter is ugly, so I created a separate 
TruncateMinidump() helper (which cleans up the SetUpData since the load_size 
was only used for truncation)


https://reviews.llvm.org/D49202



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

[Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

[Leonard Mosescu via Phabricator via lldb-commits]

lemo created this revision.
lemo added reviewers: amccarth, labath.
lemo added a project: LLDB.
Herald added a subscriber: mgrang.

Corrupted minidumps was leading to unpredictable behavior.

This change adds explicit consistency checks for the minidump early on. The 
checks are not comprehensive but they should catch obvious structural 
violations:

- streams with type == 0
- duplicate streams (same type)
- overlapping streams
- truncated minidumps

Another early check is to make sure we actually support the minidump 
architecture instead of crashing at a random place deep inside LLDB.


https://reviews.llvm.org/D49202

Files:
  source/Plugins/Process/minidump/MinidumpParser.cpp
  source/Plugins/Process/minidump/MinidumpParser.h
  source/Plugins/Process/minidump/MinidumpTypes.cpp
  source/Plugins/Process/minidump/ProcessMinidump.cpp
  unittests/Process/minidump/MinidumpParserTest.cpp

Index: unittests/Process/minidump/MinidumpParserTest.cpp
===
--- unittests/Process/minidump/MinidumpParserTest.cpp
+++ unittests/Process/minidump/MinidumpParserTest.cpp
@@ -39,15 +39,21 @@
 class MinidumpParserTest : public testing::Test {
 public:
   void SetUpData(const char *minidump_filename,
- uint64_t load_size = UINT64_MAX) {
+ uint64_t load_size = UINT64_MAX,
+ bool expect_failure = false) {
 std::string filename = GetInputFilePath(minidump_filename);
 auto BufferPtr = DataBufferLLVM::CreateSliceFromPath(filename, load_size, 0);
 
 llvm::Optional optional_parser =
 MinidumpParser::Create(BufferPtr);
 ASSERT_TRUE(optional_parser.hasValue());
 parser.reset(new MinidumpParser(optional_parser.getValue()));
 ASSERT_GT(parser->GetData().size(), 0UL);
+auto result = parser->Initialize();
+if (expect_failure)
+  ASSERT_TRUE(result.Fail());
+else
+  ASSERT_TRUE(result.Success()) << result.AsCString();
   }
 
   std::unique_ptr parser;
@@ -68,12 +74,10 @@
   EXPECT_EQ(1232UL, context.size());
 }
 
-TEST_F(MinidumpParserTest, GetThreadsTruncatedFile) {
-  SetUpData("linux-x86_64.dmp", 200);
-  llvm::ArrayRef thread_list;
-
-  thread_list = parser->GetThreads();
-  ASSERT_EQ(0UL, thread_list.size());
+TEST_F(MinidumpParserTest, TruncatedMinidumps) {
+  SetUpData("linux-x86_64.dmp", 32, true);
+  SetUpData("linux-x86_64.dmp", 100, true);
+  SetUpData("linux-x86_64.dmp", 20 * 1024, true);
 }
 
 TEST_F(MinidumpParserTest, GetArchitecture) {
Index: source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -105,7 +105,7 @@
   if (!DataPtr)
 return nullptr;
 
-  assert(DataPtr->GetByteSize() == header_size);
+  lldbassert(DataPtr->GetByteSize() == header_size);
 
   // first, only try to parse the header, beacuse we need to be fast
   llvm::ArrayRef HeaderBytes = DataPtr->GetData();
@@ -164,14 +164,33 @@
 Status ProcessMinidump::DoLoadCore() {
   Status error;
 
+  // Minidump parser initialization & consistency checks
+  error = m_minidump_parser.Initialize();
+  if (error.Fail())
+return error;
+
+  // Do we support the minidump's architecture?
+  ArchSpec arch = GetArchitecture();
+  switch (arch.GetMachine()) {
+  case llvm::Triple::x86:
+  case llvm::Triple::x86_64:
+// supported
+break;
+
+  default:
+error.SetErrorStringWithFormat("Unsupported minidump architecture: %s",
+   arch.GetArchitectureName());
+return error;
+  }
+
   m_thread_list = m_minidump_parser.GetThreads();
   m_active_exception = m_minidump_parser.GetExceptionStream();
   ReadModuleList();
-  GetTarget().SetArchitecture(GetArchitecture());
+  GetTarget().SetArchitecture(arch);
 
   llvm::Optional pid = m_minidump_parser.GetPid();
   if (!pid) {
-error.SetErrorString("failed to parse PID");
+error.SetErrorString("Failed to parse PID");
 return error;
   }
   SetID(pid.getValue());
Index: source/Plugins/Process/minidump/MinidumpTypes.cpp
===
--- source/Plugins/Process/minidump/MinidumpTypes.cpp
+++ source/Plugins/Process/minidump/MinidumpTypes.cpp
@@ -33,9 +33,6 @@
   version != MinidumpHeaderConstants::Version)
 return nullptr;
 
-  // TODO check for max number of streams ?
-  // TODO more sanity checks ?
-
   return header;
 }
 
Index: source/Plugins/Process/minidump/MinidumpParser.h
===
--- source/Plugins/Process/minidump/MinidumpParser.h
+++ source/Plugins/Process/minidump/MinidumpParser.h
@@ -89,14 +89,16 @@
 
   llvm::Optional GetMemoryRegionInfo(lldb::addr_t);
 
+  // Perform consistency checks and initialize internal data structures
+  Status Initialize();
+
+private:
+  MinidumpParser(const lldb::DataBufferSP 

[Lldb-commits] [PATCH] D48479: Represent invalid UUIDs as UUIDs with length zero

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added a comment.

> I'm not sure if there is a suitable place for that function. This is
>  needed in  "ObjectFileMachO" and two dynamic loader plugins.

Then your factory idea may be the next best thing. While we're at it, maybe
we can remove the UUID::SetBytes() from the public interface, and make the
UUID an immutable value type:

Ex. instead of:

UUID uuid;
...
uuid.SetBytes(...)

We'd have:

UUID uuid;

uuid = UUID(...);
// or
uuid = { ... };
// or
uuid = UUID::factory(...);

What do you think?


https://reviews.llvm.org/D48479



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

[Lldb-commits] [PATCH] D48479: Represent invalid UUIDs as UUIDs with length zero

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added subscribers: clayborg, labath, sas.
lemo added a comment.

> However, during parsing you need to know the meaning of a "...0" UUID.
>  In a MachO file (at least based on the comments in the code) this value is
>  used to denote the fact that the object file has no UUID. For elf, a
>  "000..0" build-id is a perfectly valid identifier (and the lack of a
>  build-id is denoted by the absence of the build-id section). The extra
>  constructor argument is my way of trying to support both scenarios. The
>  other possibility I see is to have a some kind of a factory function for
>  one of the options (or both). I don't really have a preference between the
>  two.

One solution might be to encapsulate the MachO convention in the MachO
code: check in there (maybe through a helper function) if the UUID is
"000...0" and map it to the empty UUID in that case. The UUID interface
would not have to know/care about this convention. Would this work?


https://reviews.llvm.org/D48479



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

[Lldb-commits] [PATCH] D48479: Represent invalid UUIDs as UUIDs with length zero

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added a comment.

> The slight complication here is that
>  some clients (MachO) actually use the all-zero notation to mean "no UUID
>  has been set". To keep this use case working, I have introduced an
>  additional argument to the UUID constructor, which specifies whether an
>  all-zero vector should be considered a valid UUID. For the usages where
>  the UUID data comes from a MachO file, I set this argument to false.

What is the distinction between "no UUID has been set" and "invalid UUID"? I 
find this subtlety confusing, can we simplify it to just valid/not-valid UUIDs?




Comment at: include/lldb/Utility/UUID.h:31
   // Most UUIDs are 16 bytes, but some Linux build-ids (SHA1) are 20.
   typedef uint8_t ValueType[20];
 

switch to llvm::SmallVector as suggested by Greg? 



Comment at: include/lldb/Utility/UUID.h:42
 
-  const UUID =(const UUID );
+  UUID =(const UUID ) = default;
 

If we define this we should define at least the copy constructor as well (rule 
of 3/5). In this case the cleanest solution may be to allow the implicit 
definitions (which would also allow the implicit move operations - defining 
assignment operator as defaulted would inhibit the implicit move SMFs)



Comment at: include/lldb/Utility/UUID.h:52
 
-  bool IsValid() const;
+  explicit operator bool() const { return IsValid(); }
+  bool IsValid() const { return m_num_uuid_bytes > 0; }

is this really needed? I'd prefer the truly explicit (pun intended) IsValid()



Comment at: include/lldb/Utility/UUID.h:95
 
 bool operator==(const UUID , const UUID );
 bool operator!=(const UUID , const UUID );

these can be simplified if we use llvm::SmallVector (or std::vector) 


https://reviews.llvm.org/D48479



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

[Lldb-commits] [PATCH] D48049: Add a new SBTarget::LoadCore() overload which surfaces errors if the load fails

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added a comment.

The intention in the scope of this change is just to check that the new
overload is exposed correctly through the Python API.

In general, guaranteeing specific error codes (messages?) is likely
impractical:

1. It's not always easy to do the proper checks (ex. for 'file-not-found'

you'd actually get a null process, nothing more - see SBTarget::LoadCore()).

2. It's unlikely that many clients need or want to check for specific

errors.

3. Such a strict contract would be very fragile (ex. adding more specific

error condition detection would likely break the contract)

Perhaps ironically, I could take advantage of very specific error codes for
my scenario, but I don't think it's feasible. So for LoadCore() I'd like to
aim for more of a middle ground: accurate success/fail status + an
informative error content that can be used for diagnostics (logging) and
maybe as a weak hint.

What do you think?


Repository:
  rL LLVM

https://reviews.llvm.org/D48049



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

[Lldb-commits] [PATCH] D47991: Improve SBThread's stepping API using SBError parameter.

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added subscribers: clayborg, jingham, labath, apolyakov.
lemo added a comment.

> Ah I see. That's because the last argument is a C++ default argument. It
>  looks like the convention in this file is that the error argument should be
>  the last non-defaulted argument.

I think it should be:

void StepOver(lldb::RunMode stop_other_threads, lldb::SBError ); //
no default argument!

Ie. if you want the overload with error you need to pass RunMode
explicitly. Keeping the overload set manageable is a good practice in
general, not just because of SWIG (default arguments + overloaded arguments
can easily get out of hand)


https://reviews.llvm.org/D47991



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

[Lldb-commits] [PATCH] D48049: Add a new SBTarget::LoadCore() overload which surfaces errors if the load fails

[Leonard Mosescu via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL334439: Add a new SBTarget::LoadCore() overload which 
surfaces errors if the load fails (authored by lemo, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D48049?vs=150844=150845#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D48049

Files:
  lldb/trunk/include/lldb/API/SBTarget.h
  
lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
  lldb/trunk/scripts/interface/SBTarget.i
  lldb/trunk/source/API/SBTarget.cpp

Index: lldb/trunk/scripts/interface/SBTarget.i
===
--- lldb/trunk/scripts/interface/SBTarget.i
+++ lldb/trunk/scripts/interface/SBTarget.i
@@ -77,7 +77,7 @@
 
 static const char *
 GetBroadcasterClassName ();
-
+
 bool
 IsValid() const;
 
@@ -145,7 +145,7 @@
 /// An optional listener that will receive all process events.
 /// If \a listener is valid then \a listener will listen to all
 /// process events. If not valid, then this target's debugger
-/// (SBTarget::GetDebugger()) will listen to all process events. 
+/// (SBTarget::GetDebugger()) will listen to all process events.
 ///
 /// @param[in] argv
 /// The argument array.
@@ -175,7 +175,7 @@
 /// The working directory to have the child process run in
 ///
 /// @param[in] launch_flags
-/// Some launch options specified by logical OR'ing 
+/// Some launch options specified by logical OR'ing
 /// lldb::LaunchFlags enumeration values together.
 ///
 /// @param[in] stop_at_entry
@@ -203,7 +203,7 @@
 run to completion if no user interaction is required.
 ") Launch;
 lldb::SBProcess
-Launch (SBListener , 
+Launch (SBListener ,
 char const **argv,
 char const **envp,
 const char *stdin_path,
@@ -213,7 +213,7 @@
 uint32_t launch_flags,   // See LaunchFlags
 bool stop_at_entry,
 lldb::SBError& error);
-
+
 %feature("docstring", "
 //--
 /// Launch a new process with sensible defaults.
@@ -250,10 +250,10 @@
 executable.
 ") LaunchSimple;
 lldb::SBProcess
-LaunchSimple (const char **argv, 
+LaunchSimple (const char **argv,
   const char **envp,
   const char *working_directory);
-
+
 lldb::SBProcess
 Launch (lldb::SBLaunchInfo _info, lldb::SBError& error);
 
@@ -264,6 +264,10 @@
 /// @param[in] core_file
 /// File path of the core dump.
 ///
+/// @param[out] error
+/// An error explaining what went wrong if the operation fails.
+/// (Optional)
+///
 /// @return
 ///  A process object for the newly created core file.
 //--
@@ -276,10 +280,12 @@
 ") LoadCore;
 lldb::SBProcess
 LoadCore(const char *core_file);
-
+
 lldb::SBProcess
-Attach (lldb::SBAttachInfo _info, lldb::SBError& error);
-
+LoadCore(const char *core_file, lldb::SBError );
+
+lldb::SBProcess
+Attach(lldb::SBAttachInfo _info, lldb::SBError& error);
 
 %feature("docstring", "
 //--
@@ -363,7 +369,7 @@
const char *url,
const char *plugin_name,
SBError& error);
-
+
 lldb::SBFileSpec
 GetExecutable ();
 
@@ -401,10 +407,10 @@
 
 lldb::ByteOrder
 GetByteOrder ();
-
+
 uint32_t
 GetAddressByteSize();
-
+
 const char *
 GetTriple ();
 
@@ -457,21 +463,21 @@
 /// A logical OR of one or more FunctionNameType enum bits that
 /// indicate what kind of names should be used when doing the
 /// lookup. Bits include fully qualified names, base names,
-/// C++ methods, or ObjC selectors. 
+/// C++ methods, or ObjC selectors.
 /// See FunctionNameType for more details.
 ///
 /// @return
-/// A lldb::SBSymbolContextList that gets filled in with all of 
+/// A lldb::SBSymbolContextList that gets filled in with all of
 /// the symbol contexts for all the matches.
 //--
 ") FindFunctions;
 lldb::SBSymbolContextList
-FindFunctions (const char *name, 
+FindFunctions (const char *name,
uint32_t name_type_mask = lldb::eFunctionNameTypeAny);
-
+
 lldb::SBType
 FindFirstType (const char* type);
-
+
 lldb::SBTypeList
 FindTypes (const char* type);
 
@@ -497,7 +503,7 @@
 //--
 ") 

[Lldb-commits] [PATCH] D48049: Add a new SBTarget::LoadCore() overload which surfaces errors if the load fails

[Leonard Mosescu via Phabricator via lldb-commits]

lemo marked 3 inline comments as done.
lemo added a comment.

spaces removed :)


https://reviews.llvm.org/D48049



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

[Lldb-commits] [PATCH] D48049: Add a new SBTarget::LoadCore() overload which surfaces errors if the load fails

[Leonard Mosescu via Phabricator via lldb-commits]

lemo updated this revision to Diff 150844.

https://reviews.llvm.org/D48049

Files:
  include/lldb/API/SBTarget.h
  
packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
  scripts/interface/SBTarget.i
  source/API/SBTarget.cpp

Index: source/API/SBTarget.cpp
===
--- source/API/SBTarget.cpp
+++ source/API/SBTarget.cpp
@@ -203,16 +203,26 @@
 }
 
 SBProcess SBTarget::LoadCore(const char *core_file) {
+  lldb::SBError error; // Ignored
+  return LoadCore(core_file, error);
+}
+
+SBProcess SBTarget::LoadCore(const char *core_file, lldb::SBError ) {
   SBProcess sb_process;
   TargetSP target_sp(GetSP());
   if (target_sp) {
 FileSpec filespec(core_file, true);
 ProcessSP process_sp(target_sp->CreateProcess(
 target_sp->GetDebugger().GetListener(), "", ));
 if (process_sp) {
-  process_sp->LoadCore();
-  sb_process.SetSP(process_sp);
+  error.SetError(process_sp->LoadCore());
+  if (error.Success())
+sb_process.SetSP(process_sp);
+} else {
+  error.SetErrorString("Failed to create the process");
 }
+  } else {
+error.SetErrorString("SBTarget is invalid");
   }
   return sb_process;
 }
Index: scripts/interface/SBTarget.i
===
--- scripts/interface/SBTarget.i
+++ scripts/interface/SBTarget.i
@@ -77,7 +77,7 @@
 
 static const char *
 GetBroadcasterClassName ();
-
+
 bool
 IsValid() const;
 
@@ -145,7 +145,7 @@
 /// An optional listener that will receive all process events.
 /// If \a listener is valid then \a listener will listen to all
 /// process events. If not valid, then this target's debugger
-/// (SBTarget::GetDebugger()) will listen to all process events. 
+/// (SBTarget::GetDebugger()) will listen to all process events.
 ///
 /// @param[in] argv
 /// The argument array.
@@ -175,7 +175,7 @@
 /// The working directory to have the child process run in
 ///
 /// @param[in] launch_flags
-/// Some launch options specified by logical OR'ing 
+/// Some launch options specified by logical OR'ing
 /// lldb::LaunchFlags enumeration values together.
 ///
 /// @param[in] stop_at_entry
@@ -203,7 +203,7 @@
 run to completion if no user interaction is required.
 ") Launch;
 lldb::SBProcess
-Launch (SBListener , 
+Launch (SBListener ,
 char const **argv,
 char const **envp,
 const char *stdin_path,
@@ -213,7 +213,7 @@
 uint32_t launch_flags,   // See LaunchFlags
 bool stop_at_entry,
 lldb::SBError& error);
-
+
 %feature("docstring", "
 //--
 /// Launch a new process with sensible defaults.
@@ -250,10 +250,10 @@
 executable.
 ") LaunchSimple;
 lldb::SBProcess
-LaunchSimple (const char **argv, 
+LaunchSimple (const char **argv,
   const char **envp,
   const char *working_directory);
-
+
 lldb::SBProcess
 Launch (lldb::SBLaunchInfo _info, lldb::SBError& error);
 
@@ -264,6 +264,10 @@
 /// @param[in] core_file
 /// File path of the core dump.
 ///
+/// @param[out] error
+/// An error explaining what went wrong if the operation fails.
+/// (Optional)
+///
 /// @return
 ///  A process object for the newly created core file.
 //--
@@ -276,10 +280,12 @@
 ") LoadCore;
 lldb::SBProcess
 LoadCore(const char *core_file);
-
+
 lldb::SBProcess
-Attach (lldb::SBAttachInfo _info, lldb::SBError& error);
-
+LoadCore(const char *core_file, lldb::SBError );
+
+lldb::SBProcess
+Attach(lldb::SBAttachInfo _info, lldb::SBError& error);
 
 %feature("docstring", "
 //--
@@ -363,7 +369,7 @@
const char *url,
const char *plugin_name,
SBError& error);
-
+
 lldb::SBFileSpec
 GetExecutable ();
 
@@ -401,10 +407,10 @@
 
 lldb::ByteOrder
 GetByteOrder ();
-
+
 uint32_t
 GetAddressByteSize();
-
+
 const char *
 GetTriple ();
 
@@ -457,21 +463,21 @@
 /// A logical OR of one or more FunctionNameType enum bits that
 /// indicate what kind of names should be used when doing the
 /// lookup. Bits include fully qualified names, base names,
-/// C++ methods, or ObjC selectors. 
+/// C++ methods, or ObjC selectors.
 /// See FunctionNameType for more details.
 ///
 /// @return
-/// A lldb::SBSymbolContextList that gets filled in with all of 
+/// A lldb::SBSymbolContextList that gets 

[Lldb-commits] [PATCH] D48049: Add a new SBTarget::LoadCore() overload which surfaces errors if the load fails

[Leonard Mosescu via Phabricator via lldb-commits]

lemo updated this revision to Diff 150843.

https://reviews.llvm.org/D48049

Files:
  include/lldb/API/SBTarget.h
  
packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
  scripts/interface/SBTarget.i
  source/API/SBTarget.cpp

Index: source/API/SBTarget.cpp
===
--- source/API/SBTarget.cpp
+++ source/API/SBTarget.cpp
@@ -203,16 +203,26 @@
 }
 
 SBProcess SBTarget::LoadCore(const char *core_file) {
+  lldb::SBError error; // Ignored
+  return LoadCore(core_file, error);
+}
+
+SBProcess SBTarget::LoadCore(const char *core_file, lldb::SBError ) {
   SBProcess sb_process;
   TargetSP target_sp(GetSP());
   if (target_sp) {
 FileSpec filespec(core_file, true);
 ProcessSP process_sp(target_sp->CreateProcess(
 target_sp->GetDebugger().GetListener(), "", ));
 if (process_sp) {
-  process_sp->LoadCore();
-  sb_process.SetSP(process_sp);
+  error.SetError(process_sp->LoadCore());
+  if (error.Success())
+sb_process.SetSP(process_sp);
+} else {
+  error.SetErrorString("Failed to create the process");
 }
+  } else {
+error.SetErrorString("SBTarget is invalid");
   }
   return sb_process;
 }
Index: scripts/interface/SBTarget.i
===
--- scripts/interface/SBTarget.i
+++ scripts/interface/SBTarget.i
@@ -77,7 +77,7 @@
 
 static const char *
 GetBroadcasterClassName ();
-
+
 bool
 IsValid() const;
 
@@ -145,7 +145,7 @@
 /// An optional listener that will receive all process events.
 /// If \a listener is valid then \a listener will listen to all
 /// process events. If not valid, then this target's debugger
-/// (SBTarget::GetDebugger()) will listen to all process events. 
+/// (SBTarget::GetDebugger()) will listen to all process events.
 ///
 /// @param[in] argv
 /// The argument array.
@@ -175,7 +175,7 @@
 /// The working directory to have the child process run in
 ///
 /// @param[in] launch_flags
-/// Some launch options specified by logical OR'ing 
+/// Some launch options specified by logical OR'ing
 /// lldb::LaunchFlags enumeration values together.
 ///
 /// @param[in] stop_at_entry
@@ -203,7 +203,7 @@
 run to completion if no user interaction is required.
 ") Launch;
 lldb::SBProcess
-Launch (SBListener , 
+Launch (SBListener ,
 char const **argv,
 char const **envp,
 const char *stdin_path,
@@ -213,7 +213,7 @@
 uint32_t launch_flags,   // See LaunchFlags
 bool stop_at_entry,
 lldb::SBError& error);
-
+
 %feature("docstring", "
 //--
 /// Launch a new process with sensible defaults.
@@ -250,10 +250,10 @@
 executable.
 ") LaunchSimple;
 lldb::SBProcess
-LaunchSimple (const char **argv, 
+LaunchSimple (const char **argv,
   const char **envp,
   const char *working_directory);
-
+
 lldb::SBProcess
 Launch (lldb::SBLaunchInfo _info, lldb::SBError& error);
 
@@ -264,6 +264,10 @@
 /// @param[in] core_file
 /// File path of the core dump.
 ///
+/// @param[out] error
+/// An error explaining what went wrong if the operation fails.
+/// (Optional)
+///
 /// @return
 ///  A process object for the newly created core file.
 //--
@@ -276,10 +280,12 @@
 ") LoadCore;
 lldb::SBProcess
 LoadCore(const char *core_file);
-
+
+lldb::SBProcess
+LoadCore(const char *core_file, lldb::SBError );
+
 lldb::SBProcess
 Attach (lldb::SBAttachInfo _info, lldb::SBError& error);
-
 
 %feature("docstring", "
 //--
@@ -363,7 +369,7 @@
const char *url,
const char *plugin_name,
SBError& error);
-
+
 lldb::SBFileSpec
 GetExecutable ();
 
@@ -401,10 +407,10 @@
 
 lldb::ByteOrder
 GetByteOrder ();
-
+
 uint32_t
 GetAddressByteSize();
-
+
 const char *
 GetTriple ();
 
@@ -457,21 +463,21 @@
 /// A logical OR of one or more FunctionNameType enum bits that
 /// indicate what kind of names should be used when doing the
 /// lookup. Bits include fully qualified names, base names,
-/// C++ methods, or ObjC selectors. 
+/// C++ methods, or ObjC selectors.
 /// See FunctionNameType for more details.
 ///
 /// @return
-/// A lldb::SBSymbolContextList that gets filled in with all of 
+/// A lldb::SBSymbolContextList that gets filled in with all of
 /// the symbol contexts for 

[Lldb-commits] [PATCH] D48049: Add a new SBTarget::LoadCore() overload which surfaces errors if the load fails

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added subscribers: amccarth, clayborg, zturner.
lemo added a comment.

> remove space before (

I'd be happy to make the change, but looking at the rest of the file it
seems that almost everything uses the opposite convention: Foo (...).

So, to make sure I'm making the right change, is this how it should look?

  lldb::SBProcess
  LoadCore(const char *core_file);
  
  lldb::SBProcess
  LoadCore(const char *core_file, lldb::SBError );


https://reviews.llvm.org/D48049



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

[Lldb-commits] [PATCH] D48049: Add a new SBTarget::LoadCore() overload which surfaces errors if the load fails

[Leonard Mosescu via Phabricator via lldb-commits]

lemo updated this revision to Diff 150835.
lemo added a comment.

Adding test cases for the new overload.


https://reviews.llvm.org/D48049

Files:
  include/lldb/API/SBTarget.h
  
packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
  scripts/interface/SBTarget.i
  source/API/SBTarget.cpp

Index: source/API/SBTarget.cpp
===
--- source/API/SBTarget.cpp
+++ source/API/SBTarget.cpp
@@ -203,16 +203,26 @@
 }
 
 SBProcess SBTarget::LoadCore(const char *core_file) {
+  lldb::SBError error; // Ignored
+  return LoadCore(core_file, error);
+}
+
+SBProcess SBTarget::LoadCore(const char *core_file, lldb::SBError ) {
   SBProcess sb_process;
   TargetSP target_sp(GetSP());
   if (target_sp) {
 FileSpec filespec(core_file, true);
 ProcessSP process_sp(target_sp->CreateProcess(
 target_sp->GetDebugger().GetListener(), "", ));
 if (process_sp) {
-  process_sp->LoadCore();
-  sb_process.SetSP(process_sp);
+  error.SetError(process_sp->LoadCore());
+  if (error.Success())
+sb_process.SetSP(process_sp);
+} else {
+  error.SetErrorString("Failed to create the process");
 }
+  } else {
+error.SetErrorString("SBTarget is invalid");
   }
   return sb_process;
 }
Index: scripts/interface/SBTarget.i
===
--- scripts/interface/SBTarget.i
+++ scripts/interface/SBTarget.i
@@ -77,7 +77,7 @@
 
 static const char *
 GetBroadcasterClassName ();
-
+
 bool
 IsValid() const;
 
@@ -145,7 +145,7 @@
 /// An optional listener that will receive all process events.
 /// If \a listener is valid then \a listener will listen to all
 /// process events. If not valid, then this target's debugger
-/// (SBTarget::GetDebugger()) will listen to all process events. 
+/// (SBTarget::GetDebugger()) will listen to all process events.
 ///
 /// @param[in] argv
 /// The argument array.
@@ -175,7 +175,7 @@
 /// The working directory to have the child process run in
 ///
 /// @param[in] launch_flags
-/// Some launch options specified by logical OR'ing 
+/// Some launch options specified by logical OR'ing
 /// lldb::LaunchFlags enumeration values together.
 ///
 /// @param[in] stop_at_entry
@@ -203,7 +203,7 @@
 run to completion if no user interaction is required.
 ") Launch;
 lldb::SBProcess
-Launch (SBListener , 
+Launch (SBListener ,
 char const **argv,
 char const **envp,
 const char *stdin_path,
@@ -213,7 +213,7 @@
 uint32_t launch_flags,   // See LaunchFlags
 bool stop_at_entry,
 lldb::SBError& error);
-
+
 %feature("docstring", "
 //--
 /// Launch a new process with sensible defaults.
@@ -250,10 +250,10 @@
 executable.
 ") LaunchSimple;
 lldb::SBProcess
-LaunchSimple (const char **argv, 
+LaunchSimple (const char **argv,
   const char **envp,
   const char *working_directory);
-
+
 lldb::SBProcess
 Launch (lldb::SBLaunchInfo _info, lldb::SBError& error);
 
@@ -264,6 +264,10 @@
 /// @param[in] core_file
 /// File path of the core dump.
 ///
+/// @param[out] error
+/// An error explaining what went wrong if the operation fails.
+/// (Optional)
+///
 /// @return
 ///  A process object for the newly created core file.
 //--
@@ -275,11 +279,13 @@
 loads a new core file and returns the process object.
 ") LoadCore;
 lldb::SBProcess
-LoadCore(const char *core_file);
-
+LoadCore (const char *core_file);
+
+lldb::SBProcess
+LoadCore (const char *core_file, lldb::SBError );
+
 lldb::SBProcess
 Attach (lldb::SBAttachInfo _info, lldb::SBError& error);
-
 
 %feature("docstring", "
 //--
@@ -363,7 +369,7 @@
const char *url,
const char *plugin_name,
SBError& error);
-
+
 lldb::SBFileSpec
 GetExecutable ();
 
@@ -401,10 +407,10 @@
 
 lldb::ByteOrder
 GetByteOrder ();
-
+
 uint32_t
 GetAddressByteSize();
-
+
 const char *
 GetTriple ();
 
@@ -457,21 +463,21 @@
 /// A logical OR of one or more FunctionNameType enum bits that
 /// indicate what kind of names should be used when doing the
 /// lookup. Bits include fully qualified names, base names,
-/// C++ methods, or ObjC selectors. 
+/// C++ methods, or ObjC selectors.
 /// See FunctionNameType for more details.
 ///
 /// @return
-/// A 

[Lldb-commits] [PATCH] D48049: Add a new SBTarget::LoadCore() overload which surfaces errors if the load fails

[Leonard Mosescu via Phabricator via lldb-commits]

lemo created this revision.
lemo added reviewers: zturner, clayborg, amccarth.

There was no way to find out what's wrong if SBProcess SBTarget::LoadCore(const 
char *core_file) failed. 
Additionally, the implementation was unconditionally setting sb_process, so it 
wasn't even possible to check if the return SBProcess is valid.

This change adds a new overload which surfaces the errors and also returns a 
valid SBProcess only if the core load succeeds:

SBProcess SBTarget::LoadCore(const char *core_file, SBError );


https://reviews.llvm.org/D48049

Files:
  include/lldb/API/SBTarget.h
  scripts/interface/SBTarget.i
  source/API/SBTarget.cpp

Index: source/API/SBTarget.cpp
===
--- source/API/SBTarget.cpp
+++ source/API/SBTarget.cpp
@@ -203,16 +203,26 @@
 }
 
 SBProcess SBTarget::LoadCore(const char *core_file) {
+  lldb::SBError error; // Ignored
+  return LoadCore(core_file, error);
+}
+
+SBProcess SBTarget::LoadCore(const char *core_file, lldb::SBError ) {
   SBProcess sb_process;
   TargetSP target_sp(GetSP());
   if (target_sp) {
 FileSpec filespec(core_file, true);
 ProcessSP process_sp(target_sp->CreateProcess(
 target_sp->GetDebugger().GetListener(), "", ));
 if (process_sp) {
-  process_sp->LoadCore();
-  sb_process.SetSP(process_sp);
+  error.SetError(process_sp->LoadCore());
+  if (error.Success())
+sb_process.SetSP(process_sp);
+} else {
+  error.SetErrorString("Failed to create the process");
 }
+  } else {
+error.SetErrorString("SBTarget is invalid");
   }
   return sb_process;
 }
Index: scripts/interface/SBTarget.i
===
--- scripts/interface/SBTarget.i
+++ scripts/interface/SBTarget.i
@@ -77,7 +77,7 @@
 
 static const char *
 GetBroadcasterClassName ();
-
+
 bool
 IsValid() const;
 
@@ -145,7 +145,7 @@
 /// An optional listener that will receive all process events.
 /// If \a listener is valid then \a listener will listen to all
 /// process events. If not valid, then this target's debugger
-/// (SBTarget::GetDebugger()) will listen to all process events. 
+/// (SBTarget::GetDebugger()) will listen to all process events.
 ///
 /// @param[in] argv
 /// The argument array.
@@ -175,7 +175,7 @@
 /// The working directory to have the child process run in
 ///
 /// @param[in] launch_flags
-/// Some launch options specified by logical OR'ing 
+/// Some launch options specified by logical OR'ing
 /// lldb::LaunchFlags enumeration values together.
 ///
 /// @param[in] stop_at_entry
@@ -203,7 +203,7 @@
 run to completion if no user interaction is required.
 ") Launch;
 lldb::SBProcess
-Launch (SBListener , 
+Launch (SBListener ,
 char const **argv,
 char const **envp,
 const char *stdin_path,
@@ -213,7 +213,7 @@
 uint32_t launch_flags,   // See LaunchFlags
 bool stop_at_entry,
 lldb::SBError& error);
-
+
 %feature("docstring", "
 //--
 /// Launch a new process with sensible defaults.
@@ -250,10 +250,10 @@
 executable.
 ") LaunchSimple;
 lldb::SBProcess
-LaunchSimple (const char **argv, 
+LaunchSimple (const char **argv,
   const char **envp,
   const char *working_directory);
-
+
 lldb::SBProcess
 Launch (lldb::SBLaunchInfo _info, lldb::SBError& error);
 
@@ -264,6 +264,10 @@
 /// @param[in] core_file
 /// File path of the core dump.
 ///
+/// @param[out] error
+/// An error explaining what went wrong if the operation fails.
+/// (Optional)
+///
 /// @return
 ///  A process object for the newly created core file.
 //--
@@ -275,11 +279,13 @@
 loads a new core file and returns the process object.
 ") LoadCore;
 lldb::SBProcess
-LoadCore(const char *core_file);
-
+LoadCore (const char *core_file);
+
+lldb::SBProcess
+LoadCore (const char *core_file, lldb::SBError );
+
 lldb::SBProcess
 Attach (lldb::SBAttachInfo _info, lldb::SBError& error);
-
 
 %feature("docstring", "
 //--
@@ -363,7 +369,7 @@
const char *url,
const char *plugin_name,
SBError& error);
-
+
 lldb::SBFileSpec
 GetExecutable ();
 
@@ -401,10 +407,10 @@
 
 lldb::ByteOrder
 GetByteOrder ();
-
+
 uint32_t
 GetAddressByteSize();
-
+
 const char *
 GetTriple ();
 
@@ -457,21 +463,21 @@
 /// A logical OR of one or more FunctionNameType enum bits that
 /// 

[Lldb-commits] [PATCH] D47991: Add method SBThread::StepOver with SBError parameter.

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added inline comments.



Comment at: include/lldb/API/SBThread.h:96
 
+  void StepOver(SBError ,
+lldb::RunMode stop_other_threads = lldb::eOnlyDuringStepping);

apolyakov wrote:
> aprantl wrote:
> > Where is the SBAPI documentation that is displayed on http://lldb.llvm.org 
> > ? Apparently no in this file, but it must exist *somewhere*, can you update 
> > it too, please?
> Due to http://lldb.llvm.org/: `This documentation is generated directly from 
> the source code with doxygen.`. So I think that we don't need to change 
> anything except source files.
While not perfectly consistent across SB API, most methods having an out 
SBError parameter place it last.



Comment at: include/lldb/API/SBThread.h:96
 
+  void StepOver(SBError ,
+lldb::RunMode stop_other_threads = lldb::eOnlyDuringStepping);

lemo wrote:
> apolyakov wrote:
> > aprantl wrote:
> > > Where is the SBAPI documentation that is displayed on 
> > > http://lldb.llvm.org ? Apparently no in this file, but it must exist 
> > > *somewhere*, can you update it too, please?
> > Due to http://lldb.llvm.org/: `This documentation is generated directly 
> > from the source code with doxygen.`. So I think that we don't need to 
> > change anything except source files.
> While not perfectly consistent across SB API, most methods having an out 
> SBError parameter place it last.
I believe you need to update SBThread.i as well. It is required for Python 
binding through SWIG and it also defines the Python API documentation.


https://reviews.llvm.org/D47991



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

[Lldb-commits] [PATCH] D47708: PDB support of function-level linking and splitted functions

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added a comment.

I agree, checked in binaries are not always pretty. But some coverage
depends checked in binaries (or at very least is dramatically harder to get
the same thing from source)

Are we saying that sacrificing coverage to keep tests smaller should be the
default trade off?


https://reviews.llvm.org/D47708



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

[Lldb-commits] [PATCH] D47708: PDB support of function-level linking and splitted functions

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added subscribers: zturner, lemo.
lemo added a comment.

Doesn't the LIT based test drop the split-function case (originally
produced with PGO)?

Sorry for being late to the party, but it seems beneficial to have both LIT
*and* checked in binaries since in general they are complementary: checking
against freshly built binaries only covers a matching set of toolchain
components (in particular it's hard to cover the cross-targeting scenarios).

Other than the inconvenience with Phabricator, is there a reason not to
include the original tests as well? The size of the binaries?


https://reviews.llvm.org/D47708



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

[Lldb-commits] [PATCH] D46292: Use the UUID from the minidump's CodeView Record for placeholder modules

[Leonard Mosescu via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
lemo marked an inline comment as done.
Closed by commit rL331394: Use the UUID from the minidumps CodeView 
Record for placeholder modules (authored by lemo, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D46292?vs=144790=144920#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D46292

Files:
  lldb/trunk/include/lldb/Core/Module.h
  lldb/trunk/include/lldb/Core/ModuleSpec.h
  
lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
  
lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py
  lldb/trunk/source/Commands/CommandObjectTarget.cpp
  lldb/trunk/source/Core/Module.cpp
  lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp
  lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.h
  lldb/trunk/source/Plugins/Process/minidump/MinidumpTypes.h
  lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.cpp

Index: lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py
===
--- lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py
+++ lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py
@@ -49,17 +49,36 @@
 self.process = self.target.LoadCore("fizzbuzz_no_heap.dmp")
 self.assertTrue(self.process, PROCESS_IS_VALID)
 expected_modules = [
-r"C:\Windows\System32/MSVCP120D.dll",
-r"C:\Windows\SysWOW64/kernel32.dll",
-r"C:\Users\amccarth\Documents\Visual Studio 2013\Projects\fizzbuzz\Debug/fizzbuzz.exe",
-r"C:\Windows\System32/MSVCR120D.dll",
-r"C:\Windows\SysWOW64/KERNELBASE.dll",
-r"C:\Windows\SysWOW64/ntdll.dll",
+{
+'filename' : r"C:\Windows\System32/MSVCP120D.dll",
+'uuid' : '6E51053C-E757-EB40-8D3F-9722C5BD80DD-0100',
+},
+{
+'filename' : r"C:\Windows\SysWOW64/kernel32.dll",
+'uuid' : '1B7ECBE5-5E00-1341-AB98-98D6913B52D8-0200',
+},
+{
+'filename' : r"C:\Users\amccarth\Documents\Visual Studio 2013\Projects\fizzbuzz\Debug/fizzbuzz.exe",
+'uuid' : '91B7450F-969A-F946-BF8F-2D6076EA421A-1100',
+},
+{
+'filename' : r"C:\Windows\System32/MSVCR120D.dll",
+'uuid' : '86FB8263-C446-4640-AE42-8D97B3F91FF2-0100',
+},
+{
+'filename' : r"C:\Windows\SysWOW64/KERNELBASE.dll",
+'uuid' : '4152F90B-0DCB-D44B-AC5D-186A6452E522-0100',
+},
+{
+'filename' : r"C:\Windows\SysWOW64/ntdll.dll",
+'uuid' : '6A84B0BB-2C40-5240-A16B-67650BBFE6B0-0200',
+},
 ]
 self.assertEqual(self.target.GetNumModules(), len(expected_modules))
 for module, expected in zip(self.target.modules, expected_modules):
 self.assertTrue(module.IsValid())
-self.assertEqual(module.file.fullpath, expected)
+self.assertEqual(module.file.fullpath, expected['filename'])
+self.assertEqual(module.GetUUIDString(), expected['uuid'])
 
 @expectedFailureAll(bugnumber="llvm.org/pr35193", hostoslist=["windows"])
 def test_stack_info_in_mini_dump(self):
Index: lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
===
--- lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
+++ lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
@@ -77,9 +77,49 @@
 self.target = self.dbg.GetSelectedTarget()
 self.process = self.target.LoadCore("linux-x86_64.dmp")
 self.assertTrue(self.process, PROCESS_IS_VALID)
-self.assertEqual(self.target.GetNumModules(), 9)
-for module in self.target.modules:
+expected_modules = [
+{
+'filename' : 'linux-gate.so',
+'uuid' : '4EAD28F8-88EF-3520-872B-73C6F2FE7306-C41AF22F',
+},
+{
+'filename' : 'libm-2.19.so',
+'uuid' : 'D144258E-6149-00B2-55A3-1F3FD2283A87-8670D5BC',
+},
+{
+'filename' : 'libstdc++.so.6.0.19',
+'uuid' : '76190E92-2AF7-457D-078F-75C9B15FA184-E83EB506',
+},
+{
+'filename' : 'libc-2.19.so',
+'uuid' : 'CF699A15-CAAE-64F5-0311-FC4655B86DC3-9A479789',
+},
+{
+'filename' : 'linux-x86_64',
+

[Lldb-commits] [PATCH] D46292: Use the UUID from the minidump's CodeView Record for placeholder modules

[Leonard Mosescu via Phabricator via lldb-commits]

lemo marked an inline comment as done.
lemo added inline comments.



Comment at: source/Commands/CommandObjectTarget.cpp:1390
+else {
+  strm.Printf("No object file for module: %s\n",
+  module->GetFileSpec().GetFilename().GetCString());

labath wrote:
> `strm.Format("No object file for module: {0:F}\n", module->GetFileSpec())` 
> should hopefully fit on a single line
It does not fit on a single line, but it's cleaner - done, thanks for 
suggesting it!


https://reviews.llvm.org/D46292



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

[Lldb-commits] [PATCH] D46292: Use the UUID from the minidump's CodeView Record for placeholder modules

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added inline comments.



Comment at: include/lldb/Core/Module.h:779
+  //--
+  void SetUUID(const lldb_private::UUID );
+

labath wrote:
> lemo wrote:
> > labath wrote:
> > > amccarth wrote:
> > > > Was there no SetUUID before because the UUID was considered immutable?  
> > > > I wonder if that's worth keeping.  Is the UUID always known at 
> > > > construction time?
> > > The UUID is not always know at construction time -- sometimes it can be, 
> > > if you set it on the FileSpec you use to create the module, but you don't 
> > > have to do that.
> > > 
> > > However, it is true that the UUID was constant during the lifetime of the 
> > > Module, and this changes that, which is not ideal IMO.
> > > 
> > > It seems that in the only place you call this function, you already know 
> > > the UUID. Would it be possible to do this work in the PlaceholderModule 
> > > constructor (thereby avoiding any changes to the Module API) ?
> > Good point: I agree, it's a good idea to preserve the invariant that the 
> > module UUID does not change once it's set.
> > 
> > I'd not add more complexity & arguments to the placeholder module 
> > constructor though. Moreover, even if it's technically possible to reach 
> > into the protected data members from Module I'd like to avoid doing that 
> > since Module::m_uuid is closely tied to Module::m_mutex and 
> > Module::m_did_parse_uuid (the later now renamed to m_did_set_uuid).
> > 
> > I added a check and assert to prevent Module::SetUUID from overwriting an 
> > already set UUID. (Personally I'd would've liked to have the assert be a 
> > fastfail in all builds but I remember that is somewhat against the LLDB 
> > philosophy)
> > 
> > 
> Guarding the invariant with an assert is better than nothing, but even better 
> is arranging stuff so that the invariant cannot be broken in the first place. 
> And this feels like the sort of thing that should be possible to make safe by 
> design.
> 
> How about adding a special (protected?) Module constructor, which takes a 
> UUID argument and uses it to initialize the m_uuid field. This way, it is 
> impossible by design to change the UUID mid-flight, and you still don't have 
> to reach into the protected Module fields from the other class (we could even 
> make them private if we want to).
Trying to make sure that the invariant can't be invalidated is a high bar 
considering that most Module state is protected. But I agree with the sentiment 
and here's a new iteration:

1. Made Module::SetUUID() protected
(note that the m_uuid change is conditional, not just protected by an 
lldbassert)
2. PlaceholderModule constructor is where SetUUID is called from

I'm hesitant to add another Module constructor: that class already has 2 public 
+ 1 private constructors (even using a delegating constructor, the addition of 
a new protected constructor questionable). But if you still prefer that option 
better I'll make the change.




https://reviews.llvm.org/D46292



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

[Lldb-commits] [PATCH] D46292: Use the UUID from the minidump's CodeView Record for placeholder modules

[Leonard Mosescu via Phabricator via lldb-commits]

lemo updated this revision to Diff 144790.

https://reviews.llvm.org/D46292

Files:
  include/lldb/Core/Module.h
  include/lldb/Core/ModuleSpec.h
  
packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
  
packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py
  source/Commands/CommandObjectTarget.cpp
  source/Core/Module.cpp
  source/Plugins/Process/minidump/MinidumpParser.cpp
  source/Plugins/Process/minidump/MinidumpParser.h
  source/Plugins/Process/minidump/MinidumpTypes.h
  source/Plugins/Process/minidump/ProcessMinidump.cpp

Index: source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -46,8 +46,11 @@
 //--
 class PlaceholderModule : public Module {
 public:
-  PlaceholderModule(const FileSpec _spec, const ArchSpec ) :
-Module(file_spec, arch) {}
+  PlaceholderModule(const ModuleSpec _spec) :
+Module(module_spec.GetFileSpec(), module_spec.GetArchitecture()) {
+if (module_spec.GetUUID().IsValid())
+  SetUUID(module_spec.GetUUID());
+  }
 
   // Creates a synthetic module section covering the whole module image (and
   // sets the section load address as well)
@@ -321,9 +324,10 @@
   m_is_wow64 = true;
 }
 
+const auto uuid = m_minidump_parser.GetModuleUUID(module);
 const auto file_spec =
 FileSpec(name.getValue(), true, GetArchitecture().GetTriple());
-ModuleSpec module_spec = file_spec;
+ModuleSpec module_spec(file_spec, uuid);
 Status error;
 lldb::ModuleSP module_sp = GetTarget().GetSharedModule(module_spec, );
 if (!module_sp || error.Fail()) {
@@ -335,7 +339,7 @@
   // translations (ex. identifing the module for a stack frame PC) and
   // modules/sections commands (ex. target modules list, ...)
   auto placeholder_module =
-  std::make_shared(file_spec, GetArchitecture());
+  std::make_shared(module_spec);
   placeholder_module->CreateImageSection(module, GetTarget());
   module_sp = placeholder_module;
   GetTarget().GetImages().Append(module_sp);
Index: source/Plugins/Process/minidump/MinidumpTypes.h
===
--- source/Plugins/Process/minidump/MinidumpTypes.h
+++ source/Plugins/Process/minidump/MinidumpTypes.h
@@ -43,6 +43,21 @@
 
 };
 
+enum class CvSignature : uint32_t {
+  Pdb70 = 0x53445352, // RSDS
+  ElfBuildId = 0x4270454c, // BpEL (Breakpad/Crashpad minidumps)
+};
+
+// Reference:
+// https://crashpad.chromium.org/doxygen/structcrashpad_1_1CodeViewRecordPDB70.html
+struct CvRecordPdb70 {
+  uint8_t Uuid[16];
+  llvm::support::ulittle32_t Age;
+  // char PDBFileName[];
+};
+static_assert(sizeof(CvRecordPdb70) == 20,
+  "sizeof CvRecordPdb70 is not correct!");
+
 // Reference:
 // https://msdn.microsoft.com/en-us/library/windows/desktop/ms680394.aspx
 enum class MinidumpStreamType : uint32_t {
Index: source/Plugins/Process/minidump/MinidumpParser.h
===
--- source/Plugins/Process/minidump/MinidumpParser.h
+++ source/Plugins/Process/minidump/MinidumpParser.h
@@ -17,6 +17,7 @@
 #include "lldb/Utility/ArchSpec.h"
 #include "lldb/Utility/DataBuffer.h"
 #include "lldb/Utility/Status.h"
+#include "lldb/Utility/UUID.h"
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
@@ -54,6 +55,8 @@
 
   llvm::Optional GetMinidumpString(uint32_t rva);
 
+  UUID GetModuleUUID(const MinidumpModule* module);
+
   llvm::ArrayRef GetThreads();
 
   llvm::ArrayRef GetThreadContext(const MinidumpThread );
Index: source/Plugins/Process/minidump/MinidumpParser.cpp
===
--- source/Plugins/Process/minidump/MinidumpParser.cpp
+++ source/Plugins/Process/minidump/MinidumpParser.cpp
@@ -96,6 +96,40 @@
   return parseMinidumpString(arr_ref);
 }
 
+UUID MinidumpParser::GetModuleUUID(const MinidumpModule *module) {
+  auto cv_record =
+  GetData().slice(module->CV_record.rva, module->CV_record.data_size);
+
+  // Read the CV record signature
+  const llvm::support::ulittle32_t *signature = nullptr;
+  Status error = consumeObject(cv_record, signature);
+  if (error.Fail())
+return UUID();
+
+  const CvSignature cv_signature =
+  static_cast(static_cast(*signature));
+
+  if (cv_signature == CvSignature::Pdb70) {
+// PDB70 record
+const CvRecordPdb70 *pdb70_uuid = nullptr;
+Status error = consumeObject(cv_record, pdb70_uuid);
+if (!error.Fail())
+  return UUID(pdb70_uuid, sizeof(*pdb70_uuid));
+  } else if (cv_signature == CvSignature::ElfBuildId) {
+// ELF BuildID (found in Breakpad/Crashpad generated minidumps)
+//
+// This is variable-length, but usually 20 bytes
+

[Lldb-commits] [PATCH] D46292: Use the UUID from the minidump's CodeView Record for placeholder modules

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added a comment.

Thanks for the feedback. I uploaded a new revision (incorporating some of the 
feedback, including an ELF test case)




Comment at: include/lldb/Core/Module.h:779
+  //--
+  void SetUUID(const lldb_private::UUID );
+

labath wrote:
> amccarth wrote:
> > Was there no SetUUID before because the UUID was considered immutable?  I 
> > wonder if that's worth keeping.  Is the UUID always known at construction 
> > time?
> The UUID is not always know at construction time -- sometimes it can be, if 
> you set it on the FileSpec you use to create the module, but you don't have 
> to do that.
> 
> However, it is true that the UUID was constant during the lifetime of the 
> Module, and this changes that, which is not ideal IMO.
> 
> It seems that in the only place you call this function, you already know the 
> UUID. Would it be possible to do this work in the PlaceholderModule 
> constructor (thereby avoiding any changes to the Module API) ?
Good point: I agree, it's a good idea to preserve the invariant that the module 
UUID does not change once it's set.

I'd not add more complexity & arguments to the placeholder module constructor 
though. Moreover, even if it's technically possible to reach into the protected 
data members from Module I'd like to avoid doing that since Module::m_uuid is 
closely tied to Module::m_mutex and Module::m_did_parse_uuid (the later now 
renamed to m_did_set_uuid).

I added a check and assert to prevent Module::SetUUID from overwriting an 
already set UUID. (Personally I'd would've liked to have the assert be a 
fastfail in all builds but I remember that is somewhat against the LLDB 
philosophy)





Comment at: source/Core/Module.cpp:341
+  m_uuid = uuid;
+  m_did_parse_uuid = true;
+}

amccarth wrote:
> I wonder if the lazy UUID lookup in Module::GetUUID should call this to avoid 
> code duplication.  I know it's not a lot of code, but it involves a mutex and 
> an atomic flag, so it might be nice to always have this update in just one 
> place.
Good point, but I'm afraid that would obscure the double-checked-locking in 
Module::GetUUID() so I'd rather not change it.



Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:99
 
+UUID MinidumpParser::GetModuleUUID(const MinidumpModule *module) {
+  auto cv_record =

amccarth wrote:
> I wonder if this should return an `Expected`.  The function has 
> multiple ways it can fail, in which case the error info is lost and we 
> silently return an empty UUID.
The goal is support the common CV records we expect to see from current 
toolchains and the contract for GetModuleUUID() is to either return a valid 
UUID or an empty one. 

Neither is an error: the CV record is optional and we also can't guarantee to 
handle every single CV record type.




https://reviews.llvm.org/D46292



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

[Lldb-commits] [PATCH] D46292: Use the UUID from the minidump's CodeView Record for placeholder modules

[Leonard Mosescu via Phabricator via lldb-commits]

lemo updated this revision to Diff 144751.
lemo marked 5 inline comments as done.

https://reviews.llvm.org/D46292

Files:
  include/lldb/Core/Module.h
  include/lldb/Core/ModuleSpec.h
  
packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
  
packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py
  source/Commands/CommandObjectTarget.cpp
  source/Core/Module.cpp
  source/Plugins/Process/minidump/MinidumpParser.cpp
  source/Plugins/Process/minidump/MinidumpParser.h
  source/Plugins/Process/minidump/MinidumpTypes.h
  source/Plugins/Process/minidump/ProcessMinidump.cpp

Index: source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -321,9 +321,10 @@
   m_is_wow64 = true;
 }
 
+const auto uuid = m_minidump_parser.GetModuleUUID(module);
 const auto file_spec =
 FileSpec(name.getValue(), true, GetArchitecture().GetTriple());
-ModuleSpec module_spec = file_spec;
+ModuleSpec module_spec(file_spec, uuid);
 Status error;
 lldb::ModuleSP module_sp = GetTarget().GetSharedModule(module_spec, );
 if (!module_sp || error.Fail()) {
@@ -337,6 +338,7 @@
   auto placeholder_module =
   std::make_shared(file_spec, GetArchitecture());
   placeholder_module->CreateImageSection(module, GetTarget());
+  placeholder_module->SetUUID(uuid);
   module_sp = placeholder_module;
   GetTarget().GetImages().Append(module_sp);
 }
Index: source/Plugins/Process/minidump/MinidumpTypes.h
===
--- source/Plugins/Process/minidump/MinidumpTypes.h
+++ source/Plugins/Process/minidump/MinidumpTypes.h
@@ -43,6 +43,21 @@
 
 };
 
+enum class CvSignature : uint32_t {
+  Pdb70 = 0x53445352, // RSDS
+  ElfBuildId = 0x4270454c, // BpEL (Breakpad/Crashpad minidumps)
+};
+
+// Reference:
+// https://crashpad.chromium.org/doxygen/structcrashpad_1_1CodeViewRecordPDB70.html
+struct CvRecordPdb70 {
+  uint8_t Uuid[16];
+  llvm::support::ulittle32_t Age;
+  // char PDBFileName[];
+};
+static_assert(sizeof(CvRecordPdb70) == 20,
+  "sizeof CvRecordPdb70 is not correct!");
+
 // Reference:
 // https://msdn.microsoft.com/en-us/library/windows/desktop/ms680394.aspx
 enum class MinidumpStreamType : uint32_t {
Index: source/Plugins/Process/minidump/MinidumpParser.h
===
--- source/Plugins/Process/minidump/MinidumpParser.h
+++ source/Plugins/Process/minidump/MinidumpParser.h
@@ -17,6 +17,7 @@
 #include "lldb/Utility/ArchSpec.h"
 #include "lldb/Utility/DataBuffer.h"
 #include "lldb/Utility/Status.h"
+#include "lldb/Utility/UUID.h"
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
@@ -54,6 +55,8 @@
 
   llvm::Optional GetMinidumpString(uint32_t rva);
 
+  UUID GetModuleUUID(const MinidumpModule* module);
+
   llvm::ArrayRef GetThreads();
 
   llvm::ArrayRef GetThreadContext(const MinidumpThread );
Index: source/Plugins/Process/minidump/MinidumpParser.cpp
===
--- source/Plugins/Process/minidump/MinidumpParser.cpp
+++ source/Plugins/Process/minidump/MinidumpParser.cpp
@@ -96,6 +96,40 @@
   return parseMinidumpString(arr_ref);
 }
 
+UUID MinidumpParser::GetModuleUUID(const MinidumpModule *module) {
+  auto cv_record =
+  GetData().slice(module->CV_record.rva, module->CV_record.data_size);
+
+  // Read the CV record signature
+  const llvm::support::ulittle32_t *signature = nullptr;
+  Status error = consumeObject(cv_record, signature);
+  if (error.Fail())
+return UUID();
+
+  const CvSignature cv_signature =
+  static_cast(static_cast(*signature));
+
+  if (cv_signature == CvSignature::Pdb70) {
+// PDB70 record
+const CvRecordPdb70 *pdb70_uuid = nullptr;
+Status error = consumeObject(cv_record, pdb70_uuid);
+if (!error.Fail())
+  return UUID(pdb70_uuid, sizeof(*pdb70_uuid));
+  } else if (cv_signature == CvSignature::ElfBuildId) {
+// ELF BuildID (found in Breakpad/Crashpad generated minidumps)
+//
+// This is variable-length, but usually 20 bytes
+// as the binutils ld default is a SHA-1 hash.
+// (We'll handle only 16 and 20 bytes signatures,
+// matching LLDB support for UUIDs)
+//
+if (cv_record.size() == 16 || cv_record.size() == 20)
+  return UUID(cv_record.data(), cv_record.size());
+  }
+
+  return UUID();
+}
+
 llvm::ArrayRef MinidumpParser::GetThreads() {
   llvm::ArrayRef data = GetStream(MinidumpStreamType::ThreadList);
 
Index: source/Core/Module.cpp
===
--- source/Core/Module.cpp
+++ source/Core/Module.cpp
@@ -38,6 +38,7 @@
 #include "lldb/Target/Process.h"
 #include "lldb/Target/Target.h"
 #include 

[Lldb-commits] [PATCH] D46292: Use the UUID from the minidump's CodeView Record for placeholder modules

[Leonard Mosescu via Phabricator via lldb-commits]

lemo created this revision.
lemo added reviewers: amccarth, clayborg, labath.
lemo added a project: LLDB.

This change adds support for two types of Minidump CodeView records:

1. PDB70 (reference: 
https://crashpad.chromium.org/doxygen/structcrashpad_1_1CodeViewRecordPDB70.html)

This is by far the most common record type.

2. ELF BuildID (found in Breakpad/Crashpad generated minidumps)

This would set a proper UUID for placeholder modules, in turn enabling an 
accurate match with local module images.


https://reviews.llvm.org/D46292

Files:
  include/lldb/Core/Module.h
  include/lldb/Core/ModuleSpec.h
  
packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py
  source/Commands/CommandObjectTarget.cpp
  source/Core/Module.cpp
  source/Plugins/Process/minidump/MinidumpParser.cpp
  source/Plugins/Process/minidump/MinidumpParser.h
  source/Plugins/Process/minidump/MinidumpTypes.h
  source/Plugins/Process/minidump/ProcessMinidump.cpp

Index: source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -321,9 +321,10 @@
   m_is_wow64 = true;
 }
 
+const auto uuid = m_minidump_parser.GetModuleUUID(module);
 const auto file_spec =
 FileSpec(name.getValue(), true, GetArchitecture().GetTriple());
-ModuleSpec module_spec = file_spec;
+ModuleSpec module_spec(file_spec, uuid);
 Status error;
 lldb::ModuleSP module_sp = GetTarget().GetSharedModule(module_spec, );
 if (!module_sp || error.Fail()) {
@@ -337,6 +338,7 @@
   auto placeholder_module =
   std::make_shared(file_spec, GetArchitecture());
   placeholder_module->CreateImageSection(module, GetTarget());
+  placeholder_module->SetUUID(uuid);
   module_sp = placeholder_module;
   GetTarget().GetImages().Append(module_sp);
 }
Index: source/Plugins/Process/minidump/MinidumpTypes.h
===
--- source/Plugins/Process/minidump/MinidumpTypes.h
+++ source/Plugins/Process/minidump/MinidumpTypes.h
@@ -43,6 +43,21 @@
 
 };
 
+enum class CvSignature : uint32_t {
+  Pdb70 = 0x53445352, // RSDS
+  ElfBuildId = 0x4270454c, // BpEL (Breakpad/Crashpad minidumps)
+};
+
+// Reference:
+// https://crashpad.chromium.org/doxygen/structcrashpad_1_1CodeViewRecordPDB70.html
+struct CvRecordPdb70 {
+  uint8_t Uuid[16];
+  llvm::support::ulittle32_t Age;
+  // char PDBFileName[];
+};
+static_assert(sizeof(CvRecordPdb70) == 20,
+  "sizeof CvRecordPdb70 is not correct!");
+
 // Reference:
 // https://msdn.microsoft.com/en-us/library/windows/desktop/ms680394.aspx
 enum class MinidumpStreamType : uint32_t {
Index: source/Plugins/Process/minidump/MinidumpParser.h
===
--- source/Plugins/Process/minidump/MinidumpParser.h
+++ source/Plugins/Process/minidump/MinidumpParser.h
@@ -17,6 +17,7 @@
 #include "lldb/Utility/ArchSpec.h"
 #include "lldb/Utility/DataBuffer.h"
 #include "lldb/Utility/Status.h"
+#include "lldb/Utility/UUID.h"
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
@@ -54,6 +55,8 @@
 
   llvm::Optional GetMinidumpString(uint32_t rva);
 
+  UUID GetModuleUUID(const MinidumpModule* module);
+
   llvm::ArrayRef GetThreads();
 
   llvm::ArrayRef GetThreadContext(const MinidumpThread );
Index: source/Plugins/Process/minidump/MinidumpParser.cpp
===
--- source/Plugins/Process/minidump/MinidumpParser.cpp
+++ source/Plugins/Process/minidump/MinidumpParser.cpp
@@ -96,6 +96,40 @@
   return parseMinidumpString(arr_ref);
 }
 
+UUID MinidumpParser::GetModuleUUID(const MinidumpModule *module) {
+  auto cv_record =
+  GetData().slice(module->CV_record.rva, module->CV_record.data_size);
+
+  // Read the CV record signature
+  const llvm::support::ulittle32_t *signature = nullptr;
+  Status error = consumeObject(cv_record, signature);
+  if (error.Fail())
+return UUID();
+
+  const CvSignature cv_signature =
+  static_cast(static_cast(*signature));
+
+  if (cv_signature == CvSignature::Pdb70) {
+// PDB70 record
+const CvRecordPdb70 *pdb70_uuid = nullptr;
+Status error = consumeObject(cv_record, pdb70_uuid);
+if (!error.Fail())
+  return UUID(pdb70_uuid, sizeof(*pdb70_uuid));
+  } else if (cv_signature == CvSignature::ElfBuildId) {
+// ELF BuildID (found in Breakpad/Crashpad generated minidumps)
+//
+// This is variable-length, but usually 20 bytes
+// as the binutils ld default is a SHA-1 hash.
+// (We'll handle only 16 and 20 bytes signatures,
+// matching LLDB support for UUIDs)
+//
+if (cv_record.size() == 16 || cv_record.size() == 20)
+  return UUID(cv_record.data(), cv_record.size());
+  }
+
+  return UUID();
+}
+
 llvm::ArrayRef 

[Lldb-commits] [PATCH] D45628: [LLDB] Support compressed debug info sections (.zdebug*)

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added subscribers: bgianfo, clayborg, alur, labath, penryu, lemo.
lemo added a comment.

Hi Erik, the review is still marked as requiring changes. Once that is
sorted out I'd be happy to submit this on your behalf (what is the base SVN
revision for the latest patch?)

Davide Italiano, is all the CR feedback addressed in the latest revision?


https://reviews.llvm.org/D45628



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

[Lldb-commits] [PATCH] D45700: Improve LLDB's handling of non-local minidumps

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added subscribers: bgianfo, penryu.
lemo added a comment.

> It looks like nobody except me is worried about the
>  module-without-an-object-file situation, so I guess we can try this out and
>  see how it goes.

Sorry Pavel, I meant to respond to this: most of the code seems to
explicitly handle this case (module-without-object-file), I just had to fix
a couple of cases. It's possible that more fixes will be required, but the
intention seems to be to accommodate for missing object files so at least
in this area I didn't have to break new ground.

I also considered creating placeholder object files, but it proved a bit
more intrusive since there are numerous places where it's assumed that
object files map to a real file which can be read and written to. Maybe at
some point we'll need to reconsider this (placeholder object files) but for
the initial iteration the placeholder modules seems to be sufficient. The
only downside I noticed is mostly cosmetic, for example things like "target
modules dump objfile" may print empty lines for the missing object files.

The test you've added here has been failing on windows though. I've tried

> to fix this in r330314, but it meant modifying the module.file.fullpath
>  output expectations. I'm not sure where you're going with the minidump
>  support, but if you are bothered by module.file.fullpath containing a
>  forward slash, you may want to look into fixing the SBFileSpec behavior.

Thanks!


Repository:
  rL LLVM

https://reviews.llvm.org/D45700



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

[Lldb-commits] [PATCH] D45700: Improve LLDB's handling of non-local minidumps

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added subscribers: amccarth, clayborg, labath, zturner.
lemo marked an inline comment as done.
lemo added a comment.

Greg/Pavel, does the latest revision look good to you? Thanks!


https://reviews.llvm.org/D45700



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

[Lldb-commits] [PATCH] D45700: Improve LLDB's handling of non-local minidumps

[Leonard Mosescu via Phabricator via lldb-commits]

lemo marked an inline comment as done.
lemo added a comment.

In https://reviews.llvm.org/D45700#1070491, @amccarth wrote:

> LGTM, but consider highlighting the side effect to `target` when the factory 
> makes a Placeholder module.


Good observation: taking a step back, the factory introduces too much coupling, 
especially if we want to extend this placeholder module approach to other uses. 
Because of this, I reverted back to the standalone 
PlaceholderModule::CreateImageSection() approach. Thanks Adrian!




Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:70
+  // Creates a synthetic module section covering the whole module image
+  void CreateImageSection(const MinidumpModule *module, Target& target) {
+const ConstString section_name(".module_image");

amccarth wrote:
> I didn't notice before that target is a non-const ref.  Maybe the comment 
> should explain why target is modified (and/or maybe in 
> PlaceholderModule::Create).
Updated the function comment. This is similar to how other places set the 
section load address (ex. ObjectFileELF::SetLoadAddress)


https://reviews.llvm.org/D45700



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

[Lldb-commits] [PATCH] D45700: Improve LLDB's handling of non-local minidumps

[Leonard Mosescu via Phabricator via lldb-commits]

lemo updated this revision to Diff 142960.

https://reviews.llvm.org/D45700

Files:
  
packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
  
packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py
  source/Commands/CommandObjectTarget.cpp
  source/Core/Module.cpp
  source/Core/Section.cpp
  source/Plugins/Process/minidump/ProcessMinidump.cpp
  source/Plugins/Process/minidump/ProcessMinidump.h

Index: source/Plugins/Process/minidump/ProcessMinidump.h
===
--- source/Plugins/Process/minidump/ProcessMinidump.h
+++ source/Plugins/Process/minidump/ProcessMinidump.h
@@ -61,6 +61,8 @@
 
   uint32_t GetPluginVersion() override;
 
+  SystemRuntime *GetSystemRuntime() override { return nullptr; }
+
   Status DoDestroy() override;
 
   void RefreshStateAfterStop() override;
Index: source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -19,6 +19,7 @@
 #include "lldb/Core/State.h"
 #include "lldb/Target/DynamicLoader.h"
 #include "lldb/Target/MemoryRegionInfo.h"
+#include "lldb/Target/SectionLoadList.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Target/UnixSignals.h"
 #include "lldb/Utility/DataBufferLLVM.h"
@@ -31,9 +32,53 @@
 // C includes
 // C++ includes
 
+using namespace lldb;
 using namespace lldb_private;
 using namespace minidump;
 
+//--
+/// A placeholder module used for minidumps, where the original
+/// object files may not be available (so we can't parse the object
+/// files to extract the set of sections/segments)
+///
+/// This placeholder module has a single synthetic section (.module_image)
+/// which represents the module memory range covering the whole module.
+//--
+class PlaceholderModule : public Module {
+public:
+  PlaceholderModule(const FileSpec _spec, const ArchSpec ) :
+Module(file_spec, arch) {}
+
+  // Creates a synthetic module section covering the whole module image
+  // (and sets the section load address as well)
+  void CreateImageSection(const MinidumpModule *module, Target& target) {
+const ConstString section_name(".module_image");
+lldb::SectionSP section_sp(new Section(
+shared_from_this(), // Module to which this section belongs.
+nullptr,// ObjectFile
+0,  // Section ID.
+section_name,   // Section name.
+eSectionTypeContainer,  // Section type.
+module->base_of_image,  // VM address.
+module->size_of_image,  // VM size in bytes of this section.
+0,  // Offset of this section in the file.
+module->size_of_image,  // Size of the section as found in the file.
+12, // Alignment of the section (log2)
+0,  // Flags for this section.
+1));// Number of host bytes per target byte
+section_sp->SetPermissions(ePermissionsExecutable | ePermissionsReadable);
+GetSectionList()->AddSection(section_sp);
+target.GetSectionLoadList().SetSectionLoadAddress(
+section_sp, module->base_of_image);
+  }
+
+  ObjectFile *GetObjectFile() override { return nullptr; }
+
+  SectionList *GetSectionList() override {
+return Module::GetUnifiedSectionList();
+  }
+};
+
 ConstString ProcessMinidump::GetPluginNameStatic() {
   static ConstString g_name("minidump");
   return g_name;
@@ -281,7 +326,18 @@
 Status error;
 lldb::ModuleSP module_sp = GetTarget().GetSharedModule(module_spec, );
 if (!module_sp || error.Fail()) {
-  continue;
+  // We failed to locate a matching local object file. Fortunately,
+  // the minidump format encodes enough information about each module's
+  // memory range to allow us to create placeholder modules.
+  //
+  // This enables most LLDB functionality involving address-to-module
+  // translations (ex. identifing the module for a stack frame PC) and
+  // modules/sections commands (ex. target modules list, ...)
+  auto placeholder_module =
+  std::make_shared(file_spec, GetArchitecture());
+  placeholder_module->CreateImageSection(module, GetTarget());
+  module_sp = placeholder_module;
+  GetTarget().GetImages().Append(module_sp);
 }
 
 if (log) {
Index: source/Core/Section.cpp
===
--- source/Core/Section.cpp
+++ source/Core/Section.cpp
@@ -326,10 +326,11 @@
 // The top most section prints the module basename
 const char *name = NULL;
 ModuleSP module_sp(GetModule());
-const FileSpec _spec = m_obj_file->GetFileSpec();
 
-if 

[Lldb-commits] [PATCH] D45700: Improve LLDB's handling of non-local minidumps

[Leonard Mosescu via Phabricator via lldb-commits]

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



Comment at: 
packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py:53
+for module in self.target.modules:
+self.assertTrue(module.IsValid())
+

labath wrote:
> lemo wrote:
> > labath wrote:
> > > Could we strengthen these assertions a bit. Given that this is static 
> > > data you are loading, I don't see why you couldn't hard-code the names of 
> > > the modules you should expect.
> > Done.
> > 
> > Just curious, do we have any support for golden output files? (it would be 
> > great for tests like this)
> If you're feeling adventurous, you can try rewriting this as a lit test. You 
> should be able to just do a `lldb -c my-core -o "image list"` and FileCheck 
> the output.
Thanks for the tip, I'll keep this in mind for the future.


https://reviews.llvm.org/D45700



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

[Lldb-commits] [PATCH] D45700: Improve LLDB's handling of non-local minidumps

[Leonard Mosescu via Phabricator via lldb-commits]

lemo marked 4 inline comments as done.
lemo added inline comments.



Comment at: 
packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py:53
+for module in self.target.modules:
+self.assertTrue(module.IsValid())
+

labath wrote:
> Could we strengthen these assertions a bit. Given that this is static data 
> you are loading, I don't see why you couldn't hard-code the names of the 
> modules you should expect.
Done.

Just curious, do we have any support for golden output files? (it would be 
great for tests like this)



Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:53
+  // Creates a synthetic module section covering the whole module image
+  void CreateImageSection(const MinidumpModule *module, Target& target) {
+const ConstString section_name(".module_image");

labath wrote:
> lemo wrote:
> > amccarth wrote:
> > > I wonder if this should just be part of the constructor.  I don't see a 
> > > scenario where you'd create a PlaceholderModule and not want to create 
> > > exactly one fake image section.  I know the style guide is against doing 
> > > lots of work in a constructor, but that's mostly because it's hard to 
> > > report failures, which you're not doing here anyway.
> > Thanks for the suggestion. I agree, this would look a bit cleaner to fold 
> > everything in the constructor (except the extra arguments to the 
> > constructor, but it will probably still be a net win)
> > 
> > The reason for a separate method is the little "shared_from_this()". It 
> > can't be done from the constructor since the object is not yet managed by a 
> > shared_ptr, so we need to do it post-construction.
> This can be solved by hiding the constructor and having a static factory 
> function which returns a shared_ptr.
The factory is a great idea, done. (one downside with a private constructor is 
that we lose the benefits of std::make_shared though)



Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:47
+//--
+class PlaceholderModule : public Module {
+public:

labath wrote:
> clayborg wrote:
> > I would be worth putting this class maybe in the same folder as 
> > lldb_private::Module and possibly renaming it. I can see this kind of thing 
> > being useful for symbolication in general and it won't be limited to use in 
> > minidumps. It should have enough accessors that allows an external client 
> > to modify everything.
> I concur. Besides postmortem, we can run into the situation where we cannot 
> access the loaded modules for live debugging as well.
Thanks, I agree. I think this needs a bit more baking first though, I say let's 
put it through some use first (and maybe identify a 2nd use case).
(in particular we'd also need a more generic interface and I'd like to avoid 
creating an overly complex solution which may not even fit future use cases)



Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:79
+private:
+  SectionList m_sections;
+};

clayborg wrote:
> Just use lldb_private::Module::m_sections_ap and put the sections in there? 
> Any reason not to?
Thanks, done. (no good reason, I was just extra cautious not to introduce any 
unexpected side-effects)


https://reviews.llvm.org/D45700



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

[Lldb-commits] [PATCH] D45700: Improve LLDB's handling of non-local minidumps

[Leonard Mosescu via Phabricator via lldb-commits]

lemo updated this revision to Diff 142799.
lemo edited the summary of this revision.
lemo added a comment.

Thanks for the feedback.

PS. The sample output in the description if from LLDB running on Linux, reading 
minidumps captured on Windows.


https://reviews.llvm.org/D45700

Files:
  
packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
  
packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py
  source/Commands/CommandObjectTarget.cpp
  source/Core/Module.cpp
  source/Core/Section.cpp
  source/Plugins/Process/minidump/ProcessMinidump.cpp
  source/Plugins/Process/minidump/ProcessMinidump.h

Index: source/Plugins/Process/minidump/ProcessMinidump.h
===
--- source/Plugins/Process/minidump/ProcessMinidump.h
+++ source/Plugins/Process/minidump/ProcessMinidump.h
@@ -61,6 +61,8 @@
 
   uint32_t GetPluginVersion() override;
 
+  SystemRuntime *GetSystemRuntime() override { return nullptr; }
+
   Status DoDestroy() override;
 
   void RefreshStateAfterStop() override;
Index: source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -19,6 +19,7 @@
 #include "lldb/Core/State.h"
 #include "lldb/Target/DynamicLoader.h"
 #include "lldb/Target/MemoryRegionInfo.h"
+#include "lldb/Target/SectionLoadList.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Target/UnixSignals.h"
 #include "lldb/Utility/DataBufferLLVM.h"
@@ -31,9 +32,63 @@
 // C includes
 // C++ includes
 
+using namespace lldb;
 using namespace lldb_private;
 using namespace minidump;
 
+//--
+/// A placeholder module used for minidumps, where the original
+/// object files may not be available (so we can't parse the object
+/// files to extract the set of sections/segments)
+///
+/// This placeholder module has a single synthetic section (.module_image)
+/// which represents the module memory range covering the whole module.
+//--
+class PlaceholderModule : public Module {
+public:
+  static std::shared_ptr Create(const FileSpec _spec,
+   const ArchSpec ,
+   const MinidumpModule *module,
+   Target ) {
+std::shared_ptr placeholder_module(
+new PlaceholderModule(file_spec, arch));
+placeholder_module->CreateImageSection(module, target);
+return placeholder_module;
+  }
+
+  ObjectFile *GetObjectFile() override { return nullptr; }
+
+  SectionList *GetSectionList() override {
+return Module::GetUnifiedSectionList();
+  }
+
+private:
+  PlaceholderModule(const FileSpec _spec, const ArchSpec ) :
+Module(file_spec, arch) {}
+
+  // Creates a synthetic module section covering the whole module image
+  void CreateImageSection(const MinidumpModule *module, Target& target) {
+const ConstString section_name(".module_image");
+lldb::SectionSP section_sp(new Section(
+shared_from_this(), // Module to which this section belongs.
+nullptr,// ObjectFile
+0,  // Section ID.
+section_name,   // Section name.
+eSectionTypeContainer,  // Section type.
+module->base_of_image,  // VM address.
+module->size_of_image,  // VM size in bytes of this section.
+0,  // Offset of this section in the file.
+module->size_of_image,  // Size of the section as found in the file.
+12, // Alignment of the section (log2)
+0,  // Flags for this section.
+1));// Number of host bytes per target byte
+section_sp->SetPermissions(ePermissionsExecutable | ePermissionsReadable);
+GetSectionList()->AddSection(section_sp);
+target.GetSectionLoadList().SetSectionLoadAddress(
+section_sp, module->base_of_image);
+  }
+};
+
 ConstString ProcessMinidump::GetPluginNameStatic() {
   static ConstString g_name("minidump");
   return g_name;
@@ -281,7 +336,16 @@
 Status error;
 lldb::ModuleSP module_sp = GetTarget().GetSharedModule(module_spec, );
 if (!module_sp || error.Fail()) {
-  continue;
+  // We failed to locate a matching local object file. Fortunately,
+  // the minidump format encodes enough information about each module's
+  // memory range to allow us to create placeholder modules.
+  //
+  // This enables most LLDB functionality involving address-to-module
+  // translations (ex. identifing the module for a stack frame PC) and
+  // modules/sections commands (ex. target modules list, ...)
+  module_sp =