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

[Sterling Augustine via Phabricator via lldb-commits]

saugustine added a comment.

What other progress reporting needs rate limiting?

To the best of my knowledge, we have only identified one location--this one. So 
I'm not sure a fully general solution is in order here under the YAGNI 
principle.

I favor rate limiting close to the source because generating events and 
throwing them away is pure waste and gives an overall sense to the user that 
lldb is slow.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152364

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

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

[Sterling Augustine via Phabricator via lldb-commits]

saugustine added a comment.

In D150805#4397940 , @rupprecht wrote:

> In D150805#4350849 , @JDevlieghere 
> wrote:
>
>> I also like Jordan's rate limiting idea. In my mind that should be a 
>> property of the broadcaster. Different tools (e.g. vscode vs the command 
>> line) could specify different values when register a listener.
>
> This makes sense: we could augment `lldb_private::Listener` with additional 
> members that keep track of when the last broadcast time was, and if we're 
> rate limiting. Then we could change the implementation of 
> `Listener::GetEvent(lldb::EventSP _sp, const Timeout 
> )` to continuously churn through `m_events`, returning the most 
> recent one by the time the rate limiting window is over, and discarding any 
> intermediate ones in between.
>
> One thing I'm not sure of though is how we'll avoid an unnecessary pause for 
> rate limiting on the last item. This patch avoids that because it checks 
> `data->GetCompleted() != data->GetTotal()` to decide if we should actually 
> rate limit. In the generic case, how does the listener know that an event it 
> returns is the final one, and that it should ignore the rate limiting delay?
>
> I think we could address that by adding a `bool m_important` member to 
> `lldb::Event`, and then it would be up to the broadcaster to set that to true 
> for the last one (or any intermediate ones that are similarly important & 
> should be immediately shown, e.g. warnings/errors). Would that be reasonable?

The later a decision the decision not to update is made, the more work is 
wasted. Even the fairly simple solution that checked time in a somewhat 
expensive way (via the misnamed getCurrentTime that also gets memory used) 
ended up being slower overall. In my opinion, the problem is that a single-die 
is too small a unit of work to be worth reporting on.


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] D150805: Rate limit progress reporting

[Sterling Augustine via Phabricator via lldb-commits]

saugustine abandoned this revision.
saugustine added a comment.

I'll let someone with a better understanding of the proper implementation take 
it from here.


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] D150805: Rate limit progress reporting

[Sterling Augustine via Phabricator via lldb-commits]

saugustine added a comment.

Any more thoughts on this from the reviewers?


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] D150805: Proof of concept for reducing progress-reporting frequency.

[Sterling Augustine via Phabricator via lldb-commits]

saugustine updated this revision to Diff 523951.
saugustine added a comment.

Moved the rate-limiting to Debugger.[cpp|h]

Also wrote a custom getCurrentTime function, which doesn't do the
much of the extra work the Timer.h version does.

With this change, the timing is much better:

On my local machine, for a 93k DIE application, I get the following
timings:

  

1 second rate limit
(lldb) log timers dump
580.971832328 sec (total: 580.972s; child: 0.000s; count: 93007) for void 
DWARFUnit::ExtractDIEsRWLocked()

  

0 second rate limit, but with this change in place
663.114765369 sec (total: 663.115s; child: 0.000s; count: 93007) for void 
DWARFUnit::ExtractDIEsRWLocked()

  

Without this change in place
651.826884735 sec (total: 651.827s; child: 0.000s; count: 93007) for void 
DWARFUnit::ExtractDIEsRWLocked()


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150805

Files:
  lldb/include/lldb/Core/Debugger.h
  lldb/source/Core/CoreProperties.td
  lldb/source/Core/Debugger.cpp

Index: lldb/source/Core/Debugger.cpp
===
--- lldb/source/Core/Debugger.cpp
+++ lldb/source/Core/Debugger.cpp
@@ -66,6 +66,7 @@
 #include "llvm/Support/Process.h"
 #include "llvm/Support/ThreadPool.h"
 #include "llvm/Support/Threading.h"
+#include "llvm/Support/Timer.h"
 #include "llvm/Support/raw_ostream.h"
 
 #include 
@@ -403,6 +404,17 @@
   return SetPropertyAtIndex(idx, show_progress);
 }
 
+uint64_t Debugger::GetRateLimitProgress() const {
+  const uint32_t idx = ePropertyRateLimitProgress;
+  return GetPropertyAtIndexAs(
+  idx, g_debugger_properties[idx].default_uint_value != 0);
+}
+
+bool Debugger::SetRateLimitProgress(uint64_t rate_limit) {
+  const uint32_t idx = ePropertyShowProgress;
+  return SetPropertyAtIndex(idx, rate_limit);
+}
+
 llvm::StringRef Debugger::GetShowProgressAnsiPrefix() const {
   const uint32_t idx = ePropertyShowProgressAnsiPrefix;
   return GetPropertyAtIndexAs(
@@ -1934,6 +1946,16 @@
   return {};
 }
 
+// Rate-limit calculations should be fast. TimePoints collect memory and
+// instruction counts, which is slow.
+static double getCurrentTime() {
+  using Seconds = std::chrono::duration>;
+  llvm::sys::TimePoint<> now;
+  std::chrono::nanoseconds user, sys;
+  llvm::sys::Process::GetTimeUsage(now, user, sys);
+  return Seconds(now.time_since_epoch()).count();
+}
+
 void Debugger::HandleProgressEvent(const lldb::EventSP _sp) {
   auto *data = ProgressEventData::GetEventDataFromEvent(event_sp.get());
   if (!data)
@@ -1976,6 +1998,13 @@
 
   StreamSP output = GetAsyncOutputStream();
 
+  // Rate limit progress messages, but always show the last event.
+  double current_time = getCurrentTime();
+  if (current_time < m_next_report_time &&
+  data->GetCompleted() != data->GetTotal())
+return;
+  m_next_report_time = current_time + GetRateLimitProgress();
+
   // Print over previous line, if any.
   output->Printf("\r");
 
@@ -1983,6 +2012,9 @@
 // Clear the current line.
 output->Printf("\x1B[2K");
 output->Flush();
+// This set of progress reports is complete. Reset to show the first
+// progress report of the next set.
+m_next_report_time = 0.0;
 return;
   }
 
Index: lldb/source/Core/CoreProperties.td
===
--- lldb/source/Core/CoreProperties.td
+++ lldb/source/Core/CoreProperties.td
@@ -147,6 +147,10 @@
 Global,
 DefaultTrue,
 Desc<"Whether to show progress or not if the debugger's output is an interactive color-enabled terminal.">;
+  def RateLimitProgress: Property<"rate-limit-progress", "UInt64">,
+Global,
+DefaultUnsignedValue<1>,
+Desc<"Seconds to wait between progress reports.">;
   def ShowProgressAnsiPrefix: Property<"show-progress-ansi-prefix", "String">,
 Global,
 DefaultStringValue<"${ansi.faint}">,
Index: lldb/include/lldb/Core/Debugger.h
===
--- lldb/include/lldb/Core/Debugger.h
+++ lldb/include/lldb/Core/Debugger.h
@@ -307,6 +307,10 @@
 
   bool SetShowProgress(bool show_progress);
 
+  uint64_t GetRateLimitProgress() const;
+
+  bool SetRateLimitProgress(uint64_t rate_limit);
+
   llvm::StringRef GetShowProgressAnsiPrefix() const;
 
   llvm::StringRef GetShowProgressAnsiSuffix() const;
@@ -668,6 +672,9 @@
 eBroadcastBitEventThreadIsListening = (1 << 0),
   };
 
+  // Used to rate-limit progress reports;
+  double m_next_report_time = 0.0;
+
 private:
   // Use Debugger::CreateInstance() to get a shared pointer to a new debugger
   // object
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D150805: Proof of concept for reducing progress-reporting frequency.

[Sterling Augustine via Phabricator via lldb-commits]

saugustine added a comment.

This update switches to a time-based approach as suggested by Jordan. However, 
the timing is about the same as the original. I believe because calling 
getCurrentTime every iteration is comparably slow as printing the progress 
report itself.

It probably is still a win over very slow connections, where printing is even 
slower and time would remain the same.

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.

Shall we just switch to a percentage? Printing it every percent update?

That has the issues Jordan described, where things appear to progress quickly, 
and then may grind to a halt due to some big DIE.

But I think the perfect shouldn't be the enemy of the good here.


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] D150805: Proof of concept for reducing progress-reporting frequency.

[Sterling Augustine via Phabricator via lldb-commits]

saugustine updated this revision to Diff 523191.
saugustine added a comment.

Switch rate-limiting to a time-based mechanism


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150805

Files:
  lldb/include/lldb/Core/Progress.h
  lldb/source/Core/Progress.cpp
  lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp


Index: lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
@@ -77,7 +77,7 @@
   Progress progress(
   llvm::formatv("Manually indexing DWARF for {0}", module_desc.GetData()),
   total_progress,
-  /*report_increment=*/ 1000);
+  /*seconds=*/ 1.0);
 
   std::vector sets(units_to_index.size());
 
Index: lldb/source/Core/Progress.cpp
===
--- lldb/source/Core/Progress.cpp
+++ lldb/source/Core/Progress.cpp
@@ -16,14 +16,15 @@
 
 std::atomic Progress::g_id(0);
 
-Progress::Progress(std::string title, uint64_t total, uint64_t 
report_increment,
+Progress::Progress(std::string title, uint64_t total, double seconds,
lldb_private::Debugger *debugger)
-: m_title(title), m_id(++g_id), m_completed(0),
-  m_report_increment(report_increment), m_last_reported(0), m_total(total) 
{
+: m_title(title), m_id(++g_id), m_completed(0), m_seconds(seconds),
+  m_total(total) {
   assert(total > 0);
   if (debugger)
 m_debugger_id = debugger->GetID();
   std::lock_guard guard(m_mutex);
+  m_last_report = llvm::TimeRecord::getCurrentTime().getWallTime();
   ReportProgress();
 }
 
@@ -53,11 +54,12 @@
 void Progress::ReportProgress(std::string update) {
   if (!m_complete) {
 // Make sure we only send one notification that indicates the progress is
-// complete.
+// complete, and that we do it only once every m_seconds.
 m_complete = m_completed == m_total;
+double current_time = llvm::TimeRecord::getCurrentTime().getWallTime();
 if (m_complete || m_completed == 0 ||
-m_completed >= m_last_reported + m_report_increment) {
-  m_last_reported = m_completed;
+current_time >= m_last_report + m_seconds) {
+  m_last_report = current_time;
   Debugger::ReportProgress(m_id, m_title, std::move(update), m_completed,
m_total, m_debugger_id);
 }
Index: lldb/include/lldb/Core/Progress.h
===
--- lldb/include/lldb/Core/Progress.h
+++ lldb/include/lldb/Core/Progress.h
@@ -11,6 +11,7 @@
 
 #include "lldb/Utility/ConstString.h"
 #include "lldb/lldb-types.h"
+#include "llvm/Support/Timer.h"
 #include 
 #include 
 #include 
@@ -67,13 +68,12 @@
   /// set to UINT64_MAX then an indeterminate progress indicator should be
   /// displayed.
   ///
-  /// @param [in] report_increment Notify only when progress has exceeded
-  /// this amount. Throttles messaging.
+  /// @param [in] seconds Rate limit reports to once per this many seconds.
+  /// Zero for every increment.
   ///
   /// @param [in] debugger An optional debugger pointer to specify that this
   /// progress is to be reported only to specific debuggers.
-  Progress(std::string title, uint64_t total = UINT64_MAX,
-   uint64_t report_increment = 1,
+  Progress(std::string title, uint64_t total = UINT64_MAX, double seconds = 
0.2,
lldb_private::Debugger *debugger = nullptr);
 
   /// Destroy the progress object.
@@ -105,10 +105,10 @@
   const uint64_t m_id;
   /// How much work ([0...m_total]) that has been completed.
   uint64_t m_completed;
-  /// Print a message when progress exceeds this amount.
-  uint64_t m_report_increment;
-  /// Progress at the time of last message.
-  uint64_t m_last_reported;
+  // Wall time at last progress report.
+  double m_last_report;
+  /// Rate limit reports to once every m_seconds.
+  double m_seconds;
   /// Total amount of work, UINT64_MAX for non deterministic progress.
   const uint64_t m_total;
   /// The optional debugger ID to report progress to. If this has no value then


Index: lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
@@ -77,7 +77,7 @@
   Progress progress(
   llvm::formatv("Manually indexing DWARF for {0}", module_desc.GetData()),
   total_progress,
-  /*report_increment=*/ 1000);
+  /*seconds=*/ 1.0);
 
   std::vector sets(units_to_index.size());
 
Index: lldb/source/Core/Progress.cpp
===
--- lldb/source/Core/Progress.cpp
+++ lldb/source/Core/Progress.cpp
@@ -16,14 +16,15 @@
 
 std::atomic 

[Lldb-commits] [PATCH] D150805: Proof of concept for reducing progress-reporting frequency.

[Sterling Augustine via Phabricator via lldb-commits]

saugustine updated this revision to Diff 523188.
saugustine added a comment.

Swtich rate-limiting to a time-based mechanism.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150805

Files:
  lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp


Index: lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
@@ -77,7 +77,7 @@
   Progress progress(
   llvm::formatv("Manually indexing DWARF for {0}", module_desc.GetData()),
   total_progress,
-  /*seconds=*/ 0.2);
+  /*seconds=*/ 1.0);
 
   std::vector sets(units_to_index.size());
 


Index: lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
@@ -77,7 +77,7 @@
   Progress progress(
   llvm::formatv("Manually indexing DWARF for {0}", module_desc.GetData()),
   total_progress,
-  /*seconds=*/ 0.2);
+  /*seconds=*/ 1.0);
 
   std::vector sets(units_to_index.size());
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D150805: Proof of concept for reducing progress-reporting frequency.

[Sterling Augustine via Phabricator via lldb-commits]

saugustine created this revision.
Herald added subscribers: arphaman, kristof.beyls.
Herald added a project: All.
saugustine requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Reporting progress for every DIE read turns out to be very slow when
run over a remote connection such as ssh. We have a report of it
taking over 30-minutes to load the Dwarf for Chrome via ssh (which
transfers every single write) and only about a minute over
Chrome-Remote Desktop, which is a video-conferencing style link, and
so doesn't update nearly as often.

For a 7k DIE target, this improves the speed of reading on my personal
machine (entirely local) by about 3%; data below. Over remote, slower
connections the increase is likely much greater.

Top of trunk:
(lldb) target create "crash_test"
Current executable set to 'crash_test' (aarch64).
(lldb) log timers dump
12.509606661 sec (total: 12.510s; child: 0.000s; count: 7826) for void 
DWARFUnit::ExtractDIEsRWLocked()
...

With this change:
(lldb) target create "crash_test"
Current executable set to 'crash_test' (aarch64).
(lldb) log timers dump
12.139054862 sec (total: 12.139s; child: 0.000s; count: 7826) for void 
DWARFUnit::ExtractDIEsRWLocked()


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D150805

Files:
  lldb/include/lldb/Core/Progress.h
  lldb/source/Core/Progress.cpp
  lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp


Index: lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
@@ -76,7 +76,8 @@
   const uint64_t total_progress = units_to_index.size() * 2 + 8;
   Progress progress(
   llvm::formatv("Manually indexing DWARF for {0}", module_desc.GetData()),
-  total_progress);
+  total_progress,
+  /*report_increment=*/ 1000);
 
   std::vector sets(units_to_index.size());
 
Index: lldb/source/Core/Progress.cpp
===
--- lldb/source/Core/Progress.cpp
+++ lldb/source/Core/Progress.cpp
@@ -16,9 +16,10 @@
 
 std::atomic Progress::g_id(0);
 
-Progress::Progress(std::string title, uint64_t total,
+Progress::Progress(std::string title, uint64_t total, uint64_t 
report_increment,
lldb_private::Debugger *debugger)
-: m_title(title), m_id(++g_id), m_completed(0), m_total(total) {
+: m_title(title), m_id(++g_id), m_completed(0),
+  m_report_increment(report_increment), m_last_reported(0), m_total(total) 
{
   assert(total > 0);
   if (debugger)
 m_debugger_id = debugger->GetID();
@@ -54,7 +55,11 @@
 // 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);
+if (m_complete || m_completed == 0 ||
+m_completed >= m_last_reported + m_report_increment) {
+  m_last_reported = m_completed;
+  Debugger::ReportProgress(m_id, m_title, std::move(update), m_completed,
+   m_total, m_debugger_id);
+}
   }
 }
Index: lldb/include/lldb/Core/Progress.h
===
--- lldb/include/lldb/Core/Progress.h
+++ lldb/include/lldb/Core/Progress.h
@@ -67,9 +67,13 @@
   /// set to UINT64_MAX then an indeterminate progress indicator should be
   /// displayed.
   ///
+  /// @param [in] report_increment Notify only when progress has exceeded
+  /// this amount. Throttles messaging.
+  ///
   /// @param [in] debugger An optional debugger pointer to specify that this
   /// progress is to be reported only to specific debuggers.
   Progress(std::string title, uint64_t total = UINT64_MAX,
+   uint64_t report_increment = 1,
lldb_private::Debugger *debugger = nullptr);
 
   /// Destroy the progress object.
@@ -101,6 +105,10 @@
   const uint64_t m_id;
   /// How much work ([0...m_total]) that has been completed.
   uint64_t m_completed;
+  /// Print a message when progress exceeds this amount.
+  uint64_t m_report_increment;
+  /// Progress at the time of last message.
+  uint64_t m_last_reported;
   /// Total amount of work, UINT64_MAX for non deterministic progress.
   const uint64_t m_total;
   /// The optional debugger ID to report progress to. If this has no value then


Index: lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
@@ -76,7 +76,8 @@
   const uint64_t total_progress = units_to_index.size() * 2 + 8;
   Progress progress(
   llvm::formatv("Manually indexing DWARF for {0}", 

[Lldb-commits] [PATCH] D144228: [lldb] Stop generating swig bindings for SBLaunchInfo copy constructor

[Sterling Augustine via Phabricator via lldb-commits]

saugustine added a comment.

Thanks for the quick fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144228

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

[Lldb-commits] [PATCH] D142926: [lldb] Replace SB swig interfaces with API headers

[Sterling Augustine via Phabricator via lldb-commits]

saugustine added a comment.

I don't think this necessarily matters or is a reason to revert, but we are 
seeing issues with this patch and swig 3.0.2, that disappear with swig 4.0.

The docs claim that versions past 3.0 are supported (except for some odd corner 
cases).

The failure is:

  Traceback (most recent call last):
File 
"/build/work/37f17038e9cfbfbee7db556850bc7314bdb1/google3/runfiles/google3/third_party/llvm/llvm-project/lldb/test/API/functionalities/breakpoint/address_breakpoints/TestAddressBreakpoints.py",
 line 19, in test_address_breakpoints
  self.address_breakpoints()
File 
"/build/work/37f17038e9cfbfbee7db556850bc7314bdb1/google3/runfiles/google3/third_party/llvm/llvm-project/lldb/test/API/functionalities/breakpoint/address_breakpoints/TestAddressBreakpoints.py",
 line 50, in address_breakpoints
  launch_info = lldb.SBLaunchInfo(None)
File 
"/build/work/37f17038e9cfbfbee7db556850bc7314bdb1/google3/runfiles/google3/third_party/py/lldb/dotest_wrapper.par/google3/third_party/llvm/llvm-project/lldb/lib/python/site-packages/lldb/__init__.py",
 line 6239, in __init__
  this = _lldb.new_SBLaunchInfo(*args)
  ValueError: invalid null reference in method 'new_SBLaunchInfo', argument 1 
of type 'lldb::SBLaunchInfo const &'
  
Config=x86_64-/build/work/37f17038e9cfbfbee7db556850bc7314bdb1/google3/runfiles/google3/devtools/c/lldb/testing/clang

If I modify the line

  launch_info = lldb.SBLaunchInfo(None)

to

  launch_info = lldb.SBLaunchInfo("")

I get this error, which gives a clue to the fix:

  NotImplementedError: Wrong number or type of arguments for overloaded 
function 'new_SBLaunchInfo'.
Possible C/C++ prototypes are:
  lldb::SBLaunchInfo::SBLaunchInfo(char const **)
  lldb::SBLaunchInfo::SBLaunchInfo(lldb::SBLaunchInfo const &)

So if I use a vector of strings to match the first overload, the error 
disappears and the tests pass:

  launch_info = lldb.SBLaunchInfo(["", ""])

Not sure the best way forward here. Either to abandon old versions of swig, or 
to clean up the launch command.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142926

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

[Lldb-commits] [PATCH] D143955: Revert "[lldb] Use portable format string PRIx64"

[Sterling Augustine via Phabricator via lldb-commits]

saugustine added a comment.

Committed as  5402110e0123ca323a5f6eaa3ed225027ce0179b 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143955

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

[Lldb-commits] [PATCH] D121732: Avoid using a variable-sized array for a tiny allocation.

[Sterling Augustine via Phabricator via lldb-commits]

saugustine added inline comments.



Comment at: lldb/source/Expression/DWARFExpression.cpp:1309
 stack.back().GetScalar() = DerefSizeExtractDataHelper(
-addr_bytes, sizeof(addr_bytes), objfile->GetByteOrder(), size);
+addr_bytes, size, objfile->GetByteOrder(), size);
 stack.back().ClearContext();

JDevlieghere wrote:
> Why size and not `8`?
Because that preserves the semantics of the original code.  The original code 
allocates anywhere from 1 to 8 bytes, based on size ```uint8_t 
addr_bytes[size];```. Then reads that amount from memory (line 1303 - 1304), 
and then passes the size of the allocation on this line (via 
```sizeof(addr_bytes)```).

Always passing eight here would be wrong, because the size of the pointer could 
have been anywhere from 1 to 8.

Not also how line 1309 read size-bytes read into addr_bytes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121732

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

[Lldb-commits] [PATCH] D121408: Fixing DWARFExpression handling of ValueType::FileAddress case for DW_OP_deref_size

[Sterling Augustine via Phabricator via lldb-commits]

saugustine added a comment.

Fix for the variable-sized-array issue in rG7518e0ff63cd 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121408

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

[Lldb-commits] [PATCH] D121732: Avoid using a variable-sized array for a tiny allocation.

[Sterling Augustine via Phabricator via lldb-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rG7518e0ff63cd: Avoid using a variable-sized array for a tiny 
allocation. (authored by saugustine).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121732

Files:
  lldb/source/Expression/DWARFExpression.cpp


Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -1297,7 +1297,7 @@
 addr_t load_addr = *maybe_load_addr;
 
 if (load_addr == LLDB_INVALID_ADDRESS && so_addr.IsSectionOffset()) {
-  uint8_t addr_bytes[size];
+  uint8_t addr_bytes[8];
   Status error;
 
   if (exe_ctx->GetTargetRef().ReadMemory(
@@ -1306,7 +1306,7 @@
 ObjectFile *objfile = module_sp->GetObjectFile();
 
 stack.back().GetScalar() = DerefSizeExtractDataHelper(
-addr_bytes, sizeof(addr_bytes), objfile->GetByteOrder(), size);
+addr_bytes, size, objfile->GetByteOrder(), size);
 stack.back().ClearContext();
 break;
   } else {


Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -1297,7 +1297,7 @@
 addr_t load_addr = *maybe_load_addr;
 
 if (load_addr == LLDB_INVALID_ADDRESS && so_addr.IsSectionOffset()) {
-  uint8_t addr_bytes[size];
+  uint8_t addr_bytes[8];
   Status error;
 
   if (exe_ctx->GetTargetRef().ReadMemory(
@@ -1306,7 +1306,7 @@
 ObjectFile *objfile = module_sp->GetObjectFile();
 
 stack.back().GetScalar() = DerefSizeExtractDataHelper(
-addr_bytes, sizeof(addr_bytes), objfile->GetByteOrder(), size);
+addr_bytes, size, objfile->GetByteOrder(), size);
 stack.back().ClearContext();
 break;
   } else {
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D121732: Avoid using a variable-sized array for a tiny allocation.

[Sterling Augustine via Phabricator via lldb-commits]

saugustine created this revision.
Herald added a project: All.
saugustine requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D121732

Files:
  lldb/source/Expression/DWARFExpression.cpp


Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -1297,7 +1297,7 @@
 addr_t load_addr = *maybe_load_addr;
 
 if (load_addr == LLDB_INVALID_ADDRESS && so_addr.IsSectionOffset()) {
-  uint8_t addr_bytes[size];
+  uint8_t addr_bytes[8];
   Status error;
 
   if (exe_ctx->GetTargetRef().ReadMemory(
@@ -1306,7 +1306,7 @@
 ObjectFile *objfile = module_sp->GetObjectFile();
 
 stack.back().GetScalar() = DerefSizeExtractDataHelper(
-addr_bytes, sizeof(addr_bytes), objfile->GetByteOrder(), size);
+addr_bytes, size, objfile->GetByteOrder(), size);
 stack.back().ClearContext();
 break;
   } else {


Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -1297,7 +1297,7 @@
 addr_t load_addr = *maybe_load_addr;
 
 if (load_addr == LLDB_INVALID_ADDRESS && so_addr.IsSectionOffset()) {
-  uint8_t addr_bytes[size];
+  uint8_t addr_bytes[8];
   Status error;
 
   if (exe_ctx->GetTargetRef().ReadMemory(
@@ -1306,7 +1306,7 @@
 ObjectFile *objfile = module_sp->GetObjectFile();
 
 stack.back().GetScalar() = DerefSizeExtractDataHelper(
-addr_bytes, sizeof(addr_bytes), objfile->GetByteOrder(), size);
+addr_bytes, size, objfile->GetByteOrder(), size);
 stack.back().ClearContext();
 break;
   } else {
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D121408: Fixing DWARFExpression handling of ValueType::FileAddress case for DW_OP_deref_size

[Sterling Augustine via Phabricator via lldb-commits]

saugustine added a comment.
Herald added a subscriber: JDevlieghere.

This change uses a variable-sized array, which is c99 only:

  
third_party/llvm/llvm-project/lldb/source/Expression/DWARFExpression.cpp:1300:30:
 error: variable length arrays are a C99 feature [-Werror,-Wvla-extension]
uint8_t addr_bytes[size];
   ^~~~
  
third_party/llvm/llvm-project/lldb/source/Expression/DWARFExpression.cpp:1300:30:
 note: read of non-const variable 'size' is not allowed in a constant expression
  
third_party/llvm/llvm-project/lldb/source/Expression/DWARFExpression.cpp:1242:15:
 note: declared here
uint8_t size = opcodes.GetU8();

But I the size of the array is limited to only to the address size of the 
target machine, so converting that to a simple eight-byte array and then using 
size instead of sizeof(addr_bytes) should be fine. I have a change in testing 
to do that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121408

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

[Lldb-commits] [PATCH] D107660: [lldb] Upstream support for Foundation constant classes

[Sterling Augustine via Phabricator via lldb-commits]

saugustine added a comment.

Some people complain if I revert without notification. So I always notify as 
soon as I find the patch with the problem. Some reverts are more complicated 
than others too.

I guess it adds more noise but I try to default to more communication over less.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107660

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

[Lldb-commits] [PATCH] D107660: [lldb] Upstream support for Foundation constant classes

[Sterling Augustine via Phabricator via lldb-commits]

saugustine added a comment.

Thanks for looking. Reverted with 4e5af6ef48590e7248e344ddabf245bb3de71c51 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107660

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

[Lldb-commits] [PATCH] D107660: [lldb] Upstream support for Foundation constant classes

[Sterling Augustine via Phabricator via lldb-commits]

saugustine added a comment.

This change breaks various build bots with a missing file. See below. Reverting 
shortly.

For example:

https://lab.llvm.org/buildbot/#/builders/68/builds/16816

/usr/bin/clang++  -DGTEST_HAS_RTTI=0 -DHAVE_ROUND -D_DEBUG -D_GNU_SOURCE 
-D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS 
-Itools/lldb/source/Plugins/Language/ObjC 
-I/home/worker/2.0.1/lldb-x86_64-debian/llvm-project/lldb/source/Plugins/Language/ObjC
 -Itools/lldb/source 
-I/home/worker/2.0.1/lldb-x86_64-debian/llvm-project/lldb/include 
-Itools/lldb/include -Iinclude 
-I/home/worker/2.0.1/lldb-x86_64-debian/llvm-project/llvm/include 
-I/usr/include/python3.7m 
-I/home/worker/2.0.1/lldb-x86_64-debian/llvm-project/llvm/../clang/include 
-Itools/lldb/../clang/include 
-I/home/worker/2.0.1/lldb-x86_64-debian/llvm-project/lldb/source/. -fPIC 
-fvisibility-inlines-hidden -Werror=date-time 
-Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter 
-Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic 
-Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough 
-Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor 
-Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color 
-ffunction-sections -fdata-sections -Wno-deprecated-declarations 
-Wno-unknown-pragmas -Wno-strict-aliasing -Wno-deprecated-register 
-Wno-vla-extension -O3 -DNDEBUG-fno-exceptions -fno-rtti -UNDEBUG 
-std=c++14 -MD -MT 
tools/lldb/source/Plugins/Language/ObjC/CMakeFiles/lldbPluginObjCLanguage.dir/Cocoa.cpp.o
 -MF 
tools/lldb/source/Plugins/Language/ObjC/CMakeFiles/lldbPluginObjCLanguage.dir/Cocoa.cpp.o.d
 -o 
tools/lldb/source/Plugins/Language/ObjC/CMakeFiles/lldbPluginObjCLanguage.dir/Cocoa.cpp.o
 -c 
/home/worker/2.0.1/lldb-x86_64-debian/llvm-project/lldb/source/Plugins/Language/ObjC/Cocoa.cpp
/home/worker/2.0.1/lldb-x86_64-debian/llvm-project/lldb/source/Plugins/Language/ObjC/Cocoa.cpp:10:10:
 fatal error: 'objc/runtime.h' file not found
#include "objc/runtime.h"

  ^~~~


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107660

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

[Lldb-commits] [PATCH] D85145: Use syntax highlighting also in gui mode

[Sterling Augustine via Phabricator via lldb-commits]

saugustine added a comment.

This change has a subtle isse with wattr_get and friends: saved_opts isn't 
actually used, and the documentation for them says to always pass a nullptr. 
"The parameter opts is reserved for future use, applications must supply a null 
pointer."

I fixed it in 9dbdaea9a0e6f58417b5bd8980e7ea6723fd1783 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85145

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

[Lldb-commits] [PATCH] D69143: (NFC) Delete variable made unused by llvm-svn: 375160

[Sterling Augustine via Phabricator via lldb-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rGbbbc873f83e4: (NFC) Delete variable made unused by llvm-svn: 
375160 (authored by saugustine).
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69143

Files:
  lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp


Index: lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
===
--- lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
+++ lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
@@ -1352,7 +1352,6 @@
   if (!regex.IsValid())
 return;
 
-  auto old_size = sc_list.GetSize();
   CacheFunctionNames();
 
   std::set resolved_ids;


Index: lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
===
--- lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
+++ lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
@@ -1352,7 +1352,6 @@
   if (!regex.IsValid())
 return;
 
-  auto old_size = sc_list.GetSize();
   CacheFunctionNames();
 
   std::set resolved_ids;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D69119: Modernize the rest of the Find.* API (NFC)

[Sterling Augustine via Phabricator via lldb-commits]

saugustine added a comment.

This breaks the build due to 
lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:1355:8: error: unused 
variable 'old_size' [-Werror,-Wunused-variable]

I will check in a fix shortly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69119



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