[Lldb-commits] [lldb] [lldb] Expand background symbol lookup (PR #80890)

[Jonas Devlieghere via lldb-commits]

JDevlieghere wrote:

@clayborg The idea behind the feature is to get symbols for things that are 
relevant to the user. Right now, that's only hooked up for images that appear 
in the stack trace, but there are certainly other places this would be useful. 
So yeah, I absolutely expect this to apply to other things in the future as 
well. 

We currently check the setting right before downloading, so in a generic place 
that would be shared by all the other places that could trigger a download. 
Therefore I think the "mode" should be generic. If we have different triggers 
we could have separate settings to turn those things off (e.g. only download 
symbols for images in the stack trace, not for address lookups). So down the 
line it could look something like this:

```
(lldb) settings set symbols.auto-download background
(lldb) settings set symbols.auto-download-stack-symbols true
(lldb) settings set symbols.auto-download-address-lookup false
```

Let me know if you think it's critical to add 
`symbols.auto-download-stack-symbols` now in addition to 
`symbols.auto-download` (which I like better than `symbols.download`, thanks 
for the suggestion!) . 

https://github.com/llvm/llvm-project/pull/80890
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] [NFC] Remove min pkt size for compression setting (PR #81075)

[Jonas Devlieghere via lldb-commits]

https://github.com/JDevlieghere commented:

拾

https://github.com/llvm/llvm-project/pull/81075
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Fix printf formatting of std::time_t seconds (PR #81078)

[Jonas Devlieghere via lldb-commits]

https://github.com/JDevlieghere approved this pull request.


https://github.com/llvm/llvm-project/pull/81078
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] [NFC] Remove min pkt size for compression setting (PR #81075)

[Jason Molenda via lldb-commits]

https://github.com/jasonmolenda closed 
https://github.com/llvm/llvm-project/pull/81075
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 2df42fe - [lldb] [NFC] Remove min pkt size for compression setting (#81075)

[via lldb-commits]

Author: Jason Molenda
Date: 2024-02-07T18:55:24-08:00
New Revision: 2df42fe0efd92a89ae191a598e2727c7b63de80b

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

LOG: [lldb] [NFC] Remove min pkt size for compression setting (#81075)

debugserver will not compress small packets; the overhead of the
compression header makes this useful for larger packets. I default to
384 bytes in debugserver, and added an option for lldb to request a
different cutoff. This option has never been used in lldb in the past
nine years, so I don't think there's any point to keeping it around.

Added: 


Modified: 
lldb/docs/lldb-gdb-remote.txt
lldb/tools/debugserver/source/RNBRemote.cpp

Removed: 




diff  --git a/lldb/docs/lldb-gdb-remote.txt b/lldb/docs/lldb-gdb-remote.txt
index 76ac3f28d73b0..6c29de61daba7 100644
--- a/lldb/docs/lldb-gdb-remote.txt
+++ b/lldb/docs/lldb-gdb-remote.txt
@@ -1998,16 +1998,12 @@ for this region.
 //  If the debug stub can support compression, it indictes this in the reply 
of the
 //  "qSupported" packet.  e.g.
 //   LLDB SENDS:qSupported:xmlRegisters=i386,arm,mips
-//   STUB REPLIES:  
qXfer:features:read+;SupportedCompressions=lzfse,zlib-deflate,lz4,lzma;DefaultCompressionMinSize=384
+//   STUB REPLIES:  
qXfer:features:read+;SupportedCompressions=lzfse,zlib-deflate,lz4,lzma;
 //
 //  If lldb knows how to use any of these compression algorithms, it can ask 
that this
-//  compression mode be enabled.  It may optionally change the minimum packet 
size
-//  where compression is used.  Typically small packets do not benefit from 
compression,
-//  as well as compression headers -- compression is most beneficial with 
larger packets.
+//  compression mode be enabled.  
 //
 //  QEnableCompression:type:zlib-deflate;
-//  or
-//  QEnableCompression:type:zlib-deflate;minsize:512;
 //
 //  The debug stub should reply with an uncompressed "OK" packet to indicate 
that the
 //  request was accepted.  All further packets the stub sends will use this 
compression.

diff  --git a/lldb/tools/debugserver/source/RNBRemote.cpp 
b/lldb/tools/debugserver/source/RNBRemote.cpp
index 20384920823d2..feea4c914ec53 100644
--- a/lldb/tools/debugserver/source/RNBRemote.cpp
+++ b/lldb/tools/debugserver/source/RNBRemote.cpp
@@ -3575,8 +3575,6 @@ rnb_err_t RNBRemote::HandlePacket_qSupported(const char 
*p) {
 
   if (enable_compression) {
 reply << "SupportedCompressions=lzfse,zlib-deflate,lz4,lzma;";
-reply << "DefaultCompressionMinSize=" << std::dec << m_compression_minsize
-  << ";";
   }
 
 #if (defined(__arm64__) || defined(__aarch64__))
@@ -4482,46 +4480,25 @@ rnb_err_t 
RNBRemote::HandlePacket_SetEnableAsyncProfiling(const char *p) {
   return SendPacket("OK");
 }
 
-// QEnableCompression:type:;minsize:;
+// QEnableCompression:type:;
 //
 // type: must be a type previously reported by the qXfer:features:
 // SupportedCompressions list
-//
-// minsize: is optional; by default the qXfer:features:
-// DefaultCompressionMinSize value is used
-// debugserver may have a better idea of what a good minimum packet size to
-// compress is than lldb.
 
 rnb_err_t RNBRemote::HandlePacket_QEnableCompression(const char *p) {
   p += sizeof("QEnableCompression:") - 1;
 
-  size_t new_compression_minsize = m_compression_minsize;
-  const char *new_compression_minsize_str = strstr(p, "minsize:");
-  if (new_compression_minsize_str) {
-new_compression_minsize_str += strlen("minsize:");
-errno = 0;
-new_compression_minsize = strtoul(new_compression_minsize_str, NULL, 10);
-if (errno != 0 || new_compression_minsize == ULONG_MAX) {
-  new_compression_minsize = m_compression_minsize;
-}
-  }
-
   if (strstr(p, "type:zlib-deflate;") != nullptr) {
 EnableCompressionNextSendPacket(compression_types::zlib_deflate);
-m_compression_minsize = new_compression_minsize;
 return SendPacket("OK");
   } else if (strstr(p, "type:lz4;") != nullptr) {
 EnableCompressionNextSendPacket(compression_types::lz4);
-m_compression_minsize = new_compression_minsize;
 return SendPacket("OK");
   } else if (strstr(p, "type:lzma;") != nullptr) {
 EnableCompressionNextSendPacket(compression_types::lzma);
-m_compression_minsize = new_compression_minsize;
 return SendPacket("OK");
   } else if (strstr(p, "type:lzfse;") != nullptr) {
 EnableCompressionNextSendPacket(compression_types::lzfse);
-m_compression_minsize = new_compression_minsize;
 return SendPacket("OK");
   }
 



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

[Lldb-commits] [lldb] [lldb] [NFC] Remove min pkt size for compression setting (PR #81075)

[Jason Molenda via lldb-commits]

jasonmolenda wrote:

Thanks for the review.  Yeah, testing isn't simple today because debugserver 
only enables compression on always-remote systems (iOS, watchOS, etc) (which 
may even be communicating over Bluetooth/Wifi), it doesn't enable this on 
macOS.  We could have a mode to enable it on macOS for testing instead of a 
compile-time check like now.  Now that mac-to-mac debugging is more common, 
this compile-time choice of when to enable it may need revisiting.

https://github.com/llvm/llvm-project/pull/81075
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] [NFC] Remove min pkt size for compression setting (PR #81075)

[Alex Langford via lldb-commits]

https://github.com/bulbazord approved this pull request.

LGTM. I also noticed there's no tests for this functionality...

https://github.com/llvm/llvm-project/pull/81075
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Refactor GetFormatFromCString to always check for partial matches (NFC) (PR #81018)

[Alex Langford via lldb-commits]

https://github.com/bulbazord approved this pull request.

Makes sense to me. If the test suite is happy then I think this is fine.

https://github.com/llvm/llvm-project/pull/81018
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Fix printf formatting of std::time_t seconds (PR #81078)

[via lldb-commits]

llvmbot wrote:




@llvm/pr-subscribers-lldb

Author: Jason Molenda (jasonmolenda)


Changes

This formatter
https://github.com/llvm/llvm-project/pull/78609
was originally passing the signed seconds (which can refer to times in the 
past) with an unsigned printf formatter, and had tests that expected to see 
negative values from the printf which always failed on macOS.  I'm not clear 
how they ever passed on any platform.

Fix the printf to print seconds as a signed value, and re-enable the tests.

---
Full diff: https://github.com/llvm/llvm-project/pull/81078.diff


2 Files Affected:

- (modified) lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp (+3-3) 
- (modified) 
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/chrono/TestDataFormatterLibcxxChrono.py
 (+14-16) 


``diff
diff --git a/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp 
b/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
index d0bdbe1fd4d91..b37544f6dd3ad 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
@@ -1105,7 +1105,7 @@ bool 
lldb_private::formatters::LibcxxChronoSysSecondsSummaryProvider(
 
   const std::time_t seconds = ptr_sp->GetValueAsSigned(0);
   if (seconds < chrono_timestamp_min || seconds > chrono_timestamp_max)
-stream.Printf("timestamp=%" PRIu64 " s", static_cast(seconds));
+stream.Printf("timestamp=%" PRId64 " s", static_cast(seconds));
   else {
 std::array str;
 std::size_t size =
@@ -1113,8 +1113,8 @@ bool 
lldb_private::formatters::LibcxxChronoSysSecondsSummaryProvider(
 if (size == 0)
   return false;
 
-stream.Printf("date/time=%s timestamp=%" PRIu64 " s", str.data(),
-  static_cast(seconds));
+stream.Printf("date/time=%s timestamp=%" PRId64 " s", str.data(),
+  static_cast(seconds));
   }
 
   return true;
diff --git 
a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/chrono/TestDataFormatterLibcxxChrono.py
 
b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/chrono/TestDataFormatterLibcxxChrono.py
index 9706f9e94e922..a90fb828d121a 100644
--- 
a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/chrono/TestDataFormatterLibcxxChrono.py
+++ 
b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/chrono/TestDataFormatterLibcxxChrono.py
@@ -54,17 +54,16 @@ def test_with_run_command(self):
 substrs=["ss_0 = date/time=1970-01-01T00:00:00Z timestamp=0 s"],
 )
 
-# FIXME disabled temporarily, macOS is printing this as an unsigned?
-#self.expect(
-#"frame variable ss_neg_date_time",
-#substrs=[
-#"ss_neg_date_time = date/time=-32767-01-01T00:00:00Z 
timestamp=-1096193779200 s"
-#],
-#)
-#self.expect(
-#"frame variable ss_neg_seconds",
-#substrs=["ss_neg_seconds = timestamp=-1096193779201 s"],
-#)
+self.expect(
+"frame variable ss_neg_date_time",
+substrs=[
+"ss_neg_date_time = date/time=-32767-01-01T00:00:00Z 
timestamp=-1096193779200 s"
+],
+)
+self.expect(
+"frame variable ss_neg_seconds",
+substrs=["ss_neg_seconds = timestamp=-1096193779201 s"],
+)
 
 self.expect(
 "frame variable ss_pos_date_time",
@@ -77,11 +76,10 @@ def test_with_run_command(self):
 substrs=["ss_pos_seconds = timestamp=971890963200 s"],
 )
 
-# FIXME disabled temporarily, macOS is printing this as an unsigned?
-#self.expect(
-#"frame variable ss_min",
-#substrs=["ss_min = timestamp=-9223372036854775808 s"],
-#)
+self.expect(
+"frame variable ss_min",
+substrs=["ss_min = timestamp=-9223372036854775808 s"],
+)
 self.expect(
 "frame variable ss_max",
 substrs=["ss_max = timestamp=9223372036854775807 s"],

``




https://github.com/llvm/llvm-project/pull/81078
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][libc++] Adds system_clock data formatters. (PR #78609)

[Jason Molenda via lldb-commits]

jasonmolenda wrote:

Created a new PR with my proposed fix 
https://github.com/llvm/llvm-project/pull/81078

https://github.com/llvm/llvm-project/pull/78609
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Fix printf formatting of std::time_t seconds (PR #81078)

[Jason Molenda via lldb-commits]

https://github.com/jasonmolenda created 
https://github.com/llvm/llvm-project/pull/81078

This formatter
https://github.com/llvm/llvm-project/pull/78609
was originally passing the signed seconds (which can refer to times in the 
past) with an unsigned printf formatter, and had tests that expected to see 
negative values from the printf which always failed on macOS.  I'm not clear 
how they ever passed on any platform.

Fix the printf to print seconds as a signed value, and re-enable the tests.

>From 4477bfb879c461253e62212011497da205a7ed74 Mon Sep 17 00:00:00 2001
From: Jason Molenda 
Date: Wed, 7 Feb 2024 18:13:14 -0800
Subject: [PATCH] [lldb] Fix printf formatting of std::time_t seconds

This formatter
https://github.com/llvm/llvm-project/pull/78609
was originally passing the signed seconds (which can refer to times
in the past) with an unsigned printf formatter, and had tests that
expected to see negative values from the printf which always failed
on macOS.  I'm not clear how they ever passed on any platform.

Fix the printf to print seconds as a signed value, and re-enable
the tests.
---
 .../Plugins/Language/CPlusPlus/LibCxx.cpp |  6 ++--
 .../chrono/TestDataFormatterLibcxxChrono.py   | 30 +--
 2 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp 
b/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
index d0bdbe1fd4d91a..b37544f6dd3ad3 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
@@ -1105,7 +1105,7 @@ bool 
lldb_private::formatters::LibcxxChronoSysSecondsSummaryProvider(
 
   const std::time_t seconds = ptr_sp->GetValueAsSigned(0);
   if (seconds < chrono_timestamp_min || seconds > chrono_timestamp_max)
-stream.Printf("timestamp=%" PRIu64 " s", static_cast(seconds));
+stream.Printf("timestamp=%" PRId64 " s", static_cast(seconds));
   else {
 std::array str;
 std::size_t size =
@@ -1113,8 +1113,8 @@ bool 
lldb_private::formatters::LibcxxChronoSysSecondsSummaryProvider(
 if (size == 0)
   return false;
 
-stream.Printf("date/time=%s timestamp=%" PRIu64 " s", str.data(),
-  static_cast(seconds));
+stream.Printf("date/time=%s timestamp=%" PRId64 " s", str.data(),
+  static_cast(seconds));
   }
 
   return true;
diff --git 
a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/chrono/TestDataFormatterLibcxxChrono.py
 
b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/chrono/TestDataFormatterLibcxxChrono.py
index 9706f9e94e922f..a90fb828d121a7 100644
--- 
a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/chrono/TestDataFormatterLibcxxChrono.py
+++ 
b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/chrono/TestDataFormatterLibcxxChrono.py
@@ -54,17 +54,16 @@ def test_with_run_command(self):
 substrs=["ss_0 = date/time=1970-01-01T00:00:00Z timestamp=0 s"],
 )
 
-# FIXME disabled temporarily, macOS is printing this as an unsigned?
-#self.expect(
-#"frame variable ss_neg_date_time",
-#substrs=[
-#"ss_neg_date_time = date/time=-32767-01-01T00:00:00Z 
timestamp=-1096193779200 s"
-#],
-#)
-#self.expect(
-#"frame variable ss_neg_seconds",
-#substrs=["ss_neg_seconds = timestamp=-1096193779201 s"],
-#)
+self.expect(
+"frame variable ss_neg_date_time",
+substrs=[
+"ss_neg_date_time = date/time=-32767-01-01T00:00:00Z 
timestamp=-1096193779200 s"
+],
+)
+self.expect(
+"frame variable ss_neg_seconds",
+substrs=["ss_neg_seconds = timestamp=-1096193779201 s"],
+)
 
 self.expect(
 "frame variable ss_pos_date_time",
@@ -77,11 +76,10 @@ def test_with_run_command(self):
 substrs=["ss_pos_seconds = timestamp=971890963200 s"],
 )
 
-# FIXME disabled temporarily, macOS is printing this as an unsigned?
-#self.expect(
-#"frame variable ss_min",
-#substrs=["ss_min = timestamp=-9223372036854775808 s"],
-#)
+self.expect(
+"frame variable ss_min",
+substrs=["ss_min = timestamp=-9223372036854775808 s"],
+)
 self.expect(
 "frame variable ss_max",
 substrs=["ss_max = timestamp=9223372036854775807 s"],

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

[Lldb-commits] [lldb] [lldb] [NFC] Remove min pkt size for compression setting (PR #81075)

[via lldb-commits]

llvmbot wrote:




@llvm/pr-subscribers-lldb

Author: Jason Molenda (jasonmolenda)


Changes

debugserver will not compress small packets; the overhead of the compression 
header makes this useful for larger packets.  I default to 384 bytes in 
debugserver, and added an option for lldb to request a different cutoff.  This 
option has never been used in lldb in the past nine years, so I don't think 
there's any point to keeping it around.

---
Full diff: https://github.com/llvm/llvm-project/pull/81075.diff


2 Files Affected:

- (modified) lldb/docs/lldb-gdb-remote.txt (+2-6) 
- (modified) lldb/tools/debugserver/source/RNBRemote.cpp (+1-24) 


``diff
diff --git a/lldb/docs/lldb-gdb-remote.txt b/lldb/docs/lldb-gdb-remote.txt
index 76ac3f28d73b0..6c29de61daba7 100644
--- a/lldb/docs/lldb-gdb-remote.txt
+++ b/lldb/docs/lldb-gdb-remote.txt
@@ -1998,16 +1998,12 @@ for this region.
 //  If the debug stub can support compression, it indictes this in the reply 
of the
 //  "qSupported" packet.  e.g.
 //   LLDB SENDS:qSupported:xmlRegisters=i386,arm,mips
-//   STUB REPLIES:  
qXfer:features:read+;SupportedCompressions=lzfse,zlib-deflate,lz4,lzma;DefaultCompressionMinSize=384
+//   STUB REPLIES:  
qXfer:features:read+;SupportedCompressions=lzfse,zlib-deflate,lz4,lzma;
 //
 //  If lldb knows how to use any of these compression algorithms, it can ask 
that this
-//  compression mode be enabled.  It may optionally change the minimum packet 
size
-//  where compression is used.  Typically small packets do not benefit from 
compression,
-//  as well as compression headers -- compression is most beneficial with 
larger packets.
+//  compression mode be enabled.  
 //
 //  QEnableCompression:type:zlib-deflate;
-//  or
-//  QEnableCompression:type:zlib-deflate;minsize:512;
 //
 //  The debug stub should reply with an uncompressed "OK" packet to indicate 
that the
 //  request was accepted.  All further packets the stub sends will use this 
compression.
diff --git a/lldb/tools/debugserver/source/RNBRemote.cpp 
b/lldb/tools/debugserver/source/RNBRemote.cpp
index 20384920823d2..feea4c914ec53 100644
--- a/lldb/tools/debugserver/source/RNBRemote.cpp
+++ b/lldb/tools/debugserver/source/RNBRemote.cpp
@@ -3575,8 +3575,6 @@ rnb_err_t RNBRemote::HandlePacket_qSupported(const char 
*p) {
 
   if (enable_compression) {
 reply << "SupportedCompressions=lzfse,zlib-deflate,lz4,lzma;";
-reply << "DefaultCompressionMinSize=" << std::dec << m_compression_minsize
-  << ";";
   }
 
 #if (defined(__arm64__) || defined(__aarch64__))
@@ -4482,46 +4480,25 @@ rnb_err_t 
RNBRemote::HandlePacket_SetEnableAsyncProfiling(const char *p) {
   return SendPacket("OK");
 }
 
-// QEnableCompression:type:;minsize:;
+// QEnableCompression:type:;
 //
 // type: must be a type previously reported by the qXfer:features:
 // SupportedCompressions list
-//
-// minsize: is optional; by default the qXfer:features:
-// DefaultCompressionMinSize value is used
-// debugserver may have a better idea of what a good minimum packet size to
-// compress is than lldb.
 
 rnb_err_t RNBRemote::HandlePacket_QEnableCompression(const char *p) {
   p += sizeof("QEnableCompression:") - 1;
 
-  size_t new_compression_minsize = m_compression_minsize;
-  const char *new_compression_minsize_str = strstr(p, "minsize:");
-  if (new_compression_minsize_str) {
-new_compression_minsize_str += strlen("minsize:");
-errno = 0;
-new_compression_minsize = strtoul(new_compression_minsize_str, NULL, 10);
-if (errno != 0 || new_compression_minsize == ULONG_MAX) {
-  new_compression_minsize = m_compression_minsize;
-}
-  }
-
   if (strstr(p, "type:zlib-deflate;") != nullptr) {
 EnableCompressionNextSendPacket(compression_types::zlib_deflate);
-m_compression_minsize = new_compression_minsize;
 return SendPacket("OK");
   } else if (strstr(p, "type:lz4;") != nullptr) {
 EnableCompressionNextSendPacket(compression_types::lz4);
-m_compression_minsize = new_compression_minsize;
 return SendPacket("OK");
   } else if (strstr(p, "type:lzma;") != nullptr) {
 EnableCompressionNextSendPacket(compression_types::lzma);
-m_compression_minsize = new_compression_minsize;
 return SendPacket("OK");
   } else if (strstr(p, "type:lzfse;") != nullptr) {
 EnableCompressionNextSendPacket(compression_types::lzfse);
-m_compression_minsize = new_compression_minsize;
 return SendPacket("OK");
   }
 

``




https://github.com/llvm/llvm-project/pull/81075
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] [NFC] Remove min pkt size for compression setting (PR #81075)

[Jason Molenda via lldb-commits]

https://github.com/jasonmolenda created 
https://github.com/llvm/llvm-project/pull/81075

debugserver will not compress small packets; the overhead of the compression 
header makes this useful for larger packets.  I default to 384 bytes in 
debugserver, and added an option for lldb to request a different cutoff.  This 
option has never been used in lldb in the past nine years, so I don't think 
there's any point to keeping it around.

>From 084a2ac7131d88fb07d9b9a66314c86717d2671d Mon Sep 17 00:00:00 2001
From: Jason Molenda 
Date: Wed, 7 Feb 2024 17:40:14 -0800
Subject: [PATCH] [lldb] [NFC] Remove min pkt size for compression setting

debugserver will not compress small packets; the overhead of the
compression header makes this useful for larger packets.  I default
to 384 bytes in debugserver, and added an option for lldb to request
a different cutoff.  This option has never been used in lldb in the
past nine years, so I don't think there's any point to keeping it
around.
---
 lldb/docs/lldb-gdb-remote.txt   |  8 ++-
 lldb/tools/debugserver/source/RNBRemote.cpp | 25 +
 2 files changed, 3 insertions(+), 30 deletions(-)

diff --git a/lldb/docs/lldb-gdb-remote.txt b/lldb/docs/lldb-gdb-remote.txt
index 76ac3f28d73b0..6c29de61daba7 100644
--- a/lldb/docs/lldb-gdb-remote.txt
+++ b/lldb/docs/lldb-gdb-remote.txt
@@ -1998,16 +1998,12 @@ for this region.
 //  If the debug stub can support compression, it indictes this in the reply 
of the
 //  "qSupported" packet.  e.g.
 //   LLDB SENDS:qSupported:xmlRegisters=i386,arm,mips
-//   STUB REPLIES:  
qXfer:features:read+;SupportedCompressions=lzfse,zlib-deflate,lz4,lzma;DefaultCompressionMinSize=384
+//   STUB REPLIES:  
qXfer:features:read+;SupportedCompressions=lzfse,zlib-deflate,lz4,lzma;
 //
 //  If lldb knows how to use any of these compression algorithms, it can ask 
that this
-//  compression mode be enabled.  It may optionally change the minimum packet 
size
-//  where compression is used.  Typically small packets do not benefit from 
compression,
-//  as well as compression headers -- compression is most beneficial with 
larger packets.
+//  compression mode be enabled.  
 //
 //  QEnableCompression:type:zlib-deflate;
-//  or
-//  QEnableCompression:type:zlib-deflate;minsize:512;
 //
 //  The debug stub should reply with an uncompressed "OK" packet to indicate 
that the
 //  request was accepted.  All further packets the stub sends will use this 
compression.
diff --git a/lldb/tools/debugserver/source/RNBRemote.cpp 
b/lldb/tools/debugserver/source/RNBRemote.cpp
index 20384920823d2..feea4c914ec53 100644
--- a/lldb/tools/debugserver/source/RNBRemote.cpp
+++ b/lldb/tools/debugserver/source/RNBRemote.cpp
@@ -3575,8 +3575,6 @@ rnb_err_t RNBRemote::HandlePacket_qSupported(const char 
*p) {
 
   if (enable_compression) {
 reply << "SupportedCompressions=lzfse,zlib-deflate,lz4,lzma;";
-reply << "DefaultCompressionMinSize=" << std::dec << m_compression_minsize
-  << ";";
   }
 
 #if (defined(__arm64__) || defined(__aarch64__))
@@ -4482,46 +4480,25 @@ rnb_err_t 
RNBRemote::HandlePacket_SetEnableAsyncProfiling(const char *p) {
   return SendPacket("OK");
 }
 
-// QEnableCompression:type:;minsize:;
+// QEnableCompression:type:;
 //
 // type: must be a type previously reported by the qXfer:features:
 // SupportedCompressions list
-//
-// minsize: is optional; by default the qXfer:features:
-// DefaultCompressionMinSize value is used
-// debugserver may have a better idea of what a good minimum packet size to
-// compress is than lldb.
 
 rnb_err_t RNBRemote::HandlePacket_QEnableCompression(const char *p) {
   p += sizeof("QEnableCompression:") - 1;
 
-  size_t new_compression_minsize = m_compression_minsize;
-  const char *new_compression_minsize_str = strstr(p, "minsize:");
-  if (new_compression_minsize_str) {
-new_compression_minsize_str += strlen("minsize:");
-errno = 0;
-new_compression_minsize = strtoul(new_compression_minsize_str, NULL, 10);
-if (errno != 0 || new_compression_minsize == ULONG_MAX) {
-  new_compression_minsize = m_compression_minsize;
-}
-  }
-
   if (strstr(p, "type:zlib-deflate;") != nullptr) {
 EnableCompressionNextSendPacket(compression_types::zlib_deflate);
-m_compression_minsize = new_compression_minsize;
 return SendPacket("OK");
   } else if (strstr(p, "type:lz4;") != nullptr) {
 EnableCompressionNextSendPacket(compression_types::lz4);
-m_compression_minsize = new_compression_minsize;
 return SendPacket("OK");
   } else if (strstr(p, "type:lzma;") != nullptr) {
 EnableCompressionNextSendPacket(compression_types::lzma);
-m_compression_minsize = new_compression_minsize;
 return SendPacket("OK");
   } else if (strstr(p, "type:lzfse;") != nullptr) {
 EnableCompressionNextSendPacket(compression_types::lzfse);
-m_compression_minsize = new_compression_minsize;
 return SendPacket("OK");
   }
 

[Lldb-commits] [lldb] Add more ways to find the .dwp file. (PR #81067)

[Alexander Yermolovich via lldb-commits]

ayermolo wrote:

> > Will this now work with .dwp files not having UUID?
> 
> No. If binairies have UUIDs (GNU build IDs), they need to match right now. 
> That is larger fix that involves adding a "enum UUIDFlavor" to the UUIDs so 
> we can ensure we aren't comparing two different things.
> 
> What Alexander is talking about is if we have a GNU build ID in ``, the 
> `.debug` file will have the same UUID, but llvm-dwp currently doesn't 
> copy the GNU build ID over into the `.dwp` file. This causes LLDB to not 
> allow the .dwp file to be loaded. The problem is if the .dwp file doesn't 
> have a UUID, it will make one up by calculating a CRC of the file itself, and 
> then we will compare a GNU build ID from `` to the CRC calculated by the 
> `.dwp` file and they won't match.
> 
> @dwblaikie do you know how accurate the DWO ID is? Can we avoid relying on 
> matching up the UUID on the .dwp file and solely rely on allowing it to be 
> loaded and rely on the DWO IDs matching between the skeleton unit and the 
> .dwo unit? If so, there is an easy fix I can make to this patch to solve that 
> problem.

Not sure I follow. For .dwo files path is described in Skeleton CU: 
DW_AT_comp_dir/DW_AT_dwo_name. The DWP file can have multiple CUs with 
different DWO IDs.

https://github.com/llvm/llvm-project/pull/81067
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Add more ways to find the .dwp file. (PR #81067)

[Greg Clayton via lldb-commits]

clayborg wrote:

> Will this now work with .dwp files not having UUID?

No. If binairies have UUIDs (GNU build IDs), they need to match right now. That 
is larger fix that involves adding a "enum UUIDFlavor" to the UUIDs so we can 
ensure we aren't comparing two different things.

What Alexander is talking about is if we have a GNU build ID in ``, the 
`.debug` file will have the same UUID, but llvm-dwp currently doesn't copy 
the GNU build ID over into the `.dwp` file. This causes LLDB to not allow the 
.dwp file to be loaded. The problem is if the .dwp file doesn't have a UUID, it 
will make one up by calculating a CRC of the file itself, and then we will 
compare a GNU build ID from `` to the CRC calculated by the `.dwp` file 
and they won't match.

@dwblaikie do you know how accurate the DWO ID is? Can we avoid relying on 
matching up the UUID on the .dwp file and solely rely on allowing it to be 
loaded and rely on the DWO IDs matching between the skeleton unit and the .dwo 
unit? If so, there is an easy fix I can make to this patch to solve that 
problem.


https://github.com/llvm/llvm-project/pull/81067
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Add more ways to find the .dwp file. (PR #81067)

[Kevin Frei via lldb-commits]

kevinfrei wrote:

> Will this now work with .dwp files not having UUID?

The lack of UUID is kinda why this is so important. The connection is strictly 
based on the filename. This just expands the variety of filenames that can be 
supported. One thing that's helpful is that the .gnu_debuglink can be a 
relative/absolute path, so it enables putting the files in a different location 
than right beside the binary, which is definitely an improvement.

https://github.com/llvm/llvm-project/pull/81067
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Add more ways to find the .dwp file. (PR #81067)

[Alexander Yermolovich via lldb-commits]

ayermolo wrote:

Will this now work with .dwp files not having UUID?

https://github.com/llvm/llvm-project/pull/81067
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Add more ways to find the .dwp file. (PR #81067)

[Greg Clayton via lldb-commits]

https://github.com/clayborg edited 
https://github.com/llvm/llvm-project/pull/81067
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Add more ways to find the .dwp file. (PR #81067)

[via lldb-commits]

llvmbot wrote:




@llvm/pr-subscribers-lldb

Author: Greg Clayton (clayborg)


Changes

When using split DWARF we can run into many different ways to store debug info:
- lldb loads "exe" which contains skeleton DWARF and needs to find 
"exe.dwp"
- lldb loads "exe" which is stripped but has .gnu_debuglink pointing to 
"exe.debug" with skeleton DWARF and needs to find "exe.dwp"
- lldb loads "exe" which is stripped but has .gnu_debuglink pointing to 
"exe.debug" with skeleton DWARF and needs to find 
"exe.debug.dwp"
- lldb loads "exe.debug" and needs to find "exe.dwp

Previously we only handled the first two cases. This patch adds support for the 
latter two.

---
Full diff: https://github.com/llvm/llvm-project/pull/81067.diff


3 Files Affected:

- (modified) lldb/include/lldb/Utility/FileSpecList.h (+4) 
- (modified) lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp (+44-17) 
- (modified) lldb/test/Shell/SymbolFile/DWARF/x86/dwp-separate-debug-file.cpp 
(+30) 


``diff
diff --git a/lldb/include/lldb/Utility/FileSpecList.h 
b/lldb/include/lldb/Utility/FileSpecList.h
index 49edc667ddd5b..6eb3bb9971f13 100644
--- a/lldb/include/lldb/Utility/FileSpecList.h
+++ b/lldb/include/lldb/Utility/FileSpecList.h
@@ -238,6 +238,10 @@ class FileSpecList {
   const_iterator begin() const { return m_files.begin(); }
   const_iterator end() const { return m_files.end(); }
 
+  llvm::iterator_range files() const {
+return llvm::make_range(begin(), end());
+  }
+
 protected:
   collection m_files; ///< A collection of FileSpec objects.
 };
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index 781f5c5a43677..487961fa7437f 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -4349,26 +4349,53 @@ SymbolFileDWARFDebugMap 
*SymbolFileDWARF::GetDebugMapSymfile() {
 
 const std::shared_ptr ::GetDwpSymbolFile() 
{
   llvm::call_once(m_dwp_symfile_once_flag, [this]() {
+// Create a list of files to try and append .dwp to
+FileSpecList symfiles;
+// Append the module's object file path.
+const FileSpec module_fspec = m_objfile_sp->GetModule()->GetFileSpec();
+symfiles.Append(module_fspec);
+// Append the object file for this SymbolFile only if it is different from
+// the module's file path. Our main module could be "a.out", our symbol 
file
+// could be "a.debug" and our ".dwp" file might be "a.debug.dwp" instead of
+// "a.out.dwp".
+const FileSpec symfile_fspec(m_objfile_sp->GetFileSpec());
+if (symfile_fspec != module_fspec) {
+  symfiles.Append(symfile_fspec);
+} else {
+  // If we don't have a separate debug info file, then try stripping the
+  // extension. We main module could be "a.debug" and the .dwp file could 
be
+  // "a.dwp" instead of "a.debug.dwp".
+  ConstString filename_no_ext =
+  module_fspec.GetFileNameStrippingExtension();
+  if (filename_no_ext != module_fspec.GetFilename()) {
+FileSpec module_spec_no_ext(module_fspec);
+module_spec_no_ext.SetFilename(filename_no_ext);
+symfiles.Append(module_spec_no_ext);
+  }
+}
+
+FileSpecList search_paths = Target::GetDefaultDebugFileSearchPaths();
 ModuleSpec module_spec;
 module_spec.GetFileSpec() = m_objfile_sp->GetFileSpec();
-module_spec.GetSymbolFileSpec() =
-FileSpec(m_objfile_sp->GetModule()->GetFileSpec().GetPath() + ".dwp");
-
 module_spec.GetUUID() = m_objfile_sp->GetUUID();
-FileSpecList search_paths = Target::GetDefaultDebugFileSearchPaths();
-FileSpec dwp_filespec =
-PluginManager::LocateExecutableSymbolFile(module_spec, search_paths);
-if (FileSystem::Instance().Exists(dwp_filespec)) {
-  DataBufferSP dwp_file_data_sp;
-  lldb::offset_t dwp_file_data_offset = 0;
-  ObjectFileSP dwp_obj_file = ObjectFile::FindPlugin(
-  GetObjectFile()->GetModule(), _filespec, 0,
-  FileSystem::Instance().GetByteSize(dwp_filespec), dwp_file_data_sp,
-  dwp_file_data_offset);
-  if (!dwp_obj_file)
-return;
-  m_dwp_symfile = std::make_shared(
-  *this, dwp_obj_file, DIERef::k_file_index_mask);
+for (const auto  : symfiles.files()) {
+  module_spec.GetSymbolFileSpec() =
+  FileSpec(symfile.GetPath() + ".dwp", symfile.GetPathStyle());
+  FileSpec dwp_filespec =
+  PluginManager::LocateExecutableSymbolFile(module_spec, search_paths);
+  if (FileSystem::Instance().Exists(dwp_filespec)) {
+DataBufferSP dwp_file_data_sp;
+lldb::offset_t dwp_file_data_offset = 0;
+ObjectFileSP dwp_obj_file = ObjectFile::FindPlugin(
+GetObjectFile()->GetModule(), _filespec, 0,
+FileSystem::Instance().GetByteSize(dwp_filespec), dwp_file_data_sp,
+dwp_file_data_offset);
+if (!dwp_obj_file)
+  return;
+   

[Lldb-commits] [lldb] Add more ways to find the .dwp file. (PR #81067)

[Greg Clayton via lldb-commits]

https://github.com/clayborg created 
https://github.com/llvm/llvm-project/pull/81067

When using split DWARF we can run into many different ways to store debug info:
- lldb loads "" which contains skeleton DWARF and needs to find ".dwp"
- lldb loads "" which is stripped but has .gnu_debuglink pointing to 
".debug" with skeleton DWARF and needs to find ".dwp"
- lldb loads "" which is stripped but has .gnu_debuglink pointing to 
".debug" with skeleton DWARF and needs to find ".debug.dwp"
- lldb loads ".debug" and needs to find ".dwp

Previously we only handled the first two cases. This patch adds support for the 
latter two.

>From 3c2f6039cf0e253d78b5193098b311028daaea72 Mon Sep 17 00:00:00 2001
From: Greg Clayton 
Date: Wed, 7 Feb 2024 16:43:50 -0800
Subject: [PATCH] Add more ways to find the .dwp file.

When using split DWARF we can run into many different ways to store debug info:
- lldb loads "" which contains skeleton DWARF and needs to find ".dwp"
- lldb loads "" which is stripped but has .gnu_debuglink pointing to 
".debug" with skeleton DWARF and needs to find ".dwp"
- lldb loads "" which is stripped but has .gnu_debuglink pointing to 
".debug" with skeleton DWARF and needs to find ".debug.dwp"
- lldb loads ".debug" and needs to find ".dwp

Previously we only handled the first two cases. This patch adds support for the 
latter two.
---
 lldb/include/lldb/Utility/FileSpecList.h  |  4 ++
 .../SymbolFile/DWARF/SymbolFileDWARF.cpp  | 61 +--
 .../DWARF/x86/dwp-separate-debug-file.cpp | 30 +
 3 files changed, 78 insertions(+), 17 deletions(-)

diff --git a/lldb/include/lldb/Utility/FileSpecList.h 
b/lldb/include/lldb/Utility/FileSpecList.h
index 49edc667ddd5b6..6eb3bb9971f13a 100644
--- a/lldb/include/lldb/Utility/FileSpecList.h
+++ b/lldb/include/lldb/Utility/FileSpecList.h
@@ -238,6 +238,10 @@ class FileSpecList {
   const_iterator begin() const { return m_files.begin(); }
   const_iterator end() const { return m_files.end(); }
 
+  llvm::iterator_range files() const {
+return llvm::make_range(begin(), end());
+  }
+
 protected:
   collection m_files; ///< A collection of FileSpec objects.
 };
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index 781f5c5a436778..487961fa7437fb 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -4349,26 +4349,53 @@ SymbolFileDWARFDebugMap 
*SymbolFileDWARF::GetDebugMapSymfile() {
 
 const std::shared_ptr ::GetDwpSymbolFile() 
{
   llvm::call_once(m_dwp_symfile_once_flag, [this]() {
+// Create a list of files to try and append .dwp to
+FileSpecList symfiles;
+// Append the module's object file path.
+const FileSpec module_fspec = m_objfile_sp->GetModule()->GetFileSpec();
+symfiles.Append(module_fspec);
+// Append the object file for this SymbolFile only if it is different from
+// the module's file path. Our main module could be "a.out", our symbol 
file
+// could be "a.debug" and our ".dwp" file might be "a.debug.dwp" instead of
+// "a.out.dwp".
+const FileSpec symfile_fspec(m_objfile_sp->GetFileSpec());
+if (symfile_fspec != module_fspec) {
+  symfiles.Append(symfile_fspec);
+} else {
+  // If we don't have a separate debug info file, then try stripping the
+  // extension. We main module could be "a.debug" and the .dwp file could 
be
+  // "a.dwp" instead of "a.debug.dwp".
+  ConstString filename_no_ext =
+  module_fspec.GetFileNameStrippingExtension();
+  if (filename_no_ext != module_fspec.GetFilename()) {
+FileSpec module_spec_no_ext(module_fspec);
+module_spec_no_ext.SetFilename(filename_no_ext);
+symfiles.Append(module_spec_no_ext);
+  }
+}
+
+FileSpecList search_paths = Target::GetDefaultDebugFileSearchPaths();
 ModuleSpec module_spec;
 module_spec.GetFileSpec() = m_objfile_sp->GetFileSpec();
-module_spec.GetSymbolFileSpec() =
-FileSpec(m_objfile_sp->GetModule()->GetFileSpec().GetPath() + ".dwp");
-
 module_spec.GetUUID() = m_objfile_sp->GetUUID();
-FileSpecList search_paths = Target::GetDefaultDebugFileSearchPaths();
-FileSpec dwp_filespec =
-PluginManager::LocateExecutableSymbolFile(module_spec, search_paths);
-if (FileSystem::Instance().Exists(dwp_filespec)) {
-  DataBufferSP dwp_file_data_sp;
-  lldb::offset_t dwp_file_data_offset = 0;
-  ObjectFileSP dwp_obj_file = ObjectFile::FindPlugin(
-  GetObjectFile()->GetModule(), _filespec, 0,
-  FileSystem::Instance().GetByteSize(dwp_filespec), dwp_file_data_sp,
-  dwp_file_data_offset);
-  if (!dwp_obj_file)
-return;
-  m_dwp_symfile = std::make_shared(
-  *this, dwp_obj_file, DIERef::k_file_index_mask);
+for (const auto  : symfiles.files()) {
+  module_spec.GetSymbolFileSpec() =
+  

[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)

[Jonas Devlieghere via lldb-commits]

JDevlieghere wrote:

> > My understanding is that the constructor conveys whether something is an 
> > "aggregate" progress event or not and they're broadcast differently so that 
> > the listener can decide how they want to receive these "aggregate" events.
> 
> My idea is the user decides how to listen to the events by which 
> `SBDebugger::eBroadcastBit` (`SBDebugger::eBroadcastBitProgress` or 
> `SBDebugger::eBroadcastBitProgressByCategory`) they listen to, and we will 
> deliver the events accordingly.

I think we're saying the same thing? 

> Yeah, with category, we will see "Indexing symbol table" and then we might 
> have both "a.out" and "b.out" details from two different categories needing 
> to be displayed. The debugger event that gets sent for 
> `eBroadcastBitProgressByCategory` might need some new accessors to account 
> for this.

That's a great suggestion. I hadn't considered also doing grouping by category 
for events that have details, but you're right that there's really no reason 
not to. In that case we don't need to change the constructor at all, and this 
is all controlled by the broadcast bit, which makes things even simpler. 

https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)

[Greg Clayton via lldb-commits]

clayborg wrote:

> > A few questions I have:
> > 
> > * do we really want each progress to select if it should be coalesced as a 
> > `Progress` constructor arguments? Or do we want a global setting on how 
> > progress events should be delivered?
> 
> My understanding is that the constructor conveys whether something is an 
> "aggregate" progress event or not and they're broadcast differently so that 
> the listener can decide how they want to receive these "aggregate" events.

My idea is the user decides how to listen to the events by which 
`SBDebugger::eBroadcastBit` (`SBDebugger::eBroadcastBitProgress` or 
`SBDebugger::eBroadcastBitProgressByCategory`) they listen to, and we will 
deliver the events accordingly. 

> 
> > * The current code adds a way to increment and decrement refcounts of 
> > ongoing progress categories (titles), but doesn't do anything with them, 
> > what do we envision happening with these ref counts?
> 
> With the previous idea in mind, you need the refcount in case there are 
> overlapping events:
> 
> ```
> eBroadcastBitProgress   [foo]
> eBroadcastBitProgress  [foo]
> eBroadcastBitProgressByCategory [foo   ]
> ---> (time)
> ```
> 
> So basically if you have two events `foo` that overlap as shown above, if 
> you're listening for `eBroadcastBitProgress` you get two events. If you're 
> listening for `eBroadcastBitProgressByCategory`. The refcount is used to make 
> sure you don't broadcast an events for `eBroadcastBitProgressByCategory` when 
> the first foo ends.

Yeah, with category, we will see "Indexing symbol table" and then we might have 
both "a.out" and "b.out" details from two different categories needing to be 
displayed. The debugger event that gets sent for 
`eBroadcastBitProgressByCategory` might need some new accessors to account for 
this.




https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Don't count all the frames just to skip the current inlined ones. (PR #80918)

[Jonas Devlieghere via lldb-commits]

https://github.com/JDevlieghere approved this pull request.


https://github.com/llvm/llvm-project/pull/80918
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)

[Jonas Devlieghere via lldb-commits]

JDevlieghere wrote:

> A few questions I have:
> 
> * do we really want each progress to select if it should be coalesced as a 
> `Progress` constructor arguments? Or do we want a global setting on how 
> progress events should be delivered?

My understanding is that the constructor conveys whether something is an 
"aggregate" progress event or not and they're broadcast differently so that the 
listener can decide how they want to receive these "aggregate" events. 

> * The current code adds a way to increment and decrement refcounts of ongoing 
> progress categories (titles), but doesn't do anything with them, what do we 
> envision happening with these ref counts?

With the previous idea in mind, you need the refcount in case there are 
overlapping events:

```
eBroadcastBitProgress   [foo]
eBroadcastBitProgress  [foo]
eBroadcastBitProgressByCategory [foo   ]
---> (time)
```

So basically if you have two events `foo` that overlap as shown above, if 
you're listening for `eBroadcastBitProgress` you get two events. If you're 
listening for `eBroadcastBitProgressByCategory`. The refcount is used to make 
sure you don't broadcast an events for `eBroadcastBitProgressByCategory` when 
the first foo ends. 

https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)

[Jonas Devlieghere via lldb-commits]

@@ -99,6 +105,10 @@ class Progress {
 private:
   void ReportProgress();
   static std::atomic g_id;
+  static std::atomic g_refcount;
+  /// Map that tracks each progress object and if we've seen its start and stop
+  /// events
+  static std::unordered_map g_map;

JDevlieghere wrote:

What's the benefit of doing this at the debugger level? The way I imagined this 
to work is like the other subsystems (similar to what Greg described inline). 
The downside of doing the bookkeeping in the debugger, is that we'll have to 
duplicate all this work across debuggers for progress events that are not tied 
to a single debugger. I don't know how easy it would be to change that (if we 
have access to the debugger everywhere we report progress today). Either way 
I'd like to understand the motivation behind it. 

https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)

[Greg Clayton via lldb-commits]

@@ -99,6 +105,10 @@ class Progress {
 private:
   void ReportProgress();
   static std::atomic g_id;
+  static std::atomic g_refcount;
+  /// Map that tracks each progress object and if we've seen its start and stop
+  /// events
+  static std::unordered_map g_map;

clayborg wrote:

Correct. Because we have to just in case the debugger pointer was set in the 
Progress, so we have to track it in each debugger just in case. 

https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)

[Greg Clayton via lldb-commits]

@@ -9,26 +9,34 @@
 #include "lldb/Core/Progress.h"
 
 #include "lldb/Core/Debugger.h"
-#include "lldb/Utility/StreamString.h"
 
 #include 
 
 using namespace lldb;
 using namespace lldb_private;
 
 std::atomic Progress::g_id(0);
+std::atomic Progress::g_refcount(1);
+std::unordered_map Progress::g_map = {};

clayborg wrote:

yes, might not be needed. Still needs to be threadsafe though if we do move it 
into the Debugger.h/.cpp

https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)

[Chelsea Cassanova via lldb-commits]

@@ -9,26 +9,34 @@
 #include "lldb/Core/Progress.h"
 
 #include "lldb/Core/Debugger.h"
-#include "lldb/Utility/StreamString.h"
 
 #include 
 
 using namespace lldb;
 using namespace lldb_private;
 
 std::atomic Progress::g_id(0);
+std::atomic Progress::g_refcount(1);
+std::unordered_map Progress::g_map = {};

chelcassanova wrote:

This is much better implementation of using a global map, but if we go with 
having the map being kept track of in each debugger object we might not need to 
have this implementation of a global map anymore.

https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)

[Chelsea Cassanova via lldb-commits]

@@ -99,6 +105,10 @@ class Progress {
 private:
   void ReportProgress();
   static std::atomic g_id;
+  static std::atomic g_refcount;
+  /// Map that tracks each progress object and if we've seen its start and stop
+  /// events
+  static std::unordered_map g_map;

chelcassanova wrote:

Having the debugger instance keep track of a map of progress reports falls in 
line with what I spoke about with Ismail offline and is the direction I'd vote 
to go in so that the bookkeeping isn't done in the progress objects (I'm sure 
this isn't impossible, but given that LLDB is hierarchical it would make more 
sense that debuggers have progress reports rather than progress reports looking 
after themselves).

So we still want to keep `is_debugger_specific`/ having a debugger pointer in a 
progress object, but do all the bookkeeping in all active debuggers and using 
that debugger pointer to only update the specific reports as needed if I'm 
understanding your approach correctly.

https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)

[Greg Clayton via lldb-commits]

@@ -99,6 +105,10 @@ class Progress {
 private:
   void ReportProgress();
   static std::atomic g_id;
+  static std::atomic g_refcount;
+  /// Map that tracks each progress object and if we've seen its start and stop
+  /// events
+  static std::unordered_map g_map;

clayborg wrote:

> We spoke about this a little offline but at the moment although Progress 
> reports hold on to debugger instances, these instances are null by default 
> and most Progress reports don't give a debugger instance in their 
> instantiation so we wouldn't be able to have a debugger instance hold on to 
> their own map of ongoing progress reports.
> 
> @clayborg Having a progress report holding on to a debugger instance is a 
> good idea, but if we want to use a map to keep track of ongoing reports we 
> need to know what will own the map? Do we want the map to be owned by a 
> debugger instance or could the progress class hold on to its own map of 
> ongoing reports? In the case of the former we would then want to enforce that 
> progress reports have valid debugger instances and in the latter we could 
> remove the `debugger` field from the reports altogether.

The main question is if we want all progress reports to follow the currently 
selected mode (coalesce or individual) or if we want them to be able to pick 
which one they are. I would vote to have a global setting that controls if we 
deliver things, but would love to hear what others think.

Each Progress instance _can_ have a debugger, but most don't, and those will 
get delivered to all debugger instances. So I would say that this map should 
live in each debugger object because `Progress` objects _can_ have a debugger 
set, and if they do, they will only get delivered to _that_ debugger. 

So my suggestion here is to:
- not change anything in Progress.h/cpp
- move the progress categorization code into each debugger object
- in `PrivateReportProgress` we check if anyone is listening to the new 
`eBroadcastBitProgressByCategory` bit, and then and _only_ then do we do any of 
the category counting.

So the code in `PrivateReportProgress` will now look something like:
```
static void PrivateReportProgress(Debugger , uint64_t progress_id,
  std::string title, std::string details,
  uint64_t completed, uint64_t total,
  bool is_debugger_specific) {
  // Only deliver progress events if we have any progress listeners.
  if 
(debugger.GetBroadcaster().EventTypeHasListeners(Debugger::eBroadcastBitProgressByCategory))
PrivateReportProgressByCategory(debugger, progress_id, 
is_debugger_specific, completed, total, is_debugger_specific);
  const uint32_t event_type = Debugger::eBroadcastBitProgress;
  if (!debugger.GetBroadcaster().EventTypeHasListeners(event_type))
return;
  EventSP event_sp(new Event(
  event_type,
  new ProgressEventData(progress_id, std::move(title), std::move(details),
completed, total, is_debugger_specific)));
  debugger.GetBroadcaster().BroadcastEvent(event_sp);
}
```
Then we write a new `PrivateReportProgressByCategory()` function that will do 
all this book keeping and coordingate with the debugger which will own the 
category ref count map

https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)

[Chelsea Cassanova via lldb-commits]

@@ -99,6 +105,10 @@ class Progress {
 private:
   void ReportProgress();
   static std::atomic g_id;
+  static std::atomic g_refcount;
+  /// Map that tracks each progress object and if we've seen its start and stop
+  /// events
+  static std::unordered_map g_map;

chelcassanova wrote:

We spoke about this a little offline but at the moment although Progress 
reports hold on to debugger instances, these instances are null by default and 
most Progress reports don't give a debugger instance in their instantiation so 
we wouldn't be able to have a debugger instance hold on to their own map of 
ongoing progress reports.

@clayborg Having a progress report holding on to a debugger instance is a good 
idea, but if we want to use a map to keep track of ongoing reports we need to 
know what will own the map? Do we want the map to be owned by a debugger 
instance or could the progress class hold on to its own map of ongoing reports? 
In the case of the former we would then want to enforce that progress reports 
have valid debugger instances and in the latter we could remove the `debugger` 
field from the reports altogether.

https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Don't search for separate debug files for mach-o object files (PR #81041)

[via lldb-commits]

https://github.com/jimingham closed 
https://github.com/llvm/llvm-project/pull/81041
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 50ffc53 - Don't search for separate debug files for mach-o object files (#81041)

[via lldb-commits]

Author: jimingham
Date: 2024-02-07T14:25:51-08:00
New Revision: 50ffc53e4708f3484939ef82e7b0309600a8e19f

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

LOG: Don't search for separate debug files for mach-o object files (#81041)

mach-o object files never have separate debug info, and in a big app
there can be quite a large number of object files, so even a few stats
per object file can slow launches considerably.
This patch avoids this search for Mach-o symbol files of object type.

I don't have a way to test this, the only effect is that you didn't do a
bunch of stats that weren't going to do any good anyway.

Added: 


Modified: 
lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp

Removed: 




diff  --git a/lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp 
b/lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
index 47fe0020ce18de..f46bff8f7d12e3 100644
--- a/lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
+++ b/lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
@@ -118,7 +118,13 @@ SymbolVendorMacOSX::CreateInstance(const lldb::ModuleSP 
_sp,
 FileSpec dsym_fspec(module_sp->GetSymbolFileFileSpec());
 
 ObjectFileSP dsym_objfile_sp;
-if (!dsym_fspec) {
+// On Darwin, we store the debug information either in object files,
+// using the debug map to tie them to the executable, or in a dSYM.  We
+// pass through this routine both for binaries and for .o files, but in the
+// latter case there will never be an external debug file.  So we shouldn't
+// do all the stats needed to find it.
+if (!dsym_fspec && module_sp->GetObjectFile()->CalculateType() !=
+ObjectFile::eTypeObjectFile) {
   // No symbol file was specified in the module, lets try and find one
   // ourselves.
   FileSpec file_spec = obj_file->GetFileSpec();



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

[Lldb-commits] [lldb] Don't search for separate debug files for mach-o object files (PR #81041)

[via lldb-commits]

https://github.com/jimingham updated 
https://github.com/llvm/llvm-project/pull/81041

>From 787d48cd3e2af5478f05986268e91d1bfb5a4c30 Mon Sep 17 00:00:00 2001
From: Jim Ingham 
Date: Wed, 7 Feb 2024 13:00:55 -0800
Subject: [PATCH 1/2] Don't search for separate debug files for mach-o object
 files

mach-o object files never have separate debug info, and in a big app
there can be quite a large number of object files, so even a few stats
per object file can slow launches considerably.
This patch avoids this search for Mach-o symbol files of object type.

I don't have a way to test this, the only effect is that you didn't do
a bunch of stats that weren't going to do any good anyway.
---
 .../Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp| 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp 
b/lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
index 47fe0020ce18de..4080a31224a417 100644
--- a/lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
+++ b/lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
@@ -118,7 +118,13 @@ SymbolVendorMacOSX::CreateInstance(const lldb::ModuleSP 
_sp,
 FileSpec dsym_fspec(module_sp->GetSymbolFileFileSpec());
 
 ObjectFileSP dsym_objfile_sp;
-if (!dsym_fspec) {
+// On Darwin, we store the debug information either in object files,
+// using the debug map to tie them to the executable, or in a dSYM.  We
+// pass through this routine both for binaries and for .o files, but in the
+// latter case there will never be an external debug file.  So we shouldn't
+// do all the stats needed to find it.
+if (!dsym_fspec && module_sp->GetObjectFile()->CalculateType() 
+!= ObjectFile::eTypeObjectFile) {
   // No symbol file was specified in the module, lets try and find one
   // ourselves.
   FileSpec file_spec = obj_file->GetFileSpec();

>From b2e7c7e8b342e13b457dcf7da40040f2373c7a3d Mon Sep 17 00:00:00 2001
From: Jim Ingham 
Date: Wed, 7 Feb 2024 14:16:47 -0800
Subject: [PATCH 2/2] Format futzing

---
 .../source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp 
b/lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
index 4080a31224a417..f46bff8f7d12e3 100644
--- a/lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
+++ b/lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
@@ -123,8 +123,8 @@ SymbolVendorMacOSX::CreateInstance(const lldb::ModuleSP 
_sp,
 // pass through this routine both for binaries and for .o files, but in the
 // latter case there will never be an external debug file.  So we shouldn't
 // do all the stats needed to find it.
-if (!dsym_fspec && module_sp->GetObjectFile()->CalculateType() 
-!= ObjectFile::eTypeObjectFile) {
+if (!dsym_fspec && module_sp->GetObjectFile()->CalculateType() !=
+ObjectFile::eTypeObjectFile) {
   // No symbol file was specified in the module, lets try and find one
   // ourselves.
   FileSpec file_spec = obj_file->GetFileSpec();

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

[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)

[Chelsea Cassanova via lldb-commits]

@@ -9,26 +9,34 @@
 #include "lldb/Core/Progress.h"
 
 #include "lldb/Core/Debugger.h"
-#include "lldb/Utility/StreamString.h"
 
 #include 
 
 using namespace lldb;
 using namespace lldb_private;
 
 std::atomic Progress::g_id(0);
+std::atomic Progress::g_refcount(1);
+std::unordered_map Progress::g_map = {};
 
 Progress::Progress(std::string title, std::string details,
std::optional total,
-   lldb_private::Debugger *debugger)
+   lldb_private::Debugger *debugger, bool type)
 : m_title(title), m_details(details), m_id(++g_id), m_completed(0),
-  m_total(Progress::kNonDeterministicTotal) {
+  m_total(Progress::kNonDeterministicTotal), m_type(type) {
   if (total)
 m_total = *total;
 
   if (debugger)
 m_debugger_id = debugger->GetID();
   std::lock_guard guard(m_mutex);
+
+  if (m_type == Progress::eProgressCoalecseReports) {
+g_map.emplace(title, g_refcount);

chelcassanova wrote:

Using a global refcount shouldn't be necessary here, we can just initialize the 
value to 1 and increment/decrement as needed.

https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)

[Chelsea Cassanova via lldb-commits]

@@ -117,6 +127,7 @@ class Progress {
   /// to ensure that we don't send progress updates after progress has
   /// completed.
   bool m_complete = false;
+  bool m_type;

chelcassanova wrote:

Holdover from when I used a bool for this value before switching an enum when I 
put the patch up.

https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)

[Med Ismail Bennani via lldb-commits]

https://github.com/medismailben edited 
https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)

[Med Ismail Bennani via lldb-commits]

https://github.com/medismailben edited 
https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)

[Med Ismail Bennani via lldb-commits]

https://github.com/medismailben edited 
https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)

[Med Ismail Bennani via lldb-commits]

@@ -9,26 +9,34 @@
 #include "lldb/Core/Progress.h"
 
 #include "lldb/Core/Debugger.h"
-#include "lldb/Utility/StreamString.h"
 
 #include 
 
 using namespace lldb;
 using namespace lldb_private;
 
 std::atomic Progress::g_id(0);
+std::atomic Progress::g_refcount(1);
+std::unordered_map Progress::g_map = {};
 
 Progress::Progress(std::string title, std::string details,
std::optional total,
-   lldb_private::Debugger *debugger)
+   lldb_private::Debugger *debugger, bool type)
 : m_title(title), m_details(details), m_id(++g_id), m_completed(0),
-  m_total(Progress::kNonDeterministicTotal) {
+  m_total(Progress::kNonDeterministicTotal), m_type(type) {
   if (total)
 m_total = *total;
 
   if (debugger)
 m_debugger_id = debugger->GetID();
   std::lock_guard guard(m_mutex);
+
+  if (m_type == Progress::eProgressCoalecseReports) {
+g_map.emplace(title, g_refcount);
+
+if (g_map.at(title) >= 1)

medismailben wrote:

This needs to be strictly greater than 1, otherwise you'll increment the 
refcount twice in the first progress report instantiation.

https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)

[Med Ismail Bennani via lldb-commits]

@@ -99,6 +105,10 @@ class Progress {
 private:
   void ReportProgress();
   static std::atomic g_id;
+  static std::atomic g_refcount;

medismailben wrote:

By making the refcount static, it's shared across all the `Progress` instances, 
but IIUC, you want this refcount to be different for each progress category, 
right ? I actually don't even think the `Progress` object needs to know about 
its refcount, since only the code that will handle the progress event (in 
`Debugger::HandleProgressEvent` would need it).

https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)

[Med Ismail Bennani via lldb-commits]

https://github.com/medismailben edited 
https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)

[Med Ismail Bennani via lldb-commits]

https://github.com/medismailben requested changes to this pull request.


https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)

[Greg Clayton via lldb-commits]

https://github.com/clayborg edited 
https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)

[Greg Clayton via lldb-commits]

@@ -9,26 +9,34 @@
 #include "lldb/Core/Progress.h"
 
 #include "lldb/Core/Debugger.h"
-#include "lldb/Utility/StreamString.h"
 
 #include 
 
 using namespace lldb;
 using namespace lldb_private;
 
 std::atomic Progress::g_id(0);
+std::atomic Progress::g_refcount(1);
+std::unordered_map Progress::g_map = {};

clayborg wrote:

When something isn't just an integer, like the map here, we can run into issues 
with this in a multi-threaded environment. Why? These variables are part of the 
global C++ constructor / destructor chain. If any thread uses these after the 
main thread shuts down it can cause a crash. So for global objects we typically 
will create a pointer to one and leak it with a note. See 
`lldb/source/Core/Debugger.cpp` and look for `g_debugger_list_mutex_ptr` and 
other variables right next to them and then see `Debugger::Initialize()`. 

This map also need to be threadsafe, so it needs a mutex. Might be a good idea 
to create a class to contain these and use an accessor to provide access to it. 
The solution below will make something that is thread safe _and_ also something 
that will survive the global destructor chain being called and letting other 
threads still be able to use Progress objects without crashing the process:
```
namespace {
class ProgressCategoryMap {
  std::mutex m_mutex;
  std::unordered_map m_map;

  void Increment(std::string title) {
std::lock_guard lock(m_mutex);
auto pair = g_map.emplace(title, 1); // No need to use g_refcount as it is 
just a constant 1
// pair.first will be true if insertion happened.
if (pair.first == false)
  ++pair.second; // Key already in map, increment the ref count.
  }
  void Decrement(std::string title) {
std::lock_guard lock(m_mutex);
auto pos = g_map.find(title);
if (pos == g_map.end())
  return;
if (--pos->second == 0)
  g_map.erase(pos->second); // Decremented to zero
  }
};
// Now make a global accessor to get a threadsafe copy of ProgressCategoryMap
ProgressCategoryMap *GetProgressCategoryMap() {
  static std::once_flag g_once;
  static ProgressCategoryMap *g_category_map_ptr = nullptr;
  std::call_once(g_once, []{
// NOTE: intentional leak to avoid issues with C++ destructor chain
g_category_map_ptr = new ProgressCategoryMap();
  });
  return g_category_map_ptr;
}
} // anonymous namespace
```

https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)

[Greg Clayton via lldb-commits]

@@ -9,26 +9,34 @@
 #include "lldb/Core/Progress.h"
 
 #include "lldb/Core/Debugger.h"
-#include "lldb/Utility/StreamString.h"
 
 #include 
 
 using namespace lldb;
 using namespace lldb_private;
 
 std::atomic Progress::g_id(0);
+std::atomic Progress::g_refcount(1);

clayborg wrote:

This isn't needed. This value doesn't change and is always 1, so you can either 
make it a constexpr in the class description, or it isn't really needed at all 
actually.

https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)

[Greg Clayton via lldb-commits]

@@ -55,6 +56,10 @@ namespace lldb_private {
 
 class Progress {
 public:
+  enum {
+eProgressLinearReports,
+eProgressCoalecseReports,
+  };

clayborg wrote:

Should we add a new setting for this? Something like:
```
(lldb) settings set progress-report [individual|grouped]
```

https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)

[Greg Clayton via lldb-commits]

@@ -38,6 +46,13 @@ Progress::~Progress() {
   std::lock_guard guard(m_mutex);
   if (!m_completed)
 m_completed = m_total;
+
+  if (m_type == Progress::eProgressCoalecseReports) {
+--g_map.at(m_title);
+if (g_map.at(m_title) == 0)
+  g_map.erase(m_title);

clayborg wrote:

Use the threadsafe `ProgressCategoryMap` here:
```
GetProgressCategoryMap()->Decrement(title);
```

https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)

[Greg Clayton via lldb-commits]

https://github.com/clayborg commented:

So I added comments on fixes to the current implementation in inline comments.

A few questions I have:
- do we really want each progress to select if it should be coalesced as a 
`Progress` constructor arguments? Or do we want a global setting on how 
progress events should be delivered?
- The current code adds a way to increment and decrement refcounts of ongoing 
progress categories (titles), but doesn't do anything with them, what do we 
envision happening with these ref counts?

But overall I would think we would want to keep track of this at a higher layer 
than in this class. Like in the code that would be delivering the progress 
events. If we do it that way, then we could have a global setting that controls 
if users want events delivered by category or individually. We actually won't 
need a setting for this as we can just see who is listening to the process 
events. Anyone listening to `SBDebugger::eBroadcastBitProgress` would get 
progress events delivered on a one by one basis, and anyone listening to a new 
progress bit like `SBDebugger::eBroadcastBitProgressByCategory` would just get 
different events delivered and all of this ref counting by title code from here 
would be moved into Debugger.cpp where the other progress events are being 
handled already.

https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)

[Greg Clayton via lldb-commits]

@@ -9,26 +9,34 @@
 #include "lldb/Core/Progress.h"
 
 #include "lldb/Core/Debugger.h"
-#include "lldb/Utility/StreamString.h"
 
 #include 
 
 using namespace lldb;
 using namespace lldb_private;
 
 std::atomic Progress::g_id(0);
+std::atomic Progress::g_refcount(1);
+std::unordered_map Progress::g_map = {};
 
 Progress::Progress(std::string title, std::string details,
std::optional total,
-   lldb_private::Debugger *debugger)
+   lldb_private::Debugger *debugger, bool type)
 : m_title(title), m_details(details), m_id(++g_id), m_completed(0),
-  m_total(Progress::kNonDeterministicTotal) {
+  m_total(Progress::kNonDeterministicTotal), m_type(type) {
   if (total)
 m_total = *total;
 
   if (debugger)
 m_debugger_id = debugger->GetID();
   std::lock_guard guard(m_mutex);
+
+  if (m_type == Progress::eProgressCoalecseReports) {
+g_map.emplace(title, g_refcount);
+
+if (g_map.at(title) >= 1)
+  ++g_map.at(title);

clayborg wrote:

Use the threadsafe `ProgressCategoryMap` here:
```
GetProgressCategoryMap()->Increment(title);
```

https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Don't search for separate debug files for mach-o object files (PR #81041)

[Jason Molenda via lldb-commits]

https://github.com/jasonmolenda approved this pull request.

LGTM.

https://github.com/llvm/llvm-project/pull/81041
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Don't search for separate debug files for mach-o object files (PR #81041)

[via lldb-commits]

github-actions[bot] wrote:




:warning: C/C++ code formatter, clang-format found issues in your code. 
:warning:



You can test this locally with the following command:


``bash
git-clang-format --diff 369b82218419a0218400e7483255523b8dfd6cf0 
787d48cd3e2af5478f05986268e91d1bfb5a4c30 -- 
lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
``





View the diff from clang-format here.


``diff
diff --git a/lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp 
b/lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
index 4080a31224..70228f0643 100644
--- a/lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
+++ b/lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
@@ -123,8 +123,8 @@ SymbolVendorMacOSX::CreateInstance(const lldb::ModuleSP 
_sp,
 // pass through this routine both for binaries and for .o files, but in the
 // latter case there will never be an external debug file.  So we shouldn't
 // do all the stats needed to find it.
-if (!dsym_fspec && module_sp->GetObjectFile()->CalculateType() 
-!= ObjectFile::eTypeObjectFile) {
+if (!dsym_fspec && module_sp->GetObjectFile()->CalculateType() !=
+   ObjectFile::eTypeObjectFile) {
   // No symbol file was specified in the module, lets try and find one
   // ourselves.
   FileSpec file_spec = obj_file->GetFileSpec();

``




https://github.com/llvm/llvm-project/pull/81041
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Don't search for separate debug files for mach-o object files (PR #81041)

[via lldb-commits]

llvmbot wrote:




@llvm/pr-subscribers-lldb

Author: None (jimingham)


Changes

mach-o object files never have separate debug info, and in a big app there can 
be quite a large number of object files, so even a few stats per object file 
can slow launches considerably.
This patch avoids this search for Mach-o symbol files of object type.

I don't have a way to test this, the only effect is that you didn't do a bunch 
of stats that weren't going to do any good anyway.

---
Full diff: https://github.com/llvm/llvm-project/pull/81041.diff


1 Files Affected:

- (modified) lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp 
(+7-1) 


``diff
diff --git a/lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp 
b/lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
index 47fe0020ce18d..4080a31224a41 100644
--- a/lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
+++ b/lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
@@ -118,7 +118,13 @@ SymbolVendorMacOSX::CreateInstance(const lldb::ModuleSP 
_sp,
 FileSpec dsym_fspec(module_sp->GetSymbolFileFileSpec());
 
 ObjectFileSP dsym_objfile_sp;
-if (!dsym_fspec) {
+// On Darwin, we store the debug information either in object files,
+// using the debug map to tie them to the executable, or in a dSYM.  We
+// pass through this routine both for binaries and for .o files, but in the
+// latter case there will never be an external debug file.  So we shouldn't
+// do all the stats needed to find it.
+if (!dsym_fspec && module_sp->GetObjectFile()->CalculateType() 
+!= ObjectFile::eTypeObjectFile) {
   // No symbol file was specified in the module, lets try and find one
   // ourselves.
   FileSpec file_spec = obj_file->GetFileSpec();

``




https://github.com/llvm/llvm-project/pull/81041
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Don't search for separate debug files for mach-o object files (PR #81041)

[via lldb-commits]

https://github.com/jimingham created 
https://github.com/llvm/llvm-project/pull/81041

mach-o object files never have separate debug info, and in a big app there can 
be quite a large number of object files, so even a few stats per object file 
can slow launches considerably.
This patch avoids this search for Mach-o symbol files of object type.

I don't have a way to test this, the only effect is that you didn't do a bunch 
of stats that weren't going to do any good anyway.

>From 787d48cd3e2af5478f05986268e91d1bfb5a4c30 Mon Sep 17 00:00:00 2001
From: Jim Ingham 
Date: Wed, 7 Feb 2024 13:00:55 -0800
Subject: [PATCH] Don't search for separate debug files for mach-o object files

mach-o object files never have separate debug info, and in a big app
there can be quite a large number of object files, so even a few stats
per object file can slow launches considerably.
This patch avoids this search for Mach-o symbol files of object type.

I don't have a way to test this, the only effect is that you didn't do
a bunch of stats that weren't going to do any good anyway.
---
 .../Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp| 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp 
b/lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
index 47fe0020ce18d..4080a31224a41 100644
--- a/lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
+++ b/lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
@@ -118,7 +118,13 @@ SymbolVendorMacOSX::CreateInstance(const lldb::ModuleSP 
_sp,
 FileSpec dsym_fspec(module_sp->GetSymbolFileFileSpec());
 
 ObjectFileSP dsym_objfile_sp;
-if (!dsym_fspec) {
+// On Darwin, we store the debug information either in object files,
+// using the debug map to tie them to the executable, or in a dSYM.  We
+// pass through this routine both for binaries and for .o files, but in the
+// latter case there will never be an external debug file.  So we shouldn't
+// do all the stats needed to find it.
+if (!dsym_fspec && module_sp->GetObjectFile()->CalculateType() 
+!= ObjectFile::eTypeObjectFile) {
   // No symbol file was specified in the module, lets try and find one
   // ourselves.
   FileSpec file_spec = obj_file->GetFileSpec();

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

[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)

[Med Ismail Bennani via lldb-commits]

@@ -55,6 +56,10 @@ namespace lldb_private {
 
 class Progress {
 public:
+  enum {
+eProgressLinearReports,
+eProgressCoalecseReports,

medismailben wrote:

```suggestion
eProgressCoalesceReports,
```

https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)

[Med Ismail Bennani via lldb-commits]

@@ -9,26 +9,34 @@
 #include "lldb/Core/Progress.h"
 
 #include "lldb/Core/Debugger.h"
-#include "lldb/Utility/StreamString.h"
 
 #include 
 
 using namespace lldb;
 using namespace lldb_private;
 
 std::atomic Progress::g_id(0);
+std::atomic Progress::g_refcount(1);
+std::unordered_map Progress::g_map = {};

medismailben wrote:

When you move it to the debugger instance, you don't need this anymore.

https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)

[Med Ismail Bennani via lldb-commits]

@@ -99,6 +105,10 @@ class Progress {
 private:
   void ReportProgress();
   static std::atomic g_id;
+  static std::atomic g_refcount;
+  /// Map that tracks each progress object and if we've seen its start and stop
+  /// events
+  static std::unordered_map g_map;

medismailben wrote:

I'd move this to be tied to the debugger instance instead of making it static 
here.

https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)

[Med Ismail Bennani via lldb-commits]

@@ -117,6 +127,7 @@ class Progress {
   /// to ensure that we don't send progress updates after progress has
   /// completed.
   bool m_complete = false;
+  bool m_type;

medismailben wrote:

This is confusing, why would you store your enum value in a `bool`

https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)

[Med Ismail Bennani via lldb-commits]

https://github.com/medismailben requested changes to this pull request.

I think the progress map should be held by the debugger instance, not a static 
member of the Progress class, since some progress reports could be targeting a 
specific debugger.

https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)

[Med Ismail Bennani via lldb-commits]

@@ -55,6 +56,10 @@ namespace lldb_private {
 
 class Progress {
 public:
+  enum {
+eProgressLinearReports,

medismailben wrote:

Please comment what each enum value means

https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)

[Med Ismail Bennani via lldb-commits]

https://github.com/medismailben edited 
https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Don't count all the frames just to skip the current inlined ones. (PR #80918)

[via lldb-commits]

jimingham wrote:

> Should the Doxygen comment of GetStackFrameCount warn that this is an 
> expensive operation? 
> https://lldb.llvm.org/cpp_reference/classlldb__private_1_1Thread.html#afc54feef950a58b625bbb198dc4cf57c

I added something to that effect. 

https://github.com/llvm/llvm-project/pull/80918
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Don't count all the frames just to skip the current inlined ones. (PR #80918)

[via lldb-commits]

https://github.com/jimingham updated 
https://github.com/llvm/llvm-project/pull/80918

>From c76edeec5c7430cd352c4d0ca977445800c55666 Mon Sep 17 00:00:00 2001
From: Jim Ingham 
Date: Tue, 6 Feb 2024 17:30:17 -0800
Subject: [PATCH 1/2] Don't count all the frames just to skip the current
 inlined ones.

The algorithm to find the DW_OP_entry_value requires you to find
the nearest non-inlined frame.  It did that by counting the number
of stack frames so that it could use that as a loop stopper.

That is unnecessary and inefficient.  Unnecessary because
GetFrameAtIndex will return a null frame when you step past the
oldest frame, so you already have the "got to the end" signal
without counting all the stack frames.
And counting all the stack frames can be expensive.
---
 lldb/source/Expression/DWARFExpression.cpp | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/lldb/source/Expression/DWARFExpression.cpp 
b/lldb/source/Expression/DWARFExpression.cpp
index fe4928d4f43a43..c061fd1140fff7 100644
--- a/lldb/source/Expression/DWARFExpression.cpp
+++ b/lldb/source/Expression/DWARFExpression.cpp
@@ -608,11 +608,10 @@ static bool Evaluate_DW_OP_entry_value(std::vector 
,
   StackFrameSP parent_frame = nullptr;
   addr_t return_pc = LLDB_INVALID_ADDRESS;
   uint32_t current_frame_idx = current_frame->GetFrameIndex();
-  uint32_t num_frames = thread->GetStackFrameCount();
-  for (uint32_t parent_frame_idx = current_frame_idx + 1;
-   parent_frame_idx < num_frames; ++parent_frame_idx) {
+
+  for (uint32_t parent_frame_idx = current_frame_idx + 1;;parent_frame_idx++) {
 parent_frame = thread->GetStackFrameAtIndex(parent_frame_idx);
-// Require a valid sequence of frames.
+// If this is null, we're at the end of the stack.
 if (!parent_frame)
   break;
 

>From e8659a128f34b93469e9ad9b0ed013ff6764c5be Mon Sep 17 00:00:00 2001
From: Jim Ingham 
Date: Wed, 7 Feb 2024 11:31:09 -0800
Subject: [PATCH 2/2] Add a comment about Thread::GetStackFrameCount being
 expensive.

---
 lldb/include/lldb/Target/Thread.h | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/lldb/include/lldb/Target/Thread.h 
b/lldb/include/lldb/Target/Thread.h
index e423dd4a6d2baa..30863ad4c90299 100644
--- a/lldb/include/lldb/Target/Thread.h
+++ b/lldb/include/lldb/Target/Thread.h
@@ -390,6 +390,13 @@ class Thread : public std::enable_shared_from_this,
   /// and having the thread call the SystemRuntime again.
   virtual bool ThreadHasQueueInformation() const { return false; }
 
+  /// GetStackFrameCount can be expensive.  Stacks can get very deep, and they
+  /// require memory reads for each frame.  So only use GetStackFrameCount 
when 
+  /// you need to know the depth of the stack.  When iterating over frames, its
+  /// better to generate the frames one by one with GetFrameAtIndex, and when
+  /// that returns NULL, you are at the end of the stack.  That way your loop
+  /// will only do the work it needs to, without forcing lldb to realize
+  /// StackFrames you weren't going to look at.
   virtual uint32_t GetStackFrameCount() {
 return GetStackFrameList()->GetNumFrames();
   }

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

[Lldb-commits] [lldb] Don't count all the frames just to skip the current inlined ones. (PR #80918)

[via lldb-commits]

@@ -608,11 +608,10 @@ static bool Evaluate_DW_OP_entry_value(std::vector 
,
   StackFrameSP parent_frame = nullptr;
   addr_t return_pc = LLDB_INVALID_ADDRESS;
   uint32_t current_frame_idx = current_frame->GetFrameIndex();
-  uint32_t num_frames = thread->GetStackFrameCount();
-  for (uint32_t parent_frame_idx = current_frame_idx + 1;
-   parent_frame_idx < num_frames; ++parent_frame_idx) {
+
+  for (uint32_t parent_frame_idx = current_frame_idx + 1;;parent_frame_idx++) {

jimingham wrote:

I tried this, but I don't think it makes things any clearer.   This isn't a 
simple loop, it has another break, and a continue.  You end up having to cache 
IsInline before you reset the parent_frame and that hides where the important 
next frame part of the code goes.  That's just awkward.  
I think this version is clearer.  The first thing the loop does is fetch the 
next frame and, checks if it is null as the signal that the stack walk is done. 
 The way I wrote it keeps those operations right next to one another which I 
think is easier to read..

https://github.com/llvm/llvm-project/pull/80918
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)

[Chelsea Cassanova via lldb-commits]

https://github.com/chelcassanova updated 
https://github.com/llvm/llvm-project/pull/81026

>From a80637fe2471c3f1adc2b353cef41887bcd55a3c Mon Sep 17 00:00:00 2001
From: Chelsea Cassanova 
Date: Tue, 6 Feb 2024 10:48:39 -0800
Subject: [PATCH] [lldb][progress][NFC] Add groundwork to keep track of
 progress reports

As part of the effort to improve progress reporting in
LLDB (https://discourse.llvm.org/t/rfc-improve-lldb-progress-reporting/75717)
we want a way to keep track of progress reports to see if they're
ongoing. The ultimate goal is to use this information to essentially
keep these reports alive using timeouts to more gracefully deliver these
reports.

To lay the groundwork to do this, this commit adds a map to progress
reports that keeps track of each category of reports (using the changes
from https://github.com/llvm/llvm-project/pull/77547) and uses a
refcount to check if these reports are still considered active or not. A
refcount of at least one indicates that the report is still active.

I've added an enum to `Progress.h` to toggle whether this behaviour is
used or not. By default it is not used, so this commit is marked as NFC.
---
 lldb/include/lldb/Core/Progress.h | 13 -
 lldb/source/Core/Progress.cpp | 21 ++---
 2 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/lldb/include/lldb/Core/Progress.h 
b/lldb/include/lldb/Core/Progress.h
index 5d882910246054..20a42a101e0e19 100644
--- a/lldb/include/lldb/Core/Progress.h
+++ b/lldb/include/lldb/Core/Progress.h
@@ -12,6 +12,7 @@
 #include "lldb/Utility/ConstString.h"
 #include "lldb/lldb-types.h"
 #include 
+#include 
 #include 
 #include 
 
@@ -55,6 +56,10 @@ namespace lldb_private {
 
 class Progress {
 public:
+  enum {
+eProgressLinearReports,
+eProgressCoalecseReports,
+  };
   /// Construct a progress object that will report information.
   ///
   /// The constructor will create a unique progress reporting object and
@@ -71,7 +76,8 @@ class Progress {
   /// progress is to be reported only to specific debuggers.
   Progress(std::string title, std::string details = {},
std::optional total = std::nullopt,
-   lldb_private::Debugger *debugger = nullptr);
+   lldb_private::Debugger *debugger = nullptr,
+   bool type = Progress::eProgressLinearReports);
 
   /// Destroy the progress object.
   ///
@@ -99,6 +105,10 @@ class Progress {
 private:
   void ReportProgress();
   static std::atomic g_id;
+  static std::atomic g_refcount;
+  /// Map that tracks each progress object and if we've seen its start and stop
+  /// events
+  static std::unordered_map g_map;
   /// The title of the progress activity.
   std::string m_title;
   std::string m_details;
@@ -117,6 +127,7 @@ class Progress {
   /// to ensure that we don't send progress updates after progress has
   /// completed.
   bool m_complete = false;
+  bool m_type;
 };
 
 } // namespace lldb_private
diff --git a/lldb/source/Core/Progress.cpp b/lldb/source/Core/Progress.cpp
index 732efbc342b450..e79836033efe38 100644
--- a/lldb/source/Core/Progress.cpp
+++ b/lldb/source/Core/Progress.cpp
@@ -9,7 +9,6 @@
 #include "lldb/Core/Progress.h"
 
 #include "lldb/Core/Debugger.h"
-#include "lldb/Utility/StreamString.h"
 
 #include 
 
@@ -17,18 +16,27 @@ using namespace lldb;
 using namespace lldb_private;
 
 std::atomic Progress::g_id(0);
+std::atomic Progress::g_refcount(1);
+std::unordered_map Progress::g_map = {};
 
 Progress::Progress(std::string title, std::string details,
std::optional total,
-   lldb_private::Debugger *debugger)
+   lldb_private::Debugger *debugger, bool type)
 : m_title(title), m_details(details), m_id(++g_id), m_completed(0),
-  m_total(Progress::kNonDeterministicTotal) {
+  m_total(Progress::kNonDeterministicTotal), m_type(type) {
   if (total)
 m_total = *total;
 
   if (debugger)
 m_debugger_id = debugger->GetID();
   std::lock_guard guard(m_mutex);
+
+  if (m_type == Progress::eProgressCoalecseReports) {
+g_map.emplace(title, g_refcount);
+
+if (g_map.at(title) >= 1)
+  ++g_map.at(title);
+  }
   ReportProgress();
 }
 
@@ -38,6 +46,13 @@ Progress::~Progress() {
   std::lock_guard guard(m_mutex);
   if (!m_completed)
 m_completed = m_total;
+
+  if (m_type == Progress::eProgressCoalecseReports) {
+--g_map.at(m_title);
+if (g_map.at(m_title) == 0)
+  g_map.erase(m_title);
+  }
+
   ReportProgress();
 }
 

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

[Lldb-commits] [lldb] [llvm] [lldb-dap][NFC] Add Breakpoint struct to share common logic. (PR #80753)

[Zequan Wu via lldb-commits]

https://github.com/ZequanWu updated 
https://github.com/llvm/llvm-project/pull/80753

>From c4b767909a9ffc2a3015dc9021e4c265da0d877d Mon Sep 17 00:00:00 2001
From: Zequan Wu 
Date: Mon, 5 Feb 2024 17:26:48 -0500
Subject: [PATCH 1/3] [lldb-dap][NFC] Add Breakpoint struct to share common
 logic.

---
 lldb/tools/lldb-dap/Breakpoint.cpp| 182 ++
 lldb/tools/lldb-dap/Breakpoint.h  |  34 
 lldb/tools/lldb-dap/BreakpointBase.cpp| 113 ---
 lldb/tools/lldb-dap/BreakpointBase.h  |  12 +-
 lldb/tools/lldb-dap/CMakeLists.txt|   1 +
 lldb/tools/lldb-dap/FunctionBreakpoint.cpp|  12 +-
 lldb/tools/lldb-dap/FunctionBreakpoint.h  |   4 +-
 lldb/tools/lldb-dap/JSONUtils.cpp |  46 +
 lldb/tools/lldb-dap/JSONUtils.h   |   5 +-
 lldb/tools/lldb-dap/SourceBreakpoint.cpp  |  12 +-
 lldb/tools/lldb-dap/SourceBreakpoint.h|   6 +-
 lldb/tools/lldb-dap/lldb-dap.cpp  |  17 +-
 .../gn/secondary/lldb/tools/lldb-dap/BUILD.gn |   1 +
 13 files changed, 248 insertions(+), 197 deletions(-)
 create mode 100644 lldb/tools/lldb-dap/Breakpoint.cpp
 create mode 100644 lldb/tools/lldb-dap/Breakpoint.h

diff --git a/lldb/tools/lldb-dap/Breakpoint.cpp 
b/lldb/tools/lldb-dap/Breakpoint.cpp
new file mode 100644
index 0..4ccf353b06ccc
--- /dev/null
+++ b/lldb/tools/lldb-dap/Breakpoint.cpp
@@ -0,0 +1,182 @@
+//===-- Breakpoint.cpp --*- 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
+//
+//===--===//
+
+#include "Breakpoint.h"
+#include "DAP.h"
+#include "JSONUtils.h"
+#include "llvm/ADT/StringExtras.h"
+
+using namespace lldb_dap;
+
+void Breakpoint::SetCondition() { bp.SetCondition(condition.c_str()); }
+
+void Breakpoint::SetHitCondition() {
+  uint64_t hitCount = 0;
+  if (llvm::to_integer(hitCondition, hitCount))
+bp.SetIgnoreCount(hitCount - 1);
+}
+
+// logMessage will be divided into array of LogMessagePart as two kinds:
+// 1. raw print text message, and
+// 2. interpolated expression for evaluation which is inside matching curly
+//braces.
+//
+// The function tries to parse logMessage into a list of LogMessageParts
+// for easy later access in BreakpointHitCallback.
+void Breakpoint::SetLogMessage() {
+  logMessageParts.clear();
+
+  // Contains unmatched open curly braces indices.
+  std::vector unmatched_curly_braces;
+
+  // Contains all matched curly braces in logMessage.
+  // Loop invariant: matched_curly_braces_ranges are sorted by start index in
+  // ascending order without any overlap between them.
+  std::vector> matched_curly_braces_ranges;
+
+  lldb::SBError error;
+  // Part1 - parse matched_curly_braces_ranges.
+  // locating all curly braced expression ranges in logMessage.
+  // The algorithm takes care of nested and imbalanced curly braces.
+  for (size_t i = 0; i < logMessage.size(); ++i) {
+if (logMessage[i] == '{') {
+  unmatched_curly_braces.push_back(i);
+} else if (logMessage[i] == '}') {
+  if (unmatched_curly_braces.empty())
+// Nothing to match.
+continue;
+
+  int last_unmatched_index = unmatched_curly_braces.back();
+  unmatched_curly_braces.pop_back();
+
+  // Erase any matched ranges included in the new match.
+  while (!matched_curly_braces_ranges.empty()) {
+assert(matched_curly_braces_ranges.back().first !=
+   last_unmatched_index &&
+   "How can a curley brace be matched twice?");
+if (matched_curly_braces_ranges.back().first < last_unmatched_index)
+  break;
+
+// This is a nested range let's earse it.
+assert((size_t)matched_curly_braces_ranges.back().second < i);
+matched_curly_braces_ranges.pop_back();
+  }
+
+  // Assert invariant.
+  assert(matched_curly_braces_ranges.empty() ||
+ matched_curly_braces_ranges.back().first < last_unmatched_index);
+  matched_curly_braces_ranges.emplace_back(last_unmatched_index, i);
+}
+  }
+
+  // Part2 - parse raw text and expresions parts.
+  // All expression ranges have been parsed in matched_curly_braces_ranges.
+  // The code below uses matched_curly_braces_ranges to divide logMessage
+  // into raw text parts and expression parts.
+  int last_raw_text_start = 0;
+  for (const std::pair _braces_range :
+   matched_curly_braces_ranges) {
+// Raw text before open curly brace.
+assert(curly_braces_range.first >= last_raw_text_start);
+size_t raw_text_len = curly_braces_range.first - last_raw_text_start;
+if (raw_text_len > 0) {
+  error = AppendLogMessagePart(
+  llvm::StringRef(logMessage.c_str() + last_raw_text_start,
+   

[Lldb-commits] [lldb] [lldb] Refactor GetFormatFromCString to always check for partial matches (NFC) (PR #81018)

[Dave Lee via lldb-commits]

@@ -2204,7 +2204,9 @@ static Status ParseInternal(llvm::StringRef , 
Entry _entry,
   return error;
 }
   } else if (FormatManager::GetFormatFromCString(
- entry.printf_format.c_str(), true, entry.fmt)) {
+ entry.printf_format.c_str(), true,
+ entry.fmt)) { // Try GetFormatFromCString again,

kastiglione wrote:

Pushed changes and updated the PR description.

https://github.com/llvm/llvm-project/pull/81018
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Refactor GetFormatFromCString to always check for partial matches (NFC) (PR #81018)

[Dave Lee via lldb-commits]

https://github.com/kastiglione edited 
https://github.com/llvm/llvm-project/pull/81018
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)

[via lldb-commits]

github-actions[bot] wrote:




:warning: C/C++ code formatter, clang-format found issues in your code. 
:warning:



You can test this locally with the following command:


``bash
git-clang-format --diff a8ab8306069e8e53b5148ceec7624d7d36ffb459 
d10c1c494972429eb5c42ccb8a1dfcbff5d7bfcd -- lldb/include/lldb/Core/Progress.h 
lldb/source/Core/Progress.cpp
``





View the diff from clang-format here.


``diff
diff --git a/lldb/include/lldb/Core/Progress.h 
b/lldb/include/lldb/Core/Progress.h
index fc921482ee..20a42a101e 100644
--- a/lldb/include/lldb/Core/Progress.h
+++ b/lldb/include/lldb/Core/Progress.h
@@ -76,7 +76,8 @@ public:
   /// progress is to be reported only to specific debuggers.
   Progress(std::string title, std::string details = {},
std::optional total = std::nullopt,
-   lldb_private::Debugger *debugger = nullptr, bool type = 
Progress::eProgressLinearReports);
+   lldb_private::Debugger *debugger = nullptr,
+   bool type = Progress::eProgressLinearReports);
 
   /// Destroy the progress object.
   ///
@@ -105,7 +106,8 @@ private:
   void ReportProgress();
   static std::atomic g_id;
   static std::atomic g_refcount;
-  /// Map that tracks each progress object and if we've seen its start and 
stop events
+  /// Map that tracks each progress object and if we've seen its start and stop
+  /// events
   static std::unordered_map g_map;
   /// The title of the progress activity.
   std::string m_title;

``




https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Refactor GetFormatFromCString to always check for partial matches (NFC) (PR #81018)

[Dave Lee via lldb-commits]

https://github.com/kastiglione edited 
https://github.com/llvm/llvm-project/pull/81018
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)

[via lldb-commits]

llvmbot wrote:




@llvm/pr-subscribers-lldb

Author: Chelsea Cassanova (chelcassanova)


Changes

As part of the effort to improve progress reporting in LLDB 
(https://discourse.llvm.org/t/rfc-improve-lldb-progress-reporting/75717) we 
want a way to keep track of progress reports to see if they're ongoing. The 
ultimate goal is to use this information to essentially keep these reports 
alive using timeouts to more gracefully deliver these reports.

To lay the groundwork to do this, this commit adds a map to progress reports 
that keeps track of each category of reports (using the changes from 
https://github.com/llvm/llvm-project/pull/77547) and uses a refcount to check 
if these reports are still considered active or not. A refcount of at least one 
indicates that the report is still active.

I've added an enum to `Progress.h` to toggle whether this behaviour is used or 
not. By default it is not used, so this commit is marked as NFC.

---
Full diff: https://github.com/llvm/llvm-project/pull/81026.diff


2 Files Affected:

- (modified) lldb/include/lldb/Core/Progress.h (+10-1) 
- (modified) lldb/source/Core/Progress.cpp (+18-3) 


``diff
diff --git a/lldb/include/lldb/Core/Progress.h 
b/lldb/include/lldb/Core/Progress.h
index 5d882910246054..fc921482ee12e0 100644
--- a/lldb/include/lldb/Core/Progress.h
+++ b/lldb/include/lldb/Core/Progress.h
@@ -12,6 +12,7 @@
 #include "lldb/Utility/ConstString.h"
 #include "lldb/lldb-types.h"
 #include 
+#include 
 #include 
 #include 
 
@@ -55,6 +56,10 @@ namespace lldb_private {
 
 class Progress {
 public:
+  enum {
+eProgressLinearReports,
+eProgressCoalecseReports,
+  };
   /// Construct a progress object that will report information.
   ///
   /// The constructor will create a unique progress reporting object and
@@ -71,7 +76,7 @@ class Progress {
   /// progress is to be reported only to specific debuggers.
   Progress(std::string title, std::string details = {},
std::optional total = std::nullopt,
-   lldb_private::Debugger *debugger = nullptr);
+   lldb_private::Debugger *debugger = nullptr, bool type = 
Progress::eProgressLinearReports);
 
   /// Destroy the progress object.
   ///
@@ -99,6 +104,9 @@ class Progress {
 private:
   void ReportProgress();
   static std::atomic g_id;
+  static std::atomic g_refcount;
+  /// Map that tracks each progress object and if we've seen its start and 
stop events
+  static std::unordered_map g_map;
   /// The title of the progress activity.
   std::string m_title;
   std::string m_details;
@@ -117,6 +125,7 @@ class Progress {
   /// to ensure that we don't send progress updates after progress has
   /// completed.
   bool m_complete = false;
+  bool m_type;
 };
 
 } // namespace lldb_private
diff --git a/lldb/source/Core/Progress.cpp b/lldb/source/Core/Progress.cpp
index 732efbc342b450..e79836033efe38 100644
--- a/lldb/source/Core/Progress.cpp
+++ b/lldb/source/Core/Progress.cpp
@@ -9,7 +9,6 @@
 #include "lldb/Core/Progress.h"
 
 #include "lldb/Core/Debugger.h"
-#include "lldb/Utility/StreamString.h"
 
 #include 
 
@@ -17,18 +16,27 @@ using namespace lldb;
 using namespace lldb_private;
 
 std::atomic Progress::g_id(0);
+std::atomic Progress::g_refcount(1);
+std::unordered_map Progress::g_map = {};
 
 Progress::Progress(std::string title, std::string details,
std::optional total,
-   lldb_private::Debugger *debugger)
+   lldb_private::Debugger *debugger, bool type)
 : m_title(title), m_details(details), m_id(++g_id), m_completed(0),
-  m_total(Progress::kNonDeterministicTotal) {
+  m_total(Progress::kNonDeterministicTotal), m_type(type) {
   if (total)
 m_total = *total;
 
   if (debugger)
 m_debugger_id = debugger->GetID();
   std::lock_guard guard(m_mutex);
+
+  if (m_type == Progress::eProgressCoalecseReports) {
+g_map.emplace(title, g_refcount);
+
+if (g_map.at(title) >= 1)
+  ++g_map.at(title);
+  }
   ReportProgress();
 }
 
@@ -38,6 +46,13 @@ Progress::~Progress() {
   std::lock_guard guard(m_mutex);
   if (!m_completed)
 m_completed = m_total;
+
+  if (m_type == Progress::eProgressCoalecseReports) {
+--g_map.at(m_title);
+if (g_map.at(m_title) == 0)
+  g_map.erase(m_title);
+  }
+
   ReportProgress();
 }
 

``




https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)

[Chelsea Cassanova via lldb-commits]

https://github.com/chelcassanova created 
https://github.com/llvm/llvm-project/pull/81026

As part of the effort to improve progress reporting in LLDB 
(https://discourse.llvm.org/t/rfc-improve-lldb-progress-reporting/75717) we 
want a way to keep track of progress reports to see if they're ongoing. The 
ultimate goal is to use this information to essentially keep these reports 
alive using timeouts to more gracefully deliver these reports.

To lay the groundwork to do this, this commit adds a map to progress reports 
that keeps track of each category of reports (using the changes from 
https://github.com/llvm/llvm-project/pull/77547) and uses a refcount to check 
if these reports are still considered active or not. A refcount of at least one 
indicates that the report is still active.

I've added an enum to `Progress.h` to toggle whether this behaviour is used or 
not. By default it is not used, so this commit is marked as NFC.

>From d10c1c494972429eb5c42ccb8a1dfcbff5d7bfcd Mon Sep 17 00:00:00 2001
From: Chelsea Cassanova 
Date: Tue, 6 Feb 2024 10:48:39 -0800
Subject: [PATCH] [lldb][progress][NFC] Add groundwork to keep track of
 progress reports

As part of the effort to improve progress reporting in
LLDB (https://discourse.llvm.org/t/rfc-improve-lldb-progress-reporting/75717)
we want a way to keep track of progress reports to see if they're
ongoing. The ultimate goal is to use this information to essentially
keep these reports alive using timeouts to more gracefully deliver these
reports.

To lay the groundwork to do this, this commit adds a map to progress
reports that keeps track of each category of reports (using the changes
from https://github.com/llvm/llvm-project/pull/77547) and uses a
refcount to check if these reports are still considered active or not. A
refcount of at least one indicates that the report is still active.

I've added an enum to `Progress.h` to toggle whether this behaviour is
used or not. By default it is not used, so this commit is marked as NFC.
---
 lldb/include/lldb/Core/Progress.h | 11 ++-
 lldb/source/Core/Progress.cpp | 21 ++---
 2 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/lldb/include/lldb/Core/Progress.h 
b/lldb/include/lldb/Core/Progress.h
index 5d88291024605..fc921482ee12e 100644
--- a/lldb/include/lldb/Core/Progress.h
+++ b/lldb/include/lldb/Core/Progress.h
@@ -12,6 +12,7 @@
 #include "lldb/Utility/ConstString.h"
 #include "lldb/lldb-types.h"
 #include 
+#include 
 #include 
 #include 
 
@@ -55,6 +56,10 @@ namespace lldb_private {
 
 class Progress {
 public:
+  enum {
+eProgressLinearReports,
+eProgressCoalecseReports,
+  };
   /// Construct a progress object that will report information.
   ///
   /// The constructor will create a unique progress reporting object and
@@ -71,7 +76,7 @@ class Progress {
   /// progress is to be reported only to specific debuggers.
   Progress(std::string title, std::string details = {},
std::optional total = std::nullopt,
-   lldb_private::Debugger *debugger = nullptr);
+   lldb_private::Debugger *debugger = nullptr, bool type = 
Progress::eProgressLinearReports);
 
   /// Destroy the progress object.
   ///
@@ -99,6 +104,9 @@ class Progress {
 private:
   void ReportProgress();
   static std::atomic g_id;
+  static std::atomic g_refcount;
+  /// Map that tracks each progress object and if we've seen its start and 
stop events
+  static std::unordered_map g_map;
   /// The title of the progress activity.
   std::string m_title;
   std::string m_details;
@@ -117,6 +125,7 @@ class Progress {
   /// to ensure that we don't send progress updates after progress has
   /// completed.
   bool m_complete = false;
+  bool m_type;
 };
 
 } // namespace lldb_private
diff --git a/lldb/source/Core/Progress.cpp b/lldb/source/Core/Progress.cpp
index 732efbc342b45..e79836033efe3 100644
--- a/lldb/source/Core/Progress.cpp
+++ b/lldb/source/Core/Progress.cpp
@@ -9,7 +9,6 @@
 #include "lldb/Core/Progress.h"
 
 #include "lldb/Core/Debugger.h"
-#include "lldb/Utility/StreamString.h"
 
 #include 
 
@@ -17,18 +16,27 @@ using namespace lldb;
 using namespace lldb_private;
 
 std::atomic Progress::g_id(0);
+std::atomic Progress::g_refcount(1);
+std::unordered_map Progress::g_map = {};
 
 Progress::Progress(std::string title, std::string details,
std::optional total,
-   lldb_private::Debugger *debugger)
+   lldb_private::Debugger *debugger, bool type)
 : m_title(title), m_details(details), m_id(++g_id), m_completed(0),
-  m_total(Progress::kNonDeterministicTotal) {
+  m_total(Progress::kNonDeterministicTotal), m_type(type) {
   if (total)
 m_total = *total;
 
   if (debugger)
 m_debugger_id = debugger->GetID();
   std::lock_guard guard(m_mutex);
+
+  if (m_type == Progress::eProgressCoalecseReports) {
+g_map.emplace(title, g_refcount);
+
+if (g_map.at(title) >= 1)
+  ++g_map.at(title);
+  }

[Lldb-commits] [lldb] [lldb] Add comment to ParseInternal in FormatEntity (NFC) (PR #81018)

[Dave Lee via lldb-commits]

https://github.com/kastiglione updated 
https://github.com/llvm/llvm-project/pull/81018

>From f9d4abddb0f93d27ed14278566796babad9f7f4e Mon Sep 17 00:00:00 2001
From: Dave Lee 
Date: Wed, 7 Feb 2024 10:17:51 -0800
Subject: [PATCH 1/3] [lldb] Add comment to ParseInternal in FormatEntity

---
 lldb/source/Core/FormatEntity.cpp | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lldb/source/Core/FormatEntity.cpp 
b/lldb/source/Core/FormatEntity.cpp
index 3c665c2eb2133b..b206b6e9983b85 100644
--- a/lldb/source/Core/FormatEntity.cpp
+++ b/lldb/source/Core/FormatEntity.cpp
@@ -2204,7 +2204,9 @@ static Status ParseInternal(llvm::StringRef , 
Entry _entry,
   return error;
 }
   } else if (FormatManager::GetFormatFromCString(
- entry.printf_format.c_str(), true, entry.fmt)) {
+ entry.printf_format.c_str(), true,
+ entry.fmt)) { // Try GetFormatFromCString again,
+   // with partial_match_ok = true.
 clear_printf = true;
   } else if (entry.printf_format == "tid") {
 verify_is_thread_id = true;

>From ab7b6e3e97dcfba7cdac84845d6f1a4778683955 Mon Sep 17 00:00:00 2001
From: Dave Lee 
Date: Wed, 7 Feb 2024 10:46:01 -0800
Subject: [PATCH 2/3] Refactor GetFormatFromCString to always handle partial
 matches

---
 .../include/lldb/DataFormatters/FormatManager.h |  2 +-
 lldb/source/Core/FormatEntity.cpp   | 10 ++
 lldb/source/DataFormatters/FormatManager.cpp| 17 +++--
 lldb/source/Interpreter/OptionArgParser.cpp |  3 +--
 4 files changed, 11 insertions(+), 21 deletions(-)

diff --git a/lldb/include/lldb/DataFormatters/FormatManager.h 
b/lldb/include/lldb/DataFormatters/FormatManager.h
index 986614f0c5e431..db2fe99c44cafc 100644
--- a/lldb/include/lldb/DataFormatters/FormatManager.h
+++ b/lldb/include/lldb/DataFormatters/FormatManager.h
@@ -138,7 +138,7 @@ class FormatManager : public IFormatChangeListener {
   }
 
   static bool GetFormatFromCString(const char *format_cstr,
-   bool partial_match_ok, lldb::Format 
);
+   lldb::Format );
 
   static char GetFormatAsFormatChar(lldb::Format format);
 
diff --git a/lldb/source/Core/FormatEntity.cpp 
b/lldb/source/Core/FormatEntity.cpp
index b206b6e9983b85..cdc3ea38fd2015 100644
--- a/lldb/source/Core/FormatEntity.cpp
+++ b/lldb/source/Core/FormatEntity.cpp
@@ -2151,11 +2151,7 @@ static Status ParseInternal(llvm::StringRef , 
Entry _entry,
 if (entry.printf_format.find('%') == std::string::npos) {
   bool clear_printf = false;
 
-  if (FormatManager::GetFormatFromCString(
-  entry.printf_format.c_str(), false, entry.fmt)) {
-// We have an LLDB format, so clear the printf format
-clear_printf = true;
-  } else if (entry.printf_format.size() == 1) {
+  if (entry.printf_format.size() == 1) {
 switch (entry.printf_format[0]) {
 case '@': // if this is an @ sign, print ObjC description
   entry.number = ValueObject::
@@ -2204,9 +2200,7 @@ static Status ParseInternal(llvm::StringRef , 
Entry _entry,
   return error;
 }
   } else if (FormatManager::GetFormatFromCString(
- entry.printf_format.c_str(), true,
- entry.fmt)) { // Try GetFormatFromCString again,
-   // with partial_match_ok = true.
+ entry.printf_format.c_str(), entry.fmt)) {
 clear_printf = true;
   } else if (entry.printf_format == "tid") {
 verify_is_thread_id = true;
diff --git a/lldb/source/DataFormatters/FormatManager.cpp 
b/lldb/source/DataFormatters/FormatManager.cpp
index f1f135de32ca87..092fa3c8ce496d 100644
--- a/lldb/source/DataFormatters/FormatManager.cpp
+++ b/lldb/source/DataFormatters/FormatManager.cpp
@@ -91,7 +91,7 @@ static bool GetFormatFromFormatChar(char format_char, Format 
) {
 }
 
 static bool GetFormatFromFormatName(llvm::StringRef format_name,
-bool partial_match_ok, Format ) {
+Format ) {
   uint32_t i;
   for (i = 0; i < g_num_format_infos; ++i) {
 if (format_name.equals_insensitive(g_format_infos[i].format_name)) {
@@ -100,13 +100,11 @@ static bool GetFormatFromFormatName(llvm::StringRef 
format_name,
 }
   }
 
-  if (partial_match_ok) {
-for (i = 0; i < g_num_format_infos; ++i) {
-  if (llvm::StringRef(g_format_infos[i].format_name)
-  .starts_with_insensitive(format_name)) {
-format = g_format_infos[i].format;
-return true;
-  }
+  for (i = 0; i < g_num_format_infos; ++i) {
+

[Lldb-commits] [lldb] [lldb] Add comment to ParseInternal in FormatEntity (NFC) (PR #81018)

[Dave Lee via lldb-commits]

https://github.com/kastiglione updated 
https://github.com/llvm/llvm-project/pull/81018

>From f9d4abddb0f93d27ed14278566796babad9f7f4e Mon Sep 17 00:00:00 2001
From: Dave Lee 
Date: Wed, 7 Feb 2024 10:17:51 -0800
Subject: [PATCH 1/2] [lldb] Add comment to ParseInternal in FormatEntity

---
 lldb/source/Core/FormatEntity.cpp | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lldb/source/Core/FormatEntity.cpp 
b/lldb/source/Core/FormatEntity.cpp
index 3c665c2eb2133..b206b6e9983b8 100644
--- a/lldb/source/Core/FormatEntity.cpp
+++ b/lldb/source/Core/FormatEntity.cpp
@@ -2204,7 +2204,9 @@ static Status ParseInternal(llvm::StringRef , 
Entry _entry,
   return error;
 }
   } else if (FormatManager::GetFormatFromCString(
- entry.printf_format.c_str(), true, entry.fmt)) {
+ entry.printf_format.c_str(), true,
+ entry.fmt)) { // Try GetFormatFromCString again,
+   // with partial_match_ok = true.
 clear_printf = true;
   } else if (entry.printf_format == "tid") {
 verify_is_thread_id = true;

>From ab7b6e3e97dcfba7cdac84845d6f1a4778683955 Mon Sep 17 00:00:00 2001
From: Dave Lee 
Date: Wed, 7 Feb 2024 10:46:01 -0800
Subject: [PATCH 2/2] Refactor GetFormatFromCString to always handle partial
 matches

---
 .../include/lldb/DataFormatters/FormatManager.h |  2 +-
 lldb/source/Core/FormatEntity.cpp   | 10 ++
 lldb/source/DataFormatters/FormatManager.cpp| 17 +++--
 lldb/source/Interpreter/OptionArgParser.cpp |  3 +--
 4 files changed, 11 insertions(+), 21 deletions(-)

diff --git a/lldb/include/lldb/DataFormatters/FormatManager.h 
b/lldb/include/lldb/DataFormatters/FormatManager.h
index 986614f0c5e43..db2fe99c44caf 100644
--- a/lldb/include/lldb/DataFormatters/FormatManager.h
+++ b/lldb/include/lldb/DataFormatters/FormatManager.h
@@ -138,7 +138,7 @@ class FormatManager : public IFormatChangeListener {
   }
 
   static bool GetFormatFromCString(const char *format_cstr,
-   bool partial_match_ok, lldb::Format 
);
+   lldb::Format );
 
   static char GetFormatAsFormatChar(lldb::Format format);
 
diff --git a/lldb/source/Core/FormatEntity.cpp 
b/lldb/source/Core/FormatEntity.cpp
index b206b6e9983b8..cdc3ea38fd201 100644
--- a/lldb/source/Core/FormatEntity.cpp
+++ b/lldb/source/Core/FormatEntity.cpp
@@ -2151,11 +2151,7 @@ static Status ParseInternal(llvm::StringRef , 
Entry _entry,
 if (entry.printf_format.find('%') == std::string::npos) {
   bool clear_printf = false;
 
-  if (FormatManager::GetFormatFromCString(
-  entry.printf_format.c_str(), false, entry.fmt)) {
-// We have an LLDB format, so clear the printf format
-clear_printf = true;
-  } else if (entry.printf_format.size() == 1) {
+  if (entry.printf_format.size() == 1) {
 switch (entry.printf_format[0]) {
 case '@': // if this is an @ sign, print ObjC description
   entry.number = ValueObject::
@@ -2204,9 +2200,7 @@ static Status ParseInternal(llvm::StringRef , 
Entry _entry,
   return error;
 }
   } else if (FormatManager::GetFormatFromCString(
- entry.printf_format.c_str(), true,
- entry.fmt)) { // Try GetFormatFromCString again,
-   // with partial_match_ok = true.
+ entry.printf_format.c_str(), entry.fmt)) {
 clear_printf = true;
   } else if (entry.printf_format == "tid") {
 verify_is_thread_id = true;
diff --git a/lldb/source/DataFormatters/FormatManager.cpp 
b/lldb/source/DataFormatters/FormatManager.cpp
index f1f135de32ca8..092fa3c8ce496 100644
--- a/lldb/source/DataFormatters/FormatManager.cpp
+++ b/lldb/source/DataFormatters/FormatManager.cpp
@@ -91,7 +91,7 @@ static bool GetFormatFromFormatChar(char format_char, Format 
) {
 }
 
 static bool GetFormatFromFormatName(llvm::StringRef format_name,
-bool partial_match_ok, Format ) {
+Format ) {
   uint32_t i;
   for (i = 0; i < g_num_format_infos; ++i) {
 if (format_name.equals_insensitive(g_format_infos[i].format_name)) {
@@ -100,13 +100,11 @@ static bool GetFormatFromFormatName(llvm::StringRef 
format_name,
 }
   }
 
-  if (partial_match_ok) {
-for (i = 0; i < g_num_format_infos; ++i) {
-  if (llvm::StringRef(g_format_infos[i].format_name)
-  .starts_with_insensitive(format_name)) {
-format = g_format_infos[i].format;
-return true;
-  }
+  for (i = 0; i < g_num_format_infos; ++i) {
+if 

[Lldb-commits] [lldb] [lldb] Add comment to ParseInternal in FormatEntity (NFC) (PR #81018)

[Dave Lee via lldb-commits]

@@ -2204,7 +2204,9 @@ static Status ParseInternal(llvm::StringRef , 
Entry _entry,
   return error;
 }
   } else if (FormatManager::GetFormatFromCString(
- entry.printf_format.c_str(), true, entry.fmt)) {
+ entry.printf_format.c_str(), true,
+ entry.fmt)) { // Try GetFormatFromCString again,

kastiglione wrote:

I guess if we assume that there's no overlap between printf and lldb format 
(which there shouldn't be) then the lldb format can be done first, and the 
printf can be done second, as partial.

https://github.com/llvm/llvm-project/pull/81018
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Add comment to ParseInternal in FormatEntity (NFC) (PR #81018)

[Dave Lee via lldb-commits]

@@ -2204,7 +2204,9 @@ static Status ParseInternal(llvm::StringRef , 
Entry _entry,
   return error;
 }
   } else if (FormatManager::GetFormatFromCString(
- entry.printf_format.c_str(), true, entry.fmt)) {
+ entry.printf_format.c_str(), true,
+ entry.fmt)) { // Try GetFormatFromCString again,

kastiglione wrote:

In summary, the order is: exact printf format > lldb format > partial printf 
format

https://github.com/llvm/llvm-project/pull/81018
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Add comment to ParseInternal in FormatEntity (NFC) (PR #81018)

[Dave Lee via lldb-commits]

https://github.com/kastiglione edited 
https://github.com/llvm/llvm-project/pull/81018
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Add comment to ParseInternal in FormatEntity (NFC) (PR #81018)

[Dave Lee via lldb-commits]

@@ -2204,7 +2204,9 @@ static Status ParseInternal(llvm::StringRef , 
Entry _entry,
   return error;
 }
   } else if (FormatManager::GetFormatFromCString(
- entry.printf_format.c_str(), true, entry.fmt)) {
+ entry.printf_format.c_str(), true,
+ entry.fmt)) { // Try GetFormatFromCString again,

kastiglione wrote:

doing partial from the outset would incorrectly match `%V` as `void` – the 
correct behavior is to print the lldb **V**alue. It's definitely not an ideal 
setup.

https://github.com/llvm/llvm-project/pull/81018
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Don't count all the frames just to skip the current inlined ones. (PR #80918)

[via lldb-commits]

jimingham wrote:

> > Should the Doxygen comment of GetStackFrameCount warn that this is an 
> > expensive operation? 
> > https://lldb.llvm.org/cpp_reference/classlldb__private_1_1Thread.html#afc54feef950a58b625bbb198dc4cf57c
> 
> It might be nice to add a "std::optional max_frame_count" to this 
> function to allow it to stop when it hits "max_frame_count". Like:
> 
> ```
> /// Get the number of frames in a thread.
> ///
> /// If \a max_frame_count is valid, return a number that is less than or 
> equal 
> /// to max_frame_count, else calculate the true number of frames. Calculating
> /// the total number of frames can be expensive.
> virtual uint32_t 
> lldb_private::Thread::GetStackFrameCount(std::optional 
> max_frame_count);
> ```

That seems an okay idea, but I wouldn't really want to use the new API in this 
patch.  I know I'm only looking to get past all the inlined frames, which is 
pretty cheap, but I have no way of knowing how many there are.  So it really 
wouldn't be a good idea to try to guess a max_frame_count.
I'd rather not add an API to a patch that doesn't actually use the API...

https://github.com/llvm/llvm-project/pull/80918
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Add comment to ParseInternal in FormatEntity (NFC) (PR #81018)

[Alex Langford via lldb-commits]

@@ -2204,7 +2204,9 @@ static Status ParseInternal(llvm::StringRef , 
Entry _entry,
   return error;
 }
   } else if (FormatManager::GetFormatFromCString(
- entry.printf_format.c_str(), true, entry.fmt)) {
+ entry.printf_format.c_str(), true,
+ entry.fmt)) { // Try GetFormatFromCString again,

bulbazord wrote:

Is the ordering important here? Why not try partial formatting first from the 
start? The code paths are nearly identical except for one parameter... The 
comment definitely helps disambiguate but it would be nicer if we could just 
rewrite this entirely.

https://github.com/llvm/llvm-project/pull/81018
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Expand background symbol lookup (PR #80890)

[Greg Clayton via lldb-commits]

clayborg wrote:

Does this setting only apply to the downloading of symbols that are auto 
downloaded for stack traces? Or does it apply to any other areas that auto 
download stuff? Do we want a setting to control the auto downloading of symbols 
to clarify? Something like:
```
(lldb) settings set symbols.auto-download-stack-symbols true
```
Right now your setting seems to be targeting stack auto symbol downloads, but 
this setting is pretty generic and could apply to other things in the future 
like auto downloading symbols when an address lookup happens on a module, 
finding the real definition for a type when we know a type lives in a shared 
library (like ObjC and C++ vtables can easily tell us).



https://github.com/llvm/llvm-project/pull/80890
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Add comment to ParseInternal in FormatEntity (NFC) (PR #81018)

[Dave Lee via lldb-commits]

https://github.com/kastiglione edited 
https://github.com/llvm/llvm-project/pull/81018
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Add comment to ParseInternal in FormatEntity (NFC) (PR #81018)

[Dave Lee via lldb-commits]

https://github.com/kastiglione edited 
https://github.com/llvm/llvm-project/pull/81018
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Add comment to ParseInternal in FormatEntity (NFC) (PR #81018)

[Dave Lee via lldb-commits]

https://github.com/kastiglione edited 
https://github.com/llvm/llvm-project/pull/81018
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Remove duplicate else-if branch (NFC) (PR #81018)

[Dave Lee via lldb-commits]

https://github.com/kastiglione updated 
https://github.com/llvm/llvm-project/pull/81018

>From f9d4abddb0f93d27ed14278566796babad9f7f4e Mon Sep 17 00:00:00 2001
From: Dave Lee 
Date: Wed, 7 Feb 2024 10:17:51 -0800
Subject: [PATCH] [lldb] Add comment to ParseInternal in FormatEntity

---
 lldb/source/Core/FormatEntity.cpp | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lldb/source/Core/FormatEntity.cpp 
b/lldb/source/Core/FormatEntity.cpp
index 3c665c2eb2133..b206b6e9983b8 100644
--- a/lldb/source/Core/FormatEntity.cpp
+++ b/lldb/source/Core/FormatEntity.cpp
@@ -2204,7 +2204,9 @@ static Status ParseInternal(llvm::StringRef , 
Entry _entry,
   return error;
 }
   } else if (FormatManager::GetFormatFromCString(
- entry.printf_format.c_str(), true, entry.fmt)) {
+ entry.printf_format.c_str(), true,
+ entry.fmt)) { // Try GetFormatFromCString again,
+   // with partial_match_ok = true.
 clear_printf = true;
   } else if (entry.printf_format == "tid") {
 verify_is_thread_id = true;

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

[Lldb-commits] [lldb] [lldb] Remove duplicate else-if branch (NFC) (PR #81018)

[Dave Lee via lldb-commits]

https://github.com/kastiglione reopened 
https://github.com/llvm/llvm-project/pull/81018
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Remove duplicate else-if branch (NFC) (PR #81018)

[Dave Lee via lldb-commits]

kastiglione wrote:

never mind, I see the slight difference

https://github.com/llvm/llvm-project/pull/81018
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Remove duplicate else-if branch (NFC) (PR #81018)

[Dave Lee via lldb-commits]

https://github.com/kastiglione closed 
https://github.com/llvm/llvm-project/pull/81018
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Remove duplicate else-if branch (NFC) (PR #81018)

[Dave Lee via lldb-commits]

https://github.com/kastiglione edited 
https://github.com/llvm/llvm-project/pull/81018
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Don't count all the frames just to skip the current inlined ones. (PR #80918)

[Greg Clayton via lldb-commits]

clayborg wrote:

> Should the Doxygen comment of GetStackFrameCount warn that this is an 
> expensive operation? 
> https://lldb.llvm.org/cpp_reference/classlldb__private_1_1Thread.html#afc54feef950a58b625bbb198dc4cf57c

It might be nice to add a "std::optional max_frame_count" to this 
function to allow it to stop when it hits "max_frame_count". Like:

```
/// Get the number of frames in a thread.
///
/// If \a max_frame_count is valid, return a number that is less than or equal 
/// to max_frame_count, else calculate the true number of frames. Calculating
/// the total number of frames can be expensive.
virtual uint32_t 
lldb_private::Thread::GetStackFrameCount(std::optional 
max_frame_count);
```

https://github.com/llvm/llvm-project/pull/80918
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Remove duplicate else-if branch (NFC) (PR #81018)

[via lldb-commits]

llvmbot wrote:




@llvm/pr-subscribers-lldb

Author: Dave Lee (kastiglione)


Changes



---
Full diff: https://github.com/llvm/llvm-project/pull/81018.diff


1 Files Affected:

- (modified) lldb/source/Core/FormatEntity.cpp (-3) 


``diff
diff --git a/lldb/source/Core/FormatEntity.cpp 
b/lldb/source/Core/FormatEntity.cpp
index 3c665c2eb2133..2a68d3938a201 100644
--- a/lldb/source/Core/FormatEntity.cpp
+++ b/lldb/source/Core/FormatEntity.cpp
@@ -2203,9 +2203,6 @@ static Status ParseInternal(llvm::StringRef , 
Entry _entry,
  entry.printf_format.c_str());
   return error;
 }
-  } else if (FormatManager::GetFormatFromCString(
- entry.printf_format.c_str(), true, entry.fmt)) {
-clear_printf = true;
   } else if (entry.printf_format == "tid") {
 verify_is_thread_id = true;
   } else {

``




https://github.com/llvm/llvm-project/pull/81018
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Remove duplicate else-if branch (NFC) (PR #81018)

[Dave Lee via lldb-commits]

https://github.com/kastiglione created 
https://github.com/llvm/llvm-project/pull/81018

None

>From 4f5b30fcb038c942df6a2fce975678c5b7ee1725 Mon Sep 17 00:00:00 2001
From: Dave Lee 
Date: Wed, 7 Feb 2024 08:43:14 -0800
Subject: [PATCH] [lldb] Remove duplicate else-if branch (NFC)

---
 lldb/source/Core/FormatEntity.cpp | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/lldb/source/Core/FormatEntity.cpp 
b/lldb/source/Core/FormatEntity.cpp
index 3c665c2eb2133b..2a68d3938a201d 100644
--- a/lldb/source/Core/FormatEntity.cpp
+++ b/lldb/source/Core/FormatEntity.cpp
@@ -2203,9 +2203,6 @@ static Status ParseInternal(llvm::StringRef , 
Entry _entry,
  entry.printf_format.c_str());
   return error;
 }
-  } else if (FormatManager::GetFormatFromCString(
- entry.printf_format.c_str(), true, entry.fmt)) {
-clear_printf = true;
   } else if (entry.printf_format == "tid") {
 verify_is_thread_id = true;
   } else {

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

[Lldb-commits] [lldb] Don't count all the frames just to skip the current inlined ones. (PR #80918)

[Adrian Prantl via lldb-commits]

adrian-prantl wrote:

Should the Doxygen comment of GetStackFrameCount warn that this is an expensive 
operation?
https://lldb.llvm.org/cpp_reference/classlldb__private_1_1Thread.html#afc54feef950a58b625bbb198dc4cf57c

https://github.com/llvm/llvm-project/pull/80918
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Add frame recognizer for __builtin_verbose_trap (PR #80368)

[Dan Liew via lldb-commits]

@@ -0,0 +1,9 @@
+# RUN: %clang_host -g -O0 %S/Inputs/verbose_trap.cpp -o %t.out
+# RUN: %lldb -b -s %s %t.out | FileCheck %s
+run
+# CHECK: thread #{{.*}}stop reason = Runtime Error: Function is not implemented

delcypher wrote:

`Runtime Error:` is pretty generic. Is there any reason we aren't mentioning 
that a trap was hit?

https://github.com/llvm/llvm-project/pull/80368
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Add frame recognizer for __builtin_verbose_trap (PR #80368)

[Dan Liew via lldb-commits]

@@ -0,0 +1,98 @@
+#include "lldb/Target/VerboseTrapFrameRecognizer.h"
+
+#include "lldb/Core/Module.h"
+#include "lldb/Symbol/Function.h"
+#include "lldb/Symbol/SymbolContext.h"
+#include "lldb/Target/Process.h"
+#include "lldb/Target/Target.h"
+
+#include "lldb/Utility/LLDBLog.h"
+#include "lldb/Utility/Log.h"
+
+using namespace llvm;
+using namespace lldb;
+using namespace lldb_private;
+
+VerboseTrapRecognizedStackFrame::VerboseTrapRecognizedStackFrame(
+StackFrameSP most_relevant_frame_sp, std::string stop_desc)
+: m_most_relevant_frame(most_relevant_frame_sp) {
+  m_stop_desc = std::move(stop_desc);
+}
+
+lldb::RecognizedStackFrameSP
+VerboseTrapFrameRecognizer::RecognizeFrame(lldb::StackFrameSP frame_sp) {
+  if (frame_sp->GetFrameIndex())
+return {};
+
+  ThreadSP thread_sp = frame_sp->GetThread();
+  ProcessSP process_sp = thread_sp->GetProcess();
+
+  StackFrameSP most_relevant_frame_sp = thread_sp->GetStackFrameAtIndex(1);
+
+  if (!most_relevant_frame_sp) {
+Log *log = GetLog(LLDBLog::Unwind);
+LLDB_LOG(
+log,
+"Failed to find most relevant frame: Hit unwinding bound (1 frame)!");
+return {};
+  }
+
+  SymbolContext sc = frame_sp->GetSymbolContext(eSymbolContextEverything);
+
+  if (!sc.block)
+return {};
+
+  // The runtime error is set as the function name in the inlined function info
+  // of frame #0 by the compiler
+  const InlineFunctionInfo *inline_info = nullptr;
+  Block *inline_block = sc.block->GetContainingInlinedBlock();
+
+  if (!inline_block)
+return {};
+
+  inline_info = sc.block->GetInlinedFunctionInfo();
+
+  if (!inline_info)
+return {};
+
+  auto error_message = inline_info->GetName().GetString();
+  if (error_message.empty())
+return {};
+
+  // Replaces "__llvm_verbose_trap: " with "Runtime Error: "
+  auto space_position = error_message.find(" ");
+  if (space_position == std::string::npos) {
+Log *log = GetLog(LLDBLog::Unwind);
+LLDB_LOGF(log,
+  "Unexpected function name format. Expected ': "
+  "' but got: '%s'.",
+  error_message.c_str());
+
+return {};
+  }
+
+  error_message.replace(0, space_position, "Runtime Error:");
+
+  return lldb::RecognizedStackFrameSP(new VerboseTrapRecognizedStackFrame(
+  most_relevant_frame_sp, std::move(error_message)));
+}
+
+lldb::StackFrameSP VerboseTrapRecognizedStackFrame::GetMostRelevantFrame() {
+  return m_most_relevant_frame;
+}
+
+namespace lldb_private {
+
+void RegisterVerboseTrapFrameRecognizer(Process ) {
+  RegularExpressionSP module_regex_sp = nullptr;
+  RegularExpressionSP symbol_regex_sp(
+  new RegularExpression("^__llvm_verbose_trap: "));

delcypher wrote:

Rather than hard coding the name should we be getting it from a header file 
vended by Clang?

I mentioned this in the corresponding Clang review 
[here](https://github.com/llvm/llvm-project/pull/79230#discussion_r1475241985).

This wouldn't need to be anything sophisticated. Just a header file under 
`include/clang/CodeGen` that is something like

```
#define CLANG_VERBOSE_TRAP_PREFIX "__llvm_verbose_trap"
```

and then from LLDB that would be something like:

```
new RegularExpression("^" CLANG_VERBOSE_TRAP_PREFIX  ": ")
```

That way there is a single source of truth for the name used.

https://github.com/llvm/llvm-project/pull/80368
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Add frame recognizer for __builtin_verbose_trap (PR #80368)

[Jonas Devlieghere via lldb-commits]

@@ -0,0 +1,39 @@
+#ifndef LLDB_TARGET_VERBOSETRAPFRAMERECOGNIZER_H

JDevlieghere wrote:

Missing license header.

https://github.com/llvm/llvm-project/pull/80368
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Add frame recognizer for __builtin_verbose_trap (PR #80368)

[Jonas Devlieghere via lldb-commits]

https://github.com/JDevlieghere approved this pull request.

LGTM with comments addressed. 

https://github.com/llvm/llvm-project/pull/80368
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Add frame recognizer for __builtin_verbose_trap (PR #80368)

[Jonas Devlieghere via lldb-commits]

@@ -0,0 +1,98 @@
+#include "lldb/Target/VerboseTrapFrameRecognizer.h"
+
+#include "lldb/Core/Module.h"
+#include "lldb/Symbol/Function.h"
+#include "lldb/Symbol/SymbolContext.h"
+#include "lldb/Target/Process.h"
+#include "lldb/Target/Target.h"
+
+#include "lldb/Utility/LLDBLog.h"
+#include "lldb/Utility/Log.h"
+
+using namespace llvm;
+using namespace lldb;
+using namespace lldb_private;
+
+VerboseTrapRecognizedStackFrame::VerboseTrapRecognizedStackFrame(
+StackFrameSP most_relevant_frame_sp, std::string stop_desc)
+: m_most_relevant_frame(most_relevant_frame_sp) {
+  m_stop_desc = std::move(stop_desc);
+}
+
+lldb::RecognizedStackFrameSP
+VerboseTrapFrameRecognizer::RecognizeFrame(lldb::StackFrameSP frame_sp) {
+  if (frame_sp->GetFrameIndex())
+return {};
+
+  ThreadSP thread_sp = frame_sp->GetThread();
+  ProcessSP process_sp = thread_sp->GetProcess();
+
+  StackFrameSP most_relevant_frame_sp = thread_sp->GetStackFrameAtIndex(1);
+
+  if (!most_relevant_frame_sp) {
+Log *log = GetLog(LLDBLog::Unwind);
+LLDB_LOG(
+log,
+"Failed to find most relevant frame: Hit unwinding bound (1 frame)!");
+return {};
+  }
+
+  SymbolContext sc = frame_sp->GetSymbolContext(eSymbolContextEverything);
+
+  if (!sc.block)
+return {};
+
+  // The runtime error is set as the function name in the inlined function info
+  // of frame #0 by the compiler
+  const InlineFunctionInfo *inline_info = nullptr;
+  Block *inline_block = sc.block->GetContainingInlinedBlock();
+
+  if (!inline_block)
+return {};
+
+  inline_info = sc.block->GetInlinedFunctionInfo();
+
+  if (!inline_info)
+return {};
+
+  auto error_message = inline_info->GetName().GetString();
+  if (error_message.empty())
+return {};
+
+  // Replaces "__llvm_verbose_trap: " with "Runtime Error: "
+  auto space_position = error_message.find(" ");
+  if (space_position == std::string::npos) {
+Log *log = GetLog(LLDBLog::Unwind);
+LLDB_LOGF(log,

JDevlieghere wrote:

`LLDB_LOG` for consistency?

https://github.com/llvm/llvm-project/pull/80368
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Add frame recognizer for __builtin_verbose_trap (PR #80368)

[Jonas Devlieghere via lldb-commits]

@@ -0,0 +1,98 @@
+#include "lldb/Target/VerboseTrapFrameRecognizer.h"
+
+#include "lldb/Core/Module.h"
+#include "lldb/Symbol/Function.h"
+#include "lldb/Symbol/SymbolContext.h"
+#include "lldb/Target/Process.h"
+#include "lldb/Target/Target.h"
+
+#include "lldb/Utility/LLDBLog.h"
+#include "lldb/Utility/Log.h"
+
+using namespace llvm;
+using namespace lldb;
+using namespace lldb_private;
+
+VerboseTrapRecognizedStackFrame::VerboseTrapRecognizedStackFrame(
+StackFrameSP most_relevant_frame_sp, std::string stop_desc)
+: m_most_relevant_frame(most_relevant_frame_sp) {
+  m_stop_desc = std::move(stop_desc);
+}
+
+lldb::RecognizedStackFrameSP
+VerboseTrapFrameRecognizer::RecognizeFrame(lldb::StackFrameSP frame_sp) {
+  if (frame_sp->GetFrameIndex())
+return {};
+
+  ThreadSP thread_sp = frame_sp->GetThread();
+  ProcessSP process_sp = thread_sp->GetProcess();
+
+  StackFrameSP most_relevant_frame_sp = thread_sp->GetStackFrameAtIndex(1);
+
+  if (!most_relevant_frame_sp) {
+Log *log = GetLog(LLDBLog::Unwind);
+LLDB_LOG(
+log,
+"Failed to find most relevant frame: Hit unwinding bound (1 frame)!");
+return {};
+  }
+
+  SymbolContext sc = frame_sp->GetSymbolContext(eSymbolContextEverything);
+
+  if (!sc.block)
+return {};
+
+  // The runtime error is set as the function name in the inlined function info
+  // of frame #0 by the compiler
+  const InlineFunctionInfo *inline_info = nullptr;
+  Block *inline_block = sc.block->GetContainingInlinedBlock();
+
+  if (!inline_block)
+return {};
+
+  inline_info = sc.block->GetInlinedFunctionInfo();
+
+  if (!inline_info)
+return {};
+
+  auto error_message = inline_info->GetName().GetString();
+  if (error_message.empty())
+return {};
+
+  // Replaces "__llvm_verbose_trap: " with "Runtime Error: "
+  auto space_position = error_message.find(" ");
+  if (space_position == std::string::npos) {
+Log *log = GetLog(LLDBLog::Unwind);

JDevlieghere wrote:

Maybe worth hoisting `Log *log = GetLog(LLDBLog::Unwind);`, it should be pretty 
cheap?

https://github.com/llvm/llvm-project/pull/80368
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Add frame recognizer for __builtin_verbose_trap (PR #80368)

[Jonas Devlieghere via lldb-commits]

https://github.com/JDevlieghere edited 
https://github.com/llvm/llvm-project/pull/80368
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits