from:"Ravitheja Addepally via Phabricator via lldb\-commits"

[Lldb-commits] [PATCH] D29581: Initial implementation of SB APIs for Tracing support.

2017-04-03 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja updated this revision to Diff 93850.
ravitheja added a comment.

Changes for review.


https://reviews.llvm.org/D29581

Files:
  include/lldb/API/LLDB.h
  include/lldb/API/SBDefines.h
  include/lldb/API/SBError.h
  include/lldb/API/SBProcess.h
  include/lldb/API/SBStructuredData.h
  include/lldb/API/SBTrace.h
  include/lldb/API/SBTraceOptions.h
  include/lldb/Core/StructuredDataImpl.h
  include/lldb/Target/Process.h
  include/lldb/lldb-enumerations.h
  include/lldb/lldb-forward.h
  scripts/interface/SBProcess.i
  scripts/interface/SBStructuredData.i
  scripts/interface/SBTrace.i
  scripts/interface/SBTraceOptions.i
  scripts/lldb.swig
  source/API/CMakeLists.txt
  source/API/SBProcess.cpp
  source/API/SBStructuredData.cpp
  source/API/SBTrace.cpp
  source/API/SBTraceOptions.cpp

Index: source/API/SBTraceOptions.cpp
===
--- /dev/null
+++ source/API/SBTraceOptions.cpp
@@ -0,0 +1,99 @@
+//===-- SBTraceOptions.cpp --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "lldb/API/SBTraceOptions.h"
+#include "lldb/API/SBError.h"
+#include "lldb/API/SBStream.h"
+#include "lldb/API/SBStructuredData.h"
+#include "lldb/Core/Log.h"
+#include "lldb/Core/StructuredDataImpl.h"
+#include "lldb/Target/Process.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+SBTraceOptions::SBTraceOptions(lldb::TraceType type, uint64_t trace_buffer_size,
+   uint64_t meta_data_buffer_size) {
+  m_traceoptions_sp.reset(
+  new TraceOptions(type, trace_buffer_size, meta_data_buffer_size));
+}
+
+lldb::TraceType SBTraceOptions::getType() const {
+  if (m_traceoptions_sp)
+return m_traceoptions_sp->getType();
+  return lldb::TraceType::eTraceTypeNone;
+}
+
+uint64_t SBTraceOptions::getTraceBufferSize() const {
+  if (m_traceoptions_sp)
+return m_traceoptions_sp->getTraceBufferSize();
+  return 0;
+}
+
+lldb::SBStructuredData SBTraceOptions::getTraceParams(lldb::SBError &error) {
+  error.Clear();
+  const lldb_private::StructuredData::DictionarySP dict_obj =
+  m_traceoptions_sp->getTraceParams();
+  lldb::SBStructuredData structData;
+  StructuredDataImplSP struct_data = structData.GetSP();
+  if (dict_obj && struct_data)
+struct_data->SetObjectSP(dict_obj->shared_from_this());
+  else
+error.SetErrorString("Empty trace params");
+  return structData;
+}
+
+uint64_t SBTraceOptions::getMetaDataBufferSize() const {
+  if (m_traceoptions_sp)
+return m_traceoptions_sp->getTraceBufferSize();
+  return 0;
+}
+
+void SBTraceOptions::setTraceParams(lldb::SBStructuredData ¶ms) {
+  StructuredDataImplSP struct_data_impl = params.GetSP();
+  if (m_traceoptions_sp && struct_data_impl) {
+StructuredData::ObjectSP obj_sp = struct_data_impl->GetObjectSP();
+if (obj_sp && obj_sp->GetAsDictionary() != nullptr)
+  m_traceoptions_sp->setTraceParams(
+  std::static_pointer_cast(obj_sp));
+  }
+  return;
+}
+
+void SBTraceOptions::setType(lldb::TraceType type) {
+  if (m_traceoptions_sp)
+m_traceoptions_sp->setType(type);
+}
+
+void SBTraceOptions::setTraceBufferSize(uint64_t size) {
+  if (m_traceoptions_sp)
+m_traceoptions_sp->setTraceBufferSize(size);
+}
+
+void SBTraceOptions::setMetaDataBufferSize(uint64_t size) {
+  if (m_traceoptions_sp)
+m_traceoptions_sp->setMetaDataBufferSize(size);
+}
+
+bool SBTraceOptions::IsValid() {
+  if (m_traceoptions_sp)
+return true;
+  return false;
+}
+
+void SBTraceOptions::setThreadID(lldb::tid_t thread_id) {
+  if (m_traceoptions_sp)
+m_traceoptions_sp->setThreadID(thread_id);
+}
+
+lldb::tid_t SBTraceOptions::getThreadID() {
+  if (m_traceoptions_sp)
+return m_traceoptions_sp->getThreadID();
+  return LLDB_INVALID_THREAD_ID;
+}
Index: source/API/SBTrace.cpp
===
--- /dev/null
+++ source/API/SBTrace.cpp
@@ -0,0 +1,109 @@
+//===-- SBTrace.cpp -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "lldb/Core/Log.h"
+#include "lldb/Target/Process.h"
+
+#include "lldb/API/SBTrace.h"
+#include "lldb/API/SBTraceOptions.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+class TraceImpl {
+public:
+  lldb::user_id_t uid;
+};
+
+lldb::ProcessSP SBTrace::GetSP() const { return m_opaque_wp.lock(); }
+
+size_t SBTrace::GetTraceData(SBError &error, void *buf, size_t size,
+ size_t offset, lldb::tid_t

[Lldb-commits] [PATCH] D29581: Initial implementation of SB APIs for Tracing support.

2017-04-20 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja updated this revision to Diff 95930.
ravitheja added a comment.

New diff with requested changes.


https://reviews.llvm.org/D29581

Files:
  include/lldb/API/LLDB.h
  include/lldb/API/SBDefines.h
  include/lldb/API/SBError.h
  include/lldb/API/SBProcess.h
  include/lldb/API/SBStructuredData.h
  include/lldb/API/SBTrace.h
  include/lldb/API/SBTraceOptions.h
  include/lldb/Core/StructuredDataImpl.h
  include/lldb/Core/TraceOptions.h
  include/lldb/Target/Process.h
  include/lldb/lldb-enumerations.h
  include/lldb/lldb-forward.h
  scripts/interface/SBProcess.i
  scripts/interface/SBStructuredData.i
  scripts/interface/SBTrace.i
  scripts/interface/SBTraceOptions.i
  scripts/lldb.swig
  source/API/CMakeLists.txt
  source/API/SBProcess.cpp
  source/API/SBStructuredData.cpp
  source/API/SBTrace.cpp
  source/API/SBTraceOptions.cpp

Index: source/API/SBTraceOptions.cpp
===
--- /dev/null
+++ source/API/SBTraceOptions.cpp
@@ -0,0 +1,94 @@
+//===-- SBTraceOptions.cpp --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "lldb/API/SBTraceOptions.h"
+#include "lldb/API/SBError.h"
+#include "lldb/API/SBStructuredData.h"
+#include "lldb/Core/Log.h"
+#include "lldb/Core/StructuredDataImpl.h"
+#include "lldb/Core/TraceOptions.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+SBTraceOptions::SBTraceOptions() {
+  m_traceoptions_sp.reset(new TraceOptions());
+}
+
+lldb::TraceType SBTraceOptions::getType() const {
+  if (m_traceoptions_sp)
+return m_traceoptions_sp->getType();
+  return lldb::TraceType::eTraceTypeNone;
+}
+
+uint64_t SBTraceOptions::getTraceBufferSize() const {
+  if (m_traceoptions_sp)
+return m_traceoptions_sp->getTraceBufferSize();
+  return 0;
+}
+
+lldb::SBStructuredData SBTraceOptions::getTraceParams(lldb::SBError &error) {
+  error.Clear();
+  const lldb_private::StructuredData::DictionarySP dict_obj =
+  m_traceoptions_sp->getTraceParams();
+  lldb::SBStructuredData structData;
+  if (dict_obj && structData.m_impl_up)
+structData.m_impl_up->SetObjectSP(dict_obj->shared_from_this());
+  else
+error.SetErrorString("Empty trace params");
+  return structData;
+}
+
+uint64_t SBTraceOptions::getMetaDataBufferSize() const {
+  if (m_traceoptions_sp)
+return m_traceoptions_sp->getTraceBufferSize();
+  return 0;
+}
+
+void SBTraceOptions::setTraceParams(lldb::SBStructuredData ¶ms) {
+  if (m_traceoptions_sp && params.m_impl_up) {
+StructuredData::ObjectSP obj_sp = params.m_impl_up->GetObjectSP();
+if (obj_sp && obj_sp->GetAsDictionary() != nullptr)
+  m_traceoptions_sp->setTraceParams(
+  std::static_pointer_cast(obj_sp));
+  }
+  return;
+}
+
+void SBTraceOptions::setType(lldb::TraceType type) {
+  if (m_traceoptions_sp)
+m_traceoptions_sp->setType(type);
+}
+
+void SBTraceOptions::setTraceBufferSize(uint64_t size) {
+  if (m_traceoptions_sp)
+m_traceoptions_sp->setTraceBufferSize(size);
+}
+
+void SBTraceOptions::setMetaDataBufferSize(uint64_t size) {
+  if (m_traceoptions_sp)
+m_traceoptions_sp->setMetaDataBufferSize(size);
+}
+
+bool SBTraceOptions::IsValid() {
+  if (m_traceoptions_sp)
+return true;
+  return false;
+}
+
+void SBTraceOptions::setThreadID(lldb::tid_t thread_id) {
+  if (m_traceoptions_sp)
+m_traceoptions_sp->setThreadID(thread_id);
+}
+
+lldb::tid_t SBTraceOptions::getThreadID() {
+  if (m_traceoptions_sp)
+return m_traceoptions_sp->getThreadID();
+  return LLDB_INVALID_THREAD_ID;
+}
Index: source/API/SBTrace.cpp
===
--- /dev/null
+++ source/API/SBTrace.cpp
@@ -0,0 +1,109 @@
+//===-- SBTrace.cpp -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "lldb/Core/Log.h"
+#include "lldb/Target/Process.h"
+
+#include "lldb/API/SBTrace.h"
+#include "lldb/API/SBTraceOptions.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+class TraceImpl {
+public:
+  lldb::user_id_t uid;
+};
+
+lldb::ProcessSP SBTrace::GetSP() const { return m_opaque_wp.lock(); }
+
+size_t SBTrace::GetTraceData(SBError &error, void *buf, size_t size,
+ size_t offset, lldb::tid_t thread_id) {
+  size_t bytes_read = 0;
+  ProcessSP process_sp(GetSP());
+  Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));
+  error.Clear();
+
+  if (!process_sp) {
+error.SetErrorString("invalid process");
+  } else {
+

[Lldb-commits] [PATCH] D29581: Initial implementation of SB APIs for Tracing support.

2017-04-20 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja marked 7 inline comments as done.
ravitheja added inline comments.



Comment at: include/lldb/API/SBProcess.h:251
+  /// supported. To obtain the actual used trace options please
+  /// use the GetTraceConfig API. For the custom parameters, only
+  /// the parameters recognized by the target would be used and

clayborg wrote:
> Is GetTraceConfig an internal API? If so, lets not mention it here. You 
> general comments should suffice.
No, its not an internal API, so just to throw some light to it, Processor trace 
integration for Linux does not support all buffer sizes so we are rounding the 
buffer sizes to the nearest supported buffer size and through GetTraceConfig we 
plan to communicate the actual trace buffer size being.


https://reviews.llvm.org/D29581



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

[Lldb-commits] [PATCH] D29581: Initial implementation of SB APIs for Tracing support.

2017-04-26 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja updated this revision to Diff 96682.
ravitheja added a comment.

Fixing few header file inclusions.


https://reviews.llvm.org/D29581

Files:
  include/lldb/API/LLDB.h
  include/lldb/API/SBDefines.h
  include/lldb/API/SBError.h
  include/lldb/API/SBProcess.h
  include/lldb/API/SBStructuredData.h
  include/lldb/API/SBTrace.h
  include/lldb/API/SBTraceOptions.h
  include/lldb/Core/StructuredDataImpl.h
  include/lldb/Core/TraceOptions.h
  include/lldb/Target/Process.h
  include/lldb/lldb-enumerations.h
  include/lldb/lldb-forward.h
  scripts/interface/SBProcess.i
  scripts/interface/SBStructuredData.i
  scripts/interface/SBTrace.i
  scripts/interface/SBTraceOptions.i
  scripts/lldb.swig
  source/API/CMakeLists.txt
  source/API/SBProcess.cpp
  source/API/SBStructuredData.cpp
  source/API/SBTrace.cpp
  source/API/SBTraceOptions.cpp

Index: source/API/SBTraceOptions.cpp
===
--- /dev/null
+++ source/API/SBTraceOptions.cpp
@@ -0,0 +1,94 @@
+//===-- SBTraceOptions.cpp --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "lldb/API/SBTraceOptions.h"
+#include "lldb/API/SBError.h"
+#include "lldb/API/SBStructuredData.h"
+#include "lldb/Utility/Log.h"
+#include "lldb/Core/StructuredDataImpl.h"
+#include "lldb/Core/TraceOptions.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+SBTraceOptions::SBTraceOptions() {
+  m_traceoptions_sp.reset(new TraceOptions());
+}
+
+lldb::TraceType SBTraceOptions::getType() const {
+  if (m_traceoptions_sp)
+return m_traceoptions_sp->getType();
+  return lldb::TraceType::eTraceTypeNone;
+}
+
+uint64_t SBTraceOptions::getTraceBufferSize() const {
+  if (m_traceoptions_sp)
+return m_traceoptions_sp->getTraceBufferSize();
+  return 0;
+}
+
+lldb::SBStructuredData SBTraceOptions::getTraceParams(lldb::SBError &error) {
+  error.Clear();
+  const lldb_private::StructuredData::DictionarySP dict_obj =
+  m_traceoptions_sp->getTraceParams();
+  lldb::SBStructuredData structData;
+  if (dict_obj && structData.m_impl_up)
+structData.m_impl_up->SetObjectSP(dict_obj->shared_from_this());
+  else
+error.SetErrorString("Empty trace params");
+  return structData;
+}
+
+uint64_t SBTraceOptions::getMetaDataBufferSize() const {
+  if (m_traceoptions_sp)
+return m_traceoptions_sp->getTraceBufferSize();
+  return 0;
+}
+
+void SBTraceOptions::setTraceParams(lldb::SBStructuredData ¶ms) {
+  if (m_traceoptions_sp && params.m_impl_up) {
+StructuredData::ObjectSP obj_sp = params.m_impl_up->GetObjectSP();
+if (obj_sp && obj_sp->GetAsDictionary() != nullptr)
+  m_traceoptions_sp->setTraceParams(
+  std::static_pointer_cast(obj_sp));
+  }
+  return;
+}
+
+void SBTraceOptions::setType(lldb::TraceType type) {
+  if (m_traceoptions_sp)
+m_traceoptions_sp->setType(type);
+}
+
+void SBTraceOptions::setTraceBufferSize(uint64_t size) {
+  if (m_traceoptions_sp)
+m_traceoptions_sp->setTraceBufferSize(size);
+}
+
+void SBTraceOptions::setMetaDataBufferSize(uint64_t size) {
+  if (m_traceoptions_sp)
+m_traceoptions_sp->setMetaDataBufferSize(size);
+}
+
+bool SBTraceOptions::IsValid() {
+  if (m_traceoptions_sp)
+return true;
+  return false;
+}
+
+void SBTraceOptions::setThreadID(lldb::tid_t thread_id) {
+  if (m_traceoptions_sp)
+m_traceoptions_sp->setThreadID(thread_id);
+}
+
+lldb::tid_t SBTraceOptions::getThreadID() {
+  if (m_traceoptions_sp)
+return m_traceoptions_sp->getThreadID();
+  return LLDB_INVALID_THREAD_ID;
+}
Index: source/API/SBTrace.cpp
===
--- /dev/null
+++ source/API/SBTrace.cpp
@@ -0,0 +1,109 @@
+//===-- SBTrace.cpp -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "lldb/Utility/Log.h"
+#include "lldb/Target/Process.h"
+
+#include "lldb/API/SBTrace.h"
+#include "lldb/API/SBTraceOptions.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+class TraceImpl {
+public:
+  lldb::user_id_t uid;
+};
+
+lldb::ProcessSP SBTrace::GetSP() const { return m_opaque_wp.lock(); }
+
+size_t SBTrace::GetTraceData(SBError &error, void *buf, size_t size,
+ size_t offset, lldb::tid_t thread_id) {
+  size_t bytes_read = 0;
+  ProcessSP process_sp(GetSP());
+  Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));
+  error.Clear();
+
+  if (!process_sp) {
+error.SetErrorString("invalid process");
+  } e

[Lldb-commits] [PATCH] D32585: Implementation of remote packets for Trace data.

2017-04-28 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja added inline comments.



Comment at: docs/lldb-gdb-remote.txt:212
 //--
+// QTrace:1:type:;
+//

clayborg wrote:
> Should we make all these new packets JSON based to start with? "jTrace"? If 
> we have any need for JSON at all in this or the other new packets I would say 
> lets just go with JSON packets. They are currently prefixed with "j". If we 
> go this route we should specify the mandatory key/value pairs in the header 
> doc. We should also allow a JSON dictionary from the trace config up at the 
> SBTrace layer to make it into this packet? 
I am fine prefixing the packets with "j", I did not understand what you meant 
by the last sentence. At the moment I am waiting for Pavel or Tamas to reply or 
anyone else and based on the complete feedback, I will upload a new patch if 
needed.


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] D32585: Implementation of remote packets for Trace data.

2017-05-11 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja updated this revision to Diff 98599.
ravitheja added a comment.

Changes for the feedback recieved in first round of review.


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/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

Index: source/Utility/StringExtractorGDBRemote.h
===
--- source/Utility/StringExtractorGDBRemote.h
+++ source/Utility/StringExtractorGDBRemote.h
@@ -164,6 +164,12 @@
 eServerPacketType__M,
 eServerPacketType__m,
 eServerPacketType_notify, // '%' notification
+
+eServerPacketType_JTrace_Start,
+eServerPacketType_jTraceBufferRead,
+eServerPacketType_jTraceMetaRead,
+eServerPacketType_JTrace_Stop,
+eServerPacketType_jTraceConfigRead,
   };
 
   ServerPacketType GetServerPacketType() const;
Index: source/Utility/StringExtractorGDBRemote.cpp
===
--- source/Utility/StringExtractorGDBRemote.cpp
+++ source/Utility/StringExtractorGDBRemote.cpp
@@ -279,13 +279,26 @@
 }
 break;
 
+  case 'J':
+if (PACKET_STARTS_WITH("JTrace:start:"))
+  return eServerPacketType_JTrace_Start;
+if (PACKET_STARTS_WITH("JTrace:stop:"))
+  return eServerPacketType_JTrace_Stop;
+break;
+
   case 'j':
 if (PACKET_STARTS_WITH("jModulesInfo:"))
   return eServerPacketType_jModulesInfo;
 if (PACKET_MATCHES("jSignalsInfo"))
   return eServerPacketType_jSignalsInfo;
 if (PACKET_MATCHES("jThreadsInfo"))
   return eServerPacketType_jThreadsInfo;
+if (PACKET_STARTS_WITH("jTrace:buffer:read:"))
+  return eServerPacketType_jTraceBufferRead;
+if (PACKET_STARTS_WITH("jTrace:conf:read:"))
+  return eServerPacketType_jTraceConfigRead;
+if (PACKET_STARTS_WITH("jTrace:meta:read:"))
+  return eServerPacketType_jTraceMetaRead;
 break;
 
   case 'v':
Index: source/Utility/StringExtractor.cpp
===
--- source/Utility/StringExtractor.cpp
+++ source/Utility/StringExtractor.cpp
@@ -280,6 +280,15 @@
   return result;
 }
 
+bool StringExtractor::Consume_front(const llvm::StringRef &str) {
+  llvm::StringRef S = GetStringRef();
+  if (!S.startswith(str))
+return false;
+  else
+m_index += str.size();
+  return true;
+}
+
 size_t StringExtractor::GetHexBytes(llvm::MutableArrayRef dest,
 uint8_t fail_fill_value) {
   size_t bytes_extracted = 0;
Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
===
--- source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
+++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
@@ -175,6 +175,21 @@
 
   Error DisableWatchpoint(Watchpoint *wp, bool notify = true) override;
 
+  lldb::user_id_t StartTrace(const TraceOptions &options,
+ Error &error) override;
+
+  Error StopTrace(lldb::user_id_t uid, lldb::tid_t thread_id) override;
+
+  Error GetData(lldb::user_id_t uid, lldb::tid_t thread_id,
+llvm::MutableArrayRef &buffer,
+size_t offset = 0) override;
+
+  Error GetMetaData(lldb::user_id_t uid, lldb::tid_t thread_id,
+llvm::MutableArrayRef &buffer,
+size_t offset = 0) override;
+
+  Error GetTraceConfig(lldb::user_id_t uid, TraceOptions &options) override;
+
   Error GetWatchpointSupportInfo(uint32_t &num) override;
 
   Error GetWatchpointSupportInfo(uint32_t &num, bool &after) override;
@@ -407,6 +422,10 @@
   std::map m_thread_id_to_used_usec_map;
   uint64_t m_last_signals_version = 0;
 
+  Error GetTraceData(StreamString &packet, lldb::user_id_t uid,
+ lldb::tid_t thread_id,
+ llvm::MutableArrayRef &buffer, size_t offset);
+
   static bool NewThreadNotifyBreakpointHit(void *baton,
StoppointCallbackContext *context,
lldb::user_id_t break_id,
Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -65,6 +65,7 @@
 #include "lldb/Target/ThreadPlanCallFunction.h"
 #include "lldb/Utility/CleanUp.h

[Lldb-commits] [PATCH] D32585: Implementation of remote packets for Trace data.

2017-05-11 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja marked 28 inline comments as done.
ravitheja added inline comments.



Comment at: docs/lldb-gdb-remote.txt:212
 //--
+// QTrace:1:type:;
+//

clayborg wrote:
> labath wrote:
> > ravitheja wrote:
> > > clayborg wrote:
> > > > Should we make all these new packets JSON based to start with? 
> > > > "jTrace"? If we have any need for JSON at all in this or the other new 
> > > > packets I would say lets just go with JSON packets. They are currently 
> > > > prefixed with "j". If we go this route we should specify the mandatory 
> > > > key/value pairs in the header doc. We should also allow a JSON 
> > > > dictionary from the trace config up at the SBTrace layer to make it 
> > > > into this packet? 
> > > I am fine prefixing the packets with "j", I did not understand what you 
> > > meant by the last sentence. At the moment I am waiting for Pavel or Tamas 
> > > to reply or anyone else and based on the complete feedback, I will upload 
> > > a new patch if needed.
> > I think Greg meant you should also change the packet format to take a json 
> > argument instead of key-value pairs (The packet should be JTrace if it 
> > modifies the state, right ?).
> > 
> > If there is a gdb equivalent which matches this functionality sufficiently 
> > closely, then I would prefer to stick to that. However, given that we 
> > already are attaching a json field to the packet, I think it would make 
> > sense to go json all the way.
> I think the entire packet should be:
> ```
> $jTrace:
> ```
> where  is a JSON dictionary. Ravitheja had talked before about adding an 
> implementation specific JSON dictionary into the JSON that comes in to 
> configure the trace at the lldb::SB layer. I was just saying it would be nice 
> if this implementation specific JSON was sent down as part of this packet so 
> the GDB server can use it. Maybe we just send the entire thing that came in 
> at the SB layer? 
Yes thats what is currently being done, the JSON Dictionary obtained at SB 
layer is being passed down to the server through the remote packets.



Comment at: include/lldb/Host/common/NativeProcessProtocol.h:13
 
+#include "lldb/Core/TraceOptions.h"
 #include "lldb/Host/MainLoop.h"

labath wrote:
> You are missing the traceOptions file from this patch.
The traceOptions are already merged into lldb code, please refer to 
->https://reviews.llvm.org/D29581



Comment at: include/lldb/Host/common/NativeProcessProtocol.h:429-430
+  //--
+  virtual void GetTraceConfig(lldb::user_id_t uid, lldb::tid_t threadid,
+  Error &error, TraceOptions &config) {
+error.SetErrorString("Not implemented");

zturner wrote:
> `virtual Error GetTraceConfig(user_id_t uid, tid_ threadid, TraceOptions 
> &config)`
The threadid is already inside the TraceOptions, which needs to be set by the 
user, I will update the comments here.



Comment at: 
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1161
+  if (buffersize == std::numeric_limits::max() ||
+  packet.GetBytesLeft() != 0) {
+

zturner wrote:
> Can you use `!packet.empty()`
I think its not possible to use empty function coz it checks the size of 
m_packet string inside and not the current m_index.



Comment at: 
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1391
+
+  if (error_encountered || packet.GetBytesLeft() != 0 ||
+  byte_count == std::numeric_limits::max() ||

zturner wrote:
> `!packet.empty()`
Not possible I think



Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.h:178
 
+  virtual lldb::user_id_t StartTrace(lldb::TraceOptionsSP &options,
+ Error &error) override;

labath wrote:
> `const TraceOptions &options` ? Having a reference to a shared pointer seems 
> like a huge overkill here.
Yes u are right my mistake.


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] D32585: Implementation of remote packets for Trace data.

2017-05-11 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja marked 2 inline comments as done.
ravitheja added a comment.

In https://reviews.llvm.org/D32585#740632, @labath wrote:

> I quite like that you have added just the packet plumbing code without an 
> concrete implementation. However, that is still a significant amount of 
> parsing code that should be accompanied by a test. The test suite for the 
> client side of the protocol is ready (TestGdbRemoteCommunicationClient), so 
> I'd like to see at least that.

@labath  I was considering writing Unit tests for the remote packets but I 
thought then I have to write the mock implementation for the trace operations 
as well, which might end up being a bigger piece of code than the actual packet 
parsing code.

After this patch, I will upload the actual server side code doing the trace 
stuff for linux, that part of code has unit tests for some core functions.

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] D32585: Implementation of remote packets for Trace data.

2017-05-18 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja added a comment.

>   TraceOptions opt;
>   opt.setType(...);
>   opt.setBufferSize(...);
>   opt.setMetaBufferSize(); // with an appropriate TraceOptions constructor, 
> this could be a one-liner
> std::future result = std::async(std::launch::async, [&] {
>   return client.StartTrace(opt, error);
> });
>   HandlePacket(server, 
> "JTrace:start:type:dead:buffersize:beef:metabuffersize:baad:threadid:f00d:jparams:...",
>  "47");
>   ASSERT_EQ(47, result.get());
>   ASSERT_TRUE(error.Success());
> 
> 
> This depends on the packet code being in GdbRemoteCommunicationClient and not 
> ProcessGdbRemote as it is now, but moving it there is a good idea anyway -- 
> ProcessGdbRemote should do higher level stuff, packet parsing should be left 
> in the GDBRCClient.
> 
> The server side code unfortunately cannot be tested this way, but I believe I 
> will get around to enabling that soon as well.

@labath Can I do the unit tests in a seperate patch after this patch ? It would 
be easier for me and perhaps also easier to review.


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] D32585: Implementation of remote packets for Trace data.

2017-05-18 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja added a comment.

Well nothings preventing me from doing so, I have the changes for that were 
suggested to me for this revision. I thought I would first upload them, so that 
people can look at the parallel while I upload the linux server code and Unit 
tests.


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] D32585: Implementation of remote packets for Trace data.

2017-05-22 Thread Ravitheja Addepally via Phabricator via lldb-commits

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.

2017-05-22 Thread Ravitheja Addepally via Phabricator via lldb-commits

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] D32585: Implementation of remote packets for Trace data.

2017-05-22 Thread Ravitheja Addepally via Phabricator via lldb-commits

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

[Lldb-commits] [PATCH] D32585: Implementation of remote packets for Trace data.

2017-05-23 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja added inline comments.



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");

clayborg wrote:
> Might be simpler to just do:
> 
> ```
> if (!custom_params_sp->GetAsDictionary())
> ```
> 
Yes that could be dangerous to do, coz it returns a pointer to the Dictionary , 
while setTraceParams needs a shared_pointer and I would have a to create a 
shared_pointer which is not really being shared.



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);

clayborg wrote:
> 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");
> ```
That won't match coz the new lines and extra spaces cause mismatch in the 
packet string produced. Also unfortunately the Dump Function in the 
StrucuturedData::Dictionary adds spaces around the colons between "Key" : Value 
, so removing all whitespaces won't work. Now I can still add some functions to 
format the expexted string according to the one printed by Dump function, but 
that would be more code.


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] D32585: Implementation of remote packets for Trace data.

2017-05-23 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja updated this revision to Diff 99870.
ravitheja added a comment.

Updates from last review.


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,\"metabuffersize\" : 8192,\"params\" : {\"psb\" : 1,\"tracetech\" : \"intel-pt\"},\"threadid\" : 35,\"type\" : 1}", "1");
+  ASSERT_TRUE(error.Success());
+  ASSERT_EQ(result.get(),1);
+
+  error.Clear();
+  result = std::async(std::launch::async, [&] {
+return client.SendStartTracePacket(options, error);
+  });
+
+  HandlePacket(server, "jTraceStart:{\"buffersize\" : 8192,\"metabuffersize\" : 8192,\"params\" : {\"psb\" : 1,\"tracetech\" : \"intel-pt\"},\"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 client;
+  MockServer server;
+

[Lldb-commits] [PATCH] D32585: Implementation of remote packets for Trace data.

2017-05-23 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja added inline comments.



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);

labath wrote:
> ravitheja wrote:
> > clayborg wrote:
> > > 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");
> > > ```
> > That won't match coz the new lines and extra spaces cause mismatch in the 
> > packet string produced. Also unfortunately the Dump Function in the 
> > StrucuturedData::Dictionary adds spaces around the colons between "Key" : 
> > Value , so removing all whitespaces won't work. Now I can still add some 
> > functions to format the expexted string according to the one printed by 
> > Dump function, but that would be more code.
> I've been wanting to add make these tests smarter now that they are getting 
> more use. I'll take a todo item here to add a smarter way to compare json 
> fields here. For now you can at least do the R" thing Greg suggests.
Yes I can do the R thing but then i can't format it nicely, as Greg suggested.


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] D32585: Implementation of remote packets for Trace data.

2017-05-23 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja updated this revision to Diff 99891.
ravitheja added a comment.

Modifying string literals in Unit tests.


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,205 @@
   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);
+  });
+
+  const char *expected_packet = R"(jTraceStart:{"buffersize" : 8192,"metabuffersize" : 8192,"params" : {"psb" : 1,"tracetech" : "intel-pt"},"threadid" : 35,"type" : 1})";
+  HandlePacket(server, expected_packet, "1");
+  ASSERT_TRUE(error.Success());
+  ASSERT_EQ(result.get(),1);
+
+  error.Clear();
+  result = std::async(std::launch::async, [&] {
+return client.SendStartTracePacket(options, error);
+  });
+
+  HandlePacket(server, expected_packet, "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);
+  });
+
+  const char *expected_packet = R"(jTraceStop:{"threadid" : 35,"traceid" : 3})";
+  HandlePacket(server, expected_packet, "OK");
+  ASSERT_TRUE(result.get().Success());
+
+  result = std::async(std::launch::async, [&] {
+return client.SendStopTracePacket(trace_id, thread_id);
+  });
+
+  HandlePacket(server, expected_packet, "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);
+  });
+
+  const char *expected_packet = R"(jTraceBufferRead:{"buffersize" : 32,"offset" : 0,"threadid" : 35,"traceid" : 3})";
+  HandlePacket(server, expected_packet, "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_packet, "E23");
+  ASSERT_FALSE(result.get().Success());
+  ASSERT_EQ(buffer2.size(), 0);
+}
+
+TEST_F(GDBRemoteCommunicationClientTest, SendGetMetaDataPacket) {
+  TestClient client;
+  MockServer server;
+  Connect(client, server);
+  if (HasFailure())
+

[Lldb-commits] [PATCH] D32585: Implementation of remote packets for Trace data.

2017-05-24 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja updated this revision to Diff 100051.
ravitheja added a comment.

Running clang-format.


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
@@ -13,6 +13,7 @@
 #include "Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/StructuredData.h"
+#include "lldb/Core/TraceOptions.h"
 #include "lldb/Target/MemoryRegionInfo.h"
 #include "lldb/Utility/DataBuffer.h"
 
@@ -370,3 +371,214 @@
   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);
+  });
+
+  const char *expected_packet =
+  R"(jTraceStart:{"buffersize" : 8192,"metabuffersize" : 8192,"params" : {"psb" : 1,"tracetech" : "intel-pt"},"threadid" : 35,"type" : 1})";
+  HandlePacket(server, expected_packet, "1");
+  ASSERT_TRUE(error.Success());
+  ASSERT_EQ(result.get(), 1);
+
+  error.Clear();
+  result = std::async(std::launch::async, [&] {
+return client.SendStartTracePacket(options, error);
+  });
+
+  HandlePacket(server, expected_packet, "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);
+  });
+
+  const char *expected_packet = R"(jTraceStop:{"threadid" : 35,"traceid" : 3})";
+  HandlePacket(server, expected_packet, "OK");
+  ASSERT_TRUE(result.get().Success());
+
+  result = std::async(std::launch::async, [&] {
+return client.SendStopTracePacket(trace_id, thread_id);
+  });
+
+  HandlePacket(server, expected_packet, "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);
+  });
+
+  const char *expected_packet =
+  R"(jTraceBufferRead:{"buffersize" : 32,"offset" : 0,"threadid" : 35,"traceid" : 3})";
+  HandlePacket(server, expected_packet, "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_packet, "E23");
+  ASSERT_FALSE(result.get().Success());
+  ASSERT_EQ(buffer2.size(), 0);
+}
+
+TEST_F(GDBRemoteCommunicationClientTest, SendGetMetaDataPacket) {
+  TestClient client;
+  MockServer server;
+  Connect

[Lldb-commits] [PATCH] D32585: Implementation of remote packets for Trace data.

2017-05-29 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja added a comment.

Hello, the reason I switched to allocating with new (std::nothrow) 
uint8_t[byte_count]; was because the byte_count is actually user defined and I 
do want to catch cases where lldb is not able to allocate the requested buffer 
size.


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] D33674: Implementation of Intel(R) Processor Trace support for Linux

2017-06-07 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja added inline comments.



Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:156
+  // -
+  static size_t ReadCyclicBuffer(void *buf, size_t buf_size, void *cyc_buf,
+ size_t cyc_buf_size, size_t cyc_start,

zturner wrote:
> labath wrote:
> > How about ` void ReadCyclicBuffer(ArrayRef buffer, size_t 
> > position, MutableArrayRef &dest)`
> Better yet, drop the `position` argument entirely.  `ReadCyclicBuffer(src, N, 
> dest)` is equivalent to `ReadCyclicBuffer(src.drop_front(N), dest);`
So there are couple of things i would like to mention ->
1) The data and aux buffers allocated are cyclic in nature, so we need to 
consider the cyc_start or starting index in order to correctly read them.
2) The function ReadCyclicBuffer is very generic and is capable of reading a 
certain chunk of memory starting from a certain offset.
3) Now I could have kept this function in the private domain so that the 
cyc_start need not be passed everytime, but then I also wanted to unit test 
this function.

Now keeping these in mind my questions are the following ->
@zturner @labath  I need the offset and cyc_start , both are required so the 
position argument won't suffice ?

Please correct me if i am wrong.



Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:160
+  enum PTErrorCode {
+FileNotFound = 0x23,
+ThreadNotTraced,

labath wrote:
> This seems suspicious. How did you come to choose this number?
Sometime ago I asked in the devlist about the error codes in lldb and got the 
answer that they were completely arbitrary, so 0x23 has no special meaning. The 
question that I would like to ask is do u think these error codes should be 
here ? coz in https://reviews.llvm.org/D33035 it was suggested by @clayborg 
that tools working on top of the SB API's should only receive Error Strings and 
not codes.

Currently anyone who would encounter these codes would have to refer here for 
their meaning. Also the remote packets don't allow me to send Error Strings 
instead of error codes.



Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:319-320
+int m_fd;
+void *m_mmap_data;
+void *m_mmap_aux;
+void *m_mmap_base;

zturner wrote:
> Instead of having 
> ```
> void *m_mmap_data;
> void *m_mmap_aux;
> void *m_mmap_base;
> ```
> 
> and then doing some ugly casting every time someone calls `getAuxBufferSize` 
> or `getDataBufferSize`, instead just write:
> 
> ```
> MutableArrayRef m_mmap_data;
> MutableArrayRef m_mmap_aux;
> ```
> 
> and initializing these array refs up front using the size from the header.  
> This way you don't have to worry about anyone using the buffers incorrectly, 
> and the `ReadPerfTraceAux(size_t offset)` function no longer needs to return 
> a `Status`, but instead it can just return `MutableArrayRef` since 
> nothing can go wrong.
As mentioned in my previous comments, the m_mmap_data and m_mmap_aux are cyclic 
buffers and unfortunately the kernel keeps overwriting the buffer in a cyclic 
manner. The last position written by the kernel can only be obtained by 
inspecting the data_head and aux_head fields in the  perf_event_mmap_page 
structure (pointed by m_mmap_base).

Because of this scenario you see the ugly type casting. Now regarding the 
casting in getAuxBufferSize and getDataBufferSize I did not want to store the 
buffer sizes as even if store them since they are not the biggest contributors 
to the total type casting ugliness. plus if the correct sizes are already store 
in the perf_event_mmap_page structure why store them myself ?.

Now regarding ReadPerfTraceAux(size_t offset) , i still need the size argument 
coz what if the user does not want to read the complete data from offset to the 
end and just wants a chunk in between the offset and the end ?


https://reviews.llvm.org/D33674



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

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

2017-06-07 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja added inline comments.



Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:3009-3010
+  errno = 0;
+  m_mmap_base =
+  mmap(NULL, (metabufsize + page_size), PROT_WRITE, MAP_SHARED, m_fd, 0);
+  if (m_mmap_base == MAP_FAILED) {

zturner wrote:
> Perhaps you can use `llvm::MemoryBuffer` here?  It mmaps internally
In my case, I only want to allocate it using mmap with the options i use here, 
with 
`llvm::MemoryBuffer` I see no option to force it to only use mmap and with the 
options i need ?


https://reviews.llvm.org/D33674



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

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

2017-06-14 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja updated this revision to Diff 102540.
ravitheja marked 29 inline comments as done.
ravitheja added a comment.

Changes suggested in previous round of feedback.


https://reviews.llvm.org/D33674

Files:
  include/lldb/Host/common/NativeProcessProtocol.h
  include/lldb/Host/linux/Support.h
  source/Host/linux/Support.cpp
  source/Plugins/Process/Linux/CMakeLists.txt
  source/Plugins/Process/Linux/NativeProcessLinux.cpp
  source/Plugins/Process/Linux/NativeProcessLinux.h
  source/Plugins/Process/Linux/ProcessorTrace.cpp
  source/Plugins/Process/Linux/ProcessorTrace.h
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  unittests/CMakeLists.txt
  unittests/Linux/
  unittests/Linux/CMakeLists.txt
  unittests/Linux/ProcessorTraceTest.cpp
  unittests/Process/CMakeLists.txt
  unittests/Process/Linux/
  unittests/Process/Linux/CMakeLists.txt
  unittests/Process/Linux/ProcessorTraceTest.cpp

Index: unittests/Process/Linux/ProcessorTraceTest.cpp
===
--- /dev/null
+++ unittests/Process/Linux/ProcessorTraceTest.cpp
@@ -0,0 +1,160 @@
+//===-- ProcessorTraceMonitorTest.cpp  -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "gtest/gtest.h"
+
+#include "llvm/ADT/ArrayRef.h"
+#include "ProcessorTrace.h"
+// C Includes
+
+// C++ Includes
+
+using namespace lldb_private;
+
+size_t ReadCylicBufferWrapper(void *buf, size_t buf_size,
+void *cyc_buf, size_t cyc_buf_size,
+size_t cyc_start, size_t offset) {
+  llvm::MutableArrayRef dst(reinterpret_cast (buf), buf_size);
+  llvm::MutableArrayRef src(reinterpret_cast (cyc_buf), cyc_buf_size);
+  ProcessorTraceMonitor::ReadCyclicBuffer(dst, src, cyc_start, offset);
+  return dst.size();
+}
+
+TEST(CyclicBuffer, EdgeCases) {
+  size_t bytes_read = 0;
+  uint8_t cyclic_buffer[6] = {'l', 'i', 'c', 'c', 'y', 'c'};
+
+  // We will always leave the last bytes untouched
+  // so that string comparisions work.
+  char bigger_buffer[10] = {};
+  char equal_size_buffer[7] = {};
+  char smaller_buffer[4] = {};
+
+  // nullptr in buffer
+  bytes_read = ReadCylicBufferWrapper(
+  nullptr, sizeof(smaller_buffer), cyclic_buffer, sizeof(cyclic_buffer), 3,
+  0);
+  ASSERT_EQ(0, bytes_read);
+
+  // nullptr in cyclic buffer
+  bytes_read = ReadCylicBufferWrapper(
+  smaller_buffer, sizeof(smaller_buffer), nullptr, sizeof(cyclic_buffer), 3,
+  0);
+  ASSERT_EQ(0, bytes_read);
+
+  // empty buffer to read into
+  bytes_read = ReadCylicBufferWrapper(
+  smaller_buffer, 0, cyclic_buffer, sizeof(cyclic_buffer), 3, 0);
+  ASSERT_EQ(0, bytes_read);
+
+  // empty cyclic buffer
+  bytes_read = ReadCylicBufferWrapper(
+  smaller_buffer, sizeof(smaller_buffer), cyclic_buffer, 0, 3, 0);
+  ASSERT_EQ(0, bytes_read);
+
+  // bigger offset
+  bytes_read = ReadCylicBufferWrapper(
+  smaller_buffer, sizeof(smaller_buffer), cyclic_buffer,
+  sizeof(cyclic_buffer), 3, 6);
+  ASSERT_EQ(0, bytes_read);
+
+  // wrong offset
+  bytes_read = ReadCylicBufferWrapper(
+  smaller_buffer, sizeof(smaller_buffer), cyclic_buffer,
+  sizeof(cyclic_buffer), 3, 7);
+  ASSERT_EQ(0, bytes_read);
+
+  // wrong start
+  bytes_read = ReadCylicBufferWrapper(
+  smaller_buffer, sizeof(smaller_buffer), cyclic_buffer,
+  sizeof(cyclic_buffer), 3, 7);
+  ASSERT_EQ(0, bytes_read);
+}
+
+TEST(CyclicBuffer, EqualSizeBuffer) {
+  size_t bytes_read = 0;
+  uint8_t cyclic_buffer[6] = {'l', 'i', 'c', 'c', 'y', 'c'};
+
+  char cyclic[] = "cyclic";
+  for (int i = 0; i < sizeof(cyclic); i++) {
+// We will always leave the last bytes untouched
+// so that string comparisions work.
+char equal_size_buffer[7] = {};
+bytes_read = ReadCylicBufferWrapper(
+equal_size_buffer, sizeof(cyclic_buffer), cyclic_buffer,
+sizeof(cyclic_buffer), 3, i);
+ASSERT_EQ((sizeof(cyclic) - i - 1), bytes_read);
+ASSERT_STREQ(equal_size_buffer, (cyclic + i));
+  }
+}
+
+TEST(CyclicBuffer, SmallerSizeBuffer) {
+  size_t bytes_read = 0;
+  uint8_t cyclic_buffer[6] = {'l', 'i', 'c', 'c', 'y', 'c'};
+
+  // We will always leave the last bytes untouched
+  // so that string comparisions work.
+  char smaller_buffer[4] = {};
+  bytes_read = ReadCylicBufferWrapper(
+  smaller_buffer, (sizeof(smaller_buffer) - 1), cyclic_buffer,
+  sizeof(cyclic_buffer), 3, 0);
+  ASSERT_EQ(3, bytes_read);
+  ASSERT_STREQ(smaller_buffer, "cyc");
+
+  bytes_read = ReadCylicBufferWrapper(
+  smaller_buffer, (sizeof(smaller_buffer) - 1), cyclic_buffer,
+  sizeof(cyclic_buffer), 3, 1);
+  ASSERT_EQ(3, bytes_read);
+  ASSERT_STREQ(smaller_buffer, "ycl");
+
+  bytes_read = ReadCylicBufferWrapper(
+  smaller_buffer, (sizeof(smalle

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

2017-06-14 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja added inline comments.



Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2847
+
+Status NativeProcessLinux::StopProcessorTracingOnProcess(lldb::user_id_t uid) {
+  Status ret_error;

labath wrote:
> You are  calling this function with uid == m_pt_process_uid, which means this 
> parameter is redundant, and leads to redundant sanity checks. Please remove 
> it.
Well I wanted this function to be capable of being called from other contexts 
as well, but i never ended up using that way.



Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2940-2943
+Status NativeProcessLinux::ProcessorTraceMonitor::StartTrace(
+lldb::pid_t pid, lldb::tid_t tid, TraceOptions &config) {
+  Status error;
+  Log *log(ProcessPOSIXLog::GetLogIfAllCategoriesSet(POSIX_LOG_PTRACE));

zturner wrote:
> What happens if you call this function twice in a row?  Or from different 
> threads?  Is that something you care about?
> What happens if you call this function twice in a row

Second call will fail,

>  Or from different threads

If called for different threads, then they will start to be traced, ofcourse 
assuming they are not being traced.



Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:3056
+
+  auto BufferOrError = getProcFile("cpuinfo");
+  if (!BufferOrError) {

labath wrote:
> I don't see a getProcFile overload with this signature (although I am not 
> opposed to adding one). Are you sure this compiles?
Yes sorry I forgot to add this new file.



Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:319-320
+int m_fd;
+void *m_mmap_data;
+void *m_mmap_aux;
+void *m_mmap_base;

labath wrote:
> ravitheja wrote:
> > zturner wrote:
> > > Instead of having 
> > > ```
> > > void *m_mmap_data;
> > > void *m_mmap_aux;
> > > void *m_mmap_base;
> > > ```
> > > 
> > > and then doing some ugly casting every time someone calls 
> > > `getAuxBufferSize` or `getDataBufferSize`, instead just write:
> > > 
> > > ```
> > > MutableArrayRef m_mmap_data;
> > > MutableArrayRef m_mmap_aux;
> > > ```
> > > 
> > > and initializing these array refs up front using the size from the 
> > > header.  This way you don't have to worry about anyone using the buffers 
> > > incorrectly, and the `ReadPerfTraceAux(size_t offset)` function no longer 
> > > needs to return a `Status`, but instead it can just return 
> > > `MutableArrayRef` since nothing can go wrong.
> > As mentioned in my previous comments, the m_mmap_data and m_mmap_aux are 
> > cyclic buffers and unfortunately the kernel keeps overwriting the buffer in 
> > a cyclic manner. The last position written by the kernel can only be 
> > obtained by inspecting the data_head and aux_head fields in the  
> > perf_event_mmap_page structure (pointed by m_mmap_base).
> > 
> > Because of this scenario you see the ugly type casting. Now regarding the 
> > casting in getAuxBufferSize and getDataBufferSize I did not want to store 
> > the buffer sizes as even if store them since they are not the biggest 
> > contributors to the total type casting ugliness. plus if the correct sizes 
> > are already store in the perf_event_mmap_page structure why store them 
> > myself ?.
> > 
> > Now regarding ReadPerfTraceAux(size_t offset) , i still need the size 
> > argument coz what if the user does not want to read the complete data from 
> > offset to the end and just wants a chunk in between the offset and the end ?
> So, if this refers to a structure of type `perf_event_mmap_page`, why let the 
> type of this be `perf_event_mmap_page *`? That way you can have just one 
> cast, when you initialize the member, and not each time you access it.
The thing is this structure is only available from a certain version of the 
perf_event.h. Although I have put macro guards everywhere I use this structure. 
But on previous Linux versions it won't be there, so I was afraid if i would 
run into issues.


https://reviews.llvm.org/D33674



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

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

2017-06-16 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja added inline comments.



Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:611
+
+if (ProcessorTraceMonitor::GetProcessTraceID() != LLDB_INVALID_UID) {
+  auto traceMonitor = ProcessorTraceMonitor::Create(

labath wrote:
> Every call to AddThread is followed by this block. Is there a reason we 
> cannot just move it inside the AddThread function?
Yes that could be done.



Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:282
+  // same process user id.
+  llvm::DenseSet m_pt_traced_thread_group;
 };

labath wrote:
> I am confused about the purpose of this member variable. As far as I can 
> tell, it will always contain *either* all of the threads in the process *or* 
> none of them. Is there a situation where this is not the case?
Yes there can be situations like that, for e.g if a user starts tracing on an 
individual thread followed by tracing the whole process (meaning in two 
separate start trace calls) then this Set will contain the thread ids of all 
the threads except the one on which individual tracing was started. This can be 
extended further.



Comment at: unittests/Process/Linux/ProcessorTraceTest.cpp:41
+  bytes_read = ReadCylicBufferWrapper(
+  nullptr, sizeof(smaller_buffer), cyclic_buffer, sizeof(cyclic_buffer), 3,
+  0);

labath wrote:
> I don't understand why you're testing these. Why would anyone create an 
> ArrayRef pointing to null? If you get handed an array ref, you should be free 
> to assume it points to valid data.
Well I see that ArrayRef can be constructed with a nullptr and a positive size, 
like 

```
ArrayRef(nullptr, 5);
```
hence I added the that check . If u want I can remove these checks ?


https://reviews.llvm.org/D33674



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

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

2017-06-16 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja added inline comments.



Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:158
+LLDB_LOG(log, "ProcessorTrace failed to open Config file");
+error.SetError(FileNotFound, eErrorTypePOSIX);
+return error;

labath wrote:
> eErrorTypePOSIX is used for errno error values. Please don't try to pass your 
> invented error codes as these.
Yes I did not want to use eErrorTypePOSIX but when transitioning from Status to 
llvm::Error, the m_code is only retained for eErrorTypePOSIX else its not 
retained.


https://reviews.llvm.org/D33674



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

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

2017-06-16 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja added a comment.

Just to make things clear, I will explain a use case

Suppose if we are debugging an application where the main thread spawns a 
second new thread ->

  int main() {
  int i = 0;   // user starts tracing on main thread -> gets traceid 1
  .  // Some statements
  i++; // Here he starts tracing the whole process -> gets traceid 2.
  // Now traceid=2 represents the tracing instance on the 
  // whole process, meaning all new threads spawned in 
  //  the process will be traced with this id. Although the 
  //main thread is already being traced hence 
  // will not be a part of traceid =2
  std::thread (thread_function) // New thread spawned, this will be traced with 
id =2
  .  // Some statements
  // To obtain the trace data or stop the trace on main thread
 //  the user can simply work with trace id =1 and not specify the
// thread id anywhere.
  // For threads under the process trace id hood, the thread id needs 
to be specified for
  // obtaining the trace data or if the tracing needs to be stopped 
only for that thread.
 // Now if the tracing on the process is switched off then the tracing 
on the main thread
// is not affected.
  }

Now the set of threads

  m_pt_traced_thread_group

will track threads that are being traced under the process trace id. Hence when 
a thread exits,
we need to update this set accordingly.

Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2644
+  if (iter != m_processor_trace_monitor.end())
+error = StopTrace(iter->second->GetTraceID(), thread);
+

labath wrote:
> Should you delete the iterator from the map after this? In fact, the mere act 
> of deleting the iterator should probably be what triggers the trace stop, in 
> line with RAII principles.
Yes, but the thing is i have to update the Set of threads 

```m_pt_traced_thread_group
```
which is tracking how many threads are being traced under the process trace id. 
The StopTrace calls StopProcessorTracingOnThread eventually which deletes the 
element from the various containers. I can directly call 
StopProcessorTracingOnThread instead of StopTrace.

https://reviews.llvm.org/D33674

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

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

2017-06-16 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja added inline comments.



Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:282
+  // same process user id.
+  llvm::DenseSet m_pt_traced_thread_group;
 };

labath wrote:
> ravitheja wrote:
> > labath wrote:
> > > I am confused about the purpose of this member variable. As far as I can 
> > > tell, it will always contain *either* all of the threads in the process 
> > > *or* none of them. Is there a situation where this is not the case?
> > Yes there can be situations like that, for e.g if a user starts tracing on 
> > an individual thread followed by tracing the whole process (meaning in two 
> > separate start trace calls) then this Set will contain the thread ids of 
> > all the threads except the one on which individual tracing was started. 
> > This can be extended further.
> Interesting... Is that actually useful for something? I find the semantic of 
> "trace all threads in the process except those that happen to be traced 
> already" a bit confusing. 
> 
> If you have a use for it in mind then fine, but otherwise, I'd like go for 
> the simpler option of failing the request to trace the whole process if some 
> individual threads are traced already. "you can either trace the whole 
> process *OR* any number of individual threads" sounds much easier to 
> understand. Otherwise, you'd have to think about corner cases like "What if 
> the individual thread trace is stopped but the process trace request 
> remains?" Do you then automatically start tracing the remaining thread based 
> on the previous "whole process trace" request, and so on...
Yes thats what will happen. All threads spawned will be traced unless the 
tracing on the whole process is stopped.


https://reviews.llvm.org/D33674



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

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

2017-06-16 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja added inline comments.



Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:158
+LLDB_LOG(log, "ProcessorTrace failed to open Config file");
+error.SetError(FileNotFound, eErrorTypePOSIX);
+return error;

labath wrote:
> ravitheja wrote:
> > labath wrote:
> > > eErrorTypePOSIX is used for errno error values. Please don't try to pass 
> > > your invented error codes as these.
> > Yes I did not want to use eErrorTypePOSIX but when transitioning from 
> > Status to llvm::Error, the m_code is only retained for eErrorTypePOSIX else 
> > its not retained.
> That's a good point. When I wrote the conversion function, there was no use 
> case for this --  I think you're the first person who actually want's to use 
> the error codes in some meaningful way.
> 
> What is your further plan for these error codes? Judging by the state of the 
> D33035 you won't be able to use them to display the error messages to the 
> user?
> 
> If you still want to preserve the error codes, we can definitely make this 
> happen. Actually, llvm::Error makes this even easier, as it allows you to 
> define new error categories in a distributed way. Frankly, I think the your 
> use of the "generic" error category with custom error codes is a bit of a 
> hack. I think the intended usage of the Status class was to define your own 
> ErrorType enum value and tag your errors with that (but that does not scale 
> well to many different sources of error codes).
My plan is to perhaps implement a way to pass error strings along with the 
error packets, so that the tool in D33035 can directly use those strings. I 
guess then I can just use the eErrorTypeGeneric . 

So keeping that in mind I can just remove the error codes and replace them 
everywhere with the strings ? Although the tool will not work unless the error 
strings are transported .



https://reviews.llvm.org/D33674



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

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

2017-06-19 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja added a comment.

Although a bit confusing, there is more flexibility for the user.I must also 
point out that the trace buffer available is not unlimited and there can be 
situations where a user might simultaneously want to trace newly spawned 
threads with a smaller buffer and trace an individual thread with perhaps a 
bigger buffer size.  Tracing all threads is definitely important coz the user 
might not want to separately start tracing on each new thread. Now the current 
design might be a bit confusing but I am willing to document it well with 
examples and uses cases.

Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:277
+
+  llvm::DenseMap
+  m_processor_trace_monitor;

labath wrote:
> I'd like to downgrade these to unique pointers to ProcessTraceMonitor. 
> There's no reason for these to ever outlive or escape the process instance, 
> so it's natural to say they are strongly owned by it. In other places where 
> you use ProcessorTraceMonitorSP you can just use regular pointers or 
> references (depending on whether they can be null or not).
Hi, I don't see the advantage of changing to unique pointers ? coz when the 
process dies they will be destroyed anyhow, plus using shared pointers makes it 
easier for functions operating with the ProcessTraceMonitor to work.

https://reviews.llvm.org/D33674

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

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

2017-06-19 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja added a comment.

> Start tracing the whole process
>  Ok. Traceid = 1
>  Start tracing thread 47
>  Error. Thread 47 is already traced. (???)

The error is because its already being traced, so if someone wants the trace 
log, its already available and the user need not do anything extra.


https://reviews.llvm.org/D33674



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

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

2017-06-19 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja added inline comments.



Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:277
+
+  llvm::DenseMap
+  m_processor_trace_monitor;

labath wrote:
> ravitheja wrote:
> > labath wrote:
> > > I'd like to downgrade these to unique pointers to ProcessTraceMonitor. 
> > > There's no reason for these to ever outlive or escape the process 
> > > instance, so it's natural to say they are strongly owned by it. In other 
> > > places where you use ProcessorTraceMonitorSP you can just use regular 
> > > pointers or references (depending on whether they can be null or not).
> > Hi, I don't see the advantage of changing to unique pointers ? coz when the 
> > process dies they will be destroyed anyhow, plus using shared pointers 
> > makes it easier for functions operating with the ProcessTraceMonitor to 
> > work.
> It makes it clear that the Process is the owner of these objects (and not for 
> example "sharing" them with anyone else). Plus you should use the simplest 
> tool that gets the job done and unique_ptr is definitely simpler. So I'd 
> reverse the question: If there is no need for using shared_ptr, why do it?
> 
> I disagree with the statement that it makes it harder for the functions to 
> work. Please provide an example.
Ok I was thinking that working with shared_pointers was a bit cleaner approach 
than working with references or raw pointers, but in this case won't make much 
of a difference.

I will make them unique pointers. No problem.


https://reviews.llvm.org/D33674



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

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

2017-06-19 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja added a comment.

In https://reviews.llvm.org/D33674#783893, @labath wrote:

> In https://reviews.llvm.org/D33674#783861, @ravitheja wrote:
>
> > Although a bit confusing, there is more flexibility for the user.I must 
> > also point out that the trace buffer available is not unlimited and there 
> > can be situations where a user might simultaneously want to trace newly 
> > spawned threads with a smaller buffer and trace an individual thread with 
> > perhaps a bigger buffer size.  Tracing all threads is definitely important 
> > coz the user might not want to separately start tracing on each new thread. 
> > Now the current design might be a bit confusing but I am willing to 
> > document it well with examples and uses cases.
>
>
> I agree that having the ability to trace new threads from the moment they 
> spawn is important. The confusion arises from the fact that we are treating 
> "trace all threads" and "trace new threads" as a single concept. I think the 
> cleanest solution for me would be to split those up, so that we would have 
> three operations: (1) trace a specific thread, (2) trace new threads, (3) 
> trace all threads. Then (1) and (2) can be combined in arbitrary ways, and 
> (3) is mutually exclusive with anything else. It could be I am over-designing 
> this though. I'd like to hear the opinion of anyone else that is reading this.


Well that's the whole thread group idea. Currently we have only one thread 
group i.e the process group (all existing un traced threads + newly spawned 
ones).
With separate "trace all threads" and "trace new threads", it will be multiple 
thread groups. For e.g

Lets say an application spawns 10 threads, now here we can end up with 9 thread 
groups in the worst case if the user calls  "trace all threads" after each
new spawned thread.

Now I wanted to avoid having multiple thread groups coz the implementation will 
become more complex.


https://reviews.llvm.org/D33674



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

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

2017-06-21 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja updated this revision to Diff 103355.
ravitheja added a comment.

Changes for last feedback.


https://reviews.llvm.org/D33674

Files:
  include/lldb/Host/common/NativeProcessProtocol.h
  include/lldb/Host/linux/Support.h
  source/Host/linux/Support.cpp
  source/Plugins/Process/Linux/CMakeLists.txt
  source/Plugins/Process/Linux/NativeProcessLinux.cpp
  source/Plugins/Process/Linux/NativeProcessLinux.h
  source/Plugins/Process/Linux/ProcessorTrace.cpp
  source/Plugins/Process/Linux/ProcessorTrace.h
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  unittests/CMakeLists.txt
  unittests/Process/CMakeLists.txt
  unittests/Process/Linux/
  unittests/Process/Linux/CMakeLists.txt
  unittests/Process/Linux/ProcessorTraceTest.cpp

Index: unittests/Process/Linux/ProcessorTraceTest.cpp
===
--- /dev/null
+++ unittests/Process/Linux/ProcessorTraceTest.cpp
@@ -0,0 +1,149 @@
+//===-- ProcessorTraceMonitorTest.cpp  -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "gtest/gtest.h"
+
+#include "llvm/ADT/ArrayRef.h"
+#include "ProcessorTrace.h"
+// C Includes
+
+// C++ Includes
+
+using namespace lldb_private;
+using namespace process_linux;
+
+size_t ReadCylicBufferWrapper(void *buf, size_t buf_size,
+void *cyc_buf, size_t cyc_buf_size,
+size_t cyc_start, size_t offset) {
+  llvm::MutableArrayRef dst(reinterpret_cast (buf), buf_size);
+  llvm::MutableArrayRef src(reinterpret_cast (cyc_buf), cyc_buf_size);
+  ProcessorTraceMonitor::ReadCyclicBuffer(dst, src, cyc_start, offset);
+  return dst.size();
+}
+
+TEST(CyclicBuffer, EdgeCases) {
+  size_t bytes_read = 0;
+  uint8_t cyclic_buffer[6] = {'l', 'i', 'c', 'c', 'y', 'c'};
+
+  // We will always leave the last bytes untouched
+  // so that string comparisions work.
+  char bigger_buffer[10] = {};
+  char equal_size_buffer[7] = {};
+  char smaller_buffer[4] = {};
+
+  // empty buffer to read into
+  bytes_read = ReadCylicBufferWrapper(
+  smaller_buffer, 0, cyclic_buffer, sizeof(cyclic_buffer), 3, 0);
+  ASSERT_EQ(0, bytes_read);
+
+  // empty cyclic buffer
+  bytes_read = ReadCylicBufferWrapper(
+  smaller_buffer, sizeof(smaller_buffer), cyclic_buffer, 0, 3, 0);
+  ASSERT_EQ(0, bytes_read);
+
+  // bigger offset
+  bytes_read = ReadCylicBufferWrapper(
+  smaller_buffer, sizeof(smaller_buffer), cyclic_buffer,
+  sizeof(cyclic_buffer), 3, 6);
+  ASSERT_EQ(0, bytes_read);
+
+  // wrong offset
+  bytes_read = ReadCylicBufferWrapper(
+  smaller_buffer, sizeof(smaller_buffer), cyclic_buffer,
+  sizeof(cyclic_buffer), 3, 7);
+  ASSERT_EQ(0, bytes_read);
+
+  // wrong start
+  bytes_read = ReadCylicBufferWrapper(
+  smaller_buffer, sizeof(smaller_buffer), cyclic_buffer,
+  sizeof(cyclic_buffer), 3, 7);
+  ASSERT_EQ(0, bytes_read);
+}
+
+TEST(CyclicBuffer, EqualSizeBuffer) {
+  size_t bytes_read = 0;
+  uint8_t cyclic_buffer[6] = {'l', 'i', 'c', 'c', 'y', 'c'};
+
+  char cyclic[] = "cyclic";
+  for (int i = 0; i < sizeof(cyclic); i++) {
+// We will always leave the last bytes untouched
+// so that string comparisions work.
+char equal_size_buffer[7] = {};
+bytes_read = ReadCylicBufferWrapper(
+equal_size_buffer, sizeof(cyclic_buffer), cyclic_buffer,
+sizeof(cyclic_buffer), 3, i);
+ASSERT_EQ((sizeof(cyclic) - i - 1), bytes_read);
+ASSERT_STREQ(equal_size_buffer, (cyclic + i));
+  }
+}
+
+TEST(CyclicBuffer, SmallerSizeBuffer) {
+  size_t bytes_read = 0;
+  uint8_t cyclic_buffer[6] = {'l', 'i', 'c', 'c', 'y', 'c'};
+
+  // We will always leave the last bytes untouched
+  // so that string comparisions work.
+  char smaller_buffer[4] = {};
+  bytes_read = ReadCylicBufferWrapper(
+  smaller_buffer, (sizeof(smaller_buffer) - 1), cyclic_buffer,
+  sizeof(cyclic_buffer), 3, 0);
+  ASSERT_EQ(3, bytes_read);
+  ASSERT_STREQ(smaller_buffer, "cyc");
+
+  bytes_read = ReadCylicBufferWrapper(
+  smaller_buffer, (sizeof(smaller_buffer) - 1), cyclic_buffer,
+  sizeof(cyclic_buffer), 3, 1);
+  ASSERT_EQ(3, bytes_read);
+  ASSERT_STREQ(smaller_buffer, "ycl");
+
+  bytes_read = ReadCylicBufferWrapper(
+  smaller_buffer, (sizeof(smaller_buffer) - 1), cyclic_buffer,
+  sizeof(cyclic_buffer), 3, 2);
+  ASSERT_EQ(3, bytes_read);
+  ASSERT_STREQ(smaller_buffer, "cli");
+
+  bytes_read = ReadCylicBufferWrapper(
+  smaller_buffer, (sizeof(smaller_buffer) - 1), cyclic_buffer,
+  sizeof(cyclic_buffer), 3, 3);
+  ASSERT_EQ(3, bytes_read);
+  ASSERT_STREQ(smaller_buffer, "lic");
+  {
+char smaller_buffer[4] = {};
+bytes_read = ReadCylicBufferWrapper(
+smaller_buffer, (sizeof(smaller_buffer) - 1), cyclic_buffer,
+

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

2017-06-22 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja marked 5 inline comments as done.
ravitheja added inline comments.



Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:307
+
+Status ProcessorTraceMonitor::Destroy() {
+  Status error;

labath wrote:
> I'd like this work to be done in the destructor. Just like nobody should see 
> a constructed-but-not-yet-initialized object, so the 
> destroyed-but-not-destructed state should not exist. Then you don't need to 
> worry about setting the state of the object to some sane values.
> 
> In fact, if you utilize std::unique_ptr capabilities, then you may not even 
> need a destructor at all. Just define a class
> ```
> class munmap_delete {
>   size_t m_length;
> 
> public:
>   munmap_delete(size_t length) : m_length(length) {}
>   void operator()(void *ptr) { munmap(ptr, m_length); }
> };
> ```
> 
> and then you can just declare  `
> `std::unique_ptr mmap_base(mmap_result, 
> munmap_delete(mmap_size))`
> 
> This way, the allocation and deallocation code are close together, and you 
> don't need to write yourself `// we allocated extra page` reminders.
> 
> Since we store m_aux in a ArrayRef already, this would require duplicating 
> the pointer in the unique_ptr member. I am fine with that, but if you want to 
> avoid it you can just have a getAux() member function which sythesizes the 
> arrayref on demand.
I can get the desired effect, by moving this function to the destructor, but we 
won't be able to catch munmap errors. I guess logging should be fine ? right 
now we were able to report errors in munmap.


https://reviews.llvm.org/D33674



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

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

2017-06-22 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja added inline comments.



Comment at: source/Plugins/Process/Linux/ProcessorTrace.h:79
+  // Trace id of trace instance corresponding to the process.
+  static lldb::user_id_t m_pt_process_traceid;
+

labath wrote:
> labath wrote:
> > Instead of global variables (we may want to debug more than one process 
> > eventually), we could have a member variable `std::pair > TraceOptions> m_process_trace;` (you can have a special class for that if 
> > you don't like .first and .second everywhere).
> I guess I wasn't too clear about this, but I meant a member variable in the 
> Process class. This solves the "multiple process" issue, but creates a couple 
> of other ones. All the call sites are now more complicated, and you have to 
> worry about managing the lifetime of the entries in this map. If this was a 
> Process member variable, those would be handled automatically (and it makes 
> more sense, since it is the process class that owns these traces instances).
Ok sorry now I understand what u meant.


https://reviews.llvm.org/D33674



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

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

2017-06-26 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja updated this revision to Diff 103943.
ravitheja marked 6 inline comments as done.
ravitheja added a comment.

More changes from past feedback.


https://reviews.llvm.org/D33674

Files:
  include/lldb/Host/common/NativeProcessProtocol.h
  include/lldb/Host/linux/Support.h
  source/Host/linux/Support.cpp
  source/Plugins/Process/Linux/CMakeLists.txt
  source/Plugins/Process/Linux/NativeProcessLinux.cpp
  source/Plugins/Process/Linux/NativeProcessLinux.h
  source/Plugins/Process/Linux/ProcessorTrace.cpp
  source/Plugins/Process/Linux/ProcessorTrace.h
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  unittests/Process/CMakeLists.txt
  unittests/Process/Linux/
  unittests/Process/Linux/CMakeLists.txt
  unittests/Process/Linux/ProcessorTraceTest.cpp

Index: unittests/Process/Linux/ProcessorTraceTest.cpp
===
--- /dev/null
+++ unittests/Process/Linux/ProcessorTraceTest.cpp
@@ -0,0 +1,152 @@
+//===-- ProcessorTraceMonitorTest.cpp - -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "gtest/gtest.h"
+
+#include "ProcessorTrace.h"
+#include "llvm/ADT/ArrayRef.h"
+// C Includes
+
+// C++ Includes
+
+using namespace lldb_private;
+using namespace process_linux;
+
+size_t ReadCylicBufferWrapper(void *buf, size_t buf_size, void *cyc_buf,
+  size_t cyc_buf_size, size_t cyc_start,
+  size_t offset) {
+  llvm::MutableArrayRef dst(reinterpret_cast(buf),
+ buf_size);
+  llvm::MutableArrayRef src(reinterpret_cast(cyc_buf),
+ cyc_buf_size);
+  ProcessorTraceMonitor::ReadCyclicBuffer(dst, src, cyc_start, offset);
+  return dst.size();
+}
+
+TEST(CyclicBuffer, EdgeCases) {
+  size_t bytes_read = 0;
+  uint8_t cyclic_buffer[6] = {'l', 'i', 'c', 'c', 'y', 'c'};
+
+  // We will always leave the last bytes untouched
+  // so that string comparisions work.
+  char bigger_buffer[10] = {};
+  char equal_size_buffer[7] = {};
+  char smaller_buffer[4] = {};
+
+  // empty buffer to read into
+  bytes_read = ReadCylicBufferWrapper(smaller_buffer, 0, cyclic_buffer,
+  sizeof(cyclic_buffer), 3, 0);
+  ASSERT_EQ(0, bytes_read);
+
+  // empty cyclic buffer
+  bytes_read = ReadCylicBufferWrapper(smaller_buffer, sizeof(smaller_buffer),
+  cyclic_buffer, 0, 3, 0);
+  ASSERT_EQ(0, bytes_read);
+
+  // bigger offset
+  bytes_read =
+  ReadCylicBufferWrapper(smaller_buffer, sizeof(smaller_buffer),
+ cyclic_buffer, sizeof(cyclic_buffer), 3, 6);
+  ASSERT_EQ(0, bytes_read);
+
+  // wrong offset
+  bytes_read =
+  ReadCylicBufferWrapper(smaller_buffer, sizeof(smaller_buffer),
+ cyclic_buffer, sizeof(cyclic_buffer), 3, 7);
+  ASSERT_EQ(0, bytes_read);
+
+  // wrong start
+  bytes_read =
+  ReadCylicBufferWrapper(smaller_buffer, sizeof(smaller_buffer),
+ cyclic_buffer, sizeof(cyclic_buffer), 3, 7);
+  ASSERT_EQ(0, bytes_read);
+}
+
+TEST(CyclicBuffer, EqualSizeBuffer) {
+  size_t bytes_read = 0;
+  uint8_t cyclic_buffer[6] = {'l', 'i', 'c', 'c', 'y', 'c'};
+
+  char cyclic[] = "cyclic";
+  for (int i = 0; i < sizeof(cyclic); i++) {
+// We will always leave the last bytes untouched
+// so that string comparisions work.
+char equal_size_buffer[7] = {};
+bytes_read =
+ReadCylicBufferWrapper(equal_size_buffer, sizeof(cyclic_buffer),
+   cyclic_buffer, sizeof(cyclic_buffer), 3, i);
+ASSERT_EQ((sizeof(cyclic) - i - 1), bytes_read);
+ASSERT_STREQ(equal_size_buffer, (cyclic + i));
+  }
+}
+
+TEST(CyclicBuffer, SmallerSizeBuffer) {
+  size_t bytes_read = 0;
+  uint8_t cyclic_buffer[6] = {'l', 'i', 'c', 'c', 'y', 'c'};
+
+  // We will always leave the last bytes untouched
+  // so that string comparisions work.
+  char smaller_buffer[4] = {};
+  bytes_read =
+  ReadCylicBufferWrapper(smaller_buffer, (sizeof(smaller_buffer) - 1),
+ cyclic_buffer, sizeof(cyclic_buffer), 3, 0);
+  ASSERT_EQ(3, bytes_read);
+  ASSERT_STREQ(smaller_buffer, "cyc");
+
+  bytes_read =
+  ReadCylicBufferWrapper(smaller_buffer, (sizeof(smaller_buffer) - 1),
+ cyclic_buffer, sizeof(cyclic_buffer), 3, 1);
+  ASSERT_EQ(3, bytes_read);
+  ASSERT_STREQ(smaller_buffer, "ycl");
+
+  bytes_read =
+  ReadCylicBufferWrapper(smaller_buffer, (sizeof(smaller_buffer) - 1),
+ cyclic_buffer, sizeof(cyclic_buffer), 3, 2);
+  ASSERT_EQ(3, bytes_read);
+  ASSERT_STREQ(smaller_buffer, "cli");
+
+  bytes_read

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

2017-06-26 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja added a comment.

Yes you can start looking at it. The thing with munmap stuff is the following, 
you basically don't want to give the user an opportunity to have an 
uninitialized or instances which have been destroyed but not deleted.
So I removed the Destroy function from the public space. Now the user need not 
worry about Destroy function.

Regarding the testing strategy, we have tests at two levels, one at the SB API 
level and the other at the tool level.


https://reviews.llvm.org/D33674



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

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

2017-06-26 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja added inline comments.



Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:393
+  llvm::SmallVector, 2> parts = {
+  src.slice(src_cyc_index), src.drop_back(src.size() - src_cyc_index)};
+

labath wrote:
> Isn't the drop-back expression equivalent to `src.take_front(src_cyc_index)`?
The problem with that is I don't see the documentation of some functions and I 
have to dig in the code.
Which is why I did not used it.


https://reviews.llvm.org/D33674



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

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

2017-06-26 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja added a comment.

>> Regarding the testing strategy, we have tests at two levels, one at the SB 
>> API level and the other at the tool level.
> 
> Cool, are you going to put some of those tests in this patch?

The problem with uploading tests is that they need minimum Broadwell machine 
and a newer Linux kernel (> 4.2 something )


https://reviews.llvm.org/D33674



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

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

2017-06-26 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja added a comment.

In https://reviews.llvm.org/D33674#790653, @labath wrote:

> In https://reviews.llvm.org/D33674#790595, @ravitheja wrote:
>
> > Yes you can start looking at it. The thing with munmap stuff is the 
> > following, you basically don't want to give the user an opportunity to have 
> > an uninitialized or instances which have been destroyed but not deleted.
> >  So I removed the Destroy function from the public space. Now the user need 
> > not worry about Destroy function.
>
>
> Ok, so that was one of my reasons for doing this. The other one is internal 
> code cleanlyness -- it's much easier to verify that the code is healthy by 
> just looking at it when the initialization and deinitialization is close 
> together. unique_ptr allows you to do that. In this code
>
>   if ((result = mmap(..., size)) != MAP_FAILED)
> ptr_up = std::unique_ptr(result, mmap_deleter(size));
>
>
> the init and deinit code is on adjecant line, and it's guaranteed than memory 
> will be freed. Here:
>
>   first_function() {
>   ptr = mmap(..., size);
>   ref=ArrayRef(ptr, size-1);
>   ...
>   }
>  
>   ...
>  
>   second_function() {
> ...
> // Remember to increment size by one
> munmap(ref.data(), ref.size()+1);
> ... 
>   }
>
>
> it's not so obvious because the code is far apart and you need to carry state 
> around. To verify correctness I need to look at the two pieces of code, and 
> then find all possible code paths between them.


Ok I will write it like that. Please tell me if u want more changes, I will do 
them all together for the next patch.




Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:393
+  llvm::SmallVector, 2> parts = {
+  src.slice(src_cyc_index), src.drop_back(src.size() - src_cyc_index)};
+

labath wrote:
> ravitheja wrote:
> > labath wrote:
> > > Isn't the drop-back expression equivalent to 
> > > `src.take_front(src_cyc_index)`?
> > The problem with that is I don't see the documentation of some functions 
> > and I have to dig in the code.
> > Which is why I did not used it.
> That's fine, there are a lot of these APIs and it's not possible to remember 
> all of them.  However, now that you know about the function, wouldn't you 
> agree that it's cleaner to use it?
Ok I will use that.


https://reviews.llvm.org/D33674



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

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

2017-06-27 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja updated this revision to Diff 104160.
ravitheja added a comment.

Removing Destroy() function.


https://reviews.llvm.org/D33674

Files:
  include/lldb/Host/common/NativeProcessProtocol.h
  include/lldb/Host/linux/Support.h
  source/Host/linux/Support.cpp
  source/Plugins/Process/Linux/CMakeLists.txt
  source/Plugins/Process/Linux/NativeProcessLinux.cpp
  source/Plugins/Process/Linux/NativeProcessLinux.h
  source/Plugins/Process/Linux/ProcessorTrace.cpp
  source/Plugins/Process/Linux/ProcessorTrace.h
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  unittests/Process/CMakeLists.txt
  unittests/Process/Linux/
  unittests/Process/Linux/CMakeLists.txt
  unittests/Process/Linux/ProcessorTraceTest.cpp

Index: unittests/Process/Linux/ProcessorTraceTest.cpp
===
--- /dev/null
+++ unittests/Process/Linux/ProcessorTraceTest.cpp
@@ -0,0 +1,152 @@
+//===-- ProcessorTraceMonitorTest.cpp - -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "gtest/gtest.h"
+
+#include "ProcessorTrace.h"
+#include "llvm/ADT/ArrayRef.h"
+// C Includes
+
+// C++ Includes
+
+using namespace lldb_private;
+using namespace process_linux;
+
+size_t ReadCylicBufferWrapper(void *buf, size_t buf_size, void *cyc_buf,
+  size_t cyc_buf_size, size_t cyc_start,
+  size_t offset) {
+  llvm::MutableArrayRef dst(reinterpret_cast(buf),
+ buf_size);
+  llvm::MutableArrayRef src(reinterpret_cast(cyc_buf),
+ cyc_buf_size);
+  ProcessorTraceMonitor::ReadCyclicBuffer(dst, src, cyc_start, offset);
+  return dst.size();
+}
+
+TEST(CyclicBuffer, EdgeCases) {
+  size_t bytes_read = 0;
+  uint8_t cyclic_buffer[6] = {'l', 'i', 'c', 'c', 'y', 'c'};
+
+  // We will always leave the last bytes untouched
+  // so that string comparisions work.
+  char bigger_buffer[10] = {};
+  char equal_size_buffer[7] = {};
+  char smaller_buffer[4] = {};
+
+  // empty buffer to read into
+  bytes_read = ReadCylicBufferWrapper(smaller_buffer, 0, cyclic_buffer,
+  sizeof(cyclic_buffer), 3, 0);
+  ASSERT_EQ(0, bytes_read);
+
+  // empty cyclic buffer
+  bytes_read = ReadCylicBufferWrapper(smaller_buffer, sizeof(smaller_buffer),
+  cyclic_buffer, 0, 3, 0);
+  ASSERT_EQ(0, bytes_read);
+
+  // bigger offset
+  bytes_read =
+  ReadCylicBufferWrapper(smaller_buffer, sizeof(smaller_buffer),
+ cyclic_buffer, sizeof(cyclic_buffer), 3, 6);
+  ASSERT_EQ(0, bytes_read);
+
+  // wrong offset
+  bytes_read =
+  ReadCylicBufferWrapper(smaller_buffer, sizeof(smaller_buffer),
+ cyclic_buffer, sizeof(cyclic_buffer), 3, 7);
+  ASSERT_EQ(0, bytes_read);
+
+  // wrong start
+  bytes_read =
+  ReadCylicBufferWrapper(smaller_buffer, sizeof(smaller_buffer),
+ cyclic_buffer, sizeof(cyclic_buffer), 3, 7);
+  ASSERT_EQ(0, bytes_read);
+}
+
+TEST(CyclicBuffer, EqualSizeBuffer) {
+  size_t bytes_read = 0;
+  uint8_t cyclic_buffer[6] = {'l', 'i', 'c', 'c', 'y', 'c'};
+
+  char cyclic[] = "cyclic";
+  for (int i = 0; i < sizeof(cyclic); i++) {
+// We will always leave the last bytes untouched
+// so that string comparisions work.
+char equal_size_buffer[7] = {};
+bytes_read =
+ReadCylicBufferWrapper(equal_size_buffer, sizeof(cyclic_buffer),
+   cyclic_buffer, sizeof(cyclic_buffer), 3, i);
+ASSERT_EQ((sizeof(cyclic) - i - 1), bytes_read);
+ASSERT_STREQ(equal_size_buffer, (cyclic + i));
+  }
+}
+
+TEST(CyclicBuffer, SmallerSizeBuffer) {
+  size_t bytes_read = 0;
+  uint8_t cyclic_buffer[6] = {'l', 'i', 'c', 'c', 'y', 'c'};
+
+  // We will always leave the last bytes untouched
+  // so that string comparisions work.
+  char smaller_buffer[4] = {};
+  bytes_read =
+  ReadCylicBufferWrapper(smaller_buffer, (sizeof(smaller_buffer) - 1),
+ cyclic_buffer, sizeof(cyclic_buffer), 3, 0);
+  ASSERT_EQ(3, bytes_read);
+  ASSERT_STREQ(smaller_buffer, "cyc");
+
+  bytes_read =
+  ReadCylicBufferWrapper(smaller_buffer, (sizeof(smaller_buffer) - 1),
+ cyclic_buffer, sizeof(cyclic_buffer), 3, 1);
+  ASSERT_EQ(3, bytes_read);
+  ASSERT_STREQ(smaller_buffer, "ycl");
+
+  bytes_read =
+  ReadCylicBufferWrapper(smaller_buffer, (sizeof(smaller_buffer) - 1),
+ cyclic_buffer, sizeof(cyclic_buffer), 3, 2);
+  ASSERT_EQ(3, bytes_read);
+  ASSERT_STREQ(smaller_buffer, "cli");
+
+  bytes_read =
+  ReadCylicBufferWrapper(smaller_buffer,

[Lldb-commits] [PATCH] D34945: Adding Support for Error Strings in Remote Packets

2017-07-05 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja added a comment.

Yeah that would be broken, as long as we make the error packet more than 3 
bytes, then it won't work with the older lldb clients.


https://reviews.llvm.org/D34945



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

[Lldb-commits] [PATCH] D34945: Adding Support for Error Strings in Remote Packets

2017-07-05 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja updated this revision to Diff 105150.
ravitheja added a comment.

changes from feedback.


https://reviews.llvm.org/D34945

Files:
  docs/lldb-gdb-remote.txt
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
  source/Utility/StringExtractorGDBRemote.cpp
  source/Utility/StringExtractorGDBRemote.h

Index: source/Utility/StringExtractorGDBRemote.h
===
--- source/Utility/StringExtractorGDBRemote.h
+++ source/Utility/StringExtractorGDBRemote.h
@@ -11,6 +11,7 @@
 #define utility_StringExtractorGDBRemote_h_
 
 #include "lldb/Utility/StringExtractor.h"
+#include "lldb/Utility/Status.h"
 #include "llvm/ADT/StringRef.h" // for StringRef
 
 #include 
@@ -72,6 +73,7 @@
 eServerPacketType_qGetWorkingDir,
 eServerPacketType_qFileLoadAddress,
 eServerPacketType_QEnvironment,
+eServerPacketType_QErrorStringInPacketSupported,
 eServerPacketType_QLaunchArch,
 eServerPacketType_QSetDisableASLR,
 eServerPacketType_QSetDetachOnError,
@@ -190,6 +192,8 @@
   // digits. Otherwise the error encoded in XX is returned.
   uint8_t GetError();
 
+  lldb_private::Status GetStatus();
+
   size_t GetEscapedBinaryData(std::string &str);
 
 protected:
Index: source/Utility/StringExtractorGDBRemote.cpp
===
--- source/Utility/StringExtractorGDBRemote.cpp
+++ source/Utility/StringExtractorGDBRemote.cpp
@@ -19,7 +19,7 @@
 
   switch (m_packet[0]) {
   case 'E':
-if (m_packet.size() == 3 && isxdigit(m_packet[1]) && isxdigit(m_packet[2]))
+if (isxdigit(m_packet[1]) && isxdigit(m_packet[2]))
   return eError;
 break;
 
@@ -86,6 +86,8 @@
 return eServerPacketType_QEnvironment;
   if (PACKET_STARTS_WITH("QEnvironmentHexEncoded:"))
 return eServerPacketType_QEnvironmentHexEncoded;
+  if (PACKET_STARTS_WITH("QErrorStringInPacketSupported"))
+return eServerPacketType_QErrorStringInPacketSupported;
   break;
 
 case 'P':
@@ -438,7 +440,7 @@
 }
 
 bool StringExtractorGDBRemote::IsErrorResponse() const {
-  return GetResponseType() == eError && m_packet.size() == 3 &&
+  return GetResponseType() == eError &&
  isxdigit(m_packet[1]) && isxdigit(m_packet[2]);
 }
 
@@ -450,6 +452,25 @@
   return 0;
 }
 
+lldb_private::Status
+StringExtractorGDBRemote::GetStatus() {
+  lldb_private::Status error;
+  if (GetResponseType() == eError) {
+SetFilePos(1);
+uint8_t errc = GetHexU8(255);
+error.SetError(errc, lldb::eErrorTypeGeneric);
+
+std::string error_messg ("Error ");
+error_messg += std::to_string(errc);
+size_t str_index = m_packet.find(';', m_index);
+if (str_index != std::string::npos)
+  error_messg = m_packet.substr(++str_index);
+
+error.SetErrorString(error_messg);
+  }
+  return error;
+}
+
 size_t StringExtractorGDBRemote::GetEscapedBinaryData(std::string &str) {
   // Just get the data bytes in the string as
   // GDBRemoteCommunication::CheckForPacket()
Index: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
===
--- source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
+++ source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
@@ -155,6 +155,8 @@
 
   PacketResult Handle_qRegisterInfo(StringExtractorGDBRemote &packet);
 
+  PacketResult Handle_QErrorStringInPacketSupported(StringExtractorGDBRemote &packet);
+
   PacketResult Handle_qfThreadInfo(StringExtractorGDBRemote &packet);
 
   PacketResult Handle_qsThreadInfo(StringExtractorGDBRemote &packet);
Index: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
===
--- source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -132,6 +132,9 @@
   StringExtractorGDBRemote::eServerPacketType_qRegisterInfo,
   &GDBRemoteCommunicationServerLLGS::Handle_qRegisterInfo);
   RegisterMemberFunctionHandler(
+  StringExtractorGDBRemote::eServerPacketType_QErrorStringInPacketSupported,
+  &GDBRemoteCommunicationServerLLGS::Handle_QErrorStringInPacketSupported);
+  RegisterMemberFunctionHandler(
   StringExtractorGDBRemote::eServerPacketType_QRestoreRegisterState,
   &GDBRemoteCommunicationServerLLGS::Handle_QRestoreRegisterState);
   RegisterMemberFunctionHandler(
@@ -1070,6 +1073,12 @@
 }
 
 GDBRemoteCommunication::PacketResult
+GDBRemoteCommunicationServerLLGS::Handle_QErrorStringInPacke

[Lldb-commits] [PATCH] D34945: Adding Support for Error Strings in Remote Packets

2017-07-05 Thread Ravitheja Addepally via Phabricator via lldb-commits

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



Comment at: 
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp:110
+  packet += ";";
+  packet += std::string(error.AsCString());
+  return SendPacketNoLock(packet);

labath wrote:
> Aren't you supposed to send these only if the client enabled the error 
> response?
From the discussions on the dev-list I thought the client needs to query if the 
server will send the error string or not and from the
server side its always enabled ?



Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h:64
 
+  PacketResult SendErrorResponse(Status &error);
+

labath wrote:
> Why the reference?
how about const reference ?


https://reviews.llvm.org/D34945



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

[Lldb-commits] [PATCH] D34945: Adding Support for Error Strings in Remote Packets

2017-07-05 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja added a comment.

With this patch, I see the extra packet to query from server is probably 
unnecessary. I mean the error code is still there and it should be sufficient 
for the client to detect an error. As long as it does not mistake it for 
something else it should not be a problem ? .


https://reviews.llvm.org/D34945



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

[Lldb-commits] [PATCH] D34945: Adding Support for Error Strings in Remote Packets

2017-07-05 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja updated this revision to Diff 105272.
ravitheja added a comment.

Updating Doc and changing feature to be enabled by client.


https://reviews.llvm.org/D34945

Files:
  docs/lldb-gdb-remote.txt
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
  source/Utility/StringExtractorGDBRemote.cpp
  source/Utility/StringExtractorGDBRemote.h

Index: source/Utility/StringExtractorGDBRemote.h
===
--- source/Utility/StringExtractorGDBRemote.h
+++ source/Utility/StringExtractorGDBRemote.h
@@ -11,6 +11,7 @@
 #define utility_StringExtractorGDBRemote_h_
 
 #include "lldb/Utility/StringExtractor.h"
+#include "lldb/Utility/Status.h"
 #include "llvm/ADT/StringRef.h" // for StringRef
 
 #include 
@@ -72,6 +73,7 @@
 eServerPacketType_qGetWorkingDir,
 eServerPacketType_qFileLoadAddress,
 eServerPacketType_QEnvironment,
+eServerPacketType_QEnableErrorStrings,
 eServerPacketType_QLaunchArch,
 eServerPacketType_QSetDisableASLR,
 eServerPacketType_QSetDetachOnError,
@@ -190,6 +192,8 @@
   // digits. Otherwise the error encoded in XX is returned.
   uint8_t GetError();
 
+  lldb_private::Status GetStatus();
+
   size_t GetEscapedBinaryData(std::string &str);
 
 protected:
Index: source/Utility/StringExtractorGDBRemote.cpp
===
--- source/Utility/StringExtractorGDBRemote.cpp
+++ source/Utility/StringExtractorGDBRemote.cpp
@@ -19,7 +19,7 @@
 
   switch (m_packet[0]) {
   case 'E':
-if (m_packet.size() == 3 && isxdigit(m_packet[1]) && isxdigit(m_packet[2]))
+if (isxdigit(m_packet[1]) && isxdigit(m_packet[2]))
   return eError;
 break;
 
@@ -86,6 +86,8 @@
 return eServerPacketType_QEnvironment;
   if (PACKET_STARTS_WITH("QEnvironmentHexEncoded:"))
 return eServerPacketType_QEnvironmentHexEncoded;
+  if (PACKET_STARTS_WITH("QEnableErrorStrings"))
+return eServerPacketType_QEnableErrorStrings;
   break;
 
 case 'P':
@@ -438,7 +440,7 @@
 }
 
 bool StringExtractorGDBRemote::IsErrorResponse() const {
-  return GetResponseType() == eError && m_packet.size() == 3 &&
+  return GetResponseType() == eError &&
  isxdigit(m_packet[1]) && isxdigit(m_packet[2]);
 }
 
@@ -450,6 +452,25 @@
   return 0;
 }
 
+lldb_private::Status
+StringExtractorGDBRemote::GetStatus() {
+  lldb_private::Status error;
+  if (GetResponseType() == eError) {
+SetFilePos(1);
+uint8_t errc = GetHexU8(255);
+error.SetError(errc, lldb::eErrorTypeGeneric);
+
+std::string error_messg ("Error ");
+error_messg += std::to_string(errc);
+size_t str_index = m_packet.find(';', m_index);
+if (str_index != std::string::npos)
+  error_messg = m_packet.substr(++str_index);
+
+error.SetErrorString(error_messg);
+  }
+  return error;
+}
+
 size_t StringExtractorGDBRemote::GetEscapedBinaryData(std::string &str) {
   // Just get the data bytes in the string as
   // GDBRemoteCommunication::CheckForPacket()
Index: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
===
--- source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
+++ source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
@@ -155,6 +155,8 @@
 
   PacketResult Handle_qRegisterInfo(StringExtractorGDBRemote &packet);
 
+  PacketResult Handle_QErrorStringInPacketSupported(StringExtractorGDBRemote &packet);
+
   PacketResult Handle_qfThreadInfo(StringExtractorGDBRemote &packet);
 
   PacketResult Handle_qsThreadInfo(StringExtractorGDBRemote &packet);
Index: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
===
--- source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -1128,7 +1128,7 @@
   uid = m_debugged_process_sp->StartTrace(options, error);
   LLDB_LOG(log, "uid is {0} , error is {1}", uid, error.GetError());
   if (error.Fail())
-return SendErrorResponse(error.GetError());
+return SendErrorResponse(error);
 
   StreamGDBRemote response;
   response.Printf("%" PRIx64, uid);
@@ -1165,7 +1165,7 @@
   Status error = m_debugged_process_sp->StopTrace(uid, tid);
 
   if (error.Fail())
-return SendErrorResponse(error.GetError());
+return SendErrorResponse(error);
 
   return SendOKResponse();
 }
@@ -1208,7 +1208,7 @@
   Status error = m_debugged_process_sp->GetTraceConfig(uid, options);
 
   if (error.Fail())
-return S

[Lldb-commits] [PATCH] D34945: Adding Support for Error Strings in Remote Packets

2017-07-05 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja updated this revision to Diff 105273.
ravitheja added a comment.

Forgot this in the doc.


https://reviews.llvm.org/D34945

Files:
  docs/lldb-gdb-remote.txt
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
  source/Utility/StringExtractorGDBRemote.cpp
  source/Utility/StringExtractorGDBRemote.h

Index: source/Utility/StringExtractorGDBRemote.h
===
--- source/Utility/StringExtractorGDBRemote.h
+++ source/Utility/StringExtractorGDBRemote.h
@@ -11,6 +11,7 @@
 #define utility_StringExtractorGDBRemote_h_
 
 #include "lldb/Utility/StringExtractor.h"
+#include "lldb/Utility/Status.h"
 #include "llvm/ADT/StringRef.h" // for StringRef
 
 #include 
@@ -72,6 +73,7 @@
 eServerPacketType_qGetWorkingDir,
 eServerPacketType_qFileLoadAddress,
 eServerPacketType_QEnvironment,
+eServerPacketType_QEnableErrorStrings,
 eServerPacketType_QLaunchArch,
 eServerPacketType_QSetDisableASLR,
 eServerPacketType_QSetDetachOnError,
@@ -190,6 +192,8 @@
   // digits. Otherwise the error encoded in XX is returned.
   uint8_t GetError();
 
+  lldb_private::Status GetStatus();
+
   size_t GetEscapedBinaryData(std::string &str);
 
 protected:
Index: source/Utility/StringExtractorGDBRemote.cpp
===
--- source/Utility/StringExtractorGDBRemote.cpp
+++ source/Utility/StringExtractorGDBRemote.cpp
@@ -19,7 +19,7 @@
 
   switch (m_packet[0]) {
   case 'E':
-if (m_packet.size() == 3 && isxdigit(m_packet[1]) && isxdigit(m_packet[2]))
+if (isxdigit(m_packet[1]) && isxdigit(m_packet[2]))
   return eError;
 break;
 
@@ -86,6 +86,8 @@
 return eServerPacketType_QEnvironment;
   if (PACKET_STARTS_WITH("QEnvironmentHexEncoded:"))
 return eServerPacketType_QEnvironmentHexEncoded;
+  if (PACKET_STARTS_WITH("QEnableErrorStrings"))
+return eServerPacketType_QEnableErrorStrings;
   break;
 
 case 'P':
@@ -438,7 +440,7 @@
 }
 
 bool StringExtractorGDBRemote::IsErrorResponse() const {
-  return GetResponseType() == eError && m_packet.size() == 3 &&
+  return GetResponseType() == eError &&
  isxdigit(m_packet[1]) && isxdigit(m_packet[2]);
 }
 
@@ -450,6 +452,25 @@
   return 0;
 }
 
+lldb_private::Status
+StringExtractorGDBRemote::GetStatus() {
+  lldb_private::Status error;
+  if (GetResponseType() == eError) {
+SetFilePos(1);
+uint8_t errc = GetHexU8(255);
+error.SetError(errc, lldb::eErrorTypeGeneric);
+
+std::string error_messg ("Error ");
+error_messg += std::to_string(errc);
+size_t str_index = m_packet.find(';', m_index);
+if (str_index != std::string::npos)
+  error_messg = m_packet.substr(++str_index);
+
+error.SetErrorString(error_messg);
+  }
+  return error;
+}
+
 size_t StringExtractorGDBRemote::GetEscapedBinaryData(std::string &str) {
   // Just get the data bytes in the string as
   // GDBRemoteCommunication::CheckForPacket()
Index: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
===
--- source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
+++ source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
@@ -155,6 +155,8 @@
 
   PacketResult Handle_qRegisterInfo(StringExtractorGDBRemote &packet);
 
+  PacketResult Handle_QErrorStringInPacketSupported(StringExtractorGDBRemote &packet);
+
   PacketResult Handle_qfThreadInfo(StringExtractorGDBRemote &packet);
 
   PacketResult Handle_qsThreadInfo(StringExtractorGDBRemote &packet);
Index: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
===
--- source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -1128,7 +1128,7 @@
   uid = m_debugged_process_sp->StartTrace(options, error);
   LLDB_LOG(log, "uid is {0} , error is {1}", uid, error.GetError());
   if (error.Fail())
-return SendErrorResponse(error.GetError());
+return SendErrorResponse(error);
 
   StreamGDBRemote response;
   response.Printf("%" PRIx64, uid);
@@ -1165,7 +1165,7 @@
   Status error = m_debugged_process_sp->StopTrace(uid, tid);
 
   if (error.Fail())
-return SendErrorResponse(error.GetError());
+return SendErrorResponse(error);
 
   return SendOKResponse();
 }
@@ -1208,7 +1208,7 @@
   Status error = m_debugged_process_sp->GetTraceConfig(uid, options);
 
   if (error.Fail())
-return SendErrorResponse(error.GetError());

[Lldb-commits] [PATCH] D34945: Adding Support for Error Strings in Remote Packets

2017-07-06 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja updated this revision to Diff 105380.
ravitheja added a comment.

Support for Hex encoded strings and more error checking.


https://reviews.llvm.org/D34945

Files:
  docs/lldb-gdb-remote.txt
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
  source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  source/Utility/StringExtractorGDBRemote.cpp
  source/Utility/StringExtractorGDBRemote.h

Index: source/Utility/StringExtractorGDBRemote.h
===
--- source/Utility/StringExtractorGDBRemote.h
+++ source/Utility/StringExtractorGDBRemote.h
@@ -11,6 +11,7 @@
 #define utility_StringExtractorGDBRemote_h_
 
 #include "lldb/Utility/StringExtractor.h"
+#include "lldb/Utility/Status.h"
 #include "llvm/ADT/StringRef.h" // for StringRef
 
 #include 
@@ -72,6 +73,7 @@
 eServerPacketType_qGetWorkingDir,
 eServerPacketType_qFileLoadAddress,
 eServerPacketType_QEnvironment,
+eServerPacketType_QEnableErrorStrings,
 eServerPacketType_QLaunchArch,
 eServerPacketType_QSetDisableASLR,
 eServerPacketType_QSetDetachOnError,
@@ -190,6 +192,8 @@
   // digits. Otherwise the error encoded in XX is returned.
   uint8_t GetError();
 
+  lldb_private::Status GetStatus();
+
   size_t GetEscapedBinaryData(std::string &str);
 
 protected:
Index: source/Utility/StringExtractorGDBRemote.cpp
===
--- source/Utility/StringExtractorGDBRemote.cpp
+++ source/Utility/StringExtractorGDBRemote.cpp
@@ -19,8 +19,17 @@
 
   switch (m_packet[0]) {
   case 'E':
-if (m_packet.size() == 3 && isxdigit(m_packet[1]) && isxdigit(m_packet[2]))
-  return eError;
+if (isxdigit(m_packet[1]) && isxdigit(m_packet[2])) {
+  if (m_packet.size() == 3)
+return eError;
+  if (m_packet[3] == ';') {
+auto err_string = m_packet.substr(4);
+for(auto e : err_string)
+  if (!isxdigit(e))
+break;
+return eError;
+  }
+}
 break;
 
   case 'O':
@@ -86,6 +95,8 @@
 return eServerPacketType_QEnvironment;
   if (PACKET_STARTS_WITH("QEnvironmentHexEncoded:"))
 return eServerPacketType_QEnvironmentHexEncoded;
+  if (PACKET_STARTS_WITH("QEnableErrorStrings"))
+return eServerPacketType_QEnableErrorStrings;
   break;
 
 case 'P':
@@ -438,7 +449,7 @@
 }
 
 bool StringExtractorGDBRemote::IsErrorResponse() const {
-  return GetResponseType() == eError && m_packet.size() == 3 &&
+  return GetResponseType() == eError &&
  isxdigit(m_packet[1]) && isxdigit(m_packet[2]);
 }
 
@@ -450,6 +461,24 @@
   return 0;
 }
 
+lldb_private::Status
+StringExtractorGDBRemote::GetStatus() {
+  lldb_private::Status error;
+  if (GetResponseType() == eError) {
+SetFilePos(1);
+uint8_t errc = GetHexU8(255);
+error.SetError(errc, lldb::eErrorTypeGeneric);
+
+std::string error_messg ("Error ");
+error_messg += std::to_string(errc);
+if (GetChar() == ';')
+  GetHexByteString(error_messg);
+
+error.SetErrorString(error_messg);
+  }
+  return error;
+}
+
 size_t StringExtractorGDBRemote::GetEscapedBinaryData(std::string &str) {
   // Just get the data bytes in the string as
   // GDBRemoteCommunication::CheckForPacket()
Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -1031,6 +1031,7 @@
   m_gdb_comm.GetHostInfo();
   m_gdb_comm.GetVContSupported('c');
   m_gdb_comm.GetVAttachOrWaitSupported();
+  m_gdb_comm.EnableErrorStringInPacket();
 
   // Ask the remote server for the default thread id
   if (GetTarget().GetNonStopModeEnabled())
Index: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
===
--- source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
+++ source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
@@ -155,6 +155,8 @@
 
   PacketResult Handle_qRegisterInfo(StringExtractorGDBRemote &packet);
 
+  PacketResult Handle_QErrorStringInPacketSupported(StringExtractorGDBRemote &packet);
+
   PacketResult Handle_qfThreadInfo(StringExtractorGDBRemote &packet);
 
   PacketResult Handle_qsThreadInfo(StringExtractorGDBRemote &packet);
Index: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
===
--- source/Plugins/Process/gdb-remote/GDBRemoteCommunication

[Lldb-commits] [PATCH] D34945: Adding Support for Error Strings in Remote Packets

2017-07-06 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja updated this revision to Diff 105395.
ravitheja added a comment.

Correcting mistakes.


https://reviews.llvm.org/D34945

Files:
  docs/lldb-gdb-remote.txt
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  source/Utility/StringExtractorGDBRemote.cpp
  source/Utility/StringExtractorGDBRemote.h

Index: source/Utility/StringExtractorGDBRemote.h
===
--- source/Utility/StringExtractorGDBRemote.h
+++ source/Utility/StringExtractorGDBRemote.h
@@ -10,6 +10,7 @@
 #ifndef utility_StringExtractorGDBRemote_h_
 #define utility_StringExtractorGDBRemote_h_
 
+#include "lldb/Utility/Status.h"
 #include "lldb/Utility/StringExtractor.h"
 #include "llvm/ADT/StringRef.h" // for StringRef
 
@@ -72,6 +73,7 @@
 eServerPacketType_qGetWorkingDir,
 eServerPacketType_qFileLoadAddress,
 eServerPacketType_QEnvironment,
+eServerPacketType_QEnableErrorStrings,
 eServerPacketType_QLaunchArch,
 eServerPacketType_QSetDisableASLR,
 eServerPacketType_QSetDetachOnError,
@@ -190,6 +192,8 @@
   // digits. Otherwise the error encoded in XX is returned.
   uint8_t GetError();
 
+  lldb_private::Status GetStatus();
+
   size_t GetEscapedBinaryData(std::string &str);
 
 protected:
Index: source/Utility/StringExtractorGDBRemote.cpp
===
--- source/Utility/StringExtractorGDBRemote.cpp
+++ source/Utility/StringExtractorGDBRemote.cpp
@@ -19,8 +19,18 @@
 
   switch (m_packet[0]) {
   case 'E':
-if (m_packet.size() == 3 && isxdigit(m_packet[1]) && isxdigit(m_packet[2]))
-  return eError;
+if (isxdigit(m_packet[1]) && isxdigit(m_packet[2])) {
+  if (m_packet.size() == 3)
+return eError;
+  llvm::StringRef packet_ref(m_packet);
+  if (packet_ref[3] == ';') {
+auto err_string = packet_ref.substr(4);
+for (auto e : err_string)
+  if (!isxdigit(e))
+return eResponse;
+return eError;
+  }
+}
 break;
 
   case 'O':
@@ -86,6 +96,8 @@
 return eServerPacketType_QEnvironment;
   if (PACKET_STARTS_WITH("QEnvironmentHexEncoded:"))
 return eServerPacketType_QEnvironmentHexEncoded;
+  if (PACKET_STARTS_WITH("QEnableErrorStrings"))
+return eServerPacketType_QEnableErrorStrings;
   break;
 
 case 'P':
@@ -438,8 +450,8 @@
 }
 
 bool StringExtractorGDBRemote::IsErrorResponse() const {
-  return GetResponseType() == eError && m_packet.size() == 3 &&
- isxdigit(m_packet[1]) && isxdigit(m_packet[2]);
+  return GetResponseType() == eError && isxdigit(m_packet[1]) &&
+ isxdigit(m_packet[2]);
 }
 
 uint8_t StringExtractorGDBRemote::GetError() {
@@ -450,6 +462,23 @@
   return 0;
 }
 
+lldb_private::Status StringExtractorGDBRemote::GetStatus() {
+  lldb_private::Status error;
+  if (GetResponseType() == eError) {
+SetFilePos(1);
+uint8_t errc = GetHexU8(255);
+error.SetError(errc, lldb::eErrorTypeGeneric);
+
+std::string error_messg("Error ");
+error_messg += std::to_string(errc);
+if (GetChar() == ';')
+  GetHexByteString(error_messg);
+
+error.SetErrorString(error_messg);
+  }
+  return error;
+}
+
 size_t StringExtractorGDBRemote::GetEscapedBinaryData(std::string &str) {
   // Just get the data bytes in the string as
   // GDBRemoteCommunication::CheckForPacket()
Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -1031,6 +1031,7 @@
   m_gdb_comm.GetHostInfo();
   m_gdb_comm.GetVContSupported('c');
   m_gdb_comm.GetVAttachOrWaitSupported();
+  m_gdb_comm.EnableErrorStringInPacket();
 
   // Ask the remote server for the default thread id
   if (GetTarget().GetNonStopModeEnabled())
Index: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
===
--- source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -1128,7 +1128,7 @@
   uid = m_debugged_process_sp->StartTrace(options, error);
   LLDB_LOG(log, "uid is {0} , error is {1}", uid, error.GetError());
   if (error.Fail())
-return SendErrorResponse(error.GetError());
+return SendErrorResponse(error);
 
   StreamGDBRemote response;
   response.Printf("%" PRIx64, uid);
@@ -1165,7 +1165,7 @@
   Status error = m_debugged_process_sp->StopTrace(uid, tid);
 
   if (error.Fail())
-

[Lldb-commits] [PATCH] D34945: Adding Support for Error Strings in Remote Packets

2017-07-11 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja added inline comments.



Comment at: 
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp:32-35
+  RegisterPacketHandler(
+  StringExtractorGDBRemote::eServerPacketType_QEnableErrorStrings,
+  [this](StringExtractorGDBRemote packet, Status &error, bool &interrupt,
+ bool &quit) { return this->Handle_QErrorStringEnable(packet); });

clayborg wrote:
> Why is this done here and not where all of the other packets are registered?
I did it here coz this class has the member SendErrorResponse which I wanted to 
overload. Also I think the remote packets are a bit spreadout among inherited 
classes, so it will be available for all these classes and then these won't be 
spread out like packet handling code will be in one place.


https://reviews.llvm.org/D34945



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

[Lldb-commits] [PATCH] D34945: Adding Support for Error Strings in Remote Packets

2017-07-12 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja added a comment.

Uploaded SVN Revision Number - 307773 to fix Android Builder.


https://reviews.llvm.org/D34945



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

[Lldb-commits] [PATCH] D29581: Initial implementation of SB APIs for Tracing support.

2017-03-03 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja updated this revision to Diff 90461.
ravitheja added a comment.
Herald added a subscriber: mgorny.

Changes according to the comments.


https://reviews.llvm.org/D29581

Files:
  include/lldb/API/LLDB.h
  include/lldb/API/SBDefines.h
  include/lldb/API/SBError.h
  include/lldb/API/SBProcess.h
  include/lldb/API/SBStream.h
  include/lldb/API/SBTrace.h
  include/lldb/API/SBTraceOptions.h
  include/lldb/Target/Process.h
  include/lldb/lldb-enumerations.h
  include/lldb/lldb-forward.h
  scripts/interface/SBProcess.i
  scripts/interface/SBTrace.i
  scripts/interface/SBTraceOptions.i
  scripts/lldb.swig
  source/API/CMakeLists.txt
  source/API/SBProcess.cpp
  source/API/SBTrace.cpp
  source/API/SBTraceOptions.cpp

Index: source/API/SBTraceOptions.cpp
===
--- /dev/null
+++ source/API/SBTraceOptions.cpp
@@ -0,0 +1,83 @@
+//===-- SBTraceOptions.cpp --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "lldb/API/SBTraceOptions.h"
+#include "lldb/API/SBError.h"
+#include "lldb/API/SBStream.h"
+#include "lldb/Core/Log.h"
+#include "lldb/Target/Process.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+SBTraceOptions::SBTraceOptions(lldb::TraceType type, uint64_t trace_buffer_size,
+   uint64_t meta_data_buffer_size) {
+  m_traceoptions_sp.reset(
+  new TraceOptions(type, trace_buffer_size, meta_data_buffer_size));
+}
+
+lldb::TraceType SBTraceOptions::getType() const {
+  if (m_traceoptions_sp)
+return m_traceoptions_sp->getType();
+  return lldb::TraceType::eTraceTypeNone;
+}
+
+uint64_t SBTraceOptions::getTraceBufferSize() const {
+  if (m_traceoptions_sp)
+return m_traceoptions_sp->getTraceBufferSize();
+  return 0;
+}
+
+lldb::SBError SBTraceOptions::getTraceParams(lldb::SBStream &ss) {
+  const lldb_private::StructuredData::DictionarySP dict_obj =
+  m_traceoptions_sp->getTraceParams();
+  lldb::SBError error;
+  if (dict_obj) {
+dict_obj->Dump(ss.ref(), false);
+  } else
+error.SetErrorString("Empty trace params");
+  return error;
+}
+
+uint64_t SBTraceOptions::getMetaDataBufferSize() const {
+  if (m_traceoptions_sp)
+return m_traceoptions_sp->getTraceBufferSize();
+  return 0;
+}
+
+void SBTraceOptions::setTraceParams(lldb::SBStream ¶ms) {
+  if (params.GetData() == NULL)
+return;
+  if (m_traceoptions_sp) {
+std::string str(params.GetData());
+m_traceoptions_sp->setTraceParams(str);
+return;
+  }
+}
+
+void SBTraceOptions::setType(lldb::TraceType type) {
+  if (m_traceoptions_sp)
+m_traceoptions_sp->setType(type);
+}
+
+void SBTraceOptions::setTraceBufferSize(uint64_t size) {
+  if (m_traceoptions_sp)
+m_traceoptions_sp->setTraceBufferSize(size);
+}
+
+void SBTraceOptions::setMetaDataBufferSize(uint64_t size) {
+  if (m_traceoptions_sp)
+m_traceoptions_sp->setMetaDataBufferSize(size);
+}
+
+bool SBTraceOptions::IsValid() {
+  if (m_traceoptions_sp)
+return true;
+  return false;
+}
Index: source/API/SBTrace.cpp
===
--- /dev/null
+++ source/API/SBTrace.cpp
@@ -0,0 +1,128 @@
+//===-- SBTrace.cpp -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "lldb/Core/Log.h"
+#include "lldb/Target/Process.h"
+
+#include "lldb/API/SBTrace.h"
+#include "lldb/API/SBTraceOptions.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+class SBTraceImpl {
+public:
+  lldb::user_id_t uid;
+};
+
+lldb::ProcessSP SBTrace::GetSP() const { return m_opaque_wp.lock(); }
+
+size_t SBTrace::GetTraceData(SBError &error, void *buf, size_t size,
+ size_t offset, lldb::tid_t thread_id) {
+  size_t bytes_read = 0;
+  ProcessSP process_sp(GetSP());
+  Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));
+  error.Clear();
+
+  if (log)
+log->Printf("SBProcess:: %s", __FUNCTION__);
+
+  if (!process_sp) {
+error.SetErrorString("invalid process");
+  } else {
+bytes_read = process_sp->GetData(GetTraceUID(), thread_id, error.ref(), buf,
+ size, offset);
+if (log)
+  log->Printf("SBProcess:: %s bytes_read - %" PRIx64, __FUNCTION__,
+  bytes_read);
+  }
+  return bytes_read;
+}
+
+size_t SBTrace::GetMetaData(SBError &error, void *buf, size_t size,
+size_t offset, lldb::tid_t thread_id) {
+  size_t bytes_read = 0;

[Lldb-commits] [PATCH] D29581: Initial implementation of SB APIs for Tracing support.

2017-03-03 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja added inline comments.



Comment at: include/lldb/API/SBProcess.h:269
+  lldb::user_id_t StartTrace(SBTraceOptions &sboptions, lldb::SBError &sberror,
+ lldb::tid_t thread_id = LLDB_INVALID_THREAD_ID);
+

clayborg wrote:
> Should the thread be in the SBTraceOptions? What if you want to trace N 
> threads, but not all? Seems like this would be better represented in the 
> SBTraceOptions.
We wanted to keep some symmetry among the APIs , the threadid could go in the 
TraceOptions as well.



Comment at: include/lldb/API/SBProcess.h:281-288
+  /// @param[in] buf
+  /// Buffer to write the trace data to.
+  ///
+  /// @param[in] size
+  /// The size of the buffer used to read the data. This is
+  /// also the size of the data intended to read. It is also
+  /// possible to partially read the trace data for some trace

clayborg wrote:
> Should we make a SBTraceData object instead of getting a buffer of bytes? I 
> haven't looked past this code to the code below, so I am not sure how this 
> data will be used. Just thinking out loud here.
So the idea we have is that we don't want to interpret the trace data inside 
lldb. Now one could go and make a class SBTraceData but it would just be a 
buffer of trace data, which is why we left it as a buffer of bytes.



Comment at: include/lldb/API/SBProcess.h:290-291
+  ///
+  /// @param[in] offset
+  /// The start offset to begin reading the trace data.
+  ///

clayborg wrote:
> Should the trace data not be a stream? Can you elaborate on why you think you 
> need the offset?
For processor Trace implementation buffer was more suitable for decoding 
purposes. The reason for offset was that it gives the plugin the chance to 
request partial segments of the trace data which it could decode.


https://reviews.llvm.org/D29581



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

[Lldb-commits] [PATCH] D29581: Initial implementation of SB APIs for Tracing support.

2017-03-07 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja added inline comments.



Comment at: include/lldb/API/SBTraceOptions.h:38
+  /// They should be formatted as a JSON Array.
+  void setTraceParams(lldb::SBStream ¶ms);
+

clayborg wrote:
> This should probably be:
> 
> ```
> void setTraceParams(lldb::SBStructuredData ¶ms);
> ```
> 
> Then we can add extra functions to SBStructuredData that allow you construct 
> one from XML, JSON, Apple plist, or any other structured data.
Hi, this would also mean we make SBTraceOptions a friend class of 
SBStructuredData so that we can get access to the StructuredDataObject inside ?


https://reviews.llvm.org/D29581



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

[Lldb-commits] [PATCH] D29581: Initial implementation of SB APIs for Tracing support.

2017-03-10 Thread Ravitheja Addepally via Phabricator via lldb-commits

ravitheja added inline comments.



Comment at: scripts/interface/SBTrace.i:12
+
+class LLDB_API SBTrace {
+public:

clayborg wrote:
> Do we want something in here that explains what kind of trace buffer data you 
> will be getting? I am thinking that even though we know we ask for 
> "eTraceTypeProcessorTrace", this might not be enough for a plug-in to 
> interpret these bytes. Do we need something like:
> 
> ```
> class SBTrace {
> const char *GetTraceDataName();
> };
> ```
> 
> And for intel it might return "intel processor trace version 2.0"? Or Maybe 
> it would. be better as:
> 
> ```
> class SBTrace {
> lldb::SBStructuredData GetTraceDataInfo();
> };
> ```
> 
> This could return data that could be accessed as JSON. The version could be 
> encoded in here. Maybe the exact CPU type, or CPU ID could also be encoded. I 
> am guessing that the intel processor trace has already changed format from 
> CPU to CPU and might also change in the future? This would help the plug-in 
> that will interpret the trace byte to extract them correctly?
Hello, Couldn't this be done in the GetTraceConfig (currently I am doing this) 
, as the TraceOptions already has a StructuredData for custom parameters and 
configurations, any trace technology could just convey any special or trace 
specific configurations in the custom parameters of TraceOptions ?


https://reviews.llvm.org/D29581



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

55 matches

Mail list logo