[Lldb-commits] [PATCH] D154617: [lldb][DebugNamesDWARF] Also use mangled name when matching regex

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added a comment.

In D154617#4478369 , @JDevlieghere 
wrote:

> Is the reason this is necessary because the apple accelerator tables would 
> emit an entry for both the mangled and demangled name?

Not quite. The name inside the accelerator table (Apple or DWARF 5) is always 
mangled.
So if we want a user to be able to do `frame var --regex A::`, the only way 
this can possibly work is if we demangle the table entry prior to matching 
against the regex. This is what the test mentioned in the commit message is 
testing.

In the implementation of `AppleDWARFIndex.cpp`, we also do this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154617

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

[Lldb-commits] [PATCH] D154549: [lldb][NFCI] Remove use of Stream * from TypeSystem

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve accepted this revision.
fdeazeve added a comment.

LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154549

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

[Lldb-commits] [PATCH] D154617: [lldb][DebugNamesDWARF] Also use mangled name when matching regex

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added a comment.



> Cool, might be worth mentioning that in the commit message. LGMT.

Agreed! I'll update it prior to merging


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154617

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

[Lldb-commits] [PATCH] D154610: [lldb][NFC] Remove unused parameter from DebugNames::ProcessEntry

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG2c3bb6a59027: [lldb][NFC] Remove unused parameter from 
DebugNames::ProcessEntry (authored by fdeazeve).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154610

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.h


Index: lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.h
@@ -80,8 +80,7 @@
 
   std::optional ToDIERef(const DebugNames::Entry &entry);
   bool ProcessEntry(const DebugNames::Entry &entry,
-llvm::function_ref callback,
-llvm::StringRef name);
+llvm::function_ref callback);
 
   static void MaybeLogLookupError(llvm::Error error,
   const DebugNames::NameIndex &ni,
Index: lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
@@ -62,7 +62,7 @@
 
 bool DebugNamesDWARFIndex::ProcessEntry(
 const DebugNames::Entry &entry,
-llvm::function_ref callback, llvm::StringRef name) {
+llvm::function_ref callback) {
   std::optional ref = ToDIERef(entry);
   if (!ref)
 return true;
@@ -92,7 +92,7 @@
 if (entry.tag() != DW_TAG_variable)
   continue;
 
-if (!ProcessEntry(entry, callback, basename.GetStringRef()))
+if (!ProcessEntry(entry, callback))
   return;
   }
 
@@ -113,8 +113,7 @@
 if (entry_or->tag() != DW_TAG_variable)
   continue;
 
-if (!ProcessEntry(*entry_or, callback,
-  llvm::StringRef(nte.getString(
+if (!ProcessEntry(*entry_or, callback))
   return;
   }
   MaybeLogLookupError(entry_or.takeError(), ni, nte.getString());
@@ -139,8 +138,7 @@
   continue;
 
 found_entry_for_cu = true;
-if (!ProcessEntry(*entry_or, callback,
-  llvm::StringRef(nte.getString(
+if (!ProcessEntry(*entry_or, callback))
   return;
   }
   MaybeLogLookupError(entry_or.takeError(), ni, nte.getString());
@@ -202,7 +200,7 @@
   for (const DebugNames::Entry &entry :
m_debug_names_up->equal_range(name.GetStringRef())) {
 if (isType(entry.tag())) {
-  if (!ProcessEntry(entry, callback, name.GetStringRef()))
+  if (!ProcessEntry(entry, callback))
 return;
 }
   }
@@ -216,7 +214,7 @@
   auto name = context[0].name;
   for (const DebugNames::Entry &entry : m_debug_names_up->equal_range(name)) {
 if (entry.tag() == context[0].tag) {
-  if (!ProcessEntry(entry, callback, name))
+  if (!ProcessEntry(entry, callback))
 return;
 }
   }
@@ -229,7 +227,7 @@
   for (const DebugNames::Entry &entry :
m_debug_names_up->equal_range(name.GetStringRef())) {
 if (entry.tag() == DW_TAG_namespace) {
-  if (!ProcessEntry(entry, callback, name.GetStringRef()))
+  if (!ProcessEntry(entry, callback))
 return;
 }
   }
@@ -278,8 +276,7 @@
 if (tag != DW_TAG_subprogram && tag != DW_TAG_inlined_subroutine)
   continue;
 
-if (!ProcessEntry(*entry_or, callback,
-  llvm::StringRef(nte.getString(
+if (!ProcessEntry(*entry_or, callback))
   return;
   }
   MaybeLogLookupError(entry_or.takeError(), ni, nte.getString());


Index: lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.h
@@ -80,8 +80,7 @@
 
   std::optional ToDIERef(const DebugNames::Entry &entry);
   bool ProcessEntry(const DebugNames::Entry &entry,
-llvm::function_ref callback,
-llvm::StringRef name);
+llvm::function_ref callback);
 
   static void MaybeLogLookupError(llvm::Error error,
   const DebugNames::NameIndex &ni,
Index: lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
@@ -62,7 +62,7 @@
 
 bool DebugNamesDWARFIndex::ProcessEntry(
 const DebugNames::Entry &entry,
-llvm::function_ref callback, llvm::StringRef name) {
+llvm::function_ref callback) {
   std::optional ref = ToDIERef(entry);
 

[Lldb-commits] [PATCH] D154617: [lldb][DebugNamesDWARF] Also use mangled name when matching regex

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG032de3ee0d35: [lldb][DebugNamesDWARF] Also use mangled name 
when matching regex (authored by fdeazeve).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154617

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


Index: lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
@@ -104,7 +104,8 @@
 llvm::function_ref callback) {
   for (const DebugNames::NameIndex &ni: *m_debug_names_up) {
 for (DebugNames::NameTableEntry nte: ni) {
-  if (!regex.Execute(nte.getString()))
+  Mangled mangled_name(nte.getString());
+  if (!mangled_name.NameMatches(regex))
 continue;
 
   uint64_t entry_offset = nte.getEntryOffset();


Index: lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
@@ -104,7 +104,8 @@
 llvm::function_ref callback) {
   for (const DebugNames::NameIndex &ni: *m_debug_names_up) {
 for (DebugNames::NameTableEntry nte: ni) {
-  if (!regex.Execute(nte.getString()))
+  Mangled mangled_name(nte.getString());
+  if (!mangled_name.NameMatches(regex))
 continue;
 
   uint64_t entry_offset = nte.getEntryOffset();
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D154513: [lldb][NFC] Factor out code from SymbolFileDWARF::ParseVariableDIE

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGdc04b18ad7e4: [lldb][NFC] Factor out code from 
SymbolFileDWARF::ParseVariableDIE (authored by fdeazeve).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154513

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

Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -3269,6 +3269,60 @@
   return var_sp;
 }
 
+/// Creates a DWARFExpressionList from an DW_AT_location form_value.
+static DWARFExpressionList GetExprListFromAtLocation(DWARFFormValue form_value,
+ ModuleSP module,
+ const DWARFDIE &die,
+ const addr_t func_low_pc) {
+  if (DWARFFormValue::IsBlockForm(form_value.Form())) {
+const DWARFDataExtractor &data = die.GetData();
+
+uint32_t block_offset = form_value.BlockData() - data.GetDataStart();
+uint32_t block_length = form_value.Unsigned();
+return DWARFExpressionList(
+module, DataExtractor(data, block_offset, block_length), die.GetCU());
+  }
+
+  DWARFExpressionList location_list(module, DWARFExpression(), die.GetCU());
+  DataExtractor data = die.GetCU()->GetLocationData();
+  dw_offset_t offset = form_value.Unsigned();
+  if (form_value.Form() == DW_FORM_loclistx)
+offset = die.GetCU()->GetLoclistOffset(offset).value_or(-1);
+  if (data.ValidOffset(offset)) {
+data = DataExtractor(data, offset, data.GetByteSize() - offset);
+const DWARFUnit *dwarf_cu = form_value.GetUnit();
+if (DWARFExpression::ParseDWARFLocationList(dwarf_cu, data, &location_list))
+  location_list.SetFuncFileAddress(func_low_pc);
+  }
+
+  return location_list;
+}
+
+/// Creates a DWARFExpressionList from an DW_AT_const_value. This is either a
+/// block form, or a string, or a data form. For data forms, this returns an
+/// empty list, as we cannot initialize it properly without a SymbolFileType.
+static DWARFExpressionList
+GetExprListFromAtConstValue(DWARFFormValue form_value, ModuleSP module,
+const DWARFDIE &die) {
+  const DWARFDataExtractor &debug_info_data = die.GetData();
+  if (DWARFFormValue::IsBlockForm(form_value.Form())) {
+// Retrieve the value as a block expression.
+uint32_t block_offset =
+form_value.BlockData() - debug_info_data.GetDataStart();
+uint32_t block_length = form_value.Unsigned();
+return DWARFExpressionList(
+module, DataExtractor(debug_info_data, block_offset, block_length),
+die.GetCU());
+  }
+  if (const char *str = form_value.AsCString())
+return DWARFExpressionList(module,
+   DataExtractor(str, strlen(str) + 1,
+ die.GetCU()->GetByteOrder(),
+ die.GetCU()->GetAddressByteSize()),
+   die.GetCU());
+  return DWARFExpressionList(module, DWARFExpression(), die.GetCU());
+}
+
 VariableSP SymbolFileDWARF::ParseVariableDIE(const SymbolContext &sc,
  const DWARFDIE &die,
  const lldb::addr_t func_low_pc) {
@@ -3290,7 +3344,6 @@
   const char *mangled = nullptr;
   Declaration decl;
   DWARFFormValue type_die_form;
-  DWARFExpressionList location_list(module, DWARFExpression(), die.GetCU());
   bool is_external = false;
   bool is_artificial = false;
   DWARFFormValue const_value_form, location_form;
@@ -3355,57 +3408,16 @@
   // for static constexpr member variables -- DW_AT_const_value will be
   // present in the class declaration and DW_AT_location in the DIE defining
   // the member.
-  bool location_is_const_value_data = false;
-  bool has_explicit_location = location_form.IsValid();
-  bool use_type_size_for_value = false;
-  if (location_form.IsValid()) {
-if (DWARFFormValue::IsBlockForm(location_form.Form())) {
-  const DWARFDataExtractor &data = die.GetData();
-
-  uint32_t block_offset = location_form.BlockData() - data.GetDataStart();
-  uint32_t block_length = location_form.Unsigned();
-  location_list = DWARFExpressionList(
-  module, DataExtractor(data, block_offset, block_length), die.GetCU());
-} else {
-  DataExtractor data = die.GetCU()->GetLocationData();
-  dw_offset_t offset = location_form.Unsigned();
-  if (location_form.Form() == DW_FORM_loclistx)
-offset = die.GetCU()->GetLoclistOffset(offset).value_or(-1);
-  if (data.ValidOffset(offset)) {
-data = DataExtractor(data, offset, data.GetByteSize() - offset);
-const 

[Lldb-commits] [PATCH] D154513: [lldb][NFC] Factor out code from SymbolFileDWARF::ParseVariableDIE

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added a comment.

> There are some potential improvements we could make to this code

Yup, there's a lot I want to change here, but I am taking it slowly in the 
interest of making each change easier to verify


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154513

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

[Lldb-commits] [PATCH] D154730: [lldb] Consider TAG_imported_declaration in DebugNamesIndex

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

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

In order to recognize namespace aliases as a namespace, the
DW_TAG_imported_declaration has to be taken into account. The name of these DIEs
is already included in all accelerator tables as of D143397 
.

Two of the three Index classes already handle this correctly:

1. ManualDWARFIndex (as of D143398 )
2. AppleDWARFIndex works by default with D143397 
, since apple has a table

dedicated to namespaces.

This commit updates the third index class, DWARF 5's DebugNamesDWARFIndex.
As a result, it fixes the following test with DWARF 5:
commands/expression/namespace-alias/TestInlineNamespaceAlias.py


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D154730

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


Index: lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
@@ -227,7 +227,9 @@
 ConstString name, llvm::function_ref callback) {
   for (const DebugNames::Entry &entry :
m_debug_names_up->equal_range(name.GetStringRef())) {
-if (entry.tag() == DW_TAG_namespace) {
+dwarf::Tag entry_tag = entry.tag();
+if (entry_tag == DW_TAG_namespace ||
+entry_tag == DW_TAG_imported_declaration) {
   if (!ProcessEntry(entry, callback))
 return;
 }


Index: lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
@@ -227,7 +227,9 @@
 ConstString name, llvm::function_ref callback) {
   for (const DebugNames::Entry &entry :
m_debug_names_up->equal_range(name.GetStringRef())) {
-if (entry.tag() == DW_TAG_namespace) {
+dwarf::Tag entry_tag = entry.tag();
+if (entry_tag == DW_TAG_namespace ||
+entry_tag == DW_TAG_imported_declaration) {
   if (!ProcessEntry(entry, callback))
 return;
 }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D154730: [lldb] Consider TAG_imported_declaration in DebugNamesIndex

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGcec33fc87c0c: [lldb] Consider TAG_imported_declaration in 
DebugNamesIndex (authored by fdeazeve).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154730

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


Index: lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
@@ -227,7 +227,9 @@
 ConstString name, llvm::function_ref callback) {
   for (const DebugNames::Entry &entry :
m_debug_names_up->equal_range(name.GetStringRef())) {
-if (entry.tag() == DW_TAG_namespace) {
+dwarf::Tag entry_tag = entry.tag();
+if (entry_tag == DW_TAG_namespace ||
+entry_tag == DW_TAG_imported_declaration) {
   if (!ProcessEntry(entry, callback))
 return;
 }


Index: lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
@@ -227,7 +227,9 @@
 ConstString name, llvm::function_ref callback) {
   for (const DebugNames::Entry &entry :
m_debug_names_up->equal_range(name.GetStringRef())) {
-if (entry.tag() == DW_TAG_namespace) {
+dwarf::Tag entry_tag = entry.tag();
+if (entry_tag == DW_TAG_namespace ||
+entry_tag == DW_TAG_imported_declaration) {
   if (!ProcessEntry(entry, callback))
 return;
 }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D154757: [lldb][NFCI] TestEmulation should take a Stream ref

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve accepted this revision.
fdeazeve added a comment.

Thanks for finding those!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154757

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

[Lldb-commits] [PATCH] D154843: [lldb] Disable TestNamespaceLookup in DWARF 5 + dSYM setting

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

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

The ordering in which functions are presented to the expression evaluator in
this test setting triggers a known bug in LLDB.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D154843

Files:
  lldb/test/API/lang/cpp/namespace/TestNamespaceLookup.py


Index: lldb/test/API/lang/cpp/namespace/TestNamespaceLookup.py
===
--- lldb/test/API/lang/cpp/namespace/TestNamespaceLookup.py
+++ lldb/test/API/lang/cpp/namespace/TestNamespaceLookup.py
@@ -164,7 +164,11 @@
 
 # Continue to BP_file_scope at file scope
 self.runToBkpt("continue")
-self.expect_expr("func()", result_type="int", result_value="2")
+# FIXME: In DWARF 5 with dsyms, the ordering of functions is slightly
+# different, which also hits the same issues mentioned previously.
+if (configuration.dwarf_version <= 4 or
+self.getDebugInfo() == 'dwarf'):
+self.expect_expr("func()", result_type="int", result_value="2")
 
 # Continue to BP_ns_scope at ns scope
 self.runToBkpt("continue")


Index: lldb/test/API/lang/cpp/namespace/TestNamespaceLookup.py
===
--- lldb/test/API/lang/cpp/namespace/TestNamespaceLookup.py
+++ lldb/test/API/lang/cpp/namespace/TestNamespaceLookup.py
@@ -164,7 +164,11 @@
 
 # Continue to BP_file_scope at file scope
 self.runToBkpt("continue")
-self.expect_expr("func()", result_type="int", result_value="2")
+# FIXME: In DWARF 5 with dsyms, the ordering of functions is slightly
+# different, which also hits the same issues mentioned previously.
+if (configuration.dwarf_version <= 4 or
+self.getDebugInfo() == 'dwarf'):
+self.expect_expr("func()", result_type="int", result_value="2")
 
 # Continue to BP_ns_scope at ns scope
 self.runToBkpt("continue")
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D154843: [lldb] Disable TestNamespaceLookup in DWARF 5 + dSYM setting

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG6890ad3f41c5: [lldb] Disable TestNamespaceLookup in DWARF 5 
+ dSYM setting (authored by fdeazeve).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154843

Files:
  lldb/test/API/lang/cpp/namespace/TestNamespaceLookup.py


Index: lldb/test/API/lang/cpp/namespace/TestNamespaceLookup.py
===
--- lldb/test/API/lang/cpp/namespace/TestNamespaceLookup.py
+++ lldb/test/API/lang/cpp/namespace/TestNamespaceLookup.py
@@ -164,7 +164,11 @@
 
 # Continue to BP_file_scope at file scope
 self.runToBkpt("continue")
-self.expect_expr("func()", result_type="int", result_value="2")
+# FIXME: In DWARF 5 with dsyms, the ordering of functions is slightly
+# different, which also hits the same issues mentioned previously.
+if (configuration.dwarf_version <= 4 or
+self.getDebugInfo() == 'dwarf'):
+self.expect_expr("func()", result_type="int", result_value="2")
 
 # Continue to BP_ns_scope at ns scope
 self.runToBkpt("continue")


Index: lldb/test/API/lang/cpp/namespace/TestNamespaceLookup.py
===
--- lldb/test/API/lang/cpp/namespace/TestNamespaceLookup.py
+++ lldb/test/API/lang/cpp/namespace/TestNamespaceLookup.py
@@ -164,7 +164,11 @@
 
 # Continue to BP_file_scope at file scope
 self.runToBkpt("continue")
-self.expect_expr("func()", result_type="int", result_value="2")
+# FIXME: In DWARF 5 with dsyms, the ordering of functions is slightly
+# different, which also hits the same issues mentioned previously.
+if (configuration.dwarf_version <= 4 or
+self.getDebugInfo() == 'dwarf'):
+self.expect_expr("func()", result_type="int", result_value="2")
 
 # Continue to BP_ns_scope at ns scope
 self.runToBkpt("continue")
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D155100: [lldb][NFC] Factor out code linking OSO addr of uninitialized GVs

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve created this revision.
Herald added a project: All.
fdeazeve requested review of this revision.
Herald added subscribers: lldb-commits, wangpc.
Herald added a project: LLDB.

This code was introduced in 2fc93eabf7e132abd51d0ea0ad599beb3fa44334.
In order to improve readability of ParseVariableDIE, we move this code into a
helper function. The issue this code attempted to address was fixed between
Clangs 9 and 11; as such, if we ever want to delete this code, it is a lot
easier to do so after the refactor.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D155100

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


Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -3323,6 +3323,38 @@
   return DWARFExpressionList(module, DWARFExpression(), die.GetCU());
 }
 
+/// Global variables that are not initialized may have their address set to
+/// zero. Since multiple variables may have this address, we cannot apply the
+/// OSO relink address approach we normally use.
+/// However, the executable will have a matching symbol with a good address;
+/// this function attempts to find the correct address by looking into the
+/// executable's symbol table. If it succeeds, the expr_list is updated with
+/// the new address and the executable's symbol is returned.
+static Symbol *fixupExternalAddrZeroVariable(
+SymbolFileDWARFDebugMap &debug_map_symfile, llvm::StringRef name,
+DWARFExpressionList &expr_list, const DWARFDIE &die) {
+  ObjectFile *debug_map_objfile = debug_map_symfile.GetObjectFile();
+  if (!debug_map_objfile)
+return nullptr;
+
+  Symtab *debug_map_symtab = debug_map_objfile->GetSymtab();
+  if (!debug_map_symtab)
+return nullptr;
+  Symbol *exe_symbol = debug_map_symtab->FindFirstSymbolWithNameAndType(
+  ConstString(name), eSymbolTypeData, Symtab::eDebugYes,
+  Symtab::eVisibilityExtern);
+  if (!exe_symbol || !exe_symbol->ValueIsAddress())
+return nullptr;
+  const addr_t exe_file_addr = exe_symbol->GetAddressRef().GetFileAddress();
+  if (exe_file_addr == LLDB_INVALID_ADDRESS)
+return nullptr;
+
+  DWARFExpression *location = expr_list.GetMutableExpressionAtAddress();
+  if (location->Update_DW_OP_addr(die.GetCU(), exe_file_addr))
+return exe_symbol;
+  return nullptr;
+}
+
 VariableSP SymbolFileDWARF::ParseVariableDIE(const SymbolContext &sc,
  const DWARFDIE &die,
  const lldb::addr_t func_low_pc) {
@@ -3496,49 +3528,14 @@
 scope = eValueTypeVariableStatic;
 
   if (debug_map_symfile) {
-// When leaving the DWARF in the .o files on darwin, when we have a
-// global variable that wasn't initialized, the .o file might not
-// have allocated a virtual address for the global variable. In
-// this case it will have created a symbol for the global variable
-// that is undefined/data and external and the value will be the
-// byte size of the variable. When we do the address map in
-// SymbolFileDWARFDebugMap we rely on having an address, we need to
-// do some magic here so we can get the correct address for our
-// global variable. The address for all of these entries will be
-// zero, and there will be an undefined symbol in this object file,
-// and the executable will have a matching symbol with a good
-// address. So here we dig up the correct address and replace it in
-// the location for the variable, and set the variable's symbol
-// context scope to be that of the main executable so the file
-// address will resolve correctly.
 bool linked_oso_file_addr = false;
+
 if (is_external && location_DW_OP_addr == 0) {
-  // we have a possible uninitialized extern global
-  ConstString const_name(mangled ? mangled : name);
-  ObjectFile *debug_map_objfile = debug_map_symfile->GetObjectFile();
-  if (debug_map_objfile) {
-Symtab *debug_map_symtab = debug_map_objfile->GetSymtab();
-if (debug_map_symtab) {
-  Symbol *exe_symbol =
-  debug_map_symtab->FindFirstSymbolWithNameAndType(
-  const_name, eSymbolTypeData, Symtab::eDebugYes,
-  Symtab::eVisibilityExtern);
-  if (exe_symbol) {
-if (exe_symbol->ValueIsAddress()) {
-  const addr_t exe_file_addr =
-  exe_symbol->GetAddressRef().GetFileAddress();
-  if (exe_file_addr != LLDB_INVALID_ADDRESS) {
-DWARFExpression *location =
-location_list.GetMutableExpressionAtAddress();
-if (location->

[Lldb-commits] [PATCH] D155100: [lldb][NFC] Factor out code linking OSO addr of uninitialized GVs

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG85c2e420c87c: [lldb][NFC] Factor out code linking OSO addr 
of uninitialized GVs (authored by fdeazeve).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155100

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


Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -3323,6 +3323,38 @@
   return DWARFExpressionList(module, DWARFExpression(), die.GetCU());
 }
 
+/// Global variables that are not initialized may have their address set to
+/// zero. Since multiple variables may have this address, we cannot apply the
+/// OSO relink address approach we normally use.
+/// However, the executable will have a matching symbol with a good address;
+/// this function attempts to find the correct address by looking into the
+/// executable's symbol table. If it succeeds, the expr_list is updated with
+/// the new address and the executable's symbol is returned.
+static Symbol *fixupExternalAddrZeroVariable(
+SymbolFileDWARFDebugMap &debug_map_symfile, llvm::StringRef name,
+DWARFExpressionList &expr_list, const DWARFDIE &die) {
+  ObjectFile *debug_map_objfile = debug_map_symfile.GetObjectFile();
+  if (!debug_map_objfile)
+return nullptr;
+
+  Symtab *debug_map_symtab = debug_map_objfile->GetSymtab();
+  if (!debug_map_symtab)
+return nullptr;
+  Symbol *exe_symbol = debug_map_symtab->FindFirstSymbolWithNameAndType(
+  ConstString(name), eSymbolTypeData, Symtab::eDebugYes,
+  Symtab::eVisibilityExtern);
+  if (!exe_symbol || !exe_symbol->ValueIsAddress())
+return nullptr;
+  const addr_t exe_file_addr = exe_symbol->GetAddressRef().GetFileAddress();
+  if (exe_file_addr == LLDB_INVALID_ADDRESS)
+return nullptr;
+
+  DWARFExpression *location = expr_list.GetMutableExpressionAtAddress();
+  if (location->Update_DW_OP_addr(die.GetCU(), exe_file_addr))
+return exe_symbol;
+  return nullptr;
+}
+
 VariableSP SymbolFileDWARF::ParseVariableDIE(const SymbolContext &sc,
  const DWARFDIE &die,
  const lldb::addr_t func_low_pc) {
@@ -3496,49 +3528,14 @@
 scope = eValueTypeVariableStatic;
 
   if (debug_map_symfile) {
-// When leaving the DWARF in the .o files on darwin, when we have a
-// global variable that wasn't initialized, the .o file might not
-// have allocated a virtual address for the global variable. In
-// this case it will have created a symbol for the global variable
-// that is undefined/data and external and the value will be the
-// byte size of the variable. When we do the address map in
-// SymbolFileDWARFDebugMap we rely on having an address, we need to
-// do some magic here so we can get the correct address for our
-// global variable. The address for all of these entries will be
-// zero, and there will be an undefined symbol in this object file,
-// and the executable will have a matching symbol with a good
-// address. So here we dig up the correct address and replace it in
-// the location for the variable, and set the variable's symbol
-// context scope to be that of the main executable so the file
-// address will resolve correctly.
 bool linked_oso_file_addr = false;
+
 if (is_external && location_DW_OP_addr == 0) {
-  // we have a possible uninitialized extern global
-  ConstString const_name(mangled ? mangled : name);
-  ObjectFile *debug_map_objfile = debug_map_symfile->GetObjectFile();
-  if (debug_map_objfile) {
-Symtab *debug_map_symtab = debug_map_objfile->GetSymtab();
-if (debug_map_symtab) {
-  Symbol *exe_symbol =
-  debug_map_symtab->FindFirstSymbolWithNameAndType(
-  const_name, eSymbolTypeData, Symtab::eDebugYes,
-  Symtab::eVisibilityExtern);
-  if (exe_symbol) {
-if (exe_symbol->ValueIsAddress()) {
-  const addr_t exe_file_addr =
-  exe_symbol->GetAddressRef().GetFileAddress();
-  if (exe_file_addr != LLDB_INVALID_ADDRESS) {
-DWARFExpression *location =
-location_list.GetMutableExpressionAtAddress();
-if (location->Update_DW_OP_addr(die.GetCU(),
-exe_file_addr)) {
-  linked_oso_file_addr = true;
-  symbol_context_scope = exe_symbol;
-}
-  }

[Lldb-commits] [PATCH] D153891: [lldb] Consider OP_addrx in DWARFExpression::Update_DW_OP_addr

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve abandoned this revision.
fdeazeve added a comment.

Now that we have agreement on the discourse thread, I'll close this, polish it, 
and post again with tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153891

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

[Lldb-commits] [PATCH] D153886: [DRAFT][lldb] Consider OP_addrx in DWARFExpression::Update_DW_OP_addr

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve abandoned this revision.
fdeazeve added a comment.

We're going with the other solution. See discourse thread linked


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153886

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

[Lldb-commits] [PATCH] D155197: [lldb][NFC] Refactor test to enable subsequent reuse

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

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

On a subsequent commit, I intend to update the expression and call the evaluate
function more times. This refactors enables reusing some of the existing code
for that.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D155197

Files:
  lldb/unittests/Expression/DWARFExpressionTest.cpp


Index: lldb/unittests/Expression/DWARFExpressionTest.cpp
===
--- lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -500,19 +500,26 @@
 platform_sp, target_sp);
 
   ExecutionContext exe_ctx(target_sp, false);
+
+  auto evaluate = [&](DWARFExpression &expr, Status &status, Value &result) {
+DataExtractor extractor;
+expr.GetExpressionData(extractor);
+return DWARFExpression::Evaluate(
+&exe_ctx, /*reg_ctx*/ nullptr, /*module_sp*/ {}, extractor, dwarf_cu,
+lldb::eRegisterKindLLDB,
+/*initial_value_ptr*/ nullptr,
+/*object_address_ptr*/ nullptr, result, &status);
+  };
+
   // DW_OP_addrx takes a single leb128 operand, the index in the addr table:
-  uint8_t expr[] = {DW_OP_addrx, 0x01};
-  DataExtractor extractor(expr, sizeof(expr), lldb::eByteOrderLittle,
+  uint8_t expr_data[] = {DW_OP_addrx, 0x01};
+  DataExtractor extractor(expr_data, sizeof(expr_data), lldb::eByteOrderLittle,
   /*addr_size*/ 4);
-  Value result;
-  Status status;
-  ASSERT_TRUE(DWARFExpression::Evaluate(
-  &exe_ctx, /*reg_ctx*/ nullptr, /*module_sp*/ {}, extractor, dwarf_cu,
-  lldb::eRegisterKindLLDB,
-  /*initial_value_ptr*/ nullptr,
-  /*object_address_ptr*/ nullptr, result, &status))
-  << status.ToError();
+  DWARFExpression expr(extractor);
 
+  Status status;
+  Value result;
+  ASSERT_TRUE(evaluate(expr, status, result)) << status.ToError();
   ASSERT_EQ(result.GetValueType(), Value::ValueType::LoadAddress);
   ASSERT_EQ(result.GetScalar().UInt(), 0x5678u);
 }


Index: lldb/unittests/Expression/DWARFExpressionTest.cpp
===
--- lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -500,19 +500,26 @@
 platform_sp, target_sp);
 
   ExecutionContext exe_ctx(target_sp, false);
+
+  auto evaluate = [&](DWARFExpression &expr, Status &status, Value &result) {
+DataExtractor extractor;
+expr.GetExpressionData(extractor);
+return DWARFExpression::Evaluate(
+&exe_ctx, /*reg_ctx*/ nullptr, /*module_sp*/ {}, extractor, dwarf_cu,
+lldb::eRegisterKindLLDB,
+/*initial_value_ptr*/ nullptr,
+/*object_address_ptr*/ nullptr, result, &status);
+  };
+
   // DW_OP_addrx takes a single leb128 operand, the index in the addr table:
-  uint8_t expr[] = {DW_OP_addrx, 0x01};
-  DataExtractor extractor(expr, sizeof(expr), lldb::eByteOrderLittle,
+  uint8_t expr_data[] = {DW_OP_addrx, 0x01};
+  DataExtractor extractor(expr_data, sizeof(expr_data), lldb::eByteOrderLittle,
   /*addr_size*/ 4);
-  Value result;
-  Status status;
-  ASSERT_TRUE(DWARFExpression::Evaluate(
-  &exe_ctx, /*reg_ctx*/ nullptr, /*module_sp*/ {}, extractor, dwarf_cu,
-  lldb::eRegisterKindLLDB,
-  /*initial_value_ptr*/ nullptr,
-  /*object_address_ptr*/ nullptr, result, &status))
-  << status.ToError();
+  DWARFExpression expr(extractor);
 
+  Status status;
+  Value result;
+  ASSERT_TRUE(evaluate(expr, status, result)) << status.ToError();
   ASSERT_EQ(result.GetValueType(), Value::ValueType::LoadAddress);
   ASSERT_EQ(result.GetScalar().UInt(), 0x5678u);
 }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D155198: [lldb] Consider OP_addrx in DWARFExpression::Update_DW_OP_addr

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

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

This rewrites DW_OP_addrx inside DWARFExpression as an DW_OP_addr so that we can
update addresses that are originally located in the debug_addr section.

The full discussion behind this can be found in
https://discourse.llvm.org/t/dwarfexpression-and-dw-op-addrx/71627/12, but a
summary follows.

When SymbolFileDWARF::ParseVariableDIE creates DWARFExpressions for variables
whose location is an OP_addr, it knows how to remap addresses appropriately in
the DebugMap case. It then calls DWARFExpression::Update_DW_OP_addr, which
updates the value associated with OP_addr.

However, when we have an OP_addrx, the update function does nothing. This makes
sense, as the DWARFExpression does not have a mutable view of the debug_addr
section. In non-DebugMap flows this is not an issue, as the debug_addr contains
the correct addresses of variables. In DebugMap flows, this is problematic
because the work done by RelinkOSOAddress is lost. By updating the OP to
OP_addr, we can also update the address as required,

We also explored the alternative of relinking the debug_addr section when we are
initializing OSOs (InitOSO). However, this creates an inconsistent story for
users of DWARFExpression::GetLocation_DW_OP_addr. This function returns an
address without telling callers whether that address came from an OP_addr or an
OP_addrx. If we preemptively relink OP_addrx results without doing the same for
OP_addr results, then callers can’t know whether the address they got was an
executable address or an object file address. In other words, they can’t know
whether they need to call LinkOSOFileAddress on those results or not.

This patch addresses the majority of test failures when enabling DWARF 5 for
MachO (over 200 test failures).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D155198

Files:
  lldb/source/Expression/DWARFExpression.cpp
  lldb/unittests/Expression/DWARFExpressionTest.cpp


Index: lldb/unittests/Expression/DWARFExpressionTest.cpp
===
--- lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -522,6 +522,11 @@
   ASSERT_TRUE(evaluate(expr, status, result)) << status.ToError();
   ASSERT_EQ(result.GetValueType(), Value::ValueType::LoadAddress);
   ASSERT_EQ(result.GetScalar().UInt(), 0x5678u);
+
+  ASSERT_TRUE(expr.Update_DW_OP_addr(dwarf_cu, 0xdeadbeef));
+  ASSERT_TRUE(evaluate(expr, status, result)) << status.ToError();
+  ASSERT_EQ(result.GetValueType(), Value::ValueType::LoadAddress);
+  ASSERT_EQ(result.GetScalar().UInt(), 0xdeadbeefu);
 }
 
 class CustomSymbolFileDWARF : public SymbolFileDWARF {
Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -408,13 +408,32 @@
   // the heap data so "m_data" will now correctly manage the heap data.
   m_data.SetData(encoder.GetDataBuffer());
   return true;
-} else {
-  const offset_t op_arg_size =
-  GetOpcodeDataSize(m_data, offset, op, dwarf_cu);
-  if (op_arg_size == LLDB_INVALID_OFFSET)
-break;
-  offset += op_arg_size;
 }
+if (op == DW_OP_addrx) {
+  // Replace DW_OP_addrx with DW_OP_addr, since we can't modify the
+  // read-only debug_addr table.
+  llvm::ArrayRef data_before_op = m_data.GetData().take_front(offset - 1);
+
+  // Read the addrx index to determine how many bytes it needs.
+  const lldb::offset_t old_offset = offset;
+  m_data.GetULEB128(&offset);
+  if (old_offset == offset)
+return false;
+  llvm::ArrayRef data_after_op = m_data.GetData().drop_front(offset);
+
+  DataEncoder encoder(m_data.GetByteOrder(), m_data.GetAddressByteSize());
+  encoder.AppendData(data_before_op);
+  encoder.AppendU8(DW_OP_addr);
+  encoder.AppendAddress(file_addr);
+  encoder.AppendData(data_after_op);
+  m_data.SetData(encoder.GetDataBuffer());
+  return true;
+}
+const offset_t op_arg_size =
+GetOpcodeDataSize(m_data, offset, op, dwarf_cu);
+if (op_arg_size == LLDB_INVALID_OFFSET)
+  break;
+offset += op_arg_size;
   }
   return false;
 }


Index: lldb/unittests/Expression/DWARFExpressionTest.cpp
===
--- lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -522,6 +522,11 @@
   ASSERT_TRUE(evaluate(expr, status, result)) << status.ToError();
   ASSERT_EQ(result.GetValueType(), Value::ValueType::LoadAddress);
   ASSERT_EQ(result.GetScalar().UInt(), 0x5678u);
+
+  ASSERT_TRUE(expr.Update_DW_OP_addr(dwarf_cu, 0xdeadbeef));
+  ASSERT_TRUE(evalu

[Lldb-commits] [PATCH] D155198: [lldb] Consider OP_addrx in DWARFExpression::Update_DW_OP_addr

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve updated this revision to Diff 540001.
fdeazeve added a comment.

Minor comment improvements


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155198

Files:
  lldb/source/Expression/DWARFExpression.cpp
  lldb/unittests/Expression/DWARFExpressionTest.cpp


Index: lldb/unittests/Expression/DWARFExpressionTest.cpp
===
--- lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -522,6 +522,11 @@
   ASSERT_TRUE(evaluate(expr, status, result)) << status.ToError();
   ASSERT_EQ(result.GetValueType(), Value::ValueType::LoadAddress);
   ASSERT_EQ(result.GetScalar().UInt(), 0x5678u);
+
+  ASSERT_TRUE(expr.Update_DW_OP_addr(dwarf_cu, 0xdeadbeef));
+  ASSERT_TRUE(evaluate(expr, status, result)) << status.ToError();
+  ASSERT_EQ(result.GetValueType(), Value::ValueType::LoadAddress);
+  ASSERT_EQ(result.GetScalar().UInt(), 0xdeadbeefu);
 }
 
 class CustomSymbolFileDWARF : public SymbolFileDWARF {
Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -408,13 +408,33 @@
   // the heap data so "m_data" will now correctly manage the heap data.
   m_data.SetData(encoder.GetDataBuffer());
   return true;
-} else {
-  const offset_t op_arg_size =
-  GetOpcodeDataSize(m_data, offset, op, dwarf_cu);
-  if (op_arg_size == LLDB_INVALID_OFFSET)
-break;
-  offset += op_arg_size;
 }
+if (op == DW_OP_addrx) {
+  // Replace DW_OP_addrx with DW_OP_addr, since we can't modify the
+  // read-only debug_addr table.
+  // Subtract one to account for the opcode.
+  llvm::ArrayRef data_before_op = m_data.GetData().take_front(offset - 1);
+
+  // Read the addrx index to determine how many bytes it needs.
+  const lldb::offset_t old_offset = offset;
+  m_data.GetULEB128(&offset);
+  if (old_offset == offset)
+return false;
+  llvm::ArrayRef data_after_op = m_data.GetData().drop_front(offset);
+
+  DataEncoder encoder(m_data.GetByteOrder(), m_data.GetAddressByteSize());
+  encoder.AppendData(data_before_op);
+  encoder.AppendU8(DW_OP_addr);
+  encoder.AppendAddress(file_addr);
+  encoder.AppendData(data_after_op);
+  m_data.SetData(encoder.GetDataBuffer());
+  return true;
+}
+const offset_t op_arg_size =
+GetOpcodeDataSize(m_data, offset, op, dwarf_cu);
+if (op_arg_size == LLDB_INVALID_OFFSET)
+  break;
+offset += op_arg_size;
   }
   return false;
 }


Index: lldb/unittests/Expression/DWARFExpressionTest.cpp
===
--- lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -522,6 +522,11 @@
   ASSERT_TRUE(evaluate(expr, status, result)) << status.ToError();
   ASSERT_EQ(result.GetValueType(), Value::ValueType::LoadAddress);
   ASSERT_EQ(result.GetScalar().UInt(), 0x5678u);
+
+  ASSERT_TRUE(expr.Update_DW_OP_addr(dwarf_cu, 0xdeadbeef));
+  ASSERT_TRUE(evaluate(expr, status, result)) << status.ToError();
+  ASSERT_EQ(result.GetValueType(), Value::ValueType::LoadAddress);
+  ASSERT_EQ(result.GetScalar().UInt(), 0xdeadbeefu);
 }
 
 class CustomSymbolFileDWARF : public SymbolFileDWARF {
Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -408,13 +408,33 @@
   // the heap data so "m_data" will now correctly manage the heap data.
   m_data.SetData(encoder.GetDataBuffer());
   return true;
-} else {
-  const offset_t op_arg_size =
-  GetOpcodeDataSize(m_data, offset, op, dwarf_cu);
-  if (op_arg_size == LLDB_INVALID_OFFSET)
-break;
-  offset += op_arg_size;
 }
+if (op == DW_OP_addrx) {
+  // Replace DW_OP_addrx with DW_OP_addr, since we can't modify the
+  // read-only debug_addr table.
+  // Subtract one to account for the opcode.
+  llvm::ArrayRef data_before_op = m_data.GetData().take_front(offset - 1);
+
+  // Read the addrx index to determine how many bytes it needs.
+  const lldb::offset_t old_offset = offset;
+  m_data.GetULEB128(&offset);
+  if (old_offset == offset)
+return false;
+  llvm::ArrayRef data_after_op = m_data.GetData().drop_front(offset);
+
+  DataEncoder encoder(m_data.GetByteOrder(), m_data.GetAddressByteSize());
+  encoder.AppendData(data_before_op);
+  encoder.AppendU8(DW_OP_addr);
+  encoder.AppendAddress(file_addr);
+  encoder.AppendData(data_after_op);
+  m_data.SetData(encoder.GetDataBuffer());
+  return true;
+}
+ 

[Lldb-commits] [PATCH] D155197: [lldb][NFC] Refactor test to enable subsequent reuse

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added a comment.

See the patch stack for the commit that benefits from this refactor.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155197

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

[Lldb-commits] [PATCH] D155178: [Support] Move StringExtras.h include from Error.h to Error.cpp (part 6)

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added a comment.

In D155178#4496936 , @IncludeGuardian 
wrote:

> @MaskRay seems like `ninja` on its own after preparing `lldb` isn't enough to 
> build tests. I've now built llvm, clang, flang, bolt, lldb, + tests - I'm 
> hoping that this is it now!

You can build everything required for all tests with the `check-*` targets, for 
example: `check-lldb`, `check-llvm`, `check-clang`.
There is also a `check-all`. Just make sure to kill the job after the 
compilation is done, otherwise you will be running a massive test suite ;)

To be concrete, you can do something like `ninja check-all`. Or also `cmake 
--build  --target check-all`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155178

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

[Lldb-commits] [PATCH] D155332: [lldb][NFC] Factor out CommandObject code filtering results based on scope

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

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

We will need this code in a subsequent commit.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D155332

Files:
  lldb/source/Commands/CommandObjectFrame.cpp


Index: lldb/source/Commands/CommandObjectFrame.cpp
===
--- lldb/source/Commands/CommandObjectFrame.cpp
+++ lldb/source/Commands/CommandObjectFrame.cpp
@@ -480,6 +480,25 @@
 return llvm::StringRef();
   }
 
+  /// Returns true if `scope` matches any of the options in 
`m_option_variable`.
+  bool ScopeRequested(lldb::ValueType scope) {
+switch (scope) {
+case eValueTypeVariableGlobal:
+case eValueTypeVariableStatic:
+  return m_option_variable.show_globals;
+case eValueTypeVariableArgument:
+  return m_option_variable.show_args;
+case eValueTypeVariableLocal:
+  return m_option_variable.show_locals;
+case eValueTypeInvalid:
+case eValueTypeRegister:
+case eValueTypeRegisterSet:
+case eValueTypeConstResult:
+case eValueTypeVariableThreadLocal:
+  return false;
+}
+  }
+
   bool DoExecute(Args &command, CommandReturnObject &result) override {
 // No need to check "frame" for validity as eCommandRequiresFrame ensures
 // it is valid
@@ -630,27 +649,8 @@
 if (num_variables > 0) {
   for (size_t i = 0; i < num_variables; i++) {
 var_sp = variable_list->GetVariableAtIndex(i);
-switch (var_sp->GetScope()) {
-case eValueTypeVariableGlobal:
-  if (!m_option_variable.show_globals)
-continue;
-  break;
-case eValueTypeVariableStatic:
-  if (!m_option_variable.show_globals)
+if (!ScopeRequested(var_sp->GetScope()))
 continue;
-  break;
-case eValueTypeVariableArgument:
-  if (!m_option_variable.show_args)
-continue;
-  break;
-case eValueTypeVariableLocal:
-  if (!m_option_variable.show_locals)
-continue;
-  break;
-default:
-  continue;
-  break;
-}
 std::string scope_string;
 if (m_option_variable.show_scope)
   scope_string = GetScopeString(var_sp).str();


Index: lldb/source/Commands/CommandObjectFrame.cpp
===
--- lldb/source/Commands/CommandObjectFrame.cpp
+++ lldb/source/Commands/CommandObjectFrame.cpp
@@ -480,6 +480,25 @@
 return llvm::StringRef();
   }
 
+  /// Returns true if `scope` matches any of the options in `m_option_variable`.
+  bool ScopeRequested(lldb::ValueType scope) {
+switch (scope) {
+case eValueTypeVariableGlobal:
+case eValueTypeVariableStatic:
+  return m_option_variable.show_globals;
+case eValueTypeVariableArgument:
+  return m_option_variable.show_args;
+case eValueTypeVariableLocal:
+  return m_option_variable.show_locals;
+case eValueTypeInvalid:
+case eValueTypeRegister:
+case eValueTypeRegisterSet:
+case eValueTypeConstResult:
+case eValueTypeVariableThreadLocal:
+  return false;
+}
+  }
+
   bool DoExecute(Args &command, CommandReturnObject &result) override {
 // No need to check "frame" for validity as eCommandRequiresFrame ensures
 // it is valid
@@ -630,27 +649,8 @@
 if (num_variables > 0) {
   for (size_t i = 0; i < num_variables; i++) {
 var_sp = variable_list->GetVariableAtIndex(i);
-switch (var_sp->GetScope()) {
-case eValueTypeVariableGlobal:
-  if (!m_option_variable.show_globals)
-continue;
-  break;
-case eValueTypeVariableStatic:
-  if (!m_option_variable.show_globals)
+if (!ScopeRequested(var_sp->GetScope()))
 continue;
-  break;
-case eValueTypeVariableArgument:
-  if (!m_option_variable.show_args)
-continue;
-  break;
-case eValueTypeVariableLocal:
-  if (!m_option_variable.show_locals)
-continue;
-  break;
-default:
-  continue;
-  break;
-}
 std::string scope_string;
 if (m_option_variable.show_scope)
   scope_string = GetScopeString(var_sp).str();
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D155334: [lldb] Make frame var --regex always search globals

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

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

Currently frame var --regex sometimes searches globals, sometimes it doesn't.
This happens because `StackFrame::GetVariableList` always returns the biggest
list it has, regardless of whether only globals were requested or not. In other
words, if a previous call to `GetVariableList` requested globals, all subsequent
calls will see them.

The implication here is that users of `StackFrame::GetVariableList` are expected
to filter the results of this function. This is what we do for a vanilla
`frame var` command. But it is not what we do when `--regex` is used. This
commit solves the issue by:

1. Making `--regex` imply `--globals`. This matches the behavior of `frame var

`, which will also search the global scope.

2. Making the `--regex` search respect the command object options.

See the added test for an example of the oddities this patch addresses. Without
the patch, the test fails. However it could be made to pass by calling a plain
`frame var` before calling `frame var --regex A::`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D155334

Files:
  lldb/include/lldb/Symbol/VariableList.h
  lldb/source/Commands/CommandObjectFrame.cpp
  lldb/test/API/commands/frame/var/TestFrameVar.py

Index: lldb/test/API/commands/frame/var/TestFrameVar.py
===
--- lldb/test/API/commands/frame/var/TestFrameVar.py
+++ lldb/test/API/commands/frame/var/TestFrameVar.py
@@ -58,6 +58,16 @@
 command_result = lldb.SBCommandReturnObject()
 interp = self.dbg.GetCommandInterpreter()
 
+# Ensure --regex can find globals if it is the very first frame var command.
+result = interp.HandleCommand("frame var --regex g_", command_result)
+self.assertEqual(
+result,
+lldb.eReturnStatusSuccessFinishResult,
+"frame var --regex g_ didn't succeed",
+)
+output = command_result.GetOutput()
+self.assertIn("g_var", output, "g_var not found with regex g_")
+
 # Just get args:
 result = interp.HandleCommand("frame var -l", command_result)
 self.assertEqual(
Index: lldb/source/Commands/CommandObjectFrame.cpp
===
--- lldb/source/Commands/CommandObjectFrame.cpp
+++ lldb/source/Commands/CommandObjectFrame.cpp
@@ -499,6 +499,31 @@
 }
   }
 
+  /// Finds all variables in `all_variables` whose name matches `regex`,
+  /// inserting them into `matches`. Variables already contained in `matches`
+  /// are not inserted again.
+  /// Nullopt is returned in case of no matches.
+  /// A sub-range of `matches` with all newly added variables is returned. This
+  /// may be empty if all matches were already contained in `matches`.
+  std::optional>
+  findUniqueRegexMatches(RegularExpression ®ex,
+ VariableList &matches,
+ const VariableList &all_variables) {
+bool any_matches = false;
+const size_t previous_num_vars = matches.GetSize();
+
+for (const VariableSP &var : all_variables) {
+  if (!var->NameMatches(regex) || !ScopeRequested(var->GetScope()))
+continue;
+  any_matches = true;
+  matches.AddVariableIfUnique(var);
+}
+
+if (any_matches)
+  return matches.toArrayRef().drop_front(previous_num_vars);
+return std::nullopt;
+  }
+
   bool DoExecute(Args &command, CommandReturnObject &result) override {
 // No need to check "frame" for validity as eCommandRequiresFrame ensures
 // it is valid
@@ -506,6 +531,10 @@
 
 Stream &s = result.GetOutputStream();
 
+// Using a regex should behave like looking for an exact name match: it
+// also finds globals.
+m_option_variable.show_globals |= m_option_variable.use_regex;
+
 // Be careful about the stack frame, if any summary formatter runs code, it
 // might clear the StackFrameList for the thread.  So hold onto a shared
 // pointer to the frame so it stays alive.
@@ -518,7 +547,6 @@
   result.AppendError(error.AsCString());
 
 }
-VariableSP var_sp;
 ValueObjectSP valobj_sp;
 
 TypeSummaryImplSP summary_format_sp;
@@ -551,46 +579,38 @@
 // objects from them...
 for (auto &entry : command) {
   if (m_option_variable.use_regex) {
-const size_t regex_start_index = regex_var_list.GetSize();
 llvm::StringRef name_str = entry.ref();
 RegularExpression regex(name_str);
 if (regex.IsValid()) {
-  size_t num_matches = 0;
-  const size_t num_new_regex_vars =
-  variable_list->AppendVariablesIfUnique(regex, regex_var_list,
- num_matches);
-  if (num_new_regex_vars 

[Lldb-commits] [PATCH] D155332: [lldb][NFC] Factor out CommandObject code filtering results based on scope

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG8bc97a3ee4f4: [lldb][NFC] Factor out CommandObject code 
filtering results based on scope (authored by fdeazeve).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155332

Files:
  lldb/source/Commands/CommandObjectFrame.cpp


Index: lldb/source/Commands/CommandObjectFrame.cpp
===
--- lldb/source/Commands/CommandObjectFrame.cpp
+++ lldb/source/Commands/CommandObjectFrame.cpp
@@ -480,6 +480,25 @@
 return llvm::StringRef();
   }
 
+  /// Returns true if `scope` matches any of the options in 
`m_option_variable`.
+  bool ScopeRequested(lldb::ValueType scope) {
+switch (scope) {
+case eValueTypeVariableGlobal:
+case eValueTypeVariableStatic:
+  return m_option_variable.show_globals;
+case eValueTypeVariableArgument:
+  return m_option_variable.show_args;
+case eValueTypeVariableLocal:
+  return m_option_variable.show_locals;
+case eValueTypeInvalid:
+case eValueTypeRegister:
+case eValueTypeRegisterSet:
+case eValueTypeConstResult:
+case eValueTypeVariableThreadLocal:
+  return false;
+}
+  }
+
   bool DoExecute(Args &command, CommandReturnObject &result) override {
 // No need to check "frame" for validity as eCommandRequiresFrame ensures
 // it is valid
@@ -630,27 +649,8 @@
 if (num_variables > 0) {
   for (size_t i = 0; i < num_variables; i++) {
 var_sp = variable_list->GetVariableAtIndex(i);
-switch (var_sp->GetScope()) {
-case eValueTypeVariableGlobal:
-  if (!m_option_variable.show_globals)
-continue;
-  break;
-case eValueTypeVariableStatic:
-  if (!m_option_variable.show_globals)
+if (!ScopeRequested(var_sp->GetScope()))
 continue;
-  break;
-case eValueTypeVariableArgument:
-  if (!m_option_variable.show_args)
-continue;
-  break;
-case eValueTypeVariableLocal:
-  if (!m_option_variable.show_locals)
-continue;
-  break;
-default:
-  continue;
-  break;
-}
 std::string scope_string;
 if (m_option_variable.show_scope)
   scope_string = GetScopeString(var_sp).str();


Index: lldb/source/Commands/CommandObjectFrame.cpp
===
--- lldb/source/Commands/CommandObjectFrame.cpp
+++ lldb/source/Commands/CommandObjectFrame.cpp
@@ -480,6 +480,25 @@
 return llvm::StringRef();
   }
 
+  /// Returns true if `scope` matches any of the options in `m_option_variable`.
+  bool ScopeRequested(lldb::ValueType scope) {
+switch (scope) {
+case eValueTypeVariableGlobal:
+case eValueTypeVariableStatic:
+  return m_option_variable.show_globals;
+case eValueTypeVariableArgument:
+  return m_option_variable.show_args;
+case eValueTypeVariableLocal:
+  return m_option_variable.show_locals;
+case eValueTypeInvalid:
+case eValueTypeRegister:
+case eValueTypeRegisterSet:
+case eValueTypeConstResult:
+case eValueTypeVariableThreadLocal:
+  return false;
+}
+  }
+
   bool DoExecute(Args &command, CommandReturnObject &result) override {
 // No need to check "frame" for validity as eCommandRequiresFrame ensures
 // it is valid
@@ -630,27 +649,8 @@
 if (num_variables > 0) {
   for (size_t i = 0; i < num_variables; i++) {
 var_sp = variable_list->GetVariableAtIndex(i);
-switch (var_sp->GetScope()) {
-case eValueTypeVariableGlobal:
-  if (!m_option_variable.show_globals)
-continue;
-  break;
-case eValueTypeVariableStatic:
-  if (!m_option_variable.show_globals)
+if (!ScopeRequested(var_sp->GetScope()))
 continue;
-  break;
-case eValueTypeVariableArgument:
-  if (!m_option_variable.show_args)
-continue;
-  break;
-case eValueTypeVariableLocal:
-  if (!m_option_variable.show_locals)
-continue;
-  break;
-default:
-  continue;
-  break;
-}
 std::string scope_string;
 if (m_option_variable.show_scope)
   scope_string = GetScopeString(var_sp).str();
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D155197: [lldb][NFC] Refactor test to enable subsequent reuse

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG00c60496775b: [lldb][NFC] Refactor test to enable subsequent 
reuse (authored by fdeazeve).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155197

Files:
  lldb/unittests/Expression/DWARFExpressionTest.cpp


Index: lldb/unittests/Expression/DWARFExpressionTest.cpp
===
--- lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -500,19 +500,26 @@
 platform_sp, target_sp);
 
   ExecutionContext exe_ctx(target_sp, false);
+
+  auto evaluate = [&](DWARFExpression &expr, Status &status, Value &result) {
+DataExtractor extractor;
+expr.GetExpressionData(extractor);
+return DWARFExpression::Evaluate(
+&exe_ctx, /*reg_ctx*/ nullptr, /*module_sp*/ {}, extractor, dwarf_cu,
+lldb::eRegisterKindLLDB,
+/*initial_value_ptr*/ nullptr,
+/*object_address_ptr*/ nullptr, result, &status);
+  };
+
   // DW_OP_addrx takes a single leb128 operand, the index in the addr table:
-  uint8_t expr[] = {DW_OP_addrx, 0x01};
-  DataExtractor extractor(expr, sizeof(expr), lldb::eByteOrderLittle,
+  uint8_t expr_data[] = {DW_OP_addrx, 0x01};
+  DataExtractor extractor(expr_data, sizeof(expr_data), lldb::eByteOrderLittle,
   /*addr_size*/ 4);
-  Value result;
-  Status status;
-  ASSERT_TRUE(DWARFExpression::Evaluate(
-  &exe_ctx, /*reg_ctx*/ nullptr, /*module_sp*/ {}, extractor, dwarf_cu,
-  lldb::eRegisterKindLLDB,
-  /*initial_value_ptr*/ nullptr,
-  /*object_address_ptr*/ nullptr, result, &status))
-  << status.ToError();
+  DWARFExpression expr(extractor);
 
+  Status status;
+  Value result;
+  ASSERT_TRUE(evaluate(expr, status, result)) << status.ToError();
   ASSERT_EQ(result.GetValueType(), Value::ValueType::LoadAddress);
   ASSERT_EQ(result.GetScalar().UInt(), 0x5678u);
 }


Index: lldb/unittests/Expression/DWARFExpressionTest.cpp
===
--- lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -500,19 +500,26 @@
 platform_sp, target_sp);
 
   ExecutionContext exe_ctx(target_sp, false);
+
+  auto evaluate = [&](DWARFExpression &expr, Status &status, Value &result) {
+DataExtractor extractor;
+expr.GetExpressionData(extractor);
+return DWARFExpression::Evaluate(
+&exe_ctx, /*reg_ctx*/ nullptr, /*module_sp*/ {}, extractor, dwarf_cu,
+lldb::eRegisterKindLLDB,
+/*initial_value_ptr*/ nullptr,
+/*object_address_ptr*/ nullptr, result, &status);
+  };
+
   // DW_OP_addrx takes a single leb128 operand, the index in the addr table:
-  uint8_t expr[] = {DW_OP_addrx, 0x01};
-  DataExtractor extractor(expr, sizeof(expr), lldb::eByteOrderLittle,
+  uint8_t expr_data[] = {DW_OP_addrx, 0x01};
+  DataExtractor extractor(expr_data, sizeof(expr_data), lldb::eByteOrderLittle,
   /*addr_size*/ 4);
-  Value result;
-  Status status;
-  ASSERT_TRUE(DWARFExpression::Evaluate(
-  &exe_ctx, /*reg_ctx*/ nullptr, /*module_sp*/ {}, extractor, dwarf_cu,
-  lldb::eRegisterKindLLDB,
-  /*initial_value_ptr*/ nullptr,
-  /*object_address_ptr*/ nullptr, result, &status))
-  << status.ToError();
+  DWARFExpression expr(extractor);
 
+  Status status;
+  Value result;
+  ASSERT_TRUE(evaluate(expr, status, result)) << status.ToError();
   ASSERT_EQ(result.GetValueType(), Value::ValueType::LoadAddress);
   ASSERT_EQ(result.GetScalar().UInt(), 0x5678u);
 }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D155198: [lldb] Consider OP_addrx in DWARFExpression::Update_DW_OP_addr

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve updated this revision to Diff 541986.
fdeazeve added a comment.

Rebased


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155198

Files:
  lldb/source/Expression/DWARFExpression.cpp
  lldb/unittests/Expression/DWARFExpressionTest.cpp


Index: lldb/unittests/Expression/DWARFExpressionTest.cpp
===
--- lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -522,6 +522,11 @@
   ASSERT_TRUE(evaluate(expr, status, result)) << status.ToError();
   ASSERT_EQ(result.GetValueType(), Value::ValueType::LoadAddress);
   ASSERT_EQ(result.GetScalar().UInt(), 0x5678u);
+
+  ASSERT_TRUE(expr.Update_DW_OP_addr(dwarf_cu, 0xdeadbeef));
+  ASSERT_TRUE(evaluate(expr, status, result)) << status.ToError();
+  ASSERT_EQ(result.GetValueType(), Value::ValueType::LoadAddress);
+  ASSERT_EQ(result.GetScalar().UInt(), 0xdeadbeefu);
 }
 
 class CustomSymbolFileDWARF : public SymbolFileDWARF {
Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -408,13 +408,33 @@
   // the heap data so "m_data" will now correctly manage the heap data.
   m_data.SetData(encoder.GetDataBuffer());
   return true;
-} else {
-  const offset_t op_arg_size =
-  GetOpcodeDataSize(m_data, offset, op, dwarf_cu);
-  if (op_arg_size == LLDB_INVALID_OFFSET)
-break;
-  offset += op_arg_size;
 }
+if (op == DW_OP_addrx) {
+  // Replace DW_OP_addrx with DW_OP_addr, since we can't modify the
+  // read-only debug_addr table.
+  // Subtract one to account for the opcode.
+  llvm::ArrayRef data_before_op = m_data.GetData().take_front(offset - 1);
+
+  // Read the addrx index to determine how many bytes it needs.
+  const lldb::offset_t old_offset = offset;
+  m_data.GetULEB128(&offset);
+  if (old_offset == offset)
+return false;
+  llvm::ArrayRef data_after_op = m_data.GetData().drop_front(offset);
+
+  DataEncoder encoder(m_data.GetByteOrder(), m_data.GetAddressByteSize());
+  encoder.AppendData(data_before_op);
+  encoder.AppendU8(DW_OP_addr);
+  encoder.AppendAddress(file_addr);
+  encoder.AppendData(data_after_op);
+  m_data.SetData(encoder.GetDataBuffer());
+  return true;
+}
+const offset_t op_arg_size =
+GetOpcodeDataSize(m_data, offset, op, dwarf_cu);
+if (op_arg_size == LLDB_INVALID_OFFSET)
+  break;
+offset += op_arg_size;
   }
   return false;
 }


Index: lldb/unittests/Expression/DWARFExpressionTest.cpp
===
--- lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -522,6 +522,11 @@
   ASSERT_TRUE(evaluate(expr, status, result)) << status.ToError();
   ASSERT_EQ(result.GetValueType(), Value::ValueType::LoadAddress);
   ASSERT_EQ(result.GetScalar().UInt(), 0x5678u);
+
+  ASSERT_TRUE(expr.Update_DW_OP_addr(dwarf_cu, 0xdeadbeef));
+  ASSERT_TRUE(evaluate(expr, status, result)) << status.ToError();
+  ASSERT_EQ(result.GetValueType(), Value::ValueType::LoadAddress);
+  ASSERT_EQ(result.GetScalar().UInt(), 0xdeadbeefu);
 }
 
 class CustomSymbolFileDWARF : public SymbolFileDWARF {
Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -408,13 +408,33 @@
   // the heap data so "m_data" will now correctly manage the heap data.
   m_data.SetData(encoder.GetDataBuffer());
   return true;
-} else {
-  const offset_t op_arg_size =
-  GetOpcodeDataSize(m_data, offset, op, dwarf_cu);
-  if (op_arg_size == LLDB_INVALID_OFFSET)
-break;
-  offset += op_arg_size;
 }
+if (op == DW_OP_addrx) {
+  // Replace DW_OP_addrx with DW_OP_addr, since we can't modify the
+  // read-only debug_addr table.
+  // Subtract one to account for the opcode.
+  llvm::ArrayRef data_before_op = m_data.GetData().take_front(offset - 1);
+
+  // Read the addrx index to determine how many bytes it needs.
+  const lldb::offset_t old_offset = offset;
+  m_data.GetULEB128(&offset);
+  if (old_offset == offset)
+return false;
+  llvm::ArrayRef data_after_op = m_data.GetData().drop_front(offset);
+
+  DataEncoder encoder(m_data.GetByteOrder(), m_data.GetAddressByteSize());
+  encoder.AppendData(data_before_op);
+  encoder.AppendU8(DW_OP_addr);
+  encoder.AppendAddress(file_addr);
+  encoder.AppendData(data_after_op);
+  m_data.SetData(encoder.GetDataBuffer());
+  return true;
+}
+const offset_t o

[Lldb-commits] [PATCH] D155334: [lldb] Make frame var --regex always search globals

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve updated this revision to Diff 541987.
fdeazeve added a comment.

Rebased


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155334

Files:
  lldb/include/lldb/Symbol/VariableList.h
  lldb/source/Commands/CommandObjectFrame.cpp
  lldb/test/API/commands/frame/var/TestFrameVar.py

Index: lldb/test/API/commands/frame/var/TestFrameVar.py
===
--- lldb/test/API/commands/frame/var/TestFrameVar.py
+++ lldb/test/API/commands/frame/var/TestFrameVar.py
@@ -58,6 +58,16 @@
 command_result = lldb.SBCommandReturnObject()
 interp = self.dbg.GetCommandInterpreter()
 
+# Ensure --regex can find globals if it is the very first frame var command.
+result = interp.HandleCommand("frame var --regex g_", command_result)
+self.assertEqual(
+result,
+lldb.eReturnStatusSuccessFinishResult,
+"frame var --regex g_ didn't succeed",
+)
+output = command_result.GetOutput()
+self.assertIn("g_var", output, "g_var not found with regex g_")
+
 # Just get args:
 result = interp.HandleCommand("frame var -l", command_result)
 self.assertEqual(
Index: lldb/source/Commands/CommandObjectFrame.cpp
===
--- lldb/source/Commands/CommandObjectFrame.cpp
+++ lldb/source/Commands/CommandObjectFrame.cpp
@@ -499,6 +499,31 @@
 }
   }
 
+  /// Finds all variables in `all_variables` whose name matches `regex`,
+  /// inserting them into `matches`. Variables already contained in `matches`
+  /// are not inserted again.
+  /// Nullopt is returned in case of no matches.
+  /// A sub-range of `matches` with all newly added variables is returned. This
+  /// may be empty if all matches were already contained in `matches`.
+  std::optional>
+  findUniqueRegexMatches(RegularExpression ®ex,
+ VariableList &matches,
+ const VariableList &all_variables) {
+bool any_matches = false;
+const size_t previous_num_vars = matches.GetSize();
+
+for (const VariableSP &var : all_variables) {
+  if (!var->NameMatches(regex) || !ScopeRequested(var->GetScope()))
+continue;
+  any_matches = true;
+  matches.AddVariableIfUnique(var);
+}
+
+if (any_matches)
+  return matches.toArrayRef().drop_front(previous_num_vars);
+return std::nullopt;
+  }
+
   bool DoExecute(Args &command, CommandReturnObject &result) override {
 // No need to check "frame" for validity as eCommandRequiresFrame ensures
 // it is valid
@@ -506,6 +531,10 @@
 
 Stream &s = result.GetOutputStream();
 
+// Using a regex should behave like looking for an exact name match: it
+// also finds globals.
+m_option_variable.show_globals |= m_option_variable.use_regex;
+
 // Be careful about the stack frame, if any summary formatter runs code, it
 // might clear the StackFrameList for the thread.  So hold onto a shared
 // pointer to the frame so it stays alive.
@@ -518,7 +547,6 @@
   result.AppendError(error.AsCString());
 
 }
-VariableSP var_sp;
 ValueObjectSP valobj_sp;
 
 TypeSummaryImplSP summary_format_sp;
@@ -551,46 +579,38 @@
 // objects from them...
 for (auto &entry : command) {
   if (m_option_variable.use_regex) {
-const size_t regex_start_index = regex_var_list.GetSize();
 llvm::StringRef name_str = entry.ref();
 RegularExpression regex(name_str);
 if (regex.IsValid()) {
-  size_t num_matches = 0;
-  const size_t num_new_regex_vars =
-  variable_list->AppendVariablesIfUnique(regex, regex_var_list,
- num_matches);
-  if (num_new_regex_vars > 0) {
-for (size_t regex_idx = regex_start_index,
-end_index = regex_var_list.GetSize();
- regex_idx < end_index; ++regex_idx) {
-  var_sp = regex_var_list.GetVariableAtIndex(regex_idx);
-  if (var_sp) {
-valobj_sp = frame->GetValueObjectForFrameVariable(
-var_sp, m_varobj_options.use_dynamic);
-if (valobj_sp) {
-  std::string scope_string;
-  if (m_option_variable.show_scope)
-scope_string = GetScopeString(var_sp).str();
-
-  if (!scope_string.empty())
-s.PutCString(scope_string);
-
-  if (m_option_variable.show_decl &&
-  var_sp->GetDeclaration().GetFile()) {
-bool show_fullpaths = false;
-bool show_module = true;
-if (var_sp->DumpDec

[Lldb-commits] [PATCH] D155334: [lldb] Make frame var --regex always search globals

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve updated this revision to Diff 542122.
fdeazeve added a comment.

Address review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155334

Files:
  lldb/include/lldb/Symbol/VariableList.h
  lldb/source/Commands/CommandObjectFrame.cpp
  lldb/test/API/commands/frame/var/TestFrameVar.py

Index: lldb/test/API/commands/frame/var/TestFrameVar.py
===
--- lldb/test/API/commands/frame/var/TestFrameVar.py
+++ lldb/test/API/commands/frame/var/TestFrameVar.py
@@ -58,6 +58,16 @@
 command_result = lldb.SBCommandReturnObject()
 interp = self.dbg.GetCommandInterpreter()
 
+# Ensure --regex can find globals if it is the very first frame var command.
+self.expect("frame var --regex g_", substrs=["g_var"])
+
+# Ensure the requested scope is respected:
+self.expect(
+"frame var --regex argc --no-args",
+error=True,
+substrs=["no variables matched the regular expression 'argc'"],
+)
+
 # Just get args:
 result = interp.HandleCommand("frame var -l", command_result)
 self.assertEqual(
Index: lldb/source/Commands/CommandObjectFrame.cpp
===
--- lldb/source/Commands/CommandObjectFrame.cpp
+++ lldb/source/Commands/CommandObjectFrame.cpp
@@ -499,6 +499,31 @@
 }
   }
 
+  /// Finds all the variables in `all_variables` whose name matches `regex`,
+  /// inserting them into `matches`. Variables already contained in `matches`
+  /// are not inserted again.
+  /// Nullopt is returned in case of no matches.
+  /// A sub-range of `matches` with all newly inserted variables is returned.
+  /// This may be empty if all matches were already contained in `matches`.
+  std::optional>
+  findUniqueRegexMatches(RegularExpression ®ex,
+ VariableList &matches,
+ const VariableList &all_variables) {
+bool any_matches = false;
+const size_t previous_num_vars = matches.GetSize();
+
+for (const VariableSP &var : all_variables) {
+  if (!var->NameMatches(regex) || !ScopeRequested(var->GetScope()))
+continue;
+  any_matches = true;
+  matches.AddVariableIfUnique(var);
+}
+
+if (any_matches)
+  return matches.toArrayRef().drop_front(previous_num_vars);
+return std::nullopt;
+  }
+
   bool DoExecute(Args &command, CommandReturnObject &result) override {
 // No need to check "frame" for validity as eCommandRequiresFrame ensures
 // it is valid
@@ -506,6 +531,10 @@
 
 Stream &s = result.GetOutputStream();
 
+// Using a regex should behave like looking for an exact name match: it
+// also finds globals.
+m_option_variable.show_globals |= m_option_variable.use_regex;
+
 // Be careful about the stack frame, if any summary formatter runs code, it
 // might clear the StackFrameList for the thread.  So hold onto a shared
 // pointer to the frame so it stays alive.
@@ -518,7 +547,6 @@
   result.AppendError(error.AsCString());
 
 }
-VariableSP var_sp;
 ValueObjectSP valobj_sp;
 
 TypeSummaryImplSP summary_format_sp;
@@ -551,46 +579,38 @@
 // objects from them...
 for (auto &entry : command) {
   if (m_option_variable.use_regex) {
-const size_t regex_start_index = regex_var_list.GetSize();
 llvm::StringRef name_str = entry.ref();
 RegularExpression regex(name_str);
 if (regex.IsValid()) {
-  size_t num_matches = 0;
-  const size_t num_new_regex_vars =
-  variable_list->AppendVariablesIfUnique(regex, regex_var_list,
- num_matches);
-  if (num_new_regex_vars > 0) {
-for (size_t regex_idx = regex_start_index,
-end_index = regex_var_list.GetSize();
- regex_idx < end_index; ++regex_idx) {
-  var_sp = regex_var_list.GetVariableAtIndex(regex_idx);
-  if (var_sp) {
-valobj_sp = frame->GetValueObjectForFrameVariable(
-var_sp, m_varobj_options.use_dynamic);
-if (valobj_sp) {
-  std::string scope_string;
-  if (m_option_variable.show_scope)
-scope_string = GetScopeString(var_sp).str();
-
-  if (!scope_string.empty())
-s.PutCString(scope_string);
-
-  if (m_option_variable.show_decl &&
-  var_sp->GetDeclaration().GetFile()) {
-bool show_fullpaths = false;
-bool show_module = true;
-if (var_sp->DumpDeclaration(&s, show_fullpaths,
- 

[Lldb-commits] [PATCH] D155334: [lldb] Make frame var --regex always search globals

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG8b19d13fde6e: [lldb] Make frame var --regex always search 
globals (authored by fdeazeve).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155334

Files:
  lldb/include/lldb/Symbol/VariableList.h
  lldb/source/Commands/CommandObjectFrame.cpp
  lldb/test/API/commands/frame/var/TestFrameVar.py

Index: lldb/test/API/commands/frame/var/TestFrameVar.py
===
--- lldb/test/API/commands/frame/var/TestFrameVar.py
+++ lldb/test/API/commands/frame/var/TestFrameVar.py
@@ -58,6 +58,16 @@
 command_result = lldb.SBCommandReturnObject()
 interp = self.dbg.GetCommandInterpreter()
 
+# Ensure --regex can find globals if it is the very first frame var command.
+self.expect("frame var --regex g_", substrs=["g_var"])
+
+# Ensure the requested scope is respected:
+self.expect(
+"frame var --regex argc --no-args",
+error=True,
+substrs=["no variables matched the regular expression 'argc'"],
+)
+
 # Just get args:
 result = interp.HandleCommand("frame var -l", command_result)
 self.assertEqual(
Index: lldb/source/Commands/CommandObjectFrame.cpp
===
--- lldb/source/Commands/CommandObjectFrame.cpp
+++ lldb/source/Commands/CommandObjectFrame.cpp
@@ -499,6 +499,31 @@
 }
   }
 
+  /// Finds all the variables in `all_variables` whose name matches `regex`,
+  /// inserting them into `matches`. Variables already contained in `matches`
+  /// are not inserted again.
+  /// Nullopt is returned in case of no matches.
+  /// A sub-range of `matches` with all newly inserted variables is returned.
+  /// This may be empty if all matches were already contained in `matches`.
+  std::optional>
+  findUniqueRegexMatches(RegularExpression ®ex,
+ VariableList &matches,
+ const VariableList &all_variables) {
+bool any_matches = false;
+const size_t previous_num_vars = matches.GetSize();
+
+for (const VariableSP &var : all_variables) {
+  if (!var->NameMatches(regex) || !ScopeRequested(var->GetScope()))
+continue;
+  any_matches = true;
+  matches.AddVariableIfUnique(var);
+}
+
+if (any_matches)
+  return matches.toArrayRef().drop_front(previous_num_vars);
+return std::nullopt;
+  }
+
   bool DoExecute(Args &command, CommandReturnObject &result) override {
 // No need to check "frame" for validity as eCommandRequiresFrame ensures
 // it is valid
@@ -506,6 +531,10 @@
 
 Stream &s = result.GetOutputStream();
 
+// Using a regex should behave like looking for an exact name match: it
+// also finds globals.
+m_option_variable.show_globals |= m_option_variable.use_regex;
+
 // Be careful about the stack frame, if any summary formatter runs code, it
 // might clear the StackFrameList for the thread.  So hold onto a shared
 // pointer to the frame so it stays alive.
@@ -518,7 +547,6 @@
   result.AppendError(error.AsCString());
 
 }
-VariableSP var_sp;
 ValueObjectSP valobj_sp;
 
 TypeSummaryImplSP summary_format_sp;
@@ -551,46 +579,38 @@
 // objects from them...
 for (auto &entry : command) {
   if (m_option_variable.use_regex) {
-const size_t regex_start_index = regex_var_list.GetSize();
 llvm::StringRef name_str = entry.ref();
 RegularExpression regex(name_str);
 if (regex.IsValid()) {
-  size_t num_matches = 0;
-  const size_t num_new_regex_vars =
-  variable_list->AppendVariablesIfUnique(regex, regex_var_list,
- num_matches);
-  if (num_new_regex_vars > 0) {
-for (size_t regex_idx = regex_start_index,
-end_index = regex_var_list.GetSize();
- regex_idx < end_index; ++regex_idx) {
-  var_sp = regex_var_list.GetVariableAtIndex(regex_idx);
-  if (var_sp) {
-valobj_sp = frame->GetValueObjectForFrameVariable(
-var_sp, m_varobj_options.use_dynamic);
-if (valobj_sp) {
-  std::string scope_string;
-  if (m_option_variable.show_scope)
-scope_string = GetScopeString(var_sp).str();
-
-  if (!scope_string.empty())
-s.PutCString(scope_string);
-
-  if (m_option_variable.show_decl &&
-  var_sp->GetDeclaration().GetFile()) {
-bool show_fullpaths = false;
-bool show_module = 

[Lldb-commits] [PATCH] D155198: [lldb] Consider OP_addrx in DWARFExpression::Update_DW_OP_addr

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added a comment.

Hi @clayborg, just wanted to make sure this doesn't fall off your radar :)
This fixes all outstanding DWARF 5 issues with Debug Maps, so it would be cool 
if we could get this in.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155198

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

[Lldb-commits] [PATCH] D158023: [lldb] Simplify the LLDB website structure

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added a comment.

In D158023#4590502 , @bulbazord wrote:

> I think the categories you've set are the same ones I would have created. May 
> I suggest renaming `Extending LLDB` to `Scripting LLDB` or something similar? 
> I think "Extending" and "Developing" are close enough in meaning that it may 
> be confusing to end users.

+1 to scripting; this is the word we use all the time, so it makes sense to 
highlight it as the name of a category.

Btw, what do you think of the extensions page? This patch currently keeps 
"extensions" under "using lldb", but if we read that page it contains no 
"using" information. It explains at length how an extension is *implemented* in 
terms of DWARF. Because of that, I think this page is aimed at developers.


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

https://reviews.llvm.org/D158023

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

[Lldb-commits] [PATCH] D158043: [lldb][NFCI] Module constructor should take ConstString by value

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added inline comments.



Comment at: lldb/include/lldb/Core/Module.h:126
   Module(
-  const FileSpec &file_spec, const ArchSpec &arch,
-  const ConstString *object_name = nullptr,
+  const FileSpec &file_spec, const ArchSpec &arch, ConstString object_name,
   lldb::offset_t object_offset = 0,

Out of curiosity, what was the rationale for removing the default parameter 
here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158043

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

[Lldb-commits] [PATCH] D158043: [lldb][NFCI] Module constructor should take ConstString by value

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

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

LGTM! Good catch :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158043

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

[Lldb-commits] [PATCH] D158023: [lldb] Simplify the LLDB website structure

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

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

LGTM!


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

https://reviews.llvm.org/D158023

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

[Lldb-commits] [PATCH] D158470: [lldb] Add support for recognizing swift mangled names

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added inline comments.



Comment at: lldb/source/Core/Mangled.cpp:61
 
+  // Swift's older style of mangling used "_T" as a mangling prefix. This can
+  // lead to false positives with other symbols that just so happen to start

JDevlieghere wrote:
> aprantl wrote:
> > Feel free to completely ignore this, it's a pointless micro optimization.
> > 
> > I'm curious if it would be more efficient to write this as 
> > 
> > ```
> > switch (name[0]) {
> >   case '?': return Mangled::eManglingSchemeMSVC;
> >   case '_': 
> >  switch(name[1]) {
> > ...
> >  }
> >   case '$': 
> >  switch(name[1]) {
> > ...
> >  }
> > ```
> > or if it would actually be slower than the chain of "vector" comparisons we 
> > have right now?
> I actually started writing a similar comment before discarding it. Even if 
> this code is as hot as I expect it to be, I don't think it would outweigh the 
> complexity and the potential for bugs. I really like how you can glance at 
> the code and see the different mangling schemes and that's the first thing 
> we'd lose. Anyway happy to be proven wrong too. 
Honestly the optimizer is pretty good at doing those, look at the IR: 
https://godbolt.org/z/PY3TeadbM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158470

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

[Lldb-commits] [PATCH] D159011: [lldb][NFCI] Remove use of ConstString from UnixSignals

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added a comment.

I am slightly wary of making this a global set without any kind of thread safe 
mechanism, as I (naively, not really knowing how this class is used) would 
expect us to be able to instantiate multiple servers and have them be accessed 
concurrently. As such, it makes more sense to have each server own its set of 
strings.

Have we considered having UnixSignals be the one to extend the lifetime of 
strings? I couldn't figure out if there is any advantage to this vs the other 
suggestion.




Comment at: 
lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp:48
+// from the remote, these strings need persistent storage client-side.
+static llvm::StringSet<> g_signal_string_storage;
 

Is there a strong motivation for making this a global static, instead of a 
private member of PlatformRemoteGDBServer?
My assumption here is that we only ever have one PlatformRemoteGDBServer active 
at any given time, otherwise (if concurrent servers now or in the future) the 
global approach is not thread safe.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159011

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

[Lldb-commits] [PATCH] D159011: [lldb][NFCI] Remove use of ConstString from UnixSignals

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added a comment.

> I decided to make it a global object to guarantee its lifetime independent of 
> any `PlatformRemoteGDBServer` instance. I don't mind wrapping it in a 
> concurrent structure to guarantee thread safety though.

At that point we would pretty much be re-implementing the notion of a 
ConstString but with a separate pool, right?

By the way, feel free to disregard this discussion, I just wanted to make sure 
we are aware that we lost one safety we had before.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159011

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

[Lldb-commits] [PATCH] D152870: [lldb][NFCI] Remove StructuredData::Array::GetItemAtIndexAsString overloads with ConstString

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

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

LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152870

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

[Lldb-commits] [PATCH] D159150: [lldb][NFCI] Replace bespoke iterator check with std::next

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve accepted this revision.
fdeazeve added a comment.

Nice catch!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159150

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

[Lldb-commits] [PATCH] D159313: [lldb][NFCI] Remove use of ConstString in StructuredData

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added inline comments.



Comment at: lldb/include/lldb/Utility/StructuredData.h:436
   auto array_sp = std::make_shared();
-  collection::const_iterator iter;
-  for (iter = m_dict.begin(); iter != m_dict.end(); ++iter) {
+  for (auto iter = m_dict.begin(); iter != m_dict.end(); ++iter) {
 auto key_object_sp = std::make_shared();

Since we are touching this line anyway, could you replace this with

```
for (StringRef key : llvm::make_first_range(m_dict))
```

This has a bit less cognitive burden IMO



Comment at: lldb/include/lldb/Utility/StructuredData.h:438
 auto key_object_sp = std::make_shared();
-key_object_sp->SetValue(iter->first.AsCString());
+key_object_sp->SetValue(iter->first());
 array_sp->Push(key_object_sp);

Also since you are touching this line, one could argue for it to be deleted and 
folded into `std::make_shared(Key);`

I don't feel strongly about it though, it could also be more appropriate for a 
separate commit



Comment at: lldb/include/lldb/Utility/StructuredData.h:448
-  if (!key.empty()) {
-ConstString key_cs(key);
-collection::const_iterator iter = m_dict.find(key_cs);

This line in particular also shows why I think this patch is appropriate: even 
queries were introducing strings into the pool



Comment at: lldb/include/lldb/Utility/StructuredData.h:537
 void AddItem(llvm::StringRef key, ObjectSP value_sp) {
-  ConstString key_cs(key);
-  m_dict[key_cs] = std::move(value_sp);
+  m_dict.insert({key, std::move(value_sp)});
 }

This is not equivalent to the old code, right? The previous code would 
overwrite the contents if the key already existed.



Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:2035
   uint32_t reg;
-  if (llvm::to_integer(key.AsCString(), reg))
 expedited_register_map[reg] =

This is another win: the old code was calling `StringRef(const char*)`, which 
has to iterate over the entire string to find the null terminating character.



Comment at: lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp:248
   if (do_truncate_remapping_names) {
-FileSpec build_path(key.AsCString());
 FileSpec source_path(DBGSourcePath.c_str());

same win here



Comment at: lldb/source/Target/Target.cpp:3866
-s.Printf("%s : %s\n", key.GetCString(),
-  object->GetStringValue().str().c_str());
 return true;

we talked about it in private, but I feel like stating this publicly: does 
anyone know if this was legal? `object->GetStringValue().str()` creates a 
temporary std::string. And then we call `c_str()` and pass that to the Printf 
function. Does the temporary std::string have its lifetime extended?



Comment at: lldb/source/Utility/StructuredData.cpp:173
+
+  llvm::sort(sorted_entries, [&](const Entry &lhs, const Entry &rhs) -> bool {
+return lhs.first < rhs.first;

I thought pairs had `operator <` defined already?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159313

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

[Lldb-commits] [PATCH] D159011: [lldb][NFCI] Remove use of ConstString from UnixSignals

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve accepted this revision.
fdeazeve added a comment.

LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159011

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

[Lldb-commits] [PATCH] D159313: [lldb][NFCI] Remove use of ConstString in StructuredData

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

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

LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159313

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

[Lldb-commits] [PATCH] D131901: [lldb][debugserver] Revert "Use llvm::all_of (NFC)" for debugserver

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve accepted this revision.
fdeazeve added a comment.

LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131901

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

[Lldb-commits] [PATCH] D132004: [lldb][Tests] Skip static-only tests in TestConstStaticIntegralMember.py for dsym variant

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added a comment.

This test is now failing even on the "basic" Clang (i.e. tip of trunk) test 
matrix entry:

https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake-matrix/5043/execution/node/62/log/?consoleFull

The log above is for the "Test DWARF4" portion of the job


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132004

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

[Lldb-commits] [PATCH] D132004: [lldb][Tests] Skip static-only tests in TestConstStaticIntegralMember.py for dsym variant

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added a comment.

In D132004#3728394 , @fdeazeve wrote:

> This test is now failing even on the "basic" Clang (i.e. tip of trunk) test 
> matrix entry:
>
> https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake-matrix/5043/execution/node/62/log/?consoleFull
>
> The log above is for the "Test DWARF4" portion of the job

Looking at the logs, the Dwarf 2 and Dwarf 4 jobs do not use ToT, but rather 
Clang 13. For example, for the Dwarf 2 job, we see this:

> "/Users/buildslave/jenkins/workspace/lldb-cmake-matrix/clang_1300_build/bin/clang"
>   -std=c++11 -gdwarf-2 -isysroot

Which is weird, given how we don't build Clang 13 until much later on the 
pipeline.
I think the bot has a polluted environment somehow across runs. For example, 
build 5042:

https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake-matrix/5042/execution/node/52/log/?consoleFull

> ERROR:root:/Users/buildslave/jenkins/workspace/lldb-cmake-matrix/clang_1300_build/bin/clang
>  is not a valid compiler executable; aborting...

Sometimes the Clang13 builds get cleaned up (see how sometimes the Clang 13 
jobs take 30s to run, and sometimes 40min).
I'm guessing that this error shows up when the previous build got cleaned up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132004

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

[Lldb-commits] [PATCH] D132004: [lldb][Tests] Skip static-only tests in TestConstStaticIntegralMember.py for dsym variant

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added a comment.

Greendragon is leaking environment configurations across runs. I'll address 
this in a separate PR


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132004

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

[Lldb-commits] [PATCH] D132257: [lldb] Create flag to use a custom libcxx in tests

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

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

When running LLDB tests with a separately built copy of libcxx, it may
be necessary to also use the `nostdinc++` and `cxx-isystem` flags, so
that Clang doesn't keep multiple paths to the standard library in its
include search list. This is particularly important when also running
LLDB tests using older versions of clang, which may not deal with
multiple standard libraries properly.

These issues have all been seen in the LLDB matrix green dragon bot,
which ensures that tip-of-trunk LLDBs can debug code compiled with older
versions of Clang/libcxx.

Once this is merged, llvm-zorg can be updated to use the flags. Local
testing has been done by compiling the project with:

`-DLLDB_TEST_USER_ARGS="--custom-libcpp=`

We verified that all LLDB tests failed to find standard library files.
Then we fixed the invalid path to point to a proper libcxx, and the
tests passed.

A similar effect could have been accomplished by repurposing either
CXXFLAGS or CXXFLAGS_EXTRAS, but these are already for many different
purposes, so we choose to isolate the new behavior into a new
environment variable.

We also choose to keep the logic of formatting the flags inside the
Makefile, as that's where we can check which compiler is being used, as
it is already done in other places there.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D132257

Files:
  lldb/packages/Python/lldbsuite/test/dotest.py
  lldb/packages/Python/lldbsuite/test/dotest_args.py
  lldb/packages/Python/lldbsuite/test/make/Makefile.rules


Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -402,6 +402,14 @@
endif
 endif
 
+ifdef CUSTOM_LIBCPP
+ifeq (,$(findstring clang,$(CC)))
+$(error "CUSTOM_LIBCPP only supported for Clang compilers")
+endif
+
+CXXFLAGS += -nostdinc++ -cxx-isystem $(CUSTOM_LIBCPP)
+endif
+
 #--
 # Additional system libraries
 #--
Index: lldb/packages/Python/lldbsuite/test/dotest_args.py
===
--- lldb/packages/Python/lldbsuite/test/dotest_args.py
+++ lldb/packages/Python/lldbsuite/test/dotest_args.py
@@ -172,6 +172,11 @@
 type=str,
 metavar='A plugin whose tests will be enabled',
 help='A plugin whose tests will be enabled. The only currently 
supported plugin is intel-pt.')
+group.add_argument(
+'--custom-libcpp',
+metavar='custom-libcpp',
+dest='custom_libcpp',
+help='(Clang only) Specify path to a custom standard library to use. 
Disables search for other standard C++ libraries.')
 
 # Configuration options
 group = parser.add_argument_group('Remote platform options')
Index: lldb/packages/Python/lldbsuite/test/dotest.py
===
--- lldb/packages/Python/lldbsuite/test/dotest.py
+++ lldb/packages/Python/lldbsuite/test/dotest.py
@@ -335,6 +335,9 @@
 # that explicitly require no debug info.
 os.environ['CFLAGS'] = '-gdwarf-{}'.format(configuration.dwarf_version)
 
+if args.custom_libcpp:
+os.environ['CUSTOM_LIBCPP'] = args.custom_libcpp
+
 if args.settings:
 for setting in args.settings:
 if not len(setting) == 1 or not setting[0].count('='):


Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -402,6 +402,14 @@
 	endif
 endif
 
+ifdef CUSTOM_LIBCPP
+ifeq (,$(findstring clang,$(CC)))
+$(error "CUSTOM_LIBCPP only supported for Clang compilers")
+endif
+
+CXXFLAGS += -nostdinc++ -cxx-isystem $(CUSTOM_LIBCPP)
+endif
+
 #--
 # Additional system libraries
 #--
Index: lldb/packages/Python/lldbsuite/test/dotest_args.py
===
--- lldb/packages/Python/lldbsuite/test/dotest_args.py
+++ lldb/packages/Python/lldbsuite/test/dotest_args.py
@@ -172,6 +172,11 @@
 type=str,
 metavar='A plugin whose tests will be enabled',
 help='A plugin whose tests will be enabled. The only currently supported plugin is intel-pt.')
+group.add_argument(
+'--custom-libcp

[Lldb-commits] [PATCH] D132257: [lldb] Create flag to use a custom libcxx in tests

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve updated this revision to Diff 454073.
fdeazeve added a comment.
Herald added a subscriber: JDevlieghere.

Fixed typo in commit message


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132257

Files:
  lldb/packages/Python/lldbsuite/test/dotest.py
  lldb/packages/Python/lldbsuite/test/dotest_args.py
  lldb/packages/Python/lldbsuite/test/make/Makefile.rules


Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -402,6 +402,14 @@
endif
 endif
 
+ifdef CUSTOM_LIBCPP
+ifeq (,$(findstring clang,$(CC)))
+$(error "CUSTOM_LIBCPP only supported for Clang compilers")
+endif
+
+CXXFLAGS += -nostdinc++ -cxx-isystem $(CUSTOM_LIBCPP)
+endif
+
 #--
 # Additional system libraries
 #--
Index: lldb/packages/Python/lldbsuite/test/dotest_args.py
===
--- lldb/packages/Python/lldbsuite/test/dotest_args.py
+++ lldb/packages/Python/lldbsuite/test/dotest_args.py
@@ -172,6 +172,11 @@
 type=str,
 metavar='A plugin whose tests will be enabled',
 help='A plugin whose tests will be enabled. The only currently 
supported plugin is intel-pt.')
+group.add_argument(
+'--custom-libcpp',
+metavar='custom-libcpp',
+dest='custom_libcpp',
+help='(Clang only) Specify path to a custom standard library to use. 
Disables search for other standard C++ libraries.')
 
 # Configuration options
 group = parser.add_argument_group('Remote platform options')
Index: lldb/packages/Python/lldbsuite/test/dotest.py
===
--- lldb/packages/Python/lldbsuite/test/dotest.py
+++ lldb/packages/Python/lldbsuite/test/dotest.py
@@ -335,6 +335,9 @@
 # that explicitly require no debug info.
 os.environ['CFLAGS'] = '-gdwarf-{}'.format(configuration.dwarf_version)
 
+if args.custom_libcpp:
+os.environ['CUSTOM_LIBCPP'] = args.custom_libcpp
+
 if args.settings:
 for setting in args.settings:
 if not len(setting) == 1 or not setting[0].count('='):


Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -402,6 +402,14 @@
 	endif
 endif
 
+ifdef CUSTOM_LIBCPP
+ifeq (,$(findstring clang,$(CC)))
+$(error "CUSTOM_LIBCPP only supported for Clang compilers")
+endif
+
+CXXFLAGS += -nostdinc++ -cxx-isystem $(CUSTOM_LIBCPP)
+endif
+
 #--
 # Additional system libraries
 #--
Index: lldb/packages/Python/lldbsuite/test/dotest_args.py
===
--- lldb/packages/Python/lldbsuite/test/dotest_args.py
+++ lldb/packages/Python/lldbsuite/test/dotest_args.py
@@ -172,6 +172,11 @@
 type=str,
 metavar='A plugin whose tests will be enabled',
 help='A plugin whose tests will be enabled. The only currently supported plugin is intel-pt.')
+group.add_argument(
+'--custom-libcpp',
+metavar='custom-libcpp',
+dest='custom_libcpp',
+help='(Clang only) Specify path to a custom standard library to use. Disables search for other standard C++ libraries.')
 
 # Configuration options
 group = parser.add_argument_group('Remote platform options')
Index: lldb/packages/Python/lldbsuite/test/dotest.py
===
--- lldb/packages/Python/lldbsuite/test/dotest.py
+++ lldb/packages/Python/lldbsuite/test/dotest.py
@@ -335,6 +335,9 @@
 # that explicitly require no debug info.
 os.environ['CFLAGS'] = '-gdwarf-{}'.format(configuration.dwarf_version)
 
+if args.custom_libcpp:
+os.environ['CUSTOM_LIBCPP'] = args.custom_libcpp
+
 if args.settings:
 for setting in args.settings:
 if not len(setting) == 1 or not setting[0].count('='):
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D132257: [lldb] Create flag to use a custom libcxx in tests

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added inline comments.



Comment at: lldb/packages/Python/lldbsuite/test/dotest.py:338-339
 
+if args.custom_libcpp:
+os.environ['CUSTOM_LIBCPP'] = args.custom_libcpp
+

aprantl wrote:
> JDevlieghere wrote:
> > aprantl wrote:
> > > JDevlieghere wrote:
> > > > Please don't rely on environment variables to pass arguments to the 
> > > > Make invocation. This makes it really tedious to debug make 
> > > > invocations. Instead, pass these explicitly as part of the make 
> > > > invocation from the builders 
> > > > (`packages/Python/lldbsuite/test/builders/`). 
> > > That is a very good point. Maybe we should just fix the -E option, which 
> > > doesn't work anyway due to Makefiles setting CFLAGS_EXTRAS and use that.
> > I think a specific flag is fine, we already have something similar for 
> > using the "hermetic" libc++ (I left a comment below about that). We should 
> > see if we can unify this.
> It looks like (apart from making -E not use CFLAGS_EXTRAS but a fresh 
> variable) we should use that mechanism for all the options passed using 
> os.environ().
I'm a bit puzzled by the Builder class, since I don't recall encountering it 
while tracing the variables from CMake to the final Makefile invocation. I'll 
try to figure this out based on your patch.



Comment at: lldb/packages/Python/lldbsuite/test/dotest_args.py:175
 help='A plugin whose tests will be enabled. The only currently 
supported plugin is intel-pt.')
+group.add_argument(
+'--custom-libcpp',

JDevlieghere wrote:
> We should also think about how this interacts with `--hermetic-libcxx` (and 
> make the name of the options consistent). 
If I understand correctly, that flag is a more restricted behaviour of what is 
being implemented here: `hermetic-libcxx` is _only_ about looking for a libcxx 
built alongside LLDB / Clang, whereas this patch allows an arbitrary libcxx to 
be used (the use cases we're aiming  for don't even use a Clang built alongside 
LLDB).

I believe `hermetic-libcxx` can be implemented in terms of the new 
functionality being added here, but I'll look some more at your patch to make 
sure that's the case


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132257

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

[Lldb-commits] [PATCH] D132263: [lldb] Support specifying a custom libcxx for the API tests

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

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

Thanks for merging the two patches! This LGTM!


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

https://reviews.llvm.org/D132263

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

[Lldb-commits] [PATCH] D132257: [lldb] Create flag to use a custom libcxx in tests

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve abandoned this revision.
fdeazeve added a comment.

Abandoning in favour of D132263 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132257

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

[Lldb-commits] [PATCH] D132596: [lldb][nfc] Remove unused makefile test variables

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

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

The variables LLDB_USING_LIBSTDCPP and LLDB_USING_LIBSTDCPP are no
longer used anywhere.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D132596

Files:
  lldb/packages/Python/lldbsuite/test/make/Makefile.rules


Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -381,13 +381,12 @@
 ifeq (1,$(USE_LIBSTDCPP))
# Clang requires an extra flag: -stdlib=libstdc++
ifneq (,$(findstring clang,$(CC)))
-   CXXFLAGS += -stdlib=libstdc++ -DLLDB_USING_LIBSTDCPP
+   CXXFLAGS += -stdlib=libstdc++
LDFLAGS += -stdlib=libstdc++
endif
 endif
 
 ifeq (1,$(USE_LIBCPP))
-   CXXFLAGS += -DLLDB_USING_LIBCPP
ifneq ($(and $(LIBCPP_INCLUDE_DIR), $(LIBCPP_LIBRARY_DIR)),)
CXXFLAGS += -nostdlib++ -nostdinc++ -cxx-isystem 
$(LIBCPP_INCLUDE_DIR)
LDFLAGS += -L$(LLVM_LIBS_DIR) -Wl,-rpath,$(LIBCPP_LIBRARY_DIR) 
-lc++


Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -381,13 +381,12 @@
 ifeq (1,$(USE_LIBSTDCPP))
 	# Clang requires an extra flag: -stdlib=libstdc++
 	ifneq (,$(findstring clang,$(CC)))
-		CXXFLAGS += -stdlib=libstdc++ -DLLDB_USING_LIBSTDCPP
+		CXXFLAGS += -stdlib=libstdc++
 		LDFLAGS += -stdlib=libstdc++
 	endif
 endif
 
 ifeq (1,$(USE_LIBCPP))
-	CXXFLAGS += -DLLDB_USING_LIBCPP
 	ifneq ($(and $(LIBCPP_INCLUDE_DIR), $(LIBCPP_LIBRARY_DIR)),)
 		CXXFLAGS += -nostdlib++ -nostdinc++ -cxx-isystem $(LIBCPP_INCLUDE_DIR)
 		LDFLAGS += -L$(LLVM_LIBS_DIR) -Wl,-rpath,$(LIBCPP_LIBRARY_DIR) -lc++
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D132598: [lldb] Add more dylib paths for exception breakpoints

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

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

When setting a breakpoint upon throwing exceptions, LLDB only
searches for the libc++abi code inside dylibs named:

1. libc++abi.dylib
2. libSystem.B.dylib

However, this fails to account for libs with a version number. For
example, when building the libcxx and libcxxabi runtimes, the following
dylibs are generated:

build/lib/libc++abi.1.0.dylib
build/lib/libc++abi.1.dylib -> libc++abi.1.0.dylib
build/lib/libc++abi.dylib -> libc++abi.1.dylib

If we are debugging a program linked against any of the "versioned"
libs, the breakpoint doesn't work. This commit adds these names to the
search list.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D132598

Files:
  
lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp


Index: 
lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp
===
--- 
lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp
+++ 
lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp
@@ -453,6 +453,8 @@
 // Apple binaries.
 filter_modules.EmplaceBack("libc++abi.dylib");
 filter_modules.EmplaceBack("libSystem.B.dylib");
+filter_modules.EmplaceBack("libc++abi.1.0.dylib");
+filter_modules.EmplaceBack("libc++abi.1.dylib");
   }
   return target.GetSearchFilterForModuleList(&filter_modules);
 }


Index: lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp
===
--- lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp
+++ lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp
@@ -453,6 +453,8 @@
 // Apple binaries.
 filter_modules.EmplaceBack("libc++abi.dylib");
 filter_modules.EmplaceBack("libSystem.B.dylib");
+filter_modules.EmplaceBack("libc++abi.1.0.dylib");
+filter_modules.EmplaceBack("libc++abi.1.dylib");
   }
   return target.GetSearchFilterForModuleList(&filter_modules);
 }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D132598: [lldb] Add more dylib paths for exception breakpoints

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added inline comments.



Comment at: 
lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp:457
+filter_modules.EmplaceBack("libc++abi.1.0.dylib");
+filter_modules.EmplaceBack("libc++abi.1.dylib");
   }

aprantl wrote:
> Should these be ordered such that the most likely matches on the most modern 
> systems come first?
According to @jasonmolenda, the order is not relevant from a practical point of 
view, as breakpoints will be set in _all_ of the above (if they exist). Unless 
your intent here is for readers to know which files are more common?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132598

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

[Lldb-commits] [PATCH] D139361: [lldb-tests] Force system's libcxx on tests failing with debug symbols

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGb1fcc1840c31: [lldb-tests] Force system's libcxx on 
tests failing with debug symbols (authored by fdeazeve).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139361

Files:
  
lldb/test/API/commands/expression/import-std-module/deque-dbg-info-content/Makefile
  
lldb/test/API/commands/expression/import-std-module/list-dbg-info-content/Makefile
  
lldb/test/API/commands/expression/import-std-module/vector-dbg-info-content/Makefile


Index: 
lldb/test/API/commands/expression/import-std-module/vector-dbg-info-content/Makefile
===
--- 
lldb/test/API/commands/expression/import-std-module/vector-dbg-info-content/Makefile
+++ 
lldb/test/API/commands/expression/import-std-module/vector-dbg-info-content/Makefile
@@ -1,3 +1,5 @@
-USE_LIBCPP := 1
+# FIXME: once the expression evaluator can handle std libraries with debug
+# info, change this to USE_LIBCPP=1
+USE_SYSTEM_STDLIB := 1
 CXX_SOURCES := main.cpp
 include Makefile.rules
Index: 
lldb/test/API/commands/expression/import-std-module/list-dbg-info-content/Makefile
===
--- 
lldb/test/API/commands/expression/import-std-module/list-dbg-info-content/Makefile
+++ 
lldb/test/API/commands/expression/import-std-module/list-dbg-info-content/Makefile
@@ -1,3 +1,5 @@
-USE_LIBCPP := 1
+# FIXME: once the expression evaluator can handle std libraries with debug
+# info, change this to USE_LIBCPP=1
+USE_SYSTEM_STDLIB := 1
 CXX_SOURCES := main.cpp
 include Makefile.rules
Index: 
lldb/test/API/commands/expression/import-std-module/deque-dbg-info-content/Makefile
===
--- 
lldb/test/API/commands/expression/import-std-module/deque-dbg-info-content/Makefile
+++ 
lldb/test/API/commands/expression/import-std-module/deque-dbg-info-content/Makefile
@@ -1,3 +1,5 @@
-USE_LIBCPP := 1
+# FIXME: once the expression evaluator can handle std libraries with debug
+# info, change this to USE_LIBCPP=1
+USE_SYSTEM_STDLIB := 1
 CXX_SOURCES := main.cpp
 include Makefile.rules


Index: lldb/test/API/commands/expression/import-std-module/vector-dbg-info-content/Makefile
===
--- lldb/test/API/commands/expression/import-std-module/vector-dbg-info-content/Makefile
+++ lldb/test/API/commands/expression/import-std-module/vector-dbg-info-content/Makefile
@@ -1,3 +1,5 @@
-USE_LIBCPP := 1
+# FIXME: once the expression evaluator can handle std libraries with debug
+# info, change this to USE_LIBCPP=1
+USE_SYSTEM_STDLIB := 1
 CXX_SOURCES := main.cpp
 include Makefile.rules
Index: lldb/test/API/commands/expression/import-std-module/list-dbg-info-content/Makefile
===
--- lldb/test/API/commands/expression/import-std-module/list-dbg-info-content/Makefile
+++ lldb/test/API/commands/expression/import-std-module/list-dbg-info-content/Makefile
@@ -1,3 +1,5 @@
-USE_LIBCPP := 1
+# FIXME: once the expression evaluator can handle std libraries with debug
+# info, change this to USE_LIBCPP=1
+USE_SYSTEM_STDLIB := 1
 CXX_SOURCES := main.cpp
 include Makefile.rules
Index: lldb/test/API/commands/expression/import-std-module/deque-dbg-info-content/Makefile
===
--- lldb/test/API/commands/expression/import-std-module/deque-dbg-info-content/Makefile
+++ lldb/test/API/commands/expression/import-std-module/deque-dbg-info-content/Makefile
@@ -1,3 +1,5 @@
-USE_LIBCPP := 1
+# FIXME: once the expression evaluator can handle std libraries with debug
+# info, change this to USE_LIBCPP=1
+USE_SYSTEM_STDLIB := 1
 CXX_SOURCES := main.cpp
 include Makefile.rules
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D143265: [lldb] Enable arm64 target for entry values test

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve created this revision.
fdeazeve added a reviewer: Michael137.
Herald added subscribers: omjavaid, kristof.beyls.
Herald added a project: All.
fdeazeve requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This test is supposed to work in arm64.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D143265

Files:
  
lldb/test/API/functionalities/param_entry_vals/basic_entry_values/TestBasicEntryValues.py


Index: 
lldb/test/API/functionalities/param_entry_vals/basic_entry_values/TestBasicEntryValues.py
===
--- 
lldb/test/API/functionalities/param_entry_vals/basic_entry_values/TestBasicEntryValues.py
+++ 
lldb/test/API/functionalities/param_entry_vals/basic_entry_values/TestBasicEntryValues.py
@@ -2,7 +2,7 @@
 from lldbsuite.test.decorators import *
 from lldbsuite.test import lldbplatformutil
 
-supported_archs = ["x86_64", "aarch64"]
+supported_archs = ["x86_64", "aarch64", "arm64", "arm64e"]
 decorators = [skipIf(archs=no_match(supported_archs)),
  skipIf(compiler="clang", compiler_version=['<', '11.0']),
  skipUnlessHasCallSiteInfo,


Index: lldb/test/API/functionalities/param_entry_vals/basic_entry_values/TestBasicEntryValues.py
===
--- lldb/test/API/functionalities/param_entry_vals/basic_entry_values/TestBasicEntryValues.py
+++ lldb/test/API/functionalities/param_entry_vals/basic_entry_values/TestBasicEntryValues.py
@@ -2,7 +2,7 @@
 from lldbsuite.test.decorators import *
 from lldbsuite.test import lldbplatformutil
 
-supported_archs = ["x86_64", "aarch64"]
+supported_archs = ["x86_64", "aarch64", "arm64", "arm64e"]
 decorators = [skipIf(archs=no_match(supported_archs)),
  skipIf(compiler="clang", compiler_version=['<', '11.0']),
  skipUnlessHasCallSiteInfo,
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D143265: [lldb] Enable arm64 target for entry values test

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG2e70da2e431a: [lldb] Enable arm64 target for entry values 
test (authored by fdeazeve).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143265

Files:
  
lldb/test/API/functionalities/param_entry_vals/basic_entry_values/TestBasicEntryValues.py


Index: 
lldb/test/API/functionalities/param_entry_vals/basic_entry_values/TestBasicEntryValues.py
===
--- 
lldb/test/API/functionalities/param_entry_vals/basic_entry_values/TestBasicEntryValues.py
+++ 
lldb/test/API/functionalities/param_entry_vals/basic_entry_values/TestBasicEntryValues.py
@@ -2,7 +2,7 @@
 from lldbsuite.test.decorators import *
 from lldbsuite.test import lldbplatformutil
 
-supported_archs = ["x86_64", "aarch64"]
+supported_archs = ["x86_64", "aarch64", "arm64", "arm64e"]
 decorators = [skipIf(archs=no_match(supported_archs)),
  skipIf(compiler="clang", compiler_version=['<', '11.0']),
  skipUnlessHasCallSiteInfo,


Index: lldb/test/API/functionalities/param_entry_vals/basic_entry_values/TestBasicEntryValues.py
===
--- lldb/test/API/functionalities/param_entry_vals/basic_entry_values/TestBasicEntryValues.py
+++ lldb/test/API/functionalities/param_entry_vals/basic_entry_values/TestBasicEntryValues.py
@@ -2,7 +2,7 @@
 from lldbsuite.test.decorators import *
 from lldbsuite.test import lldbplatformutil
 
-supported_archs = ["x86_64", "aarch64"]
+supported_archs = ["x86_64", "aarch64", "arm64", "arm64e"]
 decorators = [skipIf(archs=no_match(supported_archs)),
  skipIf(compiler="clang", compiler_version=['<', '11.0']),
  skipUnlessHasCallSiteInfo,
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D146714: [lldb] Explicitly set libcxx paths when USE_SYSTEM_LIBCXX is provided

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

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

For tests marked as "USE_SYSTEM_LIBCXX", the expectation is that the
system's standard library should be used. However, the implementation of
this flag is such that we simply don't pass _any_ libcxxx-related flags
to Clang; in turn, Clang will use its defaults.

For a Clang/Libcxx pair compiled together, Clang defaults to:

1. The headers of the sibling libcxx.
2. The libraries of the system.

This mismatch is actually a bug in the driver; once fixed, however, (2)
would point to the sibling libcxx as well, which is _not_ what test
authors intended with the USE_SYSTEM_LIBCXX flag.

As such, this patch explicitly sets a path to the system's libraries.
This change is done only in Apple platforms so that we can test this
works in this case first.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D146714

Files:
  lldb/packages/Python/lldbsuite/test/make/Makefile.rules


Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -428,6 +428,13 @@
endif
 endif
 
+ifeq (1, $(USE_SYSTEM_STDLIB))
+ifeq "$(OS)" "Darwin"
+CXXFLAGS += -nostdlib++ -nostdinc++ -cxx-isystem 
$(SDKROOT)/usr/include/c++/v1
+LDFLAGS += -L$(SDKROOT)/usr/lib -Wl,-rpath,$(SDKROOT)/usr/lib -lc++
+endif
+endif
+
 # If no explicit request was made, but we have paths to a custom libcxx, use
 # them.
 ifeq ($(or $(USE_LIBSTDCPP), $(USE_LIBCPP), $(USE_SYSTEM_STDLIB)),)


Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -428,6 +428,13 @@
 	endif
 endif
 
+ifeq (1, $(USE_SYSTEM_STDLIB))
+ifeq "$(OS)" "Darwin"
+CXXFLAGS += -nostdlib++ -nostdinc++ -cxx-isystem $(SDKROOT)/usr/include/c++/v1
+LDFLAGS += -L$(SDKROOT)/usr/lib -Wl,-rpath,$(SDKROOT)/usr/lib -lc++
+endif
+endif
+
 # If no explicit request was made, but we have paths to a custom libcxx, use
 # them.
 ifeq ($(or $(USE_LIBSTDCPP), $(USE_LIBCPP), $(USE_SYSTEM_STDLIB)),)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D146714: [lldb] Explicitly set libcxx paths when USE_SYSTEM_LIBCXX is provided

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added inline comments.



Comment at: lldb/packages/Python/lldbsuite/test/make/Makefile.rules:434
+CXXFLAGS += -nostdlib++ -nostdinc++ -cxx-isystem 
$(SDKROOT)/usr/include/c++/v1
+LDFLAGS += -L$(SDKROOT)/usr/lib -Wl,-rpath,$(SDKROOT)/usr/lib -lc++
+endif

Michael137 wrote:
> Should we add a hard error if we get here with an empty `SDKROOT`?
I thought about this, but at the start of this file we have this:

```
# Handle SDKROOT on Darwin
#--

ifeq "$(OS)" "Darwin"
ifeq "$(SDKROOT)" ""
# We haven't otherwise set the SDKROOT, so set it now to macosx
SDKROOT := $(shell xcrun --sdk macosx --show-sdk-path)
endif
endif
```

So it would be a bit redundant...
But I think you're right that we could have an error in case someone, someday, 
changes the logic above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146714

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

[Lldb-commits] [PATCH] D146714: [lldb] Explicitly set libcxx paths when USE_SYSTEM_LIBCXX is provided

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve updated this revision to Diff 507711.
fdeazeve added a comment.

Fix typos in commit message.
Add hard error in case SDKROOT is not set.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146714

Files:
  lldb/packages/Python/lldbsuite/test/make/Makefile.rules


Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -428,6 +428,16 @@
endif
 endif
 
+ifeq (1, $(USE_SYSTEM_STDLIB))
+ifeq "$(OS)" "Darwin"
+ifeq "$(SDKROOT)" ""
+ $(error "SDKROOT should have been set")
+endif
+CXXFLAGS += -nostdlib++ -nostdinc++ -cxx-isystem 
$(SDKROOT)/usr/include/c++/v1
+LDFLAGS += -L$(SDKROOT)/usr/lib -Wl,-rpath,$(SDKROOT)/usr/lib -lc++
+endif
+endif
+
 # If no explicit request was made, but we have paths to a custom libcxx, use
 # them.
 ifeq ($(or $(USE_LIBSTDCPP), $(USE_LIBCPP), $(USE_SYSTEM_STDLIB)),)


Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -428,6 +428,16 @@
 	endif
 endif
 
+ifeq (1, $(USE_SYSTEM_STDLIB))
+ifeq "$(OS)" "Darwin"
+ifeq "$(SDKROOT)" ""
+ $(error "SDKROOT should have been set")
+endif
+CXXFLAGS += -nostdlib++ -nostdinc++ -cxx-isystem $(SDKROOT)/usr/include/c++/v1
+LDFLAGS += -L$(SDKROOT)/usr/lib -Wl,-rpath,$(SDKROOT)/usr/lib -lc++
+endif
+endif
+
 # If no explicit request was made, but we have paths to a custom libcxx, use
 # them.
 ifeq ($(or $(USE_LIBSTDCPP), $(USE_LIBCPP), $(USE_SYSTEM_STDLIB)),)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D146714: [lldb] Explicitly set libcxx paths when USE_SYSTEM_LIBCXX is provided

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve updated this revision to Diff 507756.
fdeazeve added a comment.

Improve error message


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146714

Files:
  lldb/packages/Python/lldbsuite/test/make/Makefile.rules


Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -428,6 +428,16 @@
endif
 endif
 
+ifeq (1, $(USE_SYSTEM_STDLIB))
+ifeq "$(OS)" "Darwin"
+ifeq "$(SDKROOT)" ""
+ $(error "SDKROOT must be set on Darwin to use the system liccxx")
+endif
+CXXFLAGS += -nostdlib++ -nostdinc++ -cxx-isystem 
$(SDKROOT)/usr/include/c++/v1
+LDFLAGS += -L$(SDKROOT)/usr/lib -Wl,-rpath,$(SDKROOT)/usr/lib -lc++
+endif
+endif
+
 # If no explicit request was made, but we have paths to a custom libcxx, use
 # them.
 ifeq ($(or $(USE_LIBSTDCPP), $(USE_LIBCPP), $(USE_SYSTEM_STDLIB)),)


Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -428,6 +428,16 @@
 	endif
 endif
 
+ifeq (1, $(USE_SYSTEM_STDLIB))
+ifeq "$(OS)" "Darwin"
+ifeq "$(SDKROOT)" ""
+ $(error "SDKROOT must be set on Darwin to use the system liccxx")
+endif
+CXXFLAGS += -nostdlib++ -nostdinc++ -cxx-isystem $(SDKROOT)/usr/include/c++/v1
+LDFLAGS += -L$(SDKROOT)/usr/lib -Wl,-rpath,$(SDKROOT)/usr/lib -lc++
+endif
+endif
+
 # If no explicit request was made, but we have paths to a custom libcxx, use
 # them.
 ifeq ($(or $(USE_LIBSTDCPP), $(USE_LIBCPP), $(USE_SYSTEM_STDLIB)),)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D146714: [lldb] Explicitly set libcxx paths when USE_SYSTEM_LIBCXX is provided

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve updated this revision to Diff 507757.
fdeazeve added a comment.

Fix typo in error msg


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146714

Files:
  lldb/packages/Python/lldbsuite/test/make/Makefile.rules


Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -428,6 +428,16 @@
endif
 endif
 
+ifeq (1, $(USE_SYSTEM_STDLIB))
+ifeq "$(OS)" "Darwin"
+ifeq "$(SDKROOT)" ""
+ $(error "SDKROOT must be set on Darwin to use the system libcxx")
+endif
+CXXFLAGS += -nostdlib++ -nostdinc++ -cxx-isystem 
$(SDKROOT)/usr/include/c++/v1
+LDFLAGS += -L$(SDKROOT)/usr/lib -Wl,-rpath,$(SDKROOT)/usr/lib -lc++
+endif
+endif
+
 # If no explicit request was made, but we have paths to a custom libcxx, use
 # them.
 ifeq ($(or $(USE_LIBSTDCPP), $(USE_LIBCPP), $(USE_SYSTEM_STDLIB)),)


Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -428,6 +428,16 @@
 	endif
 endif
 
+ifeq (1, $(USE_SYSTEM_STDLIB))
+ifeq "$(OS)" "Darwin"
+ifeq "$(SDKROOT)" ""
+ $(error "SDKROOT must be set on Darwin to use the system libcxx")
+endif
+CXXFLAGS += -nostdlib++ -nostdinc++ -cxx-isystem $(SDKROOT)/usr/include/c++/v1
+LDFLAGS += -L$(SDKROOT)/usr/lib -Wl,-rpath,$(SDKROOT)/usr/lib -lc++
+endif
+endif
+
 # If no explicit request was made, but we have paths to a custom libcxx, use
 # them.
 ifeq ($(or $(USE_LIBSTDCPP), $(USE_LIBCPP), $(USE_SYSTEM_STDLIB)),)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D146714: [lldb] Explicitly set libcxx paths when USE_SYSTEM_LIBCXX is provided

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGc640a146c4ca: [lldb] Explicitly set libcxx paths when 
USE_SYSTEM_STDLIB is provided (authored by fdeazeve).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146714

Files:
  lldb/packages/Python/lldbsuite/test/make/Makefile.rules


Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -428,6 +428,16 @@
endif
 endif
 
+ifeq (1, $(USE_SYSTEM_STDLIB))
+ifeq "$(OS)" "Darwin"
+ifeq "$(SDKROOT)" ""
+ $(error "SDKROOT must be set on Darwin to use the system libcxx")
+endif
+CXXFLAGS += -nostdlib++ -nostdinc++ -cxx-isystem 
$(SDKROOT)/usr/include/c++/v1
+LDFLAGS += -L$(SDKROOT)/usr/lib -Wl,-rpath,$(SDKROOT)/usr/lib -lc++
+endif
+endif
+
 # If no explicit request was made, but we have paths to a custom libcxx, use
 # them.
 ifeq ($(or $(USE_LIBSTDCPP), $(USE_LIBCPP), $(USE_SYSTEM_STDLIB)),)


Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -428,6 +428,16 @@
 	endif
 endif
 
+ifeq (1, $(USE_SYSTEM_STDLIB))
+ifeq "$(OS)" "Darwin"
+ifeq "$(SDKROOT)" ""
+ $(error "SDKROOT must be set on Darwin to use the system libcxx")
+endif
+CXXFLAGS += -nostdlib++ -nostdinc++ -cxx-isystem $(SDKROOT)/usr/include/c++/v1
+LDFLAGS += -L$(SDKROOT)/usr/lib -Wl,-rpath,$(SDKROOT)/usr/lib -lc++
+endif
+endif
+
 # If no explicit request was made, but we have paths to a custom libcxx, use
 # them.
 ifeq ($(or $(USE_LIBSTDCPP), $(USE_LIBCPP), $(USE_SYSTEM_STDLIB)),)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D146977: [lldb-server/linux] Use waitpid(-1) to collect inferior events

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added a comment.

@labath I believe this broke the lldb bots, because the declaration of some 
virtual functions had the const qualifier removed, but not the definition:

  
/Users/buildslave/jenkins/workspace/lldb-cmake@2/llvm-project/lldb/tools/lldb-server/lldb-gdbserver.cpp:79:36:
 error: non-virtual member function marked 'override' hides virtual member 
function
   MainLoop &mainloop) const override {
 ^
  
/Users/buildslave/jenkins/workspace/lldb-cmake@2/llvm-project/lldb/include/lldb/Host/common/NativeProcessProtocol.h:302:5:
 note: hidden overloaded virtual function 
'lldb_private::NativeProcessProtocol::Manager::Launch' declared here: different 
number of parameters (2 vs 3)
  Launch(ProcessLaunchInfo &launch_info,
  ^
  
/Users/buildslave/jenkins/workspace/lldb-cmake@2/llvm-project/lldb/tools/lldb-server/lldb-gdbserver.cpp:84:36:
 error: non-virtual member function marked 'override' hides virtual member 
function
   MainLoop &mainloop) const override {
 ^
  
/Users/buildslave/jenkins/workspace/lldb-cmake@2/llvm-project/lldb/include/lldb/Host/common/NativeProcessProtocol.h:324:5:
 note: hidden overloaded virtual function 
'lldb_private::NativeProcessProtocol::Manager::Attach' declared here: different 
number of parameters (2 vs 3)
  Attach(lldb::pid_t pid, NativeDelegate &native_delegate) = 0;
  ^
  
/Users/buildslave/jenkins/workspace/lldb-cmake@2/llvm-project/lldb/tools/lldb-server/lldb-gdbserver.cpp:434:24:
 error: variable type '(anonymous namespace)::NativeProcessManager' is an 
abstract class
NativeProcessManager manager(mainloop);
 ^
  
/Users/buildslave/jenkins/workspace/lldb-cmake@2/llvm-project/lldb/include/lldb/Host/common/NativeProcessProtocol.h:302:5:
 note: unimplemented pure virtual method 'Launch' in 'NativeProcessManager'
  Launch(ProcessLaunchInfo &launch_info,
  ^
  
/Users/buildslave/jenkins/workspace/lldb-cmake@2/llvm-project/lldb/include/lldb/Host/common/NativeProcessProtocol.h:324:5:
 note: unimplemented pure virtual method 'Attach' in 'NativeProcessManager'
  Attach(lldb::pid_t pid, NativeDelegate &native_delegate) = 0;

Could you have a look? 
https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/53015/console


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146977

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

[Lldb-commits] [PATCH] D146977: [lldb-server/linux] Use waitpid(-1) to collect inferior events

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added a comment.

Jonas fixed the build failures in c8522cadf7331bedd548ad5e2c6ef100c9166259 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146977

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

[Lldb-commits] [PATCH] D148175: [lldb] Add `operator StringRef` to ConstString

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added a comment.

> since ConstStrings live forever

So long as this is true, I don't see any pitfalls either.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148175

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

[Lldb-commits] [PATCH] D150219: [lldb][NFCI] Remove n^2 loops and simplify iterator usage

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

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

The code inside Broadcaster makes usage of iterators using olden C++ coding
style. Hidden in this old style is a couple of N^2 loops: we iterate over a map
(sequentially), removing the first element that matches some predicate. The
search is _always_ done from the start of the map, which implies that, if the
map has N elements and if all matches happen on the second half of the map, then
we visit the first N/2 elements exactly N/2 * N/2 times.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D150219

Files:
  lldb/source/Utility/Broadcaster.cpp

Index: lldb/source/Utility/Broadcaster.cpp
===
--- lldb/source/Utility/Broadcaster.cpp
+++ lldb/source/Utility/Broadcaster.cpp
@@ -377,13 +377,10 @@
 
   // Go through the map and delete the exact matches, and build a list of
   // matches that weren't exact to re-add:
-  while (true) {
-collection::iterator iter, end_iter = m_event_map.end();
-iter = find_if(m_event_map.begin(), end_iter,
-   listener_matches_and_shared_bits);
-if (iter == end_iter) {
+  for (auto begin = m_event_map.begin(), end = m_event_map.end();;) {
+auto iter = find_if(begin, end, listener_matches_and_shared_bits);
+if (iter == m_event_map.end())
   break;
-}
 uint32_t iter_event_bits = (*iter).first.GetEventBits();
 removed_some = true;
 
@@ -392,12 +389,12 @@
   to_be_readded.emplace_back(event_spec.GetBroadcasterClass(),
  new_event_bits);
 }
-m_event_map.erase(iter);
+iter = m_event_map.erase(iter);
   }
 
   // Okay now add back the bits that weren't completely removed:
-  for (size_t i = 0; i < to_be_readded.size(); i++) {
-m_event_map.insert(event_listener_key(to_be_readded[i], listener_sp));
+  for (const auto &event : to_be_readded) {
+m_event_map.insert(event_listener_key(event, listener_sp));
   }
 
   return removed_some;
@@ -412,9 +409,8 @@
 return input.first.IsContainedIn(event_spec);
   };
 
-  collection::const_iterator iter, end_iter = m_event_map.end();
-  iter = find_if(m_event_map.begin(), end_iter, event_spec_matches);
-  if (iter != end_iter)
+  auto iter = llvm::find_if(m_event_map, event_spec_matches);
+  if (iter != m_event_map.end())
 return (*iter).second;
 
   return nullptr;
@@ -427,24 +423,20 @@
 return input.get() == listener;
   };
 
-  listener_collection::iterator iter = m_listeners.begin(),
-end_iter = m_listeners.end();
-
-  iter = std::find_if(iter, end_iter, listeners_predicate);
-  if (iter != end_iter)
+  if (auto iter = llvm::find_if(m_listeners, listeners_predicate);
+  iter != m_listeners.end())
 m_listeners.erase(iter);
 
-  while (true) {
-auto events_predicate =
-[&listener](const event_listener_key &input) -> bool {
-  return input.second.get() == listener;
-};
-collection::iterator iter, end_iter = m_event_map.end();
-iter = find_if(m_event_map.begin(), end_iter, events_predicate);
-if (iter == end_iter)
+  auto events_predicate = [listener](const event_listener_key &input) -> bool {
+return input.second.get() == listener;
+  };
+
+  for (auto iter = m_event_map.begin(), end = m_event_map.end();;) {
+iter = find_if(iter, end, events_predicate);
+if (iter == m_event_map.end())
   break;
 
-m_event_map.erase(iter);
+iter = m_event_map.erase(iter);
   }
 }
 
@@ -459,13 +451,12 @@
   if (m_listeners.erase(listener_sp) == 0)
 return;
 
-  while (true) {
-collection::iterator iter, end_iter = m_event_map.end();
-iter = find_if(m_event_map.begin(), end_iter, listener_matches);
+  for (auto iter = m_event_map.begin(), end_iter = m_event_map.end();;) {
+iter = find_if(iter, end_iter, listener_matches);
 if (iter == end_iter)
   break;
 
-m_event_map.erase(iter);
+iter = m_event_map.erase(iter);
   }
 }
 
@@ -490,11 +481,9 @@
 
 void BroadcasterManager::Clear() {
   std::lock_guard guard(m_manager_mutex);
-  listener_collection::iterator end_iter = m_listeners.end();
 
-  for (listener_collection::iterator iter = m_listeners.begin();
-   iter != end_iter; iter++)
-(*iter)->BroadcasterManagerWillDestruct(this->shared_from_this());
+  for (auto &listener : m_listeners)
+listener->BroadcasterManagerWillDestruct(this->shared_from_this());
   m_listeners.clear();
   m_event_map.clear();
 }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D150219: [lldb][NFCI] Remove n^2 loops and simplify iterator usage

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve updated this revision to Diff 520786.
fdeazeve added a comment.
Herald added a subscriber: JDevlieghere.

Update commit message


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150219

Files:
  lldb/source/Utility/Broadcaster.cpp

Index: lldb/source/Utility/Broadcaster.cpp
===
--- lldb/source/Utility/Broadcaster.cpp
+++ lldb/source/Utility/Broadcaster.cpp
@@ -377,13 +377,10 @@
 
   // Go through the map and delete the exact matches, and build a list of
   // matches that weren't exact to re-add:
-  while (true) {
-collection::iterator iter, end_iter = m_event_map.end();
-iter = find_if(m_event_map.begin(), end_iter,
-   listener_matches_and_shared_bits);
-if (iter == end_iter) {
+  for (auto begin = m_event_map.begin(), end = m_event_map.end();;) {
+auto iter = find_if(begin, end, listener_matches_and_shared_bits);
+if (iter == m_event_map.end())
   break;
-}
 uint32_t iter_event_bits = (*iter).first.GetEventBits();
 removed_some = true;
 
@@ -392,12 +389,12 @@
   to_be_readded.emplace_back(event_spec.GetBroadcasterClass(),
  new_event_bits);
 }
-m_event_map.erase(iter);
+iter = m_event_map.erase(iter);
   }
 
   // Okay now add back the bits that weren't completely removed:
-  for (size_t i = 0; i < to_be_readded.size(); i++) {
-m_event_map.insert(event_listener_key(to_be_readded[i], listener_sp));
+  for (const auto &event : to_be_readded) {
+m_event_map.insert(event_listener_key(event, listener_sp));
   }
 
   return removed_some;
@@ -412,9 +409,8 @@
 return input.first.IsContainedIn(event_spec);
   };
 
-  collection::const_iterator iter, end_iter = m_event_map.end();
-  iter = find_if(m_event_map.begin(), end_iter, event_spec_matches);
-  if (iter != end_iter)
+  auto iter = llvm::find_if(m_event_map, event_spec_matches);
+  if (iter != m_event_map.end())
 return (*iter).second;
 
   return nullptr;
@@ -427,24 +423,20 @@
 return input.get() == listener;
   };
 
-  listener_collection::iterator iter = m_listeners.begin(),
-end_iter = m_listeners.end();
-
-  iter = std::find_if(iter, end_iter, listeners_predicate);
-  if (iter != end_iter)
+  if (auto iter = llvm::find_if(m_listeners, listeners_predicate);
+  iter != m_listeners.end())
 m_listeners.erase(iter);
 
-  while (true) {
-auto events_predicate =
-[&listener](const event_listener_key &input) -> bool {
-  return input.second.get() == listener;
-};
-collection::iterator iter, end_iter = m_event_map.end();
-iter = find_if(m_event_map.begin(), end_iter, events_predicate);
-if (iter == end_iter)
+  auto events_predicate = [listener](const event_listener_key &input) -> bool {
+return input.second.get() == listener;
+  };
+
+  for (auto iter = m_event_map.begin(), end = m_event_map.end();;) {
+iter = find_if(iter, end, events_predicate);
+if (iter == m_event_map.end())
   break;
 
-m_event_map.erase(iter);
+iter = m_event_map.erase(iter);
   }
 }
 
@@ -459,13 +451,12 @@
   if (m_listeners.erase(listener_sp) == 0)
 return;
 
-  while (true) {
-collection::iterator iter, end_iter = m_event_map.end();
-iter = find_if(m_event_map.begin(), end_iter, listener_matches);
+  for (auto iter = m_event_map.begin(), end_iter = m_event_map.end();;) {
+iter = find_if(iter, end_iter, listener_matches);
 if (iter == end_iter)
   break;
 
-m_event_map.erase(iter);
+iter = m_event_map.erase(iter);
   }
 }
 
@@ -490,11 +481,9 @@
 
 void BroadcasterManager::Clear() {
   std::lock_guard guard(m_manager_mutex);
-  listener_collection::iterator end_iter = m_listeners.end();
 
-  for (listener_collection::iterator iter = m_listeners.begin();
-   iter != end_iter; iter++)
-(*iter)->BroadcasterManagerWillDestruct(this->shared_from_this());
+  for (auto &listener : m_listeners)
+listener->BroadcasterManagerWillDestruct(this->shared_from_this());
   m_listeners.clear();
   m_event_map.clear();
 }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D150219: [lldb][NFCI] Remove n^2 loops and simplify iterator usage

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added a comment.

Heavily inspired by @bulbazord's D150168 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150219

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

[Lldb-commits] [PATCH] D150219: [lldb][NFCI] Remove n^2 loops and simplify iterator usage

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added inline comments.



Comment at: lldb/source/Utility/Broadcaster.cpp:382
+auto iter = find_if(begin, end, listener_matches_and_shared_bits);
+if (iter == m_event_map.end())
   break;

Oh, this should be `== end`



Comment at: lldb/source/Utility/Broadcaster.cpp:436
+iter = find_if(iter, end, events_predicate);
+if (iter == m_event_map.end())
   break;

this should be `== end`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150219

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

[Lldb-commits] [PATCH] D150219: [lldb][NFCI] Remove n^2 loops and simplify iterator usage

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added inline comments.



Comment at: lldb/source/Utility/Broadcaster.cpp:430
 
-  while (true) {
-auto events_predicate =
-[&listener](const event_listener_key &input) -> bool {
-  return input.second.get() == listener;
-};
-collection::iterator iter, end_iter = m_event_map.end();
-iter = find_if(m_event_map.begin(), end_iter, events_predicate);
-if (iter == end_iter)
+  auto events_predicate = [listener](const event_listener_key &input) -> bool {
+return input.second.get() == listener;

mib wrote:
> 
`listener` is a raw pointer, we shouldn't capture those by reference


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150219

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

[Lldb-commits] [PATCH] D150219: [lldb][NFCI] Remove n^2 loops and simplify iterator usage

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve updated this revision to Diff 520796.
fdeazeve edited the summary of this revision.
fdeazeve added a comment.

Address review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150219

Files:
  lldb/source/Utility/Broadcaster.cpp

Index: lldb/source/Utility/Broadcaster.cpp
===
--- lldb/source/Utility/Broadcaster.cpp
+++ lldb/source/Utility/Broadcaster.cpp
@@ -377,13 +377,10 @@
 
   // Go through the map and delete the exact matches, and build a list of
   // matches that weren't exact to re-add:
-  while (true) {
-collection::iterator iter, end_iter = m_event_map.end();
-iter = find_if(m_event_map.begin(), end_iter,
-   listener_matches_and_shared_bits);
-if (iter == end_iter) {
+  for (auto iter = m_event_map.begin(), end = m_event_map.end();;) {
+iter = find_if(iter, end, listener_matches_and_shared_bits);
+if (iter == end)
   break;
-}
 uint32_t iter_event_bits = (*iter).first.GetEventBits();
 removed_some = true;
 
@@ -392,12 +389,12 @@
   to_be_readded.emplace_back(event_spec.GetBroadcasterClass(),
  new_event_bits);
 }
-m_event_map.erase(iter);
+iter = m_event_map.erase(iter);
   }
 
   // Okay now add back the bits that weren't completely removed:
-  for (size_t i = 0; i < to_be_readded.size(); i++) {
-m_event_map.insert(event_listener_key(to_be_readded[i], listener_sp));
+  for (const auto &event : to_be_readded) {
+m_event_map.insert(event_listener_key(event, listener_sp));
   }
 
   return removed_some;
@@ -412,9 +409,8 @@
 return input.first.IsContainedIn(event_spec);
   };
 
-  collection::const_iterator iter, end_iter = m_event_map.end();
-  iter = find_if(m_event_map.begin(), end_iter, event_spec_matches);
-  if (iter != end_iter)
+  auto iter = llvm::find_if(m_event_map, event_spec_matches);
+  if (iter != m_event_map.end())
 return (*iter).second;
 
   return nullptr;
@@ -427,24 +423,21 @@
 return input.get() == listener;
   };
 
-  listener_collection::iterator iter = m_listeners.begin(),
-end_iter = m_listeners.end();
-
-  iter = std::find_if(iter, end_iter, listeners_predicate);
-  if (iter != end_iter)
+  if (auto iter = llvm::find_if(m_listeners, listeners_predicate);
+  iter != m_listeners.end())
 m_listeners.erase(iter);
 
-  while (true) {
-auto events_predicate =
-[&listener](const event_listener_key &input) -> bool {
-  return input.second.get() == listener;
-};
-collection::iterator iter, end_iter = m_event_map.end();
-iter = find_if(m_event_map.begin(), end_iter, events_predicate);
-if (iter == end_iter)
+  auto events_predicate = [listener](const event_listener_key &input) -> bool {
+return input.second.get() == listener;
+  };
+
+  // TODO: use 'std::map::erase_if' when moving to c++20.
+  for (auto iter = m_event_map.begin(), end = m_event_map.end();;) {
+iter = find_if(iter, end, events_predicate);
+if (iter == end)
   break;
 
-m_event_map.erase(iter);
+iter = m_event_map.erase(iter);
   }
 }
 
@@ -459,13 +452,13 @@
   if (m_listeners.erase(listener_sp) == 0)
 return;
 
-  while (true) {
-collection::iterator iter, end_iter = m_event_map.end();
-iter = find_if(m_event_map.begin(), end_iter, listener_matches);
+  // TODO: use 'std::map::erase_if' when moving to c++20.
+  for (auto iter = m_event_map.begin(), end_iter = m_event_map.end();;) {
+iter = find_if(iter, end_iter, listener_matches);
 if (iter == end_iter)
   break;
 
-m_event_map.erase(iter);
+iter = m_event_map.erase(iter);
   }
 }
 
@@ -490,11 +483,9 @@
 
 void BroadcasterManager::Clear() {
   std::lock_guard guard(m_manager_mutex);
-  listener_collection::iterator end_iter = m_listeners.end();
 
-  for (listener_collection::iterator iter = m_listeners.begin();
-   iter != end_iter; iter++)
-(*iter)->BroadcasterManagerWillDestruct(this->shared_from_this());
+  for (auto &listener : m_listeners)
+listener->BroadcasterManagerWillDestruct(this->shared_from_this());
   m_listeners.clear();
   m_event_map.clear();
 }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D150219: [lldb][NFCI] Remove n^2 loops and simplify iterator usage

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added inline comments.



Comment at: lldb/source/Utility/Broadcaster.cpp:381
+  for (auto begin = m_event_map.begin(), end = m_event_map.end();;) {
+auto iter = find_if(begin, end, listener_matches_and_shared_bits);
+if (iter == m_event_map.end())

jingham wrote:
> Why do you re-look-up `m_event_map.end()` instead of using the `end` variable 
> you defined in the `for` initializer?
> 
> std::map::erase says it only invalidates iterators to the erased element, so 
> you should be fine to use `end` here (and since you do exactly that in the 
> previous line) it should be good here too.
Yup, this was just a mistake on my part. Will address in the next version



Comment at: lldb/source/Utility/Broadcaster.cpp:392
 }
-m_event_map.erase(iter);
+iter = m_event_map.erase(iter);
   }

bulbazord wrote:
> I don't think you need to actually capture the iterator here? std::map::erase 
> doesn't invalidate existing iterators* (so `begin` and `end` are fine) and we 
> redefine `iter` at the beginning of this loop.
> 
> *:https://en.cppreference.com/w/cpp/container/map/erase
Argh, the variable `begin` shouldn't exist, it should all be `iter`, otherwise 
we're not addressing the quadratic behaviour.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150219

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

[Lldb-commits] [PATCH] D150222: [lldb][NFCI] Remove custom dwarf LEB128 types

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added a comment.

Nice cleanup! This gets rid of some implicit conversions


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150222

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

[Lldb-commits] [PATCH] D150219: [lldb][NFCI] Remove n^2 loops and simplify iterator usage

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG25495c9b4c05: [lldb][NFCI] Remove n^2 loops and simplify 
iterator usage (authored by fdeazeve).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150219

Files:
  lldb/source/Utility/Broadcaster.cpp

Index: lldb/source/Utility/Broadcaster.cpp
===
--- lldb/source/Utility/Broadcaster.cpp
+++ lldb/source/Utility/Broadcaster.cpp
@@ -377,13 +377,10 @@
 
   // Go through the map and delete the exact matches, and build a list of
   // matches that weren't exact to re-add:
-  while (true) {
-collection::iterator iter, end_iter = m_event_map.end();
-iter = find_if(m_event_map.begin(), end_iter,
-   listener_matches_and_shared_bits);
-if (iter == end_iter) {
+  for (auto iter = m_event_map.begin(), end = m_event_map.end();;) {
+iter = find_if(iter, end, listener_matches_and_shared_bits);
+if (iter == end)
   break;
-}
 uint32_t iter_event_bits = (*iter).first.GetEventBits();
 removed_some = true;
 
@@ -392,12 +389,12 @@
   to_be_readded.emplace_back(event_spec.GetBroadcasterClass(),
  new_event_bits);
 }
-m_event_map.erase(iter);
+iter = m_event_map.erase(iter);
   }
 
   // Okay now add back the bits that weren't completely removed:
-  for (size_t i = 0; i < to_be_readded.size(); i++) {
-m_event_map.insert(event_listener_key(to_be_readded[i], listener_sp));
+  for (const auto &event : to_be_readded) {
+m_event_map.insert(event_listener_key(event, listener_sp));
   }
 
   return removed_some;
@@ -412,9 +409,8 @@
 return input.first.IsContainedIn(event_spec);
   };
 
-  collection::const_iterator iter, end_iter = m_event_map.end();
-  iter = find_if(m_event_map.begin(), end_iter, event_spec_matches);
-  if (iter != end_iter)
+  auto iter = llvm::find_if(m_event_map, event_spec_matches);
+  if (iter != m_event_map.end())
 return (*iter).second;
 
   return nullptr;
@@ -427,24 +423,21 @@
 return input.get() == listener;
   };
 
-  listener_collection::iterator iter = m_listeners.begin(),
-end_iter = m_listeners.end();
-
-  iter = std::find_if(iter, end_iter, listeners_predicate);
-  if (iter != end_iter)
+  if (auto iter = llvm::find_if(m_listeners, listeners_predicate);
+  iter != m_listeners.end())
 m_listeners.erase(iter);
 
-  while (true) {
-auto events_predicate =
-[&listener](const event_listener_key &input) -> bool {
-  return input.second.get() == listener;
-};
-collection::iterator iter, end_iter = m_event_map.end();
-iter = find_if(m_event_map.begin(), end_iter, events_predicate);
-if (iter == end_iter)
+  auto events_predicate = [listener](const event_listener_key &input) -> bool {
+return input.second.get() == listener;
+  };
+
+  // TODO: use 'std::map::erase_if' when moving to c++20.
+  for (auto iter = m_event_map.begin(), end = m_event_map.end();;) {
+iter = find_if(iter, end, events_predicate);
+if (iter == end)
   break;
 
-m_event_map.erase(iter);
+iter = m_event_map.erase(iter);
   }
 }
 
@@ -459,13 +452,13 @@
   if (m_listeners.erase(listener_sp) == 0)
 return;
 
-  while (true) {
-collection::iterator iter, end_iter = m_event_map.end();
-iter = find_if(m_event_map.begin(), end_iter, listener_matches);
+  // TODO: use 'std::map::erase_if' when moving to c++20.
+  for (auto iter = m_event_map.begin(), end_iter = m_event_map.end();;) {
+iter = find_if(iter, end_iter, listener_matches);
 if (iter == end_iter)
   break;
 
-m_event_map.erase(iter);
+iter = m_event_map.erase(iter);
   }
 }
 
@@ -490,11 +483,9 @@
 
 void BroadcasterManager::Clear() {
   std::lock_guard guard(m_manager_mutex);
-  listener_collection::iterator end_iter = m_listeners.end();
 
-  for (listener_collection::iterator iter = m_listeners.begin();
-   iter != end_iter; iter++)
-(*iter)->BroadcasterManagerWillDestruct(this->shared_from_this());
+  for (auto &listener : m_listeners)
+listener->BroadcasterManagerWillDestruct(this->shared_from_this());
   m_listeners.clear();
   m_event_map.clear();
 }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D150228: [lldb][NFCI] Replace dw_form_t with llvm::dwarf::Form

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve accepted this revision.
fdeazeve added a comment.

LGTM! I like how the static_cast now is explicit about the fact that some 
truncation is going on from the ULEB reading.

Do you have plans to do something similar for the attribute typedef?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150228

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

[Lldb-commits] [PATCH] D150363: [lldb][nfc] Simplify DebugRanges class

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

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

Most of the code changed here dates back to 2010, when LLDB was first
introduced upstream, as such it benefits from a slight cleanup.

The method "dump" is not used anywhere nor is it tested, so this commit removes
it.

The "findRanges" method returns a boolean which is never checked and indicates
whether the method found anything/assigned a range map to the out parameter.
This commit folds the out parameter into the return type of the method.

A handful of typedefs were also never used and therefore removed.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D150363

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp

Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
@@ -1032,9 +1032,7 @@
 if (!debug_ranges)
   return llvm::make_error(
   "No debug_ranges section");
-DWARFRangeList ranges;
-debug_ranges->FindRanges(this, offset, ranges);
-return ranges;
+return debug_ranges->FindRanges(this, offset);
   }
 
   if (!GetRnglistTable())
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.h
@@ -22,21 +22,14 @@
   DWARFDebugRanges();
 
   void Extract(lldb_private::DWARFContext &context);
-  bool FindRanges(const DWARFUnit *cu, dw_offset_t debug_ranges_offset,
-  DWARFRangeList &range_list) const;
-
-  static void Dump(lldb_private::Stream &s,
-   const lldb_private::DWARFDataExtractor &debug_ranges_data,
-   lldb::offset_t *offset_ptr, dw_addr_t cu_base_addr);
+  DWARFRangeList FindRanges(const DWARFUnit *cu,
+dw_offset_t debug_ranges_offset) const;
 
 protected:
   bool Extract(lldb_private::DWARFContext &context, lldb::offset_t *offset_ptr,
DWARFRangeList &range_list);
 
-  typedef std::map range_map;
-  typedef range_map::iterator range_map_iterator;
-  typedef range_map::const_iterator range_map_const_iterator;
-  range_map m_range_map;
+  std::map m_range_map;
 };
 
 #endif // LLDB_SOURCE_PLUGINS_SYMBOLFILE_DWARF_DWARFDEBUGRANGES_H
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.cpp
@@ -8,7 +8,6 @@
 
 #include "DWARFDebugRanges.h"
 #include "DWARFUnit.h"
-#include "lldb/Utility/Stream.h"
 
 using namespace lldb_private;
 
@@ -72,55 +71,17 @@
   return range_offset != *offset_ptr;
 }
 
-void DWARFDebugRanges::Dump(Stream &s,
-const DWARFDataExtractor &debug_ranges_data,
-lldb::offset_t *offset_ptr,
-dw_addr_t cu_base_addr) {
-  uint32_t addr_size = s.GetAddressByteSize();
-
-  dw_addr_t base_addr = cu_base_addr;
-  while (
-  debug_ranges_data.ValidOffsetForDataOfSize(*offset_ptr, 2 * addr_size)) {
-dw_addr_t begin = debug_ranges_data.GetMaxU64(offset_ptr, addr_size);
-dw_addr_t end = debug_ranges_data.GetMaxU64(offset_ptr, addr_size);
-// Extend 4 byte addresses that consists of 32 bits of 1's to be 64 bits of
-// ones
-if (begin == 0xull && addr_size == 4)
-  begin = LLDB_INVALID_ADDRESS;
-
-s.Indent();
-if (begin == 0 && end == 0) {
-  s.PutCString(" End");
-  break;
-} else if (begin == LLDB_INVALID_ADDRESS) {
-  // A base address selection entry
-  base_addr = end;
-  DumpAddress(s.AsRawOstream(), base_addr, sizeof(dw_addr_t),
-  " Base address = ");
-} else {
-  // Convert from offset to an address
-  dw_addr_t begin_addr = begin + base_addr;
-  dw_addr_t end_addr = end + base_addr;
-
-  DumpAddressRange(s.AsRawOstream(), begin_addr, end_addr,
-   sizeof(dw_addr_t), nullptr);
-}
-  }
-}
-
-bool DWARFDebugRanges::FindRanges(const DWARFUnit *cu,
-  dw_offset_t debug_ranges_offset,
-  DWARFRangeList &range_list) const {
+DWARFRangeList
+DWARFDebugRanges::FindRanges(const DWARFUnit *cu,
+ dw_offset_t debug_ranges_offset) const {
   dw_addr_t debug_ranges_address = cu->GetRangesBase() + debug_ranges_offset;
-  range_map_const_iterator pos = m_range_map.find(debug_ranges_address);
-  if (pos != m

[Lldb-commits] [PATCH] D150366: [lldb][NFCI] Use llvm's libDebugInfo for DebugRanges

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

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

In an effort to unify the different dwarf parsers available in the codebase,
this commit removes LLDB's custom parsing for the `.debug_ranges` DWARF section,
instead calling into LLVM's parser.

Subsequent work should look into unifying `llvm::DWARDebugRangeList` (whose
entries are pairs of (start, end) addresses) with `lldb::DWARFRangeList` (whose
entries are pairs of (start, length)). The lists themselves are also different
data structures, but functionally equivalent.

Depends on D150363 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D150366

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.h

Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.h
@@ -26,9 +26,6 @@
 dw_offset_t debug_ranges_offset) const;
 
 protected:
-  bool Extract(lldb_private::DWARFContext &context, lldb::offset_t *offset_ptr,
-   DWARFRangeList &range_list);
-
   std::map m_range_map;
 };
 
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.cpp
@@ -8,67 +8,34 @@
 
 #include "DWARFDebugRanges.h"
 #include "DWARFUnit.h"
+#include "llvm/DebugInfo/DWARF/DWARFDebugRangeList.h"
 
 using namespace lldb_private;
 
-static dw_addr_t GetBaseAddressMarker(uint32_t addr_size) {
-  switch(addr_size) {
-case 2:
-  return 0x;
-case 4:
-  return 0x;
-case 8:
-  return 0x;
-  }
-  llvm_unreachable("GetBaseAddressMarker unsupported address size.");
-}
-
 DWARFDebugRanges::DWARFDebugRanges() : m_range_map() {}
 
 void DWARFDebugRanges::Extract(DWARFContext &context) {
-  DWARFRangeList range_list;
-  lldb::offset_t offset = 0;
-  dw_offset_t debug_ranges_offset = offset;
-  while (Extract(context, &offset, range_list)) {
-range_list.Sort();
-m_range_map[debug_ranges_offset] = range_list;
-debug_ranges_offset = offset;
-  }
-}
-
-bool DWARFDebugRanges::Extract(DWARFContext &context,
-   lldb::offset_t *offset_ptr,
-   DWARFRangeList &range_list) {
-  range_list.Clear();
-
-  lldb::offset_t range_offset = *offset_ptr;
-  const DWARFDataExtractor &debug_ranges_data = context.getOrLoadRangesData();
-  uint32_t addr_size = debug_ranges_data.GetAddressByteSize();
-  dw_addr_t base_addr = 0;
-  dw_addr_t base_addr_marker = GetBaseAddressMarker(addr_size);
-
-  while (
-  debug_ranges_data.ValidOffsetForDataOfSize(*offset_ptr, 2 * addr_size)) {
-dw_addr_t begin = debug_ranges_data.GetMaxU64(offset_ptr, addr_size);
-dw_addr_t end = debug_ranges_data.GetMaxU64(offset_ptr, addr_size);
-
-if (!begin && !end) {
-  // End of range list
-  break;
-}
-
-if (begin == base_addr_marker) {
-  base_addr = end;
-  continue;
+  llvm::DWARFDataExtractor extractor =
+  context.getOrLoadRangesData().GetAsLLVM();
+  llvm::DWARFDebugRangeList extracted_list;
+  uint64_t current_offset = 0;
+  auto extract_next_list = [&] {
+if (auto error = extracted_list.extract(extractor, ¤t_offset)) {
+  consumeError(std::move(error));
+  return false;
 }
-
-// Filter out empty ranges
-if (begin < end)
-  range_list.Append(DWARFRangeList::Entry(begin + base_addr, end - begin));
+return true;
+  };
+
+  uint64_t previous_offset = current_offset;
+  while (extractor.isValidOffset(current_offset) && extract_next_list()) {
+DWARFRangeList &lldb_range_list = m_range_map[previous_offset];
+for (auto &range : extracted_list.getEntries())
+  lldb_range_list.Append(range.StartAddress,
+ range.EndAddress - range.StartAddress);
+lldb_range_list.Sort();
+previous_offset = current_offset;
   }
-
-  // Make sure we consumed at least something
-  return range_offset != *offset_ptr;
 }
 
 DWARFRangeList
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D150363: [lldb][nfc] Simplify DebugRanges class

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.h:32
 
-  typedef std::map range_map;
-  typedef range_map::iterator range_map_iterator;
-  typedef range_map::const_iterator range_map_const_iterator;
-  range_map m_range_map;
+  std::map m_range_map;
 };

rastogishubham wrote:
> Any reason why the name was changed to `m_range_map`?
It was also `m_range_map` before. Maybe you're confusing it with name of the 
typedef that got deleted? The diff highlighting here is misleading


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150363

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

[Lldb-commits] [PATCH] D150299: [lldb][NFCI] Redefine dw_attr_t typedef with llvm::dwarf::Attribute

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

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

LGTM! Sorry for the delay!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150299

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

[Lldb-commits] [PATCH] D150402: [lldb][NFCI] Change return type of DWARFDebugInfoEntry::GetAttributes

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

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

Thanks for doing this! In particular, all the early returns are very welcome :)

I suspect you already have this in your radar, but `ExtractFormValueAtIndex` 
could probably return an `optional` to cleanup some code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150402

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

[Lldb-commits] [PATCH] D150363: [lldb][nfc] Simplify DebugRanges class

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG81be68062f49: [lldb][nfc] Simplify DebugRanges class 
(authored by fdeazeve).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150363

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp

Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
@@ -1032,9 +1032,7 @@
 if (!debug_ranges)
   return llvm::make_error(
   "No debug_ranges section");
-DWARFRangeList ranges;
-debug_ranges->FindRanges(this, offset, ranges);
-return ranges;
+return debug_ranges->FindRanges(this, offset);
   }
 
   if (!GetRnglistTable())
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.h
@@ -22,21 +22,14 @@
   DWARFDebugRanges();
 
   void Extract(lldb_private::DWARFContext &context);
-  bool FindRanges(const DWARFUnit *cu, dw_offset_t debug_ranges_offset,
-  DWARFRangeList &range_list) const;
-
-  static void Dump(lldb_private::Stream &s,
-   const lldb_private::DWARFDataExtractor &debug_ranges_data,
-   lldb::offset_t *offset_ptr, dw_addr_t cu_base_addr);
+  DWARFRangeList FindRanges(const DWARFUnit *cu,
+dw_offset_t debug_ranges_offset) const;
 
 protected:
   bool Extract(lldb_private::DWARFContext &context, lldb::offset_t *offset_ptr,
DWARFRangeList &range_list);
 
-  typedef std::map range_map;
-  typedef range_map::iterator range_map_iterator;
-  typedef range_map::const_iterator range_map_const_iterator;
-  range_map m_range_map;
+  std::map m_range_map;
 };
 
 #endif // LLDB_SOURCE_PLUGINS_SYMBOLFILE_DWARF_DWARFDEBUGRANGES_H
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.cpp
@@ -8,7 +8,6 @@
 
 #include "DWARFDebugRanges.h"
 #include "DWARFUnit.h"
-#include "lldb/Utility/Stream.h"
 
 using namespace lldb_private;
 
@@ -72,55 +71,17 @@
   return range_offset != *offset_ptr;
 }
 
-void DWARFDebugRanges::Dump(Stream &s,
-const DWARFDataExtractor &debug_ranges_data,
-lldb::offset_t *offset_ptr,
-dw_addr_t cu_base_addr) {
-  uint32_t addr_size = s.GetAddressByteSize();
-
-  dw_addr_t base_addr = cu_base_addr;
-  while (
-  debug_ranges_data.ValidOffsetForDataOfSize(*offset_ptr, 2 * addr_size)) {
-dw_addr_t begin = debug_ranges_data.GetMaxU64(offset_ptr, addr_size);
-dw_addr_t end = debug_ranges_data.GetMaxU64(offset_ptr, addr_size);
-// Extend 4 byte addresses that consists of 32 bits of 1's to be 64 bits of
-// ones
-if (begin == 0xull && addr_size == 4)
-  begin = LLDB_INVALID_ADDRESS;
-
-s.Indent();
-if (begin == 0 && end == 0) {
-  s.PutCString(" End");
-  break;
-} else if (begin == LLDB_INVALID_ADDRESS) {
-  // A base address selection entry
-  base_addr = end;
-  DumpAddress(s.AsRawOstream(), base_addr, sizeof(dw_addr_t),
-  " Base address = ");
-} else {
-  // Convert from offset to an address
-  dw_addr_t begin_addr = begin + base_addr;
-  dw_addr_t end_addr = end + base_addr;
-
-  DumpAddressRange(s.AsRawOstream(), begin_addr, end_addr,
-   sizeof(dw_addr_t), nullptr);
-}
-  }
-}
-
-bool DWARFDebugRanges::FindRanges(const DWARFUnit *cu,
-  dw_offset_t debug_ranges_offset,
-  DWARFRangeList &range_list) const {
+DWARFRangeList
+DWARFDebugRanges::FindRanges(const DWARFUnit *cu,
+ dw_offset_t debug_ranges_offset) const {
   dw_addr_t debug_ranges_address = cu->GetRangesBase() + debug_ranges_offset;
-  range_map_const_iterator pos = m_range_map.find(debug_ranges_address);
-  if (pos != m_range_map.end()) {
-range_list = pos->second;
-
-// All DW_AT_ranges are relative to the base address of the compile
-// unit. We add the compile unit base address to make sure all the
-// addresses are properly fixed up.
-range_list.Slide(cu->GetBaseAddress());
-return true;
-  }
-  return false;
+  auto pos = m_range_map.find(debug_ranges_address);
+  DWARFRangeList ans =
+  pos == m_range_map.end() ? DWARFRangeList() : 

[Lldb-commits] [PATCH] D150366: [lldb][NFCI] Use llvm's libDebugInfo for DebugRanges

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added a comment.

In D150366#4336168 , @bulbazord wrote:

> Ok, this looks like it's doing the same thing to me which is good. My 
> understanding of this change is that you're changing 
> `lldb::DWARFDebugRanges::Extract` to use `llvm::DWARFDebugRangeList` instead 
> of `lldb::DWARFRangeList`.
>
> Out of curiosity, do you have an idea of the change to performance (if any)? 
> I wouldn't expect it to be very different if at all because I don't think the 
> algorithms between lldb and llvm are different but it would be nice to make 
> sure.

I ran an experiment with a C++ project that forced LLDB to parse roughly 30,000 
range entries and no significant differences were detected:

  // LLDB's implementation
   Time (mean ± σ): 695.2 ms ±   7.9 ms[User: 157.8 ms, System: 69.0 ms]
   Range (min … max):   678.7 ms … 703.1 ms10 runs
  // LLVM's implementation
   Time (mean ± σ): 694.9 ms ±   9.7 ms[User: 158.8 ms, System: 70.5 ms]
   Range (min … max):   675.2 ms … 711.7 ms10 runs


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150366

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

[Lldb-commits] [PATCH] D150366: [lldb][NFCI] Use llvm's libDebugInfo for DebugRanges

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG70aad4ec90f2: [lldb][NFCI] Use llvm's libDebugInfo for 
DebugRanges (authored by fdeazeve).

Changed prior to commit:
  https://reviews.llvm.org/D150366?vs=521300&id=524731#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150366

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.h

Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.h
@@ -26,9 +26,6 @@
 dw_offset_t debug_ranges_offset) const;
 
 protected:
-  bool Extract(lldb_private::DWARFContext &context, lldb::offset_t *offset_ptr,
-   DWARFRangeList &range_list);
-
   std::map m_range_map;
 };
 
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.cpp
@@ -8,67 +8,35 @@
 
 #include "DWARFDebugRanges.h"
 #include "DWARFUnit.h"
+#include "llvm/DebugInfo/DWARF/DWARFDebugRangeList.h"
 
 using namespace lldb_private;
 
-static dw_addr_t GetBaseAddressMarker(uint32_t addr_size) {
-  switch(addr_size) {
-case 2:
-  return 0x;
-case 4:
-  return 0x;
-case 8:
-  return 0x;
-  }
-  llvm_unreachable("GetBaseAddressMarker unsupported address size.");
-}
-
 DWARFDebugRanges::DWARFDebugRanges() : m_range_map() {}
 
 void DWARFDebugRanges::Extract(DWARFContext &context) {
-  DWARFRangeList range_list;
-  lldb::offset_t offset = 0;
-  dw_offset_t debug_ranges_offset = offset;
-  while (Extract(context, &offset, range_list)) {
-range_list.Sort();
-m_range_map[debug_ranges_offset] = range_list;
-debug_ranges_offset = offset;
-  }
-}
-
-bool DWARFDebugRanges::Extract(DWARFContext &context,
-   lldb::offset_t *offset_ptr,
-   DWARFRangeList &range_list) {
-  range_list.Clear();
-
-  lldb::offset_t range_offset = *offset_ptr;
-  const DWARFDataExtractor &debug_ranges_data = context.getOrLoadRangesData();
-  uint32_t addr_size = debug_ranges_data.GetAddressByteSize();
-  dw_addr_t base_addr = 0;
-  dw_addr_t base_addr_marker = GetBaseAddressMarker(addr_size);
-
-  while (
-  debug_ranges_data.ValidOffsetForDataOfSize(*offset_ptr, 2 * addr_size)) {
-dw_addr_t begin = debug_ranges_data.GetMaxU64(offset_ptr, addr_size);
-dw_addr_t end = debug_ranges_data.GetMaxU64(offset_ptr, addr_size);
-
-if (!begin && !end) {
-  // End of range list
-  break;
-}
-
-if (begin == base_addr_marker) {
-  base_addr = end;
-  continue;
+  llvm::DWARFDataExtractor extractor =
+  context.getOrLoadRangesData().GetAsLLVM();
+  llvm::DWARFDebugRangeList extracted_list;
+  uint64_t current_offset = 0;
+  auto extract_next_list = [&] {
+if (auto error = extracted_list.extract(extractor, ¤t_offset)) {
+  consumeError(std::move(error));
+  return false;
 }
-
-// Filter out empty ranges
-if (begin < end)
-  range_list.Append(DWARFRangeList::Entry(begin + base_addr, end - begin));
+return true;
+  };
+
+  uint64_t previous_offset = current_offset;
+  while (extractor.isValidOffset(current_offset) && extract_next_list()) {
+DWARFRangeList &lldb_range_list = m_range_map[previous_offset];
+lldb_range_list.Reserve(extracted_list.getEntries().size());
+for (auto &range : extracted_list.getEntries())
+  lldb_range_list.Append(range.StartAddress,
+ range.EndAddress - range.StartAddress);
+lldb_range_list.Sort();
+previous_offset = current_offset;
   }
-
-  // Make sure we consumed at least something
-  return range_offset != *offset_ptr;
 }
 
 DWARFRangeList
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D151425: [lldb][nfc] Place comment in the right place

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

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

This line has been misplaced since it was introduced in 2011 by
c26e4454035a4160cffc3c865cf83be194ca38c4.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D151425

Files:
  lldb/include/lldb/Utility/RangeMap.h


Index: lldb/include/lldb/Utility/RangeMap.h
===
--- lldb/include/lldb/Utility/RangeMap.h
+++ lldb/include/lldb/Utility/RangeMap.h
@@ -42,9 +42,9 @@
 size = 0;
   }
 
-  // Set the start value for the range, and keep the same size
   BaseType GetRangeBase() const { return base; }
 
+  // Set the start value for the range, and keep the same size
   void SetRangeBase(BaseType b) { base = b; }
 
   void Slide(BaseType slide) { base += slide; }


Index: lldb/include/lldb/Utility/RangeMap.h
===
--- lldb/include/lldb/Utility/RangeMap.h
+++ lldb/include/lldb/Utility/RangeMap.h
@@ -42,9 +42,9 @@
 size = 0;
   }
 
-  // Set the start value for the range, and keep the same size
   BaseType GetRangeBase() const { return base; }
 
+  // Set the start value for the range, and keep the same size
   void SetRangeBase(BaseType b) { base = b; }
 
   void Slide(BaseType slide) { base += slide; }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D151451: [lldb][nfc] Refactor methods with out parameter

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

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

Currently, the method `GetAttributeAddressRanges` takes a DWARFRangeList as a
parameter, just to immediately clear it. The method also returns the size of
this list. Such an API was obfuscating the intent of the call sites (it's not
clear from the method name what it returns) and it was obfuscating redundant
checks on the size of the list.

This commit refactors the method to return the list and to also make the call
sites use the more explicit `IsEmpty` method.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D151451

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -840,9 +840,9 @@
   if (!dwarf_ast)
 return nullptr;
 
-  DWARFRangeList ranges;
-  if (die.GetDIE()->GetAttributeAddressRanges(die.GetCU(), ranges,
-  /*check_hi_lo_pc=*/true) == 0)
+  DWARFRangeList ranges = die.GetDIE()->GetAttributeAddressRanges(
+  die.GetCU(), /*check_hi_lo_pc=*/true);
+  if (ranges.IsEmpty())
 return nullptr;
 
   // Union of all ranges in the function DIE (if the function is
@@ -3208,10 +3208,9 @@
   DWARFDIE function_die = GetDIE(sc.function->GetID());
 
   dw_addr_t func_lo_pc = LLDB_INVALID_ADDRESS;
-  DWARFRangeList ranges;
-  if (function_die.GetDIE()->GetAttributeAddressRanges(
-  function_die.GetCU(), ranges,
-  /*check_hi_lo_pc=*/true))
+  DWARFRangeList ranges = function_die.GetDIE()->GetAttributeAddressRanges(
+  function_die.GetCU(), /*check_hi_lo_pc=*/true);
+  if (!ranges.IsEmpty())
 func_lo_pc = ranges.GetMinRangeBase(0);
   if (func_lo_pc != LLDB_INVALID_ADDRESS) {
 const size_t num_variables =
@@ -4282,4 +4281,3 @@
 args.insert({comp_unit, Args(flags)});
   }
 }
-
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
@@ -94,8 +94,8 @@
   uint64_t fail_value,
   bool check_specification_or_abstract_origin = false) const;
 
-  size_t GetAttributeAddressRanges(
-  DWARFUnit *cu, DWARFRangeList &ranges, bool check_hi_lo_pc,
+  DWARFRangeList GetAttributeAddressRanges(
+  DWARFUnit *cu, bool check_hi_lo_pc,
   bool check_specification_or_abstract_origin = false) const;
 
   const char *GetName(const DWARFUnit *cu) const;
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
@@ -636,15 +636,16 @@
   return false;
 }
 
-size_t DWARFDebugInfoEntry::GetAttributeAddressRanges(
-DWARFUnit *cu, DWARFRangeList &ranges, bool check_hi_lo_pc,
+DWARFRangeList DWARFDebugInfoEntry::GetAttributeAddressRanges(
+DWARFUnit *cu, bool check_hi_lo_pc,
 bool check_specification_or_abstract_origin) const {
-  ranges.Clear();
 
   DWARFFormValue form_value;
-  if (GetAttributeValue(cu, DW_AT_ranges, form_value)) {
-ranges = GetRangesOrReportError(*cu, *this, form_value);
-  } else if (check_hi_lo_pc) {
+  if (GetAttributeValue(cu, DW_AT_ranges, form_value))
+return GetRangesOrReportError(*cu, *this, form_value);
+
+  DWARFRangeList ranges;
+  if (check_hi_lo_pc) {
 dw_addr_t lo_pc = LLDB_INVALID_ADDRESS;
 dw_addr_t hi_pc = LLDB_INVALID_ADDRESS;
 if (GetAttributeAddressRange(cu, lo_pc, hi_pc, LLDB_INVALID_ADDRESS,
@@ -653,7 +654,7 @@
 ranges.Append(DWARFRangeList::Entry(lo_pc, hi_pc - lo_pc));
 }
   }
-  return ranges.GetSize();
+  return ranges;
 }
 
 // GetName
@@ -716,9 +717,8 @@
 DWARFUnit *cu, DWARFDebugAranges *debug_aranges) const {
   if (m_tag) {
 if (m_tag == DW_TAG_subprogram) {
-  DWARFRangeList ranges;
-  GetAttributeAddressRanges(cu, ranges,
-/*check_hi_lo_pc=*/true);
+  DWARFRangeList ranges =
+  GetAttributeAddressRanges(cu, /*check_hi_lo_pc=*/true);
   for (const auto &r : ranges) {
 debug_aranges->AppendRange(GetOffset(), r.GetRangeBase(),
r.GetRangeEnd());
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cp

[Lldb-commits] [PATCH] D151425: [lldb][nfc] Place comment in the right place

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added inline comments.



Comment at: lldb/include/lldb/Utility/RangeMap.h:47
 
+  // Set the start value for the range, and keep the same size
   void SetRangeBase(BaseType b) { base = b; }

JDevlieghere wrote:
> 
I can update it for sure, but note that no comments in this file are doxygen 
enabled


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151425

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

[Lldb-commits] [PATCH] D151425: [lldb][nfc] Place comment in the right place

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG52b03b63972a: [lldb][nfc] Place comment in the right place 
(authored by fdeazeve).

Changed prior to commit:
  https://reviews.llvm.org/D151425?vs=525544&id=525679#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151425

Files:
  lldb/include/lldb/Utility/RangeMap.h


Index: lldb/include/lldb/Utility/RangeMap.h
===
--- lldb/include/lldb/Utility/RangeMap.h
+++ lldb/include/lldb/Utility/RangeMap.h
@@ -42,9 +42,9 @@
 size = 0;
   }
 
-  // Set the start value for the range, and keep the same size
   BaseType GetRangeBase() const { return base; }
 
+  /// Set the start value for the range, and keep the same size
   void SetRangeBase(BaseType b) { base = b; }
 
   void Slide(BaseType slide) { base += slide; }


Index: lldb/include/lldb/Utility/RangeMap.h
===
--- lldb/include/lldb/Utility/RangeMap.h
+++ lldb/include/lldb/Utility/RangeMap.h
@@ -42,9 +42,9 @@
 size = 0;
   }
 
-  // Set the start value for the range, and keep the same size
   BaseType GetRangeBase() const { return base; }
 
+  /// Set the start value for the range, and keep the same size
   void SetRangeBase(BaseType b) { base = b; }
 
   void Slide(BaseType slide) { base += slide; }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D151451: [lldb][nfc] Refactor methods with out parameter

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG346afb857228: [lldb][nfc] Refactor methods with out 
parameter (authored by fdeazeve).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151451

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -840,9 +840,9 @@
   if (!dwarf_ast)
 return nullptr;
 
-  DWARFRangeList ranges;
-  if (die.GetDIE()->GetAttributeAddressRanges(die.GetCU(), ranges,
-  /*check_hi_lo_pc=*/true) == 0)
+  DWARFRangeList ranges = die.GetDIE()->GetAttributeAddressRanges(
+  die.GetCU(), /*check_hi_lo_pc=*/true);
+  if (ranges.IsEmpty())
 return nullptr;
 
   // Union of all ranges in the function DIE (if the function is
@@ -3208,10 +3208,9 @@
   DWARFDIE function_die = GetDIE(sc.function->GetID());
 
   dw_addr_t func_lo_pc = LLDB_INVALID_ADDRESS;
-  DWARFRangeList ranges;
-  if (function_die.GetDIE()->GetAttributeAddressRanges(
-  function_die.GetCU(), ranges,
-  /*check_hi_lo_pc=*/true))
+  DWARFRangeList ranges = function_die.GetDIE()->GetAttributeAddressRanges(
+  function_die.GetCU(), /*check_hi_lo_pc=*/true);
+  if (!ranges.IsEmpty())
 func_lo_pc = ranges.GetMinRangeBase(0);
   if (func_lo_pc != LLDB_INVALID_ADDRESS) {
 const size_t num_variables =
@@ -4282,4 +4281,3 @@
 args.insert({comp_unit, Args(flags)});
   }
 }
-
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
@@ -94,8 +94,8 @@
   uint64_t fail_value,
   bool check_specification_or_abstract_origin = false) const;
 
-  size_t GetAttributeAddressRanges(
-  DWARFUnit *cu, DWARFRangeList &ranges, bool check_hi_lo_pc,
+  DWARFRangeList GetAttributeAddressRanges(
+  DWARFUnit *cu, bool check_hi_lo_pc,
   bool check_specification_or_abstract_origin = false) const;
 
   const char *GetName(const DWARFUnit *cu) const;
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
@@ -636,15 +636,16 @@
   return false;
 }
 
-size_t DWARFDebugInfoEntry::GetAttributeAddressRanges(
-DWARFUnit *cu, DWARFRangeList &ranges, bool check_hi_lo_pc,
+DWARFRangeList DWARFDebugInfoEntry::GetAttributeAddressRanges(
+DWARFUnit *cu, bool check_hi_lo_pc,
 bool check_specification_or_abstract_origin) const {
-  ranges.Clear();
 
   DWARFFormValue form_value;
-  if (GetAttributeValue(cu, DW_AT_ranges, form_value)) {
-ranges = GetRangesOrReportError(*cu, *this, form_value);
-  } else if (check_hi_lo_pc) {
+  if (GetAttributeValue(cu, DW_AT_ranges, form_value))
+return GetRangesOrReportError(*cu, *this, form_value);
+
+  DWARFRangeList ranges;
+  if (check_hi_lo_pc) {
 dw_addr_t lo_pc = LLDB_INVALID_ADDRESS;
 dw_addr_t hi_pc = LLDB_INVALID_ADDRESS;
 if (GetAttributeAddressRange(cu, lo_pc, hi_pc, LLDB_INVALID_ADDRESS,
@@ -653,7 +654,7 @@
 ranges.Append(DWARFRangeList::Entry(lo_pc, hi_pc - lo_pc));
 }
   }
-  return ranges.GetSize();
+  return ranges;
 }
 
 // GetName
@@ -716,9 +717,8 @@
 DWARFUnit *cu, DWARFDebugAranges *debug_aranges) const {
   if (m_tag) {
 if (m_tag == DW_TAG_subprogram) {
-  DWARFRangeList ranges;
-  GetAttributeAddressRanges(cu, ranges,
-/*check_hi_lo_pc=*/true);
+  DWARFRangeList ranges =
+  GetAttributeAddressRanges(cu, /*check_hi_lo_pc=*/true);
   for (const auto &r : ranges) {
 debug_aranges->AppendRange(GetOffset(), r.GetRangeBase(),
r.GetRangeEnd());
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
@@ -168,10 +168,9 @@
   }
 
   if (match_addr_range) {
-DWARFRangeList ranges;
-if (m_die->GetAttributeAddressRanges(m_cu, ranges,
- /*check_hi_lo_pc=*/true) &&
-ra

[Lldb-commits] [PATCH] D151603: [lldb][NFCI] Refactor Language::GetFormatterPrefixSuffix

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added inline comments.



Comment at: lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp:1001
 
-bool ObjCLanguage::GetFormatterPrefixSuffix(ValueObject &valobj,
-ConstString type_hint,
-std::string &prefix,
-std::string &suffix) {
-  static ConstString g_CFBag("CFBag");
-  static ConstString g_CFBinaryHeap("CFBinaryHeap");
-
-  static ConstString g_NSNumberChar("NSNumber:char");
-  static ConstString g_NSNumberShort("NSNumber:short");
-  static ConstString g_NSNumberInt("NSNumber:int");
-  static ConstString g_NSNumberLong("NSNumber:long");
-  static ConstString g_NSNumberInt128("NSNumber:int128_t");
-  static ConstString g_NSNumberFloat("NSNumber:float");
-  static ConstString g_NSNumberDouble("NSNumber:double");
-
-  static ConstString g_NSData("NSData");
-  static ConstString g_NSArray("NSArray");
-  static ConstString g_NSString("NSString");
-  static ConstString g_NSStringStar("NSString*");
-
-  if (type_hint.IsEmpty())
-return false;
-
-  prefix.clear();
-  suffix.clear();
-
-  if (type_hint == g_CFBag || type_hint == g_CFBinaryHeap) {
-prefix = "@";
-return true;
-  }
-
-  if (type_hint == g_NSNumberChar) {
-prefix = "(char)";
-return true;
-  }
-  if (type_hint == g_NSNumberShort) {
-prefix = "(short)";
-return true;
-  }
-  if (type_hint == g_NSNumberInt) {
-prefix = "(int)";
-return true;
-  }
-  if (type_hint == g_NSNumberLong) {
-prefix = "(long)";
-return true;
-  }
-  if (type_hint == g_NSNumberInt128) {
-prefix = "(int128_t)";
-return true;
-  }
-  if (type_hint == g_NSNumberFloat) {
-prefix = "(float)";
-return true;
-  }
-  if (type_hint == g_NSNumberDouble) {
-prefix = "(double)";
-return true;
-  }
-
-  if (type_hint == g_NSData || type_hint == g_NSArray) {
-prefix = "@\"";
-suffix = "\"";
-return true;
-  }
-
-  if (type_hint == g_NSString || type_hint == g_NSStringStar) {
-prefix = "@";
-return true;
-  }
+std::pair
+ObjCLanguage::GetFormatterPrefixSuffix(llvm::StringRef type_hint) {

We can make this whole map const and remove the explicit call_once by folding 
the `insert` calls into the ctor:

```
static constexpr llvm::StringRef empty;
static const llvm::StringMap<
std::pair>
g_affix_map = {{"CFBag", std::make_pair("@", empty)},
   {"CFBinaryHeap", std::make_pair("@", empty)},
   ..., };
```

If you're so inclined, you can even get rid of the final make_pair calls:

```
static const llvm::StringMap<
std::pair>
g_affix_map = {{"CFBag", {"@", empty}},
   {"CFBinaryHeap", {"@", empty}}};
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151603

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

[Lldb-commits] [PATCH] D151919: [lldb][NFCI] Apply IndexEntry to DWARFUnitHeader outside of extraction

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve accepted this revision.
fdeazeve added a comment.

I'm not exactly familiar with DWOs, but the code motions LGTM!




Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:882
+  // we have a valid one to set it to.
+  assert(index_entry);
+  assert(!m_index_entry);

If LLVM's function is like this as well, we shouldn't do anything now and just 
refactor it later once we make the switch. But a pointer parameter in a 
function that asserts not null immediately is more expressive as a reference. 
Note that, in the code where this is called, we have a:

```
if (ptr)
  ApplyIndex(ptr)
```

So the assert is not needed, as the callee respects the contract and can do a 
`*ptr`



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151919

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

[Lldb-commits] [PATCH] D152010: [lldb][NFCI] ConstString methods should take StringRefs by value

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve accepted this revision.
fdeazeve added a comment.

LGTM, thanks for picking this up!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152010

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

[Lldb-commits] [PATCH] D152210: [lldb][NFCI] Remove use of ConstString from OptionValueProperties

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

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

LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152210

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

[Lldb-commits] [PATCH] D152315: [lldb][NFCI] Refactor TypeSystemClang::GetBasicTypeEnumeration

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve accepted this revision.
fdeazeve added a comment.

LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152315

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

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

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve accepted this revision.
fdeazeve added inline comments.



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

JDevlieghere wrote:
> If we're going to return a `std::string`, we might as well use a 
> `raw_string_ostream` and simplify this function. 
+1, we can avoid the conversion of SmallString->std::string (and enable copy 
elision) by declaring `name` as std::string

(we don't necessarily need to do it in this patch though, either way is fine 
IMO)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152324

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

[Lldb-commits] [PATCH] D152449: [lldb][NFC]Update debug (eh-frame) tests to preserve old behaviour.

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added a comment.

Thanks for looking into this! I think we should just remove the XFAIL instead, 
for the reasons you mentioned in the other patch!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152449

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

[Lldb-commits] [PATCH] D152449: [lldb][NFC]Update debug (eh-frame) tests to preserve old behaviour.

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

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

LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152449

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

[Lldb-commits] [PATCH] D152476: [lldb] Remove lldb's DWARFAbbreviationDeclarationSet in favor of llvm's

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

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

This is awesome! I believe you said there was no measurable perf diff?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152476

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

[Lldb-commits] [PATCH] D152872: Add support for __debug_line_str in Mach-O

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve accepted this revision.
fdeazeve added a comment.

Nice and subtle fix! :)


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

https://reviews.llvm.org/D152872

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

[Lldb-commits] [PATCH] D152846: [lldb][NFCI] Remove custom matcher classes in Listener in favor of lambdas

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

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

Nice catch! Also agree with the suggestion of using the STLExtras wrapper


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152846

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