[Lldb-commits] [lldb] 1912879 - Reland "[lldb][DWARFASTParserClang] Attach linkage name to ctors/dtors if missing"

2023-02-12 Thread Michael Buch via lldb-commits

Author: Michael Buch
Date: 2023-02-12T10:14:43Z
New Revision: 19128792e2aa320c1a149f7f93638cbd7f3c83c6

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

LOG: Reland "[lldb][DWARFASTParserClang] Attach linkage name to ctors/dtors if 
missing"

This relands the commit previously reverted in
`d2cc2c5610ffa78736aa99512bc85a85417efb0a` due to failures on Linux
when debugging split-debug-info enabled executables.

The problem was we called `SymbolFileDWARF::FindFunctions` directly
instead of `Module::FindFunctions` which resulted in a nullptr
dereference because the backing `SymbolFileDWARFDwo` didn't have
an index attached to it. The relanded version calls `Module::FindFunctions`
instead.

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

Added: 
lldb/test/API/lang/cpp/external_ctor_dtor_lookup/Makefile

lldb/test/API/lang/cpp/external_ctor_dtor_lookup/TestExternalCtorDtorLookup.py
lldb/test/API/lang/cpp/external_ctor_dtor_lookup/lib.h
lldb/test/API/lang/cpp/external_ctor_dtor_lookup/main.cpp

Modified: 
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

Removed: 




diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index 4429b4fcae2a0..8401d648c6704 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -888,6 +888,46 @@ ConvertDWARFCallingConventionToClang(const 
ParsedDWARFTypeAttributes &attrs) {
   return clang::CC_C;
 }
 
+/// Given a DIE with an external definition (and thus no linkage name)
+/// find the definitions by lookup into the DWARF name index.
+/// We check the DW_AT_specification for each DIE in the index with
+/// the same name as the specified 'die' until we find one that references
+/// 'die'. Then return that linkage name. If no such DIE is found in the index,
+/// returns nullptr.
+static char const *FindLinkageName(DWARFDIE die) {
+  auto *dwarf = die.GetDWARF();
+  if (!dwarf)
+return nullptr;
+
+  ConstString func_name(die.GetName());
+  if (!func_name)
+return nullptr;
+
+  SymbolContextList sc_list;
+  Module::LookupInfo lookup_info(func_name,
+ FunctionNameType::eFunctionNameTypeMethod |
+ FunctionNameType::eFunctionNameTypeFull,
+ LanguageType::eLanguageTypeUnknown);
+  dwarf->GetObjectFile()->GetModule()->FindFunctions(lookup_info, {}, {},
+ sc_list);
+
+  for (auto const &sc : sc_list.SymbolContexts()) {
+if (auto *func = sc.function) {
+  auto func_die = dwarf->GetDIE(func->GetID());
+  if (!func_die.IsValid())
+continue;
+
+  auto spec_die =
+  func_die.GetAttributeValueAsReferenceDIE(DW_AT_specification);
+  if (spec_die.IsValid() && spec_die == die) {
+return func->GetMangled().GetMangledName().AsCString();
+  }
+}
+  }
+
+  return nullptr;
+}
+
 TypeSP DWARFASTParserClang::ParseSubroutine(const DWARFDIE &die,
ParsedDWARFTypeAttributes &attrs) {
   Log *log = GetLog(DWARFLog::TypeCompletion | DWARFLog::Lookups);
@@ -1116,6 +1156,9 @@ TypeSP DWARFASTParserClang::ParseSubroutine(const 
DWARFDIE &die,
   if (attrs.accessibility == eAccessNone)
 attrs.accessibility = eAccessPublic;
 
+  if (!attrs.mangled_name && attrs.storage == clang::SC_Extern)
+attrs.mangled_name = FindLinkageName(die);
+
   clang::CXXMethodDecl *cxx_method_decl =
   m_ast.AddMethodToCXXRecordType(
   class_opaque_type.GetOpaqueQualType(),

diff  --git a/lldb/test/API/lang/cpp/external_ctor_dtor_lookup/Makefile 
b/lldb/test/API/lang/cpp/external_ctor_dtor_lookup/Makefile
new file mode 100644
index 0..eba15476332f9
--- /dev/null
+++ b/lldb/test/API/lang/cpp/external_ctor_dtor_lookup/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+   
+include Makefile.rules 

diff  --git 
a/lldb/test/API/lang/cpp/external_ctor_dtor_lookup/TestExternalCtorDtorLookup.py
 
b/lldb/test/API/lang/cpp/external_ctor_dtor_lookup/TestExternalCtorDtorLookup.py
new file mode 100644
index 0..e436252800a79
--- /dev/null
+++ 
b/lldb/test/API/lang/cpp/external_ctor_dtor_lookup/TestExternalCtorDtorLookup.py
@@ -0,0 +1,31 @@
+"""
+Test that we can constructors/destructors
+without a linkage name because they are
+marked DW_AT_external and the fallback
+mangled-name-guesser in LLDB doesn't account
+for ABI tags.
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest

[Lldb-commits] [PATCH] D143842: [lldb][test] Add check for Xcode binutils version to test-runner

2023-02-12 Thread Michael Buch via Phabricator via lldb-commits
Michael137 created this revision.
Herald added a project: All.
Michael137 requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Some tests in LLDB require the use of binutils tools. On Darwin,
we need to use the tools that ship with Xcode. Otherwise we risk
crashing in unpredictable ways.

This patch adds a check for this and bails early if the user has
a non-Xcode binutils in their path before the Xcode ones.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D143842

Files:
  lldb/packages/Python/lldbsuite/test/dotest.py


Index: lldb/packages/Python/lldbsuite/test/dotest.py
===
--- lldb/packages/Python/lldbsuite/test/dotest.py
+++ lldb/packages/Python/lldbsuite/test/dotest.py
@@ -869,6 +869,28 @@
 if platform not in ["freebsd", "linux", "netbsd"]:
 configuration.skip_categories.append("fork")
 
+def checkDarwinBinutilsPath():
+from lldbsuite.test import lldbplatformutil
+
+if not lldbplatformutil.platformIsDarwin():
+return
+
+cmd = ["strip", "--version"]
+process = subprocess.Popen(
+cmd,
+stdout=subprocess.PIPE,
+stderr=subprocess.STDOUT)
+cmd_output = process.stdout.read()
+output_str = cmd_output.decode("utf-8")
+if "GNU strip (GNU Binutils)" in output_str:
+print("""
+The tool 'strip' in your path is not the one from the Xcode
+toolchain. Please make sure the Xcode tools are before any
+other tools in your path. Run `xcode -f strip` for the correct
+toolchain path.
+Exiting...
+""")
+sys.exit(0)
 
 def run_suite():
 # On MacOS X, check to make sure that domain for com.apple.DebugSymbols 
defaults
@@ -956,6 +978,7 @@
 checkDebugServerSupport()
 checkObjcSupport()
 checkForkVForkSupport()
+checkDarwinBinutilsPath()
 
 skipped_categories_list = ", ".join(configuration.skip_categories)
 print("Skipping the following test categories: 
{}".format(configuration.skip_categories))


Index: lldb/packages/Python/lldbsuite/test/dotest.py
===
--- lldb/packages/Python/lldbsuite/test/dotest.py
+++ lldb/packages/Python/lldbsuite/test/dotest.py
@@ -869,6 +869,28 @@
 if platform not in ["freebsd", "linux", "netbsd"]:
 configuration.skip_categories.append("fork")
 
+def checkDarwinBinutilsPath():
+from lldbsuite.test import lldbplatformutil
+
+if not lldbplatformutil.platformIsDarwin():
+return
+
+cmd = ["strip", "--version"]
+process = subprocess.Popen(
+cmd,
+stdout=subprocess.PIPE,
+stderr=subprocess.STDOUT)
+cmd_output = process.stdout.read()
+output_str = cmd_output.decode("utf-8")
+if "GNU strip (GNU Binutils)" in output_str:
+print("""
+The tool 'strip' in your path is not the one from the Xcode
+toolchain. Please make sure the Xcode tools are before any
+other tools in your path. Run `xcode -f strip` for the correct
+toolchain path.
+Exiting...
+""")
+sys.exit(0)
 
 def run_suite():
 # On MacOS X, check to make sure that domain for com.apple.DebugSymbols defaults
@@ -956,6 +978,7 @@
 checkDebugServerSupport()
 checkObjcSupport()
 checkForkVForkSupport()
+checkDarwinBinutilsPath()
 
 skipped_categories_list = ", ".join(configuration.skip_categories)
 print("Skipping the following test categories: {}".format(configuration.skip_categories))
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D143842: [lldb][test] Add check for Xcode binutils version to test-runner

2023-02-12 Thread Thorsten via Phabricator via lldb-commits
tschuett added inline comments.



Comment at: lldb/packages/Python/lldbsuite/test/dotest.py:889
+toolchain. Please make sure the Xcode tools are before any
+other tools in your path. Run `xcode -f strip` for the correct
+toolchain path.

Did you mean `xcrun -f strip`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143842

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


[Lldb-commits] [PATCH] D143842: [lldb][test] Add check for Xcode binutils version to test-runner

2023-02-12 Thread Michael Buch via Phabricator via lldb-commits
Michael137 added inline comments.



Comment at: lldb/packages/Python/lldbsuite/test/dotest.py:889
+toolchain. Please make sure the Xcode tools are before any
+other tools in your path. Run `xcode -f strip` for the correct
+toolchain path.

tschuett wrote:
> Did you mean `xcrun -f strip`?
yup! good catch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143842

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


[Lldb-commits] [PATCH] D143842: [lldb][test] Add check for Xcode binutils version to test-runner

2023-02-12 Thread Michael Buch via Phabricator via lldb-commits
Michael137 added a comment.

My preference would be to check explicitly whether the tool is from the xcode 
toolchain. But the tools dont really have a distinguishable version (in fact 
the `strip` from Xcode doesnt have a version commandline flag. Perhaps that’s 
deliberate and a good way to disbiguate homebrew vs xcode?)

Another option would be to check that homebrew/binutils only appears after 
xcode in the user path


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143842

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


[Lldb-commits] [PATCH] D143398: [lldb][DWARFASTParserClang] Correctly resolve imported namespaces during expression evaluation

2023-02-12 Thread Michael Buch via Phabricator via lldb-commits
Michael137 reopened this revision.
Michael137 added a comment.
This revision is now accepted and ready to land.

Reopening to Ireland slightly adjusted patch.
New patch makes sure we populate the `ManualDWARFIndex` with 
`DW_TAG_imported_declaration`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143398

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


[Lldb-commits] [PATCH] D143398: [lldb][DWARFASTParserClang] Correctly resolve imported namespaces during expression evaluation

2023-02-12 Thread Michael Buch via Phabricator via lldb-commits
Michael137 updated this revision to Diff 496774.
Michael137 added a comment.
Herald added a subscriber: arphaman.

- Populate `ManualDWARFIndex`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143398

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
  lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
  lldb/test/API/commands/expression/namespace-alias/Makefile
  lldb/test/API/commands/expression/namespace-alias/TestInlineNamespaceAlias.py
  lldb/test/API/commands/expression/namespace-alias/main.cpp

Index: lldb/test/API/commands/expression/namespace-alias/main.cpp
===
--- /dev/null
+++ lldb/test/API/commands/expression/namespace-alias/main.cpp
@@ -0,0 +1,21 @@
+namespace A {
+inline namespace _A {
+namespace B {
+namespace C {
+int a = -1;
+
+int func() { return 0; }
+} // namespace C
+} // namespace B
+
+namespace C = B::C;
+namespace D = B::C;
+
+} // namespace _A
+} // namespace A
+
+namespace E = A;
+namespace F = E::C;
+namespace G = F;
+
+int main(int argc, char **argv) { return A::B::C::a; }
Index: lldb/test/API/commands/expression/namespace-alias/TestInlineNamespaceAlias.py
===
--- /dev/null
+++ lldb/test/API/commands/expression/namespace-alias/TestInlineNamespaceAlias.py
@@ -0,0 +1,37 @@
+"""
+Test that we correctly handle namespace
+expression evaluation through namespace
+aliases.
+"""
+
+import lldb
+
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestInlineNamespace(TestBase):
+def do_test(self, params):
+self.build()
+
+lldbutil.run_to_source_breakpoint(self,
+  "return A::B::C::a", lldb.SBFileSpec("main.cpp"))
+
+self.expect_expr("A::C::a", result_type="int", result_value="-1")
+self.expect_expr("A::D::a", result_type="int", result_value="-1")
+
+self.expect_expr("A::C::func()", result_type="int", result_value="0")
+self.expect_expr("A::D::func()", result_type="int", result_value="0")
+
+self.expect_expr("E::C::a", result_type="int", result_value="-1")
+self.expect_expr("E::D::a", result_type="int", result_value="-1")
+self.expect_expr("F::a", result_type="int", result_value="-1")
+self.expect_expr("G::a", result_type="int", result_value="-1")
+
+@skipIf(debug_info=no_match(["dsym"]))
+def test_dsym(self):
+self.do_test({})
+
+@skipIf(debug_info="dsym")
+def test_dwarf(self):
+self.do_test(dict(CFLAGS_EXTRAS="-gdwarf-5 -gpubnames"))
Index: lldb/test/API/commands/expression/namespace-alias/Makefile
===
--- /dev/null
+++ lldb/test/API/commands/expression/namespace-alias/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
Index: lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
@@ -21,6 +21,7 @@
 #include "lldb/Utility/DataExtractor.h"
 #include "lldb/Utility/Stream.h"
 #include "lldb/Utility/Timer.h"
+#include "llvm/BinaryFormat/Dwarf.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/ThreadPool.h"
 #include 
@@ -208,6 +209,7 @@
 case DW_TAG_enumeration_type:
 case DW_TAG_inlined_subroutine:
 case DW_TAG_namespace:
+case DW_TAG_imported_declaration:
 case DW_TAG_string_type:
 case DW_TAG_structure_type:
 case DW_TAG_subprogram:
@@ -354,6 +356,7 @@
   break;
 
 case DW_TAG_namespace:
+case DW_TAG_imported_declaration:
   if (name)
 set.namespaces.Insert(ConstString(name), ref);
   break;
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
@@ -132,6 +132,17 @@
 
   clang::NamespaceDecl *ResolveNamespaceDIE(const DWARFDIE &die);
 
+  /// Returns the namespace decl that a DW_TAG_imported_declaration imports.
+  ///
+  /// \param[in] die The import declaration to resolve. If the DIE is not a
+  ///DW_TAG_imported_declaration the behaviour is undefined.
+  ///
+  /// \returns The decl corresponding to the namespace that the specified
+  ///  'die' imports. If the imported entity is not a namespace
+  ///  or another import declaration, returns nullptr. If an error
+  ///  occurs, returns nullptr.
+  clang::NamespaceDecl *ResolveImportedDeclarationDIE(const DWARFDIE &die);

[Lldb-commits] [PATCH] D143398: [lldb][DWARFASTParserClang] Correctly resolve imported namespaces during expression evaluation

2023-02-12 Thread Michael Buch via Phabricator via lldb-commits
Michael137 updated this revision to Diff 496775.
Michael137 added a comment.

- Remove redundant header


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143398

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
  lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
  lldb/test/API/commands/expression/namespace-alias/Makefile
  lldb/test/API/commands/expression/namespace-alias/TestInlineNamespaceAlias.py
  lldb/test/API/commands/expression/namespace-alias/main.cpp

Index: lldb/test/API/commands/expression/namespace-alias/main.cpp
===
--- /dev/null
+++ lldb/test/API/commands/expression/namespace-alias/main.cpp
@@ -0,0 +1,21 @@
+namespace A {
+inline namespace _A {
+namespace B {
+namespace C {
+int a = -1;
+
+int func() { return 0; }
+} // namespace C
+} // namespace B
+
+namespace C = B::C;
+namespace D = B::C;
+
+} // namespace _A
+} // namespace A
+
+namespace E = A;
+namespace F = E::C;
+namespace G = F;
+
+int main(int argc, char **argv) { return A::B::C::a; }
Index: lldb/test/API/commands/expression/namespace-alias/TestInlineNamespaceAlias.py
===
--- /dev/null
+++ lldb/test/API/commands/expression/namespace-alias/TestInlineNamespaceAlias.py
@@ -0,0 +1,37 @@
+"""
+Test that we correctly handle namespace
+expression evaluation through namespace
+aliases.
+"""
+
+import lldb
+
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestInlineNamespace(TestBase):
+def do_test(self, params):
+self.build()
+
+lldbutil.run_to_source_breakpoint(self,
+  "return A::B::C::a", lldb.SBFileSpec("main.cpp"))
+
+self.expect_expr("A::C::a", result_type="int", result_value="-1")
+self.expect_expr("A::D::a", result_type="int", result_value="-1")
+
+self.expect_expr("A::C::func()", result_type="int", result_value="0")
+self.expect_expr("A::D::func()", result_type="int", result_value="0")
+
+self.expect_expr("E::C::a", result_type="int", result_value="-1")
+self.expect_expr("E::D::a", result_type="int", result_value="-1")
+self.expect_expr("F::a", result_type="int", result_value="-1")
+self.expect_expr("G::a", result_type="int", result_value="-1")
+
+@skipIf(debug_info=no_match(["dsym"]))
+def test_dsym(self):
+self.do_test({})
+
+@skipIf(debug_info="dsym")
+def test_dwarf(self):
+self.do_test(dict(CFLAGS_EXTRAS="-gdwarf-5 -gpubnames"))
Index: lldb/test/API/commands/expression/namespace-alias/Makefile
===
--- /dev/null
+++ lldb/test/API/commands/expression/namespace-alias/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
Index: lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
@@ -208,6 +208,7 @@
 case DW_TAG_enumeration_type:
 case DW_TAG_inlined_subroutine:
 case DW_TAG_namespace:
+case DW_TAG_imported_declaration:
 case DW_TAG_string_type:
 case DW_TAG_structure_type:
 case DW_TAG_subprogram:
@@ -354,6 +355,7 @@
   break;
 
 case DW_TAG_namespace:
+case DW_TAG_imported_declaration:
   if (name)
 set.namespaces.Insert(ConstString(name), ref);
   break;
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
@@ -132,6 +132,17 @@
 
   clang::NamespaceDecl *ResolveNamespaceDIE(const DWARFDIE &die);
 
+  /// Returns the namespace decl that a DW_TAG_imported_declaration imports.
+  ///
+  /// \param[in] die The import declaration to resolve. If the DIE is not a
+  ///DW_TAG_imported_declaration the behaviour is undefined.
+  ///
+  /// \returns The decl corresponding to the namespace that the specified
+  ///  'die' imports. If the imported entity is not a namespace
+  ///  or another import declaration, returns nullptr. If an error
+  ///  occurs, returns nullptr.
+  clang::NamespaceDecl *ResolveImportedDeclarationDIE(const DWARFDIE &die);
+
   bool ParseTemplateDIE(const DWARFDIE &die,
 lldb_private::TypeSystemClang::TemplateParameterInfos
 &template_param_infos);
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

[Lldb-commits] [lldb] 129eb5b - [lldb] Add the ability to provide a message to a progress event update

2023-02-12 Thread Jonas Devlieghere via lldb-commits

Author: Jonas Devlieghere
Date: 2023-02-12T11:17:58-08:00
New Revision: 129eb5bcab91a12ed3c4712279f201834ae2d8e1

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

LOG: [lldb] Add the ability to provide a message to a progress event update

Consider the following example as motivation. Say you have to load
symbols for 3 dynamic libraries: `libFoo`, `libBar` and `libBaz`.
Currently, there are two ways to report process for this operation:

 1. As 3 separate progress instances. In this case you create a progress
instance with the message "Loading symbols: libFoo", "Loading
symbols: libBar", and "Loading symbols: libBaz" respectively. Each
progress event gets a unique ID and therefore cannot be correlated
by the consumer.

 2. As 1 progress instance with 3 units of work. The title would be
"Loading symbols" and you call Progress::Increment for each of the
libraries. The 3 progress events share the same ID and can easily be
correlated, however, in the current design, there's no way to
include the name of the libraries.

The second approach is preferred when the amount of work is known in
advance, because determinate progress can be reported (i.e. x out of y
operations completed). An additional benefit is that the progress
consumer can decide to ignore certain progress updates by their ID if
they are deemed to noisy, which isn't trivial for the first approach due
to the use of different progress IDs.

This patch adds the ability to add a message (detail) to a progress
event update. For the example described above, progress can now be
displayed as shown:

  [1/3] Loading symbols: libFoo
  [2/3] Loading symbols: libBar
  [3/3] Loading symbols: libBaz

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

Added: 


Modified: 
lldb/include/lldb/Core/Debugger.h
lldb/include/lldb/Core/DebuggerEvents.h
lldb/include/lldb/Core/Progress.h
lldb/source/API/SBDebugger.cpp
lldb/source/Core/Debugger.cpp
lldb/source/Core/DebuggerEvents.cpp
lldb/source/Core/Progress.cpp

Removed: 




diff  --git a/lldb/include/lldb/Core/Debugger.h 
b/lldb/include/lldb/Core/Debugger.h
index dd3e6c061fcf6..9f83a37bc75cc 100644
--- a/lldb/include/lldb/Core/Debugger.h
+++ b/lldb/include/lldb/Core/Debugger.h
@@ -484,8 +484,9 @@ class Debugger : public 
std::enable_shared_from_this,
   ///   debugger identifier that this progress should be delivered to. If this
   ///   optional parameter does not have a value, the progress will be
   ///   delivered to all debuggers.
-  static void ReportProgress(uint64_t progress_id, const std::string &message,
- uint64_t completed, uint64_t total,
+  static void ReportProgress(uint64_t progress_id, std::string title,
+ std::string details, uint64_t completed,
+ uint64_t total,
  std::optional debugger_id);
 
   static void ReportDiagnosticImpl(DiagnosticEventData::Type type,

diff  --git a/lldb/include/lldb/Core/DebuggerEvents.h 
b/lldb/include/lldb/Core/DebuggerEvents.h
index 0dade0f9629a0..f9ae67efac323 100644
--- a/lldb/include/lldb/Core/DebuggerEvents.h
+++ b/lldb/include/lldb/Core/DebuggerEvents.h
@@ -21,10 +21,11 @@ class Stream;
 
 class ProgressEventData : public EventData {
 public:
-  ProgressEventData(uint64_t progress_id, const std::string &message,
+  ProgressEventData(uint64_t progress_id, std::string title, std::string 
update,
 uint64_t completed, uint64_t total, bool debugger_specific)
-  : m_message(message), m_id(progress_id), m_completed(completed),
-m_total(total), m_debugger_specific(debugger_specific) {}
+  : m_title(std::move(title)), m_details(std::move(update)),
+m_id(progress_id), m_completed(completed), m_total(total),
+m_debugger_specific(debugger_specific) {}
 
   static ConstString GetFlavorString();
 
@@ -41,12 +42,30 @@ class ProgressEventData : public EventData {
   bool IsFinite() const { return m_total != UINT64_MAX; }
   uint64_t GetCompleted() const { return m_completed; }
   uint64_t GetTotal() const { return m_total; }
-  const std::string &GetMessage() const { return m_message; }
+  std::string GetMessage() const {
+std::string message = m_title;
+if (!m_details.empty()) {
+  message.append(": ");
+  message.append(m_details);
+}
+return message;
+  }
+  const std::string &GetTitle() const { return m_title; }
+  const std::string &GetDetails() const { return m_details; }
   bool IsDebuggerSpecific() const { return m_debugger_specific; }
 
 private:
-  std::string m_message;
+  /// The title of this progress event. The value is expected to remain stable
+  /// for a given progress ID.
+  std::string m_ti

[Lldb-commits] [PATCH] D143690: [lldb] Add the ability to provide a message to a progress event update

2023-02-12 Thread Jonas Devlieghere via Phabricator via lldb-commits
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG129eb5bcab91: [lldb] Add the ability to provide a message to 
a progress event update (authored by JDevlieghere).
Herald added a project: LLDB.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143690

Files:
  lldb/include/lldb/Core/Debugger.h
  lldb/include/lldb/Core/DebuggerEvents.h
  lldb/include/lldb/Core/Progress.h
  lldb/source/API/SBDebugger.cpp
  lldb/source/Core/Debugger.cpp
  lldb/source/Core/DebuggerEvents.cpp
  lldb/source/Core/Progress.cpp

Index: lldb/source/Core/Progress.cpp
===
--- lldb/source/Core/Progress.cpp
+++ lldb/source/Core/Progress.cpp
@@ -36,7 +36,7 @@
   }
 }
 
-void Progress::Increment(uint64_t amount) {
+void Progress::Increment(uint64_t amount, std::string update) {
   if (amount > 0) {
 std::lock_guard guard(m_mutex);
 // Watch out for unsigned overflow and make sure we don't increment too
@@ -45,16 +45,16 @@
   m_completed = m_total;
 else
   m_completed += amount;
-ReportProgress();
+ReportProgress(update);
   }
 }
 
-void Progress::ReportProgress() {
+void Progress::ReportProgress(std::string update) {
   if (!m_complete) {
 // Make sure we only send one notification that indicates the progress is
 // complete.
 m_complete = m_completed == m_total;
-Debugger::ReportProgress(m_id, m_title, m_completed, m_total,
- m_debugger_id);
+Debugger::ReportProgress(m_id, m_title, std::move(update), m_completed,
+ m_total, m_debugger_id);
   }
 }
Index: lldb/source/Core/DebuggerEvents.cpp
===
--- lldb/source/Core/DebuggerEvents.cpp
+++ lldb/source/Core/DebuggerEvents.cpp
@@ -33,7 +33,9 @@
 }
 
 void ProgressEventData::Dump(Stream *s) const {
-  s->Printf(" id = %" PRIu64 ", message = \"%s\"", m_id, m_message.c_str());
+  s->Printf(" id = %" PRIu64 ", title = \"%s\"", m_id, m_title.c_str());
+  if (!m_details.empty())
+s->Printf(", details = \"%s\"", m_details.c_str());
   if (m_completed == 0 || m_completed == m_total)
 s->Printf(", type = %s", m_completed == 0 ? "start" : "end");
   else
@@ -58,6 +60,8 @@
 return {};
 
   auto dictionary_sp = std::make_shared();
+  dictionary_sp->AddStringItem("title", progress_data->GetTitle());
+  dictionary_sp->AddStringItem("details", progress_data->GetDetails());
   dictionary_sp->AddStringItem("message", progress_data->GetMessage());
   dictionary_sp->AddIntegerItem("progress_id", progress_data->GetID());
   dictionary_sp->AddIntegerItem("completed", progress_data->GetCompleted());
Index: lldb/source/Core/Debugger.cpp
===
--- lldb/source/Core/Debugger.cpp
+++ lldb/source/Core/Debugger.cpp
@@ -1286,7 +1286,7 @@
 }
 
 static void PrivateReportProgress(Debugger &debugger, uint64_t progress_id,
-  const std::string &message,
+  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.
@@ -1294,13 +1294,15 @@
   if (!debugger.GetBroadcaster().EventTypeHasListeners(event_type))
 return;
   EventSP event_sp(new Event(
-  event_type, new ProgressEventData(progress_id, message, completed, total,
-is_debugger_specific)));
+  event_type,
+  new ProgressEventData(progress_id, std::move(title), std::move(details),
+completed, total, is_debugger_specific)));
   debugger.GetBroadcaster().BroadcastEvent(event_sp);
 }
 
-void Debugger::ReportProgress(uint64_t progress_id, const std::string &message,
-  uint64_t completed, uint64_t total,
+void Debugger::ReportProgress(uint64_t progress_id, std::string title,
+  std::string details, uint64_t completed,
+  uint64_t total,
   std::optional debugger_id) {
   // Check if this progress is for a specific debugger.
   if (debugger_id) {
@@ -1308,8 +1310,9 @@
 // still exists.
 DebuggerSP debugger_sp = FindDebuggerWithID(*debugger_id);
 if (debugger_sp)
-  PrivateReportProgress(*debugger_sp, progress_id, message, completed,
-total, /*is_debugger_specific*/ true);
+  PrivateReportProgress(*debugger_sp, progress_id, std::move(title),
+std::move(details), completed, total,
+/*is_debugger_specific*/ true);
 return;
   }
   // The progress event is no