[Lldb-commits] [PATCH] D140630: [lldb-vscode] Add data breakpoint support

2023-01-09 Thread Callum Macmillan via Phabricator via lldb-commits 
cimacmillan added a comment.

Ping! :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140630

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

[Lldb-commits] [PATCH] D140630: [lldb-vscode] Add data breakpoint support

2023-01-09 Thread Callum Macmillan via Phabricator via lldb-commits 
cimacmillan added a comment.

Ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140630

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

[Lldb-commits] [PATCH] D139955: [LLDB] Change formatting to use llvm::formatv

2023-01-09 Thread Pavel Labath via Phabricator via lldb-commits 
labath accepted this revision.
labath added a comment.

I'm sorry for the delay, I was out for the holidays. Looks now, thanks for your 
patience.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139955

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

[Lldb-commits] [PATCH] D140368: [lldb] Consider all breakpoints in breakpoint detection

2023-01-09 Thread Pavel Kosov via Phabricator via lldb-commits 
kpdev42 added a comment.

@jingham
May I kindly ask you to take a look at this patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140368

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

[Lldb-commits] [PATCH] D139955: [LLDB] Change formatting to use llvm::formatv

2023-01-09 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere accepted this revision.
JDevlieghere added a comment.
This revision is now accepted and ready to land.

LGTM too


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139955

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

[Lldb-commits] [PATCH] D141298: Move from llvm::makeArrayRef to ArrayRef deduction guides - last part

2023-01-09 Thread serge via Phabricator via lldb-commits 
serge-sans-paille added a comment.
Herald added subscribers: Michael137, JDevlieghere.

Once that patch lands, I'll mark `makeArrayRef` as deprecated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141298

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

[Lldb-commits] [PATCH] D141298: Move from llvm::makeArrayRef to ArrayRef deduction guides - last part

2023-01-09 Thread Mehdi AMINI via Phabricator via lldb-commits 
mehdi_amini added a comment.

Seems mechanical, and if it build everywhere LGTM :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141298

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

[Lldb-commits] [PATCH] D140800: [OptTable] Precompute OptTable prefixes union table through tablegen

2023-01-09 Thread serge via Phabricator via lldb-commits 
serge-sans-paille added a comment.

@nikic : gentle ping :-)


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

https://reviews.llvm.org/D140800

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

[Lldb-commits] [lldb] 1d6243d - [lldb] Fix symbol table use after free

2023-01-09 Thread Augusto Noronha via lldb-commits 

Author: Augusto Noronha
Date: 2023-01-09T10:27:18-08:00
New Revision: 1d6243db90b09c61d78a14268bb88a73792b63ab

URL: 
https://github.com/llvm/llvm-project/commit/1d6243db90b09c61d78a14268bb88a73792b63ab
DIFF: 
https://github.com/llvm/llvm-project/commit/1d6243db90b09c61d78a14268bb88a73792b63ab.diff

LOG: [lldb] Fix symbol table use after free

The symbol file stores a raw pointer to the main object file's symbol
table. This pointer, however, can be freed, if ObjectFile::ClearSymtab
is ever called. This patch makes sure out pointer to the symbol file
is valid before using it.

Added: 


Modified: 
lldb/include/lldb/Symbol/SymbolFile.h
lldb/source/Symbol/SymbolFile.cpp

Removed: 




diff  --git a/lldb/include/lldb/Symbol/SymbolFile.h 
b/lldb/include/lldb/Symbol/SymbolFile.h
index d5fe0331fe5a8..4b5499304664b 100644
--- a/lldb/include/lldb/Symbol/SymbolFile.h
+++ b/lldb/include/lldb/Symbol/SymbolFile.h
@@ -504,7 +504,6 @@ class SymbolFileCommon : public SymbolFile {
// file)
   std::optional> m_compile_units;
   TypeList m_type_list;
-  Symtab *m_symtab = nullptr;
   uint32_t m_abilities = 0;
   bool m_calculated_abilities = false;
   bool m_index_was_loaded_from_cache = false;
@@ -517,6 +516,10 @@ class SymbolFileCommon : public SymbolFile {
 private:
   SymbolFileCommon(const SymbolFileCommon &) = delete;
   const SymbolFileCommon &operator=(const SymbolFileCommon &) = delete;
+
+  /// Do not use m_symtab directly, as it may be freed. Use GetSymtab()
+  /// to access it instead.
+  Symtab *m_symtab = nullptr;
 };
 
 } // namespace lldb_private

diff  --git a/lldb/source/Symbol/SymbolFile.cpp 
b/lldb/source/Symbol/SymbolFile.cpp
index c7af908543e88..b271efd07bfe3 100644
--- a/lldb/source/Symbol/SymbolFile.cpp
+++ b/lldb/source/Symbol/SymbolFile.cpp
@@ -164,16 +164,15 @@ SymbolFile::RegisterInfoResolver::~RegisterInfoResolver() 
= default;
 
 Symtab *SymbolFileCommon::GetSymtab() {
   std::lock_guard guard(GetModuleMutex());
-  if (m_symtab)
-return m_symtab;
-
   // Fetch the symtab from the main object file.
-  m_symtab = GetMainObjectFile()->GetSymtab();
-
-  // Then add our symbols to it.
-  if (m_symtab)
-AddSymbols(*m_symtab);
+  auto *symtab = GetMainObjectFile()->GetSymtab();
+  if (m_symtab != symtab) {
+m_symtab = symtab;
 
+// Then add our symbols to it.
+if (m_symtab)
+  AddSymbols(*m_symtab);
+  }
   return m_symtab;
 }
 
@@ -186,8 +185,8 @@ void SymbolFileCommon::SectionFileAddressesChanged() {
   ObjectFile *symfile_objfile = GetObjectFile();
   if (symfile_objfile != module_objfile)
 symfile_objfile->SectionFileAddressesChanged();
-  if (m_symtab)
-m_symtab->SectionFileAddressesChanged();
+  if (auto *symtab = GetSymtab())
+symtab->SectionFileAddressesChanged();
 }
 
 uint32_t SymbolFileCommon::GetNumCompileUnits() {



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

[Lldb-commits] [PATCH] D68655: Trust the arange accelerator tables in dSYMs

2023-01-09 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere updated this revision to Diff 487509.
JDevlieghere added a comment.

> Apparently for very small / gapless programs, clang emits single-value 
> DW_AT_low_pc/high_pc in the compile unit, and dsymutil does not rewrite that 
> into DW_AT_ranges.

Although this statement is correct, it shouldn't affect this patch. Even when 
the compile units uses DW_AT_low_pc/high_pc, dsymutil still emits the correct 
aranges. Without a counter-example we should go ahead with this patch.


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

https://reviews.llvm.org/D68655

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


Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
@@ -53,13 +53,20 @@
   }
 
   // Manually build arange data for everything that wasn't in .debug_aranges.
-  const size_t num_units = GetNumUnits();
-  for (size_t idx = 0; idx < num_units; ++idx) {
-DWARFUnit *cu = GetUnitAtIndex(idx);
-
-dw_offset_t offset = cu->GetOffset();
-if (cus_with_data.find(offset) == cus_with_data.end())
-  cu->BuildAddressRangeTable(m_cu_aranges_up.get());
+  // Skip this step for dSYMs as we can trust dsymutil to have emitted complete
+  // aranges.
+  const bool is_dsym =
+  m_dwarf.GetObjectFile() &&
+  m_dwarf.GetObjectFile()->GetType() == ObjectFile::eTypeDebugInfo;
+  if (!is_dsym) {
+const size_t num_units = GetNumUnits();
+for (size_t idx = 0; idx < num_units; ++idx) {
+  DWARFUnit *cu = GetUnitAtIndex(idx);
+
+  dw_offset_t offset = cu->GetOffset();
+  if (cus_with_data.find(offset) == cus_with_data.end())
+cu->BuildAddressRangeTable(m_cu_aranges_up.get());
+}
   }
 
   const bool minimize = true;


Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
@@ -53,13 +53,20 @@
   }
 
   // Manually build arange data for everything that wasn't in .debug_aranges.
-  const size_t num_units = GetNumUnits();
-  for (size_t idx = 0; idx < num_units; ++idx) {
-DWARFUnit *cu = GetUnitAtIndex(idx);
-
-dw_offset_t offset = cu->GetOffset();
-if (cus_with_data.find(offset) == cus_with_data.end())
-  cu->BuildAddressRangeTable(m_cu_aranges_up.get());
+  // Skip this step for dSYMs as we can trust dsymutil to have emitted complete
+  // aranges.
+  const bool is_dsym =
+  m_dwarf.GetObjectFile() &&
+  m_dwarf.GetObjectFile()->GetType() == ObjectFile::eTypeDebugInfo;
+  if (!is_dsym) {
+const size_t num_units = GetNumUnits();
+for (size_t idx = 0; idx < num_units; ++idx) {
+  DWARFUnit *cu = GetUnitAtIndex(idx);
+
+  dw_offset_t offset = cu->GetOffset();
+  if (cus_with_data.find(offset) == cus_with_data.end())
+cu->BuildAddressRangeTable(m_cu_aranges_up.get());
+}
   }
 
   const bool minimize = true;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D139955: [LLDB] Change formatting to use llvm::formatv

2023-01-09 Thread Alexander Yermolovich via Phabricator via lldb-commits 
ayermolo updated this revision to Diff 487512.
ayermolo added a comment.

rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139955

Files:
  lldb/include/lldb/Core/Module.h
  lldb/include/lldb/Utility/Status.h
  lldb/source/Core/Module.cpp
  lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFTypeUnit.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
  lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
  lldb/source/Symbol/CompileUnit.cpp
  lldb/source/Symbol/DWARFCallFrameInfo.cpp
  lldb/source/Target/SectionLoadList.cpp
  lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s
  lldb/test/Shell/SymbolFile/DWARF/x86/debug_ranges-missing-section.s

Index: lldb/test/Shell/SymbolFile/DWARF/x86/debug_ranges-missing-section.s
===
--- lldb/test/Shell/SymbolFile/DWARF/x86/debug_ranges-missing-section.s
+++ lldb/test/Shell/SymbolFile/DWARF/x86/debug_ranges-missing-section.s
@@ -2,7 +2,7 @@
 # RUN: %lldb %t -o "image lookup -v -s lookup_ranges" -o exit 2>%t.error | FileCheck %s
 # RUN: cat %t.error | FileCheck %s --check-prefix ERROR
 
-# ERROR: DIE has DW_AT_ranges(DW_FORM_sec_offset 0x47) attribute, but range extraction failed (No debug_ranges section),
+# ERROR: DIE has DW_AT_ranges(DW_FORM_sec_offset 0x0047) attribute, but range extraction failed (No debug_ranges section),
 # CHECK:  Function: id = {0x001c}, name = "ranges", range = [0x-0x0004)
 # CHECK:Blocks: id = {0x001c}, range = [0x-0x0004)
 
Index: lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s
===
--- lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s
+++ lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s
@@ -22,7 +22,7 @@
 # RUN: cat %t.error | FileCheck --check-prefix=ERROR %s
 
 # RNGLISTX-LABEL: image lookup -v -s lookup_rnglists
-# ERROR: error: {{.*}} {0x003f}: DIE has DW_AT_ranges(DW_FORM_rnglistx 0x0) attribute, but range extraction failed (DW_FORM_rnglistx cannot be used without DW_AT_rnglists_base for CU at 0x), please file a bug and attach the file at the start of this error message
+# ERROR: error: {{.*}} [0x003f]: DIE has DW_AT_ranges(DW_FORM_rnglistx 0x) attribute, but range extraction failed (DW_FORM_rnglistx cannot be used without DW_AT_rnglists_base for CU at 0x), please file a bug and attach the file at the start of this error message
 
 # RUN: llvm-mc -triple=x86_64-pc-linux -filetype=obj \
 # RUN:   --defsym RNGLISTX=0 --defsym RNGLISTBASE=0 %s > %t-rnglistbase
@@ -31,7 +31,7 @@
 # RUN: cat %t.error | FileCheck --check-prefix=ERRORBASE %s
 
 # RNGLISTBASE-LABEL: image lookup -v -s lookup_rnglists
-# ERRORBASE: error: {{.*}}-rnglistbase {0x0043}: DIE has DW_AT_ranges(DW_FORM_rnglistx 0x0) attribute, but range extraction failed (invalid range list table index 0; OffsetEntryCount is 0, DW_AT_rnglists_base is 24), please file a bug and attach the file at the start of this error message
+# ERRORBASE: error: {{.*}}-rnglistbase [0x0043]: DIE has DW_AT_ranges(DW_FORM_rnglistx 0x) attribute, but range extraction failed (invalid range list table index 0; OffsetEntryCount is 0, DW_AT_rnglists_base is 24), please file a bug and attach the file at the start of this error message
 
 .text
 rnglists:
Index: lldb/source/Target/SectionLoadList.cpp
===
--- lldb/source/Target/SectionLoadList.cpp
+++ lldb/source/Target/SectionLoadList.cpp
@@ -106,8 +106,8 @@
   ModuleSP curr_module_sp(ats_pos->second->GetModule());
   if (curr_module_sp) {
 module_sp->ReportWarning(
-"address 0x%16.16" PRIx64
-" maps to more than one section: %s.%s and %s.%s",
+"address {0:x16} maps to more than one section: {1}.{2} and "
+"{3}.{4}",
 load_addr, module_sp->GetFileSpec().GetFilename().GetCString(),
 section->GetName().GetCString(),
 curr_module_sp->GetFileSpec().GetFilename().GetCString(),

[Lldb-commits] [lldb] e262b8f - [LLDB] Change formatting to use llvm::formatv

2023-01-09 Thread Alexander Yermolovich via lldb-commits 

Author: Alexander Yermolovich
Date: 2023-01-09T11:29:43-08:00
New Revision: e262b8f48af9fdca8380f2f079e50291956aad71

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

LOG: [LLDB] Change formatting to use llvm::formatv

In preparation for eanbling 64bit support in LLDB switching to use llvm::formatv
instead of format MACROs.

Reviewed By: labath, JDevlieghere

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

Added: 


Modified: 
lldb/include/lldb/Core/Module.h
lldb/include/lldb/Utility/Status.h
lldb/source/Core/Module.cpp
lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
lldb/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp
lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp
lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp
lldb/source/Plugins/SymbolFile/DWARF/DWARFTypeUnit.cpp
lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
lldb/source/Symbol/CompileUnit.cpp
lldb/source/Symbol/DWARFCallFrameInfo.cpp
lldb/source/Target/SectionLoadList.cpp
lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s
lldb/test/Shell/SymbolFile/DWARF/x86/debug_ranges-missing-section.s

Removed: 




diff  --git a/lldb/include/lldb/Core/Module.h b/lldb/include/lldb/Core/Module.h
index 56c3de1249c9..31f7894178d7 100644
--- a/lldb/include/lldb/Core/Module.h
+++ b/lldb/include/lldb/Core/Module.h
@@ -825,22 +825,35 @@ class Module : public 
std::enable_shared_from_this,
   // architecture, path and object name (if any)). This centralizes code so
   // that everyone doesn't need to format their error and log messages on their
   // own and keeps the output a bit more consistent.
-  void LogMessage(Log *log, const char *format, ...)
-  __attribute__((format(printf, 3, 4)));
+  template 
+  void LogMessage(Log *log, const char *format, Args &&...args) {
+LogMessage(log, llvm::formatv(format, std::forward(args)...));
+  }
 
-  void LogMessageVerboseBacktrace(Log *log, const char *format, ...)
-  __attribute__((format(printf, 3, 4)));
+  template 
+  void LogMessageVerboseBacktrace(Log *log, const char *format,
+  Args &&...args) {
+LogMessageVerboseBacktrace(
+log, llvm::formatv(format, std::forward(args)...));
+  }
 
-  void ReportWarning(const char *format, ...)
-  __attribute__((format(printf, 2, 3)));
+  template 
+  void ReportWarning(const char *format, Args &&...args) {
+ReportWarning(llvm::formatv(format, std::forward(args)...));
+  }
 
-  void ReportError(const char *format, ...)
-  __attribute__((format(printf, 2, 3)));
+  template 
+  void ReportError(const char *format, Args &&...args) {
+ReportError(llvm::formatv(format, std::forward(args)...));
+  }
 
   // Only report an error once when the module is first detected to be modified
   // so we don't spam the console with many messages.
-  void ReportErrorIfModifyDetected(const char *format, ...)
-  __attribute__((format(printf, 2, 3)));
+  template 
+  void ReportErrorIfModifyDetected(const char *format, Args &&...args) {
+ReportErrorIfModifyDetected(
+llvm::formatv(format, std::forward(args)...));
+  }
 
   void ReportWarningOptimization(std::optional debugger_id);
 
@@ -1155,6 +1168,13 @@ class Module : public 
std::enable_shared_from_this,
 
   Module(const Module &) = delete;
   const Module &operator=(const Module &) = delete;
+
+  void LogMessage(Log *log, const llvm::formatv_object_base &payload);
+  void LogMessageVerboseBacktrace(Log *log,
+  const llvm::formatv_object_base &payload);
+  void ReportWarning(const llvm::formatv_object_base &payload);
+  void ReportError(const llvm::formatv_object_base &payload);
+  void ReportErrorIfModifyDetected(const llvm::formatv_object_base &payload);
 };
 
 } // namespace lldb_private

diff  --git a/lldb/include/lldb/Utility/Status.h 
b/lldb/include/lldb/Utility/Status.h
index bee2b57b6ea9..ac410552438e 100644
--- a/lldb/include/lldb/Utility/Status.h
+++ b/lldb/include/lldb/Utility/Status.h
@@ -64,6 +64,11 @@ class Status {
   explicit Status(const char *format, ...)
   __attribute__((format(printf, 2, 3)));
 
+  template 
+  static Status createWithFormat(const char *format, Args &&...args) {
+

[Lldb-commits] [PATCH] D139955: [LLDB] Change formatting to use llvm::formatv

2023-01-09 Thread Alexander Yermolovich via Phabricator via lldb-commits 
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGe262b8f48af9: [LLDB] Change formatting to use llvm::formatv 
(authored by ayermolo).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139955

Files:
  lldb/include/lldb/Core/Module.h
  lldb/include/lldb/Utility/Status.h
  lldb/source/Core/Module.cpp
  lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFTypeUnit.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
  lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
  lldb/source/Symbol/CompileUnit.cpp
  lldb/source/Symbol/DWARFCallFrameInfo.cpp
  lldb/source/Target/SectionLoadList.cpp
  lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s
  lldb/test/Shell/SymbolFile/DWARF/x86/debug_ranges-missing-section.s

Index: lldb/test/Shell/SymbolFile/DWARF/x86/debug_ranges-missing-section.s
===
--- lldb/test/Shell/SymbolFile/DWARF/x86/debug_ranges-missing-section.s
+++ lldb/test/Shell/SymbolFile/DWARF/x86/debug_ranges-missing-section.s
@@ -2,7 +2,7 @@
 # RUN: %lldb %t -o "image lookup -v -s lookup_ranges" -o exit 2>%t.error | FileCheck %s
 # RUN: cat %t.error | FileCheck %s --check-prefix ERROR
 
-# ERROR: DIE has DW_AT_ranges(DW_FORM_sec_offset 0x47) attribute, but range extraction failed (No debug_ranges section),
+# ERROR: DIE has DW_AT_ranges(DW_FORM_sec_offset 0x0047) attribute, but range extraction failed (No debug_ranges section),
 # CHECK:  Function: id = {0x001c}, name = "ranges", range = [0x-0x0004)
 # CHECK:Blocks: id = {0x001c}, range = [0x-0x0004)
 
Index: lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s
===
--- lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s
+++ lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s
@@ -22,7 +22,7 @@
 # RUN: cat %t.error | FileCheck --check-prefix=ERROR %s
 
 # RNGLISTX-LABEL: image lookup -v -s lookup_rnglists
-# ERROR: error: {{.*}} {0x003f}: DIE has DW_AT_ranges(DW_FORM_rnglistx 0x0) attribute, but range extraction failed (DW_FORM_rnglistx cannot be used without DW_AT_rnglists_base for CU at 0x), please file a bug and attach the file at the start of this error message
+# ERROR: error: {{.*}} [0x003f]: DIE has DW_AT_ranges(DW_FORM_rnglistx 0x) attribute, but range extraction failed (DW_FORM_rnglistx cannot be used without DW_AT_rnglists_base for CU at 0x), please file a bug and attach the file at the start of this error message
 
 # RUN: llvm-mc -triple=x86_64-pc-linux -filetype=obj \
 # RUN:   --defsym RNGLISTX=0 --defsym RNGLISTBASE=0 %s > %t-rnglistbase
@@ -31,7 +31,7 @@
 # RUN: cat %t.error | FileCheck --check-prefix=ERRORBASE %s
 
 # RNGLISTBASE-LABEL: image lookup -v -s lookup_rnglists
-# ERRORBASE: error: {{.*}}-rnglistbase {0x0043}: DIE has DW_AT_ranges(DW_FORM_rnglistx 0x0) attribute, but range extraction failed (invalid range list table index 0; OffsetEntryCount is 0, DW_AT_rnglists_base is 24), please file a bug and attach the file at the start of this error message
+# ERRORBASE: error: {{.*}}-rnglistbase [0x0043]: DIE has DW_AT_ranges(DW_FORM_rnglistx 0x) attribute, but range extraction failed (invalid range list table index 0; OffsetEntryCount is 0, DW_AT_rnglists_base is 24), please file a bug and attach the file at the start of this error message
 
 .text
 rnglists:
Index: lldb/source/Target/SectionLoadList.cpp
===
--- lldb/source/Target/SectionLoadList.cpp
+++ lldb/source/Target/SectionLoadList.cpp
@@ -106,8 +106,8 @@
   ModuleSP curr_module_sp(ats_pos->second->GetModule());
   if (curr_module_sp) {
 module_sp->ReportWarning(
-"address 0x%16.16" PRIx64
-" maps to more than one section: %s.%s and %s.%s",
+"address {0:x16} maps to more than one section: {1}.{2} and "
+"{3}.{4}",
 load_addr, module_sp->GetFileSpec().G

[Lldb-commits] [PATCH] D139955: [LLDB] Change formatting to use llvm::formatv

2023-01-09 Thread Alexander Yermolovich via Phabricator via lldb-commits 
ayermolo added a comment.

Thanks for reviewing, really appreciate it!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139955

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

[Lldb-commits] [PATCH] D141318: [lldb] Store shared pointers in DieToTypePtr map instead of raw pointers

2023-01-09 Thread Augusto Noronha via Phabricator via lldb-commits 
augusto2112 created this revision.
augusto2112 added reviewers: labath, clayborg, aprantl.
Herald added a reviewer: shafik.
Herald added a project: All.
augusto2112 requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Storing raw pointers in DieToTypePtr may cause use-after-frees to occur,
since there are no guarantees that the shared pointers that owns the
underlying pointer to the type are kept around as long as the map.
Change the map to store a shared pointer instead.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D141318

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

Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
@@ -56,7 +56,6 @@
 class SymbolFileDWARFDwo;
 class SymbolFileDWARFDwp;
 
-#define DIE_IS_BEING_PARSED ((lldb_private::Type *)1)
 
 class SymbolFileDWARF : public lldb_private::SymbolFileCommon,
 public lldb_private::UserID {
@@ -233,6 +232,10 @@
 
   static bool SupportedVersion(uint16_t version);
 
+  /// Returns a sentinel pointer that indicates that a DIE is being
+  /// parsed into a type.
+  static std::shared_ptr GetSentinelDieBeingParsed();
+
   DWARFDIE
   GetDeclContextDIEContainingDIE(const DWARFDIE &die);
 
@@ -348,7 +351,8 @@
   lldb_private::ConstString ConstructFunctionDemangledName(const DWARFDIE &die);
 
 protected:
-  typedef llvm::DenseMap
+  typedef llvm::DenseMap>
   DIEToTypePtr;
   typedef llvm::DenseMap
   DIEToVariableSP;
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -517,6 +517,13 @@
   return version >= 2 && version <= 5;
 }
 
+std::shared_ptr
+SymbolFileDWARF::GetSentinelDieBeingParsed() {
+  static std::shared_ptr sentinel =
+  std::make_shared();
+  return sentinel;
+}
+
 uint32_t SymbolFileDWARF::CalculateAbilities() {
   uint32_t abilities = 0;
   if (m_objfile_sp != nullptr) {
@@ -1602,17 +1609,18 @@
 // to SymbolFileDWARF::ResolveClangOpaqueTypeDefinition are done.
 GetForwardDeclClangTypeToDie().erase(die_it);
 
-Type *type = GetDIEToType().lookup(dwarf_die.GetDIE());
+auto type_sp = GetDIEToType().lookup(dwarf_die.GetDIE());
 
 Log *log = GetLog(DWARFLog::DebugInfo | DWARFLog::TypeCompletion);
 if (log)
   GetObjectFile()->GetModule()->LogMessageVerboseBacktrace(
   log, "0x%8.8" PRIx64 ": %s '%s' resolving forward declaration...",
   dwarf_die.GetID(), dwarf_die.GetTagAsCString(),
-  type->GetName().AsCString());
+  type_sp->GetName().AsCString());
 assert(compiler_type);
 if (DWARFASTParser *dwarf_ast = GetDWARFParser(*dwarf_die.GetCU()))
-  return dwarf_ast->CompleteTypeFromDWARF(dwarf_die, type, compiler_type);
+  return dwarf_ast->CompleteTypeFromDWARF(dwarf_die, type_sp.get(),
+  compiler_type);
   }
   return false;
 }
@@ -1624,7 +1632,7 @@
 Type *type = GetTypeForDIE(die, resolve_function_context).get();
 
 if (assert_not_being_parsed) {
-  if (type != DIE_IS_BEING_PARSED)
+  if (type != SymbolFileDWARF::GetSentinelDieBeingParsed().get())
 return type;
 
   GetObjectFile()->GetModule()->ReportError(
@@ -2686,10 +2694,13 @@
 
 TypeSP SymbolFileDWARF::GetTypeForDIE(const DWARFDIE &die,
   bool resolve_function_context) {
-  TypeSP type_sp;
   if (die) {
-Type *type_ptr = GetDIEToType().lookup(die.GetDIE());
-if (type_ptr == nullptr) {
+if (auto type_sp = GetDIEToType().lookup(die.GetDIE())) {
+  if (type_sp.get() !=
+  SymbolFileDWARF::GetSentinelDieBeingParsed().get()) {
+return type_sp;
+  }
+} else {
   SymbolContextScope *scope;
   if (auto *dwarf_cu = llvm::dyn_cast(die.GetCU()))
 scope = GetCompUnitForDWARFCompUnit(*dwarf_cu);
@@ -2708,13 +2719,10 @@
   !GetFunction(DWARFDIE(die.GetCU(), parent_die), sc))
 sc = sc_backup;
 
-  type_sp = ParseType(sc, die, nullptr);
-} else if (type_ptr != DIE_IS_BEING_PARSED) {
-  // Get the original shared pointer for this type
-  type_sp = type_ptr->shared_from_this();
+  return ParseType(sc, die, nullptr);
 }
   }
-  return type_sp;
+  return nullptr;
 }
 
 DWARFDIE
@@ -2850,7 +2858,9 @@
   return true;
 
 Type *resolved_type = ResolveType(type_die, false, true);
-if (!resolved_type || resolved_type == DIE_IS_BEING_PARSED)
+if (!resolved_type ||
+

[Lldb-commits] [PATCH] D141318: [lldb] Store shared pointers in DieToTypePtr map instead of raw pointers

2023-01-09 Thread Augusto Noronha via Phabricator via lldb-commits 
augusto2112 added a comment.
Herald added a subscriber: JDevlieghere.

I changed the value of the map to a shared pointer, but could change it to a 
weak pointer instead, not sure which is more appropriate in this case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141318

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

[Lldb-commits] [PATCH] D141318: [lldb] Store shared pointers in DieToTypePtr map instead of raw pointers

2023-01-09 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere added a comment.

In D141318#4037541 , @augusto2112 
wrote:

> I changed the value of the map to a shared pointer, but could change it to a 
> weak pointer instead, not sure which is more appropriate in this case.

I was about to ask the same thing. What currently determines the lifetime of 
the stored objects?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141318

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

[Lldb-commits] [PATCH] D68655: Trust the arange accelerator tables in dSYMs

2023-01-09 Thread Jason Molenda via Phabricator via lldb-commits 
jasonmolenda added a comment.

I agree with landing this change.


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

https://reviews.llvm.org/D68655

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

[Lldb-commits] [PATCH] D141318: [lldb] Store shared pointers in DieToTypePtr map instead of raw pointers

2023-01-09 Thread Augusto Noronha via Phabricator via lldb-commits 
augusto2112 added a comment.

In D141318#4037557 , @JDevlieghere 
wrote:

> In D141318#4037541 , @augusto2112 
> wrote:
>
>> I changed the value of the map to a shared pointer, but could change it to a 
>> weak pointer instead, not sure which is more appropriate in this case.
>
> I was about to ask the same thing. What currently determines the lifetime of 
> the stored objects?

Currently, a bunch of functions in `DWARFASTParserClang` and `SymbolFileDWARF` 
will create a new `Type` SP, store the underlying pointer in the map, and 
return the SP. The lifetime of the stored objects varies by whatever the caller 
does with the returned SP. Looking at some of the callers, I don't see a very 
clear pattern, most seem to use it and immediately discard it  (use after free 
when we look up the same type again and the underlying pointer is still in the 
map), but at least one caller (`SymbolFileDWARF::ResolveType`) returns the 
underlying pointer to //its// callers, which would still be UB even if we 
changed the map to store weak pointers. Based on that I'm thinking we should 
store them as shared pointers instead of weak ones.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141318

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

[Lldb-commits] [PATCH] D141318: [lldb] Store shared pointers in DieToTypePtr map instead of raw pointers

2023-01-09 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere added a comment.

In D141318#4037640 , @augusto2112 
wrote:

> In D141318#4037557 , @JDevlieghere 
> wrote:
>
>> In D141318#4037541 , @augusto2112 
>> wrote:
>>
>>> I changed the value of the map to a shared pointer, but could change it to 
>>> a weak pointer instead, not sure which is more appropriate in this case.
>>
>> I was about to ask the same thing. What currently determines the lifetime of 
>> the stored objects?
>
> Currently, a bunch of functions in `DWARFASTParserClang` and 
> `SymbolFileDWARF` will create a new `Type` SP, store the underlying pointer 
> in the map, and return the SP. The lifetime of the stored objects varies by 
> whatever the caller does with the returned SP. Looking at some of the 
> callers, I don't see a very clear pattern, most seem to use it and 
> immediately discard it  (use after free when we look up the same type again 
> and the underlying pointer is still in the map), but at least one caller 
> (`SymbolFileDWARF::ResolveType`) returns the underlying pointer to //its// 
> callers, which would still be UB even if we changed the map to store weak 
> pointers. Based on that I'm thinking we should store them as shared pointers 
> instead of weak ones.

Then maybe the solution is to store the type as unique pointers in the map and 
hand out raw pointers, relying on the map to keep the object alive? I feel 
pretty uncomfortable with handing out shared pointers without clearly 
understanding its lifetime cycle.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141318

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

[Lldb-commits] [lldb] 8b259fe - [lldb] Trust the arange accelerator tables in dSYMs

2023-01-09 Thread Jonas Devlieghere via lldb-commits 

Author: Jonas Devlieghere
Date: 2023-01-09T14:34:57-08:00
New Revision: 8b259fe573e109ea03c91aca34d5bd60f1a432ff

URL: 
https://github.com/llvm/llvm-project/commit/8b259fe573e109ea03c91aca34d5bd60f1a432ff
DIFF: 
https://github.com/llvm/llvm-project/commit/8b259fe573e109ea03c91aca34d5bd60f1a432ff.diff

LOG: [lldb] Trust the arange accelerator tables in dSYMs

When ingesting aranges from a dSYM, always trust the contents of the
accelerator table since it always comes from dsymutil.

According to Instruments, skipping the decoding of all CU DIEs to get at
the DW_AT_ranges attribute removes ~3.5 seconds from setting a
breakpoint by file/line when debugging clang with a dSYM. Interestingly
on the wall clock the speedup is less noticeable, but still present.

rdar://problem/56057688

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

Added: 


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

Removed: 




diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
index 8933b0804a01..47f9672ba596 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
@@ -53,13 +53,20 @@ const DWARFDebugAranges 
&DWARFDebugInfo::GetCompileUnitAranges() {
   }
 
   // Manually build arange data for everything that wasn't in .debug_aranges.
-  const size_t num_units = GetNumUnits();
-  for (size_t idx = 0; idx < num_units; ++idx) {
-DWARFUnit *cu = GetUnitAtIndex(idx);
-
-dw_offset_t offset = cu->GetOffset();
-if (cus_with_data.find(offset) == cus_with_data.end())
-  cu->BuildAddressRangeTable(m_cu_aranges_up.get());
+  // Skip this step for dSYMs as we can trust dsymutil to have emitted complete
+  // aranges.
+  const bool is_dsym =
+  m_dwarf.GetObjectFile() &&
+  m_dwarf.GetObjectFile()->GetType() == ObjectFile::eTypeDebugInfo;
+  if (!is_dsym) {
+const size_t num_units = GetNumUnits();
+for (size_t idx = 0; idx < num_units; ++idx) {
+  DWARFUnit *cu = GetUnitAtIndex(idx);
+
+  dw_offset_t offset = cu->GetOffset();
+  if (cus_with_data.find(offset) == cus_with_data.end())
+cu->BuildAddressRangeTable(m_cu_aranges_up.get());
+}
   }
 
   const bool minimize = true;



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

[Lldb-commits] [PATCH] D68655: Trust the arange accelerator tables in dSYMs

2023-01-09 Thread Jonas Devlieghere via Phabricator via lldb-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rG8b259fe573e1: [lldb] Trust the arange accelerator tables in 
dSYMs (authored by JDevlieghere).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68655

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


Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
@@ -53,13 +53,20 @@
   }
 
   // Manually build arange data for everything that wasn't in .debug_aranges.
-  const size_t num_units = GetNumUnits();
-  for (size_t idx = 0; idx < num_units; ++idx) {
-DWARFUnit *cu = GetUnitAtIndex(idx);
-
-dw_offset_t offset = cu->GetOffset();
-if (cus_with_data.find(offset) == cus_with_data.end())
-  cu->BuildAddressRangeTable(m_cu_aranges_up.get());
+  // Skip this step for dSYMs as we can trust dsymutil to have emitted complete
+  // aranges.
+  const bool is_dsym =
+  m_dwarf.GetObjectFile() &&
+  m_dwarf.GetObjectFile()->GetType() == ObjectFile::eTypeDebugInfo;
+  if (!is_dsym) {
+const size_t num_units = GetNumUnits();
+for (size_t idx = 0; idx < num_units; ++idx) {
+  DWARFUnit *cu = GetUnitAtIndex(idx);
+
+  dw_offset_t offset = cu->GetOffset();
+  if (cus_with_data.find(offset) == cus_with_data.end())
+cu->BuildAddressRangeTable(m_cu_aranges_up.get());
+}
   }
 
   const bool minimize = true;


Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
@@ -53,13 +53,20 @@
   }
 
   // Manually build arange data for everything that wasn't in .debug_aranges.
-  const size_t num_units = GetNumUnits();
-  for (size_t idx = 0; idx < num_units; ++idx) {
-DWARFUnit *cu = GetUnitAtIndex(idx);
-
-dw_offset_t offset = cu->GetOffset();
-if (cus_with_data.find(offset) == cus_with_data.end())
-  cu->BuildAddressRangeTable(m_cu_aranges_up.get());
+  // Skip this step for dSYMs as we can trust dsymutil to have emitted complete
+  // aranges.
+  const bool is_dsym =
+  m_dwarf.GetObjectFile() &&
+  m_dwarf.GetObjectFile()->GetType() == ObjectFile::eTypeDebugInfo;
+  if (!is_dsym) {
+const size_t num_units = GetNumUnits();
+for (size_t idx = 0; idx < num_units; ++idx) {
+  DWARFUnit *cu = GetUnitAtIndex(idx);
+
+  dw_offset_t offset = cu->GetOffset();
+  if (cus_with_data.find(offset) == cus_with_data.end())
+cu->BuildAddressRangeTable(m_cu_aranges_up.get());
+}
   }
 
   const bool minimize = true;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D141021: [lldb] Add lldb-framework-cleanup target

2023-01-09 Thread Alex Langford via Phabricator via lldb-commits 
bulbazord added a comment.

I think this idea will work but I have a few comments and questions:

Based on my understanding of this change, we're supposed to manually run the 
build system with the target `lldb-framework-cleanup` before we perform an 
install. Is this the case? This seems most useful when you have a script that 
controls the build process (e.g. swift's `build-script`).
I suppose this doesn't really change anything if you don't know about this new 
target, but it would be useful to document this somewhere (perhaps in 
`docs/resources/build.rst`?)

> Making this a dependency of the install target is not sufficient as the 
> target needs to be build before the install phase.

I can see why making the `install` target depend on `lldb-framework-cleanup` is 
not good, but what about making it a dependency of the target that installs the 
framework? I think it's something like `install-liblldb`? You could then 
guarantee that the cleanup happens before we install the framework. Maybe this 
is still too naïve or brittle though, what do you think?


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

https://reviews.llvm.org/D141021

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

[Lldb-commits] [PATCH] D141021: [lldb] Add lldb-framework-cleanup target

2023-01-09 Thread Med Ismail Bennani via Phabricator via lldb-commits 
mib added inline comments.



Comment at: lldb/cmake/modules/AddLLDB.cmake:247-249
+  # Create a target to remove the target again before the install phase. We
+  # intentionally use remove_directory because the target can be a directory
+  # and it's harmless for files.

The comment here is confusing to me. May be you can try to re-phrase it.


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

https://reviews.llvm.org/D141021

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

[Lldb-commits] [lldb] f8d7ab8 - Return a shared_ptr from ScratchTypeSystemClang::GetForTarget()

2023-01-09 Thread Adrian Prantl via lldb-commits 

Author: Adrian Prantl
Date: 2023-01-09T15:04:53-08:00
New Revision: f8d7ab8cf8e859dcc7f696e8d01ed6fca502446a

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

LOG: Return a shared_ptr from ScratchTypeSystemClang::GetForTarget()

The current interface theoretically could lead to a use-after-free
when a client holds on to the returned pointer. Fix this by returning
a shared_ptr to the scratch typesystem.

rdar://103619233

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

Added: 


Modified: 
lldb/include/lldb/lldb-forward.h
lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp
lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp
lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOSXDYLD.cpp
lldb/source/Plugins/ExpressionParser/Clang/ASTResultSynthesizer.cpp
lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
lldb/source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.cpp
lldb/source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.h
lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
lldb/source/Plugins/Language/ObjC/NSArray.cpp
lldb/source/Plugins/Language/ObjC/NSDictionary.cpp
lldb/source/Plugins/Language/ObjC/NSError.cpp
lldb/source/Plugins/Language/ObjC/NSException.cpp
lldb/source/Plugins/Language/ObjC/NSIndexPath.cpp

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

lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntime.cpp

lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp

lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp
lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
lldb/source/Plugins/SystemRuntime/MacOSX/AppleGetItemInfoHandler.cpp
lldb/source/Plugins/SystemRuntime/MacOSX/AppleGetPendingItemsHandler.cpp
lldb/source/Plugins/SystemRuntime/MacOSX/AppleGetQueuesHandler.cpp
lldb/source/Plugins/SystemRuntime/MacOSX/AppleGetThreadItemInfoHandler.cpp
lldb/source/Plugins/SystemRuntime/MacOSX/SystemRuntimeMacOSX.cpp
lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h

Removed: 




diff  --git a/lldb/include/lldb/lldb-forward.h 
b/lldb/include/lldb/lldb-forward.h
index 5774645924e48..24ec83b1a20ef 100644
--- a/lldb/include/lldb/lldb-forward.h
+++ b/lldb/include/lldb/lldb-forward.h
@@ -258,6 +258,7 @@ class TypeNameSpecifierImpl;
 class TypeSummaryImpl;
 class TypeSummaryOptions;
 class TypeSystem;
+class TypeSystemClang;
 class UUID;
 class UnixSignals;
 class Unwind;
@@ -432,6 +433,7 @@ typedef 
std::shared_ptr
 typedef std::shared_ptr TypeEnumMemberImplSP;
 typedef std::shared_ptr TypeFilterImplSP;
 typedef std::shared_ptr TypeSystemSP;
+typedef std::shared_ptr TypeSystemClangSP;
 typedef std::weak_ptr TypeSystemWP;
 typedef std::shared_ptr TypeFormatImplSP;
 typedef std::shared_ptr

diff  --git 
a/lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp 
b/lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp
index a54d4c062f375..a72d888c51e71 100644
--- a/lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp
+++ b/lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp
@@ -1112,14 +1112,14 @@ DynamicLoaderDarwin::GetThreadLocalData(const 
lldb::ModuleSP module_sp,
 }
 StackFrameSP frame_sp = thread_sp->GetStackFrameAtIndex(0);
 if (frame_sp) {
-  TypeSystemClang *clang_ast_context =
+  TypeSystemClangSP scratch_ts_sp =
   ScratchTypeSystemClang::GetForTarget(target);
 
-  if (!clang_ast_context)
+  if (!scratch_ts_sp)
 return LLDB_INVALID_ADDRESS;
 
   CompilerType clang_void_ptr_type =
-  clang_ast_context->GetBasicType(eBasicTypeVoid).GetPointerType();
+  scratch_ts_sp->GetBasicType(eBasicTypeVoid).GetPointerType();
   Address pthread_getspecific_addr = GetPthreadSetSpecificAddress();
   if (pthread_getspecific_addr.IsValid()) {
 EvaluateExpressionOptions options;

diff  --git 
a/lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp 
b/lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp
index 74db38e4f4efe..dedceb7c9043f 100644
--- a/lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp
+++ b/lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp
@@ -26

[Lldb-commits] [PATCH] D141100: Return a shared_ptr from ScratchTypeSystemClang::GetForTarget()

2023-01-09 Thread Adrian Prantl via Phabricator via lldb-commits 
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGf8d7ab8cf8e8: Return a shared_ptr from 
ScratchTypeSystemClang::GetForTarget() (authored by aprantl).
Herald added a subscriber: lldb-commits.

Changed prior to commit:
  https://reviews.llvm.org/D141100?vs=487558&id=487569#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141100

Files:
  lldb/include/lldb/lldb-forward.h
  lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp
  lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp
  lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOSXDYLD.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ASTResultSynthesizer.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
  lldb/source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.h
  lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
  lldb/source/Plugins/Language/ObjC/NSArray.cpp
  lldb/source/Plugins/Language/ObjC/NSDictionary.cpp
  lldb/source/Plugins/Language/ObjC/NSError.cpp
  lldb/source/Plugins/Language/ObjC/NSException.cpp
  lldb/source/Plugins/Language/ObjC/NSIndexPath.cpp
  
lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp
  lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntime.cpp
  
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
  
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp
  lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
  lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
  lldb/source/Plugins/SystemRuntime/MacOSX/AppleGetItemInfoHandler.cpp
  lldb/source/Plugins/SystemRuntime/MacOSX/AppleGetPendingItemsHandler.cpp
  lldb/source/Plugins/SystemRuntime/MacOSX/AppleGetQueuesHandler.cpp
  lldb/source/Plugins/SystemRuntime/MacOSX/AppleGetThreadItemInfoHandler.cpp
  lldb/source/Plugins/SystemRuntime/MacOSX/SystemRuntimeMacOSX.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h

Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
@@ -1182,7 +1182,7 @@
   /// this parameter is false, this function returns a nullptr.
   /// \return The scratch type system of the target or a nullptr in case an
   /// error occurred.
-  static TypeSystemClang *
+  static lldb::TypeSystemClangSP
   GetForTarget(Target &target,
std::optional ast_kind = DefaultAST,
bool create_on_demand = true);
@@ -1194,8 +1194,8 @@
   /// \param lang_opts The LangOptions of a clang ASTContext that the caller
   ///  wants to export type information from. This is used to
   ///  find the best matching sub-AST that will be returned.
-  static TypeSystemClang *GetForTarget(Target &target,
-   const clang::LangOptions &lang_opts) {
+  static lldb::TypeSystemClangSP
+  GetForTarget(Target &target, const clang::LangOptions &lang_opts) {
 return GetForTarget(target, InferIsolatedASTKindFromLangOpts(lang_opts));
   }
 
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -12,6 +12,7 @@
 #include "llvm/Support/FormatVariadic.h"
 
 #include 
+#include 
 #include 
 #include 
 
@@ -9922,7 +9923,7 @@
   m_scratch_ast_source_up.reset();
 }
 
-TypeSystemClang *
+TypeSystemClangSP
 ScratchTypeSystemClang::GetForTarget(Target &target,
  std::optional ast_kind,
  bool create_on_demand) {
@@ -9933,16 +9934,17 @@
"Couldn't get scratch TypeSystemClang");
 return nullptr;
   }
-  auto ts = *type_system_or_err;
+  auto ts_sp = *type_system_or_err;
   ScratchTypeSystemClang *scratch_ast =
-  llvm::dyn_cast_or_null(ts.get());
+  llvm::dyn_cast_or_null(ts_sp.get());
   if (!scratch_ast)
 return nullptr;
   // If no dedicated sub-AST was requested, just return the main AST.
   if (ast_kind == DefaultAST)
-return scratch_ast;
+return std::static_pointer_cast(ts_sp);
   // Search the sub-ASTs.
-  return &scratch_ast->GetIsolatedAST(*ast_kind);
+  return std::static_pointer_cast(
+  scratch_ast->GetIsolatedAST(*ast_kind).shared_from_

[Lldb-commits] [PATCH] D141318: [lldb] Store shared pointers in DieToTypePtr map instead of raw pointers

2023-01-09 Thread Augusto Noronha via Phabricator via lldb-commits 
augusto2112 added a comment.

In D141318#4037687 , @JDevlieghere 
wrote:

> In D141318#4037640 , @augusto2112 
> wrote:
>
>> In D141318#4037557 , @JDevlieghere 
>> wrote:
>>
>>> In D141318#4037541 , @augusto2112 
>>> wrote:
>>>
 I changed the value of the map to a shared pointer, but could change it to 
 a weak pointer instead, not sure which is more appropriate in this case.
>>>
>>> I was about to ask the same thing. What currently determines the lifetime 
>>> of the stored objects?
>>
>> Currently, a bunch of functions in `DWARFASTParserClang` and 
>> `SymbolFileDWARF` will create a new `Type` SP, store the underlying pointer 
>> in the map, and return the SP. The lifetime of the stored objects varies by 
>> whatever the caller does with the returned SP. Looking at some of the 
>> callers, I don't see a very clear pattern, most seem to use it and 
>> immediately discard it  (use after free when we look up the same type again 
>> and the underlying pointer is still in the map), but at least one caller 
>> (`SymbolFileDWARF::ResolveType`) returns the underlying pointer to //its// 
>> callers, which would still be UB even if we changed the map to store weak 
>> pointers. Based on that I'm thinking we should store them as shared pointers 
>> instead of weak ones.
>
> Then maybe the solution is to store the type as unique pointers in the map 
> and hand out raw pointers, relying on the map to keep the object alive? I 
> feel pretty uncomfortable with handing out shared pointers without clearly 
> understanding its lifetime cycle.

Yeah that seems like a better solution. I'm checking how hard implementing that 
would be. `TypeSP` is used in a bunch of places in LLDB.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141318

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

[Lldb-commits] [lldb] 9b737f1 - [lldb] Limit trusting aranges to dSYMs only.

2023-01-09 Thread Jonas Devlieghere via lldb-commits 

Author: Jonas Devlieghere
Date: 2023-01-09T15:38:05-08:00
New Revision: 9b737f148d88501a0a778e1adacf342108286bb0

URL: 
https://github.com/llvm/llvm-project/commit/9b737f148d88501a0a778e1adacf342108286bb0
DIFF: 
https://github.com/llvm/llvm-project/commit/9b737f148d88501a0a778e1adacf342108286bb0.diff

LOG: [lldb] Limit trusting aranges to dSYMs only.

Limit trusting the arange accelerator tables (8b259fe573e1) to dSYMs
only, and not any debug info object file.

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

Added: 


Modified: 
lldb/include/lldb/Symbol/ObjectFile.h
lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp

Removed: 




diff  --git a/lldb/include/lldb/Symbol/ObjectFile.h 
b/lldb/include/lldb/Symbol/ObjectFile.h
index 4d6c8f1105a04..e2e0623c9f9ed 100644
--- a/lldb/include/lldb/Symbol/ObjectFile.h
+++ b/lldb/include/lldb/Symbol/ObjectFile.h
@@ -682,6 +682,10 @@ class ObjectFile : public 
std::enable_shared_from_this,
 return symbol_name;
   }
 
+  /// Can we trust the address ranges accelerator associated with this object
+  /// file to be complete.
+  virtual bool CanTrustAddressRanges() { return false; }
+
   static lldb::SymbolType GetSymbolTypeFromName(
   llvm::StringRef name,
   lldb::SymbolType symbol_type_hint = lldb::eSymbolTypeUndefined);

diff  --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp 
b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
index 767ab87c2f529..84856590fe8ad 100644
--- a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -6094,6 +6094,12 @@ bool ObjectFileMachO::GetIsDynamicLinkEditor() {
   return m_header.filetype == llvm::MachO::MH_DYLINKER;
 }
 
+bool ObjectFileMachO::CanTrustAddressRanges() {
+  // Dsymutil guarantees that the .debug_aranges accelerator is complete and 
can
+  // be trusted by LLDB.
+  return m_header.filetype == llvm::MachO::MH_DSYM;
+}
+
 bool ObjectFileMachO::AllowAssemblyEmulationUnwindPlans() {
   return m_allow_assembly_emulation_unwind_plans;
 }

diff  --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h 
b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
index 343e3bc004b6f..3b458d8da8342 100644
--- a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
+++ b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
@@ -143,6 +143,8 @@ class ObjectFileMachO : public lldb_private::ObjectFile {
 
   bool GetIsDynamicLinkEditor() override;
 
+  bool CanTrustAddressRanges() override;
+
   static bool ParseHeader(lldb_private::DataExtractor &data,
   lldb::offset_t *data_offset_ptr,
   llvm::MachO::mach_header &header);

diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
index 47f9672ba5968..b631985d60c45 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
@@ -53,12 +53,13 @@ const DWARFDebugAranges 
&DWARFDebugInfo::GetCompileUnitAranges() {
   }
 
   // Manually build arange data for everything that wasn't in .debug_aranges.
-  // Skip this step for dSYMs as we can trust dsymutil to have emitted complete
-  // aranges.
-  const bool is_dsym =
-  m_dwarf.GetObjectFile() &&
-  m_dwarf.GetObjectFile()->GetType() == ObjectFile::eTypeDebugInfo;
-  if (!is_dsym) {
+  // The .debug_aranges accelerator is not guaranteed to be complete.
+  // Tools such as dsymutil can provide stronger guarantees than required by 
the
+  // standard. Without that guarantee, we have to iterate over every CU in the
+  // .debug_info and make sure there's a corresponding entry in the table and 
if
+  // not, add one for every subprogram.
+  ObjectFile *OF = m_dwarf.GetObjectFile();
+  if (!OF || !OF->CanTrustAddressRanges()) {
 const size_t num_units = GetNumUnits();
 for (size_t idx = 0; idx < num_units; ++idx) {
   DWARFUnit *cu = GetUnitAtIndex(idx);



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

[Lldb-commits] [PATCH] D141330: [lldb] Limit 8b259fe573e1 to dSYMs

2023-01-09 Thread Jonas Devlieghere via Phabricator via lldb-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rG9b737f148d88: [lldb] Limit trusting aranges to dSYMs only. 
(authored by JDevlieghere).
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Changed prior to commit:
  https://reviews.llvm.org/D141330?vs=487579&id=487586#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141330

Files:
  lldb/include/lldb/Symbol/ObjectFile.h
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp


Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
@@ -53,12 +53,13 @@
   }
 
   // Manually build arange data for everything that wasn't in .debug_aranges.
-  // Skip this step for dSYMs as we can trust dsymutil to have emitted complete
-  // aranges.
-  const bool is_dsym =
-  m_dwarf.GetObjectFile() &&
-  m_dwarf.GetObjectFile()->GetType() == ObjectFile::eTypeDebugInfo;
-  if (!is_dsym) {
+  // The .debug_aranges accelerator is not guaranteed to be complete.
+  // Tools such as dsymutil can provide stronger guarantees than required by 
the
+  // standard. Without that guarantee, we have to iterate over every CU in the
+  // .debug_info and make sure there's a corresponding entry in the table and 
if
+  // not, add one for every subprogram.
+  ObjectFile *OF = m_dwarf.GetObjectFile();
+  if (!OF || !OF->CanTrustAddressRanges()) {
 const size_t num_units = GetNumUnits();
 for (size_t idx = 0; idx < num_units; ++idx) {
   DWARFUnit *cu = GetUnitAtIndex(idx);
Index: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
===
--- lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
+++ lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
@@ -143,6 +143,8 @@
 
   bool GetIsDynamicLinkEditor() override;
 
+  bool CanTrustAddressRanges() override;
+
   static bool ParseHeader(lldb_private::DataExtractor &data,
   lldb::offset_t *data_offset_ptr,
   llvm::MachO::mach_header &header);
Index: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
===
--- lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -6094,6 +6094,12 @@
   return m_header.filetype == llvm::MachO::MH_DYLINKER;
 }
 
+bool ObjectFileMachO::CanTrustAddressRanges() {
+  // Dsymutil guarantees that the .debug_aranges accelerator is complete and 
can
+  // be trusted by LLDB.
+  return m_header.filetype == llvm::MachO::MH_DSYM;
+}
+
 bool ObjectFileMachO::AllowAssemblyEmulationUnwindPlans() {
   return m_allow_assembly_emulation_unwind_plans;
 }
Index: lldb/include/lldb/Symbol/ObjectFile.h
===
--- lldb/include/lldb/Symbol/ObjectFile.h
+++ lldb/include/lldb/Symbol/ObjectFile.h
@@ -682,6 +682,10 @@
 return symbol_name;
   }
 
+  /// Can we trust the address ranges accelerator associated with this object
+  /// file to be complete.
+  virtual bool CanTrustAddressRanges() { return false; }
+
   static lldb::SymbolType GetSymbolTypeFromName(
   llvm::StringRef name,
   lldb::SymbolType symbol_type_hint = lldb::eSymbolTypeUndefined);


Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
@@ -53,12 +53,13 @@
   }
 
   // Manually build arange data for everything that wasn't in .debug_aranges.
-  // Skip this step for dSYMs as we can trust dsymutil to have emitted complete
-  // aranges.
-  const bool is_dsym =
-  m_dwarf.GetObjectFile() &&
-  m_dwarf.GetObjectFile()->GetType() == ObjectFile::eTypeDebugInfo;
-  if (!is_dsym) {
+  // The .debug_aranges accelerator is not guaranteed to be complete.
+  // Tools such as dsymutil can provide stronger guarantees than required by the
+  // standard. Without that guarantee, we have to iterate over every CU in the
+  // .debug_info and make sure there's a corresponding entry in the table and if
+  // not, add one for every subprogram.
+  ObjectFile *OF = m_dwarf.GetObjectFile();
+  if (!OF || !OF->CanTrustAddressRanges()) {
 const size_t num_units = GetNumUnits();
 for (size_t idx = 0; idx < num_units; ++idx) {
   DWARFUnit *cu = GetUnitAtIndex(idx);
Index: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h

[Lldb-commits] [PATCH] D139379: [llvm][dwwarf] Change CU/TU index to 64-bit

2023-01-09 Thread David Blaikie via Phabricator via lldb-commits 
dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.



In D139379#4015624 , @ayermolo wrote:

> In D139379#4015569 , @dblaikie 
> wrote:
>
>> In D139379#3972876 , @ayermolo 
>> wrote:
>>
>>> In D139379#3972871 , @dblaikie 
>>> wrote:
>>>
 Perhaps the change to use accessors could be removed, now that you've used 
 it to find all the places that needed to be fixed up? (like just using it 
 for cleanup/temporary purposes, without needing to commit that API change?)
>>>
>>> I am not sure what other projects are using it, that I missed, or not in 
>>> llvm-trunk, but are based of it.
>>
>> It's awkward to convolute the API to ensure a breakage - I think it's best 
>> to leave it as-is, for the most part. I guess you needed the 32 bit 
>> accessors so existing code doesn't become UB because of truncation to 32 bit?
>>
>> Could the code keep the existing member names, provide the wrappers without 
>> turning the members into an array and needing named index constants, etc, at 
>> least? (though even then, seems like there's more to it than needed)
>
> Thanks for circling back to this during holidays. :)
> Right, that was my original thought pattern. I am fully open to the idea that 
> this is not the right approach. :)

I don't know that there's enough out of tree usage to worry about forcing a 
break by changing the API like this - or to include the "get*32" functions.

Is this actually an undefined behavior issue (I don't think the implicit 
conversion would be any more broken than the explicit cast, at least - but 
can't find the specific wording about defined/undefined behavior off hand at 
the moment) , or only an attempt to identify places that could benefit from 
updates to support 64 bit lengths?

Eh, still seems weird, but guess it's not that much of a big deal - so let's go 
with it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139379

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

[Lldb-commits] [PATCH] D141021: [lldb] Add lldb-framework-cleanup target

2023-01-09 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere added a comment.

In D141021#4037840 , @bulbazord wrote:

> I think this idea will work but I have a few comments and questions:
>
> Based on my understanding of this change, we're supposed to manually run the 
> build system with the target `lldb-framework-cleanup` before we perform an 
> install. Is this the case? This seems most useful when you have a script that 
> controls the build process (e.g. swift's `build-script`).
> I suppose this doesn't really change anything if you don't know about this 
> new target, but it would be useful to document this somewhere (perhaps in 
> `docs/resources/build.rst`?)
>
>> Making this a dependency of the install target is not sufficient as the 
>> target needs to be build before the install phase.
>
> I can see why making the `install` target depend on `lldb-framework-cleanup` 
> is not good, but what about making it a dependency of the target that 
> installs the framework? I think it's something like `install-liblldb`? You 
> could then guarantee that the cleanup happens before we install the 
> framework. Maybe this is still too naïve or brittle though, what do you think?

Interesting, I tried the exact thing that you described and my custom command 
wasn't run, but I cannot reproduce that anymore. I don't know what I did 
differently then, but as this seems to work and is clearly more desirable, I'll 
update the patch.


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

https://reviews.llvm.org/D141021

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

[Lldb-commits] [PATCH] D141021: [lldb] Add lldb-framework-cleanup target

2023-01-09 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere updated this revision to Diff 487613.
JDevlieghere marked an inline comment as done.
JDevlieghere added a comment.

Make `lldb-framework-cleanup` a dependency of `install-liblldb`.


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

https://reviews.llvm.org/D141021

Files:
  lldb/cmake/modules/AddLLDB.cmake
  lldb/cmake/modules/LLDBConfig.cmake
  lldb/source/API/CMakeLists.txt


Index: lldb/source/API/CMakeLists.txt
===
--- lldb/source/API/CMakeLists.txt
+++ lldb/source/API/CMakeLists.txt
@@ -211,4 +211,7 @@
 
 if(LLDB_BUILD_FRAMEWORK)
   include(LLDBFramework)
+
+  add_dependencies(install-liblldb
+lldb-framework-cleanup)
 endif()
Index: lldb/cmake/modules/LLDBConfig.cmake
===
--- lldb/cmake/modules/LLDBConfig.cmake
+++ lldb/cmake/modules/LLDBConfig.cmake
@@ -101,6 +101,20 @@
   # Essentially, emit the framework's dSYM outside of the framework directory.
   set(LLDB_DEBUGINFO_INSTALL_PREFIX 
${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/bin CACHE STRING
   "Directory to emit dSYM files stripped from executables and libraries 
(Darwin Only)")
+
+  # Custom target to remove the targets (binaries, directories) that were
+  # copied into LLDB.framework in the build tree.
+  #
+  # These targets need to be removed before the install phase because otherwise
+  # because otherwise they may overwrite already installed binaries with the
+  # wrong RPATH (i.e. build RPATH instead of install RPATH).
+  #
+  # This target needs to be created here (rather than in API/CMakeLists.txt)
+  # because add_lldb_tool creates the custom rules to copy the binaries before
+  # the framework target exists and that's the only place where this is
+  # tracked.
+  add_custom_target(lldb-framework-cleanup
+COMMENT "Cleaning up build-tree frameworks in preparation for install")
 endif()
 
 if(APPLE AND CMAKE_GENERATOR STREQUAL Xcode)
Index: lldb/cmake/modules/AddLLDB.cmake
===
--- lldb/cmake/modules/AddLLDB.cmake
+++ lldb/cmake/modules/AddLLDB.cmake
@@ -243,6 +243,16 @@
 COMMAND ${CMAKE_COMMAND} -E copy $ ${copy_dest}
 COMMENT "Copy ${name} to ${copy_dest}"
   )
+
+  # Create a custom target to remove the copy again from LLDB.framework in the
+  # build tree.
+  # Intentionally use remove_directory because the target can be a either a
+  # file or directory and using remove_directory is harmless for files.
+  add_custom_target(${name}-cleanup
+COMMAND ${CMAKE_COMMAND} -E remove_directory ${copy_dest}
+COMMENT "Removing ${name} from LLDB.framework")
+  add_dependencies(lldb-framework-cleanup
+${name}-cleanup)
 endfunction()
 
 # Add extra install steps for dSYM creation and stripping for the given target.


Index: lldb/source/API/CMakeLists.txt
===
--- lldb/source/API/CMakeLists.txt
+++ lldb/source/API/CMakeLists.txt
@@ -211,4 +211,7 @@
 
 if(LLDB_BUILD_FRAMEWORK)
   include(LLDBFramework)
+
+  add_dependencies(install-liblldb
+lldb-framework-cleanup)
 endif()
Index: lldb/cmake/modules/LLDBConfig.cmake
===
--- lldb/cmake/modules/LLDBConfig.cmake
+++ lldb/cmake/modules/LLDBConfig.cmake
@@ -101,6 +101,20 @@
   # Essentially, emit the framework's dSYM outside of the framework directory.
   set(LLDB_DEBUGINFO_INSTALL_PREFIX ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/bin CACHE STRING
   "Directory to emit dSYM files stripped from executables and libraries (Darwin Only)")
+
+  # Custom target to remove the targets (binaries, directories) that were
+  # copied into LLDB.framework in the build tree.
+  #
+  # These targets need to be removed before the install phase because otherwise
+  # because otherwise they may overwrite already installed binaries with the
+  # wrong RPATH (i.e. build RPATH instead of install RPATH).
+  #
+  # This target needs to be created here (rather than in API/CMakeLists.txt)
+  # because add_lldb_tool creates the custom rules to copy the binaries before
+  # the framework target exists and that's the only place where this is
+  # tracked.
+  add_custom_target(lldb-framework-cleanup
+COMMENT "Cleaning up build-tree frameworks in preparation for install")
 endif()
 
 if(APPLE AND CMAKE_GENERATOR STREQUAL Xcode)
Index: lldb/cmake/modules/AddLLDB.cmake
===
--- lldb/cmake/modules/AddLLDB.cmake
+++ lldb/cmake/modules/AddLLDB.cmake
@@ -243,6 +243,16 @@
 COMMAND ${CMAKE_COMMAND} -E copy $ ${copy_dest}
 COMMENT "Copy ${name} to ${copy_dest}"
   )
+
+  # Create a custom target to remove the copy again from LLDB.framework in the
+  # build tree.
+  # Intentionally use remove_directory because the target can be a either a
+  # file or directory and using remove_directory is harml

[Lldb-commits] [PATCH] D140368: [lldb] Consider all breakpoints in breakpoint detection

2023-01-09 Thread Jim Ingham via Phabricator via lldb-commits 
jingham requested changes to this revision.
jingham added a comment.
This revision now requires changes to proceed.

I think this patch is correct, but could be clearer - mostly because the 
original code didn't choose good enough names.

What's going on here is that the "internal_breakpoint" variable is getting used 
in two ways.  In the loop over breakpoint locations we're using it to cache the 
result of `IsInternal()` for the current breakpoint location.  But then outside 
the loop we really want it to mean "were all the breakpoint locations that said 
we should stop internal breakpoints?", which gets used in:

  if ((!m_should_stop || internal_breakpoint) &&
  thread_sp->CompletedPlanOverridesBreakpoint()) {

The two are equivalent when there aren't other active & matching breakpoints at 
the same site, but not otherwise, so you're right we need to fix that.

Your patch eliminates the first use and makes `internal_breakpoint` just follow 
the outer meaning.   That's fine, but then `internal_breakpoint` ends up being 
a pretty confusing name - given we've just iterated over a bunch of 
breakpoints.  Instead, it should have a name that indicates it is a value 
computed from ALL the breakpoint locations we looked at, something like 
`all_stopping_locs_internal`.  That starts as true, and if you see any 
non-internal breakpoint says we should stop, then turning this to `false` will 
make total sense.  And the outer check will also make more sense, it will be:

  if ((!m_should_stop || all_stopping_locs_internal) &&
  thread_sp->CompletedPlanOverridesBreakpoint()) {

I also had a couple of clean-up suggestions for the test.  These should be 
pretty simple cleanups.




Comment at: 
lldb/test/API/functionalities/breakpoint/thread_plan_user_breakpoint/TestThreadPlanUserBreakpoint.py:31
+# Setup three breakpoints
+self.lines = [line_number('main.cpp', "breakpoint_%i" % i) for i in 
range(3)]
+

It's easier to use BreakpointCreateBySourceRegex for this, since you can do the 
search & set in one step.



Comment at: 
lldb/test/API/functionalities/breakpoint/thread_plan_user_breakpoint/TestThreadPlanUserBreakpoint.py:34
+self.breakpoints = [self.target.BreakpointCreateByLocation(src, line) 
for line in self.lines]
+self.assertTrue(
+self.breakpoints[0] and self.breakpoints[0].GetNumLocations() == 1,

Why do you only check one of the breakpoints?



Comment at: 
lldb/test/API/functionalities/breakpoint/thread_plan_user_breakpoint/TestThreadPlanUserBreakpoint.py:39
+# Start debugging
+self.process = self.target.LaunchSimple(
+None, None, self.get_process_working_directory())

You are making three breakpoints, but really the one in main is the "stop to 
start the test breakpoint" and the other two are the ones for the test.  So 
this would all be more compact if you did:


```
(target, process, thread, _) = lldbutil.run_to_source_breakpoint(self, 
"breakpoint_0", lldb.SBFileSpec("main.cpp")

```

That will do all the jobs of making the target, setting the initial breakpoint, 
starting a process, and making sure that you hit the initial breakpoint.  Then 
you can just make the other two breakpoints and continue on as you have done 
here.



Comment at: 
lldb/test/API/functionalities/breakpoint/thread_plan_user_breakpoint/TestThreadPlanUserBreakpoint.py:75
+# Make sure we install the breakpoint at the right address:
+# on some architectures (e.g, aarch64), step-out stops before the next 
source line
+return_addr = self.thread.GetFrameAtIndex(1).GetPC()

Step out ALWAYS stops directly on returning to the caller frame.  You need that 
so that if you have:


```
foo(bar(), baz());

```
and are stopped in `bar` below this code, then `step-out` followed by `step-in` 
will land you in `baz`.  If `step-out` finished the source line, you would be 
past the call to `baz` before you got control back.

The only architecture dependency here is whether the compiler needed to emit 
more code for a given source line after the return from the callee.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140368

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

[Lldb-commits] [PATCH] D141219: Add a .lldbinit file to autoload LLVM/Clang data formatters

2023-01-09 Thread Med Ismail Bennani via Phabricator via lldb-commits 
mib added a comment.

I guess we could even throw MLIR formatters in there for completeness




Comment at: .lldbinit:2-3
+# To autoload set `settings set target.load-cwd-lldbinit true` in ~/.lldbinit
+command script import ./llvm/utils/lldbDataFormatters.py
+command script import ./clang/utils/ClangDataFormat.py




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141219

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

[Lldb-commits] [PATCH] D141219: Add a .lldbinit file to autoload LLVM/Clang data formatters

2023-01-09 Thread Med Ismail Bennani via Phabricator via lldb-commits 
mib added a subscriber: mehdi_amini.
mib added a comment.

In D141219#4038658 , @mib wrote:

> I guess we could even throw MLIR formatters in there for completeness

I don't know exactly how well those are maintained ... @mehdi_amini do you 
think it would be safe to add these here ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141219

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