Re: [Lldb-commits] [PATCH] D23545: Minidump parsing

[Pavel Labath via lldb-commits]

labath added inline comments.


Comment at: lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp:146
@@ +145,3 @@
+break;
+}
+

You have a "enumeration not handled in a switch" warning here. Could you do 
something about that?


Repository:
  rL LLVM

https://reviews.llvm.org/D23545



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

Re: [Lldb-commits] [PATCH] D23545: Minidump parsing

[Dimitar Vlahovski via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL280356: Minidump parsing (authored by dvlahovski).

Changed prior to commit:
  https://reviews.llvm.org/D23545?vs=69340=69988#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D23545

Files:
  lldb/trunk/cmake/LLDBDependencies.cmake
  lldb/trunk/source/Plugins/Process/CMakeLists.txt
  lldb/trunk/source/Plugins/Process/minidump/CMakeLists.txt
  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/MinidumpTypes.h
  lldb/trunk/unittests/Process/CMakeLists.txt
  lldb/trunk/unittests/Process/minidump/CMakeLists.txt
  lldb/trunk/unittests/Process/minidump/Inputs/linux-x86_64.cpp
  lldb/trunk/unittests/Process/minidump/Inputs/linux-x86_64.dmp
  lldb/trunk/unittests/Process/minidump/MinidumpParserTest.cpp

Index: lldb/trunk/cmake/LLDBDependencies.cmake
===
--- lldb/trunk/cmake/LLDBDependencies.cmake
+++ lldb/trunk/cmake/LLDBDependencies.cmake
@@ -81,6 +81,7 @@
   lldbPluginInstrumentationRuntimeThreadSanitizer
   lldbPluginSystemRuntimeMacOSX
   lldbPluginProcessElfCore
+  lldbPluginProcessMinidump
   lldbPluginJITLoaderGDB
   lldbPluginExpressionParserClang
   lldbPluginExpressionParserGo
Index: lldb/trunk/unittests/Process/minidump/Inputs/linux-x86_64.cpp
===
--- lldb/trunk/unittests/Process/minidump/Inputs/linux-x86_64.cpp
+++ lldb/trunk/unittests/Process/minidump/Inputs/linux-x86_64.cpp
@@ -0,0 +1,25 @@
+// Example source from breakpad's linux tutorial
+// https://chromium.googlesource.com/breakpad/breakpad/+/master/docs/linux_starter_guide.md
+
+#include 
+#include 
+#include 
+
+#include "client/linux/handler/exception_handler.h"
+
+
+static bool dumpCallback(const google_breakpad::MinidumpDescriptor& descriptor,
+void* context, bool succeeded) {
+printf("Dump path: %s\n", descriptor.path());
+return succeeded;
+}
+
+void crash() { volatile int* a = (int*)(NULL); *a = 1; }
+
+int main(int argc, char* argv[]) {
+google_breakpad::MinidumpDescriptor descriptor("/tmp");
+google_breakpad::ExceptionHandler eh(descriptor, NULL, dumpCallback, NULL, true, -1);
+printf("pid: %d\n", getpid());
+crash();
+return 0;
+}
Index: lldb/trunk/unittests/Process/minidump/MinidumpParserTest.cpp
===
--- lldb/trunk/unittests/Process/minidump/MinidumpParserTest.cpp
+++ lldb/trunk/unittests/Process/minidump/MinidumpParserTest.cpp
@@ -0,0 +1,97 @@
+//===-- MinidumpTypesTest.cpp ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+// Project includes
+#include "Plugins/Process/minidump/MinidumpParser.h"
+#include "Plugins/Process/minidump/MinidumpTypes.h"
+
+// Other libraries and framework includes
+#include "gtest/gtest.h"
+
+#include "lldb/Core/ArchSpec.h"
+#include "lldb/Core/DataExtractor.h"
+#include "lldb/Host/FileSpec.h"
+
+#include "llvm/ADT/Optional.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
+
+// C includes
+
+// C++ includes
+#include 
+
+extern const char *TestMainArgv0;
+
+using namespace lldb_private;
+using namespace minidump;
+
+class MinidumpParserTest : public testing::Test
+{
+public:
+void
+SetUp() override
+{
+llvm::StringRef dmp_folder = llvm::sys::path::parent_path(TestMainArgv0);
+inputs_folder = dmp_folder;
+llvm::sys::path::append(inputs_folder, "Inputs");
+}
+
+void
+SetUpData(const char *minidump_filename, size_t load_size = SIZE_MAX)
+{
+llvm::SmallString<128> filename = inputs_folder;
+llvm::sys::path::append(filename, minidump_filename);
+FileSpec minidump_file(filename.c_str(), false);
+lldb::DataBufferSP data_sp(minidump_file.MemoryMapFileContents(0, load_size));
+llvm::Optional optional_parser = MinidumpParser::Create(data_sp);
+ASSERT_TRUE(optional_parser.hasValue());
+parser.reset(new MinidumpParser(optional_parser.getValue()));
+ASSERT_GT(parser->GetByteSize(), 0UL);
+}
+
+llvm::SmallString<128> inputs_folder;
+std::unique_ptr parser;
+};
+
+TEST_F(MinidumpParserTest, GetThreads)
+{
+SetUpData("linux-x86_64.dmp");
+llvm::Optional thread_list;
+
+thread_list = parser->GetThreads();
+ASSERT_TRUE(thread_list.hasValue());
+ASSERT_EQ(1UL, thread_list->size());
+
+const MinidumpThread *thread = thread_list.getValue()[0];
+ASSERT_EQ(16001UL, thread->thread_id);

Re: [Lldb-commits] [PATCH] D23545: Minidump parsing

[Adrian McCarthy via lldb-commits]

amccarth added a comment.

LGTM.


https://reviews.llvm.org/D23545



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

Re: [Lldb-commits] [PATCH] D23545: Minidump parsing

[Zachary Turner via lldb-commits]

zturner accepted this revision.
zturner added a reviewer: zturner.
This revision is now accepted and ready to land.


Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:60
@@ +59,3 @@
+MinidumpParser parser(data_buf_sp, header, directory_map);
+return llvm::Optional(parser);
+}

You can just write `return parser` here.  It's implicitly convertible to an 
`llvm::Optional<>`.


Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:65
@@ +64,3 @@
+   const llvm::DenseMap _map)
+: m_data_sp(data_buf_sp), m_header(header), m_directory_map(directory_map)
+{

This isn't really a performance critical class, so it doesn't matter too much, 
but you could `std::move(directory_map)` to avoid the copy.


https://reviews.llvm.org/D23545



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

Re: [Lldb-commits] [PATCH] D23545: Minidump parsing

[Dimitar Vlahovski via lldb-commits]

dvlahovski updated this revision to Diff 69340.
dvlahovski added a comment.

Making MinidumpParser constuctor private


https://reviews.llvm.org/D23545

Files:
  cmake/LLDBDependencies.cmake
  source/Plugins/Process/CMakeLists.txt
  source/Plugins/Process/minidump/CMakeLists.txt
  source/Plugins/Process/minidump/MinidumpParser.cpp
  source/Plugins/Process/minidump/MinidumpParser.h
  source/Plugins/Process/minidump/MinidumpTypes.cpp
  source/Plugins/Process/minidump/MinidumpTypes.h
  unittests/Process/CMakeLists.txt
  unittests/Process/minidump/CMakeLists.txt
  unittests/Process/minidump/Inputs/linux-x86_64.dmp
  unittests/Process/minidump/MinidumpParserTest.cpp

Index: unittests/Process/minidump/MinidumpParserTest.cpp
===
--- /dev/null
+++ unittests/Process/minidump/MinidumpParserTest.cpp
@@ -0,0 +1,97 @@
+//===-- MinidumpTypesTest.cpp ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+// Project includes
+#include "Plugins/Process/minidump/MinidumpParser.h"
+#include "Plugins/Process/minidump/MinidumpTypes.h"
+
+// Other libraries and framework includes
+#include "gtest/gtest.h"
+
+#include "lldb/Core/ArchSpec.h"
+#include "lldb/Core/DataExtractor.h"
+#include "lldb/Host/FileSpec.h"
+
+#include "llvm/ADT/Optional.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
+
+// C includes
+
+// C++ includes
+#include 
+
+extern const char *TestMainArgv0;
+
+using namespace lldb_private;
+using namespace minidump;
+
+class MinidumpParserTest : public testing::Test
+{
+public:
+void
+SetUp() override
+{
+llvm::StringRef dmp_folder = llvm::sys::path::parent_path(TestMainArgv0);
+inputs_folder = dmp_folder;
+llvm::sys::path::append(inputs_folder, "Inputs");
+}
+
+void
+SetUpData(const char *minidump_filename, size_t load_size = SIZE_MAX)
+{
+llvm::SmallString<128> filename = inputs_folder;
+llvm::sys::path::append(filename, minidump_filename);
+FileSpec minidump_file(filename.c_str(), false);
+lldb::DataBufferSP data_sp(minidump_file.MemoryMapFileContents(0, load_size));
+llvm::Optional optional_parser = MinidumpParser::Create(data_sp);
+ASSERT_TRUE(optional_parser.hasValue());
+parser.reset(new MinidumpParser(optional_parser.getValue()));
+ASSERT_GT(parser->GetByteSize(), 0UL);
+}
+
+llvm::SmallString<128> inputs_folder;
+std::unique_ptr parser;
+};
+
+TEST_F(MinidumpParserTest, GetThreads)
+{
+SetUpData("linux-x86_64.dmp");
+llvm::Optional thread_list;
+
+thread_list = parser->GetThreads();
+ASSERT_TRUE(thread_list.hasValue());
+ASSERT_EQ(1UL, thread_list->size());
+
+const MinidumpThread *thread = thread_list.getValue()[0];
+ASSERT_EQ(16001UL, thread->thread_id);
+}
+
+TEST_F(MinidumpParserTest, GetThreadsTruncatedFile)
+{
+SetUpData("linux-x86_64.dmp", 200);
+llvm::Optional thread_list;
+
+thread_list = parser->GetThreads();
+ASSERT_FALSE(thread_list.hasValue());
+}
+
+TEST_F(MinidumpParserTest, GetArchitecture)
+{
+SetUpData("linux-x86_64.dmp");
+ASSERT_EQ(llvm::Triple::ArchType::x86_64, parser->GetArchitecture().GetTriple().getArch());
+}
+
+TEST_F(MinidumpParserTest, GetMiscInfo)
+{
+SetUpData("linux-x86_64.dmp");
+const MinidumpMiscInfo *misc_info = parser->GetMiscInfo();
+ASSERT_EQ(nullptr, misc_info);
+// linux breakpad generated minidump files don't have misc info stream
+}
Index: unittests/Process/minidump/CMakeLists.txt
===
--- /dev/null
+++ unittests/Process/minidump/CMakeLists.txt
@@ -0,0 +1,8 @@
+add_lldb_unittest(LLDBMinidumpTests
+  MinidumpParserTest.cpp
+  )
+
+set(test_inputs
+   linux-x86_64.dmp)
+
+add_unittest_inputs(LLDBMinidumpTests "${test_inputs}")
Index: unittests/Process/CMakeLists.txt
===
--- unittests/Process/CMakeLists.txt
+++ unittests/Process/CMakeLists.txt
@@ -1 +1,2 @@
 add_subdirectory(gdb-remote)
+add_subdirectory(minidump)
Index: source/Plugins/Process/minidump/MinidumpTypes.h
===
--- /dev/null
+++ source/Plugins/Process/minidump/MinidumpTypes.h
@@ -0,0 +1,298 @@
+//===-- MinidumpTypes.h -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef 

Re: [Lldb-commits] [PATCH] D23545: Minidump parsing

[Pavel Labath via lldb-commits]

labath added a comment.

Looks fine to me. Adrian, Zachary, any more thoughts here?



Comment at: source/Plugins/Process/minidump/MinidumpParser.h:42
@@ +41,3 @@
+public:
+explicit MinidumpParser(const lldb::DataBufferSP _buf_sp, const 
MinidumpHeader *header,
+const llvm::DenseMap _map);

If you have a `Create` function, the constructor should probably be private 
(also, `explicit` is not necessary anymore).


https://reviews.llvm.org/D23545



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

Re: [Lldb-commits] [PATCH] D23545: Minidump parsing

[Dimitar Vlahovski via lldb-commits]

dvlahovski added a comment.

I changed all of the `llvm::Optional` returning functions to 
return only `const type *`
and signalize for 'failure' by returning a `nullptr`. In the cases where I 
return objects (e.g. vector of threads)
I'm still using the `llvm::Optional` pattern.
I also think that using `llvm::Error` is a good idea, but I think that it'll be 
difficult, because of 
collisions with the `lldb::Error`, and also at some point I have to convert 
objects from one type to the other.

So that's why I stick to the `llvm::Optional`.
I also used it on the creation of `MinidumpParser` (following somewhat the 
pattern that you proposed)


https://reviews.llvm.org/D23545



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

Re: [Lldb-commits] [PATCH] D23545: Minidump parsing

[Dimitar Vlahovski via lldb-commits]

dvlahovski updated this revision to Diff 69254.
dvlahovski added a comment.

Changed the constructing pattern of MinidumpParser

Now there is a static Create method that returns
and llvm::Optional MinidumpParser.


https://reviews.llvm.org/D23545

Files:
  cmake/LLDBDependencies.cmake
  source/Plugins/Process/CMakeLists.txt
  source/Plugins/Process/minidump/CMakeLists.txt
  source/Plugins/Process/minidump/MinidumpParser.cpp
  source/Plugins/Process/minidump/MinidumpParser.h
  source/Plugins/Process/minidump/MinidumpTypes.cpp
  source/Plugins/Process/minidump/MinidumpTypes.h
  unittests/Process/CMakeLists.txt
  unittests/Process/minidump/CMakeLists.txt
  unittests/Process/minidump/Inputs/linux-x86_64.dmp
  unittests/Process/minidump/MinidumpParserTest.cpp

Index: unittests/Process/minidump/MinidumpParserTest.cpp
===
--- /dev/null
+++ unittests/Process/minidump/MinidumpParserTest.cpp
@@ -0,0 +1,97 @@
+//===-- MinidumpTypesTest.cpp ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+// Project includes
+#include "Plugins/Process/minidump/MinidumpParser.h"
+#include "Plugins/Process/minidump/MinidumpTypes.h"
+
+// Other libraries and framework includes
+#include "gtest/gtest.h"
+
+#include "lldb/Core/ArchSpec.h"
+#include "lldb/Core/DataExtractor.h"
+#include "lldb/Host/FileSpec.h"
+
+#include "llvm/ADT/Optional.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
+
+// C includes
+
+// C++ includes
+#include 
+
+extern const char *TestMainArgv0;
+
+using namespace lldb_private;
+using namespace minidump;
+
+class MinidumpParserTest : public testing::Test
+{
+public:
+void
+SetUp() override
+{
+llvm::StringRef dmp_folder = llvm::sys::path::parent_path(TestMainArgv0);
+inputs_folder = dmp_folder;
+llvm::sys::path::append(inputs_folder, "Inputs");
+}
+
+void
+SetUpData(const char *minidump_filename, size_t load_size = SIZE_MAX)
+{
+llvm::SmallString<128> filename = inputs_folder;
+llvm::sys::path::append(filename, minidump_filename);
+FileSpec minidump_file(filename.c_str(), false);
+lldb::DataBufferSP data_sp(minidump_file.MemoryMapFileContents(0, load_size));
+llvm::Optional optional_parser = MinidumpParser::Create(data_sp);
+ASSERT_TRUE(optional_parser.hasValue());
+parser.reset(new MinidumpParser(optional_parser.getValue()));
+ASSERT_GT(parser->GetByteSize(), 0UL);
+}
+
+llvm::SmallString<128> inputs_folder;
+std::unique_ptr parser;
+};
+
+TEST_F(MinidumpParserTest, GetThreads)
+{
+SetUpData("linux-x86_64.dmp");
+llvm::Optional thread_list;
+
+thread_list = parser->GetThreads();
+ASSERT_TRUE(thread_list.hasValue());
+ASSERT_EQ(1UL, thread_list->size());
+
+const MinidumpThread *thread = thread_list.getValue()[0];
+ASSERT_EQ(16001UL, thread->thread_id);
+}
+
+TEST_F(MinidumpParserTest, GetThreadsTruncatedFile)
+{
+SetUpData("linux-x86_64.dmp", 200);
+llvm::Optional thread_list;
+
+thread_list = parser->GetThreads();
+ASSERT_FALSE(thread_list.hasValue());
+}
+
+TEST_F(MinidumpParserTest, GetArchitecture)
+{
+SetUpData("linux-x86_64.dmp");
+ASSERT_EQ(llvm::Triple::ArchType::x86_64, parser->GetArchitecture().GetTriple().getArch());
+}
+
+TEST_F(MinidumpParserTest, GetMiscInfo)
+{
+SetUpData("linux-x86_64.dmp");
+const MinidumpMiscInfo *misc_info = parser->GetMiscInfo();
+ASSERT_EQ(nullptr, misc_info);
+// linux breakpad generated minidump files don't have misc info stream
+}
Index: unittests/Process/minidump/CMakeLists.txt
===
--- /dev/null
+++ unittests/Process/minidump/CMakeLists.txt
@@ -0,0 +1,8 @@
+add_lldb_unittest(LLDBMinidumpTests
+  MinidumpParserTest.cpp
+  )
+
+set(test_inputs
+   linux-x86_64.dmp)
+
+add_unittest_inputs(LLDBMinidumpTests "${test_inputs}")
Index: unittests/Process/CMakeLists.txt
===
--- unittests/Process/CMakeLists.txt
+++ unittests/Process/CMakeLists.txt
@@ -1 +1,2 @@
 add_subdirectory(gdb-remote)
+add_subdirectory(minidump)
Index: source/Plugins/Process/minidump/MinidumpTypes.h
===
--- /dev/null
+++ source/Plugins/Process/minidump/MinidumpTypes.h
@@ -0,0 +1,298 @@
+//===-- MinidumpTypes.h -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//

Re: [Lldb-commits] [PATCH] D23545: Minidump parsing

[Zachary Turner via lldb-commits]

zturner added a comment.

Were you still going to change all the `Optionals` to raw pointers (or even 
better, `llvm::Errors`)



Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:21
@@ +20,3 @@
+MinidumpParser::MinidumpParser(const lldb::DataBufferSP _buf_sp)
+: m_data_sp(data_buf_sp), m_header(nullptr), m_valid_data(true)
+{

I'm not crazy about the idea of allowing the constructor to fail and then 
checking `IsValidData()` after you construct it.  One way LLVM solves this is 
by having a static function that returns `Expected`.  You would use it like 
this:

```
Expected MinidumpParser::create(const lldb::DataBufferSP 
_buf_sp) {
  if (data_buf_sp->GetByteSize() < sizeof(MinidumpHeader))
return make_error("Insufficient buffer!");

  MinidumpHeader *header = nullptr;
  if (auto EC = MinidumpHeader::Parse(header_data, header))
return std::move(EC);

   ...

   return MinidumpParser(header, directory_map);
}

auto Parser = MinidumpParser::create(buffer);
if (!Parser) {
  // Handle the error
}
  
```

This way it's impossible to create a MinidumpParser that is invalid.

Up to you whether you want to do this, but I think it is a very good (and safe) 
pattern that I'd love to see LLDB start adopting more often.


https://reviews.llvm.org/D23545



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

Re: [Lldb-commits] [PATCH] D23545: Minidump parsing

[Dimitar Vlahovski via lldb-commits]

dvlahovski updated this revision to Diff 68690.
dvlahovski added a comment.

Move creation of ArrayRef after sanity check

Add a test that checks that the thread list is not present in a
truncated minidump file


https://reviews.llvm.org/D23545

Files:
  cmake/LLDBDependencies.cmake
  source/Plugins/Process/CMakeLists.txt
  source/Plugins/Process/minidump/CMakeLists.txt
  source/Plugins/Process/minidump/MinidumpParser.cpp
  source/Plugins/Process/minidump/MinidumpParser.h
  source/Plugins/Process/minidump/MinidumpTypes.cpp
  source/Plugins/Process/minidump/MinidumpTypes.h
  unittests/Process/CMakeLists.txt
  unittests/Process/minidump/CMakeLists.txt
  unittests/Process/minidump/Inputs/linux-x86_64.dmp
  unittests/Process/minidump/MinidumpParserTest.cpp

Index: unittests/Process/minidump/MinidumpParserTest.cpp
===
--- /dev/null
+++ unittests/Process/minidump/MinidumpParserTest.cpp
@@ -0,0 +1,96 @@
+//===-- MinidumpTypesTest.cpp ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+// Project includes
+#include "Plugins/Process/minidump/MinidumpParser.h"
+#include "Plugins/Process/minidump/MinidumpTypes.h"
+
+// Other libraries and framework includes
+#include "gtest/gtest.h"
+
+#include "lldb/Core/ArchSpec.h"
+#include "lldb/Core/DataExtractor.h"
+#include "lldb/Host/FileSpec.h"
+
+#include "llvm/ADT/Optional.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
+
+// C includes
+
+// C++ includes
+#include 
+
+extern const char *TestMainArgv0;
+
+using namespace lldb_private;
+using namespace minidump;
+
+class MinidumpParserTest : public testing::Test
+{
+public:
+void
+SetUp() override
+{
+llvm::StringRef dmp_folder = llvm::sys::path::parent_path(TestMainArgv0);
+inputs_folder = dmp_folder;
+llvm::sys::path::append(inputs_folder, "Inputs");
+}
+
+void
+SetUpData(const char *minidump_filename, size_t load_size = SIZE_MAX)
+{
+llvm::SmallString<128> filename = inputs_folder;
+llvm::sys::path::append(filename, minidump_filename);
+FileSpec minidump_file(filename.c_str(), false);
+lldb::DataBufferSP data_sp(minidump_file.MemoryMapFileContents(0, load_size));
+parser.reset(new MinidumpParser(data_sp));
+ASSERT_GT(parser->GetByteSize(), 0UL);
+ASSERT_TRUE(static_cast(parser));
+}
+
+llvm::SmallString<128> inputs_folder;
+std::unique_ptr parser;
+};
+
+TEST_F(MinidumpParserTest, GetThreads)
+{
+SetUpData("linux-x86_64.dmp");
+llvm::Optional thread_list;
+
+thread_list = parser->GetThreads();
+ASSERT_TRUE(thread_list.hasValue());
+ASSERT_EQ(1UL, thread_list->size());
+
+const MinidumpThread *thread = thread_list.getValue()[0];
+ASSERT_EQ(16001UL, thread->thread_id);
+}
+
+TEST_F(MinidumpParserTest, GetThreadsTruncatedFile)
+{
+SetUpData("linux-x86_64.dmp", 128);
+llvm::Optional thread_list;
+
+thread_list = parser->GetThreads();
+ASSERT_FALSE(thread_list.hasValue());
+}
+
+TEST_F(MinidumpParserTest, GetArchitecture)
+{
+SetUpData("linux-x86_64.dmp");
+ASSERT_EQ(llvm::Triple::ArchType::x86_64, parser->GetArchitecture().GetTriple().getArch());
+}
+
+TEST_F(MinidumpParserTest, GetMiscInfo)
+{
+SetUpData("linux-x86_64.dmp");
+const MinidumpMiscInfo *misc_info = parser->GetMiscInfo();
+ASSERT_EQ(nullptr, misc_info);
+// linux breakpad generated minidump files don't have misc info stream
+}
Index: unittests/Process/minidump/CMakeLists.txt
===
--- /dev/null
+++ unittests/Process/minidump/CMakeLists.txt
@@ -0,0 +1,8 @@
+add_lldb_unittest(LLDBMinidumpTests
+  MinidumpParserTest.cpp
+  )
+
+set(test_inputs
+   linux-x86_64.dmp)
+
+add_unittest_inputs(LLDBMinidumpTests "${test_inputs}")
Index: unittests/Process/CMakeLists.txt
===
--- unittests/Process/CMakeLists.txt
+++ unittests/Process/CMakeLists.txt
@@ -1 +1,2 @@
 add_subdirectory(gdb-remote)
+add_subdirectory(minidump)
Index: source/Plugins/Process/minidump/MinidumpTypes.h
===
--- /dev/null
+++ source/Plugins/Process/minidump/MinidumpTypes.h
@@ -0,0 +1,298 @@
+//===-- MinidumpTypes.h -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef 

Re: [Lldb-commits] [PATCH] D23545: Minidump parsing

[Pavel Labath via lldb-commits]

labath added a comment.

Looks good as far as I am concerned. You might want to add a test or two that 
makes sure we handle invalid data reasonably. E.g., load only the first X kb of 
the minidump (so that the thread list is not present in the loaded block), and 
make sure getting a thread list fails (and does not crash).



Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:40
@@ +39,3 @@
+// check if there is enough data for the parsing of the directory list
+if ((directory_list_offset + sizeof(MinidumpDirectory) * 
m_header->streams_count) > m_data_sp->GetByteSize())
+{

Please put this before the construction of the ArrayRef - so we don't create a 
dangling object for no reason.


https://reviews.llvm.org/D23545



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

Re: [Lldb-commits] [PATCH] D23545: Minidump parsing

[Dimitar Vlahovski via lldb-commits]

dvlahovski added a comment.

I'm thinking of doing the following approach regarding the plugin 
implementation:
Start writing a new plugin "from scratch", which will be inspired strongly by 
ProcessWinMiniDump and also borrowing stuff from ProcessElfCore.
Also my plan is to make a single cross-platform Minidump plugin. At least for 
now I think that there aren't so many differences between Minidumps
generated on different platforms. (If the code gets messy probably a separate 
plugin for each platform is better.)
Also I think that I could reuse the Windows Minidump tests by pointing them to 
the new plugin and see if we
have at least the same (working) functionality.


https://reviews.llvm.org/D23545



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

Re: [Lldb-commits] [PATCH] D23545: Minidump parsing

[Dimitar Vlahovski via lldb-commits]

dvlahovski updated this revision to Diff 68670.
dvlahovski added a comment.

Adding sanity checks whether the data buffer

contains the amound of bytes that we want to read.
All of the functions that were returning llvm::Optional
now return just const something *
and indicate for failure with the return of nullptr.


https://reviews.llvm.org/D23545

Files:
  cmake/LLDBDependencies.cmake
  source/Plugins/Process/CMakeLists.txt
  source/Plugins/Process/minidump/CMakeLists.txt
  source/Plugins/Process/minidump/MinidumpParser.cpp
  source/Plugins/Process/minidump/MinidumpParser.h
  source/Plugins/Process/minidump/MinidumpTypes.cpp
  source/Plugins/Process/minidump/MinidumpTypes.h
  unittests/Process/CMakeLists.txt
  unittests/Process/minidump/CMakeLists.txt
  unittests/Process/minidump/Inputs/linux-x86_64.dmp
  unittests/Process/minidump/MinidumpParserTest.cpp

Index: unittests/Process/minidump/MinidumpParserTest.cpp
===
--- /dev/null
+++ unittests/Process/minidump/MinidumpParserTest.cpp
@@ -0,0 +1,87 @@
+//===-- MinidumpTypesTest.cpp ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+// Project includes
+#include "Plugins/Process/minidump/MinidumpParser.h"
+#include "Plugins/Process/minidump/MinidumpTypes.h"
+
+// Other libraries and framework includes
+#include "gtest/gtest.h"
+
+#include "lldb/Core/ArchSpec.h"
+#include "lldb/Core/DataExtractor.h"
+#include "lldb/Host/FileSpec.h"
+
+#include "llvm/ADT/Optional.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
+
+// C includes
+
+// C++ includes
+#include 
+
+extern const char *TestMainArgv0;
+
+using namespace lldb_private;
+using namespace minidump;
+
+class MinidumpParserTest : public testing::Test
+{
+public:
+void
+SetUp() override
+{
+llvm::StringRef dmp_folder = llvm::sys::path::parent_path(TestMainArgv0);
+inputs_folder = dmp_folder;
+llvm::sys::path::append(inputs_folder, "Inputs");
+}
+
+void
+SetUpData(const char *minidump_filename)
+{
+llvm::SmallString<128> filename = inputs_folder;
+llvm::sys::path::append(filename, minidump_filename);
+FileSpec minidump_file(filename.c_str(), false);
+lldb::DataBufferSP data_sp(minidump_file.MemoryMapFileContents());
+parser.reset(new MinidumpParser(data_sp));
+ASSERT_GT(parser->GetByteSize(), 0UL);
+ASSERT_TRUE(static_cast(parser));
+}
+
+llvm::SmallString<128> inputs_folder;
+std::unique_ptr parser;
+};
+
+TEST_F(MinidumpParserTest, GetThreads)
+{
+SetUpData("linux-x86_64.dmp");
+llvm::Optional thread_list;
+
+thread_list = parser->GetThreads();
+ASSERT_TRUE(thread_list.hasValue());
+ASSERT_EQ(1UL, thread_list->size());
+
+const MinidumpThread *thread = thread_list.getValue()[0];
+ASSERT_EQ(16001UL, thread->thread_id);
+}
+
+TEST_F(MinidumpParserTest, GetArchitecture)
+{
+SetUpData("linux-x86_64.dmp");
+ASSERT_EQ(llvm::Triple::ArchType::x86_64, parser->GetArchitecture().GetTriple().getArch());
+}
+
+TEST_F(MinidumpParserTest, GetMiscInfo)
+{
+SetUpData("linux-x86_64.dmp");
+const MinidumpMiscInfo *misc_info = parser->GetMiscInfo();
+ASSERT_EQ(nullptr, misc_info);
+// linux breakpad generated minidump files don't have misc info stream
+}
Index: unittests/Process/minidump/CMakeLists.txt
===
--- /dev/null
+++ unittests/Process/minidump/CMakeLists.txt
@@ -0,0 +1,8 @@
+add_lldb_unittest(LLDBMinidumpTests
+  MinidumpParserTest.cpp
+  )
+
+set(test_inputs
+   linux-x86_64.dmp)
+
+add_unittest_inputs(LLDBMinidumpTests "${test_inputs}")
Index: unittests/Process/CMakeLists.txt
===
--- unittests/Process/CMakeLists.txt
+++ unittests/Process/CMakeLists.txt
@@ -1 +1,2 @@
 add_subdirectory(gdb-remote)
+add_subdirectory(minidump)
Index: source/Plugins/Process/minidump/MinidumpTypes.h
===
--- /dev/null
+++ source/Plugins/Process/minidump/MinidumpTypes.h
@@ -0,0 +1,298 @@
+//===-- MinidumpTypes.h -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef liblldb_MinidumpTypes_h_
+#define liblldb_MinidumpTypes_h_
+
+// Project includes
+
+// Other libraries and framework includes
+#include "lldb/Core/Error.h"
+
+#include 

Re: [Lldb-commits] [PATCH] D23545: Minidump parsing

[Pavel Labath via lldb-commits]

labath added a comment.

> > Do you have a suggestion where the parsing code to be, if not in the same 
> > directory?

> 

> 

> The parsing code will end up being used by multiple plugins--the new one 
> you're building for Linux and the existing one that's Windows-specific.


I was hoping that we could avoid having a separate plugin for each OS. 
ProcessElfCore already handles linux and freebsd core files. It should be even 
simpler for minidumps, as the format there should be more similar. If that does 
not play out then we will need to move the generic code to a separate place, 
but I am hoping we can keep it in one plugin (maybe we can have a small class 
inside the plugin which abstracts any os-specific details we encounter).

> What I thought would happen would be that you'd write the parsing code first, 
> hook up the Windows-minidump plugin to use it (since the Windows-minidump 
> plugin tests would help validate that your parsing is correct/complete) and 
> then either add a new plugin for non-Windows.  That would argue for the 
> minidump parsing to be in a common location that could be used by both 
> plugins.  I don't have a good idea of where that would go.

> 

> Another approach is to make the Windows plugin platform agnostic once you 
> replace the limited use of the minidump APIs with your own parsing code.

> 

> Food for thought.  No need to hold up this patch for that.  The code can be 
> moved later if appropriate.


We'll see how it goes...



Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:22
@@ +21,3 @@
+{
+llvm::ArrayRef header_data(m_data_sp->GetBytes(), 
sizeof(MinidumpHeader));
+m_header = MinidumpHeader::Parse(header_data);

You should check whether the data buffer contains more than 
`sizeof(MinidumpHeader)` bytes. Or you can just use `m_data_sp->GetByteSize()` 
for size, and let the parse function deal with that.


Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:31
@@ +30,3 @@
+Error error;
+llvm::ArrayRef directory_data(m_data_sp->GetBytes() + 
directory_list_offset,
+   sizeof(MinidumpDirectory) * 
m_header.getValue()->streams_count);

Same thing. How do you know this memory exists? Is that checked somewhere?


Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:60
@@ +59,3 @@
+{
+llvm::DenseMap::iterator iter = 
m_directory_map.find((uint32_t)stream_type);
+if (iter == m_directory_map.end())

Suggestion: `auto iter = ` ?


Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:260
@@ +259,3 @@
+
+uint8_t number_of_processors;
+uint8_t product_type;

dvlahovski wrote:
> amccarth wrote:
> > I'll concede that the static_assert is useful.  Given that, showing the 
> > arithmetic explicitly will be helpful when one of these assertions trips, 
> > because you'll be able to see how it's supposed to correspond to the struct.
> Yes that was my inital insentive to write explicitly the arithmetic
What I did not like about the original approach is that it declared a new 
global variable (in a header, so it is visible to everyone), just to store the 
temporary result. I think we should avoid that. I don't really care whether you 
write `sizeof(FOO) = 47` or `sizeof(FOO) = 4*10+4+3`


https://reviews.llvm.org/D23545



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

Re: [Lldb-commits] [PATCH] D23545: Minidump parsing

[Dimitar Vlahovski via lldb-commits]

dvlahovski added inline comments.


Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:260
@@ +259,3 @@
+
+uint8_t number_of_processors;
+uint8_t product_type;

amccarth wrote:
> I'll concede that the static_assert is useful.  Given that, showing the 
> arithmetic explicitly will be helpful when one of these assertions trips, 
> because you'll be able to see how it's supposed to correspond to the struct.
Yes that was my inital insentive to write explicitly the arithmetic


https://reviews.llvm.org/D23545



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

Re: [Lldb-commits] [PATCH] D23545: Minidump parsing

[Zachary Turner via lldb-commits]

zturner added inline comments.


Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:133
@@ +132,3 @@
+// This matches the Linux kernel definitions from 
+enum MinidumpPCPUInformationARMElfHwCaps
+{

dvlahovski wrote:
> amccarth wrote:
> > zturner wrote:
> > > Should this be `enum class`?
> > These look like individual bits that will be bitwise-ORed together, which 
> > is trickier to do with an enum class.
> > 
> > But you may still want to specify the underlying type, like `uint32_t`.
> Yes, forgot to change it ...
Yea, see my earlier comment.  This enum uses `LLVM_MARK_AS_BITMASK_ENUM()` 
which makes this possible.


https://reviews.llvm.org/D23545



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

Re: [Lldb-commits] [PATCH] D23545: Minidump parsing

[Dimitar Vlahovski via lldb-commits]

dvlahovski added inline comments.


Comment at: source/Plugins/Process/minidump/MinidumpTypes.cpp:21
@@ +20,3 @@
+
+llvm::Optional
+MinidumpHeader::Parse(llvm::ArrayRef )

zturner wrote:
> I think these can all just return the pointer instead of `llvm::Optional<>`.  
> return `nullptr` to indicate failure.  Optionally, make the signature be 
> something like `Error MinidumpHeader::Parse(llvm::ArrayRef , 
> const MinidumpHeader *)` which would allow you to propagate the error 
> up (if you wanted to).
> 
> At the very least though, there's no point using `llvm::Optional<>` if 
> `nullptr` can be used to indicate failure.
Yes, fair point. Now that I'm returning pointers, `nullptr` is better for 
indicating failure.


Comment at: source/Plugins/Process/minidump/MinidumpTypes.cpp:30
@@ +29,3 @@
+const MinidumpHeaderConstants version =
+static_cast(static_cast(header->version) & 0x);
+

zturner wrote:
> Where does the `0x` come from?
There is a comment in the header, but probably it should be here. The higher 16 
bit of the version field are implementation specific.
The lower 16 bits are the version number (which is always the same constant 
number)


https://reviews.llvm.org/D23545



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

Re: [Lldb-commits] [PATCH] D23545: Minidump parsing

[Adrian McCarthy via lldb-commits]

amccarth added a comment.

In https://reviews.llvm.org/D23545#519675, @dvlahovski wrote:

> In https://reviews.llvm.org/D23545#516808, @amccarth wrote:
>
> > Are we putting this code in the right place?  I wouldn't expect minidump 
> > parsing to fall under Plugins/Process.
> >
> > I assume the eventual intent is to turn the Windows-specific code into a 
> > user of your new code?  I look forward to seeing that happen.
>
>
> My idea was that the new plugin will live in Plugins/Process/minidump.
>  Do you have a suggestion where the parsing code to be, if not in the same 
> directory?


The parsing code will end up being used by multiple plugins--the new one you're 
building for Linux and the existing one that's Windows-specific.

What I thought would happen would be that you'd write the parsing code first, 
hook up the Windows-minidump plugin to use it (since the Windows-minidump 
plugin tests would help validate that your parsing is correct/complete) and 
then either add a new plugin for non-Windows.  That would argue for the 
minidump parsing to be in a common location that could be used by both plugins. 
 I don't have a good idea of where that would go.

Another approach is to make the Windows plugin platform agnostic once you 
replace the limited use of the minidump APIs with your own parsing code.

Food for thought.  No need to hold up this patch for that.  The code can be 
moved later if appropriate.



Comment at: source/Plugins/Process/minidump/MinidumpTypes.cpp:22
@@ +21,3 @@
+llvm::Optional
+MinidumpHeader::Parse(llvm::ArrayRef )
+{

I find it amusing that all these `Parse` methods are in MinidumpTypes.cpp 
rather than MinidumpParser.cpp.  Just an observation--not a request to change 
it.


Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:133
@@ +132,3 @@
+// This matches the Linux kernel definitions from 
+enum MinidumpPCPUInformationARMElfHwCaps
+{

zturner wrote:
> Should this be `enum class`?
These look like individual bits that will be bitwise-ORed together, which is 
trickier to do with an enum class.

But you may still want to specify the underlying type, like `uint32_t`.


Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:258
@@ +257,3 @@
+llvm::support::ulittle16_t processor_level;
+llvm::support::ulittle16_t processor_revision;
+

dvlahovski wrote:
> The idea of this is to be sure that the compiler didn't align stuff 
> wrongly/not in the way we expect.
OK.


Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:260
@@ +259,3 @@
+
+uint8_t number_of_processors;
+uint8_t product_type;

I'll concede that the static_assert is useful.  Given that, showing the 
arithmetic explicitly will be helpful when one of these assertions trips, 
because you'll be able to see how it's supposed to correspond to the struct.


https://reviews.llvm.org/D23545



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

Re: [Lldb-commits] [PATCH] D23545: Minidump parsing

[Dimitar Vlahovski via lldb-commits]

dvlahovski added a comment.

Also FYI I copied the implementation of `consumeObject` to my header.


https://reviews.llvm.org/D23545



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

Re: [Lldb-commits] [PATCH] D23545: Minidump parsing

[Zachary Turner via lldb-commits]

zturner added inline comments.


Comment at: source/Plugins/Process/minidump/MinidumpParser.h:67
@@ +66,3 @@
+bool m_valid_data;
+llvm::DenseMap m_directory_map;
+};

dvlahovski wrote:
> zturner wrote:
> > Can this be `llvm::DenseMap > MinidumpLocationDescriptor>`?
> > 
> > No point erasing the type information of the enum.
> If I use MinidumpStreamType as the keys type then I think I have to specify 
> the getEmptyKey() and getTombstoneKey() functions via DenseMapInfo.
> And in the "unsigned" template specialisation of DenseMapInfo:
> 
> 
> ```
> // Provide DenseMapInfo for unsigned ints.
> template<> struct DenseMapInfo {
>   static inline unsigned getEmptyKey() { return ~0U; }
>   static inline unsigned getTombstoneKey() { return ~0U - 1; }
>   static unsigned getHashValue(const unsigned& Val) { return Val * 37U; }
>   static bool isEqual(const unsigned& LHS, const unsigned& RHS) {
> return LHS == RHS;
>   }
> };
> ```
> 
> So I should probably add there "special" values in the enum as well in order 
> for it to work?
That's unfortunate, but it looks like you're right.  It's probably not worth 
going to that much effort.  It could probably be done by partially specializing 
`DenseMapInfo` for all enums, but I don't think it's worth it.  So I suppose 
it's fine to leave as a `uint32_t`.


https://reviews.llvm.org/D23545



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

Re: [Lldb-commits] [PATCH] D23545: Minidump parsing

[Dimitar Vlahovski via lldb-commits]

dvlahovski added inline comments.


Comment at: source/Plugins/Process/minidump/MinidumpParser.h:67
@@ +66,3 @@
+bool m_valid_data;
+llvm::DenseMap m_directory_map;
+};

zturner wrote:
> Can this be `llvm::DenseMap`?
> 
> No point erasing the type information of the enum.
If I use MinidumpStreamType as the keys type then I think I have to specify the 
getEmptyKey() and getTombstoneKey() functions via DenseMapInfo.
And in the "unsigned" template specialisation of DenseMapInfo:


```
// Provide DenseMapInfo for unsigned ints.
template<> struct DenseMapInfo {
  static inline unsigned getEmptyKey() { return ~0U; }
  static inline unsigned getTombstoneKey() { return ~0U - 1; }
  static unsigned getHashValue(const unsigned& Val) { return Val * 37U; }
  static bool isEqual(const unsigned& LHS, const unsigned& RHS) {
return LHS == RHS;
  }
};
```

So I should probably add there "special" values in the enum as well in order 
for it to work?


Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:133
@@ +132,3 @@
+// This matches the Linux kernel definitions from 
+enum MinidumpPCPUInformationARMElfHwCaps
+{

zturner wrote:
> Should this be `enum class`?
Yes, forgot to change it ...


https://reviews.llvm.org/D23545



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

Re: [Lldb-commits] [PATCH] D23545: Minidump parsing

[Zachary Turner via lldb-commits]

zturner added inline comments.


Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:259
@@ +258,3 @@
+static_assert(sizeof(MinidumpSystemInfo) == MINIDUMP_SYSTEM_INFO_SIZE, "sizeof 
MinidumpSystemInfo is not correct!");
+
+struct MinidumpMiscInfo

I think the `static_assert` is a good idea.  Since this corresponds to an on 
disk format where the size of the structure is fixed and known, the static 
assert is a good idea.  But the calculation is unnecessary I think.  It's more 
readable if someone can just look at it and say "ok it has to be 32 bytes" or 
whatever, rather than doing this math in their head.


https://reviews.llvm.org/D23545



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

Re: [Lldb-commits] [PATCH] D23545: Minidump parsing

[Zachary Turner via lldb-commits]

zturner added inline comments.


Comment at: source/Plugins/Process/minidump/MinidumpParser.h:67
@@ +66,3 @@
+bool m_valid_data;
+llvm::DenseMap m_directory_map;
+};

Can this be `llvm::DenseMap`?

No point erasing the type information of the enum.


Comment at: source/Plugins/Process/minidump/MinidumpTypes.cpp:21
@@ +20,3 @@
+
+llvm::Optional
+MinidumpHeader::Parse(llvm::ArrayRef )

I think these can all just return the pointer instead of `llvm::Optional<>`.  
return `nullptr` to indicate failure.  Optionally, make the signature be 
something like `Error MinidumpHeader::Parse(llvm::ArrayRef , 
const MinidumpHeader *)` which would allow you to propagate the error up 
(if you wanted to).

At the very least though, there's no point using `llvm::Optional<>` if 
`nullptr` can be used to indicate failure.


Comment at: source/Plugins/Process/minidump/MinidumpTypes.cpp:30
@@ +29,3 @@
+const MinidumpHeaderConstants version =
+static_cast(static_cast(header->version) & 0x);
+

Where does the `0x` come from?


Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:133
@@ +132,3 @@
+// This matches the Linux kernel definitions from 
+enum MinidumpPCPUInformationARMElfHwCaps
+{

Should this be `enum class`?


https://reviews.llvm.org/D23545



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

Re: [Lldb-commits] [PATCH] D23545: Minidump parsing

[Dimitar Vlahovski via lldb-commits]

dvlahovski added a comment.

In https://reviews.llvm.org/D23545#516808, @amccarth wrote:

> Are we putting this code in the right place?  I wouldn't expect minidump 
> parsing to fall under Plugins/Process.
>
> I assume the eventual intent is to turn the Windows-specific code into a user 
> of your new code?  I look forward to seeing that happen.


My idea was that the new plugin will live in Plugins/Process/minidump.
Do you have a suggestion where the parsing code to be, if not in the same 
directory?


https://reviews.llvm.org/D23545



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

Re: [Lldb-commits] [PATCH] D23545: Minidump parsing

[Dimitar Vlahovski via lldb-commits]

dvlahovski added inline comments.


Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:258
@@ +257,3 @@
+llvm::support::ulittle16_t processor_level;
+llvm::support::ulittle16_t processor_revision;
+

The idea of this is to be sure that the compiler didn't align stuff wrongly/not 
in the way we expect.


https://reviews.llvm.org/D23545



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

Re: [Lldb-commits] [PATCH] D23545: Minidump parsing

[Dimitar Vlahovski via lldb-commits]

dvlahovski updated this revision to Diff 68557.
dvlahovski added a comment.

Update of the code, regarding the remarks

of zturner and amccarth.


https://reviews.llvm.org/D23545

Files:
  cmake/LLDBDependencies.cmake
  source/Plugins/Process/CMakeLists.txt
  source/Plugins/Process/minidump/CMakeLists.txt
  source/Plugins/Process/minidump/MinidumpParser.cpp
  source/Plugins/Process/minidump/MinidumpParser.h
  source/Plugins/Process/minidump/MinidumpTypes.cpp
  source/Plugins/Process/minidump/MinidumpTypes.h
  unittests/Process/CMakeLists.txt
  unittests/Process/minidump/CMakeLists.txt
  unittests/Process/minidump/Inputs/linux-x86_64.dmp
  unittests/Process/minidump/MinidumpParserTest.cpp

Index: unittests/Process/minidump/MinidumpParserTest.cpp
===
--- /dev/null
+++ unittests/Process/minidump/MinidumpParserTest.cpp
@@ -0,0 +1,87 @@
+//===-- MinidumpTypesTest.cpp ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+// Project includes
+#include "Plugins/Process/minidump/MinidumpParser.h"
+#include "Plugins/Process/minidump/MinidumpTypes.h"
+
+// Other libraries and framework includes
+#include "gtest/gtest.h"
+
+#include "lldb/Core/ArchSpec.h"
+#include "lldb/Core/DataExtractor.h"
+#include "lldb/Host/FileSpec.h"
+
+#include "llvm/ADT/Optional.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
+
+// C includes
+
+// C++ includes
+#include 
+
+extern const char *TestMainArgv0;
+
+using namespace lldb_private;
+using namespace minidump;
+
+class MinidumpParserTest : public testing::Test
+{
+public:
+void
+SetUp() override
+{
+llvm::StringRef dmp_folder = llvm::sys::path::parent_path(TestMainArgv0);
+inputs_folder = dmp_folder;
+llvm::sys::path::append(inputs_folder, "Inputs");
+}
+
+void
+SetUpData(const char *minidump_filename)
+{
+llvm::SmallString<128> filename = inputs_folder;
+llvm::sys::path::append(filename, minidump_filename);
+FileSpec minidump_file(filename.c_str(), false);
+lldb::DataBufferSP data_sp(minidump_file.MemoryMapFileContents());
+parser.reset(new MinidumpParser(data_sp));
+ASSERT_GT(parser->GetByteSize(), 0UL);
+ASSERT_TRUE(static_cast(parser));
+}
+
+llvm::SmallString<128> inputs_folder;
+std::unique_ptr parser;
+};
+
+TEST_F(MinidumpParserTest, GetThreads)
+{
+SetUpData("linux-x86_64.dmp");
+llvm::Optional thread_list;
+
+thread_list = parser->GetThreads();
+ASSERT_TRUE(thread_list.hasValue());
+ASSERT_EQ(1UL, thread_list->size());
+
+const MinidumpThread *thread = thread_list.getValue()[0];
+ASSERT_EQ(16001UL, thread->thread_id);
+}
+
+TEST_F(MinidumpParserTest, GetArchitecture)
+{
+SetUpData("linux-x86_64.dmp");
+ASSERT_EQ(llvm::Triple::ArchType::x86_64, parser->GetArchitecture().GetTriple().getArch());
+}
+
+TEST_F(MinidumpParserTest, GetMiscInfo)
+{
+SetUpData("linux-x86_64.dmp");
+llvm::Optional misc_info = parser->GetMiscInfo();
+ASSERT_FALSE(misc_info.hasValue());
+// linux breakpad generated minidump files don't have misc info stream
+}
Index: unittests/Process/minidump/CMakeLists.txt
===
--- /dev/null
+++ unittests/Process/minidump/CMakeLists.txt
@@ -0,0 +1,8 @@
+add_lldb_unittest(LLDBMinidumpTests
+  MinidumpParserTest.cpp
+  )
+
+set(test_inputs
+   linux-x86_64.dmp)
+
+add_unittest_inputs(LLDBMinidumpTests "${test_inputs}")
Index: unittests/Process/CMakeLists.txt
===
--- unittests/Process/CMakeLists.txt
+++ unittests/Process/CMakeLists.txt
@@ -1 +1,2 @@
 add_subdirectory(gdb-remote)
+add_subdirectory(minidump)
Index: source/Plugins/Process/minidump/MinidumpTypes.h
===
--- /dev/null
+++ source/Plugins/Process/minidump/MinidumpTypes.h
@@ -0,0 +1,298 @@
+//===-- MinidumpTypes.h -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef liblldb_MinidumpTypes_h_
+#define liblldb_MinidumpTypes_h_
+
+// Project includes
+
+// Other libraries and framework includes
+#include "lldb/Core/Error.h"
+
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/BitmaskEnum.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/Support/Endian.h"
+
+// C includes
+// C++ includes
+
+// Reference:
+// 

Re: [Lldb-commits] [PATCH] D23545: Minidump parsing

[Zachary Turner via lldb-commits]

zturner added inline comments.


Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:29
@@ +28,3 @@
+
+lldb::ByteOrder byteorder = m_data.GetByteOrder();
+lldb::offset_t directory_list_offset = m_header->stream_directory_rva;

I asked some people familiar with breakpad and windows history, and the answer 
I got was that while minidump may have support big endian prior to windows 2000 
when it supported PPC and MIPS, but probably not anymore.

At this point we have full control over the minidump pipeline.  If we want to 
generate them from within LLDB, we can write them using little endian.  And if 
we want to read them, we can assume they are not from a file prior to windows 
2000.  I think it's safe to assume little endian unless we have strong evidence 
that we need to parse a big endian minidump.


Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:35
@@ +34,3 @@
+{
+m_data.ExtractBytes(directory_list_offset, sizeof(MinidumpDirectory), 
byteorder, );
+directory_list_offset += sizeof(MinidumpDirectory);

Note that this will make a copy of the data.  Based on my previous email, this 
could be written as follows:

const MiniDumpDirectory *directory = nullptr;

```
for (uint32_t i=0; i < m_header->streams_count; ++i) {
  if (auto EC = consumeObject(data, directory))
return EC;
  m_directory_map[directory.stream_type = directory->location;
}
```

No copying is involved here.  You would need to make a few changes to do this 
but I think it is worth it.  Same concept applies throughout the rest of this 
file, so I won't mention it again.


Comment at: source/Plugins/Process/minidump/MinidumpParser.h:13-25
@@ +12,15 @@
+
+// C includes
+
+// C++ includes
+#include 
+#include 
+
+// Other libraries and framework includes
+#include "lldb/Core/ArchSpec.h"
+#include "lldb/Core/DataExtractor.h"
+#include "llvm/ADT/Optional.h"
+
+// Project includes
+#include "MinidumpTypes.h"
+

Please include files in the following order:

1. Files in the same directory

2. LLDB includes

3. LLVM includes

4. C and C++ system includes

Although this isn't consistent with the rest of the codebase, it was recently 
agreed that this is what we want to move towards, so all new code should follow 
this pattern.


Comment at: source/Plugins/Process/minidump/MinidumpParser.h:61
@@ +60,3 @@
+llvm::Optional m_header;
+std::unordered_map m_directory_map;
+};

Please use `llvm::DenseMap`.  `std::unordered_map` should probably not be used 
for anything ever.


Comment at: source/Plugins/Process/minidump/MinidumpParser.h:67
@@ +66,1 @@
+#endif // liblldb_MinidumpParser_h_
\ No newline at end of file


Newline here.


Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:40
@@ +39,3 @@
+
+enum MinidumpStreamType
+{

amccarth wrote:
> // Reference:  
> https://msdn.microsoft.com/en-us/library/windows/desktop/ms680394.aspx
In LLVM where we parse PDB and CodeView debug info types, we have adopted the 
convention that these are all `enum classes`, and we continue to use LLVM 
naming style instead of using Microsoft all caps style.  So I would write this 
as:

```
enum class MiniDumpStreamType : uint32_t {
  Unused = 0,
  Reserved0 = 1,
  Reserved1 = 2,
  ThreadList = 3,
  ...
};
```

The `: uint32_t` is important since enums are signed by default on Microsoft 
compilers.


Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:85
@@ +84,3 @@
+{
+MINIDUMP_CPU_ARCHITECTURE_X86 = 0, /* PROCESSOR_ARCHITECTURE_INTEL 
*/
+MINIDUMP_CPU_ARCHITECTURE_MIPS = 1,/* PROCESSOR_ARCHITECTURE_MIPS 
*/

Same goes for these, as well as a few more enums later on.  I would make these 
all enum classes and name them using the LLVM naming conventions.  


Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:147
@@ +146,3 @@
+MINIDUMP_CPU_ARM_ELF_HWCAP_IDIVT = (1 << 18),
+};
+

Enum classes don't play nicely with flags, but LLVM has a file called 
`llvm/ADT/BitmaskEnum.h` which improves this significantly.  So these can also 
be an enum class, just use the proper macro from that file so that bitwise 
operations become possible.


Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:151
@@ +150,3 @@
+{
+uint32_t signature;
+uint32_t version;

As mentioned in my previous response, all these could be come LLVM endian aware 
types from `llvm/Support/Endian.h' which would allow you to `reinterpret_cast` 
straight from the underlying buffer.


Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:165-166
@@ +164,4 @@
+};
+const int MINIDUMP_HEADER_SIZE = 3 * 4 + sizeof(RVA) + 2 * 4 + 8;

Re: [Lldb-commits] [PATCH] D23545: Minidump parsing

[Zachary Turner via lldb-commits]

Btw the endian aware types are usable, just include
"llvm/Support/Endian.h". It's just the consumeObject functions that are not
On Tue, Aug 16, 2016 at 10:01 AM Zachary Turner  wrote:

> zturner added a comment.
>
> LLVM does have something similar to DataExtractor, but unfortunately it's
> not present in the correct library for you to easily be able to reuse it.
> I actually own the code in question, so I'm willing to fix that for you if
> you're interested, but at the same time the code is very simple.  LLVM has
> some endian-aware data types that are very convenient to work with and
> mostly eliminate the need to worry about endianness while parsing, which is
> what DataExtractor mostly does for you.  To use LLVM's types, for example,
> you would change this:
>
>   struct MinidumpHeader
>   {
>   uint32_t signature;
>   uint32_t version;
>   uint32_t streams_count;
>   RVA stream_directory_rva; // offset of the stream directory
>   uint32_t checksum;
>   uint32_t time_date_stamp; // time_t format
>   uint64_t flags;
>
>   static bool
>   SignatureMatchAndSetByteOrder(DataExtractor , lldb::offset_t
> *offset);
>
>   static llvm::Optional
>   Parse(const DataExtractor , lldb::offset_t *offset);
>   };
>
> to this:
>
>   struct MinidumpHeader
>   {
>   support::ulittle32_t signature;
>   support::ulittle32_t version;
>   support::ulittle32_t streams_count;
>   support::ulittle32_t stream_directory_rva; // offset of the stream
> directory
>   support::ulittle32_t checksum;
>   support::ulittle32_t time_date_stamp; // time_t format
>   support::ulittle64_t flags;
>   };
>
> All you have to do now is `reinterpret_cast` the buffer to a
> `MiniDumpHeader*` and you're good to go.  So pretty much the entirety of
> the `DataExtractor` class boils down to a single template function:
>
>   template
>   Error consumeObject(ArrayRef , const T *) {
> if (Buffer.size() < sizeof(T))
>   return make_error("Insufficient buffer!");
> Object = reinterpret_cast(Buffer.data());
> Buffer = Buffer.drop_front(sizeof(T));
> return Error::success();
>   }
>
> For starters, this is nice because it means you're not copying memory
> around unnecessarily.  You're just pointing to the memory that's already
> there.  With DataExtractor you are always copying bytes around.  Dump files
> can be large (even minidumps!) and copying all this memory around is
> inefficient.
>
> It also makes the syntax cleaner.  You have to call different functions on
> DataExtractor depending on what you want to extract.  `GetU8` or `GetU16`
> for example.  This one function works with almost everything.  A few simple
> template specializations and overloads can make it even more powerful.  For
> example:
>
>   Error consumeObject(ArrayRef , StringRef ) {
>  ZeroString = StringRef(reinterpret_cast *>(Buffer.front()));
>  Buffer = Buffer.drop_front(ZeroString.size() + 1);
>  return Error::success();
>   }
>
> I have some more comments on the CL, but I have to run to a meeting, so I
> will be back later.
>
>
> https://reviews.llvm.org/D23545
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D23545: Minidump parsing

[Zachary Turner via lldb-commits]

zturner added a comment.

LLVM does have something similar to DataExtractor, but unfortunately it's not 
present in the correct library for you to easily be able to reuse it.  I 
actually own the code in question, so I'm willing to fix that for you if you're 
interested, but at the same time the code is very simple.  LLVM has some 
endian-aware data types that are very convenient to work with and mostly 
eliminate the need to worry about endianness while parsing, which is what 
DataExtractor mostly does for you.  To use LLVM's types, for example, you would 
change this:

  struct MinidumpHeader
  {
  uint32_t signature;
  uint32_t version;
  uint32_t streams_count;
  RVA stream_directory_rva; // offset of the stream directory
  uint32_t checksum;
  uint32_t time_date_stamp; // time_t format
  uint64_t flags;
  
  static bool
  SignatureMatchAndSetByteOrder(DataExtractor , lldb::offset_t 
*offset);
  
  static llvm::Optional
  Parse(const DataExtractor , lldb::offset_t *offset);
  };

to this:

  struct MinidumpHeader
  {
  support::ulittle32_t signature;
  support::ulittle32_t version;
  support::ulittle32_t streams_count;
  support::ulittle32_t stream_directory_rva; // offset of the stream 
directory
  support::ulittle32_t checksum;
  support::ulittle32_t time_date_stamp; // time_t format
  support::ulittle64_t flags;
  };

All you have to do now is `reinterpret_cast` the buffer to a `MiniDumpHeader*` 
and you're good to go.  So pretty much the entirety of the `DataExtractor` 
class boils down to a single template function:

  template
  Error consumeObject(ArrayRef , const T *) {
if (Buffer.size() < sizeof(T))
  return make_error("Insufficient buffer!");
Object = reinterpret_cast(Buffer.data());
Buffer = Buffer.drop_front(sizeof(T));
return Error::success();
  }

For starters, this is nice because it means you're not copying memory around 
unnecessarily.  You're just pointing to the memory that's already there.  With 
DataExtractor you are always copying bytes around.  Dump files can be large 
(even minidumps!) and copying all this memory around is inefficient.

It also makes the syntax cleaner.  You have to call different functions on 
DataExtractor depending on what you want to extract.  `GetU8` or `GetU16` for 
example.  This one function works with almost everything.  A few simple 
template specializations and overloads can make it even more powerful.  For 
example:

  Error consumeObject(ArrayRef , StringRef ) {
 ZeroString = StringRef(reinterpret_cast(Buffer.front()));
 Buffer = Buffer.drop_front(ZeroString.size() + 1);
 return Error::success();
  }

I have some more comments on the CL, but I have to run to a meeting, so I will 
be back later.


https://reviews.llvm.org/D23545



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

Re: [Lldb-commits] [PATCH] D23545: Minidump parsing

[Adrian McCarthy via lldb-commits]

amccarth added a comment.

Are we putting this code in the right place?  I wouldn't expect minidump 
parsing to fall under Plugins/Process.

I assume the eventual intent is to turn the Windows-specific code into a user 
of your new code?  I look forward to seeing that happen.



Comment at: source/Plugins/Process/minidump/MinidumpTypes.cpp:13
@@ +12,3 @@
+#include "MinidumpParser.h"
+#include "MinidumpTypes.h"
+

Include MinidumpTypes.h first in MinidumpTypes.cpp.  This helps ensure that 
headers can stand alone.


Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:14
@@ +13,3 @@
+// C includes
+#include 
+

Maybe I'm missing it, but it doesn't seem like this header is using anything 
from `` (which should be under C++ includes).


Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:40
@@ +39,3 @@
+
+enum MinidumpStreamType
+{

// Reference:  
https://msdn.microsoft.com/en-us/library/windows/desktop/ms680394.aspx


Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:257
@@ +256,3 @@
+};
+const int MINIDUMP_SYSTEM_INFO_SIZE = 3 * 2 + 2 * 1 + 4 * 4 + sizeof(RVA) + 2 
* 2 + MINIDUMP_CPU_INFO_SIZE;
+static_assert(sizeof(MinidumpSystemInfo) == MINIDUMP_SYSTEM_INFO_SIZE, "sizeof 
MinidumpSystemInfo is not correct!");

Why do the arithmetic and static_assert?  Why not use 
`sizeof(MinidumpSystemInfo)`?


https://reviews.llvm.org/D23545



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

Re: [Lldb-commits] [PATCH] D23545: Minidump parsing

[Zachary Turner via lldb-commits]

I see.  Let me follow up with the breakpad developers to ask them about
that.  If you only have to support one endianness the code can be much
cleaner.

On Tue, Aug 16, 2016 at 9:44 AM Dimitar Vlahovski 
wrote:

> dvlahovski added inline comments.
>
> 
> Comment at: source/Plugins/Process/minidump/MinidumpTypes.cpp:31-35
> @@ +30,7 @@
> +// the signature might be byte swapped
> +data.SetByteOrder(lldb::eByteOrderBig);
> +*offset -= 4;
> +signature = data.GetU32(offset);
> +if (signature == MINIDUMP_SIGNATURE)
> +return true;
> +
> 
> zturner wrote:
> > Why would this be true?  Is there a minidump specification somewhere
> that says that both big endian and little endian are valid byte orderings?
> I would expect that being a Microsoft created format, it should always be
> little endian, and if there is a file in big endian we can consider it
> corrupt.
> I didn't find anything in the MS documentation about the minidump's
> endianess.
> I am doing this because breakpad does this too:
> https://chromium.googlesource.com/breakpad/breakpad/+/master/src/processor/minidump.cc#4384
>
>
>
> https://reviews.llvm.org/D23545
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D23545: Minidump parsing

[Dimitar Vlahovski via lldb-commits]

dvlahovski added inline comments.


Comment at: source/Plugins/Process/minidump/MinidumpTypes.cpp:31-35
@@ +30,7 @@
+// the signature might be byte swapped
+data.SetByteOrder(lldb::eByteOrderBig);
+*offset -= 4;
+signature = data.GetU32(offset);
+if (signature == MINIDUMP_SIGNATURE)
+return true;
+

zturner wrote:
> Why would this be true?  Is there a minidump specification somewhere that 
> says that both big endian and little endian are valid byte orderings?  I 
> would expect that being a Microsoft created format, it should always be 
> little endian, and if there is a file in big endian we can consider it 
> corrupt.
I didn't find anything in the MS documentation about the minidump's endianess.
I am doing this because breakpad does this too: 
https://chromium.googlesource.com/breakpad/breakpad/+/master/src/processor/minidump.cc#4384



https://reviews.llvm.org/D23545



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

Re: [Lldb-commits] [PATCH] D23545: Minidump parsing

[Zachary Turner via lldb-commits]

zturner added a subscriber: zturner.


Comment at: source/Plugins/Process/minidump/MinidumpTypes.cpp:31-35
@@ +30,7 @@
+// the signature might be byte swapped
+data.SetByteOrder(lldb::eByteOrderBig);
+*offset -= 4;
+signature = data.GetU32(offset);
+if (signature == MINIDUMP_SIGNATURE)
+return true;
+

Why would this be true?  Is there a minidump specification somewhere that says 
that both big endian and little endian are valid byte orderings?  I would 
expect that being a Microsoft created format, it should always be little 
endian, and if there is a file in big endian we can consider it corrupt.


https://reviews.llvm.org/D23545



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

Re: [Lldb-commits] [PATCH] D23545: Minidump parsing

[Dimitar Vlahovski via lldb-commits]

Thanks for the comment. Do you have anything particular in mind from the
llvm types? Is there something similar to DataExtractor?

On Tue, Aug 16, 2016 at 4:21 PM, Zachary Turner  wrote:

> I would prefer to use llvm types to do the parsing wherever possible. Why
> do we need DataExtractor? If the only reason is to force little endian,
> just use the types in llvm/Endian.h
> On Tue, Aug 16, 2016 at 8:13 AM Dimitar Vlahovski via lldb-commits <
> lldb-commits@lists.llvm.org> wrote:
>
>> dvlahovski updated this revision to Diff 68180.
>> dvlahovski added a comment.
>>
>> Fixing a little bug - should get the byteorder
>>
>> after calling SignatureMatchAndSetByteOrder
>>
>>
>> https://reviews.llvm.org/D23545
>>
>> Files:
>>   cmake/LLDBDependencies.cmake
>>   source/Plugins/Process/CMakeLists.txt
>>   source/Plugins/Process/minidump/CMakeLists.txt
>>   source/Plugins/Process/minidump/MinidumpParser.cpp
>>   source/Plugins/Process/minidump/MinidumpParser.h
>>   source/Plugins/Process/minidump/MinidumpTypes.cpp
>>   source/Plugins/Process/minidump/MinidumpTypes.h
>>   unittests/Process/CMakeLists.txt
>>   unittests/Process/minidump/CMakeLists.txt
>>   unittests/Process/minidump/Inputs/linux-x86_64.dmp
>>   unittests/Process/minidump/MinidumpParserTest.cpp
>>
>> ___
>> lldb-commits mailing list
>> lldb-commits@lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D23545: Minidump parsing

[Zachary Turner via lldb-commits]

I would prefer to use llvm types to do the parsing wherever possible. Why
do we need DataExtractor? If the only reason is to force little endian,
just use the types in llvm/Endian.h
On Tue, Aug 16, 2016 at 8:13 AM Dimitar Vlahovski via lldb-commits <
lldb-commits@lists.llvm.org> wrote:

> dvlahovski updated this revision to Diff 68180.
> dvlahovski added a comment.
>
> Fixing a little bug - should get the byteorder
>
> after calling SignatureMatchAndSetByteOrder
>
>
> https://reviews.llvm.org/D23545
>
> Files:
>   cmake/LLDBDependencies.cmake
>   source/Plugins/Process/CMakeLists.txt
>   source/Plugins/Process/minidump/CMakeLists.txt
>   source/Plugins/Process/minidump/MinidumpParser.cpp
>   source/Plugins/Process/minidump/MinidumpParser.h
>   source/Plugins/Process/minidump/MinidumpTypes.cpp
>   source/Plugins/Process/minidump/MinidumpTypes.h
>   unittests/Process/CMakeLists.txt
>   unittests/Process/minidump/CMakeLists.txt
>   unittests/Process/minidump/Inputs/linux-x86_64.dmp
>   unittests/Process/minidump/MinidumpParserTest.cpp
>
> ___
> lldb-commits mailing list
> lldb-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D23545: Minidump parsing

[Dimitar Vlahovski via lldb-commits]

dvlahovski updated this revision to Diff 68180.
dvlahovski added a comment.

Fixing a little bug - should get the byteorder

after calling SignatureMatchAndSetByteOrder


https://reviews.llvm.org/D23545

Files:
  cmake/LLDBDependencies.cmake
  source/Plugins/Process/CMakeLists.txt
  source/Plugins/Process/minidump/CMakeLists.txt
  source/Plugins/Process/minidump/MinidumpParser.cpp
  source/Plugins/Process/minidump/MinidumpParser.h
  source/Plugins/Process/minidump/MinidumpTypes.cpp
  source/Plugins/Process/minidump/MinidumpTypes.h
  unittests/Process/CMakeLists.txt
  unittests/Process/minidump/CMakeLists.txt
  unittests/Process/minidump/Inputs/linux-x86_64.dmp
  unittests/Process/minidump/MinidumpParserTest.cpp

Index: unittests/Process/minidump/MinidumpParserTest.cpp
===
--- /dev/null
+++ unittests/Process/minidump/MinidumpParserTest.cpp
@@ -0,0 +1,81 @@
+//===-- MinidumpTypesTest.cpp ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include 
+
+#include "gtest/gtest.h"
+
+#include "lldb/Core/ArchSpec.h"
+#include "lldb/Core/DataExtractor.h"
+#include "lldb/Host/FileSpec.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
+
+#include "Plugins/Process/minidump/MinidumpParser.h"
+#include "Plugins/Process/minidump/MinidumpTypes.h"
+
+extern const char *TestMainArgv0;
+
+using namespace lldb_private;
+using namespace minidump;
+
+class MinidumpParserTest : public testing::Test
+{
+public:
+void
+SetUp() override
+{
+llvm::StringRef dmp_folder = llvm::sys::path::parent_path(TestMainArgv0);
+inputs_folder = dmp_folder;
+llvm::sys::path::append(inputs_folder, "Inputs");
+}
+
+void
+SetUpData(const char *minidump_filename)
+{
+llvm::SmallString<128> filename = inputs_folder;
+llvm::sys::path::append(filename, minidump_filename);
+FileSpec minidump_file(filename.c_str(), false);
+lldb::DataBufferSP data_sp(minidump_file.MemoryMapFileContents());
+parser.reset(new MinidumpParser(data_sp));
+ASSERT_GT(parser->GetByteSize(), 0UL);
+ASSERT_TRUE(static_cast(parser));
+}
+
+llvm::SmallString<128> inputs_folder;
+std::unique_ptr parser;
+};
+
+TEST_F(MinidumpParserTest, GetThreads)
+{
+SetUpData("linux-x86_64.dmp");
+llvm::Optional thread_list;
+
+thread_list = parser->GetThreads();
+ASSERT_TRUE(thread_list.hasValue());
+ASSERT_EQ(1UL, thread_list->size());
+
+MinidumpThread thread = thread_list.getValue()[0];
+ASSERT_EQ(16001UL, thread.thread_id);
+}
+
+TEST_F(MinidumpParserTest, GetArchitecture)
+{
+SetUpData("linux-x86_64.dmp");
+ASSERT_EQ(llvm::Triple::ArchType::x86_64, parser->GetArchitecture().GetTriple().getArch());
+}
+
+TEST_F(MinidumpParserTest, GetMiscInfo)
+{
+SetUpData("linux-x86_64.dmp");
+llvm::Optional misc_info = parser->GetMiscInfo();
+ASSERT_FALSE(misc_info.hasValue());
+// linux breakpad generated minidump files don't have misc info stream
+}
Index: unittests/Process/minidump/CMakeLists.txt
===
--- /dev/null
+++ unittests/Process/minidump/CMakeLists.txt
@@ -0,0 +1,8 @@
+add_lldb_unittest(LLDBMinidumpTests
+  MinidumpParserTest.cpp
+  )
+
+set(test_inputs
+   linux-x86_64.dmp)
+
+add_unittest_inputs(LLDBMinidumpTests "${test_inputs}")
Index: unittests/Process/CMakeLists.txt
===
--- unittests/Process/CMakeLists.txt
+++ unittests/Process/CMakeLists.txt
@@ -1 +1,2 @@
 add_subdirectory(gdb-remote)
+add_subdirectory(minidump)
Index: source/Plugins/Process/minidump/MinidumpTypes.h
===
--- /dev/null
+++ source/Plugins/Process/minidump/MinidumpTypes.h
@@ -0,0 +1,277 @@
+//===-- MinidumpTypes.h -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef liblldb_MinidumpTypes_h_
+#define liblldb_MinidumpTypes_h_
+
+// C includes
+#include 
+
+// C++ includes
+
+// Other libraries and framework includes
+#include "lldb/Core/DataExtractor.h"
+#include "llvm/ADT/Optional.h"
+
+// Reference:
+// https://msdn.microsoft.com/en-us/library/windows/desktop/ms679293(v=vs.85).aspx
+// https://chromium.googlesource.com/breakpad/breakpad/
+
+namespace lldb_private
+{
+
+namespace minidump
+{
+
+// RVA 

[Lldb-commits] [PATCH] D23545: Minidump parsing

[Dimitar Vlahovski via lldb-commits]

dvlahovski created this revision.
dvlahovski added reviewers: labath, amccarth.
dvlahovski added a subscriber: lldb-commits.
Herald added subscribers: dschuff, srhines, danalbert, tberghammer.

This is a work-in-progress minidump parsing code.
There are still some more structures/data streams that need to be added.
The aim ot this is to be used in the implementation of
a minidump debugging plugin that works on all platforms/architectures.
Currently we have a windows-only plugin that uses the WinAPI to parse
the dump files.
Also added unittests for the current functionality.

https://reviews.llvm.org/D23545

Files:
  cmake/LLDBDependencies.cmake
  source/Plugins/Process/CMakeLists.txt
  source/Plugins/Process/minidump/CMakeLists.txt
  source/Plugins/Process/minidump/MinidumpParser.cpp
  source/Plugins/Process/minidump/MinidumpParser.h
  source/Plugins/Process/minidump/MinidumpTypes.cpp
  source/Plugins/Process/minidump/MinidumpTypes.h
  unittests/Process/CMakeLists.txt
  unittests/Process/minidump/CMakeLists.txt
  unittests/Process/minidump/Inputs/linux-x86_64.dmp
  unittests/Process/minidump/MinidumpParserTest.cpp

Index: unittests/Process/minidump/MinidumpParserTest.cpp
===
--- /dev/null
+++ unittests/Process/minidump/MinidumpParserTest.cpp
@@ -0,0 +1,86 @@
+//===-- MinidumpTypesTest.cpp ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#if defined(_MSC_VER) && (_HAS_EXCEPTIONS == 0)
+// Workaround for MSVC standard library bug, which fails to include  when
+// exceptions are disabled.
+#include 
+#endif
+
+#include "gtest/gtest.h"
+
+#include "lldb/Core/ArchSpec.h"
+#include "lldb/Core/DataExtractor.h"
+#include "lldb/Host/FileSpec.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
+
+#include "Plugins/Process/minidump/MinidumpParser.h"
+#include "Plugins/Process/minidump/MinidumpTypes.h"
+
+extern const char *TestMainArgv0;
+
+using namespace lldb_private;
+using namespace minidump;
+
+class MinidumpParserTest : public testing::Test
+{
+public:
+void
+SetUp() override
+{
+llvm::StringRef dmp_folder = llvm::sys::path::parent_path(TestMainArgv0);
+inputs_folder = dmp_folder;
+llvm::sys::path::append(inputs_folder, "Inputs");
+}
+
+void
+SetUpData(const char *minidump_filename)
+{
+llvm::SmallString<128> filename = inputs_folder;
+llvm::sys::path::append(filename, minidump_filename);
+FileSpec minidump_file(filename.c_str(), false);
+lldb::DataBufferSP data_sp(minidump_file.MemoryMapFileContents());
+DataExtractor data(data_sp, lldb::eByteOrderLittle, 4);
+ASSERT_GT(data.GetByteSize(), 0UL);
+parser.SetData(data);
+ASSERT_TRUE(parser.ParseHeader());
+}
+
+llvm::SmallString<128> inputs_folder;
+MinidumpParser parser;
+};
+
+TEST_F(MinidumpParserTest, GetThreads)
+{
+SetUpData("linux-x86_64.dmp");
+llvm::Optional thread_list;
+
+thread_list = parser.GetThreads();
+ASSERT_TRUE(thread_list.hasValue());
+ASSERT_EQ(1UL, thread_list->size());
+
+MinidumpThread thread = thread_list.getValue()[0];
+ASSERT_EQ(16001UL, thread.thread_id);
+}
+
+TEST_F(MinidumpParserTest, GetArchitecture)
+{
+SetUpData("linux-x86_64.dmp");
+ASSERT_EQ(llvm::Triple::ArchType::x86_64, parser.GetArchitecture().GetTriple().getArch());
+}
+
+TEST_F(MinidumpParserTest, GetMiscInfo)
+{
+SetUpData("linux-x86_64.dmp");
+llvm::Optional misc_info = parser.GetMiscInfo();
+ASSERT_FALSE(misc_info.hasValue());
+// linux breakpad generated minidump files don't have misc info stream
+}
Index: unittests/Process/minidump/CMakeLists.txt
===
--- /dev/null
+++ unittests/Process/minidump/CMakeLists.txt
@@ -0,0 +1,8 @@
+add_lldb_unittest(LLDBMinidumpTests
+  MinidumpParserTest.cpp
+  )
+
+set(test_inputs
+   linux-x86_64.dmp)
+
+add_unittest_inputs(LLDBMinidumpTests "${test_inputs}")
Index: unittests/Process/CMakeLists.txt
===
--- unittests/Process/CMakeLists.txt
+++ unittests/Process/CMakeLists.txt
@@ -1 +1,2 @@
 add_subdirectory(gdb-remote)
+add_subdirectory(minidump)
Index: source/Plugins/Process/minidump/MinidumpTypes.h
===
--- /dev/null
+++ source/Plugins/Process/minidump/MinidumpTypes.h
@@ -0,0 +1,275 @@
+//===-- MinidumpTypes.h -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the