[Lldb-commits] [PATCH] D32585: Implementation of remote packets for Trace data.
ravitheja updated this revision to Diff 99716.
ravitheja added a comment.
Adding unit tests and making the packets fully JSON.
https://reviews.llvm.org/D32585
Files:
docs/lldb-gdb-remote.txt
include/lldb/API/SBTrace.h
include/lldb/Core/TraceOptions.h
include/lldb/Host/common/NativeProcessProtocol.h
include/lldb/Target/Process.h
include/lldb/Utility/StringExtractor.h
source/API/SBProcess.cpp
source/API/SBTrace.cpp
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
source/Utility/StringExtractor.cpp
source/Utility/StringExtractorGDBRemote.cpp
source/Utility/StringExtractorGDBRemote.h
unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
Index: unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
===
--- unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
+++ unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
@@ -14,6 +14,7 @@
#include "lldb/Core/ModuleSpec.h"
#include "lldb/Core/StructuredData.h"
#include "lldb/Target/MemoryRegionInfo.h"
+#include "lldb/Core/TraceOptions.h"
#include "lldb/Utility/DataBuffer.h"
#include "llvm/ADT/ArrayRef.h"
@@ -370,3 +371,202 @@
HandlePacket(server, "qMemoryRegionInfo:4000", "start:4000;size:;");
EXPECT_FALSE(result.get().Success());
}
+
+TEST_F(GDBRemoteCommunicationClientTest, SendStartTracePacket) {
+ TestClient client;
+ MockServer server;
+ Connect(client, server);
+ if (HasFailure())
+return;
+
+ TraceOptions options;
+ Status error;
+
+ options.setType(lldb::TraceType::eTraceTypeProcessorTrace);
+ options.setMetaDataBufferSize(8192);
+ options.setTraceBufferSize(8192);
+ options.setThreadID(0x23);
+
+ StructuredData::DictionarySP custom_params = std::make_shared ();
+ custom_params->AddStringItem("tracetech","intel-pt");
+ custom_params->AddIntegerItem("psb",0x01);
+
+ options.setTraceParams(custom_params);
+
+ std::future result = std::async(std::launch::async, [&] {
+return client.SendStartTracePacket(options, error);
+ });
+
+ HandlePacket(server, "jTraceStart:{\"buffersize\" : 8192,\"jparams\" : {\"psb\" : 1,\"tracetech\" : \"intel-pt\"},\"metabuffersize\" : 8192,\"threadid\" : 35,\"type\" : 1}", "1");
+ ASSERT_EQ(result.get(),1);
+ ASSERT_TRUE(error.Success());
+
+ error.Clear();
+ result = std::async(std::launch::async, [&] {
+return client.SendStartTracePacket(options, error);
+ });
+
+ HandlePacket(server, "jTraceStart:{\"buffersize\" : 8192,\"jparams\" : {\"psb\" : 1,\"tracetech\" : \"intel-pt\"},\"metabuffersize\" : 8192,\"threadid\" : 35,\"type\" : 1}", "E23");
+ ASSERT_EQ(result.get(), LLDB_INVALID_UID);
+ ASSERT_FALSE(error.Success());
+}
+
+TEST_F(GDBRemoteCommunicationClientTest, SendStopTracePacket) {
+ TestClient client;
+ MockServer server;
+ Connect(client, server);
+ if (HasFailure())
+return;
+
+ lldb::tid_t thread_id = 0x23;
+ lldb::user_id_t trace_id = 3;
+
+ std::future result = std::async(std::launch::async, [&] {
+return client.SendStopTracePacket(trace_id, thread_id);
+ });
+
+ HandlePacket(server, "jTraceStop:{\"threadid\" : 35,\"traceid\" : 3}", "OK");
+ ASSERT_TRUE(result.get().Success());
+
+ result = std::async(std::launch::async, [&] {
+return client.SendStopTracePacket(trace_id, thread_id);
+ });
+
+ HandlePacket(server, "jTraceStop:{\"threadid\" : 35,\"traceid\" : 3}", "E23");
+ ASSERT_FALSE(result.get().Success());
+}
+
+TEST_F(GDBRemoteCommunicationClientTest, SendGetDataPacket) {
+ TestClient client;
+ MockServer server;
+ Connect(client, server);
+ if (HasFailure())
+return;
+
+ lldb::tid_t thread_id = 0x23;
+ lldb::user_id_t trace_id = 3;
+
+ uint8_t buf[32] = {};
+ llvm::MutableArrayRef buffer (buf, 32);
+ size_t offset = 0;
+
+ std::future result = std::async(std::launch::async, [&] {
+return client.SendGetDataPacket(trace_id, thread_id, buffer, offset);
+ });
+
+ StringRef expected("jTraceBufferRead:{\"buffersize\" : 32,\"offset\" : 0,\"threadid\" : 35,\"traceid\" : 3}");
+ HandlePacket(server, expected, "123456");
+ ASSERT_TRUE(result.get().Success());
+ ASSERT_EQ(buffer.size(), 3);
+ ASSERT_EQ(buf[0], 0x12);
+ ASSERT_EQ(buf[1], 0x34);
+ ASSERT_EQ(buf[2], 0x56);
+
+ llvm::MutableArrayRef buffer2 (buf, 32);
+ result = std::async(std::launch::async, [&] {
+return client.SendGetDataPacket(trace_id, thread_id, buffer2, offset);
+ });
+
+ HandlePacket(server, expected, "E23");
+ ASSERT_FALSE(result.get().Success());
+ ASSERT_EQ(buffer2.size(), 0);
+}
+
+TEST_F(GDBRemoteCommunicationClientTest, SendGetMetaDataPacket) {
+ TestClient clie
[Lldb-commits] [PATCH] D32585: Implementation of remote packets for Trace data.
ravitheja added a comment. Sorry I had to update, since the Error class has been changed to Status. https://reviews.llvm.org/D32585 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32930: New framework for lldb client-server communication tests.
labath added a comment.
Ok, so the comments below are basically a collection of nits. The only two
major issues that still need addressing are:
- getting rid of the sleep in the startup code
- deciding on the naming of members
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:23
+ auto elements_or_error = SplitPairList("ProcessInfo", response);
+ if (auto split_error = elements_or_error.takeError()) {
+return std::move(split_error);
`if (!elements_or_error) return elements_or_error.takeError()` is a bit shorter
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:27
+
+ auto elements = *elements_or_error;
+ if (elements["pid"].getAsInteger(16, process_info.pid))
This introduces a copy. It does not matter much for test code, but best be wary
of that issue in general. You should do `auto &elements = ...`
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:47
+ ThreadInfo() = default;
+ explicit ThreadInfo(llvm::StringRef name, llvm::StringRef reason,
+ const RegisterMap ®isters, unsigned int signal);
explicit not needed
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:50
+
+ std::string ReadRegister(unsigned int register_id) const;
+ bool ReadRegisterAsUint64(unsigned int register_id, uint64_t &value) const;
StringRef
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:22
+#include
+#include
+#include
Including iostream is banned.
Besides, I'm pretty sure you don't need it for what you are doing in this file.
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:60
+GTEST_LOG_(ERROR) << formatv("Failure to launch lldb server: {0}.",
+ status.AsCString())
+ .str();
Status has a format provider, so you can just drop the `.AsCString()` thingy
here.
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:124
+ if (!SendMessage("jThreadsInfo", response))
+return jthreads_info;
+ auto creation = JThreadsInfo::Create(response, process_info->GetEndian());
`return llvm::None;` Then you can drop the dummy optional object.
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:131
+
+ return *creation;
+ // jthreads_info = *creation;
std::move(*creation) to avoid a copy
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:137
+const StopReply &TestClient::GetLatestStopReply() {
+ return (stop_reply.getValue());
+}
extra parens
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:216
+
+ stop_reply = *creation;
+ return true;
std::move
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:222
+ StartListenThread(g_local_host, 0);
+ auto connection = (ConnectionFileDescriptor *)GetConnection();
+ uint16_t listening_port = connection->GetListeningPort(UINT32_MAX);
static_cast
https://reviews.llvm.org/D32930
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32585: Implementation of remote packets for Trace data.
labath added a comment.
Thank you for taking the time to write the test. Just a couple of more comments
on things I noticed when going through this again:
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:3172
+ if (custom_params)
+json_packet.AddItem("jparams", custom_params);
+
I don't think we need the "j" in the "jparams" now that the whole packet is
json.
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:3288
+
+ json_dict->GetValueForKeyAsInteger("type", type);
+ options.setType(static_cast(type));
I'm wondering whether we should be sending the trace type as an opaque number
-- it's much easier to allocate IDs and manage compatibility if this is a
string field. Greg, what do you think?
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1142
+ StructuredData::ObjectSP custom_params_sp =
json_dict->GetValueForKey("jparams");
+ options.setTraceParams(static_pointer_cast
(custom_params_sp));
+
What if jparams **isn't** a dictionary?
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1298
+ std::vector buffer(byte_count, '\0');
+ if (buffer.empty())
+return SendErrorResponse(0x78);
this is dead code. empty will never return true here. The process will just get
killed if it fails to allocate.
However, you are right that we should check the allocation as we are allocating
a buffer of user-provided size. I recommend explicitly allocating a uint8_t[]
using nothrow new. It doesn't stop us from getting killed if the kernel
overcommits, but it's better than nothing.
Alternatively you could also change the API to leave the memory allocation to
the callee, which will be in a better position do determine whether the size is
reasonable.
https://reviews.llvm.org/D32585
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D33409: Add support for new (3.0.11+) swigs
labath created this revision.
A change in swig 3.0.9 has caused it to generate modules incompatible
with us using them as __init__.py (bug #769). Swig 3.0.11 adds a setting to help
fix this problem, so use that. Support for older versions of swig remains
unaffected.
https://reviews.llvm.org/D33409
Files:
scripts/lldb.swig
Index: scripts/lldb.swig
===
--- scripts/lldb.swig
+++ scripts/lldb.swig
@@ -31,8 +31,24 @@
and a source file location. SBCompileUnit contains SBLineEntry(s)."
%enddef
+/*
+Since version 3.0.9, swig's logic for importing the native module has changed
in
+a way that is incompatible with our usage of the python module as __init__.py
+(See swig bug #769). Fortunately, since version 3.0.11, swig provides a way
for
+us to override the module import logic to suit our needs. This does that.
+
+Older swig versions will simply ignore this setting.
+*/
+%define MODULEIMPORT
+"import $module"
+%enddef
+// These versions will not generate working python modules, so error out early.
+#if SWIG_VERSION >= 0x030009 && SWIG_VERSION < 0x030011
+#error Swig versions 3.0.9 and 3.0.10 are incompatible with lldb.
+#endif
+
// The name of the module to be created.
-%module(docstring=DOCSTRING) lldb
+%module(docstring=DOCSTRING, moduleimport=MODULEIMPORT) lldb
// Parameter types will be used in the autodoc string.
%feature("autodoc", "1");
Index: scripts/lldb.swig
===
--- scripts/lldb.swig
+++ scripts/lldb.swig
@@ -31,8 +31,24 @@
and a source file location. SBCompileUnit contains SBLineEntry(s)."
%enddef
+/*
+Since version 3.0.9, swig's logic for importing the native module has changed in
+a way that is incompatible with our usage of the python module as __init__.py
+(See swig bug #769). Fortunately, since version 3.0.11, swig provides a way for
+us to override the module import logic to suit our needs. This does that.
+
+Older swig versions will simply ignore this setting.
+*/
+%define MODULEIMPORT
+"import $module"
+%enddef
+// These versions will not generate working python modules, so error out early.
+#if SWIG_VERSION >= 0x030009 && SWIG_VERSION < 0x030011
+#error Swig versions 3.0.9 and 3.0.10 are incompatible with lldb.
+#endif
+
// The name of the module to be created.
-%module(docstring=DOCSTRING) lldb
+%module(docstring=DOCSTRING, moduleimport=MODULEIMPORT) lldb
// Parameter types will be used in the autodoc string.
%feature("autodoc", "1");
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32522: Test case for the issue raised in D32271
labath added inline comments.
Comment at:
packages/Python/lldbsuite/test/functionalities/process_attach/TestProcessAttach.py:47
+exe = os.path.join('.','newdir','proc_attach')
+self.addTearDownHook(lambda: shutil.rmtree(os.path.join(os.getcwd(
+
emaste wrote:
> shouldn't this be ... `rmtree(os.path.join(os.getcwd(), 'newdir'))` instead?
Probably not because he does a os.chdir(newdir) below. However, that does not
make this a good idea, as you will delete an unexpected directory if the test
fails before you get a chance to chdir. I like the Ed's earlier suggestion to
launch the process from a separate directory more than this.
Repository:
rL LLVM
https://reviews.llvm.org/D32522
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32585: Implementation of remote packets for Trace data.
krytarowski added inline comments.
Comment at: include/lldb/Host/common/NativeProcessProtocol.h:352
+ lldb::tid_t thread = LLDB_INVALID_THREAD_ID) {
+return Status("Not implemented");
+ }
This is Linuxism. Not every OS can trace on per-thread basis.
https://reviews.llvm.org/D32585
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r303553 - Fix incorrect Status -> Error rename in IOHandler
Author: labath
Date: Mon May 22 09:13:38 2017
New Revision: 303553
URL: http://llvm.org/viewvc/llvm-project?rev=303553&view=rev
Log:
Fix incorrect Status -> Error rename in IOHandler
Change 302872 was a massive rename of the Error class to Status.
The change included an incorrect rename of the "Status" window
in the LLDB GUI from "Status to "Error". This patch undoes this incorrect
rename and restores the status window's correct name.
Differential Revision: https://reviews.llvm.org/D33241
Patch by Brian Gianforcaro.
Modified:
lldb/trunk/source/Core/IOHandler.cpp
Modified: lldb/trunk/source/Core/IOHandler.cpp
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Core/IOHandler.cpp?rev=303553&r1=303552&r2=303553&view=diff
==
--- lldb/trunk/source/Core/IOHandler.cpp (original)
+++ lldb/trunk/source/Core/IOHandler.cpp Mon May 22 09:13:38 2017
@@ -4640,7 +4640,7 @@ void IOHandlerCursesGUI::Activate() {
WindowSP threads_window_sp(
main_window_sp->CreateSubWindow("Threads", threads_bounds, false));
WindowSP status_window_sp(
-main_window_sp->CreateSubWindow("Error", status_bounds, false));
+main_window_sp->CreateSubWindow("Status", status_bounds, false));
status_window_sp->SetCanBeActive(
false); // Don't let the status bar become the active window
main_window_sp->SetDelegate(
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D33347: Fix incorrect Status -> Error rename in IOHandler
labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. Committed as r303553. Thanks for the patch. https://reviews.llvm.org/D33347 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32585: Implementation of remote packets for Trace data.
ravitheja added inline comments.
Comment at: include/lldb/Host/common/NativeProcessProtocol.h:352
+ lldb::tid_t thread = LLDB_INVALID_THREAD_ID) {
+return Status("Not implemented");
+ }
krytarowski wrote:
> This is Linuxism. Not every OS can trace on per-thread basis.
Yes , the trace id (uid in the example u commented) could refer to a trace
instance on a process or a thread. So the following two things can happen ->
1) uid corresponds to a trace instance on a process. In this case thread only
needs to specified if the user wants to stop tracing on an individual thread
inside a process. If thread level tracing is not supported then thread arg need
not be specified.
2) uid corresponds to a trace instance on a thread. In which case the thread
argument need not specified and the trace instance should be able to stop the
trace.
So to summarize the thread argument only comes into picture when thread level
tracing is supported and the user wants to stop tracing on an individual thread
of a process being traced.
https://reviews.llvm.org/D32585
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D33409: Add support for new (3.0.11+) swigs
Lgtm
On Mon, May 22, 2017 at 6:49 AM Pavel Labath via Phabricator <
revi...@reviews.llvm.org> wrote:
> labath created this revision.
>
> A change in swig 3.0.9 has caused it to generate modules incompatible
> with us using them as __init__.py (bug #769). Swig 3.0.11 adds a setting
> to help
> fix this problem, so use that. Support for older versions of swig remains
> unaffected.
>
>
> https://reviews.llvm.org/D33409
>
> Files:
> scripts/lldb.swig
>
>
> Index: scripts/lldb.swig
> ===
> --- scripts/lldb.swig
> +++ scripts/lldb.swig
> @@ -31,8 +31,24 @@
>and a source file location. SBCompileUnit contains SBLineEntry(s)."
> %enddef
>
> +/*
> +Since version 3.0.9, swig's logic for importing the native module has
> changed in
> +a way that is incompatible with our usage of the python module as
> __init__.py
> +(See swig bug #769). Fortunately, since version 3.0.11, swig provides a
> way for
> +us to override the module import logic to suit our needs. This does that.
> +
> +Older swig versions will simply ignore this setting.
> +*/
> +%define MODULEIMPORT
> +"import $module"
> +%enddef
> +// These versions will not generate working python modules, so error out
> early.
> +#if SWIG_VERSION >= 0x030009 && SWIG_VERSION < 0x030011
> +#error Swig versions 3.0.9 and 3.0.10 are incompatible with lldb.
> +#endif
> +
> // The name of the module to be created.
> -%module(docstring=DOCSTRING) lldb
> +%module(docstring=DOCSTRING, moduleimport=MODULEIMPORT) lldb
>
> // Parameter types will be used in the autodoc string.
> %feature("autodoc", "1");
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32930: New framework for lldb client-server communication tests.
jmajors updated this revision to Diff 99788.
jmajors marked 11 inline comments as done.
jmajors added a comment.
CR feedback.
https://reviews.llvm.org/D32930
Files:
unittests/CMakeLists.txt
unittests/tools/CMakeLists.txt
unittests/tools/lldb-server/CMakeLists.txt
unittests/tools/lldb-server/inferior/thread_inferior.cpp
unittests/tools/lldb-server/tests/CMakeLists.txt
unittests/tools/lldb-server/tests/MessageObjects.cpp
unittests/tools/lldb-server/tests/MessageObjects.h
unittests/tools/lldb-server/tests/TestClient.cpp
unittests/tools/lldb-server/tests/TestClient.h
unittests/tools/lldb-server/tests/ThreadIdsInJstopinfoTest.cpp
Index: unittests/tools/lldb-server/tests/ThreadIdsInJstopinfoTest.cpp
===
--- /dev/null
+++ unittests/tools/lldb-server/tests/ThreadIdsInJstopinfoTest.cpp
@@ -0,0 +1,58 @@
+//===-- ThreadsInJstopinfoTest.cpp --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "TestClient.h"
+#include "gtest/gtest.h"
+#include
+
+using namespace llgs_tests;
+
+class ThreadsInJstopinfoTest : public ::testing::Test {
+protected:
+ virtual void SetUp() { TestClient::Initialize(); }
+};
+
+TEST_F(ThreadsInJstopinfoTest, TestStopReplyContainsThreadPcsLlgs) {
+ std::vector inferior_args;
+ // This inferior spawns N threads, then forces a break.
+ inferior_args.push_back(THREAD_INFERIOR);
+ inferior_args.push_back("4");
+
+ auto test_info = ::testing::UnitTest::GetInstance()->current_test_info();
+
+ TestClient client(test_info->name(), test_info->test_case_name());
+ ASSERT_TRUE(client.StartDebugger());
+ ASSERT_TRUE(client.SetInferior(inferior_args));
+ ASSERT_TRUE(client.ListThreadsInStopReply());
+ ASSERT_TRUE(client.ContinueAll());
+ unsigned int pc_reg = client.GetPcRegisterId();
+ ASSERT_NE(pc_reg, UINT_MAX);
+
+ auto jthreads_info = client.GetJThreadsInfo();
+ ASSERT_TRUE(jthreads_info);
+
+ auto stop_reply = client.GetLatestStopReply();
+ auto stop_reply_pcs = stop_reply.GetThreadPcs();
+ auto thread_infos = jthreads_info->GetThreadInfos();
+ ASSERT_EQ(stop_reply_pcs.size(), thread_infos.size())
+ << "Thread count mismatch.";
+
+ for (auto stop_reply_pc : stop_reply_pcs) {
+unsigned long tid = stop_reply_pc.first;
+ASSERT_TRUE(thread_infos.find(tid) != thread_infos.end())
+<< "Thread ID: " << tid << " not in JThreadsInfo.";
+uint64_t pc_value;
+ASSERT_TRUE(thread_infos[tid].ReadRegisterAsUint64(pc_reg, pc_value))
+<< "Failure reading ThreadInfo register " << pc_reg;
+ASSERT_EQ(stop_reply_pcs[tid], pc_value)
+<< "Mismatched PC for thread: " << tid;
+ }
+
+ ASSERT_TRUE(client.StopDebugger());
+}
Index: unittests/tools/lldb-server/tests/TestClient.h
===
--- /dev/null
+++ unittests/tools/lldb-server/tests/TestClient.h
@@ -0,0 +1,61 @@
+//===-- TestClient.h *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "MessageObjects.h"
+#include "Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h"
+#include "lldb/Core/ArchSpec.h"
+#include "lldb/Target/ProcessLaunchInfo.h"
+#include "llvm/ADT/Optional.h"
+#include
+#include
+
+namespace llgs_tests {
+// TODO: Make the test client an abstract base class, with different children
+// for different types of connections: llgs v. debugserver
+class TestClient
+: public lldb_private::process_gdb_remote::GDBRemoteCommunicationClient {
+public:
+ static void Initialize();
+ TestClient(const std::string &test_name, const std::string &test_case_name);
+ virtual ~TestClient();
+ LLVM_NODISCARD bool StartDebugger();
+ LLVM_NODISCARD bool StopDebugger();
+ LLVM_NODISCARD bool SetInferior(llvm::ArrayRef inferior_args);
+ LLVM_NODISCARD bool ListThreadsInStopReply();
+ LLVM_NODISCARD bool SetBreakpoint(unsigned long address);
+ LLVM_NODISCARD bool ContinueAll();
+ LLVM_NODISCARD bool ContinueThread(unsigned long thread_id);
+ const ProcessInfo &GetProcessInfo();
+ llvm::Optional GetJThreadsInfo();
+ const StopReply &GetLatestStopReply();
+ LLVM_NODISCARD bool SendMessage(llvm::StringRef message);
+ LLVM_NODISCARD bool SendMessage(llvm::StringRef message,
+ std::string &response_string);
+ LLVM_NODISCARD bool SendMessage(llvm::StringRef message,
+ std::string &response_string,
+ PacketR
[Lldb-commits] [PATCH] D32930: New framework for lldb client-server communication tests.
jmajors added a comment. Addressed more feedback. https://reviews.llvm.org/D32930 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32585: Implementation of remote packets for Trace data.
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
Close, some minor fixes.
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:3293
+ if (custom_params_sp) {
+if (custom_params_sp->GetType() !=
StructuredData::Type::eTypeDictionary) {
+ error.SetErrorString("Invalid Configuration obtained");
Might be simpler to just do:
```
if (!custom_params_sp->GetAsDictionary())
```
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:188
+ StringExtractorGDBRemote::eServerPacketType_jTraceStart,
+ &GDBRemoteCommunicationServerLLGS::Handle_jTrace_start);
+ RegisterMemberFunctionHandler(
Rename to match previous inline comments
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:191
+ StringExtractorGDBRemote::eServerPacketType_jTraceBufferRead,
+ &GDBRemoteCommunicationServerLLGS::Handle_jTrace_read);
+ RegisterMemberFunctionHandler(
Rename to match previous inline comments
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:194
+ StringExtractorGDBRemote::eServerPacketType_jTraceMetaRead,
+ &GDBRemoteCommunicationServerLLGS::Handle_jTrace_read);
+ RegisterMemberFunctionHandler(
Rename to match previous inline comments
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:197
+ StringExtractorGDBRemote::eServerPacketType_jTraceStop,
+ &GDBRemoteCommunicationServerLLGS::Handle_jTrace_stop);
+ RegisterMemberFunctionHandler(
Rename to match previous inline comments
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:200
+ StringExtractorGDBRemote::eServerPacketType_jTraceConfigRead,
+ &GDBRemoteCommunicationServerLLGS::Handle_jTrace_conf_read);
+
Rename to match previous inline comments
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1141
+
+ StructuredData::ObjectSP custom_params_sp =
json_dict->GetValueForKey("jparams");
+ options.setTraceParams(static_pointer_cast
(custom_params_sp));
verify this is a dictionary first before the static cast below.
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h:192
+ PacketResult Handle_jTrace_start(StringExtractorGDBRemote &packet);
+
"Handle_jTraceStart" to make it match the packet name of "jTraceStart"
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h:194
+
+ PacketResult Handle_jTrace_read(StringExtractorGDBRemote &packet);
+
"Handle_jTraceRead"
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h:196
+
+ PacketResult Handle_jTrace_stop(StringExtractorGDBRemote &packet);
+
Handle_jTraceStop
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h:198
+
+ PacketResult Handle_jTrace_conf_read(StringExtractorGDBRemote &packet);
+
Handle_ jTraceConfigRead
Comment at:
unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp:400
+
+ HandlePacket(server, "jTraceStart:{\"buffersize\" : 8192,\"jparams\" :
{\"psb\" : 1,\"tracetech\" : \"intel-pt\"},\"metabuffersize\" :
8192,\"threadid\" : 35,\"type\" : 1}", "1");
+ ASSERT_EQ(result.get(),1);
Use the R"( so you don't have to desensitize the double quotes and so you can
format nicely:
```
const char *json = R"(
jTraceStart: {
"buffersize" : 8192,
"metabuffersize" : 8192,
"threadid" : 35,
"type" : 1,
"jparams" : {
"psb" : 1,
"tracetech" : "intel-pt"
}
})";
HandlePacket(server, json, "1");
```
Comment at:
unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp:409
+
+ HandlePacket(server, "jTraceStart:{\"buffersize\" : 8192,\"jparams\" :
{\"psb\" : 1,\"tracetech\" : \"intel-pt\"},\"metabuffersize\" :
8192,\"threadid\" : 35,\"type\" : 1}", "E23");
+ ASSERT_EQ(result.get(), LLDB_INVALID_UID);
Ditto
Comment at:
unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp:428
+
+ HandlePacket(server, "jTraceStop:{\"threadid\" : 35,\"traceid\" : 3}", "OK");
+ ASSERT_TRUE(result.get().Success());
Ditto, but inline the R"(...)"
Comment at:
unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp:435
+
+ HandlePacket(server, "jTraceStop:{\"threadid\" : 35,\"traceid\" : 3}",
"E23");
+ ASSERT_FALSE(result.get().Success());
Ditto, but inline the R"(...)"
==
[Lldb-commits] [PATCH] D32820: Parallelize demangling
scott.smith marked an inline comment as done. scott.smith added a comment. In https://reviews.llvm.org/D32820#759141, @emaste wrote: > > Without tcmalloc, on Ubuntu 14.04, 40 core VM: 13% > > With tcmalloc, on Ubuntu 14.04, 40 core VM: 24% (built using cmake ... > > -DCMAKE_EXE_LINKER_FLAGS=-ltcmalloc_minimal, which amazingly only works > > when building with clang, not gcc...) > > Do you have a brief set of steps you use for benchmarking? I'd like to > compare on FreeBSD using a similar test. time lldb -b -o 'b main' -o 'run' /my/program (sometimes I use 'perf stat' instead of time; I don't know if FreeBSD has something similar to Linux's perf - basically instruction count, cycle count, branch counts and mispredict rate, etc.) my program happens to have a lot of symbols and a lot of libraries. I tried this benchmark with lldb itself, and all but one of my changes have no effect because lldb only links in one large library, so YMMV depending on the application. Repository: rL LLVM https://reviews.llvm.org/D32820 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D33426: Introduce new command: thread unique-stacks
bgianfo created this revision.
This patch introduces a new thread command "unique-stacks".
The command is based off of "thread backtrace all" but will instead
find all threads which share matching call stacks and de-duplicate
their output, listing call stack and all the threads which share it.
This is especially useful for apps which use thread/task pools
sitting around waiting for work and cause excessive duplicate output.
I needed this behavior recently when debugging a core with 700+ threads.
https://reviews.llvm.org/D33426
Files:
include/lldb/Target/Thread.h
source/Commands/CommandObjectThread.cpp
source/Core/Debugger.cpp
source/Target/Thread.cpp
Index: source/Target/Thread.cpp
===
--- source/Target/Thread.cpp
+++ source/Target/Thread.cpp
@@ -1912,39 +1912,42 @@
size_t Thread::GetStatus(Stream &strm, uint32_t start_frame,
uint32_t num_frames, uint32_t num_frames_with_source,
- bool stop_format) {
- ExecutionContext exe_ctx(shared_from_this());
- Target *target = exe_ctx.GetTargetPtr();
- Process *process = exe_ctx.GetProcessPtr();
- size_t num_frames_shown = 0;
- strm.Indent();
- bool is_selected = false;
- if (process) {
-if (process->GetThreadList().GetSelectedThread().get() == this)
- is_selected = true;
- }
- strm.Printf("%c ", is_selected ? '*' : ' ');
- if (target && target->GetDebugger().GetUseExternalEditor()) {
-StackFrameSP frame_sp = GetStackFrameAtIndex(start_frame);
-if (frame_sp) {
- SymbolContext frame_sc(
- frame_sp->GetSymbolContext(eSymbolContextLineEntry));
- if (frame_sc.line_entry.line != 0 && frame_sc.line_entry.file) {
-Host::OpenFileInExternalEditor(frame_sc.line_entry.file,
- frame_sc.line_entry.line);
+ bool stop_format, bool only_stacks) {
+
+ if (!only_stacks) {
+ExecutionContext exe_ctx(shared_from_this());
+Target *target = exe_ctx.GetTargetPtr();
+Process *process = exe_ctx.GetProcessPtr();
+strm.Indent();
+bool is_selected = false;
+if (process) {
+ if (process->GetThreadList().GetSelectedThread().get() == this)
+is_selected = true;
+}
+strm.Printf("%c ", is_selected ? '*' : ' ');
+if (target && target->GetDebugger().GetUseExternalEditor()) {
+ StackFrameSP frame_sp = GetStackFrameAtIndex(start_frame);
+ if (frame_sp) {
+SymbolContext frame_sc(
+frame_sp->GetSymbolContext(eSymbolContextLineEntry));
+if (frame_sc.line_entry.line != 0 && frame_sc.line_entry.file) {
+ Host::OpenFileInExternalEditor(frame_sc.line_entry.file,
+ frame_sc.line_entry.line);
+}
}
}
- }
- DumpUsingSettingsFormat(strm, start_frame, stop_format);
+DumpUsingSettingsFormat(strm, start_frame, stop_format);
+ }
+ size_t num_frames_shown = 0;
if (num_frames > 0) {
strm.IndentMore();
const bool show_frame_info = true;
const char *selected_frame_marker = nullptr;
-if (num_frames == 1 ||
+if (num_frames == 1 || only_stacks ||
(GetID() != GetProcess()->GetThreadList().GetSelectedThread()->GetID()))
strm.IndentMore();
else
Index: source/Core/Debugger.cpp
===
--- source/Core/Debugger.cpp
+++ source/Core/Debugger.cpp
@@ -236,7 +236,7 @@
"when displaying thread information."},
{"thread-stop-format", OptionValue::eTypeFormatEntity, true, 0,
DEFAULT_THREAD_STOP_FORMAT, nullptr, "The default thread format "
- "string to usewhen displaying thread "
+ "string to use when displaying thread "
"information as part of the stop display."},
{"use-external-editor", OptionValue::eTypeBoolean, true, false, nullptr,
nullptr, "Whether to use an external editor or not."},
Index: source/Commands/CommandObjectThread.cpp
===
--- source/Commands/CommandObjectThread.cpp
+++ source/Commands/CommandObjectThread.cpp
@@ -42,7 +42,7 @@
using namespace lldb_private;
//-
-// CommandObjectThreadBacktrace
+// CommandObjectIterateOverThreads
//-
class CommandObjectIterateOverThreads : public CommandObjectParsed {
@@ -291,6 +291,122 @@
CommandOptions m_options;
};
+//-
+// CommandObjectUniqueThreadStacks
+//-
+
+class CommandObjectUniqueThreadStacks : public CommandObjectParsed {
+
+ class UniqueS
[Lldb-commits] [PATCH] D33426: Introduce new command: thread unique-stacks
jingham requested changes to this revision. jingham added a comment. This revision now requires changes to proceed. The idea is great. I think it is a little confusing that you would do: (lldb) thread backtrace all to get all threads but (lldb) thread unique-stacks to backtrace the unique stacks. Wouldn't it be more logical to do: (lldb) thread backtrace unique It's really just another specification of the threads you are backtracing. A few inline comments, we use '_' word separators for locals. Comment at: source/Commands/CommandObjectThread.cpp:344 + +std::vector uniqueStacks; +Process *process = m_exe_ctx.GetProcessPtr(); uniqueStacks -> unique_stacks Comment at: source/Commands/CommandObjectThread.cpp:384-385 +// Grab each frame's address +std::stack stackFrames; +const uint32_t frameCount = thread->GetStackFrameCount(); +for (uint32_t frameIndex = 0; frameIndex < frameCount; frameIndex++) We do local variables as: word_other_word, not camel-case. Comment at: source/Commands/CommandObjectThread.cpp:393 +// Try to match the threads stack to and existing thread. +bool foundMatch = false; +for (UniqueStack& uniqueStack : uniqueStacks) found_match https://reviews.llvm.org/D33426 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D33426: Introduce new command: thread unique-stacks
bgianfo updated this revision to Diff 99844.
bgianfo marked 3 inline comments as done.
bgianfo added a comment.
This iteration addresses Jim's feedback.
I've updated all of the code to match the surrounding style where
I missed it originally. (branches/variable names)
I also moved the command under "thread backtrace unique".
This required refactoring the thread iterator command object.
I've tried to reuse as much as code possible in the thread iterator
so that we actually use HandleOneThread to push the rending of
of the call stack down to the BackTrace command.
The bucketing has been moved directly into the command iterator
when he user passes in the "unique" option.
https://reviews.llvm.org/D33426
Files:
include/lldb/Target/Thread.h
source/Commands/CommandObjectThread.cpp
source/Core/Debugger.cpp
source/Target/Thread.cpp
Index: source/Target/Thread.cpp
===
--- source/Target/Thread.cpp
+++ source/Target/Thread.cpp
@@ -1912,39 +1912,42 @@
size_t Thread::GetStatus(Stream &strm, uint32_t start_frame,
uint32_t num_frames, uint32_t num_frames_with_source,
- bool stop_format) {
- ExecutionContext exe_ctx(shared_from_this());
- Target *target = exe_ctx.GetTargetPtr();
- Process *process = exe_ctx.GetProcessPtr();
- size_t num_frames_shown = 0;
- strm.Indent();
- bool is_selected = false;
- if (process) {
-if (process->GetThreadList().GetSelectedThread().get() == this)
- is_selected = true;
- }
- strm.Printf("%c ", is_selected ? '*' : ' ');
- if (target && target->GetDebugger().GetUseExternalEditor()) {
-StackFrameSP frame_sp = GetStackFrameAtIndex(start_frame);
-if (frame_sp) {
- SymbolContext frame_sc(
- frame_sp->GetSymbolContext(eSymbolContextLineEntry));
- if (frame_sc.line_entry.line != 0 && frame_sc.line_entry.file) {
-Host::OpenFileInExternalEditor(frame_sc.line_entry.file,
- frame_sc.line_entry.line);
+ bool stop_format, bool only_stacks) {
+
+ if (!only_stacks) {
+ExecutionContext exe_ctx(shared_from_this());
+Target *target = exe_ctx.GetTargetPtr();
+Process *process = exe_ctx.GetProcessPtr();
+strm.Indent();
+bool is_selected = false;
+if (process) {
+ if (process->GetThreadList().GetSelectedThread().get() == this)
+is_selected = true;
+}
+strm.Printf("%c ", is_selected ? '*' : ' ');
+if (target && target->GetDebugger().GetUseExternalEditor()) {
+ StackFrameSP frame_sp = GetStackFrameAtIndex(start_frame);
+ if (frame_sp) {
+SymbolContext frame_sc(
+frame_sp->GetSymbolContext(eSymbolContextLineEntry));
+if (frame_sc.line_entry.line != 0 && frame_sc.line_entry.file) {
+ Host::OpenFileInExternalEditor(frame_sc.line_entry.file,
+ frame_sc.line_entry.line);
+}
}
}
- }
- DumpUsingSettingsFormat(strm, start_frame, stop_format);
+DumpUsingSettingsFormat(strm, start_frame, stop_format);
+ }
+ size_t num_frames_shown = 0;
if (num_frames > 0) {
strm.IndentMore();
const bool show_frame_info = true;
const char *selected_frame_marker = nullptr;
-if (num_frames == 1 ||
+if (num_frames == 1 || only_stacks ||
(GetID() != GetProcess()->GetThreadList().GetSelectedThread()->GetID()))
strm.IndentMore();
else
Index: source/Core/Debugger.cpp
===
--- source/Core/Debugger.cpp
+++ source/Core/Debugger.cpp
@@ -236,7 +236,7 @@
"when displaying thread information."},
{"thread-stop-format", OptionValue::eTypeFormatEntity, true, 0,
DEFAULT_THREAD_STOP_FORMAT, nullptr, "The default thread format "
- "string to usewhen displaying thread "
+ "string to use when displaying thread "
"information as part of the stop display."},
{"use-external-editor", OptionValue::eTypeBoolean, true, false, nullptr,
nullptr, "Whether to use an external editor or not."},
Index: source/Commands/CommandObjectThread.cpp
===
--- source/Commands/CommandObjectThread.cpp
+++ source/Commands/CommandObjectThread.cpp
@@ -42,10 +42,40 @@
using namespace lldb_private;
//-
-// CommandObjectThreadBacktrace
+// CommandObjectIterateOverThreads
//-
class CommandObjectIterateOverThreads : public CommandObjectParsed {
+
+ class UniqueStack {
+
+ public:
+UniqueStack(std::stack stack_frames, uint32_t thread_index_id)
+ : m_stack_frames(stack_frames) {
+ m_t
