date:20230518

[Lldb-commits] [PATCH] D150928: Two bug fixes for loading process save-core created core files, plus perf improvements

2023-05-18 Thread Alex Langford via Phabricator via lldb-commits

bulbazord added a comment.

Bug fixes look good to me, the perf and code simplifications are also great to 
see. A few nits and I have one concern. Otherwise looks fine to me, I think.




Comment at: lldb/include/lldb/Target/DynamicLoader.h:270-273
+  LoadBinaryWithUUIDAndAddress(Process *process, llvm::StringRef name,
+   UUID uuid, lldb::addr_t value,
+   bool value_is_offset, bool force_symbol_search,
+   bool notify, bool set_address_in_target);

Just thinking aloud here, I wonder if we should construct some kind of 
"Options" struct to contain all of these bools?



Comment at: lldb/source/Core/DynamicLoader.cpp:216-218
+if (!module_sp || !module_sp->GetSymbolFileFileSpec()) {
+  Symbols::DownloadObjectAndSymbolFile(module_spec, error,
+   force_symbol_search);

I think this **technically** changes behavior and probably not in a desirable 
way. Specifically, we previously only called `DownloadObjectAndSymbolFile` if 
`force_symbol_search` was true. Now you're passing it through which makes sense 
looking at the interface of `DownloadObjectAndSymbolFile`. However, peeking at 
the beginning of that function we see this code block:
```
  // When dbgshell_command is empty, the user has not enabled the use of an
  // external program to find the symbols, don't run it for them.
  if (!force_lookup && dbgshell_command.empty())
return false;
```
This means that even if `force_lookup` is false (or `force_symbol_search` as 
it's written here), if `dbgshell_command` is non-empty we'll still perform the 
lookup (assuming all the other checks pass) which is probably not what we 
intended performance-wise. That condition in the block above should probably be 
something like `!force_lookup || dbgshell_command.empty()` instead of `&&`.



Comment at: lldb/source/Core/DynamicLoader.cpp:246-248
+  LLDB_LOGF(log,
+"DynamicLoader::LoadBinaryWithUUIDAndAddress Loading "
+"binary UUID %s at %s 0x%" PRIx64,

You could use `LLDB_LOG` instead of `LLDB_LOGF` here and you'll get the file + 
function without explicitly writing it, but you'd have to change the formatting 
a bit since it uses `Format` and not `Printf` under the hood. Just a suggestion 
though.



Comment at: lldb/source/Core/DynamicLoader.cpp:255-257
+  LLDB_LOGF(log,
+"DynamicLoader::LoadBinaryWithUUIDAndAddress Loading "
+"binary UUID %s at file address",

Same here



Comment at: lldb/source/Core/DynamicLoader.cpp:264-266
+LLDB_LOGF(log,
+  "DynamicLoader::LoadBinaryWithUUIDAndAddress Loading binary "
+  "UUID %s from memory at address 0x%" PRIx64,

Also here



Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:7021
 }
-const bool address_is_slide = true;
 bool changed = false;
+module_sp->SetLoadAddress(process.GetTarget(), value, value_is_offset,

nit: this can be const



Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:1000
   this, "", standalone_uuid, standalone_value,
-  standalone_value_is_offset, force_symbol_search, notify);
+  standalone_value_is_offset, force_symbol_search, notify, true);
 }

nit: `, /* set_address_in_target */ true);` (or something like this)

Or you can create a `const bool` like above with `force_symbol_search` and 
`notify`.



Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:1031
+  this, llvm::StringRef(), uuid, addr, value_is_slide,
+  force_symbol_search, notify, true);
 }

Same here



Comment at: lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp:262-264
   this, llvm::StringRef(), objfile_binary_uuid,
   objfile_binary_value, objfile_binary_value_is_offset,
+  force_symbol_search, notify, true)) {

Same here



Comment at: lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp:318
   this, llvm::StringRef(), ident_uuid, ident_binary_addr,
-  value_is_offset, force_symbol_search, notify)) {
+  value_is_offset, force_symbol_search, notify, true)) {
 found_main_binary_definitively = true;

Same here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150928

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

[Lldb-commits] [PATCH] D141907: [CMake] Ensure `CLANG_RESOURCE_DIR` is respected

2023-05-18 Thread Junchang Liu via Phabricator via lldb-commits

paperchalice added a comment.

In D141907#4355228 , @tstellar wrote:

> @paperchalice Do you need someone to commit this for you?

Sure, I don't have the commit access.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141907

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

[Lldb-commits] [PATCH] D141907: [CMake] Ensure `CLANG_RESOURCE_DIR` is respected

2023-05-18 Thread Tom Stellard via Phabricator via lldb-commits

tstellar added a comment.

@paperchalice Do you need someone to commit this for you?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141907

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

[Lldb-commits] [PATCH] D150928: Two bug fixes for loading process save-core created core files, plus perf improvements

2023-05-18 Thread Jason Molenda via Phabricator via lldb-commits

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

I had two bugs with lldb's mach-o corefile loader using the "all image infos" 
LC_NOTE, and while working on those, I was looking closely at the code and 
found several perf mistakes where I would re-do work multiple times.  This 
patch unfortunately ends up doing several things at once.

1. If we load binaries in ObjectFileMachO via the "all image infos" LC_NOTE, 
`ProcessMachCore::LoadBinariesViaMetadata()` should detect that binaries were 
loaded by it, and not then scan for a `dyld` in the corefile, throw away the 
Target image list already constructed, and try to recreate it (and possibly not 
be successful).  Also fix a mistake for the "main bin spec" LC_NOTE with 
userland process type in `ProcessMachCore::LoadBinariesViaMetadata`, noting 
that this is a userland dyld situation.  It ProcessMachCore currently falls 
back to a scan, and finds dyld by exhaustive search, but that's unnecessary 
work.

2. Binaries would first be loaded into the Target at their mach-header vm 
address, then ObjectFileMachO would load each binary's segments at the correct 
vmaddrs.  With binaries in the Darwin shared cache, the actual layout in memory 
has TEXT and DATA in discontiguous vm ranges.  But when we load all the 
segments in the Target at the mach header address contiguously, we get "address 
0x... maps to more than one section" warnings.  The addresses for the file 
would then be set correctly on a segment-by-segment basis in ObjectFileMachO, 
but the warnings were displayed to the user.  For this, I add a 
`set_address_in_target` bool argument to 
`DynamicLoader::LoadBinaryWithUUIDAndAddress` to control whether it sets the 
address of the binary it finds in the Target, or if it leaves this to the 
caller.  In `ObjectFileMachO::LoadCoreFileImages` when a binary has per-segment 
vmaddrs, it sets a local `set_load_address` and we pass that in to our calls.  
Update other callers to all pass `set_load_address=true` (e.g. 
`ProcessGDBRemote::LoadStubBinaries`) to 
`DynamicLoader::LoadBinaryWithUUIDAndAddress` to preserve existing behavior for 
them.

3. Perf fix in `ObjectFileMachO::LoadCoreFileImages` where it would try to find 
the binary for a UUID multiple times as different approaches were unsuccessful. 
 These attempts can be slow, and doing them multiple times can be more slow.  
Also a lot of code duplication if we have `image.load_address` or 
`image.slide`, but most lldb internal API take a uint64_t value and a "value is 
offset/slide" bool specifically to avoid this kind of duplication..

4. A previously committed perf fix https://reviews.llvm.org/D150621 was also 
driven by this investigation, but that patch was easily chiseled off into a 
separate patch, so I did that one already.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D150928

Files:
  lldb/include/lldb/Target/DynamicLoader.h
  lldb/source/Core/DynamicLoader.cpp
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp

Index: lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
===
--- lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
+++ lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
@@ -252,20 +252,19 @@
   m_mach_kernel_addr = objfile_binary_value;
   m_dyld_plugin_name = DynamicLoaderDarwinKernel::GetPluginNameStatic();
   found_main_binary_definitively = true;
+} else if (type == ObjectFile::eBinaryTypeUser) {
+  m_dyld_addr = objfile_binary_value;
+  m_dyld_plugin_name = DynamicLoaderMacOSXDYLD::GetPluginNameStatic();
 } else {
   const bool force_symbol_search = true;
   const bool notify = true;
   if (DynamicLoader::LoadBinaryWithUUIDAndAddress(
   this, llvm::StringRef(), objfile_binary_uuid,
   objfile_binary_value, objfile_binary_value_is_offset,
-  force_symbol_search, notify)) {
+  force_symbol_search, notify, true)) {
 found_main_binary_definitively = true;
 m_dyld_plugin_name = DynamicLoaderStatic::GetPluginNameStatic();
   }
-  if (type == ObjectFile::eBinaryTypeUser) {
-m_dyld_addr = objfile_binary_value;
-m_dyld_plugin_name = DynamicLoaderMacOSXDYLD::GetPluginNameStatic();
-  }
 }
   }
 
@@ -316,7 +315,7 @@
   const bool notify = true;
   if (DynamicLoader::LoadBinaryWithUUIDAndAddress(
   this, llvm::StringRef(), ident_uuid, ident_binary_addr,
-  value_is_offset, force_symbol_search, notify)) {
+  value_is_offset, force_symbol_search, notify,

[Lldb-commits] [PATCH] D150914: [lldb][NFCI] Pre-allocate storage in ObjCLanguage::MethodName::GetFullNameWithoutCategory

2023-05-18 Thread Alex Langford via Phabricator via lldb-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rGe8d3f061ba41: [lldb][NFCI] Pre-allocate storage in 
ObjCLanguage::MethodName… (authored by bulbazord).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150914

Files:
  lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp


Index: lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp
===
--- lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp
+++ lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp
@@ -152,7 +152,15 @@
 
   llvm::StringRef class_name = GetClassName();
   llvm::StringRef selector_name = GetSelector();
+
+  // Compute the total size to avoid reallocations
+  // class name + selector name + '[' + ' ' + ']'
+  size_t total_size = class_name.size() + selector_name.size() + 3;
+  if (m_type != eTypeUnspecified)
+total_size++; // For + or -
+
   std::string name_sans_category;
+  name_sans_category.reserve(total_size);
 
   if (m_type == eTypeClassMethod)
 name_sans_category += '+';


Index: lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp
===
--- lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp
+++ lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp
@@ -152,7 +152,15 @@
 
   llvm::StringRef class_name = GetClassName();
   llvm::StringRef selector_name = GetSelector();
+
+  // Compute the total size to avoid reallocations
+  // class name + selector name + '[' + ' ' + ']'
+  size_t total_size = class_name.size() + selector_name.size() + 3;
+  if (m_type != eTypeUnspecified)
+total_size++; // For + or -
+
   std::string name_sans_category;
+  name_sans_category.reserve(total_size);
 
   if (m_type == eTypeClassMethod)
 name_sans_category += '+';
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] e8d3f06 - [lldb][NFCI] Pre-allocate storage in ObjCLanguage::MethodName::GetFullNameWithoutCategory

2023-05-18 Thread Alex Langford via lldb-commits


Author: Alex Langford
Date: 2023-05-18T16:23:15-07:00
New Revision: e8d3f061ba415e9f95739e0004fb75552d68df86

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

LOG: [lldb][NFCI] Pre-allocate storage in 
ObjCLanguage::MethodName::GetFullNameWithoutCategory

The size of a full ObjC MethodName can vary somewhat, but it is
computable ahead of time.

Using a reasonably sized ObjC application, this actually improves the
time it takes to initialize symbol indexes for ObjC names ever so
slightly. Additionally, I found that the variability in time also was
improved considerably.

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

Added: 


Modified: 
lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp

Removed: 




diff  --git a/lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp 
b/lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp
index fb87d5d80b96c..3a9e287158329 100644
--- a/lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp
+++ b/lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp
@@ -152,7 +152,15 @@ std::string 
ObjCLanguage::MethodName::GetFullNameWithoutCategory() const {
 
   llvm::StringRef class_name = GetClassName();
   llvm::StringRef selector_name = GetSelector();
+
+  // Compute the total size to avoid reallocations
+  // class name + selector name + '[' + ' ' + ']'
+  size_t total_size = class_name.size() + selector_name.size() + 3;
+  if (m_type != eTypeUnspecified)
+total_size++; // For + or -
+
   std::string name_sans_category;
+  name_sans_category.reserve(total_size);
 
   if (m_type == eTypeClassMethod)
 name_sans_category += '+';



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

[Lldb-commits] [PATCH] D149914: [lldb] Refactor ObjCLanguage::MethodName

2023-05-18 Thread Alex Langford via Phabricator via lldb-commits

bulbazord marked an inline comment as done.
bulbazord added inline comments.



Comment at: lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp:155
+  llvm::StringRef selector_name = GetSelector();
+  std::string name_sans_category;
+

bulbazord wrote:
> aprantl wrote:
> > bulbazord wrote:
> > > aprantl wrote:
> > > > Using an llvm string stream to construct the string might be faster 
> > > > here.
> > > I tested this and found that `llvm::SmallString<128>` performs about as 
> > > well as `std::string` on average. `llvm::SmallString<64>` was on average 
> > > slower though.
> > > 
> > > I think I'm going to land this with std::string, we can revisit later if 
> > > the difference actually is measurable.
> > Maybe we were talking past each other: I was thinking about 
> > https://www.llvm.org/doxygen/classllvm_1_1raw__string__ostream.html
> Oh, I totally misread what you said. I'll try that out and create a new PR if 
> I can improve this.
Turns out it can be improved, but it wasn't with raw_string_ostream! 
https://reviews.llvm.org/D150914


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149914

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

[Lldb-commits] [PATCH] D150914: [lldb][NFCI] Pre-allocate storage in ObjCLanguage::MethodName::GetFullNameWithoutCategory

2023-05-18 Thread Alex Langford via Phabricator via lldb-commits

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

The size of a full ObjC MethodName can vary somewhat, but it is
computable ahead of time.

Using a reasonably sized ObjC application, this actually improves the
time it takes to initialize symbol indexes for ObjC names ever so
slightly. Additionally, I found that the variability in time also was
improved considerably.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D150914

Files:
  lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp


Index: lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp
===
--- lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp
+++ lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp
@@ -152,7 +152,15 @@
 
   llvm::StringRef class_name = GetClassName();
   llvm::StringRef selector_name = GetSelector();
+
+  // Compute the total size to avoid reallocations
+  // class name + selector name + '[' + ' ' + ']'
+  size_t total_size = class_name.size() + selector_name.size() + 3;
+  if (m_type != eTypeUnspecified)
+total_size++; // For + or -
+
   std::string name_sans_category;
+  name_sans_category.reserve(total_size);
 
   if (m_type == eTypeClassMethod)
 name_sans_category += '+';


Index: lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp
===
--- lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp
+++ lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp
@@ -152,7 +152,15 @@
 
   llvm::StringRef class_name = GetClassName();
   llvm::StringRef selector_name = GetSelector();
+
+  // Compute the total size to avoid reallocations
+  // class name + selector name + '[' + ' ' + ']'
+  size_t total_size = class_name.size() + selector_name.size() + 3;
+  if (m_type != eTypeUnspecified)
+total_size++; // For + or -
+
   std::string name_sans_category;
+  name_sans_category.reserve(total_size);
 
   if (m_type == eTypeClassMethod)
 name_sans_category += '+';
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] e5a6950 - [lldb] Remove decl for non-existent ResolveSymbolContextForAddress overload (NFC)

2023-05-18 Thread Dave Lee via lldb-commits


Author: Dave Lee
Date: 2023-05-18T15:30:19-07:00
New Revision: e5a695026c78e6bb1abf55a0878066459e1080bc

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

LOG: [lldb] Remove decl for non-existent ResolveSymbolContextForAddress 
overload (NFC)

Added: 


Modified: 
lldb/include/lldb/Core/Module.h

Removed: 




diff  --git a/lldb/include/lldb/Core/Module.h b/lldb/include/lldb/Core/Module.h
index 31f7894178d79..565c613f63702 100644
--- a/lldb/include/lldb/Core/Module.h
+++ b/lldb/include/lldb/Core/Module.h
@@ -1106,43 +1106,6 @@ class Module : public 
std::enable_shared_from_this,
   std::once_flag m_optimization_warning;
   std::once_flag m_language_warning;
 
-  /// Resolve a file or load virtual address.
-  ///
-  /// Tries to resolve \a vm_addr as a file address (if \a
-  /// vm_addr_is_file_addr is true) or as a load address if \a
-  /// vm_addr_is_file_addr is false) in the symbol vendor. \a resolve_scope
-  /// indicates what clients wish to resolve and can be used to limit the
-  /// scope of what is parsed.
-  ///
-  /// \param[in] vm_addr
-  /// The load virtual address to resolve.
-  ///
-  /// \param[in] vm_addr_is_file_addr
-  /// If \b true, \a vm_addr is a file address, else \a vm_addr
-  /// if a load address.
-  ///
-  /// \param[in] resolve_scope
-  /// The scope that should be resolved (see
-  /// SymbolContext::Scope).
-  ///
-  /// \param[out] so_addr
-  /// The section offset based address that got resolved if
-  /// any bits are returned.
-  ///
-  /// \param[out] sc
-  //  The symbol context that has objects filled in. Each bit
-  /// in the \a resolve_scope pertains to a member in the \a sc.
-  ///
-  /// \return
-  /// A integer that contains SymbolContext::Scope bits set for
-  /// each item that was successfully resolved.
-  ///
-  /// \see SymbolContext::Scope
-  uint32_t ResolveSymbolContextForAddress(lldb::addr_t vm_addr,
-  bool vm_addr_is_file_addr,
-  lldb::SymbolContextItem 
resolve_scope,
-  Address _addr, SymbolContext );
-
   void SymbolIndicesToSymbolContextList(Symtab *symtab,
 std::vector _indexes,
 SymbolContextList _list);



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

[Lldb-commits] [PATCH] D150804: [lldb] Guarantee the lifetimes of all strings returned from SBAPI

2023-05-18 Thread Alex Langford via Phabricator via lldb-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rG41714c959d65: [lldb] Guarantee the lifetimes of all strings 
returned from SBAPI (authored by bulbazord).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150804

Files:
  lldb/docs/design/sbapi.rst
  lldb/source/API/SBAttachInfo.cpp
  lldb/source/API/SBBreakpoint.cpp
  lldb/source/API/SBBreakpointLocation.cpp
  lldb/source/API/SBBreakpointName.cpp
  lldb/source/API/SBCommandInterpreter.cpp
  lldb/source/API/SBData.cpp
  lldb/source/API/SBEvent.cpp
  lldb/source/API/SBExpressionOptions.cpp
  lldb/source/API/SBFrame.cpp
  lldb/source/API/SBFunction.cpp
  lldb/source/API/SBInstruction.cpp
  lldb/source/API/SBLaunchInfo.cpp
  lldb/source/API/SBModule.cpp
  lldb/source/API/SBPlatform.cpp
  lldb/source/API/SBProcess.cpp
  lldb/source/API/SBProcessInfo.cpp
  lldb/source/API/SBQueue.cpp
  lldb/source/API/SBStream.cpp
  lldb/source/API/SBStringList.cpp
  lldb/source/API/SBTarget.cpp
  lldb/source/API/SBThread.cpp
  lldb/source/API/SBTrace.cpp
  lldb/source/API/SBTraceCursor.cpp
  lldb/source/API/SBType.cpp
  lldb/source/API/SBTypeCategory.cpp
  lldb/source/API/SBTypeFilter.cpp
  lldb/source/API/SBTypeNameSpecifier.cpp
  lldb/source/API/SBTypeSummary.cpp
  lldb/source/API/SBTypeSynthetic.cpp
  lldb/source/API/SBUnixSignals.cpp
  lldb/source/API/SBValue.cpp
  lldb/source/API/SBWatchpoint.cpp

Index: lldb/source/API/SBWatchpoint.cpp
===
--- lldb/source/API/SBWatchpoint.cpp
+++ lldb/source/API/SBWatchpoint.cpp
@@ -210,12 +210,12 @@
   LLDB_INSTRUMENT_VA(this);
 
   lldb::WatchpointSP watchpoint_sp(GetSP());
-  if (watchpoint_sp) {
-std::lock_guard guard(
-watchpoint_sp->GetTarget().GetAPIMutex());
-return watchpoint_sp->GetConditionText();
-  }
-  return nullptr;
+  if (!watchpoint_sp)
+return nullptr;
+
+  std::lock_guard guard(
+  watchpoint_sp->GetTarget().GetAPIMutex());
+  return ConstString(watchpoint_sp->GetConditionText()).GetCString();
 }
 
 void SBWatchpoint::SetCondition(const char *condition) {
@@ -323,16 +323,16 @@
   LLDB_INSTRUMENT_VA(this);
 
   lldb::WatchpointSP watchpoint_sp(GetSP());
-  if (watchpoint_sp) {
-std::lock_guard guard(
-watchpoint_sp->GetTarget().GetAPIMutex());
-// Store the result of `GetWatchSpec()` as a ConstString
-// so that the C string we return has a sufficiently long
-// lifetime. Note this a memory leak but should be fairly
-// low impact.
-return ConstString(watchpoint_sp->GetWatchSpec()).AsCString();
-  }
-  return nullptr;
+  if (!watchpoint_sp)
+return nullptr;
+
+  std::lock_guard guard(
+  watchpoint_sp->GetTarget().GetAPIMutex());
+  // Store the result of `GetWatchSpec()` as a ConstString
+  // so that the C string we return has a sufficiently long
+  // lifetime. Note this a memory leak but should be fairly
+  // low impact.
+  return ConstString(watchpoint_sp->GetWatchSpec()).AsCString();
 }
 
 bool SBWatchpoint::IsWatchingReads() {
Index: lldb/source/API/SBValue.cpp
===
--- lldb/source/API/SBValue.cpp
+++ lldb/source/API/SBValue.cpp
@@ -291,39 +291,34 @@
 const char *SBValue::GetName() {
   LLDB_INSTRUMENT_VA(this);
 
-  const char *name = nullptr;
   ValueLocker locker;
   lldb::ValueObjectSP value_sp(GetSP(locker));
-  if (value_sp)
-name = value_sp->GetName().GetCString();
+  if (!value_sp)
+return nullptr;
 
-  return name;
+  return value_sp->GetName().GetCString();
 }
 
 const char *SBValue::GetTypeName() {
   LLDB_INSTRUMENT_VA(this);
 
-  const char *name = nullptr;
   ValueLocker locker;
   lldb::ValueObjectSP value_sp(GetSP(locker));
-  if (value_sp) {
-name = value_sp->GetQualifiedTypeName().GetCString();
-  }
+  if (!value_sp)
+return nullptr;
 
-  return name;
+  return value_sp->GetQualifiedTypeName().GetCString();
 }
 
 const char *SBValue::GetDisplayTypeName() {
   LLDB_INSTRUMENT_VA(this);
 
-  const char *name = nullptr;
   ValueLocker locker;
   lldb::ValueObjectSP value_sp(GetSP(locker));
-  if (value_sp) {
-name = value_sp->GetDisplayTypeName().GetCString();
-  }
+  if (!value_sp)
+return nullptr;
 
-  return name;
+  return value_sp->GetDisplayTypeName().GetCString();
 }
 
 size_t SBValue::GetByteSize() {
@@ -357,14 +352,11 @@
 const char *SBValue::GetValue() {
   LLDB_INSTRUMENT_VA(this);
 
-  const char *cstr = nullptr;
   ValueLocker locker;
   lldb::ValueObjectSP value_sp(GetSP(locker));
-  if (value_sp) {
-cstr = value_sp->GetValueAsCString();
-  }
-
-  return cstr;
+  if (!value_sp)
+return nullptr;
+  return ConstString(value_sp->GetValueAsCString()).GetCString();
 }
 
 ValueType SBValue::GetValueType() {
@@ -382,14 +374,12 @@
 const char *SBValue::GetObjectDescription() {
   LLDB_INSTRUMENT_VA(this);
 
-  const char *cstr = nullptr;

[Lldb-commits] [lldb] 41714c9 - [lldb] Guarantee the lifetimes of all strings returned from SBAPI

2023-05-18 Thread Alex Langford via lldb-commits


Author: Alex Langford
Date: 2023-05-18T15:13:36-07:00
New Revision: 41714c959d65ff1dd842bc0a0d44f90b06440c39

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

LOG: [lldb] Guarantee the lifetimes of all strings returned from SBAPI

LLDB should guarantee that the strings returned by SBAPI methods
live forever. I went through every method that returns a string and made
sure that it was added to the ConstString StringPool before returning if
it wasn't obvious that it was already doing so.
I've also updated the docs to document this behavior.

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

Added: 


Modified: 
lldb/docs/design/sbapi.rst
lldb/source/API/SBAttachInfo.cpp
lldb/source/API/SBBreakpoint.cpp
lldb/source/API/SBBreakpointLocation.cpp
lldb/source/API/SBBreakpointName.cpp
lldb/source/API/SBCommandInterpreter.cpp
lldb/source/API/SBData.cpp
lldb/source/API/SBEvent.cpp
lldb/source/API/SBExpressionOptions.cpp
lldb/source/API/SBFrame.cpp
lldb/source/API/SBFunction.cpp
lldb/source/API/SBInstruction.cpp
lldb/source/API/SBLaunchInfo.cpp
lldb/source/API/SBModule.cpp
lldb/source/API/SBPlatform.cpp
lldb/source/API/SBProcess.cpp
lldb/source/API/SBProcessInfo.cpp
lldb/source/API/SBQueue.cpp
lldb/source/API/SBStream.cpp
lldb/source/API/SBStringList.cpp
lldb/source/API/SBTarget.cpp
lldb/source/API/SBThread.cpp
lldb/source/API/SBTrace.cpp
lldb/source/API/SBTraceCursor.cpp
lldb/source/API/SBType.cpp
lldb/source/API/SBTypeCategory.cpp
lldb/source/API/SBTypeFilter.cpp
lldb/source/API/SBTypeNameSpecifier.cpp
lldb/source/API/SBTypeSummary.cpp
lldb/source/API/SBTypeSynthetic.cpp
lldb/source/API/SBUnixSignals.cpp
lldb/source/API/SBValue.cpp
lldb/source/API/SBWatchpoint.cpp

Removed: 




diff  --git a/lldb/docs/design/sbapi.rst b/lldb/docs/design/sbapi.rst
index f2de0e7ae60ca..cf32cc6c81558 100644
--- a/lldb/docs/design/sbapi.rst
+++ b/lldb/docs/design/sbapi.rst
@@ -72,6 +72,15 @@ building the LLDB framework for macOS, the headers are 
processed with
 ``unifdef`` prior to being copied into the framework bundle to remove macros
 involving SWIG.
 
+Lifetime
+
+Many SB API methods will return strings in the form of ``const char *`` values.
+Once created, these strings are guaranteed to live until the end of the
+debugging session. LLDB owns these strings, clients should not attempt to free
+them. Doing so may cause LLDB to crash.
+Note that this only affects the C++ API as scripting languages usually
+will usually create native string types from the ``const char *`` value.
+
 API Instrumentation
 ---
 

diff  --git a/lldb/source/API/SBAttachInfo.cpp 
b/lldb/source/API/SBAttachInfo.cpp
index 1cd1146f5d317..8ce1f1d65c496 100644
--- a/lldb/source/API/SBAttachInfo.cpp
+++ b/lldb/source/API/SBAttachInfo.cpp
@@ -94,7 +94,7 @@ void SBAttachInfo::SetResumeCount(uint32_t c) {
 const char *SBAttachInfo::GetProcessPluginName() {
   LLDB_INSTRUMENT_VA(this);
 
-  return m_opaque_sp->GetProcessPluginName();
+  return ConstString(m_opaque_sp->GetProcessPluginName()).GetCString();
 }
 
 void SBAttachInfo::SetProcessPluginName(const char *plugin_name) {

diff  --git a/lldb/source/API/SBBreakpoint.cpp 
b/lldb/source/API/SBBreakpoint.cpp
index c83fe8c13752e..6631afb1483fb 100644
--- a/lldb/source/API/SBBreakpoint.cpp
+++ b/lldb/source/API/SBBreakpoint.cpp
@@ -284,12 +284,12 @@ const char *SBBreakpoint::GetCondition() {
   LLDB_INSTRUMENT_VA(this);
 
   BreakpointSP bkpt_sp = GetSP();
-  if (bkpt_sp) {
-std::lock_guard guard(
-bkpt_sp->GetTarget().GetAPIMutex());
-return bkpt_sp->GetConditionText();
-  }
-  return nullptr;
+  if (!bkpt_sp)
+return nullptr;
+
+  std::lock_guard guard(
+  bkpt_sp->GetTarget().GetAPIMutex());
+  return ConstString(bkpt_sp->GetConditionText()).GetCString();
 }
 
 void SBBreakpoint::SetAutoContinue(bool auto_continue) {
@@ -411,18 +411,17 @@ void SBBreakpoint::SetThreadName(const char *thread_name) 
{
 const char *SBBreakpoint::GetThreadName() const {
   LLDB_INSTRUMENT_VA(this);
 
-  const char *name = nullptr;
   BreakpointSP bkpt_sp = GetSP();
-  if (bkpt_sp) {
-std::lock_guard guard(
-bkpt_sp->GetTarget().GetAPIMutex());
-const ThreadSpec *thread_spec =
-bkpt_sp->GetOptions().GetThreadSpecNoCreate();
-if (thread_spec != nullptr)
-  name = thread_spec->GetName();
-  }
+  if (!bkpt_sp)
+return nullptr;
+
+  std::lock_guard guard(
+  bkpt_sp->GetTarget().GetAPIMutex());
+  if (const ThreadSpec *thread_spec =
+  bkpt_sp->GetOptions().GetThreadSpecNoCreate())
+return ConstString(thread_spec->GetName()).GetCString();
 
-  return name;
+  return nullptr;
 }

[Lldb-commits] [PATCH] D146765: [lldb/crashlog] Load inlined symbol into interactive crashlog

2023-05-18 Thread Med Ismail Bennani via Phabricator via lldb-commits

mib marked 2 inline comments as done.
mib added inline comments.



Comment at: lldb/examples/python/crashlog.py:669
+ r'(0x[0-9a-fA-F]{4,}) +'   # addr (4 
chars or more)
+ r'((.*)(?:(?: +\+ +)([0-9]+))|[^\s]+)' # symbol + 
offset
 )

kastiglione wrote:
> kastiglione wrote:
> > mib wrote:
> > > kastiglione wrote:
> > > > mib wrote:
> > > > > @kastiglione may be you have a better idea how to handle `symbol + 
> > > > > offset` where ` + offset` might be optional.
> > > > symbol is always present?
> > > Technically, in the past the `offs` (symbol + offset) was optional:
> > > ```
> > > r'(?: +(.*))?'
> > > ```
> > > So I guess `symbol` could be missing.
> > Breaking it down, there is:
> > 
> > 1. a symbol (maybe)
> > 2. the literal text " + " followed by a number (also maybe)
> > 
> > I'll start start with 2:
> > 
> > ```
> >  \+ (\d+)
> > ```
> > 
> > For the symbol, `.*`, it should instead have at least a length of 1. I'd 
> > use `+` instead of `*`. And, it shouldn't be _any_ character. At the very 
> > least it should be non-space, which is `\S`.
> > 
> > To combine these at a high level it looks like:
> > 
> > ```
> > (?:()(?:)?)?
> > ```
> > 
> > Filling in these two with symbol='\S+' and offset=' \+ (\d+)', it becomes:
> > 
> > ```
> > (?:(\S+)(?: \+ (\d+))?)?
> > ```
> > 
> > Here's some python real session that minimally exercises this code:
> > 
> > ```
> > % python
> > >>> import re
> > >>> pat = re.compile(r"(?:(\S+)(?: \+ (\d+))?)?")
> > >>> pat.match("func + 123").groups()
> > ('func', '123')
> > >>> pat.match("func").groups()
> > ('func', None)
> > >>> pat.match("").groups()
> > (None, None)
> > >>> 
> > ```
> ```
> (?:(?: +\+ +)([0-9]+))
> ```
> 
> Non-capturing groups are only needed when you use a quantifier, like `?`, 
> `*`, or `+`. These regex has two non-capturing groups that don't use a 
> quantifier, which means you can remove the groups:
> 
> ```
>  +\+ +([0-9]+)
> ```
Thanks for the tips! Regarding replacing the `.+` by `\S+` doesn't work because 
the symbol name could have a space in it (for objc symbols).

I'll try to break down these regex in smaller ones to make it easier to debug 
in the future. 


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

https://reviews.llvm.org/D146765

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

[Lldb-commits] [PATCH] D150804: [lldb] Guarantee the lifetimes of all strings returned from SBAPI

2023-05-18 Thread Alex Langford via Phabricator via lldb-commits

bulbazord updated this revision to Diff 523578.
bulbazord marked an inline comment as done.
bulbazord added a comment.

Update SBModule::GetUUIDString -- I slightly changed behavior. Now the behavior 
matches the previous implementation exactly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150804

Files:
  lldb/docs/design/sbapi.rst
  lldb/source/API/SBAttachInfo.cpp
  lldb/source/API/SBBreakpoint.cpp
  lldb/source/API/SBBreakpointLocation.cpp
  lldb/source/API/SBBreakpointName.cpp
  lldb/source/API/SBCommandInterpreter.cpp
  lldb/source/API/SBData.cpp
  lldb/source/API/SBEvent.cpp
  lldb/source/API/SBExpressionOptions.cpp
  lldb/source/API/SBFrame.cpp
  lldb/source/API/SBFunction.cpp
  lldb/source/API/SBInstruction.cpp
  lldb/source/API/SBLaunchInfo.cpp
  lldb/source/API/SBModule.cpp
  lldb/source/API/SBPlatform.cpp
  lldb/source/API/SBProcess.cpp
  lldb/source/API/SBProcessInfo.cpp
  lldb/source/API/SBQueue.cpp
  lldb/source/API/SBStream.cpp
  lldb/source/API/SBStringList.cpp
  lldb/source/API/SBTarget.cpp
  lldb/source/API/SBThread.cpp
  lldb/source/API/SBTrace.cpp
  lldb/source/API/SBTraceCursor.cpp
  lldb/source/API/SBType.cpp
  lldb/source/API/SBTypeCategory.cpp
  lldb/source/API/SBTypeFilter.cpp
  lldb/source/API/SBTypeNameSpecifier.cpp
  lldb/source/API/SBTypeSummary.cpp
  lldb/source/API/SBTypeSynthetic.cpp
  lldb/source/API/SBUnixSignals.cpp
  lldb/source/API/SBValue.cpp
  lldb/source/API/SBWatchpoint.cpp

Index: lldb/source/API/SBWatchpoint.cpp
===
--- lldb/source/API/SBWatchpoint.cpp
+++ lldb/source/API/SBWatchpoint.cpp
@@ -210,12 +210,12 @@
   LLDB_INSTRUMENT_VA(this);
 
   lldb::WatchpointSP watchpoint_sp(GetSP());
-  if (watchpoint_sp) {
-std::lock_guard guard(
-watchpoint_sp->GetTarget().GetAPIMutex());
-return watchpoint_sp->GetConditionText();
-  }
-  return nullptr;
+  if (!watchpoint_sp)
+return nullptr;
+
+  std::lock_guard guard(
+  watchpoint_sp->GetTarget().GetAPIMutex());
+  return ConstString(watchpoint_sp->GetConditionText()).GetCString();
 }
 
 void SBWatchpoint::SetCondition(const char *condition) {
@@ -323,16 +323,16 @@
   LLDB_INSTRUMENT_VA(this);
 
   lldb::WatchpointSP watchpoint_sp(GetSP());
-  if (watchpoint_sp) {
-std::lock_guard guard(
-watchpoint_sp->GetTarget().GetAPIMutex());
-// Store the result of `GetWatchSpec()` as a ConstString
-// so that the C string we return has a sufficiently long
-// lifetime. Note this a memory leak but should be fairly
-// low impact.
-return ConstString(watchpoint_sp->GetWatchSpec()).AsCString();
-  }
-  return nullptr;
+  if (!watchpoint_sp)
+return nullptr;
+
+  std::lock_guard guard(
+  watchpoint_sp->GetTarget().GetAPIMutex());
+  // Store the result of `GetWatchSpec()` as a ConstString
+  // so that the C string we return has a sufficiently long
+  // lifetime. Note this a memory leak but should be fairly
+  // low impact.
+  return ConstString(watchpoint_sp->GetWatchSpec()).AsCString();
 }
 
 bool SBWatchpoint::IsWatchingReads() {
Index: lldb/source/API/SBValue.cpp
===
--- lldb/source/API/SBValue.cpp
+++ lldb/source/API/SBValue.cpp
@@ -291,39 +291,34 @@
 const char *SBValue::GetName() {
   LLDB_INSTRUMENT_VA(this);
 
-  const char *name = nullptr;
   ValueLocker locker;
   lldb::ValueObjectSP value_sp(GetSP(locker));
-  if (value_sp)
-name = value_sp->GetName().GetCString();
+  if (!value_sp)
+return nullptr;
 
-  return name;
+  return value_sp->GetName().GetCString();
 }
 
 const char *SBValue::GetTypeName() {
   LLDB_INSTRUMENT_VA(this);
 
-  const char *name = nullptr;
   ValueLocker locker;
   lldb::ValueObjectSP value_sp(GetSP(locker));
-  if (value_sp) {
-name = value_sp->GetQualifiedTypeName().GetCString();
-  }
+  if (!value_sp)
+return nullptr;
 
-  return name;
+  return value_sp->GetQualifiedTypeName().GetCString();
 }
 
 const char *SBValue::GetDisplayTypeName() {
   LLDB_INSTRUMENT_VA(this);
 
-  const char *name = nullptr;
   ValueLocker locker;
   lldb::ValueObjectSP value_sp(GetSP(locker));
-  if (value_sp) {
-name = value_sp->GetDisplayTypeName().GetCString();
-  }
+  if (!value_sp)
+return nullptr;
 
-  return name;
+  return value_sp->GetDisplayTypeName().GetCString();
 }
 
 size_t SBValue::GetByteSize() {
@@ -357,14 +352,11 @@
 const char *SBValue::GetValue() {
   LLDB_INSTRUMENT_VA(this);
 
-  const char *cstr = nullptr;
   ValueLocker locker;
   lldb::ValueObjectSP value_sp(GetSP(locker));
-  if (value_sp) {
-cstr = value_sp->GetValueAsCString();
-  }
-
-  return cstr;
+  if (!value_sp)
+return nullptr;
+  return ConstString(value_sp->GetValueAsCString()).GetCString();
 }
 
 ValueType SBValue::GetValueType() {
@@ -382,14 +374,12 @@
 const char *SBValue::GetObjectDescription() {

[Lldb-commits] [PATCH] D149914: [lldb] Refactor ObjCLanguage::MethodName

2023-05-18 Thread Alex Langford via Phabricator via lldb-commits

bulbazord added inline comments.



Comment at: lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp:155
+  llvm::StringRef selector_name = GetSelector();
+  std::string name_sans_category;
+

aprantl wrote:
> bulbazord wrote:
> > aprantl wrote:
> > > Using an llvm string stream to construct the string might be faster here.
> > I tested this and found that `llvm::SmallString<128>` performs about as 
> > well as `std::string` on average. `llvm::SmallString<64>` was on average 
> > slower though.
> > 
> > I think I'm going to land this with std::string, we can revisit later if 
> > the difference actually is measurable.
> Maybe we were talking past each other: I was thinking about 
> https://www.llvm.org/doxygen/classllvm_1_1raw__string__ostream.html
Oh, I totally misread what you said. I'll try that out and create a new PR if I 
can improve this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149914

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

[Lldb-commits] [PATCH] D149914: [lldb] Refactor ObjCLanguage::MethodName

2023-05-18 Thread Adrian Prantl via Phabricator via lldb-commits

aprantl added inline comments.



Comment at: lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp:155
+  llvm::StringRef selector_name = GetSelector();
+  std::string name_sans_category;
+

bulbazord wrote:
> aprantl wrote:
> > Using an llvm string stream to construct the string might be faster here.
> I tested this and found that `llvm::SmallString<128>` performs about as well 
> as `std::string` on average. `llvm::SmallString<64>` was on average slower 
> though.
> 
> I think I'm going to land this with std::string, we can revisit later if the 
> difference actually is measurable.
Maybe we were talking past each other: I was thinking about 
https://www.llvm.org/doxygen/classllvm_1_1raw__string__ostream.html


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149914

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

[Lldb-commits] [PATCH] D149914: [lldb] Refactor ObjCLanguage::MethodName

2023-05-18 Thread Alex Langford via Phabricator via lldb-commits

This revision was automatically updated to reflect the committed changes.
bulbazord marked an inline comment as done.
Closed by commit rG915256388f86: [lldb] Refactor ObjCLanguage::MethodName 
(authored by bulbazord).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149914

Files:
  lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp
  lldb/source/Plugins/Language/ObjC/ObjCLanguage.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
  lldb/unittests/Language/ObjC/ObjCLanguageTest.cpp

Index: lldb/unittests/Language/ObjC/ObjCLanguageTest.cpp
===
--- lldb/unittests/Language/ObjC/ObjCLanguageTest.cpp
+++ lldb/unittests/Language/ObjC/ObjCLanguageTest.cpp
@@ -42,53 +42,51 @@
 
   // First, be strict
   for (const auto  : strict_cases) {
-ObjCLanguage::MethodName method(test.input, /*strict = */ true);
-EXPECT_TRUE(method.IsValid(/*strict = */ true));
-EXPECT_EQ(
-test.full_name_sans_category,
-method.GetFullNameWithoutCategory(/*empty_if_no_category = */ true)
-.GetStringRef());
-EXPECT_EQ(test.class_name, method.GetClassName().GetStringRef());
+std::optional method =
+ObjCLanguage::MethodName::Create(test.input, /*strict = */ true);
+EXPECT_TRUE(method.has_value());
+EXPECT_EQ(test.full_name_sans_category,
+  method->GetFullNameWithoutCategory());
+EXPECT_EQ(test.class_name, method->GetClassName());
 EXPECT_EQ(test.class_name_with_category,
-  method.GetClassNameWithCategory().GetStringRef());
-EXPECT_EQ(test.category, method.GetCategory().GetStringRef());
-EXPECT_EQ(test.selector, method.GetSelector().GetStringRef());
+  method->GetClassNameWithCategory());
+EXPECT_EQ(test.category, method->GetCategory());
+EXPECT_EQ(test.selector, method->GetSelector());
   }
 
   // We should make sure strict parsing does not accept lax cases
   for (const auto  : lax_cases) {
-ObjCLanguage::MethodName method(test.input, /*strict = */ true);
-EXPECT_FALSE(method.IsValid(/*strict = */ true));
+std::optional method =
+ObjCLanguage::MethodName::Create(test.input, /*strict = */ true);
+EXPECT_FALSE(method.has_value());
   }
 
   // All strict cases should work when not lax
   for (const auto  : strict_cases) {
-ObjCLanguage::MethodName method(test.input, /*strict = */ false);
-EXPECT_TRUE(method.IsValid(/*strict = */ false));
-EXPECT_EQ(
-test.full_name_sans_category,
-method.GetFullNameWithoutCategory(/*empty_if_no_category = */ true)
-.GetStringRef());
-EXPECT_EQ(test.class_name, method.GetClassName().GetStringRef());
+std::optional method =
+ObjCLanguage::MethodName::Create(test.input, /*strict = */ false);
+EXPECT_TRUE(method.has_value());
+EXPECT_EQ(test.full_name_sans_category,
+  method->GetFullNameWithoutCategory());
+EXPECT_EQ(test.class_name, method->GetClassName());
 EXPECT_EQ(test.class_name_with_category,
-  method.GetClassNameWithCategory().GetStringRef());
-EXPECT_EQ(test.category, method.GetCategory().GetStringRef());
-EXPECT_EQ(test.selector, method.GetSelector().GetStringRef());
+  method->GetClassNameWithCategory());
+EXPECT_EQ(test.category, method->GetCategory());
+EXPECT_EQ(test.selector, method->GetSelector());
   }
 
   // Make sure non-strict parsing works
   for (const auto  : lax_cases) {
-ObjCLanguage::MethodName method(test.input, /*strict = */ false);
-EXPECT_TRUE(method.IsValid(/*strict = */ false));
-EXPECT_EQ(
-test.full_name_sans_category,
-method.GetFullNameWithoutCategory(/*empty_if_no_category = */ true)
-.GetStringRef());
-EXPECT_EQ(test.class_name, method.GetClassName().GetStringRef());
+std::optional method =
+ObjCLanguage::MethodName::Create(test.input, /*strict = */ false);
+EXPECT_TRUE(method.has_value());
+EXPECT_EQ(test.full_name_sans_category,
+  method->GetFullNameWithoutCategory());
+EXPECT_EQ(test.class_name, method->GetClassName());
 EXPECT_EQ(test.class_name_with_category,
-  method.GetClassNameWithCategory().GetStringRef());
-EXPECT_EQ(test.category, method.GetCategory().GetStringRef());
-EXPECT_EQ(test.selector, method.GetSelector().GetStringRef());
+  method->GetClassNameWithCategory());
+EXPECT_EQ(test.category, method->GetCategory());
+EXPECT_EQ(test.selector, method->GetSelector());
   }
 }
 
@@ -105,10 +103,12 @@
   "[]"};
 
   for (const auto  : test_cases) {
-ObjCLanguage::MethodName strict_method(name, /*strict = */ true);
-EXPECT_FALSE(strict_method.IsValid(true));
+std::optional strict_method =
+

[Lldb-commits] [lldb] 9152563 - [lldb] Refactor ObjCLanguage::MethodName

2023-05-18 Thread Alex Langford via lldb-commits


Author: Alex Langford
Date: 2023-05-18T14:47:33-07:00
New Revision: 915256388f865a1a42808f168fc388fccff14eda

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

LOG: [lldb] Refactor ObjCLanguage::MethodName

The goal of this patch is to make it easier to reason about the state of
ObjCLanguage::MethodName. I do that in several ways:
- Instead of using the constructor directly, you go through a factory
  method. It returns a std::optional so either you got an
  ObjCLanguage::MethodName or you didn't. No more checking if it's valid
  to know if you can use it or not.
- ObjCLanguage::MethodName is now immutable. You cannot change its
  internals once it is created.
- ObjCLanguage::MethodName::GetFullNameWithoutCategory previously had a
  parameter that let you get back an empty string if the method had no
  category. Every caller of this method was enabling this behavior so I
  dropped the parameter and made it the default behavior.
- No longer store all the various components of the method name as
  ConstStrings. The relevant `Get` methods now return llvm::StringRefs
  backed by the MethodName's internal storage. The lifetime of these
  StringRefs are tied to the MethodName itself, so if you need to
  persist these you need to create copies.

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

Added: 


Modified: 
lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp
lldb/source/Plugins/Language/ObjC/ObjCLanguage.h
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
lldb/unittests/Language/ObjC/ObjCLanguageTest.cpp

Removed: 




diff  --git a/lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp 
b/lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp
index 811654460afb7..fb87d5d80b96c 100644
--- a/lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp
+++ b/lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp
@@ -60,201 +60,152 @@ Language *ObjCLanguage::CreateInstance(lldb::LanguageType 
language) {
   }
 }
 
-void ObjCLanguage::MethodName::Clear() {
-  m_full.Clear();
-  m_class.Clear();
-  m_category.Clear();
-  m_selector.Clear();
-  m_type = eTypeUnspecified;
-  m_category_is_valid = false;
-}
-
-bool ObjCLanguage::MethodName::SetName(llvm::StringRef name, bool strict) {
-  Clear();
+std::optional
+ObjCLanguage::MethodName::Create(llvm::StringRef name, bool strict) {
   if (name.empty())
-return IsValid(strict);
-
-  // If "strict" is true. then the method must be specified with a '+' or '-'
-  // at the beginning. If "strict" is false, then the '+' or '-' can be omitted
-  bool valid_prefix = false;
-
-  if (name.size() > 1 && (name[0] == '+' || name[0] == '-')) {
-valid_prefix = name[1] == '[';
-if (name[0] == '+')
-  m_type = eTypeClassMethod;
-else
-  m_type = eTypeInstanceMethod;
-  } else if (!strict) {
-// "strict" is false, the name just needs to start with '['
-valid_prefix = name[0] == '[';
-  }
-
-  if (valid_prefix) {
-int name_len = name.size();
-// Objective-C methods must have at least:
-//  "-[" or "+[" prefix
-//  One character for a class name
-//  One character for the space between the class name
-//  One character for the method name
-//  "]" suffix
-if (name_len >= (5 + (strict ? 1 : 0)) && name.back() == ']') {
-  m_full.SetString(name);
-}
-  }
-  return IsValid(strict);
+return std::nullopt;
+
+  // Objective-C method minimum requirements:
+  //  - If `strict` is true, must start with '-' or '+' (1 char)
+  //  - Must be followed by '[' (1 char)
+  //  - Must have at least one character for class name (1 char)
+  //  - Must have a space between class name and method name (1 char)
+  //  - Must have at least one character for  method name (1 char)
+  //  - Must be end with ']' (1 char)
+  //  This means that the minimum size is 5 characters (6 if `strict`)
+  //  e.g. [a a] (-[a a] or +[a a] if `strict`)
+
+  // We can check length and ending invariants first
+  if (name.size() < (5 + (strict ? 1 : 0)) || name.back() != ']')
+return std::nullopt;
+
+  // Figure out type
+  Type type = eTypeUnspecified;
+  if (name.startswith("+["))
+type = eTypeClassMethod;
+  else if (name.startswith("-["))
+type = eTypeInstanceMethod;
+
+  // If there's no type and it's strict, this is invalid
+  if (strict && type == eTypeUnspecified)
+return std::nullopt;
+
+  // If not strict and type unspecified, make sure we start with '['
+  if (type == eTypeUnspecified && name.front() != '[')
+return std::nullopt;
+
+  // If we've gotten here, we're confident that this looks enough like an
+  // Objective-C method to treat it like one.
+  ObjCLanguage::MethodName method_name(name, type);
+

[Lldb-commits] [PATCH] D150804: [lldb] Guarantee the lifetimes of all strings returned from SBAPI

2023-05-18 Thread Alex Langford via Phabricator via lldb-commits

bulbazord marked an inline comment as done.
bulbazord added inline comments.



Comment at: lldb/source/API/SBFunction.cpp:181
+
+  return variable_sp->GetName().GetCString();
 }

mib wrote:
> nit: This threw me off, I thought you forgot to create a `ConsString` but it 
> seems that `ConstString` has both a `AsCString` and a `GetCString` method. It 
> would be good to stay consistent.
Yeah, maybe we should try to remove either `GetCString` or `AsCString` and be 
uniform everywhere. Let's plan on doing that later though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150804

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

[Lldb-commits] [PATCH] D149914: [lldb] Refactor ObjCLanguage::MethodName

2023-05-18 Thread Alex Langford via Phabricator via lldb-commits

bulbazord marked an inline comment as done.
bulbazord added inline comments.



Comment at: lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp:155
+  llvm::StringRef selector_name = GetSelector();
+  std::string name_sans_category;
+

aprantl wrote:
> Using an llvm string stream to construct the string might be faster here.
I tested this and found that `llvm::SmallString<128>` performs about as well as 
`std::string` on average. `llvm::SmallString<64>` was on average slower though.

I think I'm going to land this with std::string, we can revisit later if the 
difference actually is measurable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149914

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

[Lldb-commits] [PATCH] D150804: [lldb] Guarantee the lifetimes of all strings returned from SBAPI

2023-05-18 Thread Med Ismail Bennani via Phabricator via lldb-commits

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

LGTM with nit.




Comment at: lldb/source/API/SBFunction.cpp:181
+
+  return variable_sp->GetName().GetCString();
 }

nit: This threw me off, I thought you forgot to create a `ConsString` but it 
seems that `ConstString` has both a `AsCString` and a `GetCString` method. It 
would be good to stay consistent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150804

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

[Lldb-commits] [PATCH] D150716: [lldb][NFCI] Switch to using llvm::DWARFAbbreviationDeclaration

2023-05-18 Thread Alex Langford via Phabricator via lldb-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rG16eb14806d1e: [lldb][NFCI] Switch to using 
llvm::DWARFAbbreviationDeclaration (authored by bulbazord).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150716

Files:
  lldb/source/Plugins/SymbolFile/DWARF/CMakeLists.txt
  lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDefines.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
  lldb/unittests/SymbolFile/DWARF/SymbolFileDWARFTests.cpp

Index: lldb/unittests/SymbolFile/DWARF/SymbolFileDWARFTests.cpp
===
--- lldb/unittests/SymbolFile/DWARF/SymbolFileDWARFTests.cpp
+++ lldb/unittests/SymbolFile/DWARF/SymbolFileDWARFTests.cpp
@@ -15,7 +15,6 @@
 #include "llvm/Support/Path.h"
 
 #include "Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h"
-#include "Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.h"
 #include "Plugins/SymbolFile/DWARF/DWARFDataExtractor.h"
 #include "Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.h"
 #include "Plugins/SymbolFile/DWARF/DWARFDebugArangeSet.h"
@@ -105,13 +104,13 @@
   EXPECT_EQ(abbrev_set.GetIndexOffset(), 1u);
 
   auto abbrev1 = abbrev_set.GetAbbreviationDeclaration(1);
-  EXPECT_EQ(abbrev1->Tag(), DW_TAG_compile_unit);
-  EXPECT_TRUE(abbrev1->HasChildren());
-  EXPECT_EQ(abbrev1->NumAttributes(), 1u);
+  EXPECT_EQ(abbrev1->getTag(), DW_TAG_compile_unit);
+  EXPECT_TRUE(abbrev1->hasChildren());
+  EXPECT_EQ(abbrev1->getNumAttributes(), 1u);
   auto abbrev2 = abbrev_set.GetAbbreviationDeclaration(2);
-  EXPECT_EQ(abbrev2->Tag(), DW_TAG_subprogram);
-  EXPECT_FALSE(abbrev2->HasChildren());
-  EXPECT_EQ(abbrev2->NumAttributes(), 1u);
+  EXPECT_EQ(abbrev2->getTag(), DW_TAG_subprogram);
+  EXPECT_FALSE(abbrev2->hasChildren());
+  EXPECT_EQ(abbrev2->getNumAttributes(), 1u);
 }
 
 TEST_F(SymbolFileDWARFTests, TestAbbrevOrder1Start5) {
@@ -150,13 +149,13 @@
   EXPECT_EQ(abbrev_set.GetIndexOffset(), 5u);
 
   auto abbrev1 = abbrev_set.GetAbbreviationDeclaration(5);
-  EXPECT_EQ(abbrev1->Tag(), DW_TAG_compile_unit);
-  EXPECT_TRUE(abbrev1->HasChildren());
-  EXPECT_EQ(abbrev1->NumAttributes(), 1u);
+  EXPECT_EQ(abbrev1->getTag(), DW_TAG_compile_unit);
+  EXPECT_TRUE(abbrev1->hasChildren());
+  EXPECT_EQ(abbrev1->getNumAttributes(), 1u);
   auto abbrev2 = abbrev_set.GetAbbreviationDeclaration(6);
-  EXPECT_EQ(abbrev2->Tag(), DW_TAG_subprogram);
-  EXPECT_FALSE(abbrev2->HasChildren());
-  EXPECT_EQ(abbrev2->NumAttributes(), 1u);
+  EXPECT_EQ(abbrev2->getTag(), DW_TAG_subprogram);
+  EXPECT_FALSE(abbrev2->hasChildren());
+  EXPECT_EQ(abbrev2->getNumAttributes(), 1u);
 }
 
 TEST_F(SymbolFileDWARFTests, TestAbbrevOutOfOrder) {
@@ -195,13 +194,13 @@
   EXPECT_EQ(abbrev_set.GetIndexOffset(), UINT32_MAX);
 
   auto abbrev1 = abbrev_set.GetAbbreviationDeclaration(2);
-  EXPECT_EQ(abbrev1->Tag(), DW_TAG_compile_unit);
-  EXPECT_TRUE(abbrev1->HasChildren());
-  EXPECT_EQ(abbrev1->NumAttributes(), 1u);
+  EXPECT_EQ(abbrev1->getTag(), DW_TAG_compile_unit);
+  EXPECT_TRUE(abbrev1->hasChildren());
+  EXPECT_EQ(abbrev1->getNumAttributes(), 1u);
   auto abbrev2 = abbrev_set.GetAbbreviationDeclaration(1);
-  EXPECT_EQ(abbrev2->Tag(), DW_TAG_subprogram);
-  EXPECT_FALSE(abbrev2->HasChildren());
-  EXPECT_EQ(abbrev2->NumAttributes(), 1u);
+  EXPECT_EQ(abbrev2->getTag(), DW_TAG_subprogram);
+  EXPECT_FALSE(abbrev2->hasChildren());
+  EXPECT_EQ(abbrev2->getNumAttributes(), 1u);
 }
 
 TEST_F(SymbolFileDWARFTests, TestAbbrevInvalidNULLTag) {
@@ -226,9 +225,8 @@
   llvm::Error error = abbrev_set.extract(data, _offset);
   // Verify we get an error
   EXPECT_TRUE(bool(error));
-  EXPECT_EQ("abbrev decl requires non-null tag.",
+  EXPECT_EQ("abbreviation declaration requires a non-null tag",
 llvm::toString(std::move(error)));
-
 }
 
 TEST_F(SymbolFileDWARFTests, TestAbbrevNullAttrValidForm) {
@@ -255,7 +253,8 @@
   llvm::Error error = abbrev_set.extract(data, _offset);
   // Verify we get an error
   EXPECT_TRUE(bool(error));
-  EXPECT_EQ("malformed abbreviation declaration attribute",
+  EXPECT_EQ("malformed abbreviation declaration attribute. Either the "
+"attribute or the form is zero while the other is not",
 llvm::toString(std::move(error)));
 }
 
@@ -283,7 +282,8 @@
   llvm::Error error = abbrev_set.extract(data, _offset);
   // Verify we get an error
   EXPECT_TRUE(bool(error));
-  EXPECT_EQ("malformed abbreviation declaration attribute",
+  EXPECT_EQ("malformed abbreviation declaration attribute. Either

[Lldb-commits] [lldb] 16eb148 - [lldb][NFCI] Switch to using llvm::DWARFAbbreviationDeclaration

2023-05-18 Thread Alex Langford via lldb-commits


Author: Alex Langford
Date: 2023-05-18T14:26:19-07:00
New Revision: 16eb14806d1ee88d9f0c6222c0d0953a208f7911

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

LOG: [lldb][NFCI] Switch to using llvm::DWARFAbbreviationDeclaration

Both LLVM and LLDB implement DWARFAbbreviationDeclaration. As of
631ff46cbf51, llvm's implementation of
DWARFAbbreviationDeclaration::extract behaves the same as LLDB's
implementation, making it easier to merge the implementations.

The only major difference between LLDB's implementation and LLVM's
implementation is that LLVM's DWARFAbbreviationDeclaration is slightly
larger. Specifically, it has some metadata that keeps track of the size
of a declaration (if it has a fixed size) so that it can potentially
optimize extraction in some scenarios. I think this increase in size
should be acceptable and possibly useful on the LLDB side.

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

Added: 


Modified: 
lldb/source/Plugins/SymbolFile/DWARF/CMakeLists.txt
lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp
lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.h
lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
lldb/source/Plugins/SymbolFile/DWARF/DWARFDefines.h
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
lldb/unittests/SymbolFile/DWARF/SymbolFileDWARFTests.cpp

Removed: 
lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.cpp
lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.h



diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/CMakeLists.txt 
b/lldb/source/Plugins/SymbolFile/DWARF/CMakeLists.txt
index 71fc850127331..5eb2df9e7c15d 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/CMakeLists.txt
+++ b/lldb/source/Plugins/SymbolFile/DWARF/CMakeLists.txt
@@ -10,7 +10,6 @@ add_lldb_library(lldbPluginSymbolFileDWARF PLUGIN
   AppleDWARFIndex.cpp
   DebugNamesDWARFIndex.cpp
   DIERef.cpp
-  DWARFAbbreviationDeclaration.cpp
   DWARFASTParser.cpp
   DWARFASTParserClang.cpp
   DWARFAttribute.cpp

diff  --git 
a/lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.cpp
deleted file mode 100644
index f88a604fd9169..0
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.cpp
+++ /dev/null
@@ -1,81 +0,0 @@
-//===-- DWARFAbbreviationDeclaration.cpp 
--===//
-//
-// 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 "DWARFAbbreviationDeclaration.h"
-
-#include "lldb/Core/dwarf.h"
-#include "lldb/Utility/Stream.h"
-
-#include "llvm/Object/Error.h"
-
-#include "DWARFFormValue.h"
-
-using namespace lldb_private;
-using namespace lldb_private::dwarf;
-
-DWARFAbbreviationDeclaration::DWARFAbbreviationDeclaration() : m_attributes() 
{}
-
-DWARFAbbreviationDeclaration::DWARFAbbreviationDeclaration(dw_tag_t tag,
-   uint8_t 
has_children)
-: m_tag(tag), m_has_children(has_children), m_attributes() {}
-
-llvm::Expected
-DWARFAbbreviationDeclaration::extract(const DWARFDataExtractor ,
-  lldb::offset_t *offset_ptr) {
-  m_code = data.GetULEB128(offset_ptr);
-  if (m_code == 0)
-return DWARFEnumState::Complete;
-
-  m_attributes.clear();
-  m_tag = static_cast(data.GetULEB128(offset_ptr));
-  if (m_tag == DW_TAG_null)
-return llvm::make_error(
-"abbrev decl requires non-null tag.");
-
-  m_has_children = data.GetU8(offset_ptr);
-
-  while (data.ValidOffset(*offset_ptr)) {
-auto attr = static_cast(data.GetULEB128(offset_ptr));
-auto form = static_cast(data.GetULEB128(offset_ptr));
-
-// This is the last attribute for this abbrev decl, but there may still be
-// more abbrev decls, so return MoreItems to indicate to the caller that
-// they should call this function again.
-if (!attr && !form)
-  return DWARFEnumState::MoreItems;
-
-if (!attr || !form)
-  return llvm::make_error(
-  "malformed abbreviation declaration attribute");
-
-if (form == DW_FORM_implicit_const) {
-  int64_t value = data.GetSLEB128(offset_ptr);
-  m_attributes.emplace_back(attr, form, value);
-  continue;
-}
-
-m_attributes.emplace_back(attr, form);
-  }
-
-  return llvm::make_error(
-  "abbreviation declaration attribute list not terminated with a null "
-  "entry");
-}
-
-bool

[Lldb-commits] [PATCH] D150716: [lldb][NFCI] Switch to using llvm::DWARFAbbreviationDeclaration

2023-05-18 Thread Adrian Prantl via Phabricator via lldb-commits

aprantl added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp:56
   if (m_idx_offset == UINT32_MAX) {
-DWARFAbbreviationDeclarationCollConstIter pos;
-DWARFAbbreviationDeclarationCollConstIter end = m_decls.end();
-for (pos = m_decls.begin(); pos != end; ++pos) {
-  if (pos->Code() == abbrCode)
-return &(*pos);
+for (const auto  : m_decls) {
+  if (decl.getCode() == abbrCode)

bulbazord wrote:
> aprantl wrote:
> > would std::find_if be shorter or would it look worse?
> ```
> for (const auto  : m_decls) {
>   if (decl.getCode() == abbrCode)
> return 
> }
> ```
> vs.
> ```
> auto pos = std::find_if(
>  m_decls.begin(), m_decls.end(),
>  [abbrCode](const auto ) { return decl.getCode() == abbrCode; });
> if (pos != m_decls.end())
>   return &*pos;
> ```
> 
> I think it would look worse, personally.
Agreed!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150716

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

[Lldb-commits] [PATCH] D150716: [lldb][NFCI] Switch to using llvm::DWARFAbbreviationDeclaration

2023-05-18 Thread Alex Langford via Phabricator via lldb-commits

bulbazord updated this revision to Diff 523500.
bulbazord marked 2 inline comments as done.
bulbazord added a comment.

Redo include order in DWARFDebugAbbrev.h to match project guidelines


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150716

Files:
  lldb/source/Plugins/SymbolFile/DWARF/CMakeLists.txt
  lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDefines.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
  lldb/unittests/SymbolFile/DWARF/SymbolFileDWARFTests.cpp

Index: lldb/unittests/SymbolFile/DWARF/SymbolFileDWARFTests.cpp
===
--- lldb/unittests/SymbolFile/DWARF/SymbolFileDWARFTests.cpp
+++ lldb/unittests/SymbolFile/DWARF/SymbolFileDWARFTests.cpp
@@ -15,7 +15,6 @@
 #include "llvm/Support/Path.h"
 
 #include "Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h"
-#include "Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.h"
 #include "Plugins/SymbolFile/DWARF/DWARFDataExtractor.h"
 #include "Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.h"
 #include "Plugins/SymbolFile/DWARF/DWARFDebugArangeSet.h"
@@ -105,13 +104,13 @@
   EXPECT_EQ(abbrev_set.GetIndexOffset(), 1u);
 
   auto abbrev1 = abbrev_set.GetAbbreviationDeclaration(1);
-  EXPECT_EQ(abbrev1->Tag(), DW_TAG_compile_unit);
-  EXPECT_TRUE(abbrev1->HasChildren());
-  EXPECT_EQ(abbrev1->NumAttributes(), 1u);
+  EXPECT_EQ(abbrev1->getTag(), DW_TAG_compile_unit);
+  EXPECT_TRUE(abbrev1->hasChildren());
+  EXPECT_EQ(abbrev1->getNumAttributes(), 1u);
   auto abbrev2 = abbrev_set.GetAbbreviationDeclaration(2);
-  EXPECT_EQ(abbrev2->Tag(), DW_TAG_subprogram);
-  EXPECT_FALSE(abbrev2->HasChildren());
-  EXPECT_EQ(abbrev2->NumAttributes(), 1u);
+  EXPECT_EQ(abbrev2->getTag(), DW_TAG_subprogram);
+  EXPECT_FALSE(abbrev2->hasChildren());
+  EXPECT_EQ(abbrev2->getNumAttributes(), 1u);
 }
 
 TEST_F(SymbolFileDWARFTests, TestAbbrevOrder1Start5) {
@@ -150,13 +149,13 @@
   EXPECT_EQ(abbrev_set.GetIndexOffset(), 5u);
 
   auto abbrev1 = abbrev_set.GetAbbreviationDeclaration(5);
-  EXPECT_EQ(abbrev1->Tag(), DW_TAG_compile_unit);
-  EXPECT_TRUE(abbrev1->HasChildren());
-  EXPECT_EQ(abbrev1->NumAttributes(), 1u);
+  EXPECT_EQ(abbrev1->getTag(), DW_TAG_compile_unit);
+  EXPECT_TRUE(abbrev1->hasChildren());
+  EXPECT_EQ(abbrev1->getNumAttributes(), 1u);
   auto abbrev2 = abbrev_set.GetAbbreviationDeclaration(6);
-  EXPECT_EQ(abbrev2->Tag(), DW_TAG_subprogram);
-  EXPECT_FALSE(abbrev2->HasChildren());
-  EXPECT_EQ(abbrev2->NumAttributes(), 1u);
+  EXPECT_EQ(abbrev2->getTag(), DW_TAG_subprogram);
+  EXPECT_FALSE(abbrev2->hasChildren());
+  EXPECT_EQ(abbrev2->getNumAttributes(), 1u);
 }
 
 TEST_F(SymbolFileDWARFTests, TestAbbrevOutOfOrder) {
@@ -195,13 +194,13 @@
   EXPECT_EQ(abbrev_set.GetIndexOffset(), UINT32_MAX);
 
   auto abbrev1 = abbrev_set.GetAbbreviationDeclaration(2);
-  EXPECT_EQ(abbrev1->Tag(), DW_TAG_compile_unit);
-  EXPECT_TRUE(abbrev1->HasChildren());
-  EXPECT_EQ(abbrev1->NumAttributes(), 1u);
+  EXPECT_EQ(abbrev1->getTag(), DW_TAG_compile_unit);
+  EXPECT_TRUE(abbrev1->hasChildren());
+  EXPECT_EQ(abbrev1->getNumAttributes(), 1u);
   auto abbrev2 = abbrev_set.GetAbbreviationDeclaration(1);
-  EXPECT_EQ(abbrev2->Tag(), DW_TAG_subprogram);
-  EXPECT_FALSE(abbrev2->HasChildren());
-  EXPECT_EQ(abbrev2->NumAttributes(), 1u);
+  EXPECT_EQ(abbrev2->getTag(), DW_TAG_subprogram);
+  EXPECT_FALSE(abbrev2->hasChildren());
+  EXPECT_EQ(abbrev2->getNumAttributes(), 1u);
 }
 
 TEST_F(SymbolFileDWARFTests, TestAbbrevInvalidNULLTag) {
@@ -226,9 +225,8 @@
   llvm::Error error = abbrev_set.extract(data, _offset);
   // Verify we get an error
   EXPECT_TRUE(bool(error));
-  EXPECT_EQ("abbrev decl requires non-null tag.",
+  EXPECT_EQ("abbreviation declaration requires a non-null tag",
 llvm::toString(std::move(error)));
-
 }
 
 TEST_F(SymbolFileDWARFTests, TestAbbrevNullAttrValidForm) {
@@ -255,7 +253,8 @@
   llvm::Error error = abbrev_set.extract(data, _offset);
   // Verify we get an error
   EXPECT_TRUE(bool(error));
-  EXPECT_EQ("malformed abbreviation declaration attribute",
+  EXPECT_EQ("malformed abbreviation declaration attribute. Either the "
+"attribute or the form is zero while the other is not",
 llvm::toString(std::move(error)));
 }
 
@@ -283,7 +282,8 @@
   llvm::Error error = abbrev_set.extract(data, _offset);
   // Verify we get an error
   EXPECT_TRUE(bool(error));
-  EXPECT_EQ("malformed abbreviation declaration attribute",
+  EXPECT_EQ("malformed abbreviation declaration attribute. Either the "
+

[Lldb-commits] [PATCH] D150716: [lldb][NFCI] Switch to using llvm::DWARFAbbreviationDeclaration

2023-05-18 Thread Alex Langford via Phabricator via lldb-commits

bulbazord marked 2 inline comments as done.
bulbazord added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp:27
  lldb::offset_t *offset_ptr) {
+  llvm::DataExtractor llvm_data = data.GetAsLLVM();
   const lldb::offset_t begin_offset = *offset_ptr;

aprantl wrote:
> Is this intentionally a copy or should this be a reference? I have no idea 
> how heavyweight this object is...
This isn't exactly a copy. `DataExtractor::GetAsLLVM` creates an entirely new 
object, it can't be a reference. Luckily it is fairly lightweight -- We just 
copy a few numbers and a pointer I believe.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp:56
   if (m_idx_offset == UINT32_MAX) {
-DWARFAbbreviationDeclarationCollConstIter pos;
-DWARFAbbreviationDeclarationCollConstIter end = m_decls.end();
-for (pos = m_decls.begin(); pos != end; ++pos) {
-  if (pos->Code() == abbrCode)
-return &(*pos);
+for (const auto  : m_decls) {
+  if (decl.getCode() == abbrCode)

aprantl wrote:
> would std::find_if be shorter or would it look worse?
```
for (const auto  : m_decls) {
  if (decl.getCode() == abbrCode)
return 
}
```
vs.
```
auto pos = std::find_if(
 m_decls.begin(), m_decls.end(),
 [abbrCode](const auto ) { return decl.getCode() == abbrCode; });
if (pos != m_decls.end())
  return &*pos;
```

I think it would look worse, personally.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150716

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

[Lldb-commits] [PATCH] D150826: [lldb] Implement GetValueTypeFromAddressType

2023-05-18 Thread Augusto Noronha via Phabricator via lldb-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rG8fe9718dd5f2: [lldb] Implement GetValueTypeFromAddressType 
(authored by augusto2112).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150826

Files:
  lldb/include/lldb/Core/Value.h
  lldb/source/Core/Value.cpp


Index: lldb/source/Core/Value.cpp
===
--- lldb/source/Core/Value.cpp
+++ lldb/source/Core/Value.cpp
@@ -121,6 +121,20 @@
   return eAddressTypeInvalid;
 }
 
+Value::ValueType Value::GetValueTypeFromAddressType(AddressType address_type) {
+  switch (address_type) {
+case eAddressTypeFile:
+  return Value::ValueType::FileAddress;
+case eAddressTypeLoad:
+  return Value::ValueType::LoadAddress;
+case eAddressTypeHost:
+  return Value::ValueType::HostAddress;
+case eAddressTypeInvalid:
+  return Value::ValueType::Invalid;
+  }
+  llvm_unreachable("Unexpected address type!");
+}
+
 RegisterInfo *Value::GetRegisterInfo() const {
   if (m_context_type == ContextType::RegisterInfo)
 return static_cast(m_context);
Index: lldb/include/lldb/Core/Value.h
===
--- lldb/include/lldb/Core/Value.h
+++ lldb/include/lldb/Core/Value.h
@@ -145,6 +145,8 @@
 
   void Clear();
 
+  static ValueType GetValueTypeFromAddressType(AddressType address_type);
+
 protected:
   Scalar m_value;
   CompilerType m_compiler_type;


Index: lldb/source/Core/Value.cpp
===
--- lldb/source/Core/Value.cpp
+++ lldb/source/Core/Value.cpp
@@ -121,6 +121,20 @@
   return eAddressTypeInvalid;
 }
 
+Value::ValueType Value::GetValueTypeFromAddressType(AddressType address_type) {
+  switch (address_type) {
+case eAddressTypeFile:
+  return Value::ValueType::FileAddress;
+case eAddressTypeLoad:
+  return Value::ValueType::LoadAddress;
+case eAddressTypeHost:
+  return Value::ValueType::HostAddress;
+case eAddressTypeInvalid:
+  return Value::ValueType::Invalid;
+  }
+  llvm_unreachable("Unexpected address type!");
+}
+
 RegisterInfo *Value::GetRegisterInfo() const {
   if (m_context_type == ContextType::RegisterInfo)
 return static_cast(m_context);
Index: lldb/include/lldb/Core/Value.h
===
--- lldb/include/lldb/Core/Value.h
+++ lldb/include/lldb/Core/Value.h
@@ -145,6 +145,8 @@
 
   void Clear();
 
+  static ValueType GetValueTypeFromAddressType(AddressType address_type);
+
 protected:
   Scalar m_value;
   CompilerType m_compiler_type;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 8fe9718 - [lldb] Implement GetValueTypeFromAddressType

2023-05-18 Thread Augusto Noronha via lldb-commits


Author: Augusto Noronha
Date: 2023-05-18T10:29:15-07:00
New Revision: 8fe9718dd5f27168fc282c6420bfae0eb7ee6819

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

LOG: [lldb] Implement GetValueTypeFromAddressType

Value::ValueType is a superset of AddressType. Add a function to
convert an AddressType into a Value::ValueType.

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

Added: 


Modified: 
lldb/include/lldb/Core/Value.h
lldb/source/Core/Value.cpp

Removed: 




diff  --git a/lldb/include/lldb/Core/Value.h b/lldb/include/lldb/Core/Value.h
index fdda9f1db491a..ead23acc6f9b1 100644
--- a/lldb/include/lldb/Core/Value.h
+++ b/lldb/include/lldb/Core/Value.h
@@ -145,6 +145,8 @@ class Value {
 
   void Clear();
 
+  static ValueType GetValueTypeFromAddressType(AddressType address_type);
+
 protected:
   Scalar m_value;
   CompilerType m_compiler_type;

diff  --git a/lldb/source/Core/Value.cpp b/lldb/source/Core/Value.cpp
index ccd36096b5456..5a2631ca501f6 100644
--- a/lldb/source/Core/Value.cpp
+++ b/lldb/source/Core/Value.cpp
@@ -121,6 +121,20 @@ AddressType Value::GetValueAddressType() const {
   return eAddressTypeInvalid;
 }
 
+Value::ValueType Value::GetValueTypeFromAddressType(AddressType address_type) {
+  switch (address_type) {
+case eAddressTypeFile:
+  return Value::ValueType::FileAddress;
+case eAddressTypeLoad:
+  return Value::ValueType::LoadAddress;
+case eAddressTypeHost:
+  return Value::ValueType::HostAddress;
+case eAddressTypeInvalid:
+  return Value::ValueType::Invalid;
+  }
+  llvm_unreachable("Unexpected address type!");
+}
+
 RegisterInfo *Value::GetRegisterInfo() const {
   if (m_context_type == ContextType::RegisterInfo)
 return static_cast(m_context);



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

[Lldb-commits] [PATCH] D150619: [lldb] Delay removal of persistent results

2023-05-18 Thread Dave Lee via Phabricator via lldb-commits

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rGdb814552132d: [lldb] Delay removal of persistent results 
(authored by kastiglione).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150619

Files:
  lldb/source/Commands/CommandObjectDWIMPrint.cpp
  lldb/source/Commands/CommandObjectExpression.cpp
  lldb/source/Commands/CommandObjectExpression.h

Index: lldb/source/Commands/CommandObjectExpression.h
===
--- lldb/source/Commands/CommandObjectExpression.h
+++ lldb/source/Commands/CommandObjectExpression.h
@@ -41,6 +41,9 @@
 const Target ,
 const OptionGroupValueObjectDisplay _opts);
 
+bool ShouldSuppressResult(
+const OptionGroupValueObjectDisplay _opts) const;
+
 bool top_level;
 bool unwind_on_error;
 bool ignore_breakpoints;
Index: lldb/source/Commands/CommandObjectExpression.cpp
===
--- lldb/source/Commands/CommandObjectExpression.cpp
+++ lldb/source/Commands/CommandObjectExpression.cpp
@@ -10,6 +10,7 @@
 
 #include "CommandObjectExpression.h"
 #include "lldb/Core/Debugger.h"
+#include "lldb/Expression/ExpressionVariable.h"
 #include "lldb/Expression/REPL.h"
 #include "lldb/Expression/UserExpression.h"
 #include "lldb/Host/OptionParser.h"
@@ -21,6 +22,7 @@
 #include "lldb/Target/Process.h"
 #include "lldb/Target/StackFrame.h"
 #include "lldb/Target/Target.h"
+#include "lldb/lldb-enumerations.h"
 #include "lldb/lldb-private-enumerations.h"
 
 using namespace lldb;
@@ -200,13 +202,6 @@
 const Target , const OptionGroupValueObjectDisplay _opts) {
   EvaluateExpressionOptions options;
   options.SetCoerceToId(display_opts.use_objc);
-  // Explicitly disabling persistent results takes precedence over the
-  // m_verbosity/use_objc logic.
-  if (suppress_persistent_result != eLazyBoolCalculate)
-options.SetSuppressPersistentResult(suppress_persistent_result ==
-eLazyBoolYes);
-  else if (m_verbosity == eLanguageRuntimeDescriptionDisplayVerbosityCompact)
-options.SetSuppressPersistentResult(display_opts.use_objc);
   options.SetUnwindOnError(unwind_on_error);
   options.SetIgnoreBreakpoints(ignore_breakpoints);
   options.SetKeepInMemory(true);
@@ -242,6 +237,17 @@
   return options;
 }
 
+bool CommandObjectExpression::CommandOptions::ShouldSuppressResult(
+const OptionGroupValueObjectDisplay _opts) const {
+  // Explicitly disabling persistent results takes precedence over the
+  // m_verbosity/use_objc logic.
+  if (suppress_persistent_result != eLazyBoolCalculate)
+return suppress_persistent_result == eLazyBoolYes;
+
+  return display_opts.use_objc &&
+ m_verbosity == eLanguageRuntimeDescriptionDisplayVerbosityCompact;
+}
+
 CommandObjectExpression::CommandObjectExpression(
 CommandInterpreter )
 : CommandObjectRaw(interpreter, "expression",
@@ -424,8 +430,12 @@
 return false;
   }
 
-  const EvaluateExpressionOptions eval_options =
+  EvaluateExpressionOptions eval_options =
   m_command_options.GetEvaluateExpressionOptions(target, m_varobj_options);
+  // This command manually removes the result variable, make sure expression
+  // evaluation doesn't do it first.
+  eval_options.SetSuppressPersistentResult(false);
+
   ExpressionResults success = target.EvaluateExpression(
   expr, frame, result_valobj_sp, eval_options, _fixed_expression);
 
@@ -454,14 +464,25 @@
   }
 }
 
+bool suppress_result =
+m_command_options.ShouldSuppressResult(m_varobj_options);
+
 DumpValueObjectOptions options(m_varobj_options.GetAsDumpOptions(
 m_command_options.m_verbosity, format));
-options.SetHideRootName(eval_options.GetSuppressPersistentResult());
+options.SetHideRootName(suppress_result);
 options.SetVariableFormatDisplayLanguage(
 result_valobj_sp->GetPreferredDisplayLanguage());
 
 result_valobj_sp->Dump(output_stream, options);
 
+if (suppress_result)
+  if (auto result_var_sp =
+  target.GetPersistentVariable(result_valobj_sp->GetName())) {
+auto language = result_valobj_sp->GetPreferredDisplayLanguage();
+if (auto *persistent_state =
+target.GetPersistentExpressionStateForLanguage(language))
+  persistent_state->RemovePersistentVariable(result_var_sp);
+  }
 result.SetStatus(eReturnStatusSuccessFinishResult);
   }
 } else {
Index: lldb/source/Commands/CommandObjectDWIMPrint.cpp
===
--- lldb/source/Commands/CommandObjectDWIMPrint.cpp
+++ lldb/source/Commands/CommandObjectDWIMPrint.cpp

[Lldb-commits] [lldb] db81455 - [lldb] Delay removal of persistent results

2023-05-18 Thread Dave Lee via lldb-commits


Author: Dave Lee
Date: 2023-05-18T09:44:11-07:00
New Revision: db814552132d614e410118e22b22c89d35ae6062

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

LOG: [lldb] Delay removal of persistent results

Follow up to "Suppress persistent result when running po" (D144044).

This change delays removal of the persistent result until after `Dump` has been 
called.
In doing so, the persistent result is available for the purpose of getting its 
object
description.

In the original change, the persistent result removal happens indirectly, by 
setting
`EvaluateExpressionOptions::SetSuppressPersistentResult`. In practice this has 
worked,
however this exposed a latent bug in swift-lldb. The subtlety, and the bug, 
depend on
when the persisteted result variable is removed.

When the result is removed via `SetSuppressPersistentResult`, it happens within 
the call
to `Target::EvaluateExpression`. That is, by the time the call returns, the 
persistent
result is already removed.

The issue occurs shortly thereafter, when `ValueObject::Dump` is called, it 
cannot make
use of the persistent result variable (instead it uses the 
`ValueObjectConstResult`). In
swift-lldb, this causes an additional expression evaluation to happen. It first 
tries an
expression that reference `$R0` etc, but that always fails because `$R0` is 
removed. The
fallback to this failure does work most of the time, but there's at least one 
bug
involving imported Clang types.

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

Added: 


Modified: 
lldb/source/Commands/CommandObjectDWIMPrint.cpp
lldb/source/Commands/CommandObjectExpression.cpp
lldb/source/Commands/CommandObjectExpression.h

Removed: 




diff  --git a/lldb/source/Commands/CommandObjectDWIMPrint.cpp 
b/lldb/source/Commands/CommandObjectDWIMPrint.cpp
index 0b5dde9ccaa67..8fc702a1a220e 100644
--- a/lldb/source/Commands/CommandObjectDWIMPrint.cpp
+++ b/lldb/source/Commands/CommandObjectDWIMPrint.cpp
@@ -10,6 +10,7 @@
 
 #include "lldb/Core/ValueObject.h"
 #include "lldb/DataFormatters/DumpValueObjectOptions.h"
+#include "lldb/Expression/ExpressionVariable.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Interpreter/CommandObject.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
@@ -76,6 +77,7 @@ bool CommandObjectDWIMPrint::DoExecute(StringRef command,
   // If the user has not specified, default to disabling persistent results.
   if (m_expr_options.suppress_persistent_result == eLazyBoolCalculate)
 m_expr_options.suppress_persistent_result = eLazyBoolYes;
+  bool suppress_result = m_expr_options.ShouldSuppressResult(m_varobj_options);
 
   auto verbosity = GetDebugger().GetDWIMPrintVerbosity();
 
@@ -83,18 +85,23 @@ bool CommandObjectDWIMPrint::DoExecute(StringRef command,
   // Fallback to the dummy target, which can allow for expression evaluation.
   Target  = target_ptr ? *target_ptr : GetDummyTarget();
 
-  const EvaluateExpressionOptions eval_options =
+  EvaluateExpressionOptions eval_options =
   m_expr_options.GetEvaluateExpressionOptions(target, m_varobj_options);
+  // This command manually removes the result variable, make sure expression
+  // evaluation doesn't do it first.
+  eval_options.SetSuppressPersistentResult(false);
 
   DumpValueObjectOptions dump_options = m_varobj_options.GetAsDumpOptions(
   m_expr_options.m_verbosity, m_format_options.GetFormat());
-  dump_options.SetHideRootName(eval_options.GetSuppressPersistentResult());
+  dump_options.SetHideRootName(suppress_result);
+
+  StackFrame *frame = m_exe_ctx.GetFramePtr();
 
   // First, try `expr` as the name of a frame variable.
-  if (StackFrame *frame = m_exe_ctx.GetFramePtr()) {
+  if (frame) {
 auto valobj_sp = frame->FindVariable(ConstString(expr));
 if (valobj_sp && valobj_sp->GetError().Success()) {
-  if (!eval_options.GetSuppressPersistentResult()) {
+  if (!suppress_result) {
 if (auto persisted_valobj = valobj_sp->Persist())
   valobj_sp = persisted_valobj;
   }
@@ -129,6 +136,16 @@ bool CommandObjectDWIMPrint::DoExecute(StringRef command,
   }
 
   valobj_sp->Dump(result.GetOutputStream(), dump_options);
+
+  if (suppress_result)
+if (auto result_var_sp =
+target.GetPersistentVariable(valobj_sp->GetName())) {
+  auto language = valobj_sp->GetPreferredDisplayLanguage();
+  if (auto *persistent_state =
+  target.GetPersistentExpressionStateForLanguage(language))
+persistent_state->RemovePersistentVariable(result_var_sp);
+}
+
   result.SetStatus(eReturnStatusSuccessFinishResult);
   return true;
 } else {

diff  --git a/lldb/source/Commands/CommandObjectExpression.cpp

[Lldb-commits] [PATCH] D150716: [lldb][NFCI] Switch to using llvm::DWARFAbbreviationDeclaration

2023-05-18 Thread Adrian Prantl via Phabricator via lldb-commits

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

Overall seems good to me. Minor comments inline.




Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp:27
  lldb::offset_t *offset_ptr) {
+  llvm::DataExtractor llvm_data = data.GetAsLLVM();
   const lldb::offset_t begin_offset = *offset_ptr;

Is this intentionally a copy or should this be a reference? I have no idea how 
heavyweight this object is...



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp:56
   if (m_idx_offset == UINT32_MAX) {
-DWARFAbbreviationDeclarationCollConstIter pos;
-DWARFAbbreviationDeclarationCollConstIter end = m_decls.end();
-for (pos = m_decls.begin(); pos != end; ++pos) {
-  if (pos->Code() == abbrCode)
-return &(*pos);
+for (const auto  : m_decls) {
+  if (decl.getCode() == abbrCode)

would std::find_if be shorter or would it look worse?



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.h:18
 
+#include "llvm/DebugInfo/DWARF/DWARFAbbreviationDeclaration.h"
+

Nit: we usually do local includes first and then the stdlib at the bottom.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150716

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

[Lldb-commits] [PATCH] D149914: [lldb] Refactor ObjCLanguage::MethodName

2023-05-18 Thread Adrian Prantl via Phabricator via lldb-commits

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

One more comment inline but otherwise this is fine with me.




Comment at: lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp:155
+  llvm::StringRef selector_name = GetSelector();
+  std::string name_sans_category;
+

Using an llvm string stream to construct the string might be faster here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149914

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

[Lldb-commits] [PATCH] D150928: Two bug fixes for loading process save-core created core files, plus perf improvements

[Lldb-commits] [PATCH] D141907: [CMake] Ensure `CLANG_RESOURCE_DIR` is respected

[Lldb-commits] [PATCH] D141907: [CMake] Ensure `CLANG_RESOURCE_DIR` is respected

[Lldb-commits] [PATCH] D150928: Two bug fixes for loading process save-core created core files, plus perf improvements

[Lldb-commits] [PATCH] D150914: [lldb][NFCI] Pre-allocate storage in ObjCLanguage::MethodName::GetFullNameWithoutCategory

[Lldb-commits] [lldb] e8d3f06 - [lldb][NFCI] Pre-allocate storage in ObjCLanguage::MethodName::GetFullNameWithoutCategory

[Lldb-commits] [PATCH] D149914: [lldb] Refactor ObjCLanguage::MethodName

[Lldb-commits] [PATCH] D150914: [lldb][NFCI] Pre-allocate storage in ObjCLanguage::MethodName::GetFullNameWithoutCategory

[Lldb-commits] [lldb] e5a6950 - [lldb] Remove decl for non-existent ResolveSymbolContextForAddress overload (NFC)

[Lldb-commits] [PATCH] D150804: [lldb] Guarantee the lifetimes of all strings returned from SBAPI

[Lldb-commits] [lldb] 41714c9 - [lldb] Guarantee the lifetimes of all strings returned from SBAPI

[Lldb-commits] [PATCH] D146765: [lldb/crashlog] Load inlined symbol into interactive crashlog

[Lldb-commits] [PATCH] D150804: [lldb] Guarantee the lifetimes of all strings returned from SBAPI

[Lldb-commits] [PATCH] D149914: [lldb] Refactor ObjCLanguage::MethodName

[Lldb-commits] [PATCH] D149914: [lldb] Refactor ObjCLanguage::MethodName

[Lldb-commits] [PATCH] D149914: [lldb] Refactor ObjCLanguage::MethodName

[Lldb-commits] [lldb] 9152563 - [lldb] Refactor ObjCLanguage::MethodName

[Lldb-commits] [PATCH] D150804: [lldb] Guarantee the lifetimes of all strings returned from SBAPI

[Lldb-commits] [PATCH] D149914: [lldb] Refactor ObjCLanguage::MethodName

[Lldb-commits] [PATCH] D150804: [lldb] Guarantee the lifetimes of all strings returned from SBAPI

[Lldb-commits] [PATCH] D150716: [lldb][NFCI] Switch to using llvm::DWARFAbbreviationDeclaration

[Lldb-commits] [lldb] 16eb148 - [lldb][NFCI] Switch to using llvm::DWARFAbbreviationDeclaration

[Lldb-commits] [PATCH] D150716: [lldb][NFCI] Switch to using llvm::DWARFAbbreviationDeclaration

[Lldb-commits] [PATCH] D150716: [lldb][NFCI] Switch to using llvm::DWARFAbbreviationDeclaration

[Lldb-commits] [PATCH] D150716: [lldb][NFCI] Switch to using llvm::DWARFAbbreviationDeclaration

[Lldb-commits] [PATCH] D150826: [lldb] Implement GetValueTypeFromAddressType

[Lldb-commits] [lldb] 8fe9718 - [lldb] Implement GetValueTypeFromAddressType

[Lldb-commits] [PATCH] D150619: [lldb] Delay removal of persistent results

[Lldb-commits] [lldb] db81455 - [lldb] Delay removal of persistent results

[Lldb-commits] [PATCH] D150716: [lldb][NFCI] Switch to using llvm::DWARFAbbreviationDeclaration

[Lldb-commits] [PATCH] D149914: [lldb] Refactor ObjCLanguage::MethodName

31 matches

Site Navigation

Mail list logo

Footer information