[Lldb-commits] [lldb] 278df28 - [nfc] [lldb] Rename GetRnglist() to GetRnglistTable()

[Jan Kratochvil via lldb-commits]

Author: Jan Kratochvil
Date: 2021-07-21T10:45:37+02:00
New Revision: 278df28557f138481ac852d2d064d4c27626048a

URL: 
https://github.com/llvm/llvm-project/commit/278df28557f138481ac852d2d064d4c27626048a
DIFF: 
https://github.com/llvm/llvm-project/commit/278df28557f138481ac852d2d064d4c27626048a.diff

LOG: [nfc] [lldb] Rename GetRnglist() to GetRnglistTable()

My D99653 implemented a getter GetRnglist() for m_rnglist_table.

That was confusing as the getter returns DWARFDebugRnglistTable which
contains DWARFDebugRnglist as its elements.

Added: 


Modified: 
lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h

Removed: 




diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
index 62c61ed657f0b..824e43872269b 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
@@ -490,7 +490,8 @@ void DWARFUnit::SetRangesBase(dw_addr_t ranges_base) {
   m_ranges_base = ranges_base;
 }
 
-const llvm::Optional &DWARFUnit::GetRnglist() {
+const llvm::Optional &
+DWARFUnit::GetRnglistTable() {
   if (GetVersion() >= 5 && !m_rnglist_table_done) {
 m_rnglist_table_done = true;
 if (auto table_or_error =
@@ -508,7 +509,7 @@ const llvm::Optional 
&DWARFUnit::GetRnglist() {
 
 // This function is called only for DW_FORM_rnglistx.
 llvm::Expected DWARFUnit::GetRnglistOffset(uint32_t Index) {
-  if (!GetRnglist())
+  if (!GetRnglistTable())
 return llvm::createStringError(errc::invalid_argument,
"missing or invalid range list table");
   if (!m_ranges_base)
@@ -516,14 +517,14 @@ llvm::Expected 
DWARFUnit::GetRnglistOffset(uint32_t Index) {
"DW_FORM_rnglistx cannot be used without "
"DW_AT_rnglists_base for CU at 0x%8.8x",
GetOffset());
-  if (llvm::Optional off = GetRnglist()->getOffsetEntry(
+  if (llvm::Optional off = GetRnglistTable()->getOffsetEntry(
   m_dwarf.GetDWARFContext().getOrLoadRngListsData().GetAsLLVM(), 
Index))
 return *off + m_ranges_base;
   return llvm::createStringError(
   errc::invalid_argument,
   "invalid range list table index %u; OffsetEntryCount is %u, "
   "DW_AT_rnglists_base is %" PRIu64,
-  Index, GetRnglist()->getOffsetEntryCount(), m_ranges_base);
+  Index, GetRnglistTable()->getOffsetEntryCount(), m_ranges_base);
 }
 
 void DWARFUnit::SetStrOffsetsBase(dw_offset_t str_offsets_base) {
@@ -967,11 +968,11 @@ DWARFUnit::FindRnglistFromOffset(dw_offset_t offset) {
 return ranges;
   }
 
-  if (!GetRnglist())
+  if (!GetRnglistTable())
 return llvm::createStringError(errc::invalid_argument,
"missing or invalid range list table");
 
-  auto range_list_or_error = GetRnglist()->findList(
+  auto range_list_or_error = GetRnglistTable()->findList(
   m_dwarf.GetDWARFContext().getOrLoadRngListsData().GetAsLLVM(), offset);
   if (!range_list_or_error)
 return range_list_or_error.takeError();

diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
index f6b1fdbde0710..da79a6aaf64e5 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
@@ -283,7 +283,7 @@ class DWARFUnit : public lldb_private::UserID {
 return &m_die_array[0];
   }
 
-  const llvm::Optional &GetRnglist();
+  const llvm::Optional &GetRnglistTable();
 
   SymbolFileDWARF &m_dwarf;
   std::shared_ptr m_dwo;



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

[Lldb-commits] [PATCH] D105741: [trace] Add `thread trace export` command for Intel PT trace visualization

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added inline comments.



Comment at: lldb/source/Target/TraceHTR.cpp:205-217
+  // Why does using `IHTRLayer ¤t_layer = m_instruction_layer;` not work?
+  HTRInstructionLayer instruction_layer = m_instruction_layer;
+  HTRBlockLayer current_layer = BasicSuperBlockMerge(instruction_layer);
+  if (instruction_layer.GetNumUnits() == current_layer.GetNumUnits())
+return;
+  AddNewBlockLayer(current_layer);
+  while (true) {

Any idea why the "Edit" I made above does not work?
I tried this first but after using lldb to understand why it wasn't working, it 
appeared that `current_layer = new_block_layer` wasn't properly updating the 
value stored in `current_layer`. I thought that using a reference to an 
abstract class as the type for `current_layer` would allow this variable to 
bind to any value that implements the interface, but after changing the code to 
only use concrete types (the non-edited version of this code), the code behaves 
as expected. 


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

https://reviews.llvm.org/D105741

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

[Lldb-commits] [lldb] 345ace0 - [trace] [intel pt] Create a "thread trace dump stats" command

[Walter Erquinigo via lldb-commits]

Author: Walter Erquinigo
Date: 2021-07-21T09:50:15-07:00
New Revision: 345ace026b6e5cdbc38d207291e4b399d72e62ee

URL: 
https://github.com/llvm/llvm-project/commit/345ace026b6e5cdbc38d207291e4b399d72e62ee
DIFF: 
https://github.com/llvm/llvm-project/commit/345ace026b6e5cdbc38d207291e4b399d72e62ee.diff

LOG: [trace] [intel pt] Create a "thread trace dump stats" command

When the user types that command 'thread trace dump info' and there's a running 
Trace session in LLDB, a raw trace in bytes should be printed; the command 
'thread trace dump info all' should print the info for all the threads.

Original Author: hanbingwang

Reviewed By: clayborg, wallace

Differential Revision: https://reviews.llvm.org/D105717

Added: 
lldb/test/API/commands/trace/TestTraceDumpInfo.py

Modified: 
lldb/include/lldb/Target/Trace.h
lldb/source/Commands/CommandObjectThread.cpp
lldb/source/Commands/Options.td
lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
lldb/test/API/commands/trace/TestTraceLoad.py

Removed: 




diff  --git a/lldb/include/lldb/Target/Trace.h 
b/lldb/include/lldb/Target/Trace.h
index c82d17b029557..f5654988b2015 100644
--- a/lldb/include/lldb/Target/Trace.h
+++ b/lldb/include/lldb/Target/Trace.h
@@ -140,6 +140,20 @@ class Trace : public PluginInterface,
   /// trace.
   virtual lldb::TraceCursorUP GetCursor(Thread &thread) = 0;
 
+  /// Dump general info about a given thread's trace. Each Trace plug-in
+  /// decides which data to show.
+  ///
+  /// \param[in] thread
+  /// The thread that owns the trace in question.
+  ///
+  /// \param[in] s
+  /// The stream object where the info will be printed printed.
+  ///
+  /// \param[in] verbose
+  /// If \b true, print detailed info
+  /// If \b false, print compact info
+  virtual void DumpTraceInfo(Thread &thread, Stream &s, bool verbose) = 0;
+
   /// Check if a thread is currently traced by this object.
   ///
   /// \param[in] thread

diff  --git a/lldb/source/Commands/CommandObjectThread.cpp 
b/lldb/source/Commands/CommandObjectThread.cpp
index 2f8772953af40..3b58d71048739 100644
--- a/lldb/source/Commands/CommandObjectThread.cpp
+++ b/lldb/source/Commands/CommandObjectThread.cpp
@@ -2138,6 +2138,83 @@ class CommandObjectTraceDumpInstructions
   std::map> m_dumpers;
 };
 
+// CommandObjectTraceDumpInfo
+#define LLDB_OPTIONS_thread_trace_dump_info
+#include "CommandOptions.inc"
+
+class CommandObjectTraceDumpInfo : public CommandObjectIterateOverThreads {
+public:
+  class CommandOptions : public Options {
+  public:
+CommandOptions() : Options() { OptionParsingStarting(nullptr); }
+
+~CommandOptions() override = default;
+
+Status SetOptionValue(uint32_t option_idx, llvm::StringRef option_arg,
+  ExecutionContext *execution_context) override {
+  Status error;
+  const int short_option = m_getopt_table[option_idx].val;
+
+  switch (short_option) {
+  case 'v': {
+m_verbose = true;
+break;
+  }
+  default:
+llvm_unreachable("Unimplemented option");
+  }
+  return error;
+}
+
+void OptionParsingStarting(ExecutionContext *execution_context) override {
+  m_verbose = false;
+}
+
+llvm::ArrayRef GetDefinitions() override {
+  return llvm::makeArrayRef(g_thread_trace_dump_info_options);
+}
+
+// Instance variables to hold the values for command options.
+bool m_verbose;
+  };
+
+  bool DoExecute(Args &command, CommandReturnObject &result) override {
+Target &target = m_exe_ctx.GetTargetRef();
+result.GetOutputStream().Printf(
+"Trace technology: %s\n",
+target.GetTrace()->GetPluginName().AsCString());
+return CommandObjectIterateOverThreads::DoExecute(command, result);
+  }
+
+  CommandObjectTraceDumpInfo(CommandInterpreter &interpreter)
+  : CommandObjectIterateOverThreads(
+interpreter, "thread trace dump info",
+"Dump the traced information for one or more threads.  If no "
+"threads are specified, show the current thread.  Use the "
+"thread-index \"all\" to see all threads.",
+nullptr,
+eCommandRequiresProcess | eCommandTryTargetAPILock |
+eCommandProcessMustBeLaunched | eCommandProcessMustBePaused |
+eCommandProcessMustBeTraced),
+m_options() {}
+
+  ~CommandObjectTraceDumpInfo() override = default;
+
+  Options *GetOptions() override { return &m_options; }
+
+protected:
+  bool HandleOneThread(lldb::tid_t tid, CommandReturnObject &result) override {
+const TraceSP &trace_sp = m_exe_ctx.GetTargetSP()->GetTrace();
+ThreadSP thread_sp 

[Lldb-commits] [PATCH] D105717: [trace] [intel pt] Create a "thread trace dump stats" command

[Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG345ace026b6e: [trace] [intel pt] Create a "thread trace 
dump stats" command (authored by Walter Erquinigo ).

Changed prior to commit:
  https://reviews.llvm.org/D105717?vs=360044&id=360501#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105717

Files:
  lldb/include/lldb/Target/Trace.h
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Commands/Options.td
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
  lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/test/API/commands/trace/TestTraceDumpInfo.py
  lldb/test/API/commands/trace/TestTraceLoad.py

Index: lldb/test/API/commands/trace/TestTraceLoad.py
===
--- lldb/test/API/commands/trace/TestTraceLoad.py
+++ lldb/test/API/commands/trace/TestTraceLoad.py
@@ -33,7 +33,10 @@
 # check that the Process and Thread objects were created correctly
 self.expect("thread info", substrs=["tid = 3842849"])
 self.expect("thread list", substrs=["Process 1234 stopped", "tid = 3842849"])
+self.expect("thread trace dump info", substrs=['''Trace technology: intel-pt
 
+thread #1: tid = 3842849
+  Raw trace size: 4096 bytes'''])
 
 def testLoadInvalidTraces(self):
 src_dir = self.getSourceDir()
Index: lldb/test/API/commands/trace/TestTraceDumpInfo.py
===
--- /dev/null
+++ lldb/test/API/commands/trace/TestTraceDumpInfo.py
@@ -0,0 +1,41 @@
+import lldb
+from intelpt_testcase import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+from lldbsuite.test.decorators import *
+
+class TestTraceDumpInfo(TraceIntelPTTestCaseBase):
+mydir = TestBase.compute_mydir(__file__)
+
+def testErrorMessages(self):
+# We first check the output when there are no targets
+self.expect("thread trace dump info",
+substrs=["error: invalid target, create a target using the 'target create' command"],
+error=True)
+
+# We now check the output when there's a non-running target
+self.expect("target create " +
+os.path.join(self.getSourceDir(), "intelpt-trace", "a.out"))
+
+self.expect("thread trace dump info",
+substrs=["error: invalid process"],
+error=True)
+
+# Now we check the output when there's a running target without a trace
+self.expect("b main")
+self.expect("run")
+
+self.expect("thread trace dump info",
+substrs=["error: Process is not being traced"],
+error=True)
+
+def testDumpRawTraceSize(self):
+self.expect("trace load -v " +
+os.path.join(self.getSourceDir(), "intelpt-trace", "trace.json"),
+substrs=["intel-pt"])
+
+self.expect("thread trace dump info",
+substrs=['''Trace technology: intel-pt
+
+thread #1: tid = 3842849
+  Raw trace size: 4096 bytes'''])
Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
@@ -67,6 +67,10 @@
 
   lldb::TraceCursorUP GetCursor(Thread &thread) override;
 
+  void DumpTraceInfo(Thread &thread, Stream &s, bool verbose) override;
+
+  llvm::Optional GetRawTraceSize(Thread &thread);
+
   void DoRefreshLiveProcessState(
   llvm::Expected state) override;
 
Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
@@ -108,6 +108,24 @@
   return Decode(thread)->GetCursor();
 }
 
+void TraceIntelPT::DumpTraceInfo(Thread &thread, Stream &s, bool verbose) {
+  Optional raw_size = GetRawTraceSize(thread);
+  s.Printf("\nthread #%u: tid = %" PRIu64, thread.GetIndexID(), thread.GetID());
+  if (!raw_size) {
+s.Printf(", not traced\n");
+return;
+  }
+  s.Printf("\n  Raw trace size: %zu bytes\n", *raw_size);
+  return;
+}
+
+Optional TraceIntelPT::GetRawTraceSize(Thread &thread) {
+  if (IsTraced(thread))
+return Decode(thread)->GetRawTraceSize();
+  else
+return None;
+}
+
 Expected TraceIntelPT::GetCPUInfoForLiveProcess() {
   Expected> cpu_info = GetLiveProcessBinaryData("cpuInfo");
   if (!cpu_info)
Index: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
===
--- lldb/source/Plugins/Trace/intel-

[Lldb-commits] [PATCH] D106458: [LLDB][GUI] Add required property to text fields

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev created this revision.
OmarEmaraDev added a reviewer: clayborg.
Herald added a reviewer: teemperor.
OmarEmaraDev requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This patch adds a required property to text fields and their
derivatives. Additionally, the Process Name and PID fields in the attach
form were marked as required.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D106458

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1075,8 +1075,10 @@
 
 class TextFieldDelegate : public FieldDelegate {
 public:
-  TextFieldDelegate(const char *label, const char *content)
-  : m_label(label), m_cursor_position(0), m_first_visibile_char(0) {
+  TextFieldDelegate(const char *label, const char *content,
+bool required = false)
+  : m_label(label), m_required(required), m_cursor_position(0),
+m_first_visibile_char(0) {
 if (content)
   m_content = content;
   }
@@ -1238,6 +1240,13 @@
 return eKeyNotHandled;
   }
 
+  void FieldDelegateExitCallback() override {
+if (!IsSpecified() && m_required)
+  SetError("This field is required!");
+  }
+
+  bool IsSpecified() { return !m_content.empty(); }
+
   bool HasError() { return !m_error.empty(); }
 
   void ClearError() { m_error.clear(); }
@@ -1250,6 +1259,7 @@
 
 protected:
   std::string m_label;
+  bool m_required;
   // The position of the top left corner character of the border.
   std::string m_content;
   // The cursor position in the content string itself. Can be in the range
@@ -1266,8 +1276,8 @@
 
 class IntegerFieldDelegate : public TextFieldDelegate {
 public:
-  IntegerFieldDelegate(const char *label, int content)
-  : TextFieldDelegate(label, std::to_string(content).c_str()) {}
+  IntegerFieldDelegate(const char *label, int content, bool required = false)
+  : TextFieldDelegate(label, std::to_string(content).c_str(), required) {}
 
   // Only accept digits.
   bool IsAcceptableChar(int key) override { return isdigit(key); }
@@ -1279,12 +1289,15 @@
 class FileFieldDelegate : public TextFieldDelegate {
 public:
   FileFieldDelegate(const char *label, const char *content,
-bool need_to_exist = true)
-  : TextFieldDelegate(label, content), m_need_to_exist(need_to_exist) {}
+bool need_to_exist = true, bool required = false)
+  : TextFieldDelegate(label, content, required),
+m_need_to_exist(need_to_exist) {}
 
-  // Set appropriate error messages if the file doesn't exists or is, in fact, a
-  // directory.
   void FieldDelegateExitCallback() override {
+TextFieldDelegate::FieldDelegateExitCallback();
+if (!IsSpecified())
+  return;
+
 FileSpec file(GetPath());
 if (m_need_to_exist && !FileSystem::Instance().Exists(file)) {
   SetError("File doesn't exist!");
@@ -1306,12 +1319,15 @@
 class DirectoryFieldDelegate : public TextFieldDelegate {
 public:
   DirectoryFieldDelegate(const char *label, const char *content,
- bool need_to_exist = true)
-  : TextFieldDelegate(label, content), m_need_to_exist(need_to_exist) {}
+ bool need_to_exist = true, bool required = false)
+  : TextFieldDelegate(label, content, required),
+m_need_to_exist(need_to_exist) {}
 
-  // Set appropriate error messages if the directory doesn't exists or is, in
-  // fact, a file.
   void FieldDelegateExitCallback() override {
+TextFieldDelegate::FieldDelegateExitCallback();
+if (!IsSpecified())
+  return;
+
 FileSpec file(GetPath());
 if (m_need_to_exist && !FileSystem::Instance().Exists(file)) {
   SetError("Directory doesn't exist!");
@@ -1856,31 +1872,37 @@
 
   // Factory methods to create and add fields of specific types.
 
-  TextFieldDelegate *AddTextField(const char *label, const char *content) {
-TextFieldDelegate *delegate = new TextFieldDelegate(label, content);
+  TextFieldDelegate *AddTextField(const char *label, const char *content,
+  bool required = false) {
+TextFieldDelegate *delegate =
+new TextFieldDelegate(label, content, required);
 m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
   FileFieldDelegate *AddFileField(const char *label, const char *content,
-  bool need_to_exist = true) {
+  bool need_to_exist = true,
+  bool required = false) {
 FileFieldDelegate *delegate =
-new FileFieldDelegate(label, content, need_to_exist);
+new FileFieldDelegate(label, content, need_to_exist, required);
 m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;

[Lldb-commits] [PATCH] D105717: [trace] [intel pt] Create a "thread trace dump stats" command

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

Committed on behalf of @hanbingwang. Commit hash 
345ace026b6e5cdbc38d207291e4b399d72e62ee 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105717

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

[Lldb-commits] [PATCH] D106459: [LLDB][GUI] Check fields validity in actions

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev created this revision.
OmarEmaraDev added a reviewer: clayborg.
Herald added a reviewer: teemperor.
OmarEmaraDev requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This patch adds a virtual method HasError to fields, it can be
overridden by fields that have errors. Additionally, a form method
CheckFieldsValidity was added to be called by actions that expects all
the field to be valid.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D106459

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp


Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1061,6 +1061,9 @@
   // Select the last element in the field if multiple elements exists.
   virtual void FieldDelegateSelectLastElement() { return; }
 
+  // Returns true if the field has an error, false otherwise.
+  virtual bool FieldDelegateHasError() { return false; }
+
   bool FieldDelegateIsVisible() { return m_is_visible; }
 
   void FieldDelegateHide() { m_is_visible = false; }
@@ -1097,7 +1100,7 @@
   // field and an optional line for an error if it exists.
   int FieldDelegateGetHeight() override {
 int height = GetFieldHeight();
-if (HasError())
+if (FieldDelegateHasError())
   height++;
 return height;
   }
@@ -1137,7 +1140,7 @@
   }
 
   void DrawError(SubPad &surface) {
-if (!HasError())
+if (!FieldDelegateHasError())
   return;
 surface.MoveCursor(0, 0);
 surface.AttributeOn(COLOR_PAIR(RedOnBlack));
@@ -1238,7 +1241,7 @@
 return eKeyNotHandled;
   }
 
-  bool HasError() { return !m_error.empty(); }
+  bool FieldDelegateHasError() override { return !m_error.empty(); }
 
   void ClearError() { m_error.clear(); }
 
@@ -1854,6 +1857,19 @@
 
   void SetError(const char *error) { m_error = error; }
 
+  // If all fields are valid, true is returned. Otherwise, an error message is
+  // set and false is returned. This method is usually called at the start of 
an
+  // action that requires valid fields.
+  bool CheckFieldsValidity() {
+for (int i = 0; i < GetNumberOfFields(); i++) {
+  if (GetField(i)->FieldDelegateHasError()) {
+SetError("Some fields are invalid!");
+return false;
+  }
+}
+return true;
+  }
+
   // Factory methods to create and add fields of specific types.
 
   TextFieldDelegate *AddTextField(const char *label, const char *content) {
@@ -2464,6 +2480,10 @@
   void Attach(Window &window) {
 ClearError();
 
+bool all_fields_are_valid = CheckFieldsValidity();
+if (!all_fields_are_valid)
+  return;
+
 bool process_is_running = StopRunningProcess();
 if (process_is_running)
   return;


Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1061,6 +1061,9 @@
   // Select the last element in the field if multiple elements exists.
   virtual void FieldDelegateSelectLastElement() { return; }
 
+  // Returns true if the field has an error, false otherwise.
+  virtual bool FieldDelegateHasError() { return false; }
+
   bool FieldDelegateIsVisible() { return m_is_visible; }
 
   void FieldDelegateHide() { m_is_visible = false; }
@@ -1097,7 +1100,7 @@
   // field and an optional line for an error if it exists.
   int FieldDelegateGetHeight() override {
 int height = GetFieldHeight();
-if (HasError())
+if (FieldDelegateHasError())
   height++;
 return height;
   }
@@ -1137,7 +1140,7 @@
   }
 
   void DrawError(SubPad &surface) {
-if (!HasError())
+if (!FieldDelegateHasError())
   return;
 surface.MoveCursor(0, 0);
 surface.AttributeOn(COLOR_PAIR(RedOnBlack));
@@ -1238,7 +1241,7 @@
 return eKeyNotHandled;
   }
 
-  bool HasError() { return !m_error.empty(); }
+  bool FieldDelegateHasError() override { return !m_error.empty(); }
 
   void ClearError() { m_error.clear(); }
 
@@ -1854,6 +1857,19 @@
 
   void SetError(const char *error) { m_error = error; }
 
+  // If all fields are valid, true is returned. Otherwise, an error message is
+  // set and false is returned. This method is usually called at the start of an
+  // action that requires valid fields.
+  bool CheckFieldsValidity() {
+for (int i = 0; i < GetNumberOfFields(); i++) {
+  if (GetField(i)->FieldDelegateHasError()) {
+SetError("Some fields are invalid!");
+return false;
+  }
+}
+return true;
+  }
+
   // Factory methods to create and add fields of specific types.
 
   TextFieldDelegate *AddTextField(const char *label, const char *content) {
@@ -2464,6 +2480,10 @@
   void Attach(Window &window) {
 ClearError();
 
+bool all_fields_are_valid = CheckFieldsValidity();
+if (!all_fields_are_valid)
+  return;
+

[Lldb-commits] [PATCH] D106192: [LLDB][GUI] Add Create Target form

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.

I am currently breaking this patch into smaller independent viable patches as 
suggested.




Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1309
+FileSpec file_spec(m_content);
+FileSystem::Instance().Resolve(file_spec);
+return file_spec;

clayborg wrote:
> It should be a property of the FileFieldDelegate if the file is for the 
> current system. We might be specifying a file on a remote system, so it 
> doesn't always have to exist, nor should it be resolved on the current 
> system. When creating a target we would always want to specify this file 
> should exist on the current system. 
> 
> 
I was trying to add properties that indicates if the file is local or not and 
if it should be resolved or not. But they all seems identical to the 
need_to_exist property. If m_need_to_exist is false, then the file will not be 
resolved and will not be checked with the host file system (Which is what we 
want to remote files). If it is true, then it will be resolved and checked with 
the host file system (Which is what we want for local files). So adding more 
properties might be redundant? What do you think?



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1315
   bool m_need_to_exist;
+  bool m_required;
 };

clayborg wrote:
> Should this to into the FieldDelegate so we don't need to put it into both 
> FileFieldDelegate and DirectoryFieldDelegate? Then the default 
> FieldDelegate::FieldDelegateExitCallback() function could be:
> 
> ```
> virtual void FieldDelegate::FieldDelegateExitCallback() {
>   if (!m_required)
> return;
>   // Check value with another virtual function?
>   FieldDelegateValueIsValid();
> }
> ```
> This seems like many fields would want to require being set and we don't want 
> to re-invent this for each kind of field.
The fields that can have a required property is the TextField and its 
derivatives (File, Directory, and Number), so we can can add this logic there. 
This is implemented in D106458.



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2609
+
+if (!FileSystem::Instance().Exists(file_spec)) {
+  SetError("File doesn't exist!");

clayborg wrote:
> Won't this be taken care of already by the File field? We should construct 
> the FileDelegate so that:
> - the file must exist
> - the file gets resolved on the local system
> - the file field is required
> This way, the only way for the user to get to the "Create" button if is if 
> they already were able to set a valid and required by the FileFieldDelegate 
> class.
D106459 implements the necessary bits to allow for this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106192

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

[Lldb-commits] [PATCH] D106192: [LLDB][GUI] Add Create Target form

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.

Thanks @DavidSpickett! I will look into this and let you know how it goes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106192

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

[Lldb-commits] [PATCH] D106466: [llvm+lldb] 2/2: Fix#2 of DW_AT_ranges DW_FORM_sec_offset not using DW_AT_rnglists_base (used by GCC)

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil created this revision.
jankratochvil added reviewers: dblaikie, labath, clayborg.
jankratochvil added projects: LLDB, LLVM.
Herald added subscribers: JDevlieghere, hiraditya.
jankratochvil requested review of this revision.

Fix D98289  so that it works even for 2nd..nth 
compilation unit (`.debug_rnglists`).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D106466

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
  lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s
  llvm/include/llvm/DebugInfo/DWARF/DWARFListTable.h
  llvm/lib/DebugInfo/DWARF/DWARFListTable.cpp

Index: llvm/lib/DebugInfo/DWARF/DWARFListTable.cpp
===
--- llvm/lib/DebugInfo/DWARF/DWARFListTable.cpp
+++ llvm/lib/DebugInfo/DWARF/DWARFListTable.cpp
@@ -29,6 +29,32 @@
   return Error::success();
 }
 
+Error DWARFListTableHeader::create(DWARFDataExtractor Data, unsigned Version,
+   unsigned AddrSize,
+   dwarf::DwarfFormat FormatParam) {
+  Format = FormatParam;
+  HeaderOffset = 0;
+  if (Data.size() < getHeaderSize(Format))
+return createStringError(errc::invalid_argument,
+ "%s table at offset 0x%" PRIx64
+ " has too small length (0x%" PRIx64
+ ") to contain a complete header",
+ SectionName.data(), HeaderOffset, Data.size());
+
+  HeaderData.Length = Data.size() - dwarf::getUnitLengthFieldByteSize(Format);
+  HeaderData.Version = Version;
+  HeaderData.AddrSize = AddrSize;
+  HeaderData.SegSize = 0;
+  HeaderData.OffsetEntryCount = 0;
+  if (Error Err = HeaderData.verify())
+return createStringError(
+errc::invalid_argument, "parsing %s table at offset 0x%" PRIx64 ": %s",
+SectionName.data(), HeaderOffset, toString(std::move(Err)).c_str());
+
+  Data.setAddressSize(HeaderData.AddrSize);
+  return Error::success();
+}
+
 Error DWARFListTableHeader::extract(DWARFDataExtractor Data,
 uint64_t *OffsetPtr) {
   HeaderOffset = *OffsetPtr;
Index: llvm/include/llvm/DebugInfo/DWARF/DWARFListTable.h
===
--- llvm/include/llvm/DebugInfo/DWARF/DWARFListTable.h
+++ llvm/include/llvm/DebugInfo/DWARF/DWARFListTable.h
@@ -136,6 +136,10 @@
   /// Extract the table header and the array of offsets.
   Error extract(DWARFDataExtractor Data, uint64_t *OffsetPtr);
 
+  // Make up a header from DWARFUnit header to cover the whole Data section.
+  Error create(DWARFDataExtractor Data, unsigned Version, unsigned AddrSize,
+   dwarf::DwarfFormat FormatParam);
+
   /// Returns the length of the table, including the length field, or 0 if the
   /// length has not been determined (e.g. because the table has not yet been
   /// parsed, or there was a problem in parsing).
@@ -170,6 +174,11 @@
   Error extractHeaderAndOffsets(DWARFDataExtractor Data, uint64_t *OffsetPtr) {
 return Header.extract(Data, OffsetPtr);
   }
+  // Make up a header from DWARFUnit header to cover the whole Data section.
+  Error createHeaderAndOffsets(DWARFDataExtractor Data, unsigned Version,
+   unsigned AddrSize, dwarf::DwarfFormat Format) {
+return Header.create(Data, Version, AddrSize, Format);
+  }
   /// Extract an entire table, including all list entries.
   Error extract(DWARFDataExtractor Data, uint64_t *OffsetPtr);
   /// Look up a list based on a given offset. Extract it and enter it into the
Index: lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s
===
--- lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s
+++ lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s
@@ -29,7 +29,7 @@
 # RUN:   -o exit 2>&1 | FileCheck --check-prefix=RNGLISTBASE %s
 
 # RNGLISTBASE-LABEL: image lookup -v -s lookup_rnglists
-# RNGLISTBASE: error: {{.*}}-rnglistbase {0x0043}: DIE has DW_AT_ranges(DW_FORM_rnglistx 0x0) attribute, but range extraction failed (invalid range list table index 0; OffsetEntryCount is 0, DW_AT_rnglists_base is 12), please file a bug and attach the file at the start of this error message
+# RNGLISTBASE: error: {{.*}}-rnglistbase {0x0043}: DIE has DW_AT_ranges(DW_FORM_rnglistx 0x0) attribute, but range extraction failed (invalid range list table index 0; OffsetEntryCount is 0, DW_AT_rnglists_base is 24), please file a bug and attach the file at the start of this error message
 
 .text
 rnglists:
@@ -97,7 +97,7 @@
 .long   .Lrnglists_end-rnglists # DW_AT_high_pc
 .long   .Laddr_table_base0  # DW_AT_addr_base
 .ifdef RNGLISTBASE
-.long   .Ldebug_ranges0 # DW_AT_rnglists_base
+.lon

[Lldb-commits] [PATCH] D106467: [LLDB][GUI] Add Process Plugin Field

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev created this revision.
OmarEmaraDev added a reviewer: clayborg.
Herald added a reviewer: teemperor.
OmarEmaraDev requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This patch adds a new Process Plugin Field. It is a choices field that
lists all the available process plugins and can retrieve the name of the
selected plugin or an empty string if the default is selected.

The Attach form now uses that field instead of manually creating a
choices field.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D106467

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp


Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1504,6 +1504,29 @@
   int m_first_visibile_choice;
 };
 
+class ProcessPluginFieldDelegate : public ChoicesFieldDelegate {
+public:
+  ProcessPluginFieldDelegate()
+  : ChoicesFieldDelegate("Process Plugin", 3, GetPossiblePluginNames()) {}
+
+  std::vector GetPossiblePluginNames() {
+std::vector names;
+names.push_back("");
+
+size_t i = 0;
+while (auto name = PluginManager::GetProcessPluginNameAtIndex(i++))
+  names.push_back(name);
+return names;
+  }
+
+  std::string GetPluginName() {
+std::string plugin_name = GetChoiceContent();
+if (plugin_name == "")
+  return "";
+return plugin_name;
+  }
+};
+
 template  class ListFieldDelegate : public FieldDelegate {
 public:
   ListFieldDelegate(const char *label, T default_field)
@@ -1899,6 +1922,12 @@
 return delegate;
   }
 
+  ProcessPluginFieldDelegate *AddProcessPluginField() {
+ProcessPluginFieldDelegate *delegate = new ProcessPluginFieldDelegate();
+m_fields.push_back(FieldDelegateUP(delegate));
+return delegate;
+  }
+
   template 
   ListFieldDelegate *AddListField(const char *label, T default_field) {
 ListFieldDelegate *delegate =
@@ -2347,8 +2376,7 @@
 m_include_existing_field =
 AddBooleanField("Include existing processes.", false);
 m_show_advanced_field = AddBooleanField("Show advanced settings.", false);
-m_plugin_field =
-AddChoicesField("Plugin Name", 3, GetPossiblePluginNames());
+m_plugin_field = AddProcessPluginField();
 
 AddAction("Attach", [this](Window &window) { Attach(window); });
   }
@@ -2390,16 +2418,6 @@
 return module_sp->GetFileSpec().GetFilename().AsCString();
   }
 
-  std::vector GetPossiblePluginNames() {
-std::vector names;
-names.push_back("");
-
-size_t i = 0;
-while (auto name = PluginManager::GetProcessPluginNameAtIndex(i++))
-  names.push_back(name);
-return names;
-  }
-
   bool StopRunningProcess() {
 ExecutionContext exe_ctx =
 m_debugger.GetCommandInterpreter().GetExecutionContext();
@@ -2455,8 +2473,7 @@
 } else {
   attach_info.SetProcessID(m_pid_field->GetInteger());
 }
-if (m_plugin_field->GetChoiceContent() != "")
-  attach_info.SetProcessPluginName(m_plugin_field->GetChoiceContent());
+attach_info.SetProcessPluginName(m_plugin_field->GetPluginName());
 
 return attach_info;
   }
@@ -2504,7 +2521,7 @@
   BooleanFieldDelegate *m_wait_for_field;
   BooleanFieldDelegate *m_include_existing_field;
   BooleanFieldDelegate *m_show_advanced_field;
-  ChoicesFieldDelegate *m_plugin_field;
+  ProcessPluginFieldDelegate *m_plugin_field;
 };
 
 class MenuDelegate {


Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1504,6 +1504,29 @@
   int m_first_visibile_choice;
 };
 
+class ProcessPluginFieldDelegate : public ChoicesFieldDelegate {
+public:
+  ProcessPluginFieldDelegate()
+  : ChoicesFieldDelegate("Process Plugin", 3, GetPossiblePluginNames()) {}
+
+  std::vector GetPossiblePluginNames() {
+std::vector names;
+names.push_back("");
+
+size_t i = 0;
+while (auto name = PluginManager::GetProcessPluginNameAtIndex(i++))
+  names.push_back(name);
+return names;
+  }
+
+  std::string GetPluginName() {
+std::string plugin_name = GetChoiceContent();
+if (plugin_name == "")
+  return "";
+return plugin_name;
+  }
+};
+
 template  class ListFieldDelegate : public FieldDelegate {
 public:
   ListFieldDelegate(const char *label, T default_field)
@@ -1899,6 +1922,12 @@
 return delegate;
   }
 
+  ProcessPluginFieldDelegate *AddProcessPluginField() {
+ProcessPluginFieldDelegate *delegate = new ProcessPluginFieldDelegate();
+m_fields.push_back(FieldDelegateUP(delegate));
+return delegate;
+  }
+
   template 
   ListFieldDelegate *AddListField(const char *label, T default_field) {
 ListFieldDelegate *delegate =
@@ -2347,8 +2376,7 @@
 m_include_existing_field =
 AddBooleanField("Include existing proc

[Lldb-commits] [PATCH] D106355: [DWARF5] Only fallback to manual index if no entry was found

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added a comment.

Is it really just a microoptimization or can you measure any improvement? That 
`ManualDWARFIndex::Index` will be called is expected. But there should be 
`m_units_to_avoid` covering all the units so it will quickly return without 
indexing anything:

  if (units_to_index.empty())
return;

That is what happens for me on a trivial example `clang -o mainvar mainvar.c 
-Wall -gdwarf-5 -gpubnames`:

 58   if (units_to_index.empty())
  -> 59 return;

Maybe there is rather some other more serious bug to fix (I am aware for 
example D99850  but that would behave 
differently).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106355

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

[Lldb-commits] [PATCH] D105741: [trace] Add `thread trace export` command for Intel PT trace visualization

[walter erquinigo via Phabricator via lldb-commits]

wallace requested changes to this revision.
wallace added a comment.
This revision now requires changes to proceed.

Much better! We are getting closer. There's documentation missing. Besides, you 
should use unique_ptrs as mentioned in the comments so that you prevent copying 
massively large objects. I also left some indications on how to deal with 
errors.




Comment at: lldb/docs/htr.rst:1
+Hierarchical Trace Representation (HTR)
+==

jj10306 wrote:
> @clayborg @wallace Is there a way to view the "rendered" rst to ensure the 
> the format is as I intended and that the image links are working?
TL;DR: just do the small fix I mention below

you can use http://rst.ninjs.org/ to check the rendering. According to it you 
need to make the small fix I mention below.

Regarding the image, well, probably you could build the documentation locally 
and see that everything is working. If you look at docs/CMakeLists.txt, you'll 
see that there's a target called lldb-cpp-doc, but you need to enable doxygen 
first.

However, some parts of the documentation are kept within the tree and not 
exposed in the lldb website. For example, if you look at 
docs/lldb-gdb-remote.txt, it's not shown anywhere in 
https://lldb.llvm.org/index.html. So if we keep this documentation internal, 
this file is okay as it is, as the reader can open the image manually if 
needed. Once all of tracing becomes more stable and with more features, we can 
start making documentation like this more visible in the website, but that 
won't happen soon.



Comment at: lldb/docs/htr.rst:42-45
+- For each block, compute the number of distinct predecessor and successor 
blocks.
+ - **Predecessor** - the block that occurs directly before (to the left of) 
the current block
+ - **Successor** - the block that occurs directly after (to the right of) the 
current block
+- A block with more than one distinct successor is always the start of a super 
block, the super block will continue until the next block with more than one 
distinct predecessor or successor.

you need some blank lines



Comment at: lldb/include/lldb/Target/Trace.h:66-68
+  /// \param[in] count
+  /// The number of trace instructions to include in the CTF dump
+  ///

remove this and add an entry for export_format



Comment at: lldb/include/lldb/Target/TraceHTR.h:1-9
+//===-- TraceHTR.h 
===//
+//
+// Hierarchical Trace Representation (HTR) - see lldb/docs/htr.rst for
+// comprehensive HTR documentation// Part of the LLVM Project, under the Apache
+// License v2.0 with LLVM Exceptions.// See https://llvm.org/LICENSE.txt for
+// license information.// SPDX-License-Identifier: Apache-2.0 WITH
+// LLVM-exception





Comment at: lldb/include/lldb/Target/TraceHTR.h:26-36
+  HTRBlockMetadata(lldb::addr_t first_instruction_load_address,
+   size_t num_instructions,
+   llvm::DenseMap &func_calls)
+  : m_first_instruction_load_address(first_instruction_load_address),
+m_num_instructions(num_instructions), m_func_calls(func_calls) {}
+  static HTRBlockMetadata MergeMetadata(HTRBlockMetadata const &m1,
+HTRBlockMetadata const &m2);

leave spaces between functions for readability and add documentation for all 
the methods



Comment at: lldb/include/lldb/Target/TraceHTR.h:45
+/// \class HTRBlock TraceHTR.h "lldb/Target/TraceHTR.h"
+/// Block structure representing a sequence of trace "units" (ie instructions)
+/// Sequences of blocks are merged to create a new, single block

add a period . after the parenthesis



Comment at: lldb/include/lldb/Target/TraceHTR.h:50-54
+  HTRBlock(size_t offset, size_t size, HTRBlockMetadata metadata)
+  : m_offset(offset), m_size(size), m_metadata(metadata) {}
+  size_t GetOffset() const;
+  size_t GetSize() const;
+  HTRBlockMetadata const &GetMetadata() const;

spaces between methods and documentation



Comment at: lldb/include/lldb/Target/TraceHTR.h:57-63
+  /// Offset in the previous layer
+  size_t m_offset;
+  /// Number of blocks/instructions that make up this block in the previous
+  /// layer
+  size_t m_size;
+  /// General metadata for this block
+  HTRBlockMetadata m_metadata;

leave this comments here, but generally we try to make the documentation of the 
API methods more verbose than the private members, which are rarely seen



Comment at: lldb/include/lldb/Target/TraceHTR.h:72
+  size_t GetLayerId() const;
+  void SetLayerId(size_t id);
+  /// Get the metadata associated with the unit (instruction or block) at

better have a constructor here that expects an ID, so that at any point in t

[Lldb-commits] [PATCH] D106266: [C++4OpenCL] Add run line standard aliases clc++1.0 and CLC++1.0

[Anastasia Stulova via Phabricator via lldb-commits]

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

LGTM! Thanks!


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

https://reviews.llvm.org/D106266

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

[Lldb-commits] [PATCH] D105649: [LLDB] Move Trace-specific classes into separate library

[Alex Langford via Phabricator via lldb-commits]

bulbazord updated this revision to Diff 360542.
bulbazord added a comment.

Updating


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105649

Files:
  lldb/include/lldb/Target/ThreadPostMortemTrace.h
  lldb/include/lldb/Target/TraceSessionFileParser.h
  lldb/include/lldb/lldb-forward.h
  lldb/source/Plugins/Trace/CMakeLists.txt
  lldb/source/Plugins/Trace/common/CMakeLists.txt
  lldb/source/Plugins/Trace/common/ThreadPostMortemTrace.cpp
  lldb/source/Plugins/Trace/common/ThreadPostMortemTrace.h
  lldb/source/Plugins/Trace/common/TraceSessionFileParser.cpp
  lldb/source/Plugins/Trace/common/TraceSessionFileParser.h
  lldb/source/Plugins/Trace/intel-pt/CMakeLists.txt
  lldb/source/Target/CMakeLists.txt
  lldb/source/Target/ThreadPostMortemTrace.cpp
  lldb/source/Target/Trace.cpp
  lldb/source/Target/TraceSessionFileParser.cpp

Index: lldb/source/Target/Trace.cpp
===
--- lldb/source/Target/Trace.cpp
+++ lldb/source/Target/Trace.cpp
@@ -17,7 +17,6 @@
 #include "lldb/Target/Process.h"
 #include "lldb/Target/SectionLoadList.h"
 #include "lldb/Target/Thread.h"
-#include "lldb/Target/ThreadPostMortemTrace.h"
 #include "lldb/Utility/Stream.h"
 
 using namespace lldb;
Index: lldb/source/Target/CMakeLists.txt
===
--- lldb/source/Target/CMakeLists.txt
+++ lldb/source/Target/CMakeLists.txt
@@ -66,11 +66,9 @@
   ThreadPlanTracer.cpp
   ThreadPlanStack.cpp
   ThreadSpec.cpp
-  ThreadPostMortemTrace.cpp
   Trace.cpp
   TraceCursor.cpp
   TraceInstructionDumper.cpp
-  TraceSessionFileParser.cpp
   UnixSignals.cpp
   UnwindAssembly.cpp
   UnwindLLDB.cpp
Index: lldb/source/Plugins/Trace/intel-pt/CMakeLists.txt
===
--- lldb/source/Plugins/Trace/intel-pt/CMakeLists.txt
+++ lldb/source/Plugins/Trace/intel-pt/CMakeLists.txt
@@ -25,6 +25,7 @@
 lldbCore
 lldbSymbol
 lldbTarget
+lldbPluginTraceCommon
 ${LIBIPT_LIBRARY}
   LINK_COMPONENTS
 Support
Index: lldb/source/Plugins/Trace/common/TraceSessionFileParser.h
===
--- lldb/source/Plugins/Trace/common/TraceSessionFileParser.h
+++ lldb/source/Plugins/Trace/common/TraceSessionFileParser.h
@@ -11,7 +11,7 @@
 
 #include "llvm/Support/JSON.h"
 
-#include "lldb/Target/ThreadPostMortemTrace.h"
+#include "ThreadPostMortemTrace.h"
 
 namespace lldb_private {
 
Index: lldb/source/Plugins/Trace/common/TraceSessionFileParser.cpp
===
--- lldb/source/Plugins/Trace/common/TraceSessionFileParser.cpp
+++ lldb/source/Plugins/Trace/common/TraceSessionFileParser.cpp
@@ -6,7 +6,8 @@
 //
 //===--===/
 
-#include "lldb/Target/TraceSessionFileParser.h"
+#include "TraceSessionFileParser.h"
+#include "ThreadPostMortemTrace.h"
 
 #include 
 
@@ -14,7 +15,6 @@
 #include "lldb/Core/Module.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/Target.h"
-#include "lldb/Target/ThreadPostMortemTrace.h"
 
 using namespace lldb;
 using namespace lldb_private;
Index: lldb/include/lldb/Target/ThreadPostMortemTrace.h
===
--- /dev/null
+++ lldb/include/lldb/Target/ThreadPostMortemTrace.h
@@ -1,60 +0,0 @@
-//===-- ThreadPostMortemTrace.h -*- C++ -*-===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===--===//
-
-#ifndef LLDB_TARGET_THREADPOSTMORTEMTRACE_H
-#define LLDB_TARGET_THREADPOSTMORTEMTRACE_H
-
-#include "lldb/Target/Thread.h"
-
-namespace lldb_private {
-
-/// \class ThreadPostMortemTrace ThreadPostMortemTrace.h
-///
-/// Thread implementation used for representing threads gotten from trace
-/// session files, which are similar to threads from core files.
-///
-/// See \a TraceSessionFileParser for more information regarding trace session
-/// files.
-class ThreadPostMortemTrace : public Thread {
-public:
-  /// \param[in] process
-  /// The process who owns this thread.
-  ///
-  /// \param[in] tid
-  /// The tid of this thread.
-  ///
-  /// \param[in] trace_file
-  /// The file that contains the list of instructions that were traced when
-  /// this thread was being executed.
-  ThreadPostMortemTrace(Process &process, lldb::tid_t tid,
-const FileSpec &trace_file)
-  : Thread(process, tid), m_trace_file(trace_file) {}
-
-  void RefreshStateAfterStop() override;
-
-  lldb::RegisterContextSP GetRegisterContext() override;
-
-  lldb::RegisterContextSP

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

[Rumeet Dhindsa via Phabricator via lldb-commits]

rdhindsa added a comment.

Ping.


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

https://reviews.llvm.org/D105732

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

[Lldb-commits] [PATCH] D105741: [trace] Add `thread trace export` command for Intel PT trace visualization

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added inline comments.



Comment at: lldb/include/lldb/Target/TraceHTR.h:136-140
+  /// Updates block_id: block mapping and append `block_id` to the block id
+  /// trace
+  void AddNewBlock(size_t block_id, HTRBlock &block);
+  /// Append `block_id` to the block id trace
+  void AppendBlockId(size_t block_id);

wallace wrote:
> why do you need both?
`AppendBlockId` is for when we just need to append a block id to the block id 
trace. This is needed when we encounter a block that we have previously 
encountered, so we only need to append its block id and not worry about 
creating a new block.
`AddNewBlock` is needed when a new block has been encountered. In this case, 
the new block needs to be added to `m_block_defs` and the its block id needs to 
be appended to the block id trace.



Comment at: lldb/source/Commands/CommandObjectThread.cpp:2145-2151
+m_trace_export_format =
+(TraceExportFormat)OptionArgParser::ToOptionEnum(
+option_arg, GetDefinitions()[option_idx].enum_values,
+eTraceExportUnknownFormat, error);
+if (m_trace_export_format == eTraceExportUnknownFormat)
+  error.SetErrorStringWithFormat("unknown format '%s' specified.",
+ option_arg.str().c_str());

wallace wrote:
> as we are going to have exporter plugins, to simplify this code, just expect 
> the "ctf" string here explicitly. Later I'll do the smart lookup
Are you suggesting that, for the time being, we just check that `option_arg` is 
"ctf" and set `m_trace_export_format` to `eTraceExportChromeTraceFormat` if so 
and report an error otherwise?



Comment at: lldb/source/Target/TraceHTR.cpp:151
+  while (valid_cursor) {
+// TODO: how should we handle cursor errors in this loop?
+lldb::addr_t current_instruction_load_address = cursor->GetLoadAddress();

wallace wrote:
> you need to enhance the Instruction object, to support errors. You can store 
> the error string in it using a char * pointer from a ConstString. Errors are 
> not frequent, so that should be fine. You need to display the errors in the 
> export data as well, as hiding them could be misleading to the user
What Instruction object are you referring to?
Given the way the InstructionLayer currently works, vector of load addresses 
and a map for metadata, how would you suggest incorporating the error 
information?
My first thought was to store a special value (such as 0) in the vector of load 
addresses for each error, but this seems a bit hacky and doesn't allow a way to 
map a specific error back to its error message. 
What are your thoughts?



Comment at: lldb/source/Target/TraceHTR.cpp:395
+{"ph", "B"},
+{"ts", (ssize_t)i}, // TODO: is there a way to serialize size_t to 
JSON?
+// if not, how should we handle overflow here?

Is there a way to store uint64s in JSON? If not, what's the suggested way to 
handle overflow here?


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

https://reviews.llvm.org/D105741

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

[Lldb-commits] [PATCH] D106192: [LLDB][GUI] Add Create Target form

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D106192#2893862 , @OmarEmaraDev 
wrote:

> I am currently breaking this patch into smaller independent viable patches as 
> suggested.

That will make reviews much easier. Are you going to land the other smaller 
diffs first and then update this one after hey have landed?




Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1309
+FileSpec file_spec(m_content);
+FileSystem::Instance().Resolve(file_spec);
+return file_spec;

OmarEmaraDev wrote:
> clayborg wrote:
> > It should be a property of the FileFieldDelegate if the file is for the 
> > current system. We might be specifying a file on a remote system, so it 
> > doesn't always have to exist, nor should it be resolved on the current 
> > system. When creating a target we would always want to specify this file 
> > should exist on the current system. 
> > 
> > 
> I was trying to add properties that indicates if the file is local or not and 
> if it should be resolved or not. But they all seems identical to the 
> need_to_exist property. If m_need_to_exist is false, then the file will not 
> be resolved and will not be checked with the host file system (Which is what 
> we want to remote files). If it is true, then it will be resolved and checked 
> with the host file system (Which is what we want for local files). So adding 
> more properties might be redundant? What do you think?
That sounds fine. No need to add more fields that don't make sense



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1315
   bool m_need_to_exist;
+  bool m_required;
 };

OmarEmaraDev wrote:
> clayborg wrote:
> > Should this to into the FieldDelegate so we don't need to put it into both 
> > FileFieldDelegate and DirectoryFieldDelegate? Then the default 
> > FieldDelegate::FieldDelegateExitCallback() function could be:
> > 
> > ```
> > virtual void FieldDelegate::FieldDelegateExitCallback() {
> >   if (!m_required)
> > return;
> >   // Check value with another virtual function?
> >   FieldDelegateValueIsValid();
> > }
> > ```
> > This seems like many fields would want to require being set and we don't 
> > want to re-invent this for each kind of field.
> The fields that can have a required property is the TextField and its 
> derivatives (File, Directory, and Number), so we can can add this logic 
> there. This is implemented in D106458.
Sounds good!



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2609
+
+if (!FileSystem::Instance().Exists(file_spec)) {
+  SetError("File doesn't exist!");

OmarEmaraDev wrote:
> clayborg wrote:
> > Won't this be taken care of already by the File field? We should construct 
> > the FileDelegate so that:
> > - the file must exist
> > - the file gets resolved on the local system
> > - the file field is required
> > This way, the only way for the user to get to the "Create" button if is if 
> > they already were able to set a valid and required by the FileFieldDelegate 
> > class.
> D106459 implements the necessary bits to allow for this.
Sounds good!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106192

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

[Lldb-commits] [PATCH] D106458: [LLDB][GUI] Add required property to text fields

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

Nice smaller changes!




Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1079
+  TextFieldDelegate(const char *label, const char *content,
+bool required = false)
+  : m_label(label), m_required(required), m_cursor_position(0),

I would remove the default parameter to make sure everyone sets this correctly. 
It also will help avoid compilation issue if we ever add more parameters in the 
future as this can sometimes cause implicit conversion bugs.



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1279
 public:
-  IntegerFieldDelegate(const char *label, int content)
-  : TextFieldDelegate(label, std::to_string(content).c_str()) {}
+  IntegerFieldDelegate(const char *label, int content, bool required = false)
+  : TextFieldDelegate(label, std::to_string(content).c_str(), required) {}

Remove default argument value for "required" as mentioned above.



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1292
   FileFieldDelegate(const char *label, const char *content,
-bool need_to_exist = true)
-  : TextFieldDelegate(label, content), m_need_to_exist(need_to_exist) {}
+bool need_to_exist = true, bool required = false)
+  : TextFieldDelegate(label, content, required),

Remove default argument value for "required" as mentioned above.



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1322
   DirectoryFieldDelegate(const char *label, const char *content,
- bool need_to_exist = true)
-  : TextFieldDelegate(label, content), m_need_to_exist(need_to_exist) {}
+ bool need_to_exist = true, bool required = false)
+  : TextFieldDelegate(label, content, required),

Remove default argument value for "required" as mentioned above.



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1876
+  TextFieldDelegate *AddTextField(const char *label, const char *content,
+  bool required = false) {
+TextFieldDelegate *delegate =

Remove default argument value for "required" as mentioned above.



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1884-1885
   FileFieldDelegate *AddFileField(const char *label, const char *content,
-  bool need_to_exist = true) {
+  bool need_to_exist = true,
+  bool required = false) {
 FileFieldDelegate *delegate =

Remove default argument value for "need_to_exist" and "required" as mentioned 
above. Have everyone set them each time.



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1894-1895
 const char *content,
-bool need_to_exist = true) {
+bool need_to_exist = true,
+bool required = false) {
 DirectoryFieldDelegate *delegate =

Remove default argument value for "need_to_exist" and "required" as mentioned 
above. Have everyone set them each time.



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1903
+  IntegerFieldDelegate *AddIntegerField(const char *label, int content,
+bool required = false) {
+IntegerFieldDelegate *delegate =

Remove default argument value for "required" as mentioned above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106458

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

[Lldb-commits] [PATCH] D106483: [LLDB][GUI] Add Platform Plugin Field

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev created this revision.
OmarEmaraDev added a reviewer: clayborg.
Herald added a reviewer: teemperor.
OmarEmaraDev requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This patch adds a new Platform Plugin Field. It is a choices field that
lists all the available platform plugins and can retrieve the name of the
selected plugin. The default selected plugin is the currently selected
one. This patch also allows for arbitrary scrolling to make scrolling
easier when setting choices.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D106483

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1439,6 +1439,8 @@
   }
 
   void FieldDelegateDraw(SubPad &surface, bool is_selected) override {
+UpdateScrolling();
+
 surface.TitledBox(m_label.c_str());
 
 Rect content_bounds = surface.GetFrame();
@@ -1458,29 +1460,23 @@
   m_choice++;
   }
 
-  // If the cursor moved past the first visible choice, scroll up by one
-  // choice.
-  void ScrollUpIfNeeded() {
-if (m_choice < m_first_visibile_choice)
-  m_first_visibile_choice--;
-  }
+  void UpdateScrolling() {
+if (m_choice > GetLastVisibleChoice()) {
+  m_first_visibile_choice = m_choice - (m_number_of_visible_choices - 1);
+  return;
+}
 
-  // If the cursor moved past the last visible choice, scroll down by one
-  // choice.
-  void ScrollDownIfNeeded() {
-if (m_choice > GetLastVisibleChoice())
-  m_first_visibile_choice++;
+if (m_choice < m_first_visibile_choice)
+  m_first_visibile_choice = m_choice;
   }
 
   HandleCharResult FieldDelegateHandleChar(int key) override {
 switch (key) {
 case KEY_UP:
   SelectPrevious();
-  ScrollUpIfNeeded();
   return eKeyHandled;
 case KEY_DOWN:
   SelectNext();
-  ScrollDownIfNeeded();
   return eKeyHandled;
 default:
   break;
@@ -1494,6 +1490,15 @@
   // Returns the index of the choice.
   int GetChoice() { return m_choice; }
 
+  void SetChoice(const std::string &choice) {
+for (int i = 0; i < GetNumberOfChoices(); i++) {
+  if (choice == m_choices[i]) {
+m_choice = i;
+return;
+  }
+}
+  }
+
 protected:
   std::string m_label;
   int m_number_of_visible_choices;
@@ -1504,6 +1509,29 @@
   int m_first_visibile_choice;
 };
 
+class PlatformPluginFieldDelegate : public ChoicesFieldDelegate {
+public:
+  PlatformPluginFieldDelegate(Debugger &debugger)
+  : ChoicesFieldDelegate("Platform Plugin", 3, GetPossiblePluginNames()) {
+PlatformSP platform_sp = debugger.GetPlatformList().GetSelectedPlatform();
+if (platform_sp)
+  SetChoice(platform_sp->GetName().AsCString());
+  }
+
+  std::vector GetPossiblePluginNames() {
+std::vector names;
+size_t i = 0;
+while (auto name = PluginManager::GetPlatformPluginNameAtIndex(i++))
+  names.push_back(name);
+return names;
+  }
+
+  std::string GetPluginName() {
+std::string plugin_name = GetChoiceContent();
+return plugin_name;
+  }
+};
+
 template  class ListFieldDelegate : public FieldDelegate {
 public:
   ListFieldDelegate(const char *label, T default_field)
@@ -1899,6 +1927,13 @@
 return delegate;
   }
 
+  PlatformPluginFieldDelegate *AddPlatformPluginField(Debugger &debugger) {
+PlatformPluginFieldDelegate *delegate =
+new PlatformPluginFieldDelegate(debugger);
+m_fields.push_back(FieldDelegateUP(delegate));
+return delegate;
+  }
+
   template 
   ListFieldDelegate *AddListField(const char *label, T default_field) {
 ListFieldDelegate *delegate =
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D106192: [LLDB][GUI] Add Create Target form

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.

> Are you going to land the other smaller diffs first and then update this one 
> after hey have landed?

Yes. There is much dependency between patches, so the smaller ones will have to 
land first then we will rebase this one on main. Also, notice that the smaller 
patches might conflict with each other, so I will have to rebase some of them 
once other land.




Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2573
+m_show_advanced_field = AddBooleanField("Show advanced settings.", false);
+m_arch_field = AddArchField("Architecture", "");
+std::vector load_depentents_options;

clayborg wrote:
> we should add a "Platform" TextField that is populated with all of the 
> platform names. The default value should be the currently selected platform. 
> To get a list of all platform plug-in names you can do this like you did for 
> the process plug-ins:
> 
> ```
>   static const char *GetPlatformPluginNameAtIndex(uint32_t idx);
> ```
> 
> To get the current selected platform:
> ```
> ConstString default_platform_name;
> lldb::PlatformSP platform_sp = debugger.GetPlatformList()GetSelectedPlatform()
> if (platform_sp)
>   default_platform_name = platform_sp->GetName();
> ```
> This will help with remote targets. The main reason we need this is many 
> linux or android binaries have very minimal target triples encoded into the 
> ELF files, so we often need to specify "remote-android" or "remote-linux" 
> when creating targets. The user can of course set the "Architecture" with a 
> fully specified triple, but they can also avoid setting the arch and just 
> specify the platform.
> 
The required field is implemented in D106483.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106192

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

[Lldb-commits] [PATCH] D106458: [LLDB][GUI] Add required property to text fields

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev updated this revision to Diff 360575.
OmarEmaraDev added a comment.

- Remove default arguments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106458

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1075,8 +1075,9 @@
 
 class TextFieldDelegate : public FieldDelegate {
 public:
-  TextFieldDelegate(const char *label, const char *content)
-  : m_label(label), m_cursor_position(0), m_first_visibile_char(0) {
+  TextFieldDelegate(const char *label, const char *content, bool required)
+  : m_label(label), m_required(required), m_cursor_position(0),
+m_first_visibile_char(0) {
 if (content)
   m_content = content;
   }
@@ -1238,6 +1239,13 @@
 return eKeyNotHandled;
   }
 
+  void FieldDelegateExitCallback() override {
+if (!IsSpecified() && m_required)
+  SetError("This field is required!");
+  }
+
+  bool IsSpecified() { return !m_content.empty(); }
+
   bool HasError() { return !m_error.empty(); }
 
   void ClearError() { m_error.clear(); }
@@ -1250,6 +1258,7 @@
 
 protected:
   std::string m_label;
+  bool m_required;
   // The position of the top left corner character of the border.
   std::string m_content;
   // The cursor position in the content string itself. Can be in the range
@@ -1266,8 +1275,8 @@
 
 class IntegerFieldDelegate : public TextFieldDelegate {
 public:
-  IntegerFieldDelegate(const char *label, int content)
-  : TextFieldDelegate(label, std::to_string(content).c_str()) {}
+  IntegerFieldDelegate(const char *label, int content, bool required)
+  : TextFieldDelegate(label, std::to_string(content).c_str(), required) {}
 
   // Only accept digits.
   bool IsAcceptableChar(int key) override { return isdigit(key); }
@@ -1278,13 +1287,16 @@
 
 class FileFieldDelegate : public TextFieldDelegate {
 public:
-  FileFieldDelegate(const char *label, const char *content,
-bool need_to_exist = true)
-  : TextFieldDelegate(label, content), m_need_to_exist(need_to_exist) {}
+  FileFieldDelegate(const char *label, const char *content, bool need_to_exist,
+bool required)
+  : TextFieldDelegate(label, content, required),
+m_need_to_exist(need_to_exist) {}
 
-  // Set appropriate error messages if the file doesn't exists or is, in fact, a
-  // directory.
   void FieldDelegateExitCallback() override {
+TextFieldDelegate::FieldDelegateExitCallback();
+if (!IsSpecified())
+  return;
+
 FileSpec file(GetPath());
 if (m_need_to_exist && !FileSystem::Instance().Exists(file)) {
   SetError("File doesn't exist!");
@@ -1306,12 +1318,15 @@
 class DirectoryFieldDelegate : public TextFieldDelegate {
 public:
   DirectoryFieldDelegate(const char *label, const char *content,
- bool need_to_exist = true)
-  : TextFieldDelegate(label, content), m_need_to_exist(need_to_exist) {}
+ bool need_to_exist, bool required)
+  : TextFieldDelegate(label, content, required),
+m_need_to_exist(need_to_exist) {}
 
-  // Set appropriate error messages if the directory doesn't exists or is, in
-  // fact, a file.
   void FieldDelegateExitCallback() override {
+TextFieldDelegate::FieldDelegateExitCallback();
+if (!IsSpecified())
+  return;
+
 FileSpec file(GetPath());
 if (m_need_to_exist && !FileSystem::Instance().Exists(file)) {
   SetError("Directory doesn't exist!");
@@ -1856,31 +1871,35 @@
 
   // Factory methods to create and add fields of specific types.
 
-  TextFieldDelegate *AddTextField(const char *label, const char *content) {
-TextFieldDelegate *delegate = new TextFieldDelegate(label, content);
+  TextFieldDelegate *AddTextField(const char *label, const char *content,
+  bool required) {
+TextFieldDelegate *delegate =
+new TextFieldDelegate(label, content, required);
 m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
   FileFieldDelegate *AddFileField(const char *label, const char *content,
-  bool need_to_exist = true) {
+  bool need_to_exist, bool required) {
 FileFieldDelegate *delegate =
-new FileFieldDelegate(label, content, need_to_exist);
+new FileFieldDelegate(label, content, need_to_exist, required);
 m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
   DirectoryFieldDelegate *AddDirectoryField(const char *label,
 const char *content,
-bool need_to_exist = true) {
+bool need_to_exist, bool re

[Lldb-commits] [lldb] 8e6b31c - [LLDB] Move Trace-specific classes into separate library

[Alex Langford via lldb-commits]

Author: Alex Langford
Date: 2021-07-21T13:28:34-07:00
New Revision: 8e6b31c3952b366fc3fa0da8e3df7fc09fa65b05

URL: 
https://github.com/llvm/llvm-project/commit/8e6b31c3952b366fc3fa0da8e3df7fc09fa65b05
DIFF: 
https://github.com/llvm/llvm-project/commit/8e6b31c3952b366fc3fa0da8e3df7fc09fa65b05.diff

LOG: [LLDB] Move Trace-specific classes into separate library

These two classes, TraceSessionFileParser and ThreadPostMortemTrace,
seem to be useful primarily for tracing. Currently it looks like
intel-pt is the sole user of these, but that other tracing plugins could
be written in the future that take advantage of these. Unfortunately
with them in Target, there is a dependency on PluginProcessUtility. I'd
like to sever that dependency, so I moved them into a `TraceCommon`
plugin.

Differential Revision: https://reviews.llvm.org/D105649

Added: 
lldb/source/Plugins/Trace/common/CMakeLists.txt
lldb/source/Plugins/Trace/common/ThreadPostMortemTrace.cpp
lldb/source/Plugins/Trace/common/ThreadPostMortemTrace.h
lldb/source/Plugins/Trace/common/TraceSessionFileParser.cpp
lldb/source/Plugins/Trace/common/TraceSessionFileParser.h

Modified: 
lldb/include/lldb/lldb-forward.h
lldb/source/Plugins/Trace/CMakeLists.txt
lldb/source/Plugins/Trace/intel-pt/CMakeLists.txt
lldb/source/Target/CMakeLists.txt
lldb/source/Target/Trace.cpp

Removed: 
lldb/include/lldb/Target/ThreadPostMortemTrace.h
lldb/include/lldb/Target/TraceSessionFileParser.h
lldb/source/Target/ThreadPostMortemTrace.cpp
lldb/source/Target/TraceSessionFileParser.cpp



diff  --git a/lldb/include/lldb/lldb-forward.h 
b/lldb/include/lldb/lldb-forward.h
index 452dbff029b1..2206f575fb08 100644
--- a/lldb/include/lldb/lldb-forward.h
+++ b/lldb/include/lldb/lldb-forward.h
@@ -230,7 +230,6 @@ class ThreadSpec;
 class ThreadPostMortemTrace;
 class Trace;
 class TraceCursor;
-class TraceSessionFileParser;
 class Type;
 class TypeAndOrName;
 class TypeCategoryImpl;

diff  --git a/lldb/source/Plugins/Trace/CMakeLists.txt 
b/lldb/source/Plugins/Trace/CMakeLists.txt
index edbb5f14b4e6..955f88cec340 100644
--- a/lldb/source/Plugins/Trace/CMakeLists.txt
+++ b/lldb/source/Plugins/Trace/CMakeLists.txt
@@ -1,5 +1,7 @@
 option(LLDB_BUILD_INTEL_PT "Enable Building of Intel(R) Processor Trace Tool" 
OFF)
 
+add_subdirectory(common)
+
 if (LLDB_BUILD_INTEL_PT)
   add_subdirectory(intel-pt)
 endif()

diff  --git a/lldb/source/Plugins/Trace/common/CMakeLists.txt 
b/lldb/source/Plugins/Trace/common/CMakeLists.txt
new file mode 100644
index ..604ddb6233d3
--- /dev/null
+++ b/lldb/source/Plugins/Trace/common/CMakeLists.txt
@@ -0,0 +1,8 @@
+add_lldb_library(lldbPluginTraceCommon
+  ThreadPostMortemTrace.cpp
+  TraceSessionFileParser.cpp
+
+  LINK_LIBS
+lldbCore
+lldbTarget
+  )

diff  --git a/lldb/source/Target/ThreadPostMortemTrace.cpp 
b/lldb/source/Plugins/Trace/common/ThreadPostMortemTrace.cpp
similarity index 96%
rename from lldb/source/Target/ThreadPostMortemTrace.cpp
rename to lldb/source/Plugins/Trace/common/ThreadPostMortemTrace.cpp
index f8eb3e4f1f2d..45d6f3b0e098 100644
--- a/lldb/source/Target/ThreadPostMortemTrace.cpp
+++ b/lldb/source/Plugins/Trace/common/ThreadPostMortemTrace.cpp
@@ -6,7 +6,7 @@
 //
 
//===--===//
 
-#include "lldb/Target/ThreadPostMortemTrace.h"
+#include "ThreadPostMortemTrace.h"
 
 #include 
 

diff  --git a/lldb/include/lldb/Target/ThreadPostMortemTrace.h 
b/lldb/source/Plugins/Trace/common/ThreadPostMortemTrace.h
similarity index 100%
rename from lldb/include/lldb/Target/ThreadPostMortemTrace.h
rename to lldb/source/Plugins/Trace/common/ThreadPostMortemTrace.h

diff  --git a/lldb/source/Target/TraceSessionFileParser.cpp 
b/lldb/source/Plugins/Trace/common/TraceSessionFileParser.cpp
similarity index 98%
rename from lldb/source/Target/TraceSessionFileParser.cpp
rename to lldb/source/Plugins/Trace/common/TraceSessionFileParser.cpp
index 006d74b28e8b..c88ad9dc6a59 100644
--- a/lldb/source/Target/TraceSessionFileParser.cpp
+++ b/lldb/source/Plugins/Trace/common/TraceSessionFileParser.cpp
@@ -6,7 +6,8 @@
 //
 //===--===/
 
-#include "lldb/Target/TraceSessionFileParser.h"
+#include "TraceSessionFileParser.h"
+#include "ThreadPostMortemTrace.h"
 
 #include 
 
@@ -14,7 +15,6 @@
 #include "lldb/Core/Module.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/Target.h"
-#include "lldb/Target/ThreadPostMortemTrace.h"
 
 using namespace lldb;
 using namespace lldb_private;

diff  --git a/lldb/include/lldb/Target/TraceSessionFileParser.h 
b/lldb/source/Plugins/Trace/common/TraceSessionFileParser.h
similarity index 99%
rename from lldb/include/lldb/Target/TraceSessionFileParser.h
rename to lldb/source/Plugins/Trace/common/TraceSessionFileParser.h
index 68abc7d006d3..6abaffcec

[Lldb-commits] [PATCH] D105649: [LLDB] Move Trace-specific classes into separate library

[Alex Langford via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG8e6b31c3952b: [LLDB] Move Trace-specific classes into 
separate library (authored by bulbazord).

Changed prior to commit:
  https://reviews.llvm.org/D105649?vs=360542&id=360583#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105649

Files:
  lldb/include/lldb/Target/ThreadPostMortemTrace.h
  lldb/include/lldb/Target/TraceSessionFileParser.h
  lldb/include/lldb/lldb-forward.h
  lldb/source/Plugins/Trace/CMakeLists.txt
  lldb/source/Plugins/Trace/common/CMakeLists.txt
  lldb/source/Plugins/Trace/common/ThreadPostMortemTrace.cpp
  lldb/source/Plugins/Trace/common/ThreadPostMortemTrace.h
  lldb/source/Plugins/Trace/common/TraceSessionFileParser.cpp
  lldb/source/Plugins/Trace/common/TraceSessionFileParser.h
  lldb/source/Plugins/Trace/intel-pt/CMakeLists.txt
  lldb/source/Target/CMakeLists.txt
  lldb/source/Target/ThreadPostMortemTrace.cpp
  lldb/source/Target/Trace.cpp
  lldb/source/Target/TraceSessionFileParser.cpp

Index: lldb/source/Target/Trace.cpp
===
--- lldb/source/Target/Trace.cpp
+++ lldb/source/Target/Trace.cpp
@@ -17,7 +17,6 @@
 #include "lldb/Target/Process.h"
 #include "lldb/Target/SectionLoadList.h"
 #include "lldb/Target/Thread.h"
-#include "lldb/Target/ThreadPostMortemTrace.h"
 #include "lldb/Utility/Stream.h"
 
 using namespace lldb;
Index: lldb/source/Target/CMakeLists.txt
===
--- lldb/source/Target/CMakeLists.txt
+++ lldb/source/Target/CMakeLists.txt
@@ -66,11 +66,9 @@
   ThreadPlanTracer.cpp
   ThreadPlanStack.cpp
   ThreadSpec.cpp
-  ThreadPostMortemTrace.cpp
   Trace.cpp
   TraceCursor.cpp
   TraceInstructionDumper.cpp
-  TraceSessionFileParser.cpp
   UnixSignals.cpp
   UnwindAssembly.cpp
   UnwindLLDB.cpp
Index: lldb/source/Plugins/Trace/intel-pt/CMakeLists.txt
===
--- lldb/source/Plugins/Trace/intel-pt/CMakeLists.txt
+++ lldb/source/Plugins/Trace/intel-pt/CMakeLists.txt
@@ -25,6 +25,7 @@
 lldbCore
 lldbSymbol
 lldbTarget
+lldbPluginTraceCommon
 ${LIBIPT_LIBRARY}
   LINK_COMPONENTS
 Support
Index: lldb/source/Plugins/Trace/common/TraceSessionFileParser.h
===
--- lldb/source/Plugins/Trace/common/TraceSessionFileParser.h
+++ lldb/source/Plugins/Trace/common/TraceSessionFileParser.h
@@ -11,7 +11,7 @@
 
 #include "llvm/Support/JSON.h"
 
-#include "lldb/Target/ThreadPostMortemTrace.h"
+#include "ThreadPostMortemTrace.h"
 
 namespace lldb_private {
 
Index: lldb/source/Plugins/Trace/common/TraceSessionFileParser.cpp
===
--- lldb/source/Plugins/Trace/common/TraceSessionFileParser.cpp
+++ lldb/source/Plugins/Trace/common/TraceSessionFileParser.cpp
@@ -6,7 +6,8 @@
 //
 //===--===/
 
-#include "lldb/Target/TraceSessionFileParser.h"
+#include "TraceSessionFileParser.h"
+#include "ThreadPostMortemTrace.h"
 
 #include 
 
@@ -14,7 +15,6 @@
 #include "lldb/Core/Module.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/Target.h"
-#include "lldb/Target/ThreadPostMortemTrace.h"
 
 using namespace lldb;
 using namespace lldb_private;
Index: lldb/source/Plugins/Trace/common/ThreadPostMortemTrace.cpp
===
--- lldb/source/Plugins/Trace/common/ThreadPostMortemTrace.cpp
+++ lldb/source/Plugins/Trace/common/ThreadPostMortemTrace.cpp
@@ -6,7 +6,7 @@
 //
 //===--===//
 
-#include "lldb/Target/ThreadPostMortemTrace.h"
+#include "ThreadPostMortemTrace.h"
 
 #include 
 
Index: lldb/source/Plugins/Trace/common/CMakeLists.txt
===
--- /dev/null
+++ lldb/source/Plugins/Trace/common/CMakeLists.txt
@@ -0,0 +1,8 @@
+add_lldb_library(lldbPluginTraceCommon
+  ThreadPostMortemTrace.cpp
+  TraceSessionFileParser.cpp
+
+  LINK_LIBS
+lldbCore
+lldbTarget
+  )
Index: lldb/source/Plugins/Trace/CMakeLists.txt
===
--- lldb/source/Plugins/Trace/CMakeLists.txt
+++ lldb/source/Plugins/Trace/CMakeLists.txt
@@ -1,5 +1,7 @@
 option(LLDB_BUILD_INTEL_PT "Enable Building of Intel(R) Processor Trace Tool" OFF)
 
+add_subdirectory(common)
+
 if (LLDB_BUILD_INTEL_PT)
   add_subdirectory(intel-pt)
 endif()
Index: lldb/include/lldb/lldb-forward.h
===
--- lldb/include/lldb/lldb-forward.h
+++ lldb/include/lldb/lldb-forward.h
@@ -230,7 +230,6 @@
 class ThreadPostMortemTrace;
 c

[Lldb-commits] [PATCH] D105741: [trace] Add `thread trace export` command for Intel PT trace visualization

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.



Comment at: lldb/include/lldb/Target/TraceHTR.h:136-140
+  /// Updates block_id: block mapping and append `block_id` to the block id
+  /// trace
+  void AddNewBlock(size_t block_id, HTRBlock &block);
+  /// Append `block_id` to the block id trace
+  void AppendBlockId(size_t block_id);

jj10306 wrote:
> wallace wrote:
> > why do you need both?
> `AppendBlockId` is for when we just need to append a block id to the block id 
> trace. This is needed when we encounter a block that we have previously 
> encountered, so we only need to append its block id and not worry about 
> creating a new block.
> `AddNewBlock` is needed when a new block has been encountered. In this case, 
> the new block needs to be added to `m_block_defs` and the its block id needs 
> to be appended to the block id trace.
ahh, then better rename them. AppendRepeatingBlockToEnd and AppendNewBlockToEnd 
might be better. You could omit the ToEnd suffix



Comment at: lldb/source/Commands/CommandObjectThread.cpp:2145-2151
+m_trace_export_format =
+(TraceExportFormat)OptionArgParser::ToOptionEnum(
+option_arg, GetDefinitions()[option_idx].enum_values,
+eTraceExportUnknownFormat, error);
+if (m_trace_export_format == eTraceExportUnknownFormat)
+  error.SetErrorStringWithFormat("unknown format '%s' specified.",
+ option_arg.str().c_str());

jj10306 wrote:
> wallace wrote:
> > as we are going to have exporter plugins, to simplify this code, just 
> > expect the "ctf" string here explicitly. Later I'll do the smart lookup
> Are you suggesting that, for the time being, we just check that `option_arg` 
> is "ctf" and set `m_trace_export_format` to `eTraceExportChromeTraceFormat` 
> if so and report an error otherwise?
yes, just to simplify the code. This check will go away as soon as I implement 
the barebones for the TraceExporter plugins



Comment at: lldb/source/Target/TraceHTR.cpp:151
+  while (valid_cursor) {
+// TODO: how should we handle cursor errors in this loop?
+lldb::addr_t current_instruction_load_address = cursor->GetLoadAddress();

jj10306 wrote:
> wallace wrote:
> > you need to enhance the Instruction object, to support errors. You can 
> > store the error string in it using a char * pointer from a ConstString. 
> > Errors are not frequent, so that should be fine. You need to display the 
> > errors in the export data as well, as hiding them could be misleading to 
> > the user
> What Instruction object are you referring to?
> Given the way the InstructionLayer currently works, vector of load addresses 
> and a map for metadata, how would you suggest incorporating the error 
> information?
> My first thought was to store a special value (such as 0) in the vector of 
> load addresses for each error, but this seems a bit hacky and doesn't allow a 
> way to map a specific error back to its error message. 
> What are your thoughts?
If in CTR you can embed error messages, it would be nice to have that in the 
metadata. It's also valid to have an instruction address of 0 in the case of 
errors. However, you have end up having abnormal blocks like
   insn1 insn2 error_kind1 insn1 insn2 error_kind2

so if you just look at the addresses, [insn1, insn2, 0] is a block that appears 
twice.

Now the question is: if your users will want to see the specific errors, then 
you'll have to handle it accordingly in the metadata. But if your users don't 
case what kind of errors are displayed in the chrome trace view, then replacing 
an error with address 0 makes sense. You could leave a TODO here in that case. 
Let's sync up offline anyway



Comment at: lldb/source/Target/TraceHTR.cpp:395
+{"ph", "B"},
+{"ts", (ssize_t)i}, // TODO: is there a way to serialize size_t to 
JSON?
+// if not, how should we handle overflow here?

jj10306 wrote:
> Is there a way to store uint64s in JSON? If not, what's the suggested way to 
> handle overflow here?
no, only int64_t, that's the type you have to use. And you won't have an 
overflow, AFAIK OSs don't use the last bits of addresses, as they use them 
internally for other things.


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

https://reviews.llvm.org/D105741

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

[Lldb-commits] [PATCH] D100503: [lldb] [client] Implement follow-fork-mode

[Vadim Chugunov via Phabricator via lldb-commits]

vadimcn added a comment.

@mgorny  Do your changes allow debugging both the parent and child processes 
after a fork?


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

https://reviews.llvm.org/D100503

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

[Lldb-commits] [lldb] 29af527 - [intel pt] fix builds

[Walter Erquinigo via lldb-commits]

Author: Walter Erquinigo
Date: 2021-07-21T14:10:09-07:00
New Revision: 29af527c86825e420a75d66c236f2a04f19eb2fe

URL: 
https://github.com/llvm/llvm-project/commit/29af527c86825e420a75d66c236f2a04f19eb2fe
DIFF: 
https://github.com/llvm/llvm-project/commit/29af527c86825e420a75d66c236f2a04f19eb2fe.diff

LOG: [intel pt] fix builds

https://reviews.llvm.org/D105649 broke intel pt builds. Fortunately the
fix is super easy.

Added: 


Modified: 
lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.cpp
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.h

Removed: 




diff  --git a/lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp 
b/lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
index aeba0081e507..3827881454c7 100644
--- a/lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
+++ b/lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
@@ -9,12 +9,12 @@
 
 #include "llvm/Support/MemoryBuffer.h"
 
+#include "../common/ThreadPostMortemTrace.h"
 #include "DecodedThread.h"
 #include "TraceIntelPT.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/Section.h"
 #include "lldb/Target/Target.h"
-#include "lldb/Target/ThreadPostMortemTrace.h"
 #include "lldb/Utility/StringExtractor.h"
 
 using namespace lldb;

diff  --git a/lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp 
b/lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
index 7588db610faf..c12bcd3523e3 100644
--- a/lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
+++ b/lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
@@ -8,6 +8,7 @@
 
 #include "TraceIntelPT.h"
 
+#include "../common/ThreadPostMortemTrace.h"
 #include "CommandObjectTraceStartIntelPT.h"
 #include "DecodedThread.h"
 #include "TraceIntelPTConstants.h"
@@ -15,7 +16,6 @@
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/Target.h"
-#include "lldb/Target/ThreadPostMortemTrace.h"
 
 using namespace lldb;
 using namespace lldb_private;

diff  --git 
a/lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.cpp 
b/lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.cpp
index 8c837f138801..5af7c269d0cb 100644
--- a/lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.cpp
+++ b/lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.cpp
@@ -8,11 +8,11 @@
 
 #include "TraceIntelPTSessionFileParser.h"
 
+#include "../common/ThreadPostMortemTrace.h"
 #include "lldb/Core/Debugger.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Target/ThreadList.h"
-#include "lldb/Target/ThreadPostMortemTrace.h"
 
 using namespace lldb;
 using namespace lldb_private;

diff  --git 
a/lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.h 
b/lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.h
index ad902782ff68..b2667a88 100644
--- a/lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.h
+++ b/lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.h
@@ -10,7 +10,8 @@
 #define LLDB_SOURCE_PLUGINS_TRACE_INTEL_PT_TRACEINTELPTSESSIONFILEPARSER_H
 
 #include "TraceIntelPT.h"
-#include "lldb/Target/TraceSessionFileParser.h"
+
+#include "../common/TraceSessionFileParser.h"
 
 namespace lldb_private {
 namespace trace_intel_pt {



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

[Lldb-commits] [PATCH] D100503: [lldb] [client] Implement follow-fork-mode

[Michał Górny via Phabricator via lldb-commits]

mgorny added a comment.

In D100503#2894584 , @vadimcn wrote:

> @mgorny  Do your changes allow debugging both the parent and child processes 
> after a fork?

No. This patch supports choosing either the parent or the child but it's a step 
in that direction. Further work is necessary to support debugging multiple 
processes simultaneously.


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

https://reviews.llvm.org/D100503

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

[Lldb-commits] [lldb] c93dc25 - [LLDB][GUI] Add Process Plugin Field

[Greg Clayton via lldb-commits]

Author: Omar Emara
Date: 2021-07-21T14:38:29-07:00
New Revision: c93dc2597a587bc4caedc20f3829501f88cca288

URL: 
https://github.com/llvm/llvm-project/commit/c93dc2597a587bc4caedc20f3829501f88cca288
DIFF: 
https://github.com/llvm/llvm-project/commit/c93dc2597a587bc4caedc20f3829501f88cca288.diff

LOG: [LLDB][GUI] Add Process Plugin Field

This patch adds a new Process Plugin Field. It is a choices field that
lists all the available process plugins and can retrieve the name of the
selected plugin or an empty string if the default is selected.

The Attach form now uses that field instead of manually creating a
choices field.

Reviewed By: clayborg

Differential Revision: https://reviews.llvm.org/D106467

Added: 


Modified: 
lldb/source/Core/IOHandlerCursesGUI.cpp

Removed: 




diff  --git a/lldb/source/Core/IOHandlerCursesGUI.cpp 
b/lldb/source/Core/IOHandlerCursesGUI.cpp
index d54c27cea0c8..1779a5b1e882 100644
--- a/lldb/source/Core/IOHandlerCursesGUI.cpp
+++ b/lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1504,6 +1504,29 @@ class ChoicesFieldDelegate : public FieldDelegate {
   int m_first_visibile_choice;
 };
 
+class ProcessPluginFieldDelegate : public ChoicesFieldDelegate {
+public:
+  ProcessPluginFieldDelegate()
+  : ChoicesFieldDelegate("Process Plugin", 3, GetPossiblePluginNames()) {}
+
+  std::vector GetPossiblePluginNames() {
+std::vector names;
+names.push_back("");
+
+size_t i = 0;
+while (auto name = PluginManager::GetProcessPluginNameAtIndex(i++))
+  names.push_back(name);
+return names;
+  }
+
+  std::string GetPluginName() {
+std::string plugin_name = GetChoiceContent();
+if (plugin_name == "")
+  return "";
+return plugin_name;
+  }
+};
+
 template  class ListFieldDelegate : public FieldDelegate {
 public:
   ListFieldDelegate(const char *label, T default_field)
@@ -1899,6 +1922,12 @@ class FormDelegate {
 return delegate;
   }
 
+  ProcessPluginFieldDelegate *AddProcessPluginField() {
+ProcessPluginFieldDelegate *delegate = new ProcessPluginFieldDelegate();
+m_fields.push_back(FieldDelegateUP(delegate));
+return delegate;
+  }
+
   template 
   ListFieldDelegate *AddListField(const char *label, T default_field) {
 ListFieldDelegate *delegate =
@@ -2347,8 +2376,7 @@ class ProcessAttachFormDelegate : public FormDelegate {
 m_include_existing_field =
 AddBooleanField("Include existing processes.", false);
 m_show_advanced_field = AddBooleanField("Show advanced settings.", false);
-m_plugin_field =
-AddChoicesField("Plugin Name", 3, GetPossiblePluginNames());
+m_plugin_field = AddProcessPluginField();
 
 AddAction("Attach", [this](Window &window) { Attach(window); });
   }
@@ -2390,16 +2418,6 @@ class ProcessAttachFormDelegate : public FormDelegate {
 return module_sp->GetFileSpec().GetFilename().AsCString();
   }
 
-  std::vector GetPossiblePluginNames() {
-std::vector names;
-names.push_back("");
-
-size_t i = 0;
-while (auto name = PluginManager::GetProcessPluginNameAtIndex(i++))
-  names.push_back(name);
-return names;
-  }
-
   bool StopRunningProcess() {
 ExecutionContext exe_ctx =
 m_debugger.GetCommandInterpreter().GetExecutionContext();
@@ -2455,8 +2473,7 @@ class ProcessAttachFormDelegate : public FormDelegate {
 } else {
   attach_info.SetProcessID(m_pid_field->GetInteger());
 }
-if (m_plugin_field->GetChoiceContent() != "")
-  attach_info.SetProcessPluginName(m_plugin_field->GetChoiceContent());
+attach_info.SetProcessPluginName(m_plugin_field->GetPluginName());
 
 return attach_info;
   }
@@ -2504,7 +2521,7 @@ class ProcessAttachFormDelegate : public FormDelegate {
   BooleanFieldDelegate *m_wait_for_field;
   BooleanFieldDelegate *m_include_existing_field;
   BooleanFieldDelegate *m_show_advanced_field;
-  ChoicesFieldDelegate *m_plugin_field;
+  ProcessPluginFieldDelegate *m_plugin_field;
 };
 
 class MenuDelegate {



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

[Lldb-commits] [PATCH] D106467: [LLDB][GUI] Add Process Plugin Field

[Greg Clayton via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGc93dc2597a58: [LLDB][GUI] Add Process Plugin Field (authored 
by OmarEmaraDev, committed by clayborg).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106467

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp


Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1504,6 +1504,29 @@
   int m_first_visibile_choice;
 };
 
+class ProcessPluginFieldDelegate : public ChoicesFieldDelegate {
+public:
+  ProcessPluginFieldDelegate()
+  : ChoicesFieldDelegate("Process Plugin", 3, GetPossiblePluginNames()) {}
+
+  std::vector GetPossiblePluginNames() {
+std::vector names;
+names.push_back("");
+
+size_t i = 0;
+while (auto name = PluginManager::GetProcessPluginNameAtIndex(i++))
+  names.push_back(name);
+return names;
+  }
+
+  std::string GetPluginName() {
+std::string plugin_name = GetChoiceContent();
+if (plugin_name == "")
+  return "";
+return plugin_name;
+  }
+};
+
 template  class ListFieldDelegate : public FieldDelegate {
 public:
   ListFieldDelegate(const char *label, T default_field)
@@ -1899,6 +1922,12 @@
 return delegate;
   }
 
+  ProcessPluginFieldDelegate *AddProcessPluginField() {
+ProcessPluginFieldDelegate *delegate = new ProcessPluginFieldDelegate();
+m_fields.push_back(FieldDelegateUP(delegate));
+return delegate;
+  }
+
   template 
   ListFieldDelegate *AddListField(const char *label, T default_field) {
 ListFieldDelegate *delegate =
@@ -2347,8 +2376,7 @@
 m_include_existing_field =
 AddBooleanField("Include existing processes.", false);
 m_show_advanced_field = AddBooleanField("Show advanced settings.", false);
-m_plugin_field =
-AddChoicesField("Plugin Name", 3, GetPossiblePluginNames());
+m_plugin_field = AddProcessPluginField();
 
 AddAction("Attach", [this](Window &window) { Attach(window); });
   }
@@ -2390,16 +2418,6 @@
 return module_sp->GetFileSpec().GetFilename().AsCString();
   }
 
-  std::vector GetPossiblePluginNames() {
-std::vector names;
-names.push_back("");
-
-size_t i = 0;
-while (auto name = PluginManager::GetProcessPluginNameAtIndex(i++))
-  names.push_back(name);
-return names;
-  }
-
   bool StopRunningProcess() {
 ExecutionContext exe_ctx =
 m_debugger.GetCommandInterpreter().GetExecutionContext();
@@ -2455,8 +2473,7 @@
 } else {
   attach_info.SetProcessID(m_pid_field->GetInteger());
 }
-if (m_plugin_field->GetChoiceContent() != "")
-  attach_info.SetProcessPluginName(m_plugin_field->GetChoiceContent());
+attach_info.SetProcessPluginName(m_plugin_field->GetPluginName());
 
 return attach_info;
   }
@@ -2504,7 +2521,7 @@
   BooleanFieldDelegate *m_wait_for_field;
   BooleanFieldDelegate *m_include_existing_field;
   BooleanFieldDelegate *m_show_advanced_field;
-  ChoicesFieldDelegate *m_plugin_field;
+  ProcessPluginFieldDelegate *m_plugin_field;
 };
 
 class MenuDelegate {


Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1504,6 +1504,29 @@
   int m_first_visibile_choice;
 };
 
+class ProcessPluginFieldDelegate : public ChoicesFieldDelegate {
+public:
+  ProcessPluginFieldDelegate()
+  : ChoicesFieldDelegate("Process Plugin", 3, GetPossiblePluginNames()) {}
+
+  std::vector GetPossiblePluginNames() {
+std::vector names;
+names.push_back("");
+
+size_t i = 0;
+while (auto name = PluginManager::GetProcessPluginNameAtIndex(i++))
+  names.push_back(name);
+return names;
+  }
+
+  std::string GetPluginName() {
+std::string plugin_name = GetChoiceContent();
+if (plugin_name == "")
+  return "";
+return plugin_name;
+  }
+};
+
 template  class ListFieldDelegate : public FieldDelegate {
 public:
   ListFieldDelegate(const char *label, T default_field)
@@ -1899,6 +1922,12 @@
 return delegate;
   }
 
+  ProcessPluginFieldDelegate *AddProcessPluginField() {
+ProcessPluginFieldDelegate *delegate = new ProcessPluginFieldDelegate();
+m_fields.push_back(FieldDelegateUP(delegate));
+return delegate;
+  }
+
   template 
   ListFieldDelegate *AddListField(const char *label, T default_field) {
 ListFieldDelegate *delegate =
@@ -2347,8 +2376,7 @@
 m_include_existing_field =
 AddBooleanField("Include existing processes.", false);
 m_show_advanced_field = AddBooleanField("Show advanced settings.", false);
-m_plugin_field =
-AddChoicesField("Plugin Name", 3, GetPossiblePluginNames());
+m_plugin_fi

[Lldb-commits] [lldb] 9ef7de7 - [LLDB][GUI] Add required property to text fields

[Greg Clayton via lldb-commits]

Author: Omar Emara
Date: 2021-07-21T14:40:43-07:00
New Revision: 9ef7de7c819d39eaa4176d046083bf585c9274b0

URL: 
https://github.com/llvm/llvm-project/commit/9ef7de7c819d39eaa4176d046083bf585c9274b0
DIFF: 
https://github.com/llvm/llvm-project/commit/9ef7de7c819d39eaa4176d046083bf585c9274b0.diff

LOG: [LLDB][GUI] Add required property to text fields

This patch adds a required property to text fields and their
derivatives. Additionally, the Process Name and PID fields in the attach
form were marked as required.

Differential Revision: https://reviews.llvm.org/D106458

Added: 


Modified: 
lldb/source/Core/IOHandlerCursesGUI.cpp

Removed: 




diff  --git a/lldb/source/Core/IOHandlerCursesGUI.cpp 
b/lldb/source/Core/IOHandlerCursesGUI.cpp
index 1779a5b1e882..98ef6bd3d476 100644
--- a/lldb/source/Core/IOHandlerCursesGUI.cpp
+++ b/lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1075,8 +1075,9 @@ typedef std::unique_ptr FieldDelegateUP;
 
 class TextFieldDelegate : public FieldDelegate {
 public:
-  TextFieldDelegate(const char *label, const char *content)
-  : m_label(label), m_cursor_position(0), m_first_visibile_char(0) {
+  TextFieldDelegate(const char *label, const char *content, bool required)
+  : m_label(label), m_required(required), m_cursor_position(0),
+m_first_visibile_char(0) {
 if (content)
   m_content = content;
   }
@@ -1238,6 +1239,13 @@ class TextFieldDelegate : public FieldDelegate {
 return eKeyNotHandled;
   }
 
+  void FieldDelegateExitCallback() override {
+if (!IsSpecified() && m_required)
+  SetError("This field is required!");
+  }
+
+  bool IsSpecified() { return !m_content.empty(); }
+
   bool HasError() { return !m_error.empty(); }
 
   void ClearError() { m_error.clear(); }
@@ -1250,6 +1258,7 @@ class TextFieldDelegate : public FieldDelegate {
 
 protected:
   std::string m_label;
+  bool m_required;
   // The position of the top left corner character of the border.
   std::string m_content;
   // The cursor position in the content string itself. Can be in the range
@@ -1266,8 +1275,8 @@ class TextFieldDelegate : public FieldDelegate {
 
 class IntegerFieldDelegate : public TextFieldDelegate {
 public:
-  IntegerFieldDelegate(const char *label, int content)
-  : TextFieldDelegate(label, std::to_string(content).c_str()) {}
+  IntegerFieldDelegate(const char *label, int content, bool required)
+  : TextFieldDelegate(label, std::to_string(content).c_str(), required) {}
 
   // Only accept digits.
   bool IsAcceptableChar(int key) override { return isdigit(key); }
@@ -1278,13 +1287,16 @@ class IntegerFieldDelegate : public TextFieldDelegate {
 
 class FileFieldDelegate : public TextFieldDelegate {
 public:
-  FileFieldDelegate(const char *label, const char *content,
-bool need_to_exist = true)
-  : TextFieldDelegate(label, content), m_need_to_exist(need_to_exist) {}
+  FileFieldDelegate(const char *label, const char *content, bool need_to_exist,
+bool required)
+  : TextFieldDelegate(label, content, required),
+m_need_to_exist(need_to_exist) {}
 
-  // Set appropriate error messages if the file doesn't exists or is, in fact, 
a
-  // directory.
   void FieldDelegateExitCallback() override {
+TextFieldDelegate::FieldDelegateExitCallback();
+if (!IsSpecified())
+  return;
+
 FileSpec file(GetPath());
 if (m_need_to_exist && !FileSystem::Instance().Exists(file)) {
   SetError("File doesn't exist!");
@@ -1306,12 +1318,15 @@ class FileFieldDelegate : public TextFieldDelegate {
 class DirectoryFieldDelegate : public TextFieldDelegate {
 public:
   DirectoryFieldDelegate(const char *label, const char *content,
- bool need_to_exist = true)
-  : TextFieldDelegate(label, content), m_need_to_exist(need_to_exist) {}
+ bool need_to_exist, bool required)
+  : TextFieldDelegate(label, content, required),
+m_need_to_exist(need_to_exist) {}
 
-  // Set appropriate error messages if the directory doesn't exists or is, in
-  // fact, a file.
   void FieldDelegateExitCallback() override {
+TextFieldDelegate::FieldDelegateExitCallback();
+if (!IsSpecified())
+  return;
+
 FileSpec file(GetPath());
 if (m_need_to_exist && !FileSystem::Instance().Exists(file)) {
   SetError("Directory doesn't exist!");
@@ -1879,31 +1894,35 @@ class FormDelegate {
 
   // Factory methods to create and add fields of specific types.
 
-  TextFieldDelegate *AddTextField(const char *label, const char *content) {
-TextFieldDelegate *delegate = new TextFieldDelegate(label, content);
+  TextFieldDelegate *AddTextField(const char *label, const char *content,
+  bool required) {
+TextFieldDelegate *delegate =
+new TextFieldDelegate(label, content, required);
 m_fields.push_back(FieldDeleg

[Lldb-commits] [PATCH] D106458: [LLDB][GUI] Add required property to text fields

[Greg Clayton via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG9ef7de7c819d: [LLDB][GUI] Add required property to text 
fields (authored by OmarEmaraDev, committed by clayborg).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106458

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1075,8 +1075,9 @@
 
 class TextFieldDelegate : public FieldDelegate {
 public:
-  TextFieldDelegate(const char *label, const char *content)
-  : m_label(label), m_cursor_position(0), m_first_visibile_char(0) {
+  TextFieldDelegate(const char *label, const char *content, bool required)
+  : m_label(label), m_required(required), m_cursor_position(0),
+m_first_visibile_char(0) {
 if (content)
   m_content = content;
   }
@@ -1238,6 +1239,13 @@
 return eKeyNotHandled;
   }
 
+  void FieldDelegateExitCallback() override {
+if (!IsSpecified() && m_required)
+  SetError("This field is required!");
+  }
+
+  bool IsSpecified() { return !m_content.empty(); }
+
   bool HasError() { return !m_error.empty(); }
 
   void ClearError() { m_error.clear(); }
@@ -1250,6 +1258,7 @@
 
 protected:
   std::string m_label;
+  bool m_required;
   // The position of the top left corner character of the border.
   std::string m_content;
   // The cursor position in the content string itself. Can be in the range
@@ -1266,8 +1275,8 @@
 
 class IntegerFieldDelegate : public TextFieldDelegate {
 public:
-  IntegerFieldDelegate(const char *label, int content)
-  : TextFieldDelegate(label, std::to_string(content).c_str()) {}
+  IntegerFieldDelegate(const char *label, int content, bool required)
+  : TextFieldDelegate(label, std::to_string(content).c_str(), required) {}
 
   // Only accept digits.
   bool IsAcceptableChar(int key) override { return isdigit(key); }
@@ -1278,13 +1287,16 @@
 
 class FileFieldDelegate : public TextFieldDelegate {
 public:
-  FileFieldDelegate(const char *label, const char *content,
-bool need_to_exist = true)
-  : TextFieldDelegate(label, content), m_need_to_exist(need_to_exist) {}
+  FileFieldDelegate(const char *label, const char *content, bool need_to_exist,
+bool required)
+  : TextFieldDelegate(label, content, required),
+m_need_to_exist(need_to_exist) {}
 
-  // Set appropriate error messages if the file doesn't exists or is, in fact, a
-  // directory.
   void FieldDelegateExitCallback() override {
+TextFieldDelegate::FieldDelegateExitCallback();
+if (!IsSpecified())
+  return;
+
 FileSpec file(GetPath());
 if (m_need_to_exist && !FileSystem::Instance().Exists(file)) {
   SetError("File doesn't exist!");
@@ -1306,12 +1318,15 @@
 class DirectoryFieldDelegate : public TextFieldDelegate {
 public:
   DirectoryFieldDelegate(const char *label, const char *content,
- bool need_to_exist = true)
-  : TextFieldDelegate(label, content), m_need_to_exist(need_to_exist) {}
+ bool need_to_exist, bool required)
+  : TextFieldDelegate(label, content, required),
+m_need_to_exist(need_to_exist) {}
 
-  // Set appropriate error messages if the directory doesn't exists or is, in
-  // fact, a file.
   void FieldDelegateExitCallback() override {
+TextFieldDelegate::FieldDelegateExitCallback();
+if (!IsSpecified())
+  return;
+
 FileSpec file(GetPath());
 if (m_need_to_exist && !FileSystem::Instance().Exists(file)) {
   SetError("Directory doesn't exist!");
@@ -1879,31 +1894,35 @@
 
   // Factory methods to create and add fields of specific types.
 
-  TextFieldDelegate *AddTextField(const char *label, const char *content) {
-TextFieldDelegate *delegate = new TextFieldDelegate(label, content);
+  TextFieldDelegate *AddTextField(const char *label, const char *content,
+  bool required) {
+TextFieldDelegate *delegate =
+new TextFieldDelegate(label, content, required);
 m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
   FileFieldDelegate *AddFileField(const char *label, const char *content,
-  bool need_to_exist = true) {
+  bool need_to_exist, bool required) {
 FileFieldDelegate *delegate =
-new FileFieldDelegate(label, content, need_to_exist);
+new FileFieldDelegate(label, content, need_to_exist, required);
 m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
   DirectoryFieldDelegate *AddDirectoryField(const char *label,
 const char *c

[Lldb-commits] [PATCH] D106483: [LLDB][GUI] Add Platform Plugin Field

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

Actually this doesn't apply cleanly to top of tree right now. Can you resolve 
the conflicts and update the patch? Two other of your patches recently have 
been committed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106483

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

[Lldb-commits] [PATCH] D106192: [LLDB][GUI] Add Create Target form

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D106192#2894392 , @OmarEmaraDev 
wrote:

>> Are you going to land the other smaller diffs first and then update this one 
>> after hey have landed?
>
> Yes. There is much dependency between patches, so the smaller ones will have 
> to land first then we will rebase this one on main. Also, notice that the 
> smaller patches might conflict with each other, so I will have to rebase some 
> of them once other land.

Indeed! I applied two patches already, and the last one 
https://reviews.llvm.org/D106483 is conflicting. Ping me back when you get the 
conflicts resolved and we can move forward.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106192

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

[Lldb-commits] [PATCH] D106501: [trace] Add the definition of a TraceExporter plugin

[walter erquinigo via Phabricator via lldb-commits]

wallace created this revision.
wallace added a reviewer: clayborg.
Herald added a subscriber: mgorny.
wallace requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Copying from the inline documentation:

  Trace exporter plug-ins operate on traces, converting the trace data provided 
by an \a lldb_private::TraceCursor into a different format that can be digested 
by other tools, e.g. Chrome Trace Event Profiler.
  Trace exporters are supposed to operate on an architecture-agnostic fashion, 
as a TraceCursor, which feeds the data, hides the actual trace technology being 
used.

I want to use this to make the code in https://reviews.llvm.org/D105741 a 
plug-in. I also imagine that there will be more and more exporters being 
implemented, as an exporter creates something useful out of trace data. And tbh 
I don't want to keep adding more stuff to the lldb/Target folder.

This is the minimal definition for a TraceExporter plugin. I plan to use this 
with the following commands:

- thread trace export  [plug-in specific args]
  - This command would support autocompletion of plug-in names
- thread trace export list
  - This command would list the available trace exporter plug-ins

I don't plan to create yet a "process trace export" because it's easier to 
start analyzing the trace of a given thread than of the entire process. When we 
need a process-level command, we can implement it.

I also don't plan to force each "export" command implementation to support 
multiple threads (for example, "thread trace start 1 2 3" or "thread trace 
start all" operate on many threads simultaneously). The reason is that the 
format used by the exporter might or might not support multiple threads, so I'm 
leaving this decision to each trace exporter plug-in.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D106501

Files:
  lldb/include/lldb/Core/PluginManager.h
  lldb/include/lldb/Target/TraceExporter.h
  lldb/include/lldb/lldb-forward.h
  lldb/include/lldb/lldb-private-interfaces.h
  lldb/source/Core/PluginManager.cpp
  lldb/source/Target/CMakeLists.txt
  lldb/source/Target/TraceExporter.cpp

Index: lldb/source/Target/TraceExporter.cpp
===
--- /dev/null
+++ lldb/source/Target/TraceExporter.cpp
@@ -0,0 +1,32 @@
+//===-- TraceExporter.cpp -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Target/TraceExporter.h"
+
+#include "lldb/Core/PluginManager.h"
+
+using namespace lldb;
+using namespace lldb_private;
+using namespace llvm;
+
+static Error createInvalidPlugInError(StringRef plugin_name) {
+  return createStringError(
+  std::errc::invalid_argument,
+  "no trace expoter plug-in matches the specified type: \"%s\"",
+  plugin_name.data());
+}
+
+Expected
+TraceExporter::FindPlugin(llvm::StringRef plugin_name) {
+  ConstString name(plugin_name);
+  if (auto create_callback =
+  PluginManager::GetTraceExporterCreateCallback(name))
+return create_callback();
+
+  return createInvalidPlugInError(plugin_name);
+}
Index: lldb/source/Target/CMakeLists.txt
===
--- lldb/source/Target/CMakeLists.txt
+++ lldb/source/Target/CMakeLists.txt
@@ -68,6 +68,7 @@
   ThreadSpec.cpp
   Trace.cpp
   TraceCursor.cpp
+  TraceExporter.cpp
   TraceInstructionDumper.cpp
   UnixSignals.cpp
   UnwindAssembly.cpp
Index: lldb/source/Core/PluginManager.cpp
===
--- lldb/source/Core/PluginManager.cpp
+++ lldb/source/Core/PluginManager.cpp
@@ -1076,6 +1076,37 @@
   return llvm::StringRef();
 }
 
+#pragma mark TraceExporter
+
+typedef PluginInstance TraceExporterInstance;
+typedef PluginInstances TraceExporterInstances;
+
+static TraceExporterInstances &GetTraceExporterInstances() {
+  static TraceExporterInstances g_instances;
+  return g_instances;
+}
+
+bool PluginManager::RegisterPlugin(
+ConstString name, const char *description,
+TraceExporterCreateInstance create_callback) {
+  return GetTraceExporterInstances().RegisterPlugin(name, description,
+create_callback);
+}
+
+bool PluginManager::UnregisterPlugin(
+TraceExporterCreateInstance create_callback) {
+  return GetTraceExporterInstances().UnregisterPlugin(create_callback);
+}
+
+const char *
+PluginManager::GetTraceExporterPluginDescriptionAtIndex(uint32_t index) {
+  return GetTraceExporterInstances().GetDescriptionAtIndex(index);
+}
+
+const char *PluginManager::GetTraceExporterPluginNameAtIndex(uint32_t index) {
+  return GetTraceExporterInstance

[Lldb-commits] [PATCH] D106194: Tests for: D100299: Be lazier about loading .dwo files

[Eric Leese via Phabricator via lldb-commits]

Eric added a comment.

Thank you for helping me with this! Do you want me to merge these changes into 
my own CL or do you want to check this in yourself?

Instead of echoing into .c files, can we write the tests as .c files? It seems 
to be a common pattern to use a single source as two sources by compiling twice 
with different -D flags and then linking; see for example 
Shell/SymbolFile/DWARF/x86/dwarf5-partial-index.cpp


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106194

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

[Lldb-commits] [PATCH] D106501: [trace] Add the definition of a TraceExporter plugin

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
Herald added a subscriber: JDevlieghere.

Just switch to unique_ptr and this will be good to go.




Comment at: lldb/include/lldb/lldb-forward.h:445
 typedef std::shared_ptr TraceSP;
+typedef std::shared_ptr TraceExporterSP;
 typedef std::unique_ptr TraceCursorUP;

Make unique_ptr since the exporters will be short lived.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106501

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

[Lldb-commits] [PATCH] D106501: [trace] Add the definition of a TraceExporter plugin

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Lets go ahead and and the "thread trace export" command as part of this to show 
how this API will get used and function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106501

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

[Lldb-commits] [PATCH] D106226: [lldb] Improve error message when "lldb attach" fails

[APOORV SACHAN via Phabricator via lldb-commits]

apoos-maximus added a comment.

In D106226#2886627 , @DavidSpickett 
wrote:

> It changes whether the output goes to stdout or stderr and whether we set the 
> return status to failed. So I would merge it into one call to 
> `AppendErrorWithFormat`.

Thanks a lot for passing on this knowledge.

>> The error object is being returned by the Attach() call on line:399 so, 
>> should this message be handled by the Attach() function and not by 
>> DoExecute() ?
>
> Go to https://lldb.llvm.org/python_reference/index.html, search for 
> `SBTarget` and you'll see a few attach methods there. They call Target Attach 
> so if you added it there they'd also get it. Which seems like a good thing to 
> me. (but see my other comment about platforms)



> Would it be possible to move this message further down into Target::Attach to 
> a place where we know the target kind? (that would also answer your question 
> about what level it should be at)
>
> If you could return a status from that level with this message then you'd 
> only see it where it makes sense.

Looks like target related information is available in `TargetProperties` class 
whom `Target` class is inheriting so perhaps a decision based on target-type 
could be made there. I will explore further into this, as in which property(s) 
can be used and how and have an update.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106226

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

[Lldb-commits] [PATCH] D106226: [lldb] Improve error message when "lldb attach" fails

[APOORV SACHAN via Phabricator via lldb-commits]

apoos-maximus added a comment.

In D106226#2891107 , @teemperor wrote:

> Congrats on getting started on your first patch! I improving this error 
> message really seems like a good idea.

Thanks  : ) !

> From what I can see the error message here is identical to GDB's which is a 
> different project with an incompatible license. No idea if this is large 
> enough of a copy to bring us into the realm of copyright (not a lawyer), but 
> I think formulating our own (maybe even better?) error message would anyway 
> be a good idea. What about something along those lines:
>
>   error: attach failed:  (This 
> line is just the normal LLDB attach error)
>   Note that attaching might have failed due to the ptrace_scope security 
> policy
>   which restricts debuggers from attaching to other processes. See
>   the ptrace_scope documentation for more information:
> https://www.kernel.org/doc/Documentation/security/Yama.txt
>   The current ptrace_scope policy can be found here:
> /proc/sys/kernel/yama/ptrace_scope
>
> (Not sure how I feel about linking to some internet URL, but I couldn't find 
> any man page for Yama/ptrace_scope)

Agreed ! Having something of our own is a good idea. I will change it to what 
you and @rupprecht are suggesting !


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106226

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

[Lldb-commits] [PATCH] D106226: [lldb] Improve error message when "lldb attach" fails

[APOORV SACHAN via Phabricator via lldb-commits]

apoos-maximus added a comment.

In D106226#2891107 , @teemperor wrote:

> Also I wonder how we could make sure we emit this diagnostic in cases where 
> the ptrace_scope is actually the reason for the failed attach. The proper way 
> to check this seems to be checking the errno after we call ptrace and then 
> propagate the error all the way back to lldb from lldb-server. From the lldb 
> side I don't think we have any way of knowing why the attach actually failed 
> so we would emit this error speculatively which doesn't seem ideal.



In D106226#2891565 , @rupprecht wrote:

> Can we have LLDB read the value of /proc/sys/kernel/yama/ptrace_scope, and 
> only print the error if the file exists and is not 0?

Reading the value of `/proc/sys/kernel/yama/ptrace_scope` is possible even when 
running as a non-root user, so user permissions would not pose a problem here.

  [apoos_maximus@host]$ sysctl kernel.yama.ptrace_scope   
  kernel.yama.ptrace_scope = 1
  [apoos_maximus@host]$ cat /proc/sys/kernel/yama/ptrace_scope   
  1

I will check into the ptrace-API to see if there is a direct way for this, else 
look into some other kernel call through which we can read this value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106226

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

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

Might be worth checking who wrote some of this/nearby code and whether they're 
still active in the community and see if they can review this - not sure if 
Pavel is around/has the bandwidth to review this.


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

https://reviews.llvm.org/D105732

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