[Lldb-commits] [PATCH] D152364: [lldb] Rate limit progress reports -- different approach [WIP-ish]

[Pavel Labath via Phabricator via lldb-commits]

labath created this revision.
labath added reviewers: JDevlieghere, saugustine, rupprecht.
Herald added a project: All.
labath requested review of this revision.
Herald added a project: LLDB.

Have the Progress class spawn a thread to periodically send progress
reports.

The reporting period could be made configurable, but for now I've
hardcoded it to 100ms. (This is the main WIP part)

It could be argued that creating a thread for progress reporting adds
overhead, but I would counter that by saying "If the task is so fast
that creating a thread noticably slows it down, then it really doesn't
need progress reporting".

For me, this speeds up DWARF indexing by about 1.5% (which is only
slightly above the error bars), but I expect it will have a much bigger
impact in situations where printing a single progress update takes a
nontrivial amount of time.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D152364

Files:
  lldb/include/lldb/Core/Progress.h
  lldb/source/Core/Progress.cpp

Index: lldb/source/Core/Progress.cpp
===
--- lldb/source/Core/Progress.cpp
+++ lldb/source/Core/Progress.cpp
@@ -9,6 +9,8 @@
 #include "lldb/Core/Progress.h"
 
 #include "lldb/Core/Debugger.h"
+#include "lldb/Host/ThreadLauncher.h"
+#include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/StreamString.h"
 
 using namespace lldb;
@@ -22,39 +24,67 @@
   assert(total > 0);
   if (debugger)
 m_debugger_id = debugger->GetID();
-  std::lock_guard guard(m_mutex);
-  ReportProgress();
+
+  // Using a shared_future because std::function needs to be copyable.
+  if (llvm::Expected reporting_thread =
+  ThreadLauncher::LaunchThread(
+  "",
+  [this, future = std::shared_future(
+ m_stop_reporting_thread.get_future())]() {
+SendPeriodicReports(future);
+return lldb::thread_result_t();
+  })) {
+m_reporting_thread = std::move(*reporting_thread);
+  } else {
+LLDB_LOG_ERROR(GetLog(LLDBLog::Host), reporting_thread.takeError(),
+   "failed to launch host thread: {}");
+  }
 }
 
 Progress::~Progress() {
-  // Make sure to always report progress completed when this object is
-  // destructed so it indicates the progress dialog/activity should go away.
-  std::lock_guard guard(m_mutex);
-  if (!m_completed) {
-m_completed = m_total;
-ReportProgress();
+  m_stop_reporting_thread.set_value();
+  if (m_reporting_thread.IsJoinable()) {
+m_reporting_thread.Join(nullptr);
   }
 }
 
-void Progress::Increment(uint64_t amount, std::string update) {
-  if (amount > 0) {
-std::lock_guard guard(m_mutex);
-// Watch out for unsigned overflow and make sure we don't increment too
-// much and exceed m_total.
-if (amount > (m_total - m_completed))
-  m_completed = m_total;
-else
-  m_completed += amount;
-ReportProgress(update);
+void Progress::SendPeriodicReports(std::shared_future done) {
+  uint64_t last_completed = 0;
+  Debugger::ReportProgress(m_id, m_title, "", last_completed, m_total,
+   m_debugger_id);
+
+  while (last_completed != m_total &&
+ done.wait_for(std::chrono::milliseconds(100)) ==
+ std::future_status::timeout) {
+uint64_t current_completed = m_completed.load();
+if (current_completed == last_completed)
+  continue;
+
+if (current_completed == m_total ||
+current_completed < last_completed /*overflow*/) {
+  break;
+}
+
+std::string current_update;
+{
+  std::lock_guard guard(m_update_mutex);
+  current_update = std::move(m_update);
+  m_update.clear();
+}
+Debugger::ReportProgress(m_id, m_title, std::move(current_update),
+ current_completed, m_total, m_debugger_id);
+last_completed = current_completed;
   }
+
+  Debugger::ReportProgress(m_id, m_title, "", m_total, m_total, m_debugger_id);
 }
 
-void Progress::ReportProgress(std::string update) {
-  if (!m_complete) {
-// Make sure we only send one notification that indicates the progress is
-// complete.
-m_complete = m_completed == m_total;
-Debugger::ReportProgress(m_id, m_title, std::move(update), m_completed,
- m_total, m_debugger_id);
+void Progress::Increment(uint64_t amount, std::string update) {
+  if (amount == 0)
+return;
+  if (!update.empty()) {
+std::lock_guard guard(m_update_mutex);
+m_update = std::move(update);
   }
+  m_completed += amount;
 }
Index: lldb/include/lldb/Core/Progress.h
===
--- lldb/include/lldb/Core/Progress.h
+++ lldb/include/lldb/Core/Progress.h
@@ -9,9 +9,11 @@
 #ifndef LLDB_CORE_PROGRESS_H
 #define LLDB_CORE_PROGRESS_H
 
+#include "lldb/Host/HostThread.h"
 #include "lldb/Utility/ConstString.h"
 #include "lldb/lldb-types.h"
 #include 
+#include 
 #include 

[Lldb-commits] [PATCH] D150805: Rate limit progress reporting

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I agree that we should have a rate limiting mechanism very close to the source, 
to avoid wasting work for events that aren't going to be used. This is 
particularly important for debug info parsing, where we have multiple threads 
working in parallel and taking a lock even just to check whether we should 
report something could be a choke point.

In D150805#4351277 , @saugustine 
wrote:

> What would be ideal is a timing thread that wakes up every X seconds and 
> prints the results, but there isn't a good mechanism for that, and doing that 
> portably is way out of scope for this.

I've implemented something like that in D152364 
.  Let me know what you think. I like it 
because the act of reporting progress does block the reporting thread in any 
way. (At least for update-string-free updates that is, but I expect that we 
won't be sending update strings for extremely high frequency events.) However, 
I'm not entirely sure whether it meets everyone's use cases, mainly because I 
don't know what those use cases are (e.g. this implementation can "lose" an 
update string if they are coming too fast -- is that acceptable ?)

> In my opinion, the problem is that a single-die is too small a unit of work 
> to be worth reporting on.

I don't think this is correct. The unit of reporting is single DWARF Unit 
,
 which feels OK, assuming we don't do anything egregious for each update. What 
might have confused you is this code here 
,
 which tries to parse DIE trees for all units (and updates progress after each 
**unit**. However, in my test at least, the DWARF units had all their DIEs 
extracted by the time we got to this point, which meant that code was 
essentially doing nothing else except generating progress reports. I'd be 
tempted to just remove progress reporting from this step altogether, though if 
we go with something like that patch above (where a single update just 
increments an atomic var), then I guess keeping it in would not be such a 
problem either.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150805

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

[Lldb-commits] [PATCH] D152324: [lldb][NFCI] Change return type of PersistentExpressionState::GetNextPersistentVariableName

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

Is the idea to remove the ConstString from these APIs too? If yes, this LGTM, 
otherwise it seems to needlessly complicate things.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152324

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

[Lldb-commits] [PATCH] D152220: [lldb][NFCI] Change type of Broadcaster's name

[Alex Langford via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG867ee3b8a76a: [lldb][NFCI] Change type of Broadcaster's 
name (authored by bulbazord).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152220

Files:
  lldb/include/lldb/Utility/Broadcaster.h
  lldb/source/API/SBBroadcaster.cpp
  lldb/source/Core/ThreadedCommunication.cpp
  lldb/source/Utility/Broadcaster.cpp
  lldb/source/Utility/Event.cpp
  lldb/source/Utility/Listener.cpp

Index: lldb/source/Utility/Listener.cpp
===
--- lldb/source/Utility/Listener.cpp
+++ lldb/source/Utility/Listener.cpp
@@ -234,7 +234,7 @@
 
 if (m_broadcaster_names) {
   bool found_source = false;
-  ConstString event_broadcaster_name =
+  const llvm::StringRef event_broadcaster_name =
   event_sp->GetBroadcaster()->GetBroadcasterName();
   for (uint32_t i = 0; i < m_num_broadcaster_names; ++i) {
 if (m_broadcaster_names[i] == event_broadcaster_name) {
Index: lldb/source/Utility/Event.cpp
===
--- lldb/source/Utility/Event.cpp
+++ lldb/source/Utility/Event.cpp
@@ -58,13 +58,13 @@
   s->Printf("%p Event: broadcaster = %p (%s), type = 0x%8.8x (%s), data = ",
 static_cast(this),
 static_cast(broadcaster),
-broadcaster->GetBroadcasterName().GetCString(), m_type,
+broadcaster->GetBroadcasterName().c_str(), m_type,
 event_name.GetData());
 else
   s->Printf("%p Event: broadcaster = %p (%s), type = 0x%8.8x, data = ",
 static_cast(this),
 static_cast(broadcaster),
-broadcaster->GetBroadcasterName().GetCString(), m_type);
+broadcaster->GetBroadcasterName().c_str(), m_type);
   } else
 s->Printf("%p Event: broadcaster = NULL, type = 0x%8.8x, data = ",
   static_cast(this), m_type);
Index: lldb/source/Utility/Broadcaster.cpp
===
--- lldb/source/Utility/Broadcaster.cpp
+++ lldb/source/Utility/Broadcaster.cpp
@@ -23,9 +23,9 @@
 using namespace lldb;
 using namespace lldb_private;
 
-Broadcaster::Broadcaster(BroadcasterManagerSP manager_sp, const char *name)
+Broadcaster::Broadcaster(BroadcasterManagerSP manager_sp, std::string name)
 : m_broadcaster_sp(std::make_shared(*this)),
-  m_manager_sp(std::move(manager_sp)), m_broadcaster_name(name) {
+  m_manager_sp(std::move(manager_sp)), m_broadcaster_name(std::move(name)) {
   Log *log = GetLog(LLDBLog::Object);
   LLDB_LOG(log, "{0} Broadcaster::Broadcaster(\"{1}\")",
static_cast(this), GetBroadcasterName());
@@ -215,12 +215,12 @@
   if (log) {
 StreamString event_description;
 event_sp->Dump(&event_description);
-LLDB_LOGF(log,
-  "%p Broadcaster(\"%s\")::BroadcastEvent (event_sp = {%s}, "
-  "unique =%i) hijack = %p",
-  static_cast(this), GetBroadcasterName(),
-  event_description.GetData(), unique,
-  static_cast(hijacking_listener_sp.get()));
+LLDB_LOG(log,
+ "{0:x} Broadcaster(\"{1}\")::BroadcastEvent (event_sp = {2}, "
+ "unique={3}) hijack = {4:x}",
+ static_cast(this), GetBroadcasterName(),
+ event_description.GetData(), unique,
+ static_cast(hijacking_listener_sp.get()));
   }
 
   if (hijacking_listener_sp) {
Index: lldb/source/Core/ThreadedCommunication.cpp
===
--- lldb/source/Core/ThreadedCommunication.cpp
+++ lldb/source/Core/ThreadedCommunication.cpp
@@ -57,7 +57,7 @@
 ThreadedCommunication::~ThreadedCommunication() {
   LLDB_LOG(GetLog(LLDBLog::Object | LLDBLog::Communication),
"{0} ThreadedCommunication::~ThreadedCommunication (name = {1})",
-   this, GetBroadcasterName().AsCString());
+   this, GetBroadcasterName());
 }
 
 void ThreadedCommunication::Clear() {
Index: lldb/source/API/SBBroadcaster.cpp
===
--- lldb/source/API/SBBroadcaster.cpp
+++ lldb/source/API/SBBroadcaster.cpp
@@ -92,7 +92,7 @@
   LLDB_INSTRUMENT_VA(this);
 
   if (m_opaque_ptr)
-return m_opaque_ptr->GetBroadcasterName().GetCString();
+return ConstString(m_opaque_ptr->GetBroadcasterName()).GetCString();
   return nullptr;
 }
 
Index: lldb/include/lldb/Utility/Broadcaster.h
===
--- lldb/include/lldb/Utility/Broadcaster.h
+++ lldb/include/lldb/Utility/Broadcaster.h
@@ -149,10 +149,12 @@
 public:
   /// Construct with a broadcaster with a name.
   ///
+  /// \param[in] manager_sp
+  ///   A shared pointer to the BroadcasterManager that will manage this
+  ///   broadcas

[Lldb-commits] [lldb] 867ee3b - [lldb][NFCI] Change type of Broadcaster's name

[Alex Langford via lldb-commits]

Author: Alex Langford
Date: 2023-06-07T10:14:53-07:00
New Revision: 867ee3b8a76a78e02c56e78382e31bfd76fa468b

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

LOG: [lldb][NFCI] Change type of Broadcaster's name

Broadcasters don't need their names in the StringPool. It doesn't
benefit from fast comparisons and doesn't benefit from uniqueness.

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

Added: 


Modified: 
lldb/include/lldb/Utility/Broadcaster.h
lldb/source/API/SBBroadcaster.cpp
lldb/source/Core/ThreadedCommunication.cpp
lldb/source/Utility/Broadcaster.cpp
lldb/source/Utility/Event.cpp
lldb/source/Utility/Listener.cpp

Removed: 




diff  --git a/lldb/include/lldb/Utility/Broadcaster.h 
b/lldb/include/lldb/Utility/Broadcaster.h
index 081e6ee5c883f..f9887566a1609 100644
--- a/lldb/include/lldb/Utility/Broadcaster.h
+++ b/lldb/include/lldb/Utility/Broadcaster.h
@@ -149,10 +149,12 @@ class Broadcaster {
 public:
   /// Construct with a broadcaster with a name.
   ///
+  /// \param[in] manager_sp
+  ///   A shared pointer to the BroadcasterManager that will manage this
+  ///   broadcaster.
   /// \param[in] name
-  /// A NULL terminated C string that contains the name of the
-  /// broadcaster object.
-  Broadcaster(lldb::BroadcasterManagerSP manager_sp, const char *name);
+  ///   A std::string of the name that this broadcaster will have.
+  Broadcaster(lldb::BroadcasterManagerSP manager_sp, std::string name);
 
   /// Destructor.
   ///
@@ -213,11 +215,12 @@ class Broadcaster {
 return m_broadcaster_sp->AddListener(listener_sp, event_mask);
   }
 
-  /// Get the NULL terminated C string name of this Broadcaster object.
+  /// Get this broadcaster's name.
   ///
   /// \return
-  /// The NULL terminated C string name of this Broadcaster.
-  ConstString GetBroadcasterName() { return m_broadcaster_name; }
+  /// A reference to a constant std::string containing the name of the
+  /// broadcaster.
+  const std::string &GetBroadcasterName() { return m_broadcaster_name; }
 
   /// Get the event name(s) for one or more event bits.
   ///
@@ -352,8 +355,8 @@ class Broadcaster {
 uint32_t AddListener(const lldb::ListenerSP &listener_sp,
  uint32_t event_mask);
 
-const char *GetBroadcasterName() const {
-  return m_broadcaster.GetBroadcasterName().AsCString();
+const std::string &GetBroadcasterName() const {
+  return m_broadcaster.GetBroadcasterName();
 }
 
 Broadcaster *GetBroadcaster();
@@ -443,7 +446,7 @@ class Broadcaster {
   lldb::BroadcasterManagerSP m_manager_sp;
 
   /// The name of this broadcaster object.
-  const ConstString m_broadcaster_name;
+  const std::string m_broadcaster_name;
 
   Broadcaster(const Broadcaster &) = delete;
   const Broadcaster &operator=(const Broadcaster &) = delete;

diff  --git a/lldb/source/API/SBBroadcaster.cpp 
b/lldb/source/API/SBBroadcaster.cpp
index 58920931bc5fc..6e34b2f71b824 100644
--- a/lldb/source/API/SBBroadcaster.cpp
+++ b/lldb/source/API/SBBroadcaster.cpp
@@ -92,7 +92,7 @@ const char *SBBroadcaster::GetName() const {
   LLDB_INSTRUMENT_VA(this);
 
   if (m_opaque_ptr)
-return m_opaque_ptr->GetBroadcasterName().GetCString();
+return ConstString(m_opaque_ptr->GetBroadcasterName()).GetCString();
   return nullptr;
 }
 

diff  --git a/lldb/source/Core/ThreadedCommunication.cpp 
b/lldb/source/Core/ThreadedCommunication.cpp
index d547c858f0f57..ec4b0806a80e4 100644
--- a/lldb/source/Core/ThreadedCommunication.cpp
+++ b/lldb/source/Core/ThreadedCommunication.cpp
@@ -57,7 +57,7 @@ ThreadedCommunication::ThreadedCommunication(const char *name)
 ThreadedCommunication::~ThreadedCommunication() {
   LLDB_LOG(GetLog(LLDBLog::Object | LLDBLog::Communication),
"{0} ThreadedCommunication::~ThreadedCommunication (name = {1})",
-   this, GetBroadcasterName().AsCString());
+   this, GetBroadcasterName());
 }
 
 void ThreadedCommunication::Clear() {

diff  --git a/lldb/source/Utility/Broadcaster.cpp 
b/lldb/source/Utility/Broadcaster.cpp
index 58e393863b5a5..c9ecd4a7d2a91 100644
--- a/lldb/source/Utility/Broadcaster.cpp
+++ b/lldb/source/Utility/Broadcaster.cpp
@@ -23,9 +23,9 @@
 using namespace lldb;
 using namespace lldb_private;
 
-Broadcaster::Broadcaster(BroadcasterManagerSP manager_sp, const char *name)
+Broadcaster::Broadcaster(BroadcasterManagerSP manager_sp, std::string name)
 : m_broadcaster_sp(std::make_shared(*this)),
-  m_manager_sp(std::move(manager_sp)), m_broadcaster_name(name) {
+  m_manager_sp(std::move(manager_sp)), m_broadcaster_name(std::move(name)) 
{
   Log *log = GetLog(LLDBLog::Object);
   LLDB_LOG(log, "{0} Broadcaster::Broadcaster(\"{1}\")",
static_cast(this),

[Lldb-commits] [PATCH] D151950: [lldb] Unconditionally increment depth when printing children

[Augusto Noronha via Phabricator via lldb-commits]

augusto2112 updated this revision to Diff 529463.
augusto2112 added a comment.

Add test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151950

Files:
  lldb/source/DataFormatters/ValueObjectPrinter.cpp
  lldb/test/API/lang/cpp/frame-var-depth-and-elem-count/Makefile
  
lldb/test/API/lang/cpp/frame-var-depth-and-elem-count/TestFrameVarDepthAndElemCount.py
  lldb/test/API/lang/cpp/frame-var-depth-and-elem-count/main.cpp


Index: lldb/test/API/lang/cpp/frame-var-depth-and-elem-count/main.cpp
===
--- /dev/null
+++ lldb/test/API/lang/cpp/frame-var-depth-and-elem-count/main.cpp
@@ -0,0 +1,16 @@
+struct A {
+  int i = 42;
+};
+
+struct B {
+  A a;
+};
+
+struct C {
+  B b;
+};
+
+int main() {
+  C *c = new C[5];
+  return 0; // break here
+}
Index: 
lldb/test/API/lang/cpp/frame-var-depth-and-elem-count/TestFrameVarDepthAndElemCount.py
===
--- /dev/null
+++ 
lldb/test/API/lang/cpp/frame-var-depth-and-elem-count/TestFrameVarDepthAndElemCount.py
@@ -0,0 +1,27 @@
+"""
+Tests that frame variable --depth and --element-count options work correctly
+together
+"""
+import lldb
+from lldbsuite.test.lldbtest import *
+import lldbsuite.test.lldbutil as lldbutil
+
+
+class TestFrameVarDepthAndElemCount(TestBase):
+def test(self):
+"""Test that bool types work in the expression parser"""
+self.build()
+lldbutil.run_to_source_breakpoint(
+self, "// break here", lldb.SBFileSpec("main.cpp")
+)
+
+# Check that we print 5 elements but only 2 levels deep.
+self.expect('frame var --depth 2 --element-count 5 -- c', 
+substrs=[
+'[0] = {\nb ={...}\n  }',
+'[1] = {\nb ={...}\n  }',
+'[2] = {\nb ={...}\n  }',
+'[3] = {\nb ={...}\n  }',
+'[4] = {\nb ={...}\n  }',
+])
+
Index: lldb/test/API/lang/cpp/frame-var-depth-and-elem-count/Makefile
===
--- /dev/null
+++ lldb/test/API/lang/cpp/frame-var-depth-and-elem-count/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
Index: lldb/source/DataFormatters/ValueObjectPrinter.cpp
===
--- lldb/source/DataFormatters/ValueObjectPrinter.cpp
+++ lldb/source/DataFormatters/ValueObjectPrinter.cpp
@@ -590,7 +590,7 @@
 void ValueObjectPrinter::PrintChild(
 ValueObjectSP child_sp,
 const DumpValueObjectOptions::PointerDepth &curr_ptr_depth) {
-  const uint32_t consumed_depth = (!m_options.m_pointer_as_array) ? 1 : 0;
+  const uint32_t consumed_summary_depth = m_options.m_pointer_as_array ? 0 : 1;
   const bool does_consume_ptr_depth =
   ((IsPtr() && !m_options.m_pointer_as_array) || IsRef());
 
@@ -603,7 +603,7 @@
   .SetHideValue(m_options.m_hide_value)
   .SetOmitSummaryDepth(child_options.m_omit_summary_depth > 1
? child_options.m_omit_summary_depth -
- consumed_depth
+ consumed_summary_depth
: 0)
   .SetElementCount(0);
 
@@ -611,7 +611,7 @@
 ValueObjectPrinter child_printer(
 child_sp.get(), m_stream, child_options,
 does_consume_ptr_depth ? --curr_ptr_depth : curr_ptr_depth,
-m_curr_depth + consumed_depth, m_printed_instance_pointers);
+m_curr_depth + 1, m_printed_instance_pointers);
 child_printer.PrintValueObject();
   }
 }


Index: lldb/test/API/lang/cpp/frame-var-depth-and-elem-count/main.cpp
===
--- /dev/null
+++ lldb/test/API/lang/cpp/frame-var-depth-and-elem-count/main.cpp
@@ -0,0 +1,16 @@
+struct A {
+  int i = 42;
+};
+
+struct B {
+  A a;
+};
+
+struct C {
+  B b;
+};
+
+int main() {
+  C *c = new C[5];
+  return 0; // break here
+}
Index: lldb/test/API/lang/cpp/frame-var-depth-and-elem-count/TestFrameVarDepthAndElemCount.py
===
--- /dev/null
+++ lldb/test/API/lang/cpp/frame-var-depth-and-elem-count/TestFrameVarDepthAndElemCount.py
@@ -0,0 +1,27 @@
+"""
+Tests that frame variable --depth and --element-count options work correctly
+together
+"""
+import lldb
+from lldbsuite.test.lldbtest import *
+import lldbsuite.test.lldbutil as lldbutil
+
+
+class TestFrameVarDepthAndElemCount(TestBase):
+def test(self):
+"""Test that bool types work in the expression parser"""
+self.build()
+lldbutil.run_to_source_breakpoint(
+self, "// break here", lldb.SBFileSpec("main.cpp")
+)
+
+# Check that we print 5 elements but only 2 levels deep.
+self.expect('frame var --depth 2 --element-count 5 -- c', 
+ 

[Lldb-commits] [PATCH] D152409: [lldb] Never print children if the max depth has been reached

[Augusto Noronha via Phabricator via lldb-commits]

augusto2112 created this revision.
augusto2112 added reviewers: aprantl, kastiglione.
Herald added a project: All.
augusto2112 requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

When formatting a variable, the max depth would potentially be ignored
if the current value object failed to print itself. Change that to
always respect the max depth, even if failure occurs

rdar://109855463


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D152409

Files:
  lldb/source/DataFormatters/ValueObjectPrinter.cpp


Index: lldb/source/DataFormatters/ValueObjectPrinter.cpp
===
--- lldb/source/DataFormatters/ValueObjectPrinter.cpp
+++ lldb/source/DataFormatters/ValueObjectPrinter.cpp
@@ -511,6 +511,10 @@
   if (is_uninit)
 return false;
 
+  // If we have reached the maximum depth we shouldn't print any more children.
+  if (HasReachedMaximumDepth())
+return false;
+
   // if the user has specified an element count, always print children as it is
   // explicit user demand being honored
   if (m_options.m_pointer_as_array)
@@ -523,7 +527,7 @@
   if (TypeSummaryImpl *type_summary = GetSummaryFormatter())
 print_children = type_summary->DoesPrintChildren(m_valobj);
 
-  if (is_failed_description || !HasReachedMaximumDepth()) {
+  if (is_failed_description) {
 // We will show children for all concrete types. We won't show pointer
 // contents unless a pointer depth has been specified. We won't reference
 // contents unless the reference is the root object (depth of zero).


Index: lldb/source/DataFormatters/ValueObjectPrinter.cpp
===
--- lldb/source/DataFormatters/ValueObjectPrinter.cpp
+++ lldb/source/DataFormatters/ValueObjectPrinter.cpp
@@ -511,6 +511,10 @@
   if (is_uninit)
 return false;
 
+  // If we have reached the maximum depth we shouldn't print any more children.
+  if (HasReachedMaximumDepth())
+return false;
+
   // if the user has specified an element count, always print children as it is
   // explicit user demand being honored
   if (m_options.m_pointer_as_array)
@@ -523,7 +527,7 @@
   if (TypeSummaryImpl *type_summary = GetSummaryFormatter())
 print_children = type_summary->DoesPrintChildren(m_valobj);
 
-  if (is_failed_description || !HasReachedMaximumDepth()) {
+  if (is_failed_description) {
 // We will show children for all concrete types. We won't show pointer
 // contents unless a pointer depth has been specified. We won't reference
 // contents unless the reference is the root object (depth of zero).
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 7b47921 - [lldb] Reinvoke crashlog under lldb when run with -i from the command line

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2023-06-07T16:21:16-07:00
New Revision: 7b4792159cb7990f50c839713be4dbf6b31191e9

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

LOG: [lldb] Reinvoke crashlog under lldb when run with -i from the command line

Run crashlog inside lldb when invoked in interactive mode from the
command line. Currently, when passing -i to crashlog from the command
line, we symbolicate in LLDB and immediately exit right after. This
pretty much defeats the purpose of interactive mode. That said, we
wouldn't want to re-implement the driver from the crashlog script.
Re-invoking the crashlog command from inside LLDB solves the issue.

rdar://97801509

Differential revision: https://reviews.llvm.org/D152319

Added: 


Modified: 
lldb/examples/python/crashlog.py

Removed: 




diff  --git a/lldb/examples/python/crashlog.py 
b/lldb/examples/python/crashlog.py
index ea197e4f13a4a..6f69ef5bff7fc 100755
--- a/lldb/examples/python/crashlog.py
+++ b/lldb/examples/python/crashlog.py
@@ -1271,7 +1271,7 @@ def __init__(self, debugger, internal_dict):
 pass
 
 def __call__(self, debugger, command, exe_ctx, result):
-SymbolicateCrashLogs(debugger, shlex.split(command), result)
+SymbolicateCrashLogs(debugger, shlex.split(command), result, True)
 
 def get_short_help(self):
 return "Symbolicate one or more darwin crash log files."
@@ -1596,7 +1596,7 @@ def CrashLogOptionParser():
 return CreateSymbolicateCrashLogOptions("crashlog", description, True)
 
 
-def SymbolicateCrashLogs(debugger, command_args, result):
+def SymbolicateCrashLogs(debugger, command_args, result, is_command):
 option_parser = CrashLogOptionParser()
 
 if not len(command_args):
@@ -1608,6 +1608,26 @@ def SymbolicateCrashLogs(debugger, command_args, result):
 except:
 return
 
+# Interactive mode requires running the crashlog command from inside lldb.
+if options.interactive and not is_command:
+lldb_exec = (
+subprocess.check_output(["/usr/bin/xcrun", "-f", "lldb"])
+.decode("utf-8")
+.strip()
+)
+sys.exit(
+os.execv(
+lldb_exec,
+[
+lldb_exec,
+"-o",
+"command script import lldb.macosx",
+"-o",
+"crashlog {}".format(shlex.join(command_args)),
+],
+)
+)
+
 if options.version:
 print(debugger.GetVersionString())
 return
@@ -1659,7 +1679,7 @@ def should_run_in_interactive_mode(options, ci):
 # Create a new debugger instance
 debugger = lldb.SBDebugger.Create()
 result = lldb.SBCommandReturnObject()
-SymbolicateCrashLogs(debugger, sys.argv[1:], result)
+SymbolicateCrashLogs(debugger, sys.argv[1:], result, False)
 lldb.SBDebugger.Destroy(debugger)
 
 



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

[Lldb-commits] [PATCH] D152319: [lldb] Run crashlog inside lldb when invoked in interactive mode from the command line

[Jonas Devlieghere via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG7b4792159cb7: [lldb] Reinvoke crashlog under lldb when run 
with -i from the command line (authored by JDevlieghere).
Herald added a project: LLDB.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152319

Files:
  lldb/examples/python/crashlog.py


Index: lldb/examples/python/crashlog.py
===
--- lldb/examples/python/crashlog.py
+++ lldb/examples/python/crashlog.py
@@ -1271,7 +1271,7 @@
 pass
 
 def __call__(self, debugger, command, exe_ctx, result):
-SymbolicateCrashLogs(debugger, shlex.split(command), result)
+SymbolicateCrashLogs(debugger, shlex.split(command), result, True)
 
 def get_short_help(self):
 return "Symbolicate one or more darwin crash log files."
@@ -1596,7 +1596,7 @@
 return CreateSymbolicateCrashLogOptions("crashlog", description, True)
 
 
-def SymbolicateCrashLogs(debugger, command_args, result):
+def SymbolicateCrashLogs(debugger, command_args, result, is_command):
 option_parser = CrashLogOptionParser()
 
 if not len(command_args):
@@ -1608,6 +1608,26 @@
 except:
 return
 
+# Interactive mode requires running the crashlog command from inside lldb.
+if options.interactive and not is_command:
+lldb_exec = (
+subprocess.check_output(["/usr/bin/xcrun", "-f", "lldb"])
+.decode("utf-8")
+.strip()
+)
+sys.exit(
+os.execv(
+lldb_exec,
+[
+lldb_exec,
+"-o",
+"command script import lldb.macosx",
+"-o",
+"crashlog {}".format(shlex.join(command_args)),
+],
+)
+)
+
 if options.version:
 print(debugger.GetVersionString())
 return
@@ -1659,7 +1679,7 @@
 # Create a new debugger instance
 debugger = lldb.SBDebugger.Create()
 result = lldb.SBCommandReturnObject()
-SymbolicateCrashLogs(debugger, sys.argv[1:], result)
+SymbolicateCrashLogs(debugger, sys.argv[1:], result, False)
 lldb.SBDebugger.Destroy(debugger)
 
 


Index: lldb/examples/python/crashlog.py
===
--- lldb/examples/python/crashlog.py
+++ lldb/examples/python/crashlog.py
@@ -1271,7 +1271,7 @@
 pass
 
 def __call__(self, debugger, command, exe_ctx, result):
-SymbolicateCrashLogs(debugger, shlex.split(command), result)
+SymbolicateCrashLogs(debugger, shlex.split(command), result, True)
 
 def get_short_help(self):
 return "Symbolicate one or more darwin crash log files."
@@ -1596,7 +1596,7 @@
 return CreateSymbolicateCrashLogOptions("crashlog", description, True)
 
 
-def SymbolicateCrashLogs(debugger, command_args, result):
+def SymbolicateCrashLogs(debugger, command_args, result, is_command):
 option_parser = CrashLogOptionParser()
 
 if not len(command_args):
@@ -1608,6 +1608,26 @@
 except:
 return
 
+# Interactive mode requires running the crashlog command from inside lldb.
+if options.interactive and not is_command:
+lldb_exec = (
+subprocess.check_output(["/usr/bin/xcrun", "-f", "lldb"])
+.decode("utf-8")
+.strip()
+)
+sys.exit(
+os.execv(
+lldb_exec,
+[
+lldb_exec,
+"-o",
+"command script import lldb.macosx",
+"-o",
+"crashlog {}".format(shlex.join(command_args)),
+],
+)
+)
+
 if options.version:
 print(debugger.GetVersionString())
 return
@@ -1659,7 +1679,7 @@
 # Create a new debugger instance
 debugger = lldb.SBDebugger.Create()
 result = lldb.SBCommandReturnObject()
-SymbolicateCrashLogs(debugger, sys.argv[1:], result)
+SymbolicateCrashLogs(debugger, sys.argv[1:], result, False)
 lldb.SBDebugger.Destroy(debugger)
 
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 0dd62ac - [lldb] Disable variable watchpoints when going out of scope

[Med Ismail Bennani via lldb-commits]

Author: Med Ismail Bennani
Date: 2023-06-07T16:52:11-07:00
New Revision: 0dd62ace2ee2fafcb9a08953db1b9e4e20428e28

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

LOG: [lldb] Disable variable watchpoints when going out of scope

If we use a variable watchpoint with a condition using a scope variable,
if we go out-of-scope, the watpoint remains active which can the
expression evaluator to fail to parse the watchpoint condition (because
of the missing varible bindings).

This was discovered after `watchpoint_callback.test` started failing on
the green dragon bot.

This patch should address that issue by setting an internal breakpoint
on the return addresss of the current frame when creating a variable
watchpoint. The breakpoint has a callback that will disable the watchpoint
if the the breakpoint execution context matches the watchpoint execution
context.

This is only enabled for local variables.

This patch also re-enables the failing test following e1086384e584.

rdar://109574319

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

Signed-off-by: Med Ismail Bennani 

Added: 
lldb/test/Shell/Watchpoint/Inputs/val.c
lldb/test/Shell/Watchpoint/Inputs/watchpoint.in
lldb/test/Shell/Watchpoint/LocalVariableWatchpointDisabler.test

Modified: 
lldb/include/lldb/Breakpoint/Watchpoint.h
lldb/source/Breakpoint/Watchpoint.cpp
lldb/source/Commands/CommandObjectWatchpoint.cpp
lldb/test/Shell/ScriptInterpreter/Lua/watchpoint_callback.test

Removed: 




diff  --git a/lldb/include/lldb/Breakpoint/Watchpoint.h 
b/lldb/include/lldb/Breakpoint/Watchpoint.h
index 037be4539066c..3ee75516debe8 100644
--- a/lldb/include/lldb/Breakpoint/Watchpoint.h
+++ b/lldb/include/lldb/Breakpoint/Watchpoint.h
@@ -90,6 +90,40 @@ class Watchpoint : public 
std::enable_shared_from_this,
   void SetWatchVariable(bool val);
   bool CaptureWatchedValue(const ExecutionContext &exe_ctx);
 
+  /// \struct WatchpointVariableContext
+  /// \brief Represents the context of a watchpoint variable.
+  ///
+  /// This struct encapsulates the information related to a watchpoint 
variable,
+  /// including the watch ID and the execution context in which it is being
+  /// used. This struct is passed as a Baton to the \b
+  /// VariableWatchpointDisabler breakpoint callback.
+  struct WatchpointVariableContext {
+/// \brief Constructor for WatchpointVariableContext.
+/// \param watch_id The ID of the watchpoint.
+/// \param exe_ctx The execution context associated with the watchpoint.
+WatchpointVariableContext(lldb::watch_id_t watch_id,
+  ExecutionContext exe_ctx)
+: watch_id(watch_id), exe_ctx(exe_ctx) {}
+
+lldb::watch_id_t watch_id; ///< The ID of the watchpoint.
+ExecutionContext
+exe_ctx; ///< The execution context associated with the watchpoint.
+  };
+
+  class WatchpointVariableBaton : public TypedBaton 
{
+  public:
+WatchpointVariableBaton(std::unique_ptr Data)
+: TypedBaton(std::move(Data)) {}
+  };
+
+  bool SetupVariableWatchpointDisabler(lldb::StackFrameSP frame_sp) const;
+
+  /// Callback routine to disable the watchpoint set on a local variable when
+  ///  it goes out of scope.
+  static bool VariableWatchpointDisabler(
+  void *baton, lldb_private::StoppointCallbackContext *context,
+  lldb::user_id_t break_id, lldb::user_id_t break_loc_id);
+
   void GetDescription(Stream *s, lldb::DescriptionLevel level);
   void Dump(Stream *s) const override;
   void DumpSnapshots(Stream *s, const char *prefix = nullptr) const;

diff  --git a/lldb/source/Breakpoint/Watchpoint.cpp 
b/lldb/source/Breakpoint/Watchpoint.cpp
index d8b8bd505db95..597e696c71276 100644
--- a/lldb/source/Breakpoint/Watchpoint.cpp
+++ b/lldb/source/Breakpoint/Watchpoint.cpp
@@ -83,6 +83,94 @@ void Watchpoint::SetCallback(WatchpointHitCallback callback,
   SendWatchpointChangedEvent(eWatchpointEventTypeCommandChanged);
 }
 
+bool Watchpoint::SetupVariableWatchpointDisabler(StackFrameSP frame_sp) const {
+  if (!frame_sp)
+return false;
+
+  ThreadSP thread_sp = frame_sp->GetThread();
+  if (!thread_sp)
+return false;
+
+  uint32_t return_frame_index =
+  thread_sp->GetSelectedFrameIndex(DoNoSelectMostRelevantFrame) + 1;
+  if (return_frame_index >= LLDB_INVALID_FRAME_ID)
+return false;
+
+  StackFrameSP return_frame_sp(
+  thread_sp->GetStackFrameAtIndex(return_frame_index));
+  if (!return_frame_sp)
+return false;
+
+  ExecutionContext exe_ctx(return_frame_sp);
+  TargetSP target_sp = exe_ctx.GetTargetSP();
+  if (!target_sp)
+return false;
+
+  Address return_address(return_frame_sp->GetFrameCodeAddress());
+  lldb::addr_t return_addr = return_address.GetLoadAddress(target_sp.get());
+  if (return_addr == LLDB

[Lldb-commits] [PATCH] D151950: [lldb] Unconditionally increment depth when printing children

[Dave Lee via Phabricator via lldb-commits]

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

thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151950

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

[Lldb-commits] [PATCH] D152324: [lldb][NFCI] Change return type of PersistentExpressionState::GetNextPersistentVariableName

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

In D152324#4403368 , @aprantl wrote:

> Is the idea to remove the ConstString from these APIs too? If yes, this LGTM, 
> otherwise it seems to needlessly complicate things.

Yes, I was going to remove ConstString from these other APIs too. I can let 
this patch sit here until I can do more work to make sure that happens though. 
I'm somewhat confident that removing ConstString from this area of LLDB is 
going to be better long-term, but I've been wrong about this before! :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152324

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

[Lldb-commits] [PATCH] D152324: [lldb][NFCI] Change return type of PersistentExpressionState::GetNextPersistentVariableName

[Jonas Devlieghere via Phabricator via lldb-commits]

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

LGTM




Comment at: 
lldb/source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.cpp:129-134
   llvm::SmallString<64> name;
   {
 llvm::raw_svector_ostream os(name);
 os << GetPersistentVariablePrefix(is_error)
<< m_next_persistent_variable_id++;
   }

If we're going to return a `std::string`, we might as well use a 
`raw_string_ostream` and simplify this function. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152324

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