[Lldb-commits] [lldb] r278785 - Remove GetThreadSuffixSupported from GDBRemoteCommunication **base** class
Author: labath
Date: Tue Aug 16 04:36:29 2016
New Revision: 278785
URL: http://llvm.org/viewvc/llvm-project?rev=278785&view=rev
Log:
Remove GetThreadSuffixSupported from GDBRemoteCommunication **base** class
Despite its comment, the function is only used in the Client class, and its
presence was merely
complicating mock implementation in unit tests.
Modified:
lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.h
lldb/trunk/unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp
Modified: lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h?rev=278785&r1=278784&r2=278785&view=diff
==
--- lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
(original)
+++ lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h Tue
Aug 16 04:36:29 2016
@@ -126,12 +126,6 @@ public:
}
//--
-// Client and server must implement these pure virtual functions
-//--
-virtual bool
-GetThreadSuffixSupported () = 0;
-
-//--
// Set the global packet timeout.
//
// For clients, this is the timeout that gets used when sending
Modified:
lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h?rev=278785&r1=278784&r2=278785&view=diff
==
--- lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
(original)
+++ lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
Tue Aug 16 04:36:29 2016
@@ -65,7 +65,7 @@ public:
std::string &response_string);
bool
-GetThreadSuffixSupported () override;
+GetThreadSuffixSupported();
// This packet is usually sent first and the boolean return value
// indicates if the packet was send and any response was received
Modified:
lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.h
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.h?rev=278785&r1=278784&r2=278785&view=diff
==
---
lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.h
(original)
+++
lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.h
Tue Aug 16 04:36:29 2016
@@ -176,12 +176,6 @@ protected:
});
}
-bool
-GetThreadSuffixSupported () override
-{
-return true;
-}
-
//--
/// Launch a process with the current launch settings.
///
Modified: lldb/trunk/unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp?rev=278785&r1=278784&r2=278785&view=diff
==
--- lldb/trunk/unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp
(original)
+++ lldb/trunk/unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp Tue Aug
16 04:36:29 2016
@@ -60,12 +60,6 @@ struct MockServer : public GDBRemoteComm
{
MockServer() : GDBRemoteCommunicationServer("mock-server",
"mock-server.listener") { m_send_acks = false; }
-bool
-GetThreadSuffixSupported() override
-{
-return false;
-}
-
PacketResult
SendPacket(llvm::StringRef payload)
{
@@ -84,12 +78,6 @@ struct MockServer : public GDBRemoteComm
struct TestClient : public GDBRemoteClientBase
{
TestClient() : GDBRemoteClientBase("test.client", "test.client.listener")
{ m_send_acks = false; }
-
-bool
-GetThreadSuffixSupported() override
-{
-return false;
-}
};
struct ContinueFixture
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D23545: Minidump parsing
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 University of Illino
Re: [Lldb-commits] [PATCH] D23545: Minidump parsing
labath added a comment.
The implementation looks nice and clean. I have only some stylistic comments
giving it finishing touches..
Comment at: source/Plugins/Process/minidump/CMakeLists.txt:6
@@ +5,3 @@
+ MinidumpParser.cpp
+ #ProcessMinidump.cpp
+ #ThreadMinidump.cpp
Just remove the commented lines.
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:28
@@ +27,3 @@
+void
+MinidumpParser::SetData(const DataExtractor &data)
+{
What's the anticipated usage of this function? I am wondering whether it
shouldn't clear the cached data (m_header, m_directory_map) as well. Or maybe,
since you already have a matching contructor, just remove the function and have
the user construct a new object if he needs to (?)
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:33
@@ +32,3 @@
+
+// this method should be called before doing anything else with the parser
+bool
This kind of comment should go into the header, as it speaks about the
interface.
Comment at: source/Plugins/Process/minidump/MinidumpParser.h:14
@@ +13,3 @@
+// C includes
+#include
+
(nit) if you include , it's not a `C` include :)
I'd put that under `C++`
Comment at: source/Plugins/Process/minidump/MinidumpParser.h:45
@@ +44,3 @@
+
+llvm::Optional
+GetStream(MinidumpStreamType stream_type);
Does the user need to know that you are returning an iterator? The map sounds
like an implementation detail. If you don't have any usages for that in mind,
it would be just cleaner to return the `MinidumpLocationDescriptor` (or maybe a
reference to it) directly. Then you could remove the typedef above as well...
Comment at: source/Plugins/Process/minidump/MinidumpTypes.cpp:83
@@ +82,3 @@
+thread = MinidumpThread::Parse(data, offset);
+if (thread)
+thread_list.push_back(MinidumpThread(thread.getValue()));
Should we abort upon encountering the first invalid thread?
I am thinking of a situation where a corrupt file claims to have UINT32_MAX
threads, and we end up spinning here for quite a long time...
Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:35
@@ +34,3 @@
+
+#define MINIDUMP_SIGNATURE 0x504d444d // 'PMDM'
+#define MINIDUMP_VERSION 0xa793 // 42899
Let's change these into (anonymous) enums as well. We should avoid macros when
possible.
Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:120
@@ +119,3 @@
+MINIDUMP_OS_NACL = 0x8205 /* Native Client (NaCl) */
+} MINIDUMPOSPlatform;
+
This declares a variable. I assume you did not want that.
Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:10
@@ +9,3 @@
+
+#if defined(_MSC_VER) && (_HAS_EXCEPTIONS == 0)
+// Workaround for MSVC standard library bug, which fails to include
when
I think this is only necessary when you are actually using threads.
Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:51
@@ +50,3 @@
+lldb::DataBufferSP data_sp(minidump_file.MemoryMapFileContents());
+DataExtractor data(data_sp, lldb::eByteOrderLittle, 4);
+ASSERT_GT(data.GetByteSize(), 0UL);
Since it's the parser who handles setting the byte order and such, maybe we
should just pass the data buffer to it, and let it construct the data extractor
internally. Would that work?
https://reviews.llvm.org/D23545
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D23553: Move packet construction from GDBRemoteRegisterContext go the communication class
labath created this revision.
labath added a reviewer: clayborg.
labath added a subscriber: lldb-commits.
When saving/restoring registers the GDBRemoteRegisterContext class was manually
constructing
the register save/restore packets. This creates appropriate helper functions in
GDBRemoteCommunicationClient, and switches the class to use those. It also
removes what a
duplicate packet send in some of those functions, a thing that I can only
attribute to a bad
merge artefact.
I also add a test framework for testing gdb-remote client functionality and add
tests for the new
functions I introduced. I'd like to be able to test the register context
changes in isolation as
well, but currently there doesn't seem to be a way to reasonably construct a
standalone register
context object, so we'll have to rely on the end-to-end tests to verify that.
https://reviews.llvm.org/D23553
Files:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp
unittests/Process/gdb-remote/CMakeLists.txt
unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp
unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
unittests/Process/gdb-remote/GDBRemoteTestUtils.cpp
unittests/Process/gdb-remote/GDBRemoteTestUtils.h
Index: unittests/Process/gdb-remote/GDBRemoteTestUtils.h
===
--- /dev/null
+++ unittests/Process/gdb-remote/GDBRemoteTestUtils.h
@@ -0,0 +1,57 @@
+//===-- GDBRemoteTestUtils.h *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+#ifndef lldb_unittests_Process_gdb_remote_GDBRemoteTestUtils_h
+#define lldb_unittests_Process_gdb_remote_GDBRemoteTestUtils_h
+
+#include "Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h"
+
+namespace lldb_private
+{
+namespace process_gdb_remote
+{
+
+class GDBRemoteTest : public testing::Test
+{
+public:
+static void
+SetUpTestCase();
+
+static void
+TearDownTestCase();
+};
+
+void
+Connect(GDBRemoteCommunication &client, GDBRemoteCommunication &server);
+
+struct MockServer : public GDBRemoteCommunicationServer
+{
+MockServer() : GDBRemoteCommunicationServer("mock-server", "mock-server.listener") { m_send_acks = false; }
+
+PacketResult
+SendPacket(llvm::StringRef payload)
+{
+return GDBRemoteCommunicationServer::SendPacketNoLock(payload.data(), payload.size());
+}
+
+PacketResult
+GetPacket(StringExtractorGDBRemote &response)
+{
+const unsigned timeout_usec = 100; // 1s
+const bool sync_on_timeout = false;
+return WaitForPacketWithTimeoutMicroSecondsNoLock(response, timeout_usec, sync_on_timeout);
+}
+
+using GDBRemoteCommunicationServer::SendOKResponse;
+using GDBRemoteCommunicationServer::SendUnimplementedResponse;
+};
+
+} // namespace process_gdb_remote
+} // namespace lldb_private
+
+#endif // lldb_unittests_Process_gdb_remote_GDBRemoteTestUtils_h
Index: unittests/Process/gdb-remote/GDBRemoteTestUtils.cpp
===
--- /dev/null
+++ unittests/Process/gdb-remote/GDBRemoteTestUtils.cpp
@@ -0,0 +1,74 @@
+//===-- GDBRemoteTestUtils.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 "GDBRemoteTestUtils.h"
+#include "gtest/gtest.h"
+
+#include "lldb/Host/common/TCPSocket.h"
+#include "lldb/Host/posix/ConnectionFileDescriptorPosix.h"
+
+#include
+
+namespace lldb_private
+{
+namespace process_gdb_remote
+{
+
+void
+GDBRemoteTest::SetUpTestCase()
+{
+#if defined(_MSC_VER)
+WSADATA data;
+::WSAStartup(MAKEWORD(2, 2), &data);
+#endif
+}
+
+void
+GDBRemoteTest::TearDownTestCase()
+{
+#if defined(_MSC_VER)
+::WSACleanup();
+#endif
+}
+
+void
+Connect(GDBRemoteCommunication &client, GDBRemoteCommunication &server)
+{
+bool child_processes_inherit = false;
+Error error;
+TCPSocket listen_socket(child_processes_inherit, error);
+ASSERT_FALSE(error.Fail());
+error = listen_socket.Listen("127.0.0.1:0", 5);
+ASSERT_FALSE(error.Fail());
+
+Socket *accept_socket;
+std::future accept_error = std::async(std::launch::async, [&] {
+return listen_socket.Accept(
Re: [Lldb-commits] [PATCH] D23553: Move packet construction from GDBRemoteRegisterContext go the communication class
labath added inline comments.
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp:601
@@ -600,3 @@
-
-if (gdb_comm.SendPacketAndWaitForResponse(packet, packet_len,
response, false) == GDBRemoteCommunication::PacketResult::Success)
-{
The location of the duplicate `Send`. Also similar two cases in
`WriteAllRegisterValues`.
https://reviews.llvm.org/D23553
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D23553: Move packet construction from GDBRemoteRegisterContext go the communication class
labath updated this revision to Diff 68171.
labath added a comment.
Fix indentation
https://reviews.llvm.org/D23553
Files:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp
unittests/Process/gdb-remote/CMakeLists.txt
unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp
unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
unittests/Process/gdb-remote/GDBRemoteTestUtils.cpp
unittests/Process/gdb-remote/GDBRemoteTestUtils.h
Index: unittests/Process/gdb-remote/GDBRemoteTestUtils.h
===
--- /dev/null
+++ unittests/Process/gdb-remote/GDBRemoteTestUtils.h
@@ -0,0 +1,57 @@
+//===-- GDBRemoteTestUtils.h *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+#ifndef lldb_unittests_Process_gdb_remote_GDBRemoteTestUtils_h
+#define lldb_unittests_Process_gdb_remote_GDBRemoteTestUtils_h
+
+#include "Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h"
+
+namespace lldb_private
+{
+namespace process_gdb_remote
+{
+
+class GDBRemoteTest : public testing::Test
+{
+public:
+static void
+SetUpTestCase();
+
+static void
+TearDownTestCase();
+};
+
+void
+Connect(GDBRemoteCommunication &client, GDBRemoteCommunication &server);
+
+struct MockServer : public GDBRemoteCommunicationServer
+{
+MockServer() : GDBRemoteCommunicationServer("mock-server", "mock-server.listener") { m_send_acks = false; }
+
+PacketResult
+SendPacket(llvm::StringRef payload)
+{
+return GDBRemoteCommunicationServer::SendPacketNoLock(payload.data(), payload.size());
+}
+
+PacketResult
+GetPacket(StringExtractorGDBRemote &response)
+{
+const unsigned timeout_usec = 100; // 1s
+const bool sync_on_timeout = false;
+return WaitForPacketWithTimeoutMicroSecondsNoLock(response, timeout_usec, sync_on_timeout);
+}
+
+using GDBRemoteCommunicationServer::SendOKResponse;
+using GDBRemoteCommunicationServer::SendUnimplementedResponse;
+};
+
+} // namespace process_gdb_remote
+} // namespace lldb_private
+
+#endif // lldb_unittests_Process_gdb_remote_GDBRemoteTestUtils_h
Index: unittests/Process/gdb-remote/GDBRemoteTestUtils.cpp
===
--- /dev/null
+++ unittests/Process/gdb-remote/GDBRemoteTestUtils.cpp
@@ -0,0 +1,74 @@
+//===-- GDBRemoteTestUtils.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 "GDBRemoteTestUtils.h"
+#include "gtest/gtest.h"
+
+#include "lldb/Host/common/TCPSocket.h"
+#include "lldb/Host/posix/ConnectionFileDescriptorPosix.h"
+
+#include
+
+namespace lldb_private
+{
+namespace process_gdb_remote
+{
+
+void
+GDBRemoteTest::SetUpTestCase()
+{
+#if defined(_MSC_VER)
+WSADATA data;
+::WSAStartup(MAKEWORD(2, 2), &data);
+#endif
+}
+
+void
+GDBRemoteTest::TearDownTestCase()
+{
+#if defined(_MSC_VER)
+::WSACleanup();
+#endif
+}
+
+void
+Connect(GDBRemoteCommunication &client, GDBRemoteCommunication &server)
+{
+bool child_processes_inherit = false;
+Error error;
+TCPSocket listen_socket(child_processes_inherit, error);
+ASSERT_FALSE(error.Fail());
+error = listen_socket.Listen("127.0.0.1:0", 5);
+ASSERT_FALSE(error.Fail());
+
+Socket *accept_socket;
+std::future accept_error = std::async(std::launch::async, [&] {
+return listen_socket.Accept("127.0.0.1:0", child_processes_inherit, accept_socket);
+});
+
+char connect_remote_address[64];
+snprintf(connect_remote_address, sizeof(connect_remote_address), "connect://localhost:%u",
+ listen_socket.GetLocalPortNumber());
+
+std::unique_ptr conn_ap(new ConnectionFileDescriptor());
+ASSERT_EQ(conn_ap->Connect(connect_remote_address, nullptr), lldb::eConnectionStatusSuccess);
+
+client.SetConnection(conn_ap.release());
+ASSERT_TRUE(accept_error.get().Success());
+server.SetConnection(new ConnectionFileDescriptor(accept_socket));
+}
+
+} // namespace process_gdb_remote
+} // namespace lldb_private
Index: unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
=
Re: [Lldb-commits] [PATCH] D22914: [WIP] Add concurrent packets support to gdb-remote client
labath added a comment. In https://reviews.llvm.org/D22914#511294, @clayborg wrote: > OK. I can't complain if it doesn't affect performance and could allow > multiple packets to be in flight with threading, though I think solving this > issue with threading is a bit of a hack, but it should work. Thanks for > running the perf tests. Thanks. I understand your reluctance. I am not completely convinced of the optimality of the solution, but to me it looks like the best way forward at the moment. I want to proceed slowly with this, to make it as little "hacky" and obtrusive as possible. I can't commit this patch yet because it replaces the sequence mutex with a non-recursive rwlock. The biggest offender using recursive locks here is the gdb-remote register context, so I have started to do some cleanups there first, so I am able to get rid of the recursion. (The motivation for https://reviews.llvm.org/D23553). https://reviews.llvm.org/D22914 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D23554: Remove cmake legacy code
labath created this revision.
labath added a reviewer: zturner.
labath added a subscriber: lldb-commits.
Cmake 2.8 support is gone and not coming back. We can remove a bit of legacy
code now.
https://reviews.llvm.org/D23554
Files:
cmake/modules/AddLLDB.cmake
cmake/modules/LLDBConfig.cmake
source/API/CMakeLists.txt
Index: source/API/CMakeLists.txt
===
--- source/API/CMakeLists.txt
+++ source/API/CMakeLists.txt
@@ -117,5 +117,5 @@
if (LLDB_WRAP_PYTHON)
add_dependencies(liblldb swig_wrapper)
endif()
-target_link_libraries(liblldb ${cmake_2_8_12_PRIVATE} ${LLDB_SYSTEM_LIBS})
+target_link_libraries(liblldb PRIVATE ${LLDB_SYSTEM_LIBS})
Index: cmake/modules/LLDBConfig.cmake
===
--- cmake/modules/LLDBConfig.cmake
+++ cmake/modules/LLDBConfig.cmake
@@ -268,16 +268,6 @@
${PACKAGE_VERSION})
message(STATUS "LLDB version: ${LLDB_VERSION}")
-if (CMAKE_VERSION VERSION_LESS 2.8.12)
- set(cmake_2_8_12_INTERFACE)
- set(cmake_2_8_12_PRIVATE)
- set(cmake_2_8_12_PUBLIC)
-else ()
- set(cmake_2_8_12_INTERFACE INTERFACE)
- set(cmake_2_8_12_PRIVATE PRIVATE)
- set(cmake_2_8_12_PUBLIC PUBLIC)
-endif ()
-
include_directories(BEFORE
${CMAKE_CURRENT_BINARY_DIR}/include
${CMAKE_CURRENT_SOURCE_DIR}/include
Index: cmake/modules/AddLLDB.cmake
===
--- cmake/modules/AddLLDB.cmake
+++ cmake/modules/AddLLDB.cmake
@@ -4,7 +4,7 @@
endif()
if(${targetkind} MATCHES "SHARED")
-set(LINK_KEYWORD ${cmake_2_8_12_PUBLIC})
+set(LINK_KEYWORD PUBLIC)
endif()
if(${targetkind} MATCHES "SHARED" OR ${targetkind} MATCHES "EXE")
@@ -62,10 +62,10 @@
if (PARAM_SHARED)
if (LLDB_LINKER_SUPPORTS_GROUPS)
-target_link_libraries(${name} ${cmake_2_8_12_PUBLIC}
+target_link_libraries(${name} PUBLIC
-Wl,--start-group ${CLANG_USED_LIBS} -Wl,--end-group)
else()
-target_link_libraries(${name} ${cmake_2_8_12_PUBLIC}
${CLANG_USED_LIBS})
+target_link_libraries(${name} PUBLIC ${CLANG_USED_LIBS})
endif()
endif()
llvm_config(${name} ${LLVM_LINK_COMPONENTS}
${LLVM_PRIVATE_LINK_COMPONENTS})
Index: source/API/CMakeLists.txt
===
--- source/API/CMakeLists.txt
+++ source/API/CMakeLists.txt
@@ -117,5 +117,5 @@
if (LLDB_WRAP_PYTHON)
add_dependencies(liblldb swig_wrapper)
endif()
-target_link_libraries(liblldb ${cmake_2_8_12_PRIVATE} ${LLDB_SYSTEM_LIBS})
+target_link_libraries(liblldb PRIVATE ${LLDB_SYSTEM_LIBS})
Index: cmake/modules/LLDBConfig.cmake
===
--- cmake/modules/LLDBConfig.cmake
+++ cmake/modules/LLDBConfig.cmake
@@ -268,16 +268,6 @@
${PACKAGE_VERSION})
message(STATUS "LLDB version: ${LLDB_VERSION}")
-if (CMAKE_VERSION VERSION_LESS 2.8.12)
- set(cmake_2_8_12_INTERFACE)
- set(cmake_2_8_12_PRIVATE)
- set(cmake_2_8_12_PUBLIC)
-else ()
- set(cmake_2_8_12_INTERFACE INTERFACE)
- set(cmake_2_8_12_PRIVATE PRIVATE)
- set(cmake_2_8_12_PUBLIC PUBLIC)
-endif ()
-
include_directories(BEFORE
${CMAKE_CURRENT_BINARY_DIR}/include
${CMAKE_CURRENT_SOURCE_DIR}/include
Index: cmake/modules/AddLLDB.cmake
===
--- cmake/modules/AddLLDB.cmake
+++ cmake/modules/AddLLDB.cmake
@@ -4,7 +4,7 @@
endif()
if(${targetkind} MATCHES "SHARED")
-set(LINK_KEYWORD ${cmake_2_8_12_PUBLIC})
+set(LINK_KEYWORD PUBLIC)
endif()
if(${targetkind} MATCHES "SHARED" OR ${targetkind} MATCHES "EXE")
@@ -62,10 +62,10 @@
if (PARAM_SHARED)
if (LLDB_LINKER_SUPPORTS_GROUPS)
-target_link_libraries(${name} ${cmake_2_8_12_PUBLIC}
+target_link_libraries(${name} PUBLIC
-Wl,--start-group ${CLANG_USED_LIBS} -Wl,--end-group)
else()
-target_link_libraries(${name} ${cmake_2_8_12_PUBLIC} ${CLANG_USED_LIBS})
+target_link_libraries(${name} PUBLIC ${CLANG_USED_LIBS})
endif()
endif()
llvm_config(${name} ${LLVM_LINK_COMPONENTS} ${LLVM_PRIVATE_LINK_COMPONENTS})
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D20386: Correct makefile.rules to use arm/aarch64 target specific AR and OBJCOPY
labath added a comment. Sounds that we're agreed then. Can you please update the patch to use `?=` as appropriate? I will update the buildbots to set these variables explicitly as needed, and then we can get this in. https://reviews.llvm.org/D20386 ___ 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
dvlahovski updated this revision to Diff 68177.
dvlahovski added a comment.
Resolving Pavel's remarks
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 - relative virtual address - basically offset from the beginning of the file
+t
Re: [Lldb-commits] [PATCH] D23545: Minidump parsing
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 - relative vi
Re: [Lldb-commits] [PATCH] D23545: Minidump parsing
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
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] [lldb] r277117 - Fix -break-insert not working when using absolute paths (MI)
Greg: ping?
On Mon, Aug 8, 2016 at 1:38 PM, Hans Wennborg wrote:
> Greg: ping?
>
> On Wed, Aug 3, 2016 at 9:06 AM, Hans Wennborg wrote:
>> For LLDB, I think it's up to Greg to decide.
>>
>> On Tue, Aug 2, 2016 at 10:20 PM, Ilia K wrote:
>>> Hi Hans!
>>>
>>> The author of this commit asks me is there a chance to include this changes
>>> to 3.9 release? I'm not sure about our policy when RC has already been
>>> tagged.
>>>
>>> On Fri, Jul 29, 2016 at 9:01 AM, Ilia K via lldb-commits
>>> wrote:
Author: ki.stfu
Date: Fri Jul 29 01:01:20 2016
New Revision: 277117
URL: http://llvm.org/viewvc/llvm-project?rev=277117&view=rev
Log:
Fix -break-insert not working when using absolute paths (MI)
Summary:
When trying to parse the -break-insert arguments as a named location, the
string parsing was not configured to allow directory paths. This patch adds
a constructor to allow the parsing of string as directory path along with
the other parameters.
This fixes https://llvm.org/bugs/show_bug.cgi?id=28709
Patch from malape...@gmail.com
Reviewers: clayborg, ki.stfu
Subscribers: lldb-commits, ki.stfu
Differential Revision: https://reviews.llvm.org/D22902
Modified:
lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-mi/breakpoint/TestMiBreak.py
lldb/trunk/tools/lldb-mi/MICmdArgValString.cpp
lldb/trunk/tools/lldb-mi/MICmdArgValString.h
lldb/trunk/tools/lldb-mi/MICmdCmdBreak.cpp
Modified:
lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-mi/breakpoint/TestMiBreak.py
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-mi/breakpoint/TestMiBreak.py?rev=277117&r1=277116&r2=277117&view=diff
==
---
lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-mi/breakpoint/TestMiBreak.py
(original)
+++
lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-mi/breakpoint/TestMiBreak.py
Fri Jul 29 01:01:20 2016
@@ -155,7 +155,6 @@ class MiBreakTestCase(lldbmi_testcase.Mi
@skipIfWindows #llvm.org/pr24452: Get lldb-mi tests working on
Windows
@skipIfFreeBSD # llvm.org/pr22411: Failure presumably due to known
thread races
-@unittest2.expectedFailure("-break-insert doesn't work for absolute
path")
def test_lldbmi_break_insert_file_line_absolute_path(self):
"""Test that 'lldb-mi --interpreter' works for file:line
breakpoints."""
Modified: lldb/trunk/tools/lldb-mi/MICmdArgValString.cpp
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/tools/lldb-mi/MICmdArgValString.cpp?rev=277117&r1=277116&r2=277117&view=diff
==
--- lldb/trunk/tools/lldb-mi/MICmdArgValString.cpp (original)
+++ lldb/trunk/tools/lldb-mi/MICmdArgValString.cpp Fri Jul 29 01:01:20
2016
@@ -85,6 +85,30 @@ CMICmdArgValString::CMICmdArgValString(c
}
//++
+// Details: CMICmdArgValString constructor.
+// Type:Method.
+// Args:vrArgName - (R) Argument's name to search by.
+// vbMandatory - (R) True = Yes must be present, false =
optional argument.
+// vbHandleByCmd - (R) True = Command processes *this option,
false = not handled.
+// vbHandleQuotes - (R) True = Parse a string surrounded by
quotes spaces are not delimiters, false = only text up to
+// next delimiting space character.
+// vbAcceptNumbers - (R) True = Parse a string and accept as a
number if number, false = numbers not recognised as
+// vbHandleDirPaths - (R) True = Parse a string and accept as a
file path if a path, false = file paths are not
+// string types.
+// Return: None.
+// Throws: None.
+//--
+CMICmdArgValString::CMICmdArgValString(const CMIUtilString &vrArgName,
const bool vbMandatory, const bool vbHandleByCmd,
+ const bool vbHandleQuotes, const bool vbAcceptNumbers,
const bool vbHandleDirPaths)
+: CMICmdArgValBaseTemplate(vrArgName, vbMandatory, vbHandleByCmd)
+, m_bHandleQuotedString(vbHandleQuotes)
+, m_bAcceptNumbers(vbAcceptNumbers)
+, m_bHandleDirPaths(vbHandleDirPaths)
+, m_bHandleAnything(false)
+{
+}
+
+//++
// Details: CMICmdArgValString destructor.
// Type:Overridden.
// Args:None.
Modified: lldb/trunk/tools/lldb-mi/MICmdArgValString.h
URL:
http://llvm.org/viewvc/llvm
Re: [Lldb-commits] [PATCH] D23545: Minidump parsing
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
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
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] D23553: Move packet construction from GDBRemoteRegisterContext go the communication class
clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Looks good. I know there are many other functions that we should port down into the client code. https://reviews.llvm.org/D23553 ___ 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
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
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 &data, lldb::offset_t
*offset);
static llvm::Optional
Parse(const DataExtractor &data, 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 &Buffer, const T *&Object) {
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 &Buffer, StringRef &ZeroString) {
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
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 &data, lldb::offset_t
> *offset);
>
> static llvm::Optional
> Parse(const DataExtractor &data, 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 &Buffer, const T *&Object) {
> 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 &Buffer, StringRef &ZeroString) {
> 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
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);
+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;
+static_assert(sizeof(MinidumpHeader) == MINIDUMP_HEA
[Lldb-commits] [lldb] r278901 - Fix the RangeMapVector::FindEntryThatContainsOrFollows method to
Author: jmolenda
Date: Tue Aug 16 22:56:04 2016
New Revision: 278901
URL: http://llvm.org/viewvc/llvm-project?rev=278901&view=rev
Log:
Fix the RangeMapVector::FindEntryThatContainsOrFollows method to
back up the iterator, as long as it still contains the address.
std::lower_bound will point us to the entry after the one we
are really interested in, leading to problems with backtracing
in corefiles.
Modified:
lldb/trunk/include/lldb/Core/RangeMap.h
Modified: lldb/trunk/include/lldb/Core/RangeMap.h
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Core/RangeMap.h?rev=278901&r1=278900&r2=278901&view=diff
==
--- lldb/trunk/include/lldb/Core/RangeMap.h (original)
+++ lldb/trunk/include/lldb/Core/RangeMap.h Tue Aug 16 22:56:04 2016
@@ -1341,6 +1341,14 @@ namespace lldb_private {
return nullptr;
}
+// This method will return the entry that contains the given address,
or the
+// entry following that address. If you give it an address of 0 and
the first
+// entry starts at address 0x100, you will get the entry at 0x100.
+//
+// For most uses, FindEntryThatContains is the correct one to use,
this is a
+// less commonly needed behavior. It was added for core file memory
regions,
+// where we want to present a gap in the memory regions as a distinct
region,
+// so we need to know the start address of the next memory section
that exists.
const Entry *
FindEntryThatContainsOrFollows(B addr) const
{
@@ -1349,12 +1357,16 @@ namespace lldb_private {
#endif
if (!m_entries.empty())
{
+typename Collection::const_iterator begin = m_entries.begin();
typename Collection::const_iterator end = m_entries.end();
typename Collection::const_iterator pos =
std::lower_bound(m_entries.begin(), end, addr, [](const
Entry &lhs, B rhs_base) -> bool {
return lhs.GetRangeEnd() <= rhs_base;
});
+while (pos != begin && pos[-1].Contains(addr))
+--pos;
+
if (pos != end)
return &(*pos);
}
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
