[Lldb-commits] [lldb] 7319d7d - [lldb] Fix -Wunused-variable in -DLLVM_ENABLE_ASSERTIONS=off builds after D154542

[Fangrui Song via lldb-commits]

Author: Fangrui Song
Date: 2023-07-06T20:33:09-07:00
New Revision: 7319d7dbcfa94998ae3befeff0b84140dcf29a69

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

LOG: [lldb] Fix -Wunused-variable in -DLLVM_ENABLE_ASSERTIONS=off builds after 
D154542

Added: 


Modified: 
lldb/source/Target/TargetList.cpp

Removed: 




diff  --git a/lldb/source/Target/TargetList.cpp 
b/lldb/source/Target/TargetList.cpp
index 376b15ca706bee..cb198e388011ad 100644
--- a/lldb/source/Target/TargetList.cpp
+++ b/lldb/source/Target/TargetList.cpp
@@ -559,15 +559,16 @@ bool TargetList::AnyTargetContainsModule(Module &module) {
 
   void TargetList::RegisterInProcessTarget(TargetSP target_sp) {
 std::lock_guard guard(m_target_list_mutex);
-std::unordered_set::iterator iter;
-bool was_added;
-std::tie(iter, was_added) = m_in_process_target_list.insert(target_sp);
+[[maybe_unused]] bool was_added;
+std::tie(std::ignore, was_added) =
+m_in_process_target_list.insert(target_sp);
 assert(was_added && "Target pointer was left in the in-process map");
   }
   
   void TargetList::UnregisterInProcessTarget(TargetSP target_sp) {
 std::lock_guard guard(m_target_list_mutex);
-bool was_present = m_in_process_target_list.erase(target_sp);
+[[maybe_unused]] bool was_present =
+m_in_process_target_list.erase(target_sp);
 assert(was_present && "Target pointer being removed was not registered");
   }
   



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

[Lldb-commits] [PATCH] D154542: Further refinements on reporting interruption in lldb

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

Hi! The added lines have some spaces at line ends. I think using 
`clang-format-diff.py` would remove them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154542

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

[Lldb-commits] [lldb] 61d8da3 - [lldb] Replace countPopulation with popcount after D153686

[Fangrui Song via lldb-commits]

Author: Fangrui Song
Date: 2023-07-06T20:26:23-07:00
New Revision: 61d8da327cab76932e8236bbf03543c9b3f7864c

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

LOG: [lldb] Replace countPopulation with popcount after D153686

Added: 


Modified: 
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.cpp

Removed: 




diff  --git 
a/lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.cpp 
b/lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.cpp
index cb6f81b28f7cc8..29f32e97f23296 100644
--- a/lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.cpp
+++ b/lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.cpp
@@ -651,7 +651,7 @@ 
NativeRegisterContextLinux_ppc64le::GetWatchpointSize(uint32_t wp_index) {
 
   unsigned control = (m_hwp_regs[wp_index].control >> 5) & 0xff;
   if (llvm::isPowerOf2_32(control + 1)) {
-return llvm::countPopulation(control);
+return llvm::popcount(control);
   }
 
   return 0;



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

[Lldb-commits] [lldb] 9895c89 - Revert "Change the dyld notification function that lldb puts a breakpoint in"

[Jason Molenda via lldb-commits]

Author: Jason Molenda
Date: 2023-07-06T18:23:54-07:00
New Revision: 9895c8979a4f025a112d0830a8f59f1820c8c49e

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

LOG: Revert "Change the dyld notification function that lldb puts a breakpoint 
in"

This reverts commit c3192196aea279eea0a87247a02f224ebe067c44.

Reverting my second attempt at https://reviews.llvm.org/D139453
changing which dyld notification method is being used.
The Intel macOS CI bot is still failing with this
rewrite at https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/
I'll need to set up an Intel macOS system running a matching
OS version to debug this directly I think.

Added: 


Modified: 
lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp

Removed: 




diff  --git 
a/lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp 
b/lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp
index b8723628d5118c..67e79fdcec8f56 100644
--- a/lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp
+++ b/lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp
@@ -233,13 +233,12 @@ bool DynamicLoaderMacOS::NotifyBreakpointHit(void *baton,
  StoppointCallbackContext *context,
  lldb::user_id_t break_id,
  lldb::user_id_t break_loc_id) {
-  //
-  // Our breakpoint on
-  //
-  // void lldb_image_notifier(enum dyld_image_mode mode, uint32_t infoCount,
-  // const dyld_image_info info[])
-  //
-  // has been hit.  We need to read the arguments.
+  // Let the event know that the images have changed
+  // DYLD passes three arguments to the notification breakpoint.
+  // Arg1: enum dyld_notify_mode mode - 0 = adding, 1 = removing, 2 = remove
+  // all Arg2: unsigned long icount- Number of shared libraries
+  // added/removed Arg3: uint64_t mach_headers[] - Array of load addresses
+  // of binaries added/removed
 
   DynamicLoaderMacOS *dyld_instance = (DynamicLoaderMacOS *)baton;
 
@@ -269,10 +268,9 @@ bool DynamicLoaderMacOS::NotifyBreakpointHit(void *baton,
 ValueList argument_values;
 
 Value mode_value;// enum dyld_notify_mode { dyld_notify_adding=0,
- // dyld_notify_removing=1, dyld_notify_remove_all=2,
- // dyld_notify_dyld_moved=3 };
-Value count_value;   // uint32_t
-Value headers_value; // struct dyld_image_info machHeaders[]
+ // dyld_notify_removing=1, dyld_notify_remove_all=2 };
+Value count_value;   // unsigned long count
+Value headers_value; // uint64_t machHeaders[] (aka void*)
 
 CompilerType clang_void_ptr_type =
 scratch_ts_sp->GetBasicType(eBasicTypeVoid).GetPointerType();
@@ -286,8 +284,13 @@ bool DynamicLoaderMacOS::NotifyBreakpointHit(void *baton,
 mode_value.SetValueType(Value::ValueType::Scalar);
 mode_value.SetCompilerType(clang_uint32_type);
 
-count_value.SetValueType(Value::ValueType::Scalar);
-count_value.SetCompilerType(clang_uint32_type);
+if (process->GetTarget().GetArchitecture().GetAddressByteSize() == 4) {
+  count_value.SetValueType(Value::ValueType::Scalar);
+  count_value.SetCompilerType(clang_uint32_type);
+} else {
+  count_value.SetValueType(Value::ValueType::Scalar);
+  count_value.SetCompilerType(clang_uint64_type);
+}
 
 headers_value.SetValueType(Value::ValueType::Scalar);
 headers_value.SetCompilerType(clang_void_ptr_type);
@@ -309,30 +312,12 @@ bool DynamicLoaderMacOS::NotifyBreakpointHit(void *baton,
   argument_values.GetValueAtIndex(2)->GetScalar().ULongLong(-1);
   if (header_array != static_cast(-1)) {
 std::vector image_load_addresses;
-// header_array points to an array of image_infos_count elements,
-// each is
-// struct dyld_image_info {
-//   const struct mach_header* imageLoadAddress;
-//   const char*   imageFilePath;
-//   uintptr_t imageFileModDate;
-// };
-//
-// and we only need the imageLoadAddress fields.
-
-const int addrsize =
-process->GetTarget().GetArchitecture().GetAddressByteSize();
 for (uint64_t i = 0; i < image_infos_count; i++) {
   Status error;
-  addr_t dyld_image_info = header_array + (addrsize * 3 * i);
-  addr_t addr =
-  process->ReadPointerFromMemory(dyld_image_info, error);
-  if (error.Success()) {
+  addr_t addr = process->ReadUnsignedIntegerFromMemory(
+  header_array + (8

[Lldb-commits] [lldb] c319219 - Change the dyld notification function that lldb puts a breakpoint in

[Jason Molenda via lldb-commits]

Author: Jason Molenda
Date: 2023-07-06T18:00:09-07:00
New Revision: c3192196aea279eea0a87247a02f224ebe067c44

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

LOG: Change the dyld notification function that lldb puts a breakpoint in

On Darwin systems, the dynamic linker dyld has an empty function
it calls when binaries are added/removed from the process.  lldb puts
a breakpoint on this dyld function to catch the notifications.  The
function arguments are used by lldb to tell what is happening.

The linker has a natural representation when the addresses of
binaries being added/removed are in the pointer size of the process.
There is then a second function where the addresses of the binaries
are in a uint64_t array, which the debugger has been using before -
dyld allocates memory for the array, copies the values in to it,
and calls it for lldb's benefit.

This changes to using the native notifier function, with pointer-sized
addresses.

This is the second time landing this change; this time correct the
size of the image_count argument, and add a fallback if the
notification function "lldb_image_notifier" can't be found.

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

Added: 


Modified: 
lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp

Removed: 




diff  --git 
a/lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp 
b/lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp
index 67e79fdcec8f56..b8723628d5118c 100644
--- a/lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp
+++ b/lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp
@@ -233,12 +233,13 @@ bool DynamicLoaderMacOS::NotifyBreakpointHit(void *baton,
  StoppointCallbackContext *context,
  lldb::user_id_t break_id,
  lldb::user_id_t break_loc_id) {
-  // Let the event know that the images have changed
-  // DYLD passes three arguments to the notification breakpoint.
-  // Arg1: enum dyld_notify_mode mode - 0 = adding, 1 = removing, 2 = remove
-  // all Arg2: unsigned long icount- Number of shared libraries
-  // added/removed Arg3: uint64_t mach_headers[] - Array of load addresses
-  // of binaries added/removed
+  //
+  // Our breakpoint on
+  //
+  // void lldb_image_notifier(enum dyld_image_mode mode, uint32_t infoCount,
+  // const dyld_image_info info[])
+  //
+  // has been hit.  We need to read the arguments.
 
   DynamicLoaderMacOS *dyld_instance = (DynamicLoaderMacOS *)baton;
 
@@ -268,9 +269,10 @@ bool DynamicLoaderMacOS::NotifyBreakpointHit(void *baton,
 ValueList argument_values;
 
 Value mode_value;// enum dyld_notify_mode { dyld_notify_adding=0,
- // dyld_notify_removing=1, dyld_notify_remove_all=2 };
-Value count_value;   // unsigned long count
-Value headers_value; // uint64_t machHeaders[] (aka void*)
+ // dyld_notify_removing=1, dyld_notify_remove_all=2,
+ // dyld_notify_dyld_moved=3 };
+Value count_value;   // uint32_t
+Value headers_value; // struct dyld_image_info machHeaders[]
 
 CompilerType clang_void_ptr_type =
 scratch_ts_sp->GetBasicType(eBasicTypeVoid).GetPointerType();
@@ -284,13 +286,8 @@ bool DynamicLoaderMacOS::NotifyBreakpointHit(void *baton,
 mode_value.SetValueType(Value::ValueType::Scalar);
 mode_value.SetCompilerType(clang_uint32_type);
 
-if (process->GetTarget().GetArchitecture().GetAddressByteSize() == 4) {
-  count_value.SetValueType(Value::ValueType::Scalar);
-  count_value.SetCompilerType(clang_uint32_type);
-} else {
-  count_value.SetValueType(Value::ValueType::Scalar);
-  count_value.SetCompilerType(clang_uint64_type);
-}
+count_value.SetValueType(Value::ValueType::Scalar);
+count_value.SetCompilerType(clang_uint32_type);
 
 headers_value.SetValueType(Value::ValueType::Scalar);
 headers_value.SetCompilerType(clang_void_ptr_type);
@@ -312,12 +309,30 @@ bool DynamicLoaderMacOS::NotifyBreakpointHit(void *baton,
   argument_values.GetValueAtIndex(2)->GetScalar().ULongLong(-1);
   if (header_array != static_cast(-1)) {
 std::vector image_load_addresses;
+// header_array points to an array of image_infos_count elements,
+// each is
+// struct dyld_image_info {
+//   const struct mach_header* imageLoadAddress;
+//   const char*   imageFilePath;
+//   uintptr_t imageFileModDate;
+// };
+//
+// and we only need the imageLoadA

[Lldb-commits] [PATCH] D139453: Switch to a different library-loaded notification function breakpoint in Darwin dyld

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda updated this revision to Diff 537942.
jasonmolenda added a comment.
Herald added a subscriber: wangpc.

After reverting to avoid CI bot failures on macos intel, this is an updated 
patch that (1) corrects the binary image count size, (2) allows for a fallback 
if it can't find the notification function lldb_image_notifier, by name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139453

Files:
  lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp

Index: lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp
===
--- lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp
+++ lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp
@@ -233,12 +233,13 @@
  StoppointCallbackContext *context,
  lldb::user_id_t break_id,
  lldb::user_id_t break_loc_id) {
-  // Let the event know that the images have changed
-  // DYLD passes three arguments to the notification breakpoint.
-  // Arg1: enum dyld_notify_mode mode - 0 = adding, 1 = removing, 2 = remove
-  // all Arg2: unsigned long icount- Number of shared libraries
-  // added/removed Arg3: uint64_t mach_headers[] - Array of load addresses
-  // of binaries added/removed
+  //
+  // Our breakpoint on
+  //
+  // void lldb_image_notifier(enum dyld_image_mode mode, uint32_t infoCount,
+  // const dyld_image_info info[])
+  //
+  // has been hit.  We need to read the arguments.
 
   DynamicLoaderMacOS *dyld_instance = (DynamicLoaderMacOS *)baton;
 
@@ -268,9 +269,10 @@
 ValueList argument_values;
 
 Value mode_value;// enum dyld_notify_mode { dyld_notify_adding=0,
- // dyld_notify_removing=1, dyld_notify_remove_all=2 };
-Value count_value;   // unsigned long count
-Value headers_value; // uint64_t machHeaders[] (aka void*)
+ // dyld_notify_removing=1, dyld_notify_remove_all=2,
+ // dyld_notify_dyld_moved=3 };
+Value count_value;   // uint32_t
+Value headers_value; // struct dyld_image_info machHeaders[]
 
 CompilerType clang_void_ptr_type =
 scratch_ts_sp->GetBasicType(eBasicTypeVoid).GetPointerType();
@@ -284,13 +286,8 @@
 mode_value.SetValueType(Value::ValueType::Scalar);
 mode_value.SetCompilerType(clang_uint32_type);
 
-if (process->GetTarget().GetArchitecture().GetAddressByteSize() == 4) {
-  count_value.SetValueType(Value::ValueType::Scalar);
-  count_value.SetCompilerType(clang_uint32_type);
-} else {
-  count_value.SetValueType(Value::ValueType::Scalar);
-  count_value.SetCompilerType(clang_uint64_type);
-}
+count_value.SetValueType(Value::ValueType::Scalar);
+count_value.SetCompilerType(clang_uint32_type);
 
 headers_value.SetValueType(Value::ValueType::Scalar);
 headers_value.SetCompilerType(clang_void_ptr_type);
@@ -312,12 +309,30 @@
   argument_values.GetValueAtIndex(2)->GetScalar().ULongLong(-1);
   if (header_array != static_cast(-1)) {
 std::vector image_load_addresses;
+// header_array points to an array of image_infos_count elements,
+// each is
+// struct dyld_image_info {
+//   const struct mach_header* imageLoadAddress;
+//   const char*   imageFilePath;
+//   uintptr_t imageFileModDate;
+// };
+//
+// and we only need the imageLoadAddress fields.
+
+const int addrsize =
+process->GetTarget().GetArchitecture().GetAddressByteSize();
 for (uint64_t i = 0; i < image_infos_count; i++) {
   Status error;
-  addr_t addr = process->ReadUnsignedIntegerFromMemory(
-  header_array + (8 * i), 8, LLDB_INVALID_ADDRESS, error);
-  if (addr != LLDB_INVALID_ADDRESS) {
+  addr_t dyld_image_info = header_array + (addrsize * 3 * i);
+  addr_t addr =
+  process->ReadPointerFromMemory(dyld_image_info, error);
+  if (error.Success()) {
 image_load_addresses.push_back(addr);
+  } else {
+Debugger::ReportWarning(
+"DynamicLoaderMacOS::NotifyBreakpointHit unable "
+"to read binary mach-o load address at 0x%" PRIx64,
+addr);
   }
 }
 if (dyld_mode == 0) {
@@ -362,10 +377,16 @@
   Status error;
   addr_t notification_addr =
   process->ReadPointerFromMemory(notification_location, error);
-  if (ABISP abi_sp = process->GetABI())
-notification_addr 

[Lldb-commits] [PATCH] D153489: [lldb] Print hint if object description is requested but not implemented

[Augusto Noronha via Phabricator via lldb-commits]

augusto2112 added inline comments.



Comment at: lldb/include/lldb/Target/Target.h:496
 
+  std::once_flag target_once_flag;
   // These two functions fill out the Broadcaster interface:

kastiglione wrote:
> did you mean to use this, or should it be deleted?
Leftover from a different implementation, I'll delete it.



Comment at: lldb/source/Commands/CommandObjectDWIMPrint.cpp:129-130
+  static llvm::SmallPtrSet targets_warned;
+  if (targets_warned.contains(target_ptr))
+return;
+

kastiglione wrote:
> question: would people want to be warned once per target, or once per type?
> 
> If I do like the warning, I might want to know every time I `po` a type that 
> doesn't support it.
In my opinion once per target should be enough, if we do it for every type that 
could potentially be too noisy, and hopefully after seeing the hint a few times 
users will start remembering to try `p` too.



Comment at: lldb/source/Commands/CommandObjectDWIMPrint.cpp:158-162
+StreamString temp_result_stream;
+valobj_sp->Dump(temp_result_stream, dump_options);
+llvm::StringRef output = temp_result_stream.GetString();
+maybe_add_hint(output);
+result.GetOutputStream() << output;

kastiglione wrote:
> what do you think of passing in the `result`'s stream into `maybe_add_hint`? 
> Perhaps I am overlooking something, but I wonder if it would simplify the 
> code to reuse the one stream, instead of separating and then combining two 
> streams.
I need the two streams to print it in the correct order (hint first, result 
later)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153489

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

[Lldb-commits] [PATCH] D154542: Further refinements on reporting interruption in lldb

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added a comment.

LGTM! Thanks Jim!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154542

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

[Lldb-commits] [PATCH] D154542: Further refinements on reporting interruption in lldb

[Jim Ingham via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG2b0c88654212: Refine the reporting mechanism for 
interruption. (authored by jingham).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154542

Files:
  lldb/include/lldb/Core/Debugger.h
  lldb/include/lldb/Target/TargetList.h
  lldb/source/API/SBFrame.cpp
  lldb/source/Commands/CommandCompletions.cpp
  lldb/source/Commands/CommandObjectFrame.cpp
  lldb/source/Commands/CommandObjectTarget.cpp
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Core/Debugger.cpp
  lldb/source/Core/Module.cpp
  lldb/source/Interpreter/CommandInterpreter.cpp
  lldb/source/Target/StackFrameList.cpp
  lldb/source/Target/Target.cpp
  lldb/source/Target/TargetList.cpp

Index: lldb/source/Target/TargetList.cpp
===
--- lldb/source/Target/TargetList.cpp
+++ lldb/source/Target/TargetList.cpp
@@ -325,6 +325,7 @@
 return error;
   }
   target_sp.reset(new Target(debugger, arch, platform_sp, is_dummy_target));
+  debugger.GetTargetList().RegisterInProcessTarget(target_sp);
   target_sp->SetExecutableModule(exe_module_sp, load_dependent_files);
   if (user_exe_path_is_bundle)
 exe_module_sp->GetFileSpec().GetPath(resolved_bundle_exe_path,
@@ -336,6 +337,7 @@
 // No file was specified, just create an empty target with any arch if a
 // valid arch was specified
 target_sp.reset(new Target(debugger, arch, platform_sp, is_dummy_target));
+debugger.GetTargetList().RegisterInProcessTarget(target_sp);
   }
 
   if (!target_sp)
@@ -513,6 +515,7 @@
 void TargetList::AddTargetInternal(TargetSP target_sp, bool do_select) {
   lldbassert(!llvm::is_contained(m_target_list, target_sp) &&
  "target already exists it the list");
+  UnregisterInProcessTarget(target_sp);
   m_target_list.push_back(std::move(target_sp));
   if (do_select)
 SetSelectedTargetInternal(m_target_list.size() - 1);
@@ -540,3 +543,35 @@
 m_selected_target_idx = 0;
   return GetTargetAtIndex(m_selected_target_idx);
 }
+
+bool TargetList::AnyTargetContainsModule(Module &module) {
+  std::lock_guard guard(m_target_list_mutex);
+  for (const auto &target_sp : m_target_list) {
+if (target_sp->GetImages().FindModule(&module))
+  return true;
+  }
+  for (const auto &target_sp: m_in_process_target_list) {
+if (target_sp->GetImages().FindModule(&module))
+  return true;
+  }
+  return false;
+}
+
+  void TargetList::RegisterInProcessTarget(TargetSP target_sp) {
+std::lock_guard guard(m_target_list_mutex);
+std::unordered_set::iterator iter;
+bool was_added;
+std::tie(iter, was_added) = m_in_process_target_list.insert(target_sp);
+assert(was_added && "Target pointer was left in the in-process map");
+  }
+  
+  void TargetList::UnregisterInProcessTarget(TargetSP target_sp) {
+std::lock_guard guard(m_target_list_mutex);
+bool was_present = m_in_process_target_list.erase(target_sp);
+assert(was_present && "Target pointer being removed was not registered");
+  }
+  
+  bool TargetList::IsTargetInProcess(TargetSP target_sp) {
+std::lock_guard guard(m_target_list_mutex);
+return m_in_process_target_list.count(target_sp) == 1; 
+  }
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -2236,7 +2236,6 @@
 // each library in parallel.
 if (GetPreloadSymbols())
   module_sp->PreloadSymbols();
-
 llvm::SmallVector replaced_modules;
 for (ModuleSP &old_module_sp : old_modules) {
   if (m_images.GetIndexForModule(old_module_sp.get()) !=
@@ -4205,6 +4204,10 @@
 }
 
 bool TargetProperties::GetPreloadSymbols() const {
+  if (INTERRUPT_REQUESTED(m_target->GetDebugger(), 
+  "Interrupted checking preload symbols")) {
+return false;
+  }
   const uint32_t idx = ePropertyPreloadSymbols;
   return GetPropertyAtIndexAs(
   idx, g_target_properties[idx].default_uint_value != 0);
Index: lldb/source/Target/StackFrameList.cpp
===
--- lldb/source/Target/StackFrameList.cpp
+++ lldb/source/Target/StackFrameList.cpp
@@ -509,11 +509,11 @@
 } else {
   // Check for interruption when building the frames.
   // Do the check in idx > 0 so that we'll always create a 0th frame.
-  if (allow_interrupt && dbg.InterruptRequested()) {
-Log *log = GetLog(LLDBLog::Host);
-LLDB_LOG(log, "Interrupted %s", __FUNCTION__);
-was_interrupted = true;
-break;
+  if (allow_interrupt 
+  && INTERRUPT_REQUESTED(dbg, "Interrupted having fetched {0} frames",
+ m_frames.size())) {
+  was_interrupted = true;
+  break;
   }

[Lldb-commits] [lldb] 2b0c886 - Refine the reporting mechanism for interruption.

[Jim Ingham via lldb-commits]

Author: Jim Ingham
Date: 2023-07-06T16:19:19-07:00
New Revision: 2b0c8865421287a30ddda0f459f17f76bfeb1358

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

LOG: Refine the reporting mechanism for interruption.

Also, make it possible for new Targets which haven't been added to
the TargetList yet to check for interruption, and add a few more
places in building modules where we can check for interruption.

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

Added: 


Modified: 
lldb/include/lldb/Core/Debugger.h
lldb/include/lldb/Target/TargetList.h
lldb/source/API/SBFrame.cpp
lldb/source/Commands/CommandCompletions.cpp
lldb/source/Commands/CommandObjectFrame.cpp
lldb/source/Commands/CommandObjectTarget.cpp
lldb/source/Commands/CommandObjectThread.cpp
lldb/source/Core/Debugger.cpp
lldb/source/Core/Module.cpp
lldb/source/Interpreter/CommandInterpreter.cpp
lldb/source/Target/StackFrameList.cpp
lldb/source/Target/Target.cpp
lldb/source/Target/TargetList.cpp

Removed: 




diff  --git a/lldb/include/lldb/Core/Debugger.h 
b/lldb/include/lldb/Core/Debugger.h
index 3996c4c58e2baf..239429b75c4260 100644
--- a/lldb/include/lldb/Core/Debugger.h
+++ b/lldb/include/lldb/Core/Debugger.h
@@ -43,6 +43,7 @@
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/DynamicLibrary.h"
+#include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Threading.h"
 
 #include 
@@ -85,6 +86,8 @@ class Debugger : public 
std::enable_shared_from_this,
 eBroadcastSymbolChange = (1 << 3),
   };
 
+  using DebuggerList = std::vector;
+
   static ConstString GetStaticBroadcasterClass();
 
   /// Get the public broadcaster for this debugger.
@@ -411,12 +414,75 @@ class Debugger : public 
std::enable_shared_from_this,
   /// If you are on the RunCommandInterpreter thread, it will check the
   /// command interpreter state, and if it is on another thread it will
   /// check the debugger Interrupt Request state.
+  /// \param[in] cur_func
+  /// For reporting if the interruption was requested.  Don't provide this by
+  /// hand, use INTERRUPT_REQUESTED so this gets done consistently.
   ///
+  /// \param[in] formatv
+  /// A formatv string for the interrupt message.  If the elements of the 
+  /// message are expensive to compute, you can use the no-argument form of
+  /// InterruptRequested, then make up the report using REPORT_INTERRUPTION. 
+  /// 
   /// \return
   ///  A boolean value, if \b true an interruptible operation should interrupt
   ///  itself.
+  template 
+  bool InterruptRequested(const char *cur_func, 
+  const char *formatv, Args &&... args) {
+bool ret_val = InterruptRequested();
+if (ret_val) {
+  if (!formatv)
+formatv = "Unknown message";
+  if (!cur_func)
+cur_func = "";
+  ReportInterruption(InterruptionReport(cur_func, 
+llvm::formatv(formatv, 
+std::forward(args)...)));
+}
+return ret_val;
+  }
+  
+  
+  /// This handy define will keep you from having to generate a report for the
+  /// interruption by hand.  Use this except in the case where the arguments to
+  /// the message description are expensive to compute.
+#define INTERRUPT_REQUESTED(debugger, ...) \
+(debugger).InterruptRequested(__func__, __VA_ARGS__)
+
+  // This form just queries for whether to interrupt, and does no reporting:
   bool InterruptRequested();
+  
+  // FIXME: Do we want to capture a backtrace at the interruption point?
+  class InterruptionReport {
+  public:
+InterruptionReport(std::string function_name, std::string description) :
+m_function_name(std::move(function_name)), 
+m_description(std::move(description)),
+m_interrupt_time(std::chrono::system_clock::now()),
+m_thread_id(llvm::get_threadid()) {}
+
+InterruptionReport(std::string function_name, 
+const llvm::formatv_object_base &payload);
+
+  template 
+  InterruptionReport(std::string function_name,
+  const char *format, Args &&... args) :
+InterruptionReport(function_name, llvm::formatv(format, 
std::forward(args)...)) {}
+
+std::string m_function_name;
+std::string m_description;
+const std::chrono::time_point m_interrupt_time;
+const uint64_t m_thread_id;
+  };
+  void ReportInterruption(const InterruptionReport &report);
+#define REPORT_INTERRUPTION(debugger, ...) \
+(debugger).ReportInterruption(Debugger::InterruptionReport(__func__, \
+__VA_ARGS__))
 
+  static DebuggerList DebuggersRequestingInterruption();
+
+public:
+  
   // This is for use in

[Lldb-commits] [PATCH] D154542: Further refinements on reporting interruption in lldb

[Alex Langford via Phabricator via lldb-commits]

bulbazord accepted this revision.
bulbazord 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/D154542/new/

https://reviews.llvm.org/D154542

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

[Lldb-commits] [PATCH] D154542: Further refinements on reporting interruption in lldb

[Jim Ingham via Phabricator via lldb-commits]

jingham updated this revision to Diff 537908.
jingham added a comment.

Add some std;:move's, Jonas agrees all the cool kids are doing that these days.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154542

Files:
  lldb/include/lldb/Core/Debugger.h
  lldb/include/lldb/Target/TargetList.h
  lldb/source/API/SBFrame.cpp
  lldb/source/Commands/CommandCompletions.cpp
  lldb/source/Commands/CommandObjectFrame.cpp
  lldb/source/Commands/CommandObjectTarget.cpp
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Core/Debugger.cpp
  lldb/source/Core/Module.cpp
  lldb/source/Interpreter/CommandInterpreter.cpp
  lldb/source/Target/StackFrameList.cpp
  lldb/source/Target/Target.cpp
  lldb/source/Target/TargetList.cpp

Index: lldb/source/Target/TargetList.cpp
===
--- lldb/source/Target/TargetList.cpp
+++ lldb/source/Target/TargetList.cpp
@@ -325,6 +325,7 @@
 return error;
   }
   target_sp.reset(new Target(debugger, arch, platform_sp, is_dummy_target));
+  debugger.GetTargetList().RegisterInProcessTarget(target_sp);
   target_sp->SetExecutableModule(exe_module_sp, load_dependent_files);
   if (user_exe_path_is_bundle)
 exe_module_sp->GetFileSpec().GetPath(resolved_bundle_exe_path,
@@ -336,6 +337,7 @@
 // No file was specified, just create an empty target with any arch if a
 // valid arch was specified
 target_sp.reset(new Target(debugger, arch, platform_sp, is_dummy_target));
+debugger.GetTargetList().RegisterInProcessTarget(target_sp);
   }
 
   if (!target_sp)
@@ -513,6 +515,7 @@
 void TargetList::AddTargetInternal(TargetSP target_sp, bool do_select) {
   lldbassert(!llvm::is_contained(m_target_list, target_sp) &&
  "target already exists it the list");
+  UnregisterInProcessTarget(target_sp);
   m_target_list.push_back(std::move(target_sp));
   if (do_select)
 SetSelectedTargetInternal(m_target_list.size() - 1);
@@ -540,3 +543,35 @@
 m_selected_target_idx = 0;
   return GetTargetAtIndex(m_selected_target_idx);
 }
+
+bool TargetList::AnyTargetContainsModule(Module &module) {
+  std::lock_guard guard(m_target_list_mutex);
+  for (const auto &target_sp : m_target_list) {
+if (target_sp->GetImages().FindModule(&module))
+  return true;
+  }
+  for (const auto &target_sp: m_in_process_target_list) {
+if (target_sp->GetImages().FindModule(&module))
+  return true;
+  }
+  return false;
+}
+
+  void TargetList::RegisterInProcessTarget(TargetSP target_sp) {
+std::lock_guard guard(m_target_list_mutex);
+std::unordered_set::iterator iter;
+bool was_added;
+std::tie(iter, was_added) = m_in_process_target_list.insert(target_sp);
+assert(was_added && "Target pointer was left in the in-process map");
+  }
+  
+  void TargetList::UnregisterInProcessTarget(TargetSP target_sp) {
+std::lock_guard guard(m_target_list_mutex);
+bool was_present = m_in_process_target_list.erase(target_sp);
+assert(was_present && "Target pointer being removed was not registered");
+  }
+  
+  bool TargetList::IsTargetInProcess(TargetSP target_sp) {
+std::lock_guard guard(m_target_list_mutex);
+return m_in_process_target_list.count(target_sp) == 1; 
+  }
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -2236,7 +2236,6 @@
 // each library in parallel.
 if (GetPreloadSymbols())
   module_sp->PreloadSymbols();
-
 llvm::SmallVector replaced_modules;
 for (ModuleSP &old_module_sp : old_modules) {
   if (m_images.GetIndexForModule(old_module_sp.get()) !=
@@ -4205,6 +4204,10 @@
 }
 
 bool TargetProperties::GetPreloadSymbols() const {
+  if (INTERRUPT_REQUESTED(m_target->GetDebugger(), 
+  "Interrupted checking preload symbols")) {
+return false;
+  }
   const uint32_t idx = ePropertyPreloadSymbols;
   return GetPropertyAtIndexAs(
   idx, g_target_properties[idx].default_uint_value != 0);
Index: lldb/source/Target/StackFrameList.cpp
===
--- lldb/source/Target/StackFrameList.cpp
+++ lldb/source/Target/StackFrameList.cpp
@@ -509,11 +509,11 @@
 } else {
   // Check for interruption when building the frames.
   // Do the check in idx > 0 so that we'll always create a 0th frame.
-  if (allow_interrupt && dbg.InterruptRequested()) {
-Log *log = GetLog(LLDBLog::Host);
-LLDB_LOG(log, "Interrupted %s", __FUNCTION__);
-was_interrupted = true;
-break;
+  if (allow_interrupt 
+  && INTERRUPT_REQUESTED(dbg, "Interrupted having fetched {0} frames",
+ m_frames.size())) {
+  was_interrupted = true;
+  break;
   }
 
   const bool success

[Lldb-commits] [PATCH] D153489: [lldb] Print hint if object description is requested but not implemented

[Dave Lee via Phabricator via lldb-commits]

kastiglione added inline comments.



Comment at: lldb/include/lldb/Target/Target.h:496
 
+  std::once_flag target_once_flag;
   // These two functions fill out the Broadcaster interface:

did you mean to use this, or should it be deleted?



Comment at: lldb/source/Commands/CommandObjectDWIMPrint.cpp:129-130
+  static llvm::SmallPtrSet targets_warned;
+  if (targets_warned.contains(target_ptr))
+return;
+

question: would people want to be warned once per target, or once per type?

If I do like the warning, I might want to know every time I `po` a type that 
doesn't support it.



Comment at: lldb/source/Commands/CommandObjectDWIMPrint.cpp:158-162
+StreamString temp_result_stream;
+valobj_sp->Dump(temp_result_stream, dump_options);
+llvm::StringRef output = temp_result_stream.GetString();
+maybe_add_hint(output);
+result.GetOutputStream() << output;

what do you think of passing in the `result`'s stream into `maybe_add_hint`? 
Perhaps I am overlooking something, but I wonder if it would simplify the code 
to reuse the one stream, instead of separating and then combining two streams.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153489

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

[Lldb-commits] [PATCH] D154542: Further refinements on reporting interruption in lldb

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.



Comment at: lldb/include/lldb/Core/Debugger.h:455-456
+InterruptionReport(std::string function_name, std::string description) :
+m_function_name(function_name), 
+m_description(description),
+m_interrupt_time(std::chrono::system_clock::now()),

jingham wrote:
> bulbazord wrote:
> > jingham wrote:
> > > bulbazord wrote:
> > > > To avoid some unnecessary copies
> > > > 
> > > > Could also do what Ismail is suggesting.
> > > This is a local that is copied to an ivar and never used again.  Do I 
> > > really have to put move in there explicitly for the optimizer to know it 
> > > can reuse the value?
> > Yes. I created a simple example on Godbolt: https://godbolt.org/z/G4ej76Enn
> > 
> > Observe that the constructor in the example invokes the `std::string` copy 
> > constructor. Add a `std::move` and it then invokes the move constructor.
> Thanks for the example, that was interesting.  
> 
> Note however that in your example, you the flags you specified were 
> `-std=c++17` so that was an unoptimized build.  That's expected, unoptimized 
> builds are supposed to be as literal as they can be.  If you instead use the 
> same example with `-std=c++17 -O3` you won't see any copy constructors.  I 
> don't 100% mind putting this std::move in, but it seems noisy to decorate up 
> your code with hints that the optimizer can figure out on its own.
You're right that I did not consider higher optimization levels there. That 
being said, I believe this is a matter of language semantics and not 
optimization level. I've created a slightly more complex example to illustrate: 
https://godbolt.org/z/naMTcd7Ev

In this other example, I have passed `-O3`. You'll notice that in the emitted 
constructor code there are 2 calls to `operator new`. If you add `std::move` 
for both of the parameters of the constructor, both of these allocations go 
away and the generated code becomes ~60% smaller. Even with optimization 
involved, the compiler can only optimize so much without breaking semantics.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154542

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

[Lldb-commits] [PATCH] D154542: Further refinements on reporting interruption in lldb

[Jim Ingham via Phabricator via lldb-commits]

jingham added inline comments.



Comment at: lldb/include/lldb/Core/Debugger.h:455-456
+InterruptionReport(std::string function_name, std::string description) :
+m_function_name(function_name), 
+m_description(description),
+m_interrupt_time(std::chrono::system_clock::now()),

bulbazord wrote:
> jingham wrote:
> > bulbazord wrote:
> > > To avoid some unnecessary copies
> > > 
> > > Could also do what Ismail is suggesting.
> > This is a local that is copied to an ivar and never used again.  Do I 
> > really have to put move in there explicitly for the optimizer to know it 
> > can reuse the value?
> Yes. I created a simple example on Godbolt: https://godbolt.org/z/G4ej76Enn
> 
> Observe that the constructor in the example invokes the `std::string` copy 
> constructor. Add a `std::move` and it then invokes the move constructor.
Thanks for the example, that was interesting.  

Note however that in your example, you the flags you specified were 
`-std=c++17` so that was an unoptimized build.  That's expected, unoptimized 
builds are supposed to be as literal as they can be.  If you instead use the 
same example with `-std=c++17 -O3` you won't see any copy constructors.  I 
don't 100% mind putting this std::move in, but it seems noisy to decorate up 
your code with hints that the optimizer can figure out on its own.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154542

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

[Lldb-commits] [PATCH] D154649: [lldb] Fix dead lock issue when loading modules in Scripted Process

[Med Ismail Bennani via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG1f5f4169c427: [lldb] Fix dead lock issue when loading 
modules in Scripted Process (authored by mib).

Changed prior to commit:
  https://reviews.llvm.org/D154649?vs=537846&id=537887#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154649

Files:
  lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
  lldb/source/Plugins/Process/scripted/ScriptedProcess.h


Index: lldb/source/Plugins/Process/scripted/ScriptedProcess.h
===
--- lldb/source/Plugins/Process/scripted/ScriptedProcess.h
+++ lldb/source/Plugins/Process/scripted/ScriptedProcess.h
@@ -12,6 +12,7 @@
 #include "lldb/Target/Process.h"
 #include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/ScriptedMetadata.h"
+#include "lldb/Utility/State.h"
 #include "lldb/Utility/Status.h"
 
 #include "ScriptedThread.h"
@@ -93,6 +94,11 @@
   void *GetImplementation() override;
 
   void ForceScriptedState(lldb::StateType state) override {
+// If we're about to stop, we should fetch the loaded dynamic libraries
+// dictionary before emitting the private stop event to avoid having the
+// module loading happen while the process state is changing.
+if (StateIsStoppedState(state, true))
+  GetLoadedDynamicLibrariesInfos();
 SetPrivateState(state);
   }
 
Index: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
===
--- lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
+++ lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
@@ -166,7 +166,6 @@
 void ScriptedProcess::DidResume() {
   // Update the PID again, in case the user provided a placeholder pid at 
launch
   m_pid = GetInterface().GetProcessID();
-  GetLoadedDynamicLibrariesInfos();
 }
 
 Status ScriptedProcess::DoResume() {


Index: lldb/source/Plugins/Process/scripted/ScriptedProcess.h
===
--- lldb/source/Plugins/Process/scripted/ScriptedProcess.h
+++ lldb/source/Plugins/Process/scripted/ScriptedProcess.h
@@ -12,6 +12,7 @@
 #include "lldb/Target/Process.h"
 #include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/ScriptedMetadata.h"
+#include "lldb/Utility/State.h"
 #include "lldb/Utility/Status.h"
 
 #include "ScriptedThread.h"
@@ -93,6 +94,11 @@
   void *GetImplementation() override;
 
   void ForceScriptedState(lldb::StateType state) override {
+// If we're about to stop, we should fetch the loaded dynamic libraries
+// dictionary before emitting the private stop event to avoid having the
+// module loading happen while the process state is changing.
+if (StateIsStoppedState(state, true))
+  GetLoadedDynamicLibrariesInfos();
 SetPrivateState(state);
   }
 
Index: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
===
--- lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
+++ lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
@@ -166,7 +166,6 @@
 void ScriptedProcess::DidResume() {
   // Update the PID again, in case the user provided a placeholder pid at launch
   m_pid = GetInterface().GetProcessID();
-  GetLoadedDynamicLibrariesInfos();
 }
 
 Status ScriptedProcess::DoResume() {
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 1f5f416 - [lldb] Fix dead lock issue when loading modules in Scripted Process

[Med Ismail Bennani via lldb-commits]

Author: Med Ismail Bennani
Date: 2023-07-06T14:33:52-07:00
New Revision: 1f5f4169c427c51c6919e0013c89a191dba564e8

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

LOG: [lldb] Fix dead lock issue when loading modules in Scripted Process

This patch attempts to fix a dead lock when loading modules in a Scripted
Process.

This issue was triggered by loading the modules after the process did resume,
but before the process actually stop, causing the language runtime mutex to
be locked by a separate thread, responsible to unwind the stack (using
the runtime unwind plan), while the module loading thread was trying to
notify the runtimes of the newly loaded module.

To address that, this patch moves the module loading logic to be done before
sending the stop event, to prevent the dead lock situation described above.

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

Signed-off-by: Med Ismail Bennani 

Added: 


Modified: 
lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
lldb/source/Plugins/Process/scripted/ScriptedProcess.h

Removed: 




diff  --git a/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp 
b/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
index 5be1eff76ba9da..e99a2a08bd50d8 100644
--- a/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
+++ b/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
@@ -166,7 +166,6 @@ void ScriptedProcess::DidLaunch() { m_pid = 
GetInterface().GetProcessID(); }
 void ScriptedProcess::DidResume() {
   // Update the PID again, in case the user provided a placeholder pid at 
launch
   m_pid = GetInterface().GetProcessID();
-  GetLoadedDynamicLibrariesInfos();
 }
 
 Status ScriptedProcess::DoResume() {

diff  --git a/lldb/source/Plugins/Process/scripted/ScriptedProcess.h 
b/lldb/source/Plugins/Process/scripted/ScriptedProcess.h
index 24b5a317b54fa8..0335364b4010b2 100644
--- a/lldb/source/Plugins/Process/scripted/ScriptedProcess.h
+++ b/lldb/source/Plugins/Process/scripted/ScriptedProcess.h
@@ -12,6 +12,7 @@
 #include "lldb/Target/Process.h"
 #include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/ScriptedMetadata.h"
+#include "lldb/Utility/State.h"
 #include "lldb/Utility/Status.h"
 
 #include "ScriptedThread.h"
@@ -93,6 +94,11 @@ class ScriptedProcess : public Process {
   void *GetImplementation() override;
 
   void ForceScriptedState(lldb::StateType state) override {
+// If we're about to stop, we should fetch the loaded dynamic libraries
+// dictionary before emitting the private stop event to avoid having the
+// module loading happen while the process state is changing.
+if (StateIsStoppedState(state, true))
+  GetLoadedDynamicLibrariesInfos();
 SetPrivateState(state);
   }
 



___
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]

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] [lldb] 032de3e - [lldb][DebugNamesDWARF] Also use mangled name when matching regex

[Felipe de Azevedo Piovezan via lldb-commits]

Author: Felipe de Azevedo Piovezan
Date: 2023-07-06T17:15:06-04:00
New Revision: 032de3ee0d3575557f25fb2d78b9423f3f50

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

LOG: [lldb][DebugNamesDWARF] Also use mangled name when matching regex

When LLDB queries the debug names index with a regex, we should use the
`Mangled` class wrapper, which attempts to match regex first against the mangled
name and then against the demangled name. It is important to do so, otherwise
queries like `frame var --regex A::` would never work. This is what is done for
the Apple index as well.

This fixes test/API/lang/cpp/class_static/main.cpp when compiled with DWARF 5.

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

Added: 


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

Removed: 




diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
index 879d6ce4686a72..1711a229443672 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
@@ -104,7 +104,8 @@ void DebugNamesDWARFIndex::GetGlobalVariables(
 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] 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] [lldb] 2c3bb6a - [lldb][NFC] Remove unused parameter from DebugNames::ProcessEntry

[Felipe de Azevedo Piovezan via lldb-commits]

Author: Felipe de Azevedo Piovezan
Date: 2023-07-06T17:14:14-04:00
New Revision: 2c3bb6a5902791dd7a32bf1639255ea0c0366d43

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

LOG: [lldb][NFC] Remove unused parameter from DebugNames::ProcessEntry

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

Added: 


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

Removed: 




diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
index 8d931e369098f4..879d6ce4686a72 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
@@ -62,7 +62,7 @@ DebugNamesDWARFIndex::ToDIERef(const DebugNames::Entry 
&entry) {
 
 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 @@ void DebugNamesDWARFIndex::GetGlobalVariables(
 if (entry.tag() != DW_TAG_variable)
   continue;
 
-if (!ProcessEntry(entry, callback, basename.GetStringRef()))
+if (!ProcessEntry(entry, callback))
   return;
   }
 
@@ -113,8 +113,7 @@ void DebugNamesDWARFIndex::GetGlobalVariables(
 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 @@ void DebugNamesDWARFIndex::GetGlobalVariables(
   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 @@ void DebugNamesDWARFIndex::GetTypes(
   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 @@ void DebugNamesDWARFIndex::GetTypes(
   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 @@ void DebugNamesDWARFIndex::GetNamespaces(
   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 @@ void DebugNamesDWARFIndex::GetFunctions(
 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());

diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.h 
b/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.h
index 21a3d8dcc0e3eb..abbd700f1603fe 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.h
@@ -80,8 +80,7 @@ class DebugNamesDWARFIndex : public DWARFIndex {
 
   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,



___
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] D154029: [lldb-vscode] Adding support for column break points.

[David Goldman via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGda59370b0977: [lldb-vscode] Adding support for column break 
points. (authored by ashgti, committed by dgoldman).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154029

Files:
  lldb/tools/lldb-vscode/JSONUtils.cpp
  lldb/tools/lldb-vscode/JSONUtils.h
  lldb/tools/lldb-vscode/SourceBreakpoint.cpp


Index: lldb/tools/lldb-vscode/SourceBreakpoint.cpp
===
--- lldb/tools/lldb-vscode/SourceBreakpoint.cpp
+++ lldb/tools/lldb-vscode/SourceBreakpoint.cpp
@@ -16,7 +16,9 @@
   column(GetUnsigned(obj, "column", 0)) {}
 
 void SourceBreakpoint::SetBreakpoint(const llvm::StringRef source_path) {
-  bp = g_vsc.target.BreakpointCreateByLocation(source_path.str().c_str(), 
line);
+  lldb::SBFileSpecList module_list;
+  bp = g_vsc.target.BreakpointCreateByLocation(source_path.str().c_str(), line,
+   column, 0, module_list);
   // See comments in BreakpointBase::GetBreakpointLabel() for details of why
   // we add a label to our breakpoints.
   bp.AddName(GetBreakpointLabel());
Index: lldb/tools/lldb-vscode/JSONUtils.h
===
--- lldb/tools/lldb-vscode/JSONUtils.h
+++ lldb/tools/lldb-vscode/JSONUtils.h
@@ -232,13 +232,21 @@
 /// provided by the setBreakpoints request are returned to the IDE as a
 /// fallback.
 ///
+/// \param[in] request_column
+/// An optional column to use when creating the resulting "Breakpoint" 
object.
+/// It is used if the breakpoint has no valid locations.
+/// It is useful to ensure the same column
+/// provided by the setBreakpoints request are returned to the IDE as a
+/// fallback.
+///
 /// \return
 /// A "Breakpoint" JSON object with that follows the formal JSON
 /// definition outlined by Microsoft.
 llvm::json::Value
 CreateBreakpoint(lldb::SBBreakpoint &bp,
  std::optional request_path = std::nullopt,
- std::optional request_line = std::nullopt);
+ std::optional request_line = std::nullopt,
+ std::optional request_column = std::nullopt);
 
 /// Converts a LLDB module to a VS Code DAP module for use in "modules" events.
 ///
Index: lldb/tools/lldb-vscode/JSONUtils.cpp
===
--- lldb/tools/lldb-vscode/JSONUtils.cpp
+++ lldb/tools/lldb-vscode/JSONUtils.cpp
@@ -308,7 +308,8 @@
 // }
 llvm::json::Value CreateBreakpoint(lldb::SBBreakpoint &bp,
std::optional request_path,
-   std::optional request_line) {
+   std::optional request_line,
+   std::optional request_column) {
   // Each breakpoint location is treated as a separate breakpoint for VS code.
   // They don't have the notion of a single breakpoint with multiple locations.
   llvm::json::Object object;
@@ -345,11 +346,16 @@
 const auto line = line_entry.GetLine();
 if (line != UINT32_MAX)
   object.try_emplace("line", line);
+const auto column = line_entry.GetColumn();
+if (column != 0)
+  object.try_emplace("column", column);
 object.try_emplace("source", CreateSource(line_entry));
   }
   // We try to add request_line as a fallback
   if (request_line)
 object.try_emplace("line", *request_line);
+  if (request_column)
+object.try_emplace("column", *request_column);
   return llvm::json::Value(std::move(object));
 }
 


Index: lldb/tools/lldb-vscode/SourceBreakpoint.cpp
===
--- lldb/tools/lldb-vscode/SourceBreakpoint.cpp
+++ lldb/tools/lldb-vscode/SourceBreakpoint.cpp
@@ -16,7 +16,9 @@
   column(GetUnsigned(obj, "column", 0)) {}
 
 void SourceBreakpoint::SetBreakpoint(const llvm::StringRef source_path) {
-  bp = g_vsc.target.BreakpointCreateByLocation(source_path.str().c_str(), line);
+  lldb::SBFileSpecList module_list;
+  bp = g_vsc.target.BreakpointCreateByLocation(source_path.str().c_str(), line,
+   column, 0, module_list);
   // See comments in BreakpointBase::GetBreakpointLabel() for details of why
   // we add a label to our breakpoints.
   bp.AddName(GetBreakpointLabel());
Index: lldb/tools/lldb-vscode/JSONUtils.h
===
--- lldb/tools/lldb-vscode/JSONUtils.h
+++ lldb/tools/lldb-vscode/JSONUtils.h
@@ -232,13 +232,21 @@
 /// provided by the setBreakpoints request are returned to the IDE as a
 /// fallback.
 ///
+/// \param[in] request_column
+/// An optional column to use when creating the resulting "Breakpoint" object.
+/// It is used if the breakpoint has no valid locations

[Lldb-commits] [lldb] da59370 - [lldb-vscode] Adding support for column break points.

[David Goldman via lldb-commits]

Author: John Harrison
Date: 2023-07-06T16:33:22-04:00
New Revision: da59370b0977083fa081f4f113179110c86916e5

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

LOG: [lldb-vscode] Adding support for column break points.

Reviewed By: wallace

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

Added: 


Modified: 
lldb/tools/lldb-vscode/JSONUtils.cpp
lldb/tools/lldb-vscode/JSONUtils.h
lldb/tools/lldb-vscode/SourceBreakpoint.cpp

Removed: 




diff  --git a/lldb/tools/lldb-vscode/JSONUtils.cpp 
b/lldb/tools/lldb-vscode/JSONUtils.cpp
index 2c54f1a4e271bd..8632ba929f77d9 100644
--- a/lldb/tools/lldb-vscode/JSONUtils.cpp
+++ b/lldb/tools/lldb-vscode/JSONUtils.cpp
@@ -308,7 +308,8 @@ llvm::json::Value CreateScope(const llvm::StringRef name,
 // }
 llvm::json::Value CreateBreakpoint(lldb::SBBreakpoint &bp,
std::optional request_path,
-   std::optional request_line) {
+   std::optional request_line,
+   std::optional request_column) {
   // Each breakpoint location is treated as a separate breakpoint for VS code.
   // They don't have the notion of a single breakpoint with multiple locations.
   llvm::json::Object object;
@@ -345,11 +346,16 @@ llvm::json::Value CreateBreakpoint(lldb::SBBreakpoint &bp,
 const auto line = line_entry.GetLine();
 if (line != UINT32_MAX)
   object.try_emplace("line", line);
+const auto column = line_entry.GetColumn();
+if (column != 0)
+  object.try_emplace("column", column);
 object.try_emplace("source", CreateSource(line_entry));
   }
   // We try to add request_line as a fallback
   if (request_line)
 object.try_emplace("line", *request_line);
+  if (request_column)
+object.try_emplace("column", *request_column);
   return llvm::json::Value(std::move(object));
 }
 

diff  --git a/lldb/tools/lldb-vscode/JSONUtils.h 
b/lldb/tools/lldb-vscode/JSONUtils.h
index 73cfb425ee2900..45f69965914017 100644
--- a/lldb/tools/lldb-vscode/JSONUtils.h
+++ b/lldb/tools/lldb-vscode/JSONUtils.h
@@ -232,13 +232,21 @@ void AppendBreakpoint(
 /// provided by the setBreakpoints request are returned to the IDE as a
 /// fallback.
 ///
+/// \param[in] request_column
+/// An optional column to use when creating the resulting "Breakpoint" 
object.
+/// It is used if the breakpoint has no valid locations.
+/// It is useful to ensure the same column
+/// provided by the setBreakpoints request are returned to the IDE as a
+/// fallback.
+///
 /// \return
 /// A "Breakpoint" JSON object with that follows the formal JSON
 /// definition outlined by Microsoft.
 llvm::json::Value
 CreateBreakpoint(lldb::SBBreakpoint &bp,
  std::optional request_path = std::nullopt,
- std::optional request_line = std::nullopt);
+ std::optional request_line = std::nullopt,
+ std::optional request_column = std::nullopt);
 
 /// Converts a LLDB module to a VS Code DAP module for use in "modules" events.
 ///

diff  --git a/lldb/tools/lldb-vscode/SourceBreakpoint.cpp 
b/lldb/tools/lldb-vscode/SourceBreakpoint.cpp
index 742d6142d32341..7c57bf7d7c4f53 100644
--- a/lldb/tools/lldb-vscode/SourceBreakpoint.cpp
+++ b/lldb/tools/lldb-vscode/SourceBreakpoint.cpp
@@ -16,7 +16,9 @@ SourceBreakpoint::SourceBreakpoint(const llvm::json::Object 
&obj)
   column(GetUnsigned(obj, "column", 0)) {}
 
 void SourceBreakpoint::SetBreakpoint(const llvm::StringRef source_path) {
-  bp = g_vsc.target.BreakpointCreateByLocation(source_path.str().c_str(), 
line);
+  lldb::SBFileSpecList module_list;
+  bp = g_vsc.target.BreakpointCreateByLocation(source_path.str().c_str(), line,
+   column, 0, module_list);
   // See comments in BreakpointBase::GetBreakpointLabel() for details of why
   // we add a label to our breakpoints.
   bp.AddName(GetBreakpointLabel());



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

[Lldb-commits] [PATCH] D154649: [lldb] Fix dead lock issue when loading modules in Scripted Process

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.

LGTM. Might be worth adding a little comment there to make sure we remember why 
the check is there in the future.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154649

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

[Lldb-commits] [PATCH] D154649: [lldb] Fix dead lock issue when loading modules in Scripted Process

[Jim Ingham via Phabricator via lldb-commits]

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

Yes, that makes sense.  lldb always updates its shared library state in 
reaction to a stop, so it's much safer to have the scripted process emulate 
this behavior.  Plus, refreshing the state for a given stop makes more sense in 
the place where you generate the stop, than it does in the place where you tell 
yourself you've resumed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154649

___
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

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.

In D154617#4478402 , @fdeazeve wrote:

> 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.

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


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] D154649: [lldb] Fix dead lock issue when loading modules in Scripted Process

[Med Ismail Bennani via Phabricator via lldb-commits]

mib created this revision.
mib added reviewers: jingham, JDevlieghere.
mib added a project: LLDB.
Herald added a project: All.
mib requested review of this revision.
Herald added a subscriber: lldb-commits.

This patch attempts to fix a dead lock when loading modules in a Scripted
Process.

This issue was triggered by loading the modules after the process did resume,
but before the process actually stop, causing the language runtime mutex to
be locked by a separate thread, responsible to unwind the stack (using
the runtime unwind plan), while the module loading thread was trying to
notify the runtimes of the newly loaded module.

To address that, this patch moves the module loading logic to be done before
sending the stop event, to prevent the dead lock situation described above.

Signed-off-by: Med Ismail Bennani 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D154649

Files:
  lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
  lldb/source/Plugins/Process/scripted/ScriptedProcess.h


Index: lldb/source/Plugins/Process/scripted/ScriptedProcess.h
===
--- lldb/source/Plugins/Process/scripted/ScriptedProcess.h
+++ lldb/source/Plugins/Process/scripted/ScriptedProcess.h
@@ -12,6 +12,7 @@
 #include "lldb/Target/Process.h"
 #include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/ScriptedMetadata.h"
+#include "lldb/Utility/State.h"
 #include "lldb/Utility/Status.h"
 
 #include "ScriptedThread.h"
@@ -93,6 +94,8 @@
   void *GetImplementation() override;
 
   void ForceScriptedState(lldb::StateType state) override {
+if (StateIsStoppedState(state, true))
+  GetLoadedDynamicLibrariesInfos();
 SetPrivateState(state);
   }
 
Index: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
===
--- lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
+++ lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
@@ -166,7 +166,6 @@
 void ScriptedProcess::DidResume() {
   // Update the PID again, in case the user provided a placeholder pid at 
launch
   m_pid = GetInterface().GetProcessID();
-  GetLoadedDynamicLibrariesInfos();
 }
 
 Status ScriptedProcess::DoResume() {


Index: lldb/source/Plugins/Process/scripted/ScriptedProcess.h
===
--- lldb/source/Plugins/Process/scripted/ScriptedProcess.h
+++ lldb/source/Plugins/Process/scripted/ScriptedProcess.h
@@ -12,6 +12,7 @@
 #include "lldb/Target/Process.h"
 #include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/ScriptedMetadata.h"
+#include "lldb/Utility/State.h"
 #include "lldb/Utility/Status.h"
 
 #include "ScriptedThread.h"
@@ -93,6 +94,8 @@
   void *GetImplementation() override;
 
   void ForceScriptedState(lldb::StateType state) override {
+if (StateIsStoppedState(state, true))
+  GetLoadedDynamicLibrariesInfos();
 SetPrivateState(state);
   }
 
Index: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
===
--- lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
+++ lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
@@ -166,7 +166,6 @@
 void ScriptedProcess::DidResume() {
   // Update the PID again, in case the user provided a placeholder pid at launch
   m_pid = GetInterface().GetProcessID();
-  GetLoadedDynamicLibrariesInfos();
 }
 
 Status ScriptedProcess::DoResume() {
___
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.

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] D154617: [lldb][DebugNamesDWARF] Also use mangled name when matching regex

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

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


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] D154643: [lldb] Prevent crash when completing ambiguous subcommands

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere created this revision.
JDevlieghere added a reviewer: jingham.
Herald added a project: All.
JDevlieghere requested review of this revision.

Fix a crash when trying to complete an ambiguous subcommand. Take `set s tar` 
for example: here `s` is ambiguous between `set` and `show`. Pressing TAB after 
this input currently crashes LLDB because we go look for the subcommand and 
fail to find one, but still add the original command (`s`) as a completion. 
This causes problems later on when we're trying to insert the completion and 
find that the "completed string" is shorter than the input string and call 
`std::string::substr` to eliminate the common prefix.

  frame #12: 0x000133fafb44 
liblldb.17.0.0git.dylib`lldb_private::Editline::TabCommand(this=0x00010a645aa0,
 ch=9) at Editline.cpp:1044:24
 1041   std::string longest_prefix = completions.LongestCommonPrefix();
 1042   if (!longest_prefix.empty())
 1043 longest_prefix =
  -> 1044 
longest_prefix.substr(request.GetCursorArgumentPrefix().size());
  (lldb) v longest_prefix
  (std::string) longest_prefix = "s"
  (lldb) p request.GetCursorArgumentPrefix().size()
  (size_t) 3

rdar://111848598


https://reviews.llvm.org/D154643

Files:
  lldb/source/Commands/CommandObjectMultiword.cpp
  lldb/test/API/functionalities/completion/TestCompletion.py


Index: lldb/test/API/functionalities/completion/TestCompletion.py
===
--- lldb/test/API/functionalities/completion/TestCompletion.py
+++ lldb/test/API/functionalities/completion/TestCompletion.py
@@ -868,3 +868,7 @@
 self.complete_from_to("breakpoint set -N n", "breakpoint set -N n")
 self.assertTrue(bp1.AddNameWithErrorHandling("nn"))
 self.complete_from_to("breakpoint set -N ", "breakpoint set -N nn")
+
+def test_ambiguous_option(self):
+"""Make sure we don't crash when completing ambiguous commands"""
+self.complete_from_to("set s ta", [])
Index: lldb/source/Commands/CommandObjectMultiword.cpp
===
--- lldb/source/Commands/CommandObjectMultiword.cpp
+++ lldb/source/Commands/CommandObjectMultiword.cpp
@@ -274,10 +274,10 @@
 
   StringList new_matches;
   CommandObject *sub_command_object = GetSubcommandObject(arg0, &new_matches);
-  if (sub_command_object == nullptr) {
-request.AddCompletions(new_matches);
+
+  // The subcommand is ambiguous. The completion isn't meaningful.
+  if (!sub_command_object)
 return;
-  }
 
   // Remove the one match that we got from calling GetSubcommandObject.
   new_matches.DeleteStringAtIndex(0);


Index: lldb/test/API/functionalities/completion/TestCompletion.py
===
--- lldb/test/API/functionalities/completion/TestCompletion.py
+++ lldb/test/API/functionalities/completion/TestCompletion.py
@@ -868,3 +868,7 @@
 self.complete_from_to("breakpoint set -N n", "breakpoint set -N n")
 self.assertTrue(bp1.AddNameWithErrorHandling("nn"))
 self.complete_from_to("breakpoint set -N ", "breakpoint set -N nn")
+
+def test_ambiguous_option(self):
+"""Make sure we don't crash when completing ambiguous commands"""
+self.complete_from_to("set s ta", [])
Index: lldb/source/Commands/CommandObjectMultiword.cpp
===
--- lldb/source/Commands/CommandObjectMultiword.cpp
+++ lldb/source/Commands/CommandObjectMultiword.cpp
@@ -274,10 +274,10 @@
 
   StringList new_matches;
   CommandObject *sub_command_object = GetSubcommandObject(arg0, &new_matches);
-  if (sub_command_object == nullptr) {
-request.AddCompletions(new_matches);
+
+  // The subcommand is ambiguous. The completion isn't meaningful.
+  if (!sub_command_object)
 return;
-  }
 
   // Remove the one match that we got from calling GetSubcommandObject.
   new_matches.DeleteStringAtIndex(0);
___
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

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.



Comment at: lldb/source/Target/DynamicRegisterInfo.cpp:204
+m_sets.push_back(
+{ConstString(set_name).AsCString(), nullptr, 0, nullptr});
   } else {

mib wrote:
> I guess `m_sets` is a vector of `char*` ... Should we change it to 
> `lldb::StringList` or `llvm::StringSet` so we don't have to create a 
> `ConstString` here ?
`m_sets` is a vector of `lldb_private::RegisterSet` (which contains a `const 
char *`). I think it's assumed that they are backed by a `ConstString`. Because 
`RegisterSet` is defined in `lldb-private-types.h` we'd have to move it 
somewhere else before we could actually use any types other than `const char 
*`, I think?


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] D154513: [lldb][NFC] Factor out code from SymbolFileDWARF::ParseVariableDIE

[Alex Langford via Phabricator via lldb-commits]

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

Makes sense to me. There are some potential improvements we could make to this 
code (which I'm sure you can already identify), but to keep this change 
relatively risk-free we should probably stick to just shifting things around. 
Thanks for doing this!




Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3414-3420
+  DWARFExpressionList location_list = [&] {
+if (location_form.IsValid())
+  return GetExprListFromAtLocation(location_form, module, die, 
func_low_pc);
+if (const_value_form.IsValid())
+  return GetExprListFromAtConstValue(const_value_form, module, die);
+return DWARFExpressionList(module, DWARFExpression(), die.GetCU());
+  }();

Not related to the review itself, but this makes me wish C++ had "if 
expressions" from Rust...


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] D154534: [lldb][NFCI] Minor cleanups to StructuredData::GetObjectForDotSeparatedPath

[Alex Langford via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG8f7e41d0400e: [lldb][NFCI] Minor cleanups to 
StructuredData::GetObjectForDotSeparatedPath (authored by bulbazord).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154534

Files:
  lldb/source/Utility/StructuredData.cpp


Index: lldb/source/Utility/StructuredData.cpp
===
--- lldb/source/Utility/StructuredData.cpp
+++ lldb/source/Utility/StructuredData.cpp
@@ -104,36 +104,34 @@
 
 StructuredData::ObjectSP
 StructuredData::Object::GetObjectForDotSeparatedPath(llvm::StringRef path) {
-  if (this->GetType() == lldb::eStructuredDataTypeDictionary) {
+  if (GetType() == lldb::eStructuredDataTypeDictionary) {
 std::pair match = path.split('.');
-std::string key = match.first.str();
-ObjectSP value = this->GetAsDictionary()->GetValueForKey(key);
-if (value.get()) {
-  // Do we have additional words to descend?  If not, return the value
-  // we're at right now.
-  if (match.second.empty()) {
-return value;
-  } else {
-return value->GetObjectForDotSeparatedPath(match.second);
-  }
-}
-return ObjectSP();
+llvm::StringRef key = match.first;
+ObjectSP value = GetAsDictionary()->GetValueForKey(key);
+if (!value)
+  return {};
+
+// Do we have additional words to descend?  If not, return the value
+// we're at right now.
+if (match.second.empty())
+  return value;
+
+return value->GetObjectForDotSeparatedPath(match.second);
   }
 
-  if (this->GetType() == lldb::eStructuredDataTypeArray) {
+  if (GetType() == lldb::eStructuredDataTypeArray) {
 std::pair match = path.split('[');
-if (match.second.empty()) {
-  return this->shared_from_this();
-}
-errno = 0;
-uint64_t val = strtoul(match.second.str().c_str(), nullptr, 10);
-if (errno == 0) {
-  return this->GetAsArray()->GetItemAtIndex(val);
-}
-return ObjectSP();
+if (match.second.empty())
+  return shared_from_this();
+
+uint64_t val = 0;
+if (!llvm::to_integer(match.second, val, /* Base = */ 10))
+  return {};
+
+return GetAsArray()->GetItemAtIndex(val);
   }
 
-  return this->shared_from_this();
+  return shared_from_this();
 }
 
 void StructuredData::Object::DumpToStdout(bool pretty_print) const {


Index: lldb/source/Utility/StructuredData.cpp
===
--- lldb/source/Utility/StructuredData.cpp
+++ lldb/source/Utility/StructuredData.cpp
@@ -104,36 +104,34 @@
 
 StructuredData::ObjectSP
 StructuredData::Object::GetObjectForDotSeparatedPath(llvm::StringRef path) {
-  if (this->GetType() == lldb::eStructuredDataTypeDictionary) {
+  if (GetType() == lldb::eStructuredDataTypeDictionary) {
 std::pair match = path.split('.');
-std::string key = match.first.str();
-ObjectSP value = this->GetAsDictionary()->GetValueForKey(key);
-if (value.get()) {
-  // Do we have additional words to descend?  If not, return the value
-  // we're at right now.
-  if (match.second.empty()) {
-return value;
-  } else {
-return value->GetObjectForDotSeparatedPath(match.second);
-  }
-}
-return ObjectSP();
+llvm::StringRef key = match.first;
+ObjectSP value = GetAsDictionary()->GetValueForKey(key);
+if (!value)
+  return {};
+
+// Do we have additional words to descend?  If not, return the value
+// we're at right now.
+if (match.second.empty())
+  return value;
+
+return value->GetObjectForDotSeparatedPath(match.second);
   }
 
-  if (this->GetType() == lldb::eStructuredDataTypeArray) {
+  if (GetType() == lldb::eStructuredDataTypeArray) {
 std::pair match = path.split('[');
-if (match.second.empty()) {
-  return this->shared_from_this();
-}
-errno = 0;
-uint64_t val = strtoul(match.second.str().c_str(), nullptr, 10);
-if (errno == 0) {
-  return this->GetAsArray()->GetItemAtIndex(val);
-}
-return ObjectSP();
+if (match.second.empty())
+  return shared_from_this();
+
+uint64_t val = 0;
+if (!llvm::to_integer(match.second, val, /* Base = */ 10))
+  return {};
+
+return GetAsArray()->GetItemAtIndex(val);
   }
 
-  return this->shared_from_this();
+  return shared_from_this();
 }
 
 void StructuredData::Object::DumpToStdout(bool pretty_print) const {
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 8f7e41d - [lldb][NFCI] Minor cleanups to StructuredData::GetObjectForDotSeparatedPath

[Alex Langford via lldb-commits]

Author: Alex Langford
Date: 2023-07-06T08:51:54-07:00
New Revision: 8f7e41d0400ecc41f8120e330baed15b0d203036

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

LOG: [lldb][NFCI] Minor cleanups to StructuredData::GetObjectForDotSeparatedPath

This accomplishes a few minor things:
- Removed unnecessary uses of `this->`
- Removed an unnecessary std::string allocation.
- Removed some nesting to improve readability using early returns where
  it makes sense.
- Replaced `strtoul` with `llvm::to_integer` which avoids another
  std::string allocation.
- Removed braces from single statement conditions, removed
  else-after-returns.

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

Added: 


Modified: 
lldb/source/Utility/StructuredData.cpp

Removed: 




diff  --git a/lldb/source/Utility/StructuredData.cpp 
b/lldb/source/Utility/StructuredData.cpp
index 674a9fa927806a..be50df323facdd 100644
--- a/lldb/source/Utility/StructuredData.cpp
+++ b/lldb/source/Utility/StructuredData.cpp
@@ -104,36 +104,34 @@ static StructuredData::ObjectSP 
ParseJSONArray(json::Array *array) {
 
 StructuredData::ObjectSP
 StructuredData::Object::GetObjectForDotSeparatedPath(llvm::StringRef path) {
-  if (this->GetType() == lldb::eStructuredDataTypeDictionary) {
+  if (GetType() == lldb::eStructuredDataTypeDictionary) {
 std::pair match = path.split('.');
-std::string key = match.first.str();
-ObjectSP value = this->GetAsDictionary()->GetValueForKey(key);
-if (value.get()) {
-  // Do we have additional words to descend?  If not, return the value
-  // we're at right now.
-  if (match.second.empty()) {
-return value;
-  } else {
-return value->GetObjectForDotSeparatedPath(match.second);
-  }
-}
-return ObjectSP();
+llvm::StringRef key = match.first;
+ObjectSP value = GetAsDictionary()->GetValueForKey(key);
+if (!value)
+  return {};
+
+// Do we have additional words to descend?  If not, return the value
+// we're at right now.
+if (match.second.empty())
+  return value;
+
+return value->GetObjectForDotSeparatedPath(match.second);
   }
 
-  if (this->GetType() == lldb::eStructuredDataTypeArray) {
+  if (GetType() == lldb::eStructuredDataTypeArray) {
 std::pair match = path.split('[');
-if (match.second.empty()) {
-  return this->shared_from_this();
-}
-errno = 0;
-uint64_t val = strtoul(match.second.str().c_str(), nullptr, 10);
-if (errno == 0) {
-  return this->GetAsArray()->GetItemAtIndex(val);
-}
-return ObjectSP();
+if (match.second.empty())
+  return shared_from_this();
+
+uint64_t val = 0;
+if (!llvm::to_integer(match.second, val, /* Base = */ 10))
+  return {};
+
+return GetAsArray()->GetItemAtIndex(val);
   }
 
-  return this->shared_from_this();
+  return shared_from_this();
 }
 
 void StructuredData::Object::DumpToStdout(bool pretty_print) const {



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

[Lldb-commits] [PATCH] D154386: [lldb][NFCI] Remove use of ConstString from OptionValue

[Alex Langford via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGfc55b0b38446: [lldb][NFCI] Remove use of ConstString from 
OptionValue (authored by bulbazord).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154386

Files:
  lldb/source/Interpreter/OptionValue.cpp


Index: lldb/source/Interpreter/OptionValue.cpp
===
--- lldb/source/Interpreter/OptionValue.cpp
+++ lldb/source/Interpreter/OptionValue.cpp
@@ -534,8 +534,8 @@
 if (m_parent_sp->DumpQualifiedName(strm))
   dumped_something = true;
   }
-  ConstString name(GetName());
-  if (name) {
+  llvm::StringRef name(GetName());
+  if (!name.empty()) {
 if (dumped_something)
   strm.PutChar('.');
 else


Index: lldb/source/Interpreter/OptionValue.cpp
===
--- lldb/source/Interpreter/OptionValue.cpp
+++ lldb/source/Interpreter/OptionValue.cpp
@@ -534,8 +534,8 @@
 if (m_parent_sp->DumpQualifiedName(strm))
   dumped_something = true;
   }
-  ConstString name(GetName());
-  if (name) {
+  llvm::StringRef name(GetName());
+  if (!name.empty()) {
 if (dumped_something)
   strm.PutChar('.');
 else
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] fc55b0b - [lldb][NFCI] Remove use of ConstString from OptionValue

[Alex Langford via lldb-commits]

Author: Alex Langford
Date: 2023-07-06T08:49:07-07:00
New Revision: fc55b0b38446be8948a291057b3086b4a01fe0a7

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

LOG: [lldb][NFCI] Remove use of ConstString from OptionValue

Summary: No need to create a ConstString, `GetName` already returns a StringRef.

Reviewers: JDevlieghere, mib, jasonmolenda

Subscribers:

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

Added: 


Modified: 
lldb/source/Interpreter/OptionValue.cpp

Removed: 




diff  --git a/lldb/source/Interpreter/OptionValue.cpp 
b/lldb/source/Interpreter/OptionValue.cpp
index 2f110d53d95899..8d429b6bc9cf21 100644
--- a/lldb/source/Interpreter/OptionValue.cpp
+++ b/lldb/source/Interpreter/OptionValue.cpp
@@ -534,8 +534,8 @@ bool OptionValue::DumpQualifiedName(Stream &strm) const {
 if (m_parent_sp->DumpQualifiedName(strm))
   dumped_something = true;
   }
-  ConstString name(GetName());
-  if (name) {
+  llvm::StringRef name(GetName());
+  if (!name.empty()) {
 if (dumped_something)
   strm.PutChar('.');
 else



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

[Lldb-commits] [PATCH] D154542: Further refinements on reporting interruption in lldb

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.



Comment at: lldb/include/lldb/Core/Debugger.h:430-431
+  template 
+  bool InterruptRequested(const char *cur_func, 
+  const char *formatv, Args &&... args) {
+bool ret_val = InterruptRequested();

jingham wrote:
> bulbazord wrote:
> > I think it would make more sense to have `cur_func` and `formatv` be of 
> > type `llvm::StringRef`. Concretely, if you construct a `std::string` from 
> > `nullptr` (like you're doing below when you make an InterruptionReport 
> > object), you will crash.
> > 
> > I know the recommendation is to use `INTERRUPT_REQUESTED` instead of 
> > filling this manually, but inevitably somebody will go against the advice 
> > and make a mistake.
> > I think it would make more sense to have `cur_func` and `formatv` be of 
> > type `llvm::StringRef`. Concretely, if you construct a `std::string` from 
> > `nullptr` (like you're doing below when you make an InterruptionReport 
> > object), you will crash.
> > 
> > I know the recommendation is to use `INTERRUPT_REQUESTED` instead of 
> > filling this manually, but inevitably somebody will go against the advice 
> > and make a mistake.
> 
> I don't see how I can make the formatv option a StringRef.  The llvm::formatv 
> API only offers a version that takes a `const char *`.  Anyway, these are 
> formatv strings, they are almost universally going to be const strings.  
> Turning them into llvm::StringRef's and back out again to use in 
> llvm::formatv seems odd.
> 
> But you are right, we should protect against someone passing in a null 
> pointer for the function or format to InterruptRequested.   Since this is 
> just logging, an assert seems overkill, I'll just add null pointer checks 
> here and turn them into "UNKNOWN FUNCTION" and "Unknown message".
Oh... so `llvm::formatv` does take a `const char *`... My bad there, that makes 
sense, no need to change the type of `format` then.

Otherwise, I'm satisfied with the safety checks.



Comment at: lldb/include/lldb/Core/Debugger.h:455-456
+InterruptionReport(std::string function_name, std::string description) :
+m_function_name(function_name), 
+m_description(description),
+m_interrupt_time(std::chrono::system_clock::now()),

jingham wrote:
> bulbazord wrote:
> > To avoid some unnecessary copies
> > 
> > Could also do what Ismail is suggesting.
> This is a local that is copied to an ivar and never used again.  Do I really 
> have to put move in there explicitly for the optimizer to know it can reuse 
> the value?
Yes. I created a simple example on Godbolt: https://godbolt.org/z/G4ej76Enn

Observe that the constructor in the example invokes the `std::string` copy 
constructor. Add a `std::move` and it then invokes the move constructor.



Comment at: lldb/include/lldb/Core/Debugger.h:465
+  InterruptionReport(std::string function_name,
+  const char *format, Args &&... args) :
+InterruptionReport(function_name, llvm::formatv(format, 
std::forward(args)...)) {}

bulbazord wrote:
> since we're passing `format` to `formatv`, I think it would make sense for 
> its type to be `llvm::StringRef` up-front here.
As you've pointed out above, `formatv` takes a `const char *` so ignore this.



Comment at: lldb/source/Core/Debugger.cpp:1280
+const llvm::formatv_object_base &payload) :  
+m_function_name(function_name), 
+m_interrupt_time(std::chrono::system_clock::now()),

You'll also want to `std::move(function_name)` here too, I think.



Comment at: lldb/source/Target/TargetList.cpp:518
  "target already exists it the list");
+  UnregisterInProcessTarget(target_sp.get());
   m_target_list.push_back(std::move(target_sp));

jingham wrote:
> bulbazord wrote:
> > I'm somewhat puzzled by this. Why are we unregistering the target in 
> > `AddTargetInternal` when we're registering it in `CreateTargetInternal`? 
> > Maybe I don't understand the intent behind 
> > `{Register,Unregister}InProcessTarget`.
> The point behind this is someone says "make me a target" and then in the 
> process of making the target, the Target code does something slow (e.g. look 
> for external debug files) that we want to interrupt.  So we need a way for 
> the debugger to find the "in the process of being made" Targets.  
> 
>  "AddTargetInternal" is the API where the target gets added to the Debugger's 
> TargetList, so after that method has run we will find the target in the 
> regular TargetList.  But between CreateTargetInternal and AddTargetInternal, 
> that target isn't stored anywhere that the Debugger knows how to query.  I 
> added this list to hold the targets that are in the process of getting made, 
> before they get added to the Debugger's TargetList.
Gotcha, that makes sense. Thanks for explaining!


Repository:
  rG LLVM

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

[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.

When LLDB queries the debug names index with a regex, we should use the
`Mangled` class wrapper, which attempts to match regex first against the mangled
name and then against the demangled name. This is what is done for the Apple
index as well.

This fixes test/API/lang/cpp/class_static/main.cpp when compiled with DWARF 5.


Repository:
  rG LLVM Github Monorepo

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] D154610: [lldb][NFC] Remove unused parameter from DebugNames::ProcessEntry

[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.

Repository:
  rG LLVM Github Monorepo

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);
   if (!ref)
 return true;
@@ -92,7 +92,7 @@

[Lldb-commits] [PATCH] D154505: [lldb][NFC] Remove code duplication in InitOSO

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

This revision was automatically updated to reflect the committed changes.
Closed by commit rG7cea22c0be95: [lldb][NFC] Remove code duplication in InitOSO 
(authored by fdeazeve).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154505

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

Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
@@ -32,6 +32,7 @@
 #include "lldb/Symbol/SymbolVendor.h"
 #include "lldb/Symbol/TypeMap.h"
 #include "lldb/Symbol/VariableList.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/ScopedPrinter.h"
 
 #include "lldb/Target/StackFrame.h"
@@ -286,112 +287,105 @@
   // we return the abilities of the first N_OSO's DWARF.
 
   Symtab *symtab = m_objfile_sp->GetSymtab();
-  if (symtab) {
-Log *log = GetLog(DWARFLog::DebugMap);
-
-std::vector oso_indexes;
-// When a mach-o symbol is encoded, the n_type field is encoded in bits
-// 23:16, and the n_desc field is encoded in bits 15:0.
-//
-// To find all N_OSO entries that are part of the DWARF + debug map we find
-// only object file symbols with the flags value as follows: bits 23:16 ==
-// 0x66 (N_OSO) bits 15: 0 == 0x0001 (specifies this is a debug map object
-// file)
-const uint32_t k_oso_symbol_flags_value = 0x660001u;
-
-const uint32_t oso_index_count =
-symtab->AppendSymbolIndexesWithTypeAndFlagsValue(
-eSymbolTypeObjectFile, k_oso_symbol_flags_value, oso_indexes);
-
-if (oso_index_count > 0) {
-  symtab->AppendSymbolIndexesWithType(eSymbolTypeCode, Symtab::eDebugYes,
-  Symtab::eVisibilityAny,
-  m_func_indexes);
-  symtab->AppendSymbolIndexesWithType(eSymbolTypeData, Symtab::eDebugYes,
-  Symtab::eVisibilityAny,
-  m_glob_indexes);
-
-  symtab->SortSymbolIndexesByValue(m_func_indexes, true);
-  symtab->SortSymbolIndexesByValue(m_glob_indexes, true);
-
-  for (uint32_t sym_idx : m_func_indexes) {
-const Symbol *symbol = symtab->SymbolAtIndex(sym_idx);
-lldb::addr_t file_addr = symbol->GetAddressRef().GetFileAddress();
-lldb::addr_t byte_size = symbol->GetByteSize();
-DebugMap::Entry debug_map_entry(
-file_addr, byte_size, OSOEntry(sym_idx, LLDB_INVALID_ADDRESS));
-m_debug_map.Append(debug_map_entry);
-  }
-  for (uint32_t sym_idx : m_glob_indexes) {
-const Symbol *symbol = symtab->SymbolAtIndex(sym_idx);
-lldb::addr_t file_addr = symbol->GetAddressRef().GetFileAddress();
-lldb::addr_t byte_size = symbol->GetByteSize();
-DebugMap::Entry debug_map_entry(
-file_addr, byte_size, OSOEntry(sym_idx, LLDB_INVALID_ADDRESS));
-m_debug_map.Append(debug_map_entry);
-  }
-  m_debug_map.Sort();
-
-  m_compile_unit_infos.resize(oso_index_count);
-
-  for (uint32_t i = 0; i < oso_index_count; ++i) {
-const uint32_t so_idx = oso_indexes[i] - 1;
-const uint32_t oso_idx = oso_indexes[i];
-const Symbol *so_symbol = symtab->SymbolAtIndex(so_idx);
-const Symbol *oso_symbol = symtab->SymbolAtIndex(oso_idx);
-if (so_symbol && oso_symbol &&
-so_symbol->GetType() == eSymbolTypeSourceFile &&
-oso_symbol->GetType() == eSymbolTypeObjectFile) {
-  m_compile_unit_infos[i].so_file.SetFile(
-  so_symbol->GetName().AsCString(), FileSpec::Style::native);
-  m_compile_unit_infos[i].oso_path = oso_symbol->GetName();
-  m_compile_unit_infos[i].oso_mod_time =
-  llvm::sys::toTimePoint(oso_symbol->GetIntegerValue(0));
-  uint32_t sibling_idx = so_symbol->GetSiblingIndex();
-  // The sibling index can't be less that or equal to the current index
-  // "i"
-  if (sibling_idx == UINT32_MAX) {
-m_objfile_sp->GetModule()->ReportError(
-"N_SO in symbol with UID {0} has invalid sibling in debug "
-"map, "
-"please file a bug and attach the binary listed in this error",
-so_symbol->GetID());
-  } else {
-const Symbol *last_symbol = symtab->SymbolAtIndex(sibling_idx - 1);
-m_compile_unit_infos[i].first_symbol_index = so_idx;
-m_compile_unit_infos[i].last_symbol_index = sibling_idx - 1;
-m_compile_unit_infos[i].first_symbol_id = so_symbol->GetID();
-m_compile_unit_infos[i].last_symbol_id = last_symbol->GetID();
-
-LLDB_LOGF(log, "Initialized OSO 0x%8.8x: file=%s", i,
-   

[Lldb-commits] [lldb] 7cea22c - [lldb][NFC] Remove code duplication in InitOSO

[Felipe de Azevedo Piovezan via lldb-commits]

Author: Felipe de Azevedo Piovezan
Date: 2023-07-06T08:20:31-04:00
New Revision: 7cea22c0be95834317c920c21839cdd4c80fbdd8

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

LOG: [lldb][NFC] Remove code duplication in InitOSO

Two identical loops were iterating over different ranges, leading to code
duplication. We replace this by a loop over the concatenation of the ranges.

We also use early returns to avoid deeply nested code and explicitly check for a
condition mentioned in comments.

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

Added: 


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

Removed: 




diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
index 131933777bf31c..bb66fbadfb6429 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
@@ -32,6 +32,7 @@
 #include "lldb/Symbol/SymbolVendor.h"
 #include "lldb/Symbol/TypeMap.h"
 #include "lldb/Symbol/VariableList.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/ScopedPrinter.h"
 
 #include "lldb/Target/StackFrame.h"
@@ -286,112 +287,105 @@ void SymbolFileDWARFDebugMap::InitOSO() {
   // we return the abilities of the first N_OSO's DWARF.
 
   Symtab *symtab = m_objfile_sp->GetSymtab();
-  if (symtab) {
-Log *log = GetLog(DWARFLog::DebugMap);
-
-std::vector oso_indexes;
-// When a mach-o symbol is encoded, the n_type field is encoded in bits
-// 23:16, and the n_desc field is encoded in bits 15:0.
-//
-// To find all N_OSO entries that are part of the DWARF + debug map we find
-// only object file symbols with the flags value as follows: bits 23:16 ==
-// 0x66 (N_OSO) bits 15: 0 == 0x0001 (specifies this is a debug map object
-// file)
-const uint32_t k_oso_symbol_flags_value = 0x660001u;
-
-const uint32_t oso_index_count =
-symtab->AppendSymbolIndexesWithTypeAndFlagsValue(
-eSymbolTypeObjectFile, k_oso_symbol_flags_value, oso_indexes);
-
-if (oso_index_count > 0) {
-  symtab->AppendSymbolIndexesWithType(eSymbolTypeCode, Symtab::eDebugYes,
-  Symtab::eVisibilityAny,
-  m_func_indexes);
-  symtab->AppendSymbolIndexesWithType(eSymbolTypeData, Symtab::eDebugYes,
-  Symtab::eVisibilityAny,
-  m_glob_indexes);
-
-  symtab->SortSymbolIndexesByValue(m_func_indexes, true);
-  symtab->SortSymbolIndexesByValue(m_glob_indexes, true);
-
-  for (uint32_t sym_idx : m_func_indexes) {
-const Symbol *symbol = symtab->SymbolAtIndex(sym_idx);
-lldb::addr_t file_addr = symbol->GetAddressRef().GetFileAddress();
-lldb::addr_t byte_size = symbol->GetByteSize();
-DebugMap::Entry debug_map_entry(
-file_addr, byte_size, OSOEntry(sym_idx, LLDB_INVALID_ADDRESS));
-m_debug_map.Append(debug_map_entry);
-  }
-  for (uint32_t sym_idx : m_glob_indexes) {
-const Symbol *symbol = symtab->SymbolAtIndex(sym_idx);
-lldb::addr_t file_addr = symbol->GetAddressRef().GetFileAddress();
-lldb::addr_t byte_size = symbol->GetByteSize();
-DebugMap::Entry debug_map_entry(
-file_addr, byte_size, OSOEntry(sym_idx, LLDB_INVALID_ADDRESS));
-m_debug_map.Append(debug_map_entry);
-  }
-  m_debug_map.Sort();
-
-  m_compile_unit_infos.resize(oso_index_count);
-
-  for (uint32_t i = 0; i < oso_index_count; ++i) {
-const uint32_t so_idx = oso_indexes[i] - 1;
-const uint32_t oso_idx = oso_indexes[i];
-const Symbol *so_symbol = symtab->SymbolAtIndex(so_idx);
-const Symbol *oso_symbol = symtab->SymbolAtIndex(oso_idx);
-if (so_symbol && oso_symbol &&
-so_symbol->GetType() == eSymbolTypeSourceFile &&
-oso_symbol->GetType() == eSymbolTypeObjectFile) {
-  m_compile_unit_infos[i].so_file.SetFile(
-  so_symbol->GetName().AsCString(), FileSpec::Style::native);
-  m_compile_unit_infos[i].oso_path = oso_symbol->GetName();
-  m_compile_unit_infos[i].oso_mod_time =
-  llvm::sys::toTimePoint(oso_symbol->GetIntegerValue(0));
-  uint32_t sibling_idx = so_symbol->GetSiblingIndex();
-  // The sibling index can't be less that or equal to the current index
-  // "i"
-  if (sibling_idx == UINT32_MAX) {
-m_objfile_sp->GetModule()->ReportError(
-"N_SO in symbol with UID {0} has invalid sibling in debug "
-"map, "
-  

[Lldb-commits] [PATCH] D153740: [llvm][Support] Deprecate llvm::writeFileAtomically API

[Haojian Wu via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
hokein marked an inline comment as done.
Closed by commit rG7aafea001282: [llvm][Support] Deprecate 
llvm::writeFileAtomically API (authored by hokein).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153740

Files:
  llvm/include/llvm/Support/FileUtilities.h
  llvm/lib/Support/FileUtilities.cpp
  llvm/unittests/Support/CMakeLists.txt
  llvm/unittests/Support/FileUtilitiesTest.cpp

Index: llvm/unittests/Support/FileUtilitiesTest.cpp
===
--- llvm/unittests/Support/FileUtilitiesTest.cpp
+++ /dev/null
@@ -1,54 +0,0 @@
-//===- llvm/unittest/Support/FileUtilitiesTest.cpp - unit tests ---===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===--===//
-
-#include "llvm/Support/FileUtilities.h"
-#include "llvm/Support/Errc.h"
-#include "llvm/Support/ErrorHandling.h"
-#include "llvm/Support/FileSystem.h"
-#include "llvm/Support/MemoryBuffer.h"
-#include "llvm/Support/Path.h"
-#include "llvm/Testing/Support/SupportHelpers.h"
-#include "gtest/gtest.h"
-#include 
-
-using namespace llvm;
-using namespace llvm::sys;
-
-using llvm::unittest::TempDir;
-
-#define ASSERT_NO_ERROR(x) \
-  if (std::error_code ASSERT_NO_ERROR_ec = x) {\
-SmallString<128> MessageStorage;   \
-raw_svector_ostream Message(MessageStorage);   \
-Message << #x ": did not return errc::success.\n"  \
-<< "error number: " << ASSERT_NO_ERROR_ec.value() << "\n"  \
-<< "error message: " << ASSERT_NO_ERROR_ec.message() << "\n";  \
-GTEST_FATAL_FAILURE_(MessageStorage.c_str());  \
-  } else { \
-  }
-
-namespace {
-TEST(writeFileAtomicallyTest, Test) {
-  // Create unique temporary directory for these tests
-  TempDir RootTestDirectory("writeFileAtomicallyTest", /*Unique*/ true);
-
-  SmallString<128> FinalTestfilePath(RootTestDirectory.path());
-  sys::path::append(FinalTestfilePath, "foo.txt");
-  const std::string TempUniqTestFileModel =
-  std::string(FinalTestfilePath) + "-";
-  const std::string TestfileContent = "fooFOOfoo";
-
-  llvm::Error Err = llvm::writeFileAtomically(TempUniqTestFileModel, FinalTestfilePath, TestfileContent);
-  ASSERT_FALSE(static_cast(Err));
-
-  std::ifstream FinalFileStream(std::string(FinalTestfilePath.str()));
-  std::string FinalFileContent;
-  FinalFileStream >> FinalFileContent;
-  ASSERT_EQ(FinalFileContent, TestfileContent);
-}
-} // anonymous namespace
Index: llvm/unittests/Support/CMakeLists.txt
===
--- llvm/unittests/Support/CMakeLists.txt
+++ llvm/unittests/Support/CMakeLists.txt
@@ -41,7 +41,6 @@
   ExtensibleRTTITest.cpp
   FileCollectorTest.cpp
   FileOutputBufferTest.cpp
-  FileUtilitiesTest.cpp
   FormatVariadicTest.cpp
   FSUniqueIDTest.cpp
   GlobPatternTest.cpp
Index: llvm/lib/Support/FileUtilities.cpp
===
--- llvm/lib/Support/FileUtilities.cpp
+++ llvm/lib/Support/FileUtilities.cpp
@@ -267,64 +267,6 @@
   return CompareFailed;
 }
 
-void llvm::AtomicFileWriteError::log(raw_ostream &OS) const {
-  OS << "atomic_write_error: ";
-  switch (Error) {
-  case atomic_write_error::failed_to_create_uniq_file:
-OS << "failed_to_create_uniq_file";
-return;
-  case atomic_write_error::output_stream_error:
-OS << "output_stream_error";
-return;
-  case atomic_write_error::failed_to_rename_temp_file:
-OS << "failed_to_rename_temp_file";
-return;
-  }
-  llvm_unreachable("unknown atomic_write_error value in "
-   "failed_to_rename_temp_file::log()");
-}
-
-llvm::Error llvm::writeFileAtomically(StringRef TempPathModel,
-  StringRef FinalPath, StringRef Buffer) {
-  return writeFileAtomically(TempPathModel, FinalPath,
- [&Buffer](llvm::raw_ostream &OS) {
-   OS.write(Buffer.data(), Buffer.size());
-   return llvm::Error::success();
- });
-}
-
-llvm::Error llvm::writeFileAtomically(
-StringRef TempPathModel, StringRef FinalPath,
-std::function Writer) {
-  SmallString<128> GeneratedUniqPath;
-  int TempFD;
-  if (sys::fs::createUniqueFile(TempPathModel, TempFD,

[Lldb-commits] [PATCH] D154413: [lldb] Fix crash when completing register names after program exit

[David Spickett via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGfd8929904d88: [lldb] Fix crash when completing register 
names after program exit (authored by DavidSpickett).

Changed prior to commit:
  https://reviews.llvm.org/D154413?vs=537021&id=537622#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154413

Files:
  lldb/source/Commands/CommandCompletions.cpp
  lldb/test/API/functionalities/completion/TestCompletion.py


Index: lldb/test/API/functionalities/completion/TestCompletion.py
===
--- lldb/test/API/functionalities/completion/TestCompletion.py
+++ lldb/test/API/functionalities/completion/TestCompletion.py
@@ -736,13 +736,25 @@
 self.runCmd("type synthetic add -x Hoo -l test")
 self.complete_from_to("type synthetic delete ", ["Hoo"])
 
-@skipIf(archs=no_match(["x86_64"]))
-def test_register_read_and_write_on_x86(self):
-"""Test the completion of the commands register read and write on 
x86"""
-
+def test_register_no_complete(self):
 # The tab completion for "register read/write"  won't work without a 
running process.
 self.complete_from_to("register read ", "register read ")
 self.complete_from_to("register write ", "register write ")
+self.complete_from_to("register info ", "register info ")
+
+self.build()
+self.runCmd("target create {}".format(self.getBuildArtifact("a.out")))
+self.runCmd("run")
+
+# Once a program has finished you have an execution context but no 
register
+# context so completion cannot work.
+self.complete_from_to("register read ", "register read ")
+self.complete_from_to("register write ", "register write ")
+self.complete_from_to("register info ", "register info ")
+
+@skipIf(archs=no_match(["x86_64"]))
+def test_register_read_and_write_on_x86(self):
+"""Test the completion of the commands register read and write on 
x86"""
 
 self.build()
 self.main_source_spec = lldb.SBFileSpec("main.cpp")
Index: lldb/source/Commands/CommandCompletions.cpp
===
--- lldb/source/Commands/CommandCompletions.cpp
+++ lldb/source/Commands/CommandCompletions.cpp
@@ -611,6 +611,9 @@
 
   RegisterContext *reg_ctx =
   interpreter.GetExecutionContext().GetRegisterContext();
+  if (!reg_ctx)
+return;
+
   const size_t reg_num = reg_ctx->GetRegisterCount();
   for (size_t reg_idx = 0; reg_idx < reg_num; ++reg_idx) {
 const RegisterInfo *reg_info = reg_ctx->GetRegisterInfoAtIndex(reg_idx);


Index: lldb/test/API/functionalities/completion/TestCompletion.py
===
--- lldb/test/API/functionalities/completion/TestCompletion.py
+++ lldb/test/API/functionalities/completion/TestCompletion.py
@@ -736,13 +736,25 @@
 self.runCmd("type synthetic add -x Hoo -l test")
 self.complete_from_to("type synthetic delete ", ["Hoo"])
 
-@skipIf(archs=no_match(["x86_64"]))
-def test_register_read_and_write_on_x86(self):
-"""Test the completion of the commands register read and write on x86"""
-
+def test_register_no_complete(self):
 # The tab completion for "register read/write"  won't work without a running process.
 self.complete_from_to("register read ", "register read ")
 self.complete_from_to("register write ", "register write ")
+self.complete_from_to("register info ", "register info ")
+
+self.build()
+self.runCmd("target create {}".format(self.getBuildArtifact("a.out")))
+self.runCmd("run")
+
+# Once a program has finished you have an execution context but no register
+# context so completion cannot work.
+self.complete_from_to("register read ", "register read ")
+self.complete_from_to("register write ", "register write ")
+self.complete_from_to("register info ", "register info ")
+
+@skipIf(archs=no_match(["x86_64"]))
+def test_register_read_and_write_on_x86(self):
+"""Test the completion of the commands register read and write on x86"""
 
 self.build()
 self.main_source_spec = lldb.SBFileSpec("main.cpp")
Index: lldb/source/Commands/CommandCompletions.cpp
===
--- lldb/source/Commands/CommandCompletions.cpp
+++ lldb/source/Commands/CommandCompletions.cpp
@@ -611,6 +611,9 @@
 
   RegisterContext *reg_ctx =
   interpreter.GetExecutionContext().GetRegisterContext();
+  if (!reg_ctx)
+return;
+
   const size_t reg_num = reg_ctx->GetRegisterCount();
   for (size_t reg_idx = 0; reg_idx < reg_num; ++reg_idx) {
 const RegisterInfo *reg_info = reg_ctx->GetRegisterInfoAtIndex(reg_idx);
___

[Lldb-commits] [lldb] fd89299 - [lldb] Fix crash when completing register names after program exit

[David Spickett via lldb-commits]

Author: David Spickett
Date: 2023-07-06T08:35:04Z
New Revision: fd8929904d8820629bae940bce5141ef65bcaa11

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

LOG: [lldb] Fix crash when completing register names after program exit

Previously the following would crash:
(lldb) run
Process 2594053 launched: '/tmp/test.o' (aarch64)
Process 2594053 exited with status = 0 (0x)
(lldb) register read 

As the completer assumed that the execution context would always
have a register context. After a program has finished, it does not.

Split out the generic parts of the test from the x86 specific tests,
and added "register info" to both.

Reviewed By: JDevlieghere

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

Added: 


Modified: 
lldb/source/Commands/CommandCompletions.cpp
lldb/test/API/functionalities/completion/TestCompletion.py

Removed: 




diff  --git a/lldb/source/Commands/CommandCompletions.cpp 
b/lldb/source/Commands/CommandCompletions.cpp
index dad63b6aaa1744..21ffba5fa1f3d8 100644
--- a/lldb/source/Commands/CommandCompletions.cpp
+++ b/lldb/source/Commands/CommandCompletions.cpp
@@ -611,6 +611,9 @@ void CommandCompletions::Registers(CommandInterpreter 
&interpreter,
 
   RegisterContext *reg_ctx =
   interpreter.GetExecutionContext().GetRegisterContext();
+  if (!reg_ctx)
+return;
+
   const size_t reg_num = reg_ctx->GetRegisterCount();
   for (size_t reg_idx = 0; reg_idx < reg_num; ++reg_idx) {
 const RegisterInfo *reg_info = reg_ctx->GetRegisterInfoAtIndex(reg_idx);

diff  --git a/lldb/test/API/functionalities/completion/TestCompletion.py 
b/lldb/test/API/functionalities/completion/TestCompletion.py
index bc39f18fb225b8..2d83abd7a6573e 100644
--- a/lldb/test/API/functionalities/completion/TestCompletion.py
+++ b/lldb/test/API/functionalities/completion/TestCompletion.py
@@ -736,13 +736,25 @@ def test_completion_type_formatter_delete(self):
 self.runCmd("type synthetic add -x Hoo -l test")
 self.complete_from_to("type synthetic delete ", ["Hoo"])
 
-@skipIf(archs=no_match(["x86_64"]))
-def test_register_read_and_write_on_x86(self):
-"""Test the completion of the commands register read and write on 
x86"""
-
+def test_register_no_complete(self):
 # The tab completion for "register read/write"  won't work without a 
running process.
 self.complete_from_to("register read ", "register read ")
 self.complete_from_to("register write ", "register write ")
+self.complete_from_to("register info ", "register info ")
+
+self.build()
+self.runCmd("target create {}".format(self.getBuildArtifact("a.out")))
+self.runCmd("run")
+
+# Once a program has finished you have an execution context but no 
register
+# context so completion cannot work.
+self.complete_from_to("register read ", "register read ")
+self.complete_from_to("register write ", "register write ")
+self.complete_from_to("register info ", "register info ")
+
+@skipIf(archs=no_match(["x86_64"]))
+def test_register_read_and_write_on_x86(self):
+"""Test the completion of the commands register read and write on 
x86"""
 
 self.build()
 self.main_source_spec = lldb.SBFileSpec("main.cpp")



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