[Lldb-commits] [PATCH] D149663: [lldb] Remove FileSpec::GetLastPathComponent

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

In D149663#4313112 , @mib wrote:

> @bulbazord What if the FileSpec is pointing to a directory instead of a file 
> ? What would `GetFilename` return in that case compared to 
> `GetLastPathComponent` ?

FileSpec chops everything up into a directory and a filename, even if it's 
pointing to a directory. For example:

  fspec = lldb.SBFileSpec("/foo/bar/baz/")
  print(fspec.GetDirectory())
  print(fspec.GetFilename())

This will print "/foo/bar" followed by "baz". baz is clearly a directory, but 
FileSpec will treat it as the filename. `GetLastPathComponent` uses 
`llvm::sys::path::filename` to get the last element of the path, which is the 
exact same mechanism we use when constructing FileSpec's internal `m_directory` 
and `m_filename` in the first place.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149663

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

[Lldb-commits] [PATCH] D149671: [lldb] Minor cleanups at callsites of FileSpec::GetFileNameExtension

[Alex Langford via Phabricator via lldb-commits]

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

FileSpec::GetFileNameExtension returns a StringRef. In some cases we
are calling it and then storing the result in a StringRef. To prevent
cases where we store the StringRef, mutate the Filespec, and then try to
use the FileSpec afterwards, I've audited the callsites and made
adjustments to mitigate: Either marking the FileSpec it comes from as
const (to avoid mutations) or by not storing the StringRef in a local if
it makes sense not to.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D149671

Files:
  lldb/source/Commands/CommandObjectTarget.cpp
  lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
  lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h
  lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp


Index: lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp
===
--- lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp
+++ lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp
@@ -131,14 +131,13 @@
 }
 
 llvm::Error Lua::LoadModule(llvm::StringRef filename) {
-  FileSpec file(filename);
+  const FileSpec file(filename);
   if (!FileSystem::Instance().Exists(file)) {
 return llvm::make_error("invalid path",
llvm::inconvertibleErrorCode());
   }
 
-  llvm::StringRef module_extension = file.GetFileNameExtension();
-  if (module_extension != ".lua") {
+  if (file.GetFileNameExtension() != ".lua") {
 return llvm::make_error("invalid extension",
llvm::inconvertibleErrorCode());
   }
Index: lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h
===
--- lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h
+++ lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h
@@ -137,7 +137,8 @@
   // with the binary inside it ('.../foo.dSYM/Contents/Resources/DWARF/foo').
   // A dSYM bundle may have multiple DWARF binaries in them, so a vector
   // of matches is returned.
-  static std::vector GetDWARFBinaryInDSYMBundle(FileSpec 
dsym_bundle);
+  static std::vector
+  GetDWARFBinaryInDSYMBundle(const FileSpec _bundle);
 
   Status GetSharedModuleKext(const ModuleSpec _spec, Process *process,
  lldb::ModuleSP _sp,
Index: lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
===
--- lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
+++ lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
@@ -422,7 +422,7 @@
   static constexpr llvm::StringLiteral g_kdk_suffix = ".kdk";
 
   PlatformDarwinKernel *thisp = (PlatformDarwinKernel *)baton;
-  FileSpec file_spec(path);
+  const FileSpec file_spec(path);
   if (ft == llvm::sys::fs::file_type::directory_file &&
   (file_spec.GetFileNameExtension() == g_sdk_suffix ||
file_spec.GetFileNameExtension() == g_kdk_suffix)) {
@@ -483,7 +483,7 @@
   static constexpr llvm::StringLiteral g_kext_suffix = ".kext";
   static constexpr llvm::StringLiteral g_dsym_suffix = ".dSYM";
 
-  FileSpec file_spec(path);
+  const FileSpec file_spec(path);
   llvm::StringRef file_spec_extension = file_spec.GetFileNameExtension();
 
   Log *log = GetLog(LLDBLog::Platform);
@@ -692,7 +692,7 @@
 // it should iterate over every binary in the DWARF subdir
 // and return them all.
 std::vector
-PlatformDarwinKernel::GetDWARFBinaryInDSYMBundle(FileSpec dsym_bundle) {
+PlatformDarwinKernel::GetDWARFBinaryInDSYMBundle(const FileSpec _bundle) {
   std::vector results;
   static constexpr llvm::StringLiteral g_dsym_suffix = ".dSYM";
   if (dsym_bundle.GetFileNameExtension() != g_dsym_suffix) {
Index: lldb/source/Commands/CommandObjectTarget.cpp
===
--- lldb/source/Commands/CommandObjectTarget.cpp
+++ lldb/source/Commands/CommandObjectTarget.cpp
@@ -2161,7 +2161,7 @@
 }
 
 const char *pcm_path = command.GetArgumentAtIndex(0);
-FileSpec pcm_file{pcm_path};
+const FileSpec pcm_file{pcm_path};
 
 if (pcm_file.GetFileNameExtension() != ".pcm") {
   result.AppendError("file must have a .pcm extension");


Index: lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp
===
--- lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp
+++ lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp
@@ -131,14 +131,13 @@
 }
 
 llvm::Error Lua::LoadModule(llvm::StringRef filename) {
-  FileSpec file(filename);
+  const FileSpec file(filename);
   if (!FileSystem::Instance().Exists(file)) {
 return llvm::make_error("invalid path",
  

[Lldb-commits] [PATCH] D149625: [lldb] Refactor SBFileSpec::GetDirectory

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

In D149625#4312838 , @jingham wrote:

> The old code had the side-effect of NOT resolving the path of the SBFileSpec 
> in order to get its directory.  I am not sure whether that was on purpose or 
> not, however.

To be more precise, the old code had the side-effect of guaranteeing that 
whatever path was given to you was from an unresolved FileSpec. Right now, if 
the SBFileSpec is unresolved, this will do the exact same thing as before. With 
this change, if an SBFileSpec is resolved, the path you're getting is from a 
resolved FileSpec. I'm pretty sure that this shouldn't actually be different 
than what was there before since we're making a copy from the underlying 
FileSpec though, unless I'm missing something?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149625

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

[Lldb-commits] [PATCH] D149663: [lldb] Remove FileSpec::GetLastPathComponent

[Alex Langford via Phabricator via lldb-commits]

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

As far as I can tell, this just computes the filename of the FileSpec,
which is already conveniently stored in m_filename. We can use
FileSpec::GetFilename() instead.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D149663

Files:
  lldb/include/lldb/Utility/FileSpec.h
  lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
  lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp
  lldb/source/Target/Platform.cpp
  lldb/source/Utility/FileSpec.cpp
  lldb/source/Utility/XcodeSDK.cpp

Index: lldb/source/Utility/XcodeSDK.cpp
===
--- lldb/source/Utility/XcodeSDK.cpp
+++ lldb/source/Utility/XcodeSDK.cpp
@@ -242,7 +242,7 @@
 
 bool XcodeSDK::SDKSupportsModules(XcodeSDK::Type desired_type,
   const FileSpec _path) {
-  ConstString last_path_component = sdk_path.GetLastPathComponent();
+  ConstString last_path_component = sdk_path.GetFilename();
 
   if (!last_path_component)
 return false;
Index: lldb/source/Utility/FileSpec.cpp
===
--- lldb/source/Utility/FileSpec.cpp
+++ lldb/source/Utility/FileSpec.cpp
@@ -429,12 +429,6 @@
   return *this;
 }
 
-ConstString FileSpec::GetLastPathComponent() const {
-  llvm::SmallString<64> current_path;
-  GetPath(current_path, false);
-  return ConstString(llvm::sys::path::filename(current_path, m_style));
-}
-
 void FileSpec::PrependPathComponent(llvm::StringRef component) {
   llvm::SmallString<64> new_path(component);
   llvm::SmallString<64> current_path;
Index: lldb/source/Target/Platform.cpp
===
--- lldb/source/Target/Platform.cpp
+++ lldb/source/Target/Platform.cpp
@@ -435,7 +435,7 @@
 // make the new directory and get in there
 FileSpec dst_dir = rc_baton->dst;
 if (!dst_dir.GetFilename())
-  dst_dir.SetFilename(src.GetLastPathComponent());
+  dst_dir.SetFilename(src.GetFilename());
 Status error = rc_baton->platform_ptr->MakeDirectory(
 dst_dir, lldb::eFilePermissionsDirectoryDefault);
 if (error.Fail()) {
Index: lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp
===
--- lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp
+++ lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp
@@ -1268,7 +1268,7 @@
 
 auto _spec = module_sp->GetFileSpec();
 found_logging_support_module =
-(file_spec.GetLastPathComponent() == logging_module_name);
+(file_spec.GetFilename() == logging_module_name);
 if (found_logging_support_module)
   break;
   }
Index: lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
===
--- lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
+++ lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
@@ -1238,10 +1238,9 @@
 
 FileSpec platform_pull_upart(platform_file);
 std::vector path_parts;
-path_parts.push_back(
-platform_pull_upart.GetLastPathComponent().AsCString());
+path_parts.push_back(platform_pull_upart.GetFilename().AsCString());
 while (platform_pull_upart.RemoveLastPathComponent()) {
-  ConstString part = platform_pull_upart.GetLastPathComponent();
+  ConstString part = platform_pull_upart.GetFilename();
   path_parts.push_back(part.AsCString());
 }
 const size_t path_parts_size = path_parts.size();
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -166,7 +166,7 @@
 auto raw_data = coff_obj.getData();
 LLDB_SCOPED_TIMERF(
 "Calculating module crc32 %s with size %" PRIu64 " KiB",
-FileSpec(coff_obj.getFileName()).GetLastPathComponent().AsCString(),
+FileSpec(coff_obj.getFileName()).GetFilename().AsCString(),
 static_cast(raw_data.size()) / 1024);
 gnu_debuglink_crc = llvm::crc32(0, llvm::arrayRefFromStringRef(raw_data));
   }
@@ -295,7 +295,7 @@
   const auto *map = GetGlobalPluginProperties().ModuleABIMap();
   if (map->GetNumValues() > 0) {
 // Step 1: Try with the exact file name.
-auto name = file.GetLastPathComponent();
+auto name = file.GetFilename();
 module_env_option = map->GetValueForKey(name);
 if 

[Lldb-commits] [PATCH] D149625: [lldb] Refactor SBFileSpec::GetDirectory

[Alex Langford via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG2bea2d7b070d: [lldb] Refactor SBFileSpec::GetDirectory 
(authored by bulbazord).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149625

Files:
  lldb/source/API/SBFileSpec.cpp


Index: lldb/source/API/SBFileSpec.cpp
===
--- lldb/source/API/SBFileSpec.cpp
+++ lldb/source/API/SBFileSpec.cpp
@@ -114,9 +114,7 @@
 const char *SBFileSpec::GetDirectory() const {
   LLDB_INSTRUMENT_VA(this);
 
-  FileSpec directory{*m_opaque_up};
-  directory.ClearFilename();
-  return directory.GetPathAsConstString().GetCString();
+  return m_opaque_up->GetDirectory().GetCString();
 }
 
 void SBFileSpec::SetFilename(const char *filename) {


Index: lldb/source/API/SBFileSpec.cpp
===
--- lldb/source/API/SBFileSpec.cpp
+++ lldb/source/API/SBFileSpec.cpp
@@ -114,9 +114,7 @@
 const char *SBFileSpec::GetDirectory() const {
   LLDB_INSTRUMENT_VA(this);
 
-  FileSpec directory{*m_opaque_up};
-  directory.ClearFilename();
-  return directory.GetPathAsConstString().GetCString();
+  return m_opaque_up->GetDirectory().GetCString();
 }
 
 void SBFileSpec::SetFilename(const char *filename) {
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D149625: [lldb] Refactor SBFileSpec::GetDirectory

[Alex Langford via Phabricator via lldb-commits]

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

There's no reason to create an entire new filespec to mutate and grab
data from when we can just grab the data directly.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D149625

Files:
  lldb/source/API/SBFileSpec.cpp


Index: lldb/source/API/SBFileSpec.cpp
===
--- lldb/source/API/SBFileSpec.cpp
+++ lldb/source/API/SBFileSpec.cpp
@@ -114,9 +114,7 @@
 const char *SBFileSpec::GetDirectory() const {
   LLDB_INSTRUMENT_VA(this);
 
-  FileSpec directory{*m_opaque_up};
-  directory.ClearFilename();
-  return directory.GetPathAsConstString().GetCString();
+  return m_opaque_up->GetDirectory().GetCString();
 }
 
 void SBFileSpec::SetFilename(const char *filename) {


Index: lldb/source/API/SBFileSpec.cpp
===
--- lldb/source/API/SBFileSpec.cpp
+++ lldb/source/API/SBFileSpec.cpp
@@ -114,9 +114,7 @@
 const char *SBFileSpec::GetDirectory() const {
   LLDB_INSTRUMENT_VA(this);
 
-  FileSpec directory{*m_opaque_up};
-  directory.ClearFilename();
-  return directory.GetPathAsConstString().GetCString();
+  return m_opaque_up->GetDirectory().GetCString();
 }
 
 void SBFileSpec::SetFilename(const char *filename) {
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D149482: [lldb] Change ObjectValueDictionary to use a StringMap

[Alex Langford via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGe53e1de57ecc: [lldb] Change ObjectValueDictionary to use a 
StringMap (authored by bulbazord).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149482

Files:
  lldb/include/lldb/Interpreter/OptionValueDictionary.h
  lldb/source/Core/Disassembler.cpp
  lldb/source/Interpreter/OptionValueDictionary.cpp
  lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp
  lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.h
  lldb/source/Plugins/Instruction/ARM/EmulationStateARM.cpp
  lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  lldb/test/API/commands/settings/TestSettings.py
  lldb/unittests/Interpreter/TestOptionValue.cpp

Index: lldb/unittests/Interpreter/TestOptionValue.cpp
===
--- lldb/unittests/Interpreter/TestOptionValue.cpp
+++ lldb/unittests/Interpreter/TestOptionValue.cpp
@@ -146,12 +146,12 @@
   ASSERT_TRUE(dict_copy_ptr->OptionWasSet());
   ASSERT_EQ(dict_copy_ptr->GetNumValues(), 2U);
 
-  auto value_ptr = dict_copy_ptr->GetValueForKey(ConstString("A"));
+  auto value_ptr = dict_copy_ptr->GetValueForKey("A");
   ASSERT_TRUE(value_ptr);
   ASSERT_EQ(value_ptr->GetParent().get(), dict_copy_ptr);
   ASSERT_EQ(value_ptr->GetUInt64Value(), 1U);
 
-  value_ptr = dict_copy_ptr->GetValueForKey(ConstString("B"));
+  value_ptr = dict_copy_ptr->GetValueForKey("B");
   ASSERT_TRUE(value_ptr);
   ASSERT_EQ(value_ptr->GetParent().get(), dict_copy_ptr);
   ASSERT_EQ(value_ptr->GetUInt64Value(), 2U);
Index: lldb/test/API/commands/settings/TestSettings.py
===
--- lldb/test/API/commands/settings/TestSettings.py
+++ lldb/test/API/commands/settings/TestSettings.py
@@ -60,7 +60,7 @@
 self.assertEqual(self.dbg.GetSelectedTarget().GetNumBreakpoints(), 1)
 
 def test_append_target_env_vars(self):
-"""Test that 'append target.run-args' works."""
+"""Test that 'append target.env-vars' works."""
 # Append the env-vars.
 self.runCmd('settings append target.env-vars MY_ENV_VAR=YES')
 # And add hooks to restore the settings during tearDown().
@@ -268,7 +268,7 @@
 found_env_var = True
 break
 self.assertTrue(found_env_var,
-"MY_ENV_VAR was not set in LunchInfo object")
+"MY_ENV_VAR was not set in LaunchInfo object")
 
 self.assertEqual(launch_info.GetNumArguments(), 3)
 self.assertEqual(launch_info.GetArgumentAtIndex(0), "A")
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -300,20 +300,18 @@
 if (!module_env_option) {
   // Step 2: Try with the file name in lowercase.
   auto name_lower = name.GetStringRef().lower();
-  module_env_option =
-  map->GetValueForKey(ConstString(llvm::StringRef(name_lower)));
+  module_env_option = map->GetValueForKey(llvm::StringRef(name_lower));
 }
 if (!module_env_option) {
   // Step 3: Try with the file name with ".debug" suffix stripped.
   auto name_stripped = name.GetStringRef();
   if (name_stripped.consume_back_insensitive(".debug")) {
-module_env_option = map->GetValueForKey(ConstString(name_stripped));
+module_env_option = map->GetValueForKey(name_stripped);
 if (!module_env_option) {
   // Step 4: Try with the file name in lowercase with ".debug" suffix
   // stripped.
   auto name_lower = name_stripped.lower();
-  module_env_option =
-  map->GetValueForKey(ConstString(llvm::StringRef(name_lower)));
+  module_env_option = map->GetValueForKey(llvm::StringRef(name_lower));
 }
   }
 }
Index: lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
===
--- lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
+++ lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
@@ -12,7 +12,6 @@
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Symbol/UnwindPlan.h"
 #include "lldb/Utility/ArchSpec.h"
-#include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/RegisterValue.h"
 #include "lldb/Utility/Stream.h"
 
Index: lldb/source/Plugins/Instruction/ARM/EmulationStateARM.cpp
===
--- lldb/source/Plugins/Instruction/ARM/EmulationStateARM.cpp
+++ lldb/source/Plugins/Instruction/ARM/EmulationStateARM.cpp
@@ -264,8 +264,7 @@
   for (int i = 0; i < num; 

[Lldb-commits] [PATCH] D149482: [lldb] Change ObjectValueDictionary to use a StringMap

[Alex Langford via Phabricator via lldb-commits]

bulbazord updated this revision to Diff 518569.
bulbazord added a comment.

Remove unneeded StringRef


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149482

Files:
  lldb/include/lldb/Interpreter/OptionValueDictionary.h
  lldb/source/Core/Disassembler.cpp
  lldb/source/Interpreter/OptionValueDictionary.cpp
  lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp
  lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.h
  lldb/source/Plugins/Instruction/ARM/EmulationStateARM.cpp
  lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  lldb/test/API/commands/settings/TestSettings.py
  lldb/unittests/Interpreter/TestOptionValue.cpp

Index: lldb/unittests/Interpreter/TestOptionValue.cpp
===
--- lldb/unittests/Interpreter/TestOptionValue.cpp
+++ lldb/unittests/Interpreter/TestOptionValue.cpp
@@ -146,12 +146,12 @@
   ASSERT_TRUE(dict_copy_ptr->OptionWasSet());
   ASSERT_EQ(dict_copy_ptr->GetNumValues(), 2U);
 
-  auto value_ptr = dict_copy_ptr->GetValueForKey(ConstString("A"));
+  auto value_ptr = dict_copy_ptr->GetValueForKey("A");
   ASSERT_TRUE(value_ptr);
   ASSERT_EQ(value_ptr->GetParent().get(), dict_copy_ptr);
   ASSERT_EQ(value_ptr->GetUInt64Value(), 1U);
 
-  value_ptr = dict_copy_ptr->GetValueForKey(ConstString("B"));
+  value_ptr = dict_copy_ptr->GetValueForKey("B");
   ASSERT_TRUE(value_ptr);
   ASSERT_EQ(value_ptr->GetParent().get(), dict_copy_ptr);
   ASSERT_EQ(value_ptr->GetUInt64Value(), 2U);
Index: lldb/test/API/commands/settings/TestSettings.py
===
--- lldb/test/API/commands/settings/TestSettings.py
+++ lldb/test/API/commands/settings/TestSettings.py
@@ -60,7 +60,7 @@
 self.assertEqual(self.dbg.GetSelectedTarget().GetNumBreakpoints(), 1)
 
 def test_append_target_env_vars(self):
-"""Test that 'append target.run-args' works."""
+"""Test that 'append target.env-vars' works."""
 # Append the env-vars.
 self.runCmd('settings append target.env-vars MY_ENV_VAR=YES')
 # And add hooks to restore the settings during tearDown().
@@ -268,7 +268,7 @@
 found_env_var = True
 break
 self.assertTrue(found_env_var,
-"MY_ENV_VAR was not set in LunchInfo object")
+"MY_ENV_VAR was not set in LaunchInfo object")
 
 self.assertEqual(launch_info.GetNumArguments(), 3)
 self.assertEqual(launch_info.GetArgumentAtIndex(0), "A")
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -300,20 +300,18 @@
 if (!module_env_option) {
   // Step 2: Try with the file name in lowercase.
   auto name_lower = name.GetStringRef().lower();
-  module_env_option =
-  map->GetValueForKey(ConstString(llvm::StringRef(name_lower)));
+  module_env_option = map->GetValueForKey(llvm::StringRef(name_lower));
 }
 if (!module_env_option) {
   // Step 3: Try with the file name with ".debug" suffix stripped.
   auto name_stripped = name.GetStringRef();
   if (name_stripped.consume_back_insensitive(".debug")) {
-module_env_option = map->GetValueForKey(ConstString(name_stripped));
+module_env_option = map->GetValueForKey(name_stripped);
 if (!module_env_option) {
   // Step 4: Try with the file name in lowercase with ".debug" suffix
   // stripped.
   auto name_lower = name_stripped.lower();
-  module_env_option =
-  map->GetValueForKey(ConstString(llvm::StringRef(name_lower)));
+  module_env_option = map->GetValueForKey(llvm::StringRef(name_lower));
 }
   }
 }
Index: lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
===
--- lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
+++ lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
@@ -12,7 +12,6 @@
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Symbol/UnwindPlan.h"
 #include "lldb/Utility/ArchSpec.h"
-#include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/RegisterValue.h"
 #include "lldb/Utility/Stream.h"
 
Index: lldb/source/Plugins/Instruction/ARM/EmulationStateARM.cpp
===
--- lldb/source/Plugins/Instruction/ARM/EmulationStateARM.cpp
+++ lldb/source/Plugins/Instruction/ARM/EmulationStateARM.cpp
@@ -264,8 +264,7 @@
   for (int i = 0; i < num; ++i) {
 sstr.Clear();
 sstr.Printf("%c%d", kind, i);
-OptionValueSP 

[Lldb-commits] [PATCH] D149565: [lldb] Add debugger.external-editor setting

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


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

https://reviews.llvm.org/D149565

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

[Lldb-commits] [PATCH] D149262: [lldb] Add settings for expression evaluation memory allocations.

[Alex Langford via Phabricator via lldb-commits]

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

Thanks for adding the test. The test itself doesn't seem portable and likely 
will fail on several platforms though I'm not sure which off the top of my 
head. After landing this, please watch the buildbots and be prepared to add the 
necessary decorators to the test to skip those platforms.


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

https://reviews.llvm.org/D149262

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

[Lldb-commits] [PATCH] D149565: [lldb] Add debugger.external-editor setting

[Alex Langford via Phabricator via lldb-commits]

bulbazord requested changes to this revision.
bulbazord added inline comments.
This revision now requires changes to proceed.



Comment at: lldb/source/Host/macosx/objcxx/Host.mm:395-396
 
-  static std::optional g_app_fsref;
-  static std::string g_app_error;
-  static std::once_flag g_once_flag;
-  std::call_once(g_once_flag, [&]() {
-if (const char *external_editor = ::getenv("LLDB_EXTERNAL_EDITOR")) {
-  LLDB_LOG(log, "Looking for external editor: {0}", external_editor);
-
-  FSRef app_fsref;
-  CFCString editor_name(external_editor, kCFStringEncodingUTF8);
-  long app_error = ::LSFindApplicationForInfo(
-  /*inCreator=*/kLSUnknownCreator, /*inBundleID=*/NULL,
-  /*inName=*/editor_name.get(), /*outAppRef=*/_fsref,
-  /*outAppURL=*/NULL);
-  if (app_error == noErr) {
-g_app_fsref = app_fsref;
-  } else {
-g_app_error =
-llvm::formatv("could not find external editor \"{0}\": "
-  "LSFindApplicationForInfo returned error {1}",
-  external_editor, app_error)
-.str();
+  if (const char *lldb_external_editor = ::getenv("LLDB_EXTERNAL_EDITOR")) {
+if (!editor.empty()) {
+  if (editor != llvm::StringRef(lldb_external_editor)) {

JDevlieghere wrote:
> mib wrote:
> > Should we even check for the env variable is the user set the editor 
> > setting ?
> I asked that question in D149472  and this was Jim's suggestion: 
> https://reviews.llvm.org/D149472#inline-1443754
This is a hard error and not a warning, which I don't think is a good idea. I 
think the point of a setting here is to be able to change behavior during a 
debugging session instead of needing to start over. If these 2 values conflict, 
I would have to restart my session to unset LLDB_EXTERNAL_EDITOR to actually 
get the desired behavior.


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

https://reviews.llvm.org/D149565

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

[Lldb-commits] [PATCH] D149482: [lldb] Change ObjectValueDictionary to use a StringMap

[Alex Langford via Phabricator via lldb-commits]

bulbazord updated this revision to Diff 518542.
bulbazord added a comment.

Sort output for dumping to be deterministic (based on alphabetical order like 
previous implementation)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149482

Files:
  lldb/include/lldb/Interpreter/OptionValueDictionary.h
  lldb/source/Core/Disassembler.cpp
  lldb/source/Interpreter/OptionValueDictionary.cpp
  lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp
  lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.h
  lldb/source/Plugins/Instruction/ARM/EmulationStateARM.cpp
  lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  lldb/test/API/commands/settings/TestSettings.py
  lldb/unittests/Interpreter/TestOptionValue.cpp

Index: lldb/unittests/Interpreter/TestOptionValue.cpp
===
--- lldb/unittests/Interpreter/TestOptionValue.cpp
+++ lldb/unittests/Interpreter/TestOptionValue.cpp
@@ -146,12 +146,12 @@
   ASSERT_TRUE(dict_copy_ptr->OptionWasSet());
   ASSERT_EQ(dict_copy_ptr->GetNumValues(), 2U);
 
-  auto value_ptr = dict_copy_ptr->GetValueForKey(ConstString("A"));
+  auto value_ptr = dict_copy_ptr->GetValueForKey("A");
   ASSERT_TRUE(value_ptr);
   ASSERT_EQ(value_ptr->GetParent().get(), dict_copy_ptr);
   ASSERT_EQ(value_ptr->GetUInt64Value(), 1U);
 
-  value_ptr = dict_copy_ptr->GetValueForKey(ConstString("B"));
+  value_ptr = dict_copy_ptr->GetValueForKey("B");
   ASSERT_TRUE(value_ptr);
   ASSERT_EQ(value_ptr->GetParent().get(), dict_copy_ptr);
   ASSERT_EQ(value_ptr->GetUInt64Value(), 2U);
Index: lldb/test/API/commands/settings/TestSettings.py
===
--- lldb/test/API/commands/settings/TestSettings.py
+++ lldb/test/API/commands/settings/TestSettings.py
@@ -60,7 +60,7 @@
 self.assertEqual(self.dbg.GetSelectedTarget().GetNumBreakpoints(), 1)
 
 def test_append_target_env_vars(self):
-"""Test that 'append target.run-args' works."""
+"""Test that 'append target.env-vars' works."""
 # Append the env-vars.
 self.runCmd('settings append target.env-vars MY_ENV_VAR=YES')
 # And add hooks to restore the settings during tearDown().
@@ -268,7 +268,7 @@
 found_env_var = True
 break
 self.assertTrue(found_env_var,
-"MY_ENV_VAR was not set in LunchInfo object")
+"MY_ENV_VAR was not set in LaunchInfo object")
 
 self.assertEqual(launch_info.GetNumArguments(), 3)
 self.assertEqual(launch_info.GetArgumentAtIndex(0), "A")
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -300,20 +300,18 @@
 if (!module_env_option) {
   // Step 2: Try with the file name in lowercase.
   auto name_lower = name.GetStringRef().lower();
-  module_env_option =
-  map->GetValueForKey(ConstString(llvm::StringRef(name_lower)));
+  module_env_option = map->GetValueForKey(llvm::StringRef(name_lower));
 }
 if (!module_env_option) {
   // Step 3: Try with the file name with ".debug" suffix stripped.
   auto name_stripped = name.GetStringRef();
   if (name_stripped.consume_back_insensitive(".debug")) {
-module_env_option = map->GetValueForKey(ConstString(name_stripped));
+module_env_option = map->GetValueForKey(name_stripped);
 if (!module_env_option) {
   // Step 4: Try with the file name in lowercase with ".debug" suffix
   // stripped.
   auto name_lower = name_stripped.lower();
-  module_env_option =
-  map->GetValueForKey(ConstString(llvm::StringRef(name_lower)));
+  module_env_option = map->GetValueForKey(llvm::StringRef(name_lower));
 }
   }
 }
Index: lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
===
--- lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
+++ lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
@@ -12,7 +12,6 @@
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Symbol/UnwindPlan.h"
 #include "lldb/Utility/ArchSpec.h"
-#include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/RegisterValue.h"
 #include "lldb/Utility/Stream.h"
 
Index: lldb/source/Plugins/Instruction/ARM/EmulationStateARM.cpp
===
--- lldb/source/Plugins/Instruction/ARM/EmulationStateARM.cpp
+++ lldb/source/Plugins/Instruction/ARM/EmulationStateARM.cpp
@@ -264,8 +264,7 @@
   for (int i = 0; i < num; ++i) {
  

[Lldb-commits] [PATCH] D149262: [lldb] Add settings for expression evaluation memory allocations.

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

In D149262#4306884 , @kuilpd wrote:

> In D149262#4306820 , @bulbazord 
> wrote:
>
>> Sorry I should have brought this up earlier but I've noticed you don't have 
>> any tests with this change. Is it possible you could add something there? 
>> Something to make sure that these settings work as expected.
>
> Should I do it also without a target?

It's probably enough to just add a test with a target, the settings shouldn't 
mess with the non-target case I think.


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

https://reviews.llvm.org/D149262

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

[Lldb-commits] [PATCH] D149482: [lldb] Change ObjectValueDictionary to use a StringMap

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.



Comment at: lldb/source/Core/Disassembler.cpp:846
+// Note: The reason we are making the data_type a uint64_t when value 
is
+// uint32_t is that there is no eTypeUInt32 enum value.
+if (llvm::StringRef(value) == "uint32_t")

jingham wrote:
> That's kind of answering a question with a question:  Why isn't there an 
> eTypeUInt32?
I'm not sure, we'd have to ask Caroline who added this back in 2011. I added a 
comment to help me remember, but I suppose this comment may add more confusion 
than remove. I'll remove the note.



Comment at: lldb/source/Interpreter/OptionValueDictionary.cpp:39
 
-for (pos = m_values.begin(); pos != end; ++pos) {
+for (auto pos = m_values.begin(), end = m_values.end(); pos != end; ++pos) 
{
   OptionValue *option_value = pos->second.get();

jingham wrote:
> Does the llvm::StringMap dingus not support `for (auto value : m_values) {` ?
Oh, it probably does. Didn't even think about it, good catch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149482

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

[Lldb-commits] [PATCH] D149262: [lldb] Add settings for expression evaluation memory allocations.

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

Sorry I should have brought this up earlier but I've noticed you don't have any 
tests with this change. Is it possible you could add something there? Something 
to make sure that these settings work as expected.

Sorry for the churn btw, I really appreciate your patience here.




Comment at: lldb/source/Expression/LLVMUserExpression.cpp:340-348
+Process *process_sp;
+ABISP abi_sp;
+if ((process_sp = exe_ctx.GetProcessPtr()) &&
+(abi_sp = process_sp->GetABI())) {
+  stack_frame_size = abi_sp->GetStackFrameSize();
+} else {
+  stack_frame_size = 512 * 1024;

kuilpd wrote:
> bulbazord wrote:
> > kuilpd wrote:
> > > bulbazord wrote:
> > > > A few things:
> > > > - I don't think you need to re-extract process from `exe_ctx`, the 
> > > > variable `process` at the beginning of this function should have a 
> > > > valid Process pointer in it already (see: `LockAndCheckContext` at the 
> > > > beginning of this function). We already assume `target` is valid and 
> > > > use it in the same way.
> > > > - If we can't get an ABI from the Process, do we want to assume a stack 
> > > > frame size of `512 * 1024`? Maybe there is a use case I'm missing, but 
> > > > if we don't have an ABI I'm not convinced that 
> > > > `PrepareToExecuteJITExpression` should succeed. LLDB will need some 
> > > > kind of ABI information to correctly set up the register context to 
> > > > call the JITed code anyway.
> > > - I tried that, but a lot of tests fail on `GetABI()` after that, so had 
> > > to re-extract it. Not sure why.
> > > - `512 * 1024` is what was hardcoded there by default. It makes sense 
> > > that ABI has to exist, but leaving no default value in case if it's not 
> > > retreived is weird as well. Or should we return an error in that case?
> > > 
> > How do the tests fail? If the process is wrong that sounds like a pretty 
> > bad bug :(
> > 
> > I would think that we could return `false` with some logging or an error 
> > would be appropriate if we don't have an ABI. I may be misunderstanding 
> > something but I would think that the tests should still pass when we 
> > `return false` there... I hope.
> Tried returning false, even more tests failed.
> Did some digging, turns out expression can work without any target, right 
> after starting LLDB.
> So tests like [[ 
> https://github.com/llvm/llvm-project/blob/main/lldb/test/API/commands/expression/calculator_mode/TestCalculatorMode.py
>  | this one  ]] fail because there is no target or process.
Ah, right, I forgot! A given expression, if simple enough, might be compiled to 
LLVM IR and then interpreted instead of being compiled to machine code and run 
in the inferior... The fact that a stack frame size is even relevant in that 
case is strange but there's not much we can do about it without further 
refactors. Does something like this work then? If not, this should be fine.

```
if (stack_frame_size == 0) {
  ABISP abi_sp;
  if (process && (abi_sp = process->GetABI()))
stack_frame_size = abi_sp->GetStackFrameSize();
  else
stack_frame_size = 512 * 1024;
}
```


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

https://reviews.llvm.org/D149262

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

[Lldb-commits] [PATCH] D149262: [lldb] Add settings for expression evaluation memory allocations.

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.



Comment at: lldb/source/Expression/LLVMUserExpression.cpp:340-348
+Process *process_sp;
+ABISP abi_sp;
+if ((process_sp = exe_ctx.GetProcessPtr()) &&
+(abi_sp = process_sp->GetABI())) {
+  stack_frame_size = abi_sp->GetStackFrameSize();
+} else {
+  stack_frame_size = 512 * 1024;

kuilpd wrote:
> bulbazord wrote:
> > A few things:
> > - I don't think you need to re-extract process from `exe_ctx`, the variable 
> > `process` at the beginning of this function should have a valid Process 
> > pointer in it already (see: `LockAndCheckContext` at the beginning of this 
> > function). We already assume `target` is valid and use it in the same way.
> > - If we can't get an ABI from the Process, do we want to assume a stack 
> > frame size of `512 * 1024`? Maybe there is a use case I'm missing, but if 
> > we don't have an ABI I'm not convinced that `PrepareToExecuteJITExpression` 
> > should succeed. LLDB will need some kind of ABI information to correctly 
> > set up the register context to call the JITed code anyway.
> - I tried that, but a lot of tests fail on `GetABI()` after that, so had to 
> re-extract it. Not sure why.
> - `512 * 1024` is what was hardcoded there by default. It makes sense that 
> ABI has to exist, but leaving no default value in case if it's not retreived 
> is weird as well. Or should we return an error in that case?
> 
How do the tests fail? If the process is wrong that sounds like a pretty bad 
bug :(

I would think that we could return `false` with some logging or an error would 
be appropriate if we don't have an ABI. I may be misunderstanding something but 
I would think that the tests should still pass when we `return false` there... 
I hope.


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

https://reviews.llvm.org/D149262

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

[Lldb-commits] [PATCH] D149482: [lldb] Change ObjectValueDictionary to use a StringMap

[Alex Langford via Phabricator via lldb-commits]

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

llvm has a structure for maps where the key's type is a string. Using
that also means that the keys for OptionValueDictionary don't stick
around forever in ConstString's StringPool (even after they are gone).

The only thing we lose here is ordering: iterating over the map where the keys
are ConstStrings guarantees that we iterate in alphabetical order.
StringMap makes no guarantees about the ordering when you iterate over
the entire map.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D149482

Files:
  lldb/include/lldb/Interpreter/OptionValueDictionary.h
  lldb/source/Core/Disassembler.cpp
  lldb/source/Interpreter/OptionValueDictionary.cpp
  lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp
  lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.h
  lldb/source/Plugins/Instruction/ARM/EmulationStateARM.cpp
  lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  lldb/test/API/commands/settings/TestSettings.py
  lldb/unittests/Interpreter/TestOptionValue.cpp

Index: lldb/unittests/Interpreter/TestOptionValue.cpp
===
--- lldb/unittests/Interpreter/TestOptionValue.cpp
+++ lldb/unittests/Interpreter/TestOptionValue.cpp
@@ -146,12 +146,12 @@
   ASSERT_TRUE(dict_copy_ptr->OptionWasSet());
   ASSERT_EQ(dict_copy_ptr->GetNumValues(), 2U);
 
-  auto value_ptr = dict_copy_ptr->GetValueForKey(ConstString("A"));
+  auto value_ptr = dict_copy_ptr->GetValueForKey("A");
   ASSERT_TRUE(value_ptr);
   ASSERT_EQ(value_ptr->GetParent().get(), dict_copy_ptr);
   ASSERT_EQ(value_ptr->GetUInt64Value(), 1U);
 
-  value_ptr = dict_copy_ptr->GetValueForKey(ConstString("B"));
+  value_ptr = dict_copy_ptr->GetValueForKey("B");
   ASSERT_TRUE(value_ptr);
   ASSERT_EQ(value_ptr->GetParent().get(), dict_copy_ptr);
   ASSERT_EQ(value_ptr->GetUInt64Value(), 2U);
Index: lldb/test/API/commands/settings/TestSettings.py
===
--- lldb/test/API/commands/settings/TestSettings.py
+++ lldb/test/API/commands/settings/TestSettings.py
@@ -60,7 +60,7 @@
 self.assertEqual(self.dbg.GetSelectedTarget().GetNumBreakpoints(), 1)
 
 def test_append_target_env_vars(self):
-"""Test that 'append target.run-args' works."""
+"""Test that 'append target.env-vars' works."""
 # Append the env-vars.
 self.runCmd('settings append target.env-vars MY_ENV_VAR=YES')
 # And add hooks to restore the settings during tearDown().
@@ -268,7 +268,7 @@
 found_env_var = True
 break
 self.assertTrue(found_env_var,
-"MY_ENV_VAR was not set in LunchInfo object")
+"MY_ENV_VAR was not set in LaunchInfo object")
 
 self.assertEqual(launch_info.GetNumArguments(), 3)
 self.assertEqual(launch_info.GetArgumentAtIndex(0), "A")
@@ -604,6 +604,7 @@
 self.expect(
 "settings show target.env-vars",
 SETTING_MSG("target.env-vars"),
+ordered=False,
 substrs=[
 'target.env-vars (dictionary of strings) =',
 'A=B',
@@ -640,7 +641,7 @@
 def test_settings_remove_single(self):
 # Set some environment variables and use 'remove' to delete them.
 self.runCmd("settings set target.env-vars a=b c=d")
-self.expect("settings show target.env-vars", substrs=["a=b", "c=d"])
+self.expect("settings show target.env-vars", ordered=False, substrs=["a=b", "c=d"])
 self.runCmd("settings remove target.env-vars a")
 self.expect("settings show target.env-vars", matching=False, substrs=["a=b"])
 self.expect("settings show target.env-vars", substrs=["c=d"])
@@ -649,7 +650,7 @@
 
 def test_settings_remove_multiple(self):
 self.runCmd("settings set target.env-vars a=b c=d e=f")
-self.expect("settings show target.env-vars", substrs=["a=b", "c=d", "e=f"])
+self.expect("settings show target.env-vars", ordered=False, substrs=["a=b", "c=d", "e=f"])
 self.runCmd("settings remove target.env-vars a e")
 self.expect("settings show target.env-vars", matching=False, substrs=["a=b", "e=f"])
 self.expect("settings show target.env-vars", substrs=["c=d"])
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -300,20 +300,18 @@
 if (!module_env_option) {
   // Step 2: Try with the file name in lowercase.

[Lldb-commits] [PATCH] D149262: [lldb] Add settings for expression evaluation memory allocations.

[Alex Langford via Phabricator via lldb-commits]

bulbazord requested changes to this revision.
bulbazord added a comment.
This revision now requires changes to proceed.

Mostly looks good to me, just a small thing about an implementation detail.




Comment at: lldb/source/Expression/LLVMUserExpression.cpp:38
 
+using namespace lldb;
 using namespace lldb_private;

What in this file uses something from the lldb namespace now?



Comment at: lldb/source/Expression/LLVMUserExpression.cpp:340-348
+Process *process_sp;
+ABISP abi_sp;
+if ((process_sp = exe_ctx.GetProcessPtr()) &&
+(abi_sp = process_sp->GetABI())) {
+  stack_frame_size = abi_sp->GetStackFrameSize();
+} else {
+  stack_frame_size = 512 * 1024;

A few things:
- I don't think you need to re-extract process from `exe_ctx`, the variable 
`process` at the beginning of this function should have a valid Process pointer 
in it already (see: `LockAndCheckContext` at the beginning of this function). 
We already assume `target` is valid and use it in the same way.
- If we can't get an ABI from the Process, do we want to assume a stack frame 
size of `512 * 1024`? Maybe there is a use case I'm missing, but if we don't 
have an ABI I'm not convinced that `PrepareToExecuteJITExpression` should 
succeed. LLDB will need some kind of ABI information to correctly set up the 
register context to call the JITed code anyway.


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

https://reviews.llvm.org/D149262

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

[Lldb-commits] [PATCH] D149472: [lldb] Refactor host::OpenFileInExternalEditor

[Alex Langford via Phabricator via lldb-commits]

bulbazord accepted this revision.
bulbazord added a comment.

LGTM! Thanks for cleaning this up.




Comment at: lldb/source/Target/Thread.cpp:1762-1763
+  frame_sc.line_entry.file, frame_sc.line_entry.line)) {
+LLDB_LOG_ERROR(GetLog(LLDBLog::Host), std::move(e),
+   "OpenFileInExternalEditor failed: {0}");
+  }

Not related to this change but I don't think I like `LLDB_LOG_ERROR`... I read 
this and I think "Oh, if logging is disabled, will this actually consume the 
error?". After all, the other logging macros do not have a side effect. It'd be 
nice if we could create a function and name it "ConsumeErrorAndLog" or 
something to be clearer that it actually consumes it.


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

https://reviews.llvm.org/D149472

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

[Lldb-commits] [PATCH] D149472: [lldb] Refactor host::OpenFileInExternalEditor

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.



Comment at: lldb/source/Host/macosx/objcxx/Host.mm:337
+
+  LLDB_LOG(log, "Sending {0}:{1} to external editor", file_path, line_no);
+

JDevlieghere wrote:
> bulbazord wrote:
> > nit: Move log line below checking if the file_path is empty? If the 
> > file_spec is empty we may see strange log lines like "Sending :0 to 
> > external editor" which is just noise.
> I actually did that on purpose, so we could tell from the logs that it's 
> empty. It didn't seem worth adding a whole separate log line for, but if you 
> think `:10` looks weird, I'm happy to do it. 
I think a separate log line would be easier to read for somebody not familiar 
with this codepath. It would be confusing otherwise. Something like 
`"OpenFileInExternalEditor called with empty path"`? Or maybe you can keep the 
log line but change it to:

```
LLDB_LOG(log, "Sending {0}:{1} to external editor", file_path.empty() ? 
"" : file_path, line_no);



Comment at: lldb/source/Host/macosx/objcxx/Host.mm:387-388
+  static std::once_flag g_once_flag;
+  std::call_once(g_once_flag, [&]() {
+if (const char *external_editor = ::getenv("LLDB_EXTERNAL_EDITOR")) {
+  LLDB_LOG(log, "Looking for external editor: {0}", external_editor);

JDevlieghere wrote:
> bulbazord wrote:
> > I know you're preserving existing behavior here (or rather, fixing its 
> > faulty implementation), but I think it would be nice if you didn't have to 
> > restart your session and add an environment variable to get this working 
> > with an existing debug session. Or maybe make it a setting?
> I think we're trying to mimic the `EDITOR` and `VISUAL` environment variables 
> but I agree a setting would be better. I can add that in a follow-up. Which 
> one should take preference if you have both?
I would say a setting should probably take priority? That way you can change it 
during your session if desired.


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

https://reviews.llvm.org/D149472

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

[Lldb-commits] [PATCH] D149472: [lldb] Refactor host::OpenFileInExternalEditor

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

LGTM overall. A few nits but overall an excellent cleanup! :)




Comment at: lldb/source/Host/macosx/objcxx/Host.mm:337
+
+  LLDB_LOG(log, "Sending {0}:{1} to external editor", file_path, line_no);
+

nit: Move log line below checking if the file_path is empty? If the file_spec 
is empty we may see strange log lines like "Sending :0 to external editor" 
which is just noise.



Comment at: lldb/source/Host/macosx/objcxx/Host.mm:343-344
+  CFCString file_cfstr(file_path.c_str(), kCFStringEncodingUTF8);
+  CFCReleaser file_URL(::CFURLCreateWithFileSystemPath(
+  NULL, file_cfstr.get(), kCFURLPOSIXPathStyle, false));
+

Could you document what the NULL and false refer to? I think they're for 
allocator and isDirectory or something like that.



Comment at: lldb/source/Host/macosx/objcxx/Host.mm:387-388
+  static std::once_flag g_once_flag;
+  std::call_once(g_once_flag, [&]() {
+if (const char *external_editor = ::getenv("LLDB_EXTERNAL_EDITOR")) {
+  LLDB_LOG(log, "Looking for external editor: {0}", external_editor);

I know you're preserving existing behavior here (or rather, fixing its faulty 
implementation), but I think it would be nice if you didn't have to restart 
your session and add an environment variable to get this working with an 
existing debug session. Or maybe make it a setting?



Comment at: lldb/source/Host/macosx/objcxx/Host.mm:411-412
   kLSLaunchDefaults | kLSLaunchDontAddToRecents | kLSLaunchDontSwitch;
-
-  char *external_editor = ::getenv("LLDB_EXTERNAL_EDITOR");
-
-  if (external_editor) {
-LLDB_LOGF(log, "Looking for external editor \"%s\".\n", external_editor);
-
-if (g_app_name.empty() ||
-strcmp(g_app_name.c_str(), external_editor) != 0) {
-  CFCString editor_name(external_editor, kCFStringEncodingUTF8);
-  error = ::LSFindApplicationForInfo(kLSUnknownCreator, NULL,
- editor_name.get(), _app_fsref, 
NULL);
-
-  // If we found the app, then store away the name so we don't have to
-  // re-look it up.
-  if (error != noErr) {
-LLDB_LOGF(log,
-  "Could not find External Editor application, error: %ld.\n",
-  error);
-return false;
-  }
-}
-app_params.application = _app_fsref;
-  }
+  if (g_app_fsref)
+app_params.application = &(g_app_fsref.value());
 

Should we exit early if we don't have `g_app_fsref` filled in? The 
`application` field of `app_params` won't be filled in so what will 
`LSOpenURLsWithRole` do?


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

https://reviews.llvm.org/D149472

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

[Lldb-commits] [PATCH] D149096: [lldb] Speed up looking for dSYM next to executable

[Alex Langford via Phabricator via lldb-commits]

bulbazord updated this revision to Diff 517725.
bulbazord added a comment.

Address @mib's comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149096

Files:
  lldb/source/Symbol/LocateSymbolFile.cpp

Index: lldb/source/Symbol/LocateSymbolFile.cpp
===
--- lldb/source/Symbol/LocateSymbolFile.cpp
+++ lldb/source/Symbol/LocateSymbolFile.cpp
@@ -80,58 +80,50 @@
 // expanded/uncompressed dSYM and return that filepath in dsym_fspec.
 
 static bool LookForDsymNextToExecutablePath(const ModuleSpec _spec,
-const FileSpec _fspec,
+llvm::StringRef executable_path,
+llvm::sys::path::Style path_style,
 FileSpec _fspec) {
-  ConstString filename = exec_fspec.GetFilename();
-  FileSpec dsym_directory = exec_fspec;
-  dsym_directory.RemoveLastPathComponent();
-
-  std::string dsym_filename = filename.AsCString();
-  dsym_filename += ".dSYM";
-  dsym_directory.AppendPathComponent(dsym_filename);
-  dsym_directory.AppendPathComponent("Contents");
-  dsym_directory.AppendPathComponent("Resources");
-  dsym_directory.AppendPathComponent("DWARF");
-
-  if (FileSystem::Instance().Exists(dsym_directory)) {
-
-// See if the binary name exists in the dSYM DWARF
-// subdir.
-dsym_fspec = dsym_directory;
-dsym_fspec.AppendPathComponent(filename.AsCString());
-if (FileSystem::Instance().Exists(dsym_fspec) &&
-FileAtPathContainsArchAndUUID(dsym_fspec, mod_spec.GetArchitecturePtr(),
+  llvm::StringRef filename = llvm::sys::path::filename(executable_path, path_style);
+  llvm::SmallString<64> dsym_file = executable_path;
+
+  dsym_file.append(".dSYM");
+  llvm::sys::path::append(dsym_file, path_style, "Contents", "Resources",
+  "DWARF", filename);
+
+  // First, see if the binary name exists in the dSYM DWARF subdir
+  if (FileSystem::Instance().Exists(dsym_file)) {
+dsym_fspec.SetFile(dsym_file, path_style);
+if (FileAtPathContainsArchAndUUID(dsym_fspec, mod_spec.GetArchitecturePtr(),
   mod_spec.GetUUIDPtr())) {
   return true;
 }
+  }
 
-// See if we have "../CF.framework" - so we'll look for
-// CF.framework.dSYM/Contents/Resources/DWARF/CF
-// We need to drop the last suffix after '.' to match
-// 'CF' in the DWARF subdir.
-std::string binary_name(filename.AsCString());
-auto last_dot = binary_name.find_last_of('.');
-if (last_dot != std::string::npos) {
-  binary_name.erase(last_dot);
-  dsym_fspec = dsym_directory;
-  dsym_fspec.AppendPathComponent(binary_name);
-  if (FileSystem::Instance().Exists(dsym_fspec) &&
-  FileAtPathContainsArchAndUUID(dsym_fspec,
-mod_spec.GetArchitecturePtr(),
-mod_spec.GetUUIDPtr())) {
-return true;
+  // If the "executable path" is actually a framework bundle, e.g.
+  // /CF.framework, the dSYM's DWARF file may not have the `.framework`
+  // suffix (e.g. /CF.framework.dSYM/Contents/Resources/DWARF/CF).
+  // Given that `dsym_file` already will look like
+  // `/CF.framework.dSYM/Contents/Resources/DWARF/CF.framework` we an
+  // just drop the last `.framework` and check again.
+  if (filename.endswith(".framework")) {
+const auto last_dot = dsym_file.find_last_of('.');
+if (last_dot != llvm::StringRef::npos) {
+  dsym_file.truncate(last_dot - 1); // last_dot - 1 to include the dot
+  if (FileSystem::Instance().Exists(dsym_file)) {
+dsym_fspec.SetFile(dsym_file, path_style);
+if (FileAtPathContainsArchAndUUID(dsym_fspec,
+  mod_spec.GetArchitecturePtr(),
+  mod_spec.GetUUIDPtr())) {
+  return true;
+}
   }
 }
   }
 
   // See if we have a .dSYM.yaa next to this executable path.
-  FileSpec dsym_yaa_fspec = exec_fspec;
-  dsym_yaa_fspec.RemoveLastPathComponent();
-  std::string dsym_yaa_filename = filename.AsCString();
-  dsym_yaa_filename += ".dSYM.yaa";
-  dsym_yaa_fspec.AppendPathComponent(dsym_yaa_filename);
-
-  if (FileSystem::Instance().Exists(dsym_yaa_fspec)) {
+  dsym_file = executable_path;
+  dsym_file.append(".dSYM.yaa");
+  if (FileSystem::Instance().Exists(dsym_file)) {
 ModuleSpec mutable_mod_spec = mod_spec;
 Status error;
 if (Symbols::DownloadObjectAndSymbolFile(mutable_mod_spec, error, true) &&
@@ -160,52 +152,45 @@
   FileSpec _fspec) {
   Log *log = GetLog(LLDBLog::Host);
   const FileSpec _fspec = module_spec.GetFileSpec();
-  if (exec_fspec) {
-if (::LookForDsymNextToExecutablePath(module_spec, exec_fspec,
- 

[Lldb-commits] [PATCH] D149096: [lldb] Speed up looking for dSYM next to executable

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.



Comment at: lldb/source/Symbol/LocateSymbolFile.cpp:89-91
+  dsym_file.append(".dSYM");
+  llvm::sys::path::append(dsym_file, path_style, "Contents", "Resources",
+  "DWARF", filename);

mib wrote:
> Why do you need the first call to `append` ?
The code suggestion you've given doesn't work for 2 reasons:
- `llvm::sys::path::append` only takes 4 Twine arguments, so this won't compile.
- `llvm::sys::path::append` appends things as if they were paths. So for 
`/bin/ls` you'll end up with `/bin/ls/.dSYM/Contents/Resources/DWARF` instead 
of `/bin/ls.dSYM/Contents/Resources/DWARF`.



Comment at: lldb/source/Symbol/LocateSymbolFile.cpp:102
 
-// See if we have "../CF.framework" - so we'll look for
-// CF.framework.dSYM/Contents/Resources/DWARF/CF
-// We need to drop the last suffix after '.' to match
-// 'CF' in the DWARF subdir.
-std::string binary_name(filename.AsCString());
-auto last_dot = binary_name.find_last_of('.');
-if (last_dot != std::string::npos) {
-  binary_name.erase(last_dot);
-  dsym_fspec = dsym_directory;
-  dsym_fspec.AppendPathComponent(binary_name);
-  if (FileSystem::Instance().Exists(dsym_fspec) &&
-  FileAtPathContainsArchAndUUID(dsym_fspec,
-mod_spec.GetArchitecturePtr(),
-mod_spec.GetUUIDPtr())) {
-return true;
+  // See if we have ".../CF.framework" - so we'll look for
+  // CF.framework.dSYM/Contents/Resources/DWARF/CF

mib wrote:
> Is this extra dot on purpose ?
I wanted to distinguish between `$parent_dir/CF.framework` because what is the 
parent_dir in this case? I used 3 dots to indicate `$some_path/CF.framework`.



Comment at: lldb/source/Symbol/LocateSymbolFile.cpp:105-108
+  if (filename.endswith(".framework")) {
+const auto last_dot = dsym_file.find_last_of('.');
+if (last_dot != llvm::StringRef::npos) {
+  dsym_file.truncate(last_dot - 1);

mib wrote:
> This part is a bit confusing ... may be a comment would help understand what 
> we're trying to achieve here.
I'll add more to the comment.



Comment at: lldb/source/Symbol/LocateSymbolFile.cpp:175-178
+llvm::StringRef filename =
+llvm::sys::path::filename(parent_path, path_style);
+if (filename.find('.') == llvm::StringRef::npos)
+  continue;

mib wrote:
> Same here ... this could use a comment to explain what we're doing.
Will add.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149096

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

[Lldb-commits] [PATCH] D149262: [lldb] Add settings for expression evaluation memory allocations.

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.



Comment at: lldb/source/Expression/LLVMUserExpression.cpp:339
+  if (stack_frame_size == 0)
+stack_frame_size = arch == llvm::Triple::msp430 ? 512 : 512 * 1024;
 

kuilpd wrote:
> bulbazord wrote:
> > jingham wrote:
> > > This doesn't seem appropriate in generic code.  You should ask the Arch 
> > > plugin for this.
> > Oh, I missed this in the previous patch. Yeah, we shouldn't be hardcoding 
> > `llvm::Triple::msp430` in generic code. You're introducing even more uses 
> > of this in this patch, we should abstract this out into a plugin like Jim 
> > suggested.
> Do you mean creating a new MSP430 plugin in 
> lldb/source/Plugins/Architecture/, or accessing the existing ABI plugin? 
> Should I create new methods for getting stack frame size in each existing 
> plugin?
You could probably put this in the ABI plugin? The base ABI class could have a 
virtual method like `GetStackFrameSize` that returns `512 * 1024` and the 
MSP430 subclass can override it and return `512` instead. That should work, I 
think.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149262

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

[Lldb-commits] [PATCH] D149300: [lldb] Change return type of FileSpec::GetFileNameExtension

[Alex Langford via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG6fcdfc378c2f: [lldb] Change return type of 
FileSpec::GetFileNameExtension (authored by bulbazord).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149300

Files:
  lldb/include/lldb/Utility/FileSpec.h
  lldb/source/Commands/CommandObjectTarget.cpp
  lldb/source/Core/Debugger.cpp
  lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  lldb/source/Plugins/Platform/Android/PlatformAndroid.cpp
  lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
  lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  lldb/source/Utility/FileSpec.cpp
  lldb/unittests/Utility/FileSpecTest.cpp

Index: lldb/unittests/Utility/FileSpecTest.cpp
===
--- lldb/unittests/Utility/FileSpecTest.cpp
+++ lldb/unittests/Utility/FileSpecTest.cpp
@@ -456,7 +456,7 @@
   FileSpec just_dot = PosixSpec("/tmp/bar.");
 
   EXPECT_TRUE(dylib.GetFileNameExtension() == ".dylib");
-  EXPECT_TRUE(exe.GetFileNameExtension() == ConstString(nullptr));
+  EXPECT_TRUE(exe.GetFileNameExtension() == llvm::StringRef());
   EXPECT_TRUE(dSYM.GetFileNameExtension() == ".dSYM");
   EXPECT_TRUE(just_dot.GetFileNameExtension() == ".");
 
@@ -464,7 +464,7 @@
   FileSpec win_noext = WindowsSpec("C:\\tmp\\foo");
 
   EXPECT_TRUE(dll.GetFileNameExtension() == ".dll");
-  EXPECT_TRUE(win_noext.GetFileNameExtension() == ConstString(nullptr));
+  EXPECT_TRUE(win_noext.GetFileNameExtension() == llvm::StringRef());
 }
 
 TEST(FileSpecTest, TestFileNameStrippingExtension) {
Index: lldb/source/Utility/FileSpec.cpp
===
--- lldb/source/Utility/FileSpec.cpp
+++ lldb/source/Utility/FileSpec.cpp
@@ -399,9 +399,8 @@
 Denormalize(path, m_style);
 }
 
-ConstString FileSpec::GetFileNameExtension() const {
-  return ConstString(
-  llvm::sys::path::extension(m_filename.GetStringRef(), m_style));
+llvm::StringRef FileSpec::GetFileNameExtension() const {
+  return llvm::sys::path::extension(m_filename.GetStringRef(), m_style);
 }
 
 ConstString FileSpec::GetFileNameStrippingExtension() const {
@@ -478,8 +477,8 @@
 /// \b true if the filespec represents an implementation source
 /// file, \b false otherwise.
 bool FileSpec::IsSourceImplementationFile() const {
-  ConstString extension(GetFileNameExtension());
-  if (!extension)
+  llvm::StringRef extension = GetFileNameExtension();
+  if (extension.empty())
 return false;
 
   static RegularExpression g_source_file_regex(llvm::StringRef(
@@ -487,7 +486,7 @@
   "cC][pP]|[sS]|[aA][sS][mM]|[fF]|[fF]77|[fF]90|[fF]95|[fF]03|[fF][oO]["
   "rR]|[fF][tT][nN]|[fF][pP][pP]|[aA][dD][aA]|[aA][dD][bB]|[aA][dD][sS])"
   "$"));
-  return g_source_file_regex.Execute(extension.GetStringRef());
+  return g_source_file_regex.Execute(extension);
 }
 
 bool FileSpec::IsRelative() const {
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
@@ -306,7 +306,7 @@
   // On windows, we need to manually back out of the python tree, and go into
   // the bin directory. This is pretty much the inverse of what ComputePythonDir
   // does.
-  if (this_file.GetFileNameExtension() == ConstString(".pyd")) {
+  if (this_file.GetFileNameExtension() == ".pyd") {
 this_file.RemoveLastPathComponent(); // _lldb.pyd or _lldb_d.pyd
 this_file.RemoveLastPathComponent(); // lldb
 llvm::StringRef libdir = LLDB_PYTHON_RELATIVE_LIBDIR;
Index: lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp
===
--- lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp
+++ lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp
@@ -137,7 +137,7 @@
llvm::inconvertibleErrorCode());
   }
 
-  ConstString module_extension = file.GetFileNameExtension();
+  llvm::StringRef module_extension = file.GetFileNameExtension();
   if (module_extension != ".lua") {
 return llvm::make_error("invalid extension",
llvm::inconvertibleErrorCode());
Index: lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
===
--- lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
+++ lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
@@ -418,8 +418,8 @@
 FileSystem::EnumerateDirectoryResult
 PlatformDarwinKernel::FindKDKandSDKDirectoriesInDirectory(
 void *baton, 

[Lldb-commits] [PATCH] D149214: [lldb] Speed up DebugAbbrev parsing

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

In D149214#4300491 , @aprantl wrote:

> Did you also measure the extra memory consumption? I would be surprised if 
> this mattered, but we do parse a lot of DWARF DIEs...
>
> Generally this seems fine.

I compared the memory profile before/after this change. The summary is that we 
consume about 50% more memory on average (283mb vs 425mb) but our total number 
of allocations is down by over half. This makes sense because the size of 
`DWARFAbbreviationDeclaration` now includes the size of 8 `DWARFAttribute`s, so 
when we create the `DWARFAbbreviationDeclaration` and copy it into the 
`DWARFAbbreviationDeclarationSet`'s vector, we're going to allocate more memory 
to hold each one. However, most `DWARFAbbreviationDeclaration`s probably don't 
use all 8 slots of the `SmallVector` on average, so maybe we could tune this 
number further to reduce overall memory consumptions?

For what it's worth, llvm's version of `DWARFAbbreviationDeclaration` also 
stores attributes in a `SmallVector<$attribute_type, 8>` as well, so we're not 
doing any worse than LLVM in this regard.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149214

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

[Lldb-commits] [PATCH] D149300: [lldb] Change return type of FileSpec::GetFileNameExtension

[Alex Langford via Phabricator via lldb-commits]

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

These don't really need to be in ConstStrings. It's nice that comparing
ConstStrings is fast (just a pointer comparison) but the cost of
creating the ConstString usually already includes the cost of doing a
StringRef comparison anyway, so this is just extra work and extra memory
consumption for basically no benefit.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D149300

Files:
  lldb/include/lldb/Utility/FileSpec.h
  lldb/source/Commands/CommandObjectTarget.cpp
  lldb/source/Core/Debugger.cpp
  lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  lldb/source/Plugins/Platform/Android/PlatformAndroid.cpp
  lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
  lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  lldb/source/Utility/FileSpec.cpp
  lldb/unittests/Utility/FileSpecTest.cpp

Index: lldb/unittests/Utility/FileSpecTest.cpp
===
--- lldb/unittests/Utility/FileSpecTest.cpp
+++ lldb/unittests/Utility/FileSpecTest.cpp
@@ -456,7 +456,7 @@
   FileSpec just_dot = PosixSpec("/tmp/bar.");
 
   EXPECT_TRUE(dylib.GetFileNameExtension() == ".dylib");
-  EXPECT_TRUE(exe.GetFileNameExtension() == ConstString(nullptr));
+  EXPECT_TRUE(exe.GetFileNameExtension() == llvm::StringRef());
   EXPECT_TRUE(dSYM.GetFileNameExtension() == ".dSYM");
   EXPECT_TRUE(just_dot.GetFileNameExtension() == ".");
 
@@ -464,7 +464,7 @@
   FileSpec win_noext = WindowsSpec("C:\\tmp\\foo");
 
   EXPECT_TRUE(dll.GetFileNameExtension() == ".dll");
-  EXPECT_TRUE(win_noext.GetFileNameExtension() == ConstString(nullptr));
+  EXPECT_TRUE(win_noext.GetFileNameExtension() == llvm::StringRef());
 }
 
 TEST(FileSpecTest, TestFileNameStrippingExtension) {
Index: lldb/source/Utility/FileSpec.cpp
===
--- lldb/source/Utility/FileSpec.cpp
+++ lldb/source/Utility/FileSpec.cpp
@@ -399,9 +399,8 @@
 Denormalize(path, m_style);
 }
 
-ConstString FileSpec::GetFileNameExtension() const {
-  return ConstString(
-  llvm::sys::path::extension(m_filename.GetStringRef(), m_style));
+llvm::StringRef FileSpec::GetFileNameExtension() const {
+  return llvm::sys::path::extension(m_filename.GetStringRef(), m_style);
 }
 
 ConstString FileSpec::GetFileNameStrippingExtension() const {
@@ -478,8 +477,8 @@
 /// \b true if the filespec represents an implementation source
 /// file, \b false otherwise.
 bool FileSpec::IsSourceImplementationFile() const {
-  ConstString extension(GetFileNameExtension());
-  if (!extension)
+  llvm::StringRef extension = GetFileNameExtension();
+  if (extension.empty())
 return false;
 
   static RegularExpression g_source_file_regex(llvm::StringRef(
@@ -487,7 +486,7 @@
   "cC][pP]|[sS]|[aA][sS][mM]|[fF]|[fF]77|[fF]90|[fF]95|[fF]03|[fF][oO]["
   "rR]|[fF][tT][nN]|[fF][pP][pP]|[aA][dD][aA]|[aA][dD][bB]|[aA][dD][sS])"
   "$"));
-  return g_source_file_regex.Execute(extension.GetStringRef());
+  return g_source_file_regex.Execute(extension);
 }
 
 bool FileSpec::IsRelative() const {
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
@@ -306,7 +306,7 @@
   // On windows, we need to manually back out of the python tree, and go into
   // the bin directory. This is pretty much the inverse of what ComputePythonDir
   // does.
-  if (this_file.GetFileNameExtension() == ConstString(".pyd")) {
+  if (this_file.GetFileNameExtension() == ".pyd") {
 this_file.RemoveLastPathComponent(); // _lldb.pyd or _lldb_d.pyd
 this_file.RemoveLastPathComponent(); // lldb
 llvm::StringRef libdir = LLDB_PYTHON_RELATIVE_LIBDIR;
Index: lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp
===
--- lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp
+++ lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp
@@ -137,7 +137,7 @@
llvm::inconvertibleErrorCode());
   }
 
-  ConstString module_extension = file.GetFileNameExtension();
+  llvm::StringRef module_extension = file.GetFileNameExtension();
   if (module_extension != ".lua") {
 return llvm::make_error("invalid extension",
llvm::inconvertibleErrorCode());
Index: lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp

[Lldb-commits] [PATCH] D149284: [lldb] Remove finding .Bundle directories in PlatformDarwinKernel

[Alex Langford via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG33d6bd1c6674: [lldb] Remove finding .Bundle directories in 
PlatformDarwinKernel (authored by bulbazord).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149284

Files:
  lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp


Index: lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
===
--- lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
+++ lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
@@ -482,7 +482,6 @@
 bool recurse) {
   static ConstString g_kext_suffix = ConstString(".kext");
   static ConstString g_dsym_suffix = ConstString(".dSYM");
-  static ConstString g_bundle_suffix = ConstString("Bundle");
 
   FileSpec file_spec(path);
   ConstString file_spec_extension = file_spec.GetFileNameExtension();
@@ -567,8 +566,7 @@
 
   // Don't recurse into dSYM/kext/bundle directories
   if (recurse && file_spec_extension != g_dsym_suffix &&
-  file_spec_extension != g_kext_suffix &&
-  file_spec_extension != g_bundle_suffix) {
+  file_spec_extension != g_kext_suffix) {
 LLDB_LOGV(log, "PlatformDarwinKernel descending into directory '{0}'",
   file_spec);
 return FileSystem::eEnumerateDirectoryResultEnter;


Index: lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
===
--- lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
+++ lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
@@ -482,7 +482,6 @@
 bool recurse) {
   static ConstString g_kext_suffix = ConstString(".kext");
   static ConstString g_dsym_suffix = ConstString(".dSYM");
-  static ConstString g_bundle_suffix = ConstString("Bundle");
 
   FileSpec file_spec(path);
   ConstString file_spec_extension = file_spec.GetFileNameExtension();
@@ -567,8 +566,7 @@
 
   // Don't recurse into dSYM/kext/bundle directories
   if (recurse && file_spec_extension != g_dsym_suffix &&
-  file_spec_extension != g_kext_suffix &&
-  file_spec_extension != g_bundle_suffix) {
+  file_spec_extension != g_kext_suffix) {
 LLDB_LOGV(log, "PlatformDarwinKernel descending into directory '{0}'",
   file_spec);
 return FileSystem::eEnumerateDirectoryResultEnter;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D149284: [lldb] Fix finding .Bundle directories in PlatformDarwinKernel

[Alex Langford via Phabricator via lldb-commits]

bulbazord updated this revision to Diff 517309.
bulbazord added a comment.

Remove code instead of attempting to fix things


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149284

Files:
  lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp


Index: lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
===
--- lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
+++ lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
@@ -482,7 +482,6 @@
 bool recurse) {
   static ConstString g_kext_suffix = ConstString(".kext");
   static ConstString g_dsym_suffix = ConstString(".dSYM");
-  static ConstString g_bundle_suffix = ConstString("Bundle");
 
   FileSpec file_spec(path);
   ConstString file_spec_extension = file_spec.GetFileNameExtension();
@@ -567,8 +566,7 @@
 
   // Don't recurse into dSYM/kext/bundle directories
   if (recurse && file_spec_extension != g_dsym_suffix &&
-  file_spec_extension != g_kext_suffix &&
-  file_spec_extension != g_bundle_suffix) {
+  file_spec_extension != g_kext_suffix) {
 LLDB_LOGV(log, "PlatformDarwinKernel descending into directory '{0}'",
   file_spec);
 return FileSystem::eEnumerateDirectoryResultEnter;


Index: lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
===
--- lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
+++ lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
@@ -482,7 +482,6 @@
 bool recurse) {
   static ConstString g_kext_suffix = ConstString(".kext");
   static ConstString g_dsym_suffix = ConstString(".dSYM");
-  static ConstString g_bundle_suffix = ConstString("Bundle");
 
   FileSpec file_spec(path);
   ConstString file_spec_extension = file_spec.GetFileNameExtension();
@@ -567,8 +566,7 @@
 
   // Don't recurse into dSYM/kext/bundle directories
   if (recurse && file_spec_extension != g_dsym_suffix &&
-  file_spec_extension != g_kext_suffix &&
-  file_spec_extension != g_bundle_suffix) {
+  file_spec_extension != g_kext_suffix) {
 LLDB_LOGV(log, "PlatformDarwinKernel descending into directory '{0}'",
   file_spec);
 return FileSystem::eEnumerateDirectoryResultEnter;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D149284: [lldb] Fix finding .Bundle directories in PlatformDarwinKernel

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

In D149284#4299933 , @jasonmolenda 
wrote:

> I added this in 2016 via 243bd763ecad466fac748b565ea52c661107684d and I have 
> no idea what it is for.  I know of no such prefix related to kernel 
> debugging.  Let's remove it instead.

Sounds good, will update this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149284

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

[Lldb-commits] [PATCH] D149284: [lldb] Fix finding .Bundle directories in PlatformDarwinKernel

[Alex Langford via Phabricator via lldb-commits]

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

FileSpec::GetFileNameExtension returns a string where the first
character is guaranteed to be a `'.'`. This was probably just an
oversight during a refactor.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D149284

Files:
  lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp


Index: lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
===
--- lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
+++ lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
@@ -482,7 +482,7 @@
 bool recurse) {
   static ConstString g_kext_suffix = ConstString(".kext");
   static ConstString g_dsym_suffix = ConstString(".dSYM");
-  static ConstString g_bundle_suffix = ConstString("Bundle");
+  static ConstString g_bundle_suffix = ConstString(".Bundle");
 
   FileSpec file_spec(path);
   ConstString file_spec_extension = file_spec.GetFileNameExtension();


Index: lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
===
--- lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
+++ lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
@@ -482,7 +482,7 @@
 bool recurse) {
   static ConstString g_kext_suffix = ConstString(".kext");
   static ConstString g_dsym_suffix = ConstString(".dSYM");
-  static ConstString g_bundle_suffix = ConstString("Bundle");
+  static ConstString g_bundle_suffix = ConstString(".Bundle");
 
   FileSpec file_spec(path);
   ConstString file_spec_extension = file_spec.GetFileNameExtension();
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D149111: [lldb] Add support for specifying language when setting watchpoint by expression

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

In D149111#4299871 , @bulbazord wrote:

> Looks like this test does not pass on windows: 
> https://lab.llvm.org/buildbot/#/builders/219/builds/2389
>
> I'm aware and working on this.

I marked the test as unsupported on windows in 
7590cc908807b1bfc63695ca016a60327c1aacdf 
. This 
should get the bot green again.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149111

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

[Lldb-commits] [PATCH] D149111: [lldb] Add support for specifying language when setting watchpoint by expression

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

Looks like this test does not pass on windows: 
https://lab.llvm.org/buildbot/#/builders/219/builds/2389

I'm aware and working on this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149111

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

[Lldb-commits] [PATCH] D149262: [lldb] Add settings for expression evaluation memory allocations.

[Alex Langford via Phabricator via lldb-commits]

bulbazord requested changes to this revision.
bulbazord added inline comments.
This revision now requires changes to proceed.



Comment at: lldb/source/Expression/LLVMUserExpression.cpp:339
+  if (stack_frame_size == 0)
+stack_frame_size = arch == llvm::Triple::msp430 ? 512 : 512 * 1024;
 

jingham wrote:
> This doesn't seem appropriate in generic code.  You should ask the Arch 
> plugin for this.
Oh, I missed this in the previous patch. Yeah, we shouldn't be hardcoding 
`llvm::Triple::msp430` in generic code. You're introducing even more uses of 
this in this patch, we should abstract this out into a plugin like Jim 
suggested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149262

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

[Lldb-commits] [PATCH] D149111: [lldb] Add support for specifying language when setting watchpoint by expression

[Alex Langford via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGc997acb97a9b: [lldb] Add support for specifying language 
when setting watchpoint by expression (authored by bulbazord).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149111

Files:
  lldb/include/lldb/Interpreter/OptionGroupWatchpoint.h
  lldb/source/Commands/CommandObjectWatchpoint.cpp
  lldb/source/Interpreter/OptionGroupWatchpoint.cpp
  lldb/test/Shell/Watchpoint/ExpressionLanguage.test
  lldb/test/Shell/Watchpoint/Inputs/languages.cpp

Index: lldb/test/Shell/Watchpoint/Inputs/languages.cpp
===
--- /dev/null
+++ lldb/test/Shell/Watchpoint/Inputs/languages.cpp
@@ -0,0 +1,12 @@
+#include 
+#include 
+
+uint64_t g_foo = 5;
+uint64_t g_bar = 6;
+uint64_t g_baz = 7;
+
+int main() {
+  int val = 8;
+  printf("Hello world! %d\n", val);
+  return 0;
+}
Index: lldb/test/Shell/Watchpoint/ExpressionLanguage.test
===
--- /dev/null
+++ lldb/test/Shell/Watchpoint/ExpressionLanguage.test
@@ -0,0 +1,19 @@
+# RUN: %clangxx_host %p/Inputs/languages.cpp -g -o %t.out
+# RUN: %lldb -b -o 'settings set interpreter.stop-command-source-on-error false' -s %s %t.out 2>&1 | FileCheck %s
+
+settings  show interpreter.stop-command-source-on-error
+# CHECK: interpreter.stop-command-source-on-error (boolean) = false
+
+b main
+run
+# CHECK: stopped
+# CHECK-NEXT: stop reason = breakpoint
+
+watchpoint set expression -- _foo
+# CHECK: Watchpoint created:
+
+watchpoint set expression -l c++ -- _bar
+# CHECK: Watchpoint created:
+
+watchpoint set expression -l fake -- _baz
+# CHECK: Unknown language type: 'fake' for expression. List of supported languages:
Index: lldb/source/Interpreter/OptionGroupWatchpoint.cpp
===
--- lldb/source/Interpreter/OptionGroupWatchpoint.cpp
+++ lldb/source/Interpreter/OptionGroupWatchpoint.cpp
@@ -10,6 +10,7 @@
 
 #include "lldb/Host/OptionParser.h"
 #include "lldb/Interpreter/OptionArgParser.h"
+#include "lldb/Target/Language.h"
 #include "lldb/lldb-enumerations.h"
 
 using namespace lldb;
@@ -62,7 +63,17 @@
  "Specify the type of watching to perform."},
 {LLDB_OPT_SET_1, false, "size", 's', OptionParser::eRequiredArgument,
  nullptr, OptionEnumValues(g_watch_size), 0, eArgTypeByteSize,
- "Number of bytes to use to watch a region."}};
+ "Number of bytes to use to watch a region."},
+{LLDB_OPT_SET_2,
+ false,
+ "language",
+ 'l',
+ OptionParser::eRequiredArgument,
+ nullptr,
+ {},
+ 0,
+ eArgTypeLanguage,
+ "Language of expression to run"}};
 
 bool OptionGroupWatchpoint::IsWatchSizeSupported(uint32_t watch_size) {
   for (const auto& size : g_watch_size) {
@@ -81,6 +92,18 @@
   Status error;
   const int short_option = g_option_table[option_idx].short_option;
   switch (short_option) {
+  case 'l': {
+language_type = Language::GetLanguageTypeFromString(option_arg);
+if (language_type == eLanguageTypeUnknown) {
+  StreamString sstr;
+  sstr.Printf("Unknown language type: '%s' for expression. List of "
+  "supported languages:\n",
+  option_arg.str().c_str());
+  Language::PrintSupportedLanguagesForExpressions(sstr, " ", "\n");
+  error.SetErrorString(sstr.GetString());
+}
+break;
+  }
   case 'w': {
 WatchType tmp_watch_type;
 tmp_watch_type = (WatchType)OptionArgParser::ToOptionEnum(
@@ -108,6 +131,7 @@
   watch_type_specified = false;
   watch_type = eWatchInvalid;
   watch_size = 0;
+  language_type = eLanguageTypeUnknown;
 }
 
 llvm::ArrayRef OptionGroupWatchpoint::GetDefinitions() {
Index: lldb/source/Commands/CommandObjectWatchpoint.cpp
===
--- lldb/source/Commands/CommandObjectWatchpoint.cpp
+++ lldb/source/Commands/CommandObjectWatchpoint.cpp
@@ -835,8 +835,7 @@
 m_arguments.push_back(arg);
 
 // Absorb the '-w' and '-s' options into our option group.
-m_option_group.Append(_option_watchpoint, LLDB_OPT_SET_ALL,
-  LLDB_OPT_SET_1);
+m_option_group.Append(_option_watchpoint, LLDB_OPT_SET_1, LLDB_OPT_SET_1);
 m_option_group.Finalize();
   }
 
@@ -990,6 +989,7 @@
   : CommandObjectRaw(
 interpreter, "watchpoint set expression",
 "Set a watchpoint on an address by supplying an expression. "
+"Use the '-l' option to specify the language of the expression. "
 "Use the '-w' option to specify the type of watchpoint and "
 "the '-s' option to specify the byte size to watch for. "
 "If no '-w' option is specified, it defaults to write. "
@@ -1083,6 +1083,8 @@
 options.SetKeepInMemory(false);
 options.SetTryAllThreads(true);
   

[Lldb-commits] [PATCH] D149218: [lldb] Fix another GCC build failure in ScriptedPythonInterface.h

[Alex Langford via Phabricator via lldb-commits]

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

Unfortunate.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149218

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

[Lldb-commits] [PATCH] D149214: [lldb] Speed up DebugAbbrev parsing

[Alex Langford via Phabricator via lldb-commits]

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

While measuring some performance in LLDB I noticed that we were
spending a decent amount of time parsing the debug abbrev section.

There are 2 very easy ways to improve speed here:

- Move DWARFAbbreviationDeclarationSets into the the DWARFDebugAbbrev map
- Use an `llvm::SmallVector` instead of a `std::vector` for 
DWARFAbbreviationDeclaration's m_attributes field. These things hardly ever 
have more than 10-11 attributes, so SmallVector seems like a better fit.

To measure performance impact, I took a project with 10,000 c++ source
files, built objects out of them all, and linked them into one binary.
Then I loaded it into lldb, placed a breakpoint on `main`, and then
exited.

Without this patch, it took about 5.2 seconds on my machine. With this
patch, it took about 4.9 seconds, so this shaves off about 300ms of
time.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D149214

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp


Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp
@@ -112,7 +112,7 @@
 if (error)
   return error;
 
-m_abbrevCollMap[initial_cu_offset] = abbrevDeclSet;
+m_abbrevCollMap[initial_cu_offset] = std::move(abbrevDeclSet);
   }
   m_prev_abbr_offset_pos = m_abbrevCollMap.end();
   return llvm::ErrorSuccess();
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.h
@@ -58,7 +58,7 @@
   dw_uleb128_t m_code = InvalidCode;
   dw_tag_t m_tag = llvm::dwarf::DW_TAG_null;
   uint8_t m_has_children = 0;
-  DWARFAttribute::collection m_attributes;
+  llvm::SmallVector m_attributes;
 };
 
 #endif // LLDB_SOURCE_PLUGINS_SYMBOLFILE_DWARF_DWARFABBREVIATIONDECLARATION_H


Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp
@@ -112,7 +112,7 @@
 if (error)
   return error;
 
-m_abbrevCollMap[initial_cu_offset] = abbrevDeclSet;
+m_abbrevCollMap[initial_cu_offset] = std::move(abbrevDeclSet);
   }
   m_prev_abbr_offset_pos = m_abbrevCollMap.end();
   return llvm::ErrorSuccess();
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.h
@@ -58,7 +58,7 @@
   dw_uleb128_t m_code = InvalidCode;
   dw_tag_t m_tag = llvm::dwarf::DW_TAG_null;
   uint8_t m_has_children = 0;
-  DWARFAttribute::collection m_attributes;
+  llvm::SmallVector m_attributes;
 };
 
 #endif // LLDB_SOURCE_PLUGINS_SYMBOLFILE_DWARF_DWARFABBREVIATIONDECLARATION_H
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D149179: [lldb/test] Consolidate interactive scripted process debugging test

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

Seems ok to me. You might want to update the dependencies of this diff so that 
the pre-merge checks don't fail.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149179

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

[Lldb-commits] [PATCH] D149111: [lldb] Add support for specifying language when setting watchpoint by expression

[Alex Langford via Phabricator via lldb-commits]

bulbazord requested review of this revision.
bulbazord added a comment.

In D149111#4296670 , @mib wrote:

> I think we should still be able to use this new flag with the previous ones 
> (i.e. `watchpoint set -l c++ -w read -- _var`)

You can do that still with this patch. I'm placing the language flag in a 2nd 
group so that `watchpoint set variable` does not have access to this flag, but 
`watchpoint set expression` does. You can still mix and match the flags for 
`expression`.




Comment at: lldb/source/Interpreter/OptionGroupWatchpoint.cpp:67
+ "Number of bytes to use to watch a region."},
+{LLDB_OPT_SET_2,
+ false,

mib wrote:
> bulbazord wrote:
> > jasonmolenda wrote:
> > > I don't care much, but the formatting of this entry is pretty 
> > > inconsistent with the two SET_1's above.  Should they match?
> > I applied clang-format to the entire thing and this is what it gave me 
> > /shrug
> Having a different option group for this flag means it can't be used with the 
> other ones ... Is that really what you want ?
`OptionGroupWatchpoint` as far as I can tell is only used for `watchpoint set 
expression` and `watchpoint set variable`. I don't think this flag makes sense 
for `watchpoint set variable`, so I'm adding it to the 2nd group for expression 
and opting out for variable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149111

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

[Lldb-commits] [PATCH] D149111: [lldb] Add support for specifying language when setting watchpoint by expression

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.



Comment at: lldb/source/Interpreter/OptionGroupWatchpoint.cpp:67
+ "Number of bytes to use to watch a region."},
+{LLDB_OPT_SET_2,
+ false,

jasonmolenda wrote:
> I don't care much, but the formatting of this entry is pretty inconsistent 
> with the two SET_1's above.  Should they match?
I applied clang-format to the entire thing and this is what it gave me 
/shrug


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149111

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

[Lldb-commits] [PATCH] D149175: [lldb/test] Update lldbutil.fetch_next_event to match broadcaster class

[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


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

https://reviews.llvm.org/D149175

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

[Lldb-commits] [PATCH] D149175: [lldb/test] Update lldbutil.fetch_next_event to match broadcaster class

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.



Comment at: lldb/packages/Python/lldbsuite/test/lldbutil.py:1206-1207
+def fetch_next_event(test, listener, broadcaster, match_class=False, 
timeout=10):
 """Fetch one event from the listener and return it if it matches the 
provided broadcaster.
 Fails otherwise."""
 

Maybe update the docstring to reflect this new parameter? Something like `If 
match_class is true, the provided broadcaster will be matched by class rather 
than instance`?



Comment at: lldb/packages/Python/lldbsuite/test/lldbutil.py:1219-1220
 
+stream = lldb.SBStream()
+event.GetDescription(stream)
 test.fail("received event '%s' from unexpected broadcaster '%s'." %

I'm not sure why you added this part? Is there something not great about 
calling `SBEvent::GetDescription` directly?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149175

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

[Lldb-commits] [PATCH] D149111: [lldb] Add support for specifying language when setting watchpoint by expression

[Alex Langford via Phabricator via lldb-commits]

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

This is useful in contexts where you have multiple languages in play:
You may be stopped in a frame for language A, but want to set a breakpoint
with an expression using language B. The current way to do this is to
use the setting `target.language` while setting the watchpoint and
unset it after the watchpoint is set, but that's kind of clunky and
somewhat error-prone. This should add a better way to do this.

rdar://108202559


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D149111

Files:
  lldb/include/lldb/Interpreter/OptionGroupWatchpoint.h
  lldb/source/Commands/CommandObjectWatchpoint.cpp
  lldb/source/Interpreter/OptionGroupWatchpoint.cpp
  lldb/test/Shell/Watchpoint/ExpressionLanguage.test
  lldb/test/Shell/Watchpoint/Inputs/languages.cpp

Index: lldb/test/Shell/Watchpoint/Inputs/languages.cpp
===
--- /dev/null
+++ lldb/test/Shell/Watchpoint/Inputs/languages.cpp
@@ -0,0 +1,12 @@
+#include 
+#include 
+
+uint64_t g_foo = 5;
+uint64_t g_bar = 6;
+uint64_t g_baz = 7;
+
+int main() {
+  int val = 8;
+  printf("Hello world! %d\n", val);
+  return 0;
+}
Index: lldb/test/Shell/Watchpoint/ExpressionLanguage.test
===
--- /dev/null
+++ lldb/test/Shell/Watchpoint/ExpressionLanguage.test
@@ -0,0 +1,19 @@
+# RUN: %clangxx_host %p/Inputs/languages.cpp -g -o %t.out
+# RUN: %lldb -b -o 'settings set interpreter.stop-command-source-on-error false' -s %s %t.out 2>&1 | FileCheck %s
+
+settings  show interpreter.stop-command-source-on-error
+# CHECK: interpreter.stop-command-source-on-error (boolean) = false
+
+b main
+run
+# CHECK: stopped
+# CHECK-NEXT: stop reason = breakpoint
+
+watchpoint set expression -- _foo
+# CHECK: Watchpoint created:
+
+watchpoint set expression -l c++ -- _bar
+# CHECK: Watchpoint created:
+
+watchpoint set expression -l fake -- _baz
+# CHECK: Unknown language type: 'fake' for expression. List of supported languages:
Index: lldb/source/Interpreter/OptionGroupWatchpoint.cpp
===
--- lldb/source/Interpreter/OptionGroupWatchpoint.cpp
+++ lldb/source/Interpreter/OptionGroupWatchpoint.cpp
@@ -10,6 +10,7 @@
 
 #include "lldb/Host/OptionParser.h"
 #include "lldb/Interpreter/OptionArgParser.h"
+#include "lldb/Target/Language.h"
 #include "lldb/lldb-enumerations.h"
 
 using namespace lldb;
@@ -62,7 +63,17 @@
  "Specify the type of watching to perform."},
 {LLDB_OPT_SET_1, false, "size", 's', OptionParser::eRequiredArgument,
  nullptr, OptionEnumValues(g_watch_size), 0, eArgTypeByteSize,
- "Number of bytes to use to watch a region."}};
+ "Number of bytes to use to watch a region."},
+{LLDB_OPT_SET_2,
+ false,
+ "language",
+ 'l',
+ OptionParser::eRequiredArgument,
+ nullptr,
+ {},
+ 0,
+ eArgTypeLanguage,
+ "Language of expression to run"}};
 
 bool OptionGroupWatchpoint::IsWatchSizeSupported(uint32_t watch_size) {
   for (const auto& size : g_watch_size) {
@@ -81,6 +92,18 @@
   Status error;
   const int short_option = g_option_table[option_idx].short_option;
   switch (short_option) {
+  case 'l': {
+language_type = Language::GetLanguageTypeFromString(option_arg);
+if (language_type == eLanguageTypeUnknown) {
+  StreamString sstr;
+  sstr.Printf("Unknown language type: '%s' for expression. List of "
+  "supported languages:\n",
+  option_arg.str().c_str());
+  Language::PrintSupportedLanguagesForExpressions(sstr, " ", "\n");
+  error.SetErrorString(sstr.GetString());
+}
+break;
+  }
   case 'w': {
 WatchType tmp_watch_type;
 tmp_watch_type = (WatchType)OptionArgParser::ToOptionEnum(
@@ -108,6 +131,7 @@
   watch_type_specified = false;
   watch_type = eWatchInvalid;
   watch_size = 0;
+  language_type = eLanguageTypeUnknown;
 }
 
 llvm::ArrayRef OptionGroupWatchpoint::GetDefinitions() {
Index: lldb/source/Commands/CommandObjectWatchpoint.cpp
===
--- lldb/source/Commands/CommandObjectWatchpoint.cpp
+++ lldb/source/Commands/CommandObjectWatchpoint.cpp
@@ -835,8 +835,7 @@
 m_arguments.push_back(arg);
 
 // Absorb the '-w' and '-s' options into our option group.
-m_option_group.Append(_option_watchpoint, LLDB_OPT_SET_ALL,
-  LLDB_OPT_SET_1);
+m_option_group.Append(_option_watchpoint, LLDB_OPT_SET_1, LLDB_OPT_SET_1);
 m_option_group.Finalize();
   }
 
@@ -990,6 +989,7 @@
   : CommandObjectRaw(
 interpreter, "watchpoint set expression",
 "Set a watchpoint on an 

[Lldb-commits] [PATCH] D149096: [lldb] Speed up looking for dSYM next to executable

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

Perhaps the right thing to do would be to try to do the larger FileSpec changes 
instead? The changes in `Symbols::LocateExecutableSymbolFile` can probably go 
in regardless though. We call `Resolve` on all the file specs in 
`debug_file_search_paths` on line 307, so no need to call `Resolve` twice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149096

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

[Lldb-commits] [PATCH] D149096: [lldb] Speed up looking for dSYM next to executable

[Alex Langford via Phabricator via lldb-commits]

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

The goal of this patch is to speed up the code that searches for a dSYM
next to an executable.

I generated a project with 10,000 c source files and accompanying header
files using a script that I wrote. I generated objects for each of them
(with debug info in it) but intentionally did not create a dSYM for
anything. I also built a driver, and then linked everything into one
binary. The project itself is very simple, but the number of object files
involved is very large. I then measured lldb's performance debugging
this binary. (NB: I plan on putting this script into lldb's script
directory at some point after some cleanups)

Using the command `lldb -x -b -o 'b main' $binary` (launch lldb with no
lldbinit, place a breakpoint on main, and then quit), I measured how
long this takes and measured where time was being spent.

On my machine, I used hyperfine to get some statistics about how long
this actually took:

  alex@alangford many-objects % hyperfine -w 3 -- "$lldb -x -b -o 'b main' 
$binary"
  Benchmark 1: $lldb -x -b -o 'b main' $binary
Time (mean ± σ):  4.395 s ±  0.046 s[User: 3.239 s, System: 1.245 s]
Range (min … max):4.343 s …  4.471 s10 runs

Out of the ~4.4 seconds of work, it looks like ~630ms were being spent
on `LocateDSYMInVincinityOfExecutable`. After digging in further, we
were spending the majority of our time manipulating paths in `FileSpec`
with `RemoveLastPathComponent` and `AppendPathComponent`. There were a
lot of FileSystem operations as well, so I made an attempt to minimize
the amount of calls to `Exists` and `Resolve` as well.

Given how widely used the code in FileSpec is, I opted to improve this
codepath specifically rather than attempt to refactor the internals of
FileSpec. I believe that improving FileSpec is also worth doing, but as
it is a larger change with far reaching implications, I opted to make
this change instead.

With this change, we shave off approximately 600ms:

  alex@alangford many-objects % hyperfine -w 3 -- "$lldb -x -b -o 'b main' 
$binary"
  Benchmark 1: $lldb -x -b -o 'b main' $binary
Time (mean ± σ):  3.822 s ±  0.056 s[User: 2.749 s, System: 1.167 s]
Range (min … max):3.750 s …  3.920 s10 runs


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D149096

Files:
  lldb/source/Symbol/LocateSymbolFile.cpp

Index: lldb/source/Symbol/LocateSymbolFile.cpp
===
--- lldb/source/Symbol/LocateSymbolFile.cpp
+++ lldb/source/Symbol/LocateSymbolFile.cpp
@@ -80,58 +80,47 @@
 // expanded/uncompressed dSYM and return that filepath in dsym_fspec.
 
 static bool LookForDsymNextToExecutablePath(const ModuleSpec _spec,
-const FileSpec _fspec,
+llvm::StringRef executable_path,
+llvm::sys::path::Style path_style,
 FileSpec _fspec) {
-  ConstString filename = exec_fspec.GetFilename();
-  FileSpec dsym_directory = exec_fspec;
-  dsym_directory.RemoveLastPathComponent();
-
-  std::string dsym_filename = filename.AsCString();
-  dsym_filename += ".dSYM";
-  dsym_directory.AppendPathComponent(dsym_filename);
-  dsym_directory.AppendPathComponent("Contents");
-  dsym_directory.AppendPathComponent("Resources");
-  dsym_directory.AppendPathComponent("DWARF");
-
-  if (FileSystem::Instance().Exists(dsym_directory)) {
-
-// See if the binary name exists in the dSYM DWARF
-// subdir.
-dsym_fspec = dsym_directory;
-dsym_fspec.AppendPathComponent(filename.AsCString());
-if (FileSystem::Instance().Exists(dsym_fspec) &&
-FileAtPathContainsArchAndUUID(dsym_fspec, mod_spec.GetArchitecturePtr(),
+  llvm::StringRef filename = llvm::sys::path::filename(executable_path, path_style);
+  llvm::SmallString<64> dsym_file = executable_path;
+
+  dsym_file.append(".dSYM");
+  llvm::sys::path::append(dsym_file, path_style, "Contents", "Resources",
+  "DWARF", filename);
+
+  // First, see if the binary name exists in the dSYM DWARF subdir
+  if (FileSystem::Instance().Exists(dsym_file)) {
+dsym_fspec.SetFile(dsym_file, path_style);
+if (FileAtPathContainsArchAndUUID(dsym_fspec, mod_spec.GetArchitecturePtr(),
   mod_spec.GetUUIDPtr())) {
   return true;
 }
+  }
 
-// See if we have "../CF.framework" - so we'll look for
-// CF.framework.dSYM/Contents/Resources/DWARF/CF
-// We need to drop the last suffix after '.' to match
-// 'CF' in the DWARF subdir.
-std::string binary_name(filename.AsCString());
-auto last_dot = 

[Lldb-commits] [PATCH] D148395: [lldb] Unify default/hijack listener between Process{Attach, Launch}Info (NFC)

[Alex Langford via Phabricator via lldb-commits]

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

I don't have an issue here then. LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148395

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

[Lldb-commits] [PATCH] D148395: [lldb] Unify default/hijack listener between Process{Attach, Launch}Info (NFC)

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.



Comment at: lldb/include/lldb/Target/Process.h:122
 m_async(false) {
 ProcessInfo::operator=(launch_info);
 SetProcessPluginName(launch_info.GetProcessPluginName());

bulbazord wrote:
> mib wrote:
> > Because we moved `m_listener_sp` and `m_hijack_listener_sp` to 
> > `ProcessInfo` and since both `Process{Attach,Launch}Info` are derived from 
> > that class, this should also copy the listeners.
> I don't think that's true? We're taking a different subclass and initializing 
> from that, we have to be explicit about which fields are copied over, no? 
> Maybe I'm missing something here.
> 
> Either way, I'm not sure it really matters unless the `ProcessLaunchInfo` and 
> `ProcessAttachInfo` necessarily need to be in sync. Do they?
Wait, I'm being silly, you're doing `operator=` there. Okay, sorry for all the 
noise, I think I was just confused.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148395

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

[Lldb-commits] [PATCH] D148395: [lldb] Unify default/hijack listener between Process{Attach, Launch}Info (NFC)

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.



Comment at: lldb/include/lldb/Target/Process.h:122
 m_async(false) {
 ProcessInfo::operator=(launch_info);
 SetProcessPluginName(launch_info.GetProcessPluginName());

mib wrote:
> Because we moved `m_listener_sp` and `m_hijack_listener_sp` to `ProcessInfo` 
> and since both `Process{Attach,Launch}Info` are derived from that class, this 
> should also copy the listeners.
I don't think that's true? We're taking a different subclass and initializing 
from that, we have to be explicit about which fields are copied over, no? Maybe 
I'm missing something here.

Either way, I'm not sure it really matters unless the `ProcessLaunchInfo` and 
`ProcessAttachInfo` necessarily need to be in sync. Do they?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148395

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

[Lldb-commits] [PATCH] D148380: [lldb] Lock accesses to PathMappingLists's internals

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.



Comment at: lldb/source/Target/PathMappingList.cpp:72
 void PathMappingList::Append(const PathMappingList , bool notify) {
+  std::scoped_lock locks(m_mutex, rhs.m_mutex);
   ++m_mod_id;

nickdesaulniers wrote:
> ```
> /android0/llvm-project/lldb/source/Target/PathMappingList.cpp:72:3: warning: 
> 'scoped_lock' may not intend to support class template argument deduction 
> [-Wctad-maybe-unsupported]
>   std::scoped_lock locks(m_mutex, rhs.m_mutex);
>   ^
> /usr/lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/mutex:692:11: 
> note: add a deduction guide to suppress this warning
> class scoped_lock
>   ^
> ```
Fixed in `c5fc7809e05940674424aaed7dd06c6be0639864`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148380

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

[Lldb-commits] [PATCH] D145297: [lldb] Add an example of interactive scripted process debugging

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

Looks good to me. Might want to let @jingham or @JDevlieghere give it a 
look-over though.




Comment at: 
lldb/test/API/functionalities/interactive_scripted_process/interactive_scripted_process.py:9
+
+import os,json,struct,signal
+import time

mib wrote:
> bulbazord wrote:
> > nit: import these all on different lines
> why :p ?
I don't actually think it matters a ton, which is why it was a nit, but I think 
there's something to be said for consistency with the rest of lldb.


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

https://reviews.llvm.org/D145297

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

[Lldb-commits] [PATCH] D148548: [lldb] Improve breakpoint management for interactive scripted process

[Alex Langford via Phabricator via lldb-commits]

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

one nit but otherwise LGTM.




Comment at: 
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp:124
+  if (!CheckStructuredDataObject(LLVM_PRETTY_FUNCTION, obj, error))
+return {};
+

JDevlieghere wrote:
> return false
+1


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

https://reviews.llvm.org/D148548

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

[Lldb-commits] [PATCH] D148395: [lldb] Unify default/hijack listener between Process{Attach, Launch}Info (NFC)

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

In D148395#4285017 , @mib wrote:

> In D148395#4270508 , @bulbazord 
> wrote:
>
>> Creating a ProcessAttachInfo from a ProcessLaunchInfo with this change means 
>> they'll have different listeners. Is that ProcessLaunchInfo kept around for 
>> any other reason? Or is it just made to create the ProcessAttachInfo? This 
>> seems like a reasonable move to me, but I'm not sure how LaunchInfo and 
>> AttachInfo might interact.
>
> @bulbazord Not sure what you mean ... We need to convert to the 
> `ProcessLaunchInfo` into a `ProcessAttachInfo` when the user ask use to 
> launch a process but we end up asking the platform to do the launch after 
> which we attach to the process. In both cases, we use the default listener 
> (the debugger listener if the user didn't provide a custom listener).

What I'm not sure about is that the `ProcessAttachInfo` constructor we're using 
takes a `ProcessLaunchInfo` as an argument and fills in its fields with it. It 
used to be that `ProcessAttachInfo` would get its Listener and HijackListener 
from the `ProcessLaunchInfo` passed in. Now, they could have different 
Listeners and HijackListeners. When we create a `ProcessAttachInfo` from a 
`ProcessLaunchInfo`, is it expected that these 2 things will diverge in that 
way? Do we use the `ProcessLaunchInfo` that we build the `ProcessAttachInfo` 
with in any other way?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148395

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

[Lldb-commits] [PATCH] D148399: [lldb] Improve logging for process state change (NFC)

[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


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

https://reviews.llvm.org/D148399

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

[Lldb-commits] [PATCH] D148399: [lldb] Improve logging for process state change (NFC)

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

We'd go from:

  LLDB_LOGF(log,
"Process::SetPrivateState (plugin = %s, state = %s) state didn't "
"change. Ignoring...",
GetPluginName().data(), StateAsCString(new_state));

to

  LLDB_LOG(log, "(plugin = %s, state = %s) state didn't "
"change. Ignoring...",
GetPluginName().data(), StateAsCString(new_state));

`LLDB_LOG` automatically inserts the file and func into the log. There are 
plenty of places where we copy/paste logs like these into other files without 
remembering to update this piece of information, so removing it from the log 
line entirely helps prevent those kinds of issues in the future. It's a minor 
thing, but if the point of this patch is to improve the logging mechanism 
entirely, then I think we should also think about these kinds of things. See 
0a74fbb7b1d3e04ac03389f1fc455ac593c2e5ee 
 for an 
example of what I mean.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148399

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

[Lldb-commits] [PATCH] D148679: [lldb] Change setting descriptions to use StringRef instead of ConstString

[Alex Langford via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG96a800c07f02: [lldb] Change setting descriptions to use 
StringRef instead of ConstString (authored by bulbazord).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148679

Files:
  lldb/include/lldb/Core/PluginManager.h
  lldb/include/lldb/Interpreter/OptionValueProperties.h
  lldb/source/Core/Debugger.cpp
  lldb/source/Core/PluginManager.cpp
  lldb/source/Interpreter/OptionValueProperties.cpp
  lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
  lldb/source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
  lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
  lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp
  lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Target/Process.cpp
  lldb/source/Target/Target.cpp
  lldb/unittests/Interpreter/TestOptionValue.cpp

Index: lldb/unittests/Interpreter/TestOptionValue.cpp
===
--- lldb/unittests/Interpreter/TestOptionValue.cpp
+++ lldb/unittests/Interpreter/TestOptionValue.cpp
@@ -84,11 +84,10 @@
 const bool is_global = false;
 
 auto dict_sp = std::make_shared(1 << eTypeUInt64);
-props_sp->AppendProperty(ConstString("dict"), ConstString(), is_global,
- dict_sp);
+props_sp->AppendProperty(ConstString("dict"), "", is_global, dict_sp);
 
 auto file_list_sp = std::make_shared();
-props_sp->AppendProperty(ConstString("file-list"), ConstString(), is_global,
+props_sp->AppendProperty(ConstString("file-list"), "", is_global,
  file_list_sp);
 return props_sp;
   }
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -4088,8 +4088,8 @@
 std::make_unique();
 m_collection_sp->AppendProperty(
 ConstString(Properties::GetExperimentalSettingsName()),
-ConstString("Experimental settings - setting these won't produce "
-"errors if the setting is not present."),
+"Experimental settings - setting these won't produce "
+"errors if the setting is not present.",
 true, m_experimental_properties_up->GetValueProperties());
   } else {
 m_collection_sp =
@@ -4099,12 +4099,12 @@
 std::make_unique();
 m_collection_sp->AppendProperty(
 ConstString(Properties::GetExperimentalSettingsName()),
-ConstString("Experimental settings - setting these won't produce "
-"errors if the setting is not present."),
+"Experimental settings - setting these won't produce "
+"errors if the setting is not present.",
 true, m_experimental_properties_up->GetValueProperties());
 m_collection_sp->AppendProperty(
-ConstString("process"), ConstString("Settings specific to processes."),
-true, Process::GetGlobalProperties().GetValueProperties());
+ConstString("process"), "Settings specific to processes.", true,
+Process::GetGlobalProperties().GetValueProperties());
 m_collection_sp->SetValueChangedCallback(
 ePropertySaveObjectsDir, [this] { CheckJITObjectsDir(); });
   }
Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -167,8 +167,8 @@
 std::make_shared(ConstString("process"));
 m_collection_sp->Initialize(g_process_properties);
 m_collection_sp->AppendProperty(
-ConstString("thread"), ConstString("Settings specific to threads."),
-true, Thread::GetGlobalProperties().GetValueProperties());
+ConstString("thread"), "Settings specific to threads.", true,
+Thread::GetGlobalProperties().GetValueProperties());
   } else {
 m_collection_sp =
 OptionValueProperties::CreateLocalCopy(Process::GetGlobalProperties());
@@ -181,8 +181,8 @@
   std::make_unique();
   m_collection_sp->AppendProperty(
   ConstString(Properties::GetExperimentalSettingsName()),
-  ConstString("Experimental settings - setting these won't produce "
-  "errors if the setting is not present."),
+  "Experimental settings - setting these won't produce "
+  "errors if the setting is not present.",
   true, m_experimental_properties_up->GetValueProperties());
 }
 
Index: 

[Lldb-commits] [PATCH] D148676: [lldb][NFCI] Stop creating additional temporary string in Log::VAPrintf

[Alex Langford via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG77e3914be7c9: [lldb][NFCI] Stop creating additional 
temporary string in Log::VAPrintf (authored by bulbazord).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148676

Files:
  lldb/include/lldb/Utility/Log.h
  lldb/source/Utility/Log.cpp


Index: lldb/source/Utility/Log.cpp
===
--- lldb/source/Utility/Log.cpp
+++ lldb/source/Utility/Log.cpp
@@ -146,8 +146,8 @@
 // callback registered, then we call the logging callback. If we have a valid
 // file handle, we also log to the file.
 void Log::VAPrintf(const char *format, va_list args) {
-  llvm::SmallString<64> FinalMessage;
-  llvm::raw_svector_ostream Stream(FinalMessage);
+  std::string FinalMessage;
+  llvm::raw_string_ostream Stream(FinalMessage);
   WriteHeader(Stream, "", "");
 
   llvm::SmallString<64> Content;
@@ -155,7 +155,7 @@
 
   Stream << Content << "\n";
 
-  WriteMessage(std::string(FinalMessage.str()));
+  WriteMessage(FinalMessage);
 }
 
 // Printing of errors that are not fatal.
@@ -344,7 +344,7 @@
   }
 }
 
-void Log::WriteMessage(const std::string ) {
+void Log::WriteMessage(llvm::StringRef message) {
   // Make a copy of our stream shared pointer in case someone disables our log
   // while we are logging and releases the stream
   auto handler_sp = GetHandler();
Index: lldb/include/lldb/Utility/Log.h
===
--- lldb/include/lldb/Utility/Log.h
+++ lldb/include/lldb/Utility/Log.h
@@ -265,7 +265,7 @@
 
   void WriteHeader(llvm::raw_ostream , llvm::StringRef file,
llvm::StringRef function);
-  void WriteMessage(const std::string );
+  void WriteMessage(llvm::StringRef message);
 
   void Format(llvm::StringRef file, llvm::StringRef function,
   const llvm::formatv_object_base );


Index: lldb/source/Utility/Log.cpp
===
--- lldb/source/Utility/Log.cpp
+++ lldb/source/Utility/Log.cpp
@@ -146,8 +146,8 @@
 // callback registered, then we call the logging callback. If we have a valid
 // file handle, we also log to the file.
 void Log::VAPrintf(const char *format, va_list args) {
-  llvm::SmallString<64> FinalMessage;
-  llvm::raw_svector_ostream Stream(FinalMessage);
+  std::string FinalMessage;
+  llvm::raw_string_ostream Stream(FinalMessage);
   WriteHeader(Stream, "", "");
 
   llvm::SmallString<64> Content;
@@ -155,7 +155,7 @@
 
   Stream << Content << "\n";
 
-  WriteMessage(std::string(FinalMessage.str()));
+  WriteMessage(FinalMessage);
 }
 
 // Printing of errors that are not fatal.
@@ -344,7 +344,7 @@
   }
 }
 
-void Log::WriteMessage(const std::string ) {
+void Log::WriteMessage(llvm::StringRef message) {
   // Make a copy of our stream shared pointer in case someone disables our log
   // while we are logging and releases the stream
   auto handler_sp = GetHandler();
Index: lldb/include/lldb/Utility/Log.h
===
--- lldb/include/lldb/Utility/Log.h
+++ lldb/include/lldb/Utility/Log.h
@@ -265,7 +265,7 @@
 
   void WriteHeader(llvm::raw_ostream , llvm::StringRef file,
llvm::StringRef function);
-  void WriteMessage(const std::string );
+  void WriteMessage(llvm::StringRef message);
 
   void Format(llvm::StringRef file, llvm::StringRef function,
   const llvm::formatv_object_base );
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D148679: [lldb] Change setting descriptions to use StringRef instead of ConstString

[Alex Langford via Phabricator via lldb-commits]

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

These probably do not need to be in the ConstString StringPool as they
don't really need any of the advantages that ConstStrings offer.
Lifetime for these things is always static and we never need to perform
comparisons for setting descriptions.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D148679

Files:
  lldb/include/lldb/Core/PluginManager.h
  lldb/include/lldb/Interpreter/OptionValueProperties.h
  lldb/source/Core/Debugger.cpp
  lldb/source/Core/PluginManager.cpp
  lldb/source/Interpreter/OptionValueProperties.cpp
  lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
  lldb/source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
  lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
  lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp
  lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Target/Process.cpp
  lldb/source/Target/Target.cpp
  lldb/unittests/Interpreter/TestOptionValue.cpp

Index: lldb/unittests/Interpreter/TestOptionValue.cpp
===
--- lldb/unittests/Interpreter/TestOptionValue.cpp
+++ lldb/unittests/Interpreter/TestOptionValue.cpp
@@ -84,11 +84,10 @@
 const bool is_global = false;
 
 auto dict_sp = std::make_shared(1 << eTypeUInt64);
-props_sp->AppendProperty(ConstString("dict"), ConstString(), is_global,
- dict_sp);
+props_sp->AppendProperty(ConstString("dict"), "", is_global, dict_sp);
 
 auto file_list_sp = std::make_shared();
-props_sp->AppendProperty(ConstString("file-list"), ConstString(), is_global,
+props_sp->AppendProperty(ConstString("file-list"), "", is_global,
  file_list_sp);
 return props_sp;
   }
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -4088,8 +4088,8 @@
 std::make_unique();
 m_collection_sp->AppendProperty(
 ConstString(Properties::GetExperimentalSettingsName()),
-ConstString("Experimental settings - setting these won't produce "
-"errors if the setting is not present."),
+"Experimental settings - setting these won't produce "
+"errors if the setting is not present.",
 true, m_experimental_properties_up->GetValueProperties());
   } else {
 m_collection_sp =
@@ -4099,12 +4099,12 @@
 std::make_unique();
 m_collection_sp->AppendProperty(
 ConstString(Properties::GetExperimentalSettingsName()),
-ConstString("Experimental settings - setting these won't produce "
-"errors if the setting is not present."),
+"Experimental settings - setting these won't produce "
+"errors if the setting is not present.",
 true, m_experimental_properties_up->GetValueProperties());
 m_collection_sp->AppendProperty(
-ConstString("process"), ConstString("Settings specific to processes."),
-true, Process::GetGlobalProperties().GetValueProperties());
+ConstString("process"), "Settings specific to processes.", true,
+Process::GetGlobalProperties().GetValueProperties());
 m_collection_sp->SetValueChangedCallback(
 ePropertySaveObjectsDir, [this] { CheckJITObjectsDir(); });
   }
Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -167,8 +167,8 @@
 std::make_shared(ConstString("process"));
 m_collection_sp->Initialize(g_process_properties);
 m_collection_sp->AppendProperty(
-ConstString("thread"), ConstString("Settings specific to threads."),
-true, Thread::GetGlobalProperties().GetValueProperties());
+ConstString("thread"), "Settings specific to threads.", true,
+Thread::GetGlobalProperties().GetValueProperties());
   } else {
 m_collection_sp =
 OptionValueProperties::CreateLocalCopy(Process::GetGlobalProperties());
@@ -181,8 +181,8 @@
   std::make_unique();
   m_collection_sp->AppendProperty(
   ConstString(Properties::GetExperimentalSettingsName()),
-  ConstString("Experimental settings - setting these won't produce "
-  "errors if the setting is not 

[Lldb-commits] [PATCH] D148400: [lldb] Fix bug to update process public run lock with process state

[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


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

https://reviews.llvm.org/D148400

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

[Lldb-commits] [PATCH] D148676: [lldb][NFCI] Stop creating additional temporary string in Log::VAPrintf

[Alex Langford via Phabricator via lldb-commits]

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

Instead of creating a std::string from the `SmallString`,
let's just use a std::string from the start. I initially tried to make
`SmallString` work but getting it right proved complicated because
`LogHandler::Emit` will take its `StringRef` parameter and touch the raw
`const char *` from it directly, which isn't guaranteed to be
null-terminated with a `SmallString`.

I changed `WriteMessage` to take a `StringRef` instead of a
`const std::string &` for flexibility.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D148676

Files:
  lldb/include/lldb/Utility/Log.h
  lldb/source/Utility/Log.cpp


Index: lldb/source/Utility/Log.cpp
===
--- lldb/source/Utility/Log.cpp
+++ lldb/source/Utility/Log.cpp
@@ -146,8 +146,8 @@
 // callback registered, then we call the logging callback. If we have a valid
 // file handle, we also log to the file.
 void Log::VAPrintf(const char *format, va_list args) {
-  llvm::SmallString<64> FinalMessage;
-  llvm::raw_svector_ostream Stream(FinalMessage);
+  std::string FinalMessage;
+  llvm::raw_string_ostream Stream(FinalMessage);
   WriteHeader(Stream, "", "");
 
   llvm::SmallString<64> Content;
@@ -155,7 +155,7 @@
 
   Stream << Content << "\n";
 
-  WriteMessage(std::string(FinalMessage.str()));
+  WriteMessage(FinalMessage);
 }
 
 // Printing of errors that are not fatal.
@@ -344,7 +344,7 @@
   }
 }
 
-void Log::WriteMessage(const std::string ) {
+void Log::WriteMessage(llvm::StringRef message) {
   // Make a copy of our stream shared pointer in case someone disables our log
   // while we are logging and releases the stream
   auto handler_sp = GetHandler();
Index: lldb/include/lldb/Utility/Log.h
===
--- lldb/include/lldb/Utility/Log.h
+++ lldb/include/lldb/Utility/Log.h
@@ -265,7 +265,7 @@
 
   void WriteHeader(llvm::raw_ostream , llvm::StringRef file,
llvm::StringRef function);
-  void WriteMessage(const std::string );
+  void WriteMessage(llvm::StringRef message);
 
   void Format(llvm::StringRef file, llvm::StringRef function,
   const llvm::formatv_object_base );


Index: lldb/source/Utility/Log.cpp
===
--- lldb/source/Utility/Log.cpp
+++ lldb/source/Utility/Log.cpp
@@ -146,8 +146,8 @@
 // callback registered, then we call the logging callback. If we have a valid
 // file handle, we also log to the file.
 void Log::VAPrintf(const char *format, va_list args) {
-  llvm::SmallString<64> FinalMessage;
-  llvm::raw_svector_ostream Stream(FinalMessage);
+  std::string FinalMessage;
+  llvm::raw_string_ostream Stream(FinalMessage);
   WriteHeader(Stream, "", "");
 
   llvm::SmallString<64> Content;
@@ -155,7 +155,7 @@
 
   Stream << Content << "\n";
 
-  WriteMessage(std::string(FinalMessage.str()));
+  WriteMessage(FinalMessage);
 }
 
 // Printing of errors that are not fatal.
@@ -344,7 +344,7 @@
   }
 }
 
-void Log::WriteMessage(const std::string ) {
+void Log::WriteMessage(llvm::StringRef message) {
   // Make a copy of our stream shared pointer in case someone disables our log
   // while we are logging and releases the stream
   auto handler_sp = GetHandler();
Index: lldb/include/lldb/Utility/Log.h
===
--- lldb/include/lldb/Utility/Log.h
+++ lldb/include/lldb/Utility/Log.h
@@ -265,7 +265,7 @@
 
   void WriteHeader(llvm::raw_ostream , llvm::StringRef file,
llvm::StringRef function);
-  void WriteMessage(const std::string );
+  void WriteMessage(llvm::StringRef message);
 
   void Format(llvm::StringRef file, llvm::StringRef function,
   const llvm::formatv_object_base );
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D148579: [lldb] Change parameter type of StructuredData::ParseJSON

[Alex Langford via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG8bddb13c2470: [lldb] Change parameter type of 
StructuredData::ParseJSON (authored by bulbazord).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148579

Files:
  lldb/include/lldb/Utility/StructuredData.h
  lldb/source/API/SBDebugger.cpp
  lldb/source/API/SBStructuredData.cpp
  lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/source/Utility/StructuredData.cpp
  lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
  lldb/unittests/tools/lldb-server/tests/MessageObjects.cpp

Index: lldb/unittests/tools/lldb-server/tests/MessageObjects.cpp
===
--- lldb/unittests/tools/lldb-server/tests/MessageObjects.cpp
+++ lldb/unittests/tools/lldb-server/tests/MessageObjects.cpp
@@ -96,8 +96,7 @@
 ArrayRef RegInfos) {
   JThreadsInfo jthreads_info;
 
-  StructuredData::ObjectSP json =
-  StructuredData::ParseJSON(std::string(Response));
+  StructuredData::ObjectSP json = StructuredData::ParseJSON(Response);
   StructuredData::Array *array = json->GetAsArray();
   if (!array)
 return make_parsing_error("JThreadsInfo: JSON array");
Index: lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
===
--- lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
+++ lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
@@ -287,7 +287,7 @@
   server_thread.join();
 
   GTEST_LOG_(INFO) << "Formatted output: " << ss.GetData();
-  auto object_sp = StructuredData::ParseJSON(std::string(ss.GetString()));
+  auto object_sp = StructuredData::ParseJSON(ss.GetString());
   ASSERT_TRUE(bool(object_sp));
   auto dict_sp = object_sp->GetAsDictionary();
   ASSERT_TRUE(bool(dict_sp));
Index: lldb/source/Utility/StructuredData.cpp
===
--- lldb/source/Utility/StructuredData.cpp
+++ lldb/source/Utility/StructuredData.cpp
@@ -21,8 +21,7 @@
 static StructuredData::ObjectSP ParseJSONObject(json::Object *object);
 static StructuredData::ObjectSP ParseJSONArray(json::Array *array);
 
-StructuredData::ObjectSP
-StructuredData::ParseJSON(const std::string _text) {
+StructuredData::ObjectSP StructuredData::ParseJSON(llvm::StringRef json_text) {
   llvm::Expected value = json::parse(json_text);
   if (!value) {
 llvm::consumeError(value.takeError());
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -3781,8 +3781,7 @@
   response.GetResponseType();
   if (response_type == StringExtractorGDBRemote::eResponse) {
 if (!response.Empty()) {
-  object_sp =
-  StructuredData::ParseJSON(std::string(response.GetStringRef()));
+  object_sp = StructuredData::ParseJSON(response.GetStringRef());
 }
   }
 }
@@ -3853,8 +3852,7 @@
   response.GetResponseType();
   if (response_type == StringExtractorGDBRemote::eResponse) {
 if (!response.Empty()) {
-  object_sp =
-  StructuredData::ParseJSON(std::string(response.GetStringRef()));
+  object_sp = StructuredData::ParseJSON(response.GetStringRef());
 }
   }
 }
@@ -3876,8 +3874,7 @@
   response.GetResponseType();
   if (response_type == StringExtractorGDBRemote::eResponse) {
 if (!response.Empty()) {
-  object_sp =
-  StructuredData::ParseJSON(std::string(response.GetStringRef()));
+  object_sp = StructuredData::ParseJSON(response.GetStringRef());
 }
   }
 }
@@ -3909,8 +3906,7 @@
   response.GetResponseType();
   if (response_type == StringExtractorGDBRemote::eResponse) {
 if (!response.Empty()) {
-  object_sp =
-  StructuredData::ParseJSON(std::string(response.GetStringRef()));
+  object_sp = StructuredData::ParseJSON(response.GetStringRef());
 }
   }
 }
@@ -5087,8 +5083,7 @@
   }
 
   // This is an asynchronous JSON packet, destined for a StructuredDataPlugin.
-  StructuredData::ObjectSP json_sp =
-  StructuredData::ParseJSON(std::string(packet));
+  StructuredData::ObjectSP json_sp = StructuredData::ParseJSON(packet);
   if (log) {
 if (json_sp) {
   StreamString json_str;
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp

[Lldb-commits] [PATCH] D148579: [lldb] Change parameter type of StructuredData::ParseJSON

[Alex Langford via Phabricator via lldb-commits]

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

Instead of taking a `const std::string &` we can take an
`llvm::StringRef`. The motivation for this change is that many of the
callers of `ParseJSON` end up creating a temporary `std::string` from an 
existing
`StringRef` or `const char *` in order to satisfy the API. There's no
reason we need to do this.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D148579

Files:
  lldb/include/lldb/Utility/StructuredData.h
  lldb/source/API/SBDebugger.cpp
  lldb/source/API/SBStructuredData.cpp
  lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/source/Utility/StructuredData.cpp
  lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
  lldb/unittests/tools/lldb-server/tests/MessageObjects.cpp

Index: lldb/unittests/tools/lldb-server/tests/MessageObjects.cpp
===
--- lldb/unittests/tools/lldb-server/tests/MessageObjects.cpp
+++ lldb/unittests/tools/lldb-server/tests/MessageObjects.cpp
@@ -96,8 +96,7 @@
 ArrayRef RegInfos) {
   JThreadsInfo jthreads_info;
 
-  StructuredData::ObjectSP json =
-  StructuredData::ParseJSON(std::string(Response));
+  StructuredData::ObjectSP json = StructuredData::ParseJSON(Response);
   StructuredData::Array *array = json->GetAsArray();
   if (!array)
 return make_parsing_error("JThreadsInfo: JSON array");
Index: lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
===
--- lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
+++ lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
@@ -287,7 +287,7 @@
   server_thread.join();
 
   GTEST_LOG_(INFO) << "Formatted output: " << ss.GetData();
-  auto object_sp = StructuredData::ParseJSON(std::string(ss.GetString()));
+  auto object_sp = StructuredData::ParseJSON(ss.GetString());
   ASSERT_TRUE(bool(object_sp));
   auto dict_sp = object_sp->GetAsDictionary();
   ASSERT_TRUE(bool(dict_sp));
Index: lldb/source/Utility/StructuredData.cpp
===
--- lldb/source/Utility/StructuredData.cpp
+++ lldb/source/Utility/StructuredData.cpp
@@ -21,8 +21,7 @@
 static StructuredData::ObjectSP ParseJSONObject(json::Object *object);
 static StructuredData::ObjectSP ParseJSONArray(json::Array *array);
 
-StructuredData::ObjectSP
-StructuredData::ParseJSON(const std::string _text) {
+StructuredData::ObjectSP StructuredData::ParseJSON(llvm::StringRef json_text) {
   llvm::Expected value = json::parse(json_text);
   if (!value) {
 llvm::consumeError(value.takeError());
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -3781,8 +3781,7 @@
   response.GetResponseType();
   if (response_type == StringExtractorGDBRemote::eResponse) {
 if (!response.Empty()) {
-  object_sp =
-  StructuredData::ParseJSON(std::string(response.GetStringRef()));
+  object_sp = StructuredData::ParseJSON(response.GetStringRef());
 }
   }
 }
@@ -3853,8 +3852,7 @@
   response.GetResponseType();
   if (response_type == StringExtractorGDBRemote::eResponse) {
 if (!response.Empty()) {
-  object_sp =
-  StructuredData::ParseJSON(std::string(response.GetStringRef()));
+  object_sp = StructuredData::ParseJSON(response.GetStringRef());
 }
   }
 }
@@ -3876,8 +3874,7 @@
   response.GetResponseType();
   if (response_type == StringExtractorGDBRemote::eResponse) {
 if (!response.Empty()) {
-  object_sp =
-  StructuredData::ParseJSON(std::string(response.GetStringRef()));
+  object_sp = StructuredData::ParseJSON(response.GetStringRef());
 }
   }
 }
@@ -3909,8 +3906,7 @@
   response.GetResponseType();
   if (response_type == StringExtractorGDBRemote::eResponse) {
 if (!response.Empty()) {
-  object_sp =
-  StructuredData::ParseJSON(std::string(response.GetStringRef()));
+  object_sp = StructuredData::ParseJSON(response.GetStringRef());
 }
   }
 }
@@ -5087,8 +5083,7 @@
   }
 
   // This is an asynchronous JSON packet, destined for a StructuredDataPlugin.
-  StructuredData::ObjectSP json_sp =
-  

[Lldb-commits] [PATCH] D148402: [lldb] Remove use of ConstString from Args::GetShellSafeArgument

[Alex Langford via Phabricator via lldb-commits]

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148402

Files:
  lldb/source/Utility/Args.cpp


Index: lldb/source/Utility/Args.cpp
===
--- lldb/source/Utility/Args.cpp
+++ lldb/source/Utility/Args.cpp
@@ -7,7 +7,6 @@
 
//===--===//
 
 #include "lldb/Utility/Args.h"
-#include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Stream.h"
 #include "lldb/Utility/StringList.h"
@@ -385,20 +384,21 @@
 std::string Args::GetShellSafeArgument(const FileSpec ,
llvm::StringRef unsafe_arg) {
   struct ShellDescriptor {
-ConstString m_basename;
+llvm::StringRef m_basename;
 llvm::StringRef m_escapables;
   };
 
-  static ShellDescriptor g_Shells[] = {{ConstString("bash"), " '\"<>()&;"},
-   {ConstString("fish"), " '\"<>()&\\|;"},
-   {ConstString("tcsh"), " '\"<>()&;"},
-   {ConstString("zsh"), " '\"<>()&;\\|"},
-   {ConstString("sh"), " '\"<>()&;"}};
+  static ShellDescriptor g_Shells[] = {{"bash", " '\"<>()&;"},
+   {"fish", " '\"<>()&\\|;"},
+   {"tcsh", " '\"<>()&;"},
+   {"zsh", " '\"<>()&;\\|"},
+   {"sh", " '\"<>()&;"}};
 
   // safe minimal set
   llvm::StringRef escapables = " '\"";
 
-  if (auto basename = shell.GetFilename()) {
+  auto basename = shell.GetFilename().GetStringRef();
+  if (!basename.empty()) {
 for (const auto  : g_Shells) {
   if (Shell.m_basename == basename) {
 escapables = Shell.m_escapables;


Index: lldb/source/Utility/Args.cpp
===
--- lldb/source/Utility/Args.cpp
+++ lldb/source/Utility/Args.cpp
@@ -7,7 +7,6 @@
 //===--===//
 
 #include "lldb/Utility/Args.h"
-#include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Stream.h"
 #include "lldb/Utility/StringList.h"
@@ -385,20 +384,21 @@
 std::string Args::GetShellSafeArgument(const FileSpec ,
llvm::StringRef unsafe_arg) {
   struct ShellDescriptor {
-ConstString m_basename;
+llvm::StringRef m_basename;
 llvm::StringRef m_escapables;
   };
 
-  static ShellDescriptor g_Shells[] = {{ConstString("bash"), " '\"<>()&;"},
-   {ConstString("fish"), " '\"<>()&\\|;"},
-   {ConstString("tcsh"), " '\"<>()&;"},
-   {ConstString("zsh"), " '\"<>()&;\\|"},
-   {ConstString("sh"), " '\"<>()&;"}};
+  static ShellDescriptor g_Shells[] = {{"bash", " '\"<>()&;"},
+   {"fish", " '\"<>()&\\|;"},
+   {"tcsh", " '\"<>()&;"},
+   {"zsh", " '\"<>()&;\\|"},
+   {"sh", " '\"<>()&;"}};
 
   // safe minimal set
   llvm::StringRef escapables = " '\"";
 
-  if (auto basename = shell.GetFilename()) {
+  auto basename = shell.GetFilename().GetStringRef();
+  if (!basename.empty()) {
 for (const auto  : g_Shells) {
   if (Shell.m_basename == basename) {
 escapables = Shell.m_escapables;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D148380: [lldb] Lock accesses to PathMappingLists's internals

[Alex Langford via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG43ac269bdd00: [lldb] Lock accesses to 
PathMappingListss internals (authored by bulbazord).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148380

Files:
  lldb/include/lldb/Target/PathMappingList.h
  lldb/source/Target/PathMappingList.cpp

Index: lldb/source/Target/PathMappingList.cpp
===
--- lldb/source/Target/PathMappingList.cpp
+++ lldb/source/Target/PathMappingList.cpp
@@ -48,6 +48,7 @@
 
 const PathMappingList ::operator=(const PathMappingList ) {
   if (this != ) {
+std::scoped_lock locks(m_mutex, rhs.m_mutex);
 m_pairs = rhs.m_pairs;
 m_callback = nullptr;
 m_callback_baton = nullptr;
@@ -60,6 +61,7 @@
 
 void PathMappingList::Append(llvm::StringRef path, llvm::StringRef replacement,
  bool notify) {
+  std::lock_guard lock(m_mutex);
   ++m_mod_id;
   m_pairs.emplace_back(pair(NormalizePath(path), NormalizePath(replacement)));
   if (notify && m_callback)
@@ -67,6 +69,7 @@
 }
 
 void PathMappingList::Append(const PathMappingList , bool notify) {
+  std::scoped_lock locks(m_mutex, rhs.m_mutex);
   ++m_mod_id;
   if (!rhs.m_pairs.empty()) {
 const_iterator pos, end = rhs.m_pairs.end();
@@ -81,6 +84,7 @@
llvm::StringRef replacement, bool notify) {
   auto normalized_path = NormalizePath(path);
   auto normalized_replacement = NormalizePath(replacement);
+  std::lock_guard lock(m_mutex);
   for (const auto  : m_pairs) {
 if (pair.first.GetStringRef().equals(normalized_path) &&
 pair.second.GetStringRef().equals(normalized_replacement))
@@ -92,6 +96,7 @@
 
 void PathMappingList::Insert(llvm::StringRef path, llvm::StringRef replacement,
  uint32_t index, bool notify) {
+  std::lock_guard lock(m_mutex);
   ++m_mod_id;
   iterator insert_iter;
   if (index >= m_pairs.size())
@@ -106,6 +111,7 @@
 
 bool PathMappingList::Replace(llvm::StringRef path, llvm::StringRef replacement,
   uint32_t index, bool notify) {
+  std::lock_guard lock(m_mutex);
   if (index >= m_pairs.size())
 return false;
   ++m_mod_id;
@@ -116,6 +122,7 @@
 }
 
 bool PathMappingList::Remove(size_t index, bool notify) {
+  std::lock_guard lock(m_mutex);
   if (index >= m_pairs.size())
 return false;
 
@@ -130,6 +137,7 @@
 // For clients which do not need the pair index dumped, pass a pair_index >= 0
 // to only dump the indicated pair.
 void PathMappingList::Dump(Stream *s, int pair_index) {
+  std::lock_guard lock(m_mutex);
   unsigned int numPairs = m_pairs.size();
 
   if (pair_index < 0) {
@@ -147,6 +155,7 @@
 
 llvm::json::Value PathMappingList::ToJSON() {
   llvm::json::Array entries;
+  std::lock_guard lock(m_mutex);
   for (const auto  : m_pairs) {
 llvm::json::Array entry{pair.first.GetStringRef().str(),
 pair.second.GetStringRef().str()};
@@ -156,6 +165,7 @@
 }
 
 void PathMappingList::Clear(bool notify) {
+  std::lock_guard lock(m_mutex);
   if (!m_pairs.empty())
 ++m_mod_id;
   m_pairs.clear();
@@ -186,6 +196,7 @@
 
 std::optional PathMappingList::RemapPath(llvm::StringRef mapping_path,
bool only_if_exists) const {
+  std::lock_guard lock(m_mutex);
   if (m_pairs.empty() || mapping_path.empty())
 return {};
   LazyBool path_is_relative = eLazyBoolCalculate;
@@ -224,6 +235,7 @@
 PathMappingList::ReverseRemapPath(const FileSpec , FileSpec ) const {
   std::string path = file.GetPath();
   llvm::StringRef path_ref(path);
+  std::lock_guard lock(m_mutex);
   for (const auto  : m_pairs) {
 llvm::StringRef removed_prefix = it.second.GetStringRef();
 if (!path_ref.consume_front(it.second.GetStringRef()))
@@ -252,6 +264,7 @@
 
 bool PathMappingList::Replace(llvm::StringRef path, llvm::StringRef new_path,
   bool notify) {
+  std::lock_guard lock(m_mutex);
   uint32_t idx = FindIndexForPath(path);
   if (idx < m_pairs.size()) {
 ++m_mod_id;
@@ -264,6 +277,7 @@
 }
 
 bool PathMappingList::Remove(ConstString path, bool notify) {
+  std::lock_guard lock(m_mutex);
   iterator pos = FindIteratorForPath(path);
   if (pos != m_pairs.end()) {
 ++m_mod_id;
@@ -277,6 +291,7 @@
 
 PathMappingList::const_iterator
 PathMappingList::FindIteratorForPath(ConstString path) const {
+  std::lock_guard lock(m_mutex);
   const_iterator pos;
   const_iterator begin = m_pairs.begin();
   const_iterator end = m_pairs.end();
@@ -290,6 +305,7 @@
 
 PathMappingList::iterator
 PathMappingList::FindIteratorForPath(ConstString path) {
+  std::lock_guard lock(m_mutex);
   iterator pos;
   iterator begin = m_pairs.begin();
   iterator end = m_pairs.end();
@@ -303,6 +319,7 @@
 
 bool PathMappingList::GetPathsAtIndex(uint32_t idx, 

[Lldb-commits] [PATCH] D148548: [lldb] Improve breakpoint management for interactive scripted process

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

Seems okay to me, but it's a little messy that we're having to manage 
breakpoints like this.




Comment at: lldb/bindings/interface/SBTargetExtensions.i:144-171
+class watchpoints_access(object):
+'''A helper object that will lazily hand out watchpoints for a 
target when supplied an index.'''
+def __init__(self, sbtarget):
+self.sbtarget = sbtarget
+
+def __len__(self):
+if self.sbtarget:

Are these used at all?



Comment at: 
lldb/test/API/functionalities/interactive_scripted_process/interactive_scripted_process.py:269-278
+bkpt_file = lldb.SBFileSpec(tf.name)
+error = self.driving_target.BreakpointsWriteToFile(bkpt_file)
+if error.Fail():
+log("Failed to save breakpoints from driving target (%s)"
+% error.GetCString())
+bkpts_list = lldb.SBBreakpointList(self.target)
+error = self.target.BreakpointsCreateFromFile(bkpt_file, 
bkpts_list)

It's interesting that we dump to a file. It'd be cool if we could dump it to a 
StructuredData or something instead of a file.



Comment at: 
lldb/test/API/functionalities/interactive_scripted_process/interactive_scripted_process.py:282
+if error.Success():
+self.driving_target.DeleteAllBreakpoints()
+for bkpt in self.target.breakpoints:

Why do we delete all of the breakpoints just to re-set them afterwards? Is 
there a difference between what we set and what was there before?


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

https://reviews.llvm.org/D148548

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

[Lldb-commits] [PATCH] D145297: [lldb] Add an example of interactive scripted process debugging

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

A few high-level comments:

- You're doing a lot of type-annotations in python which is good, but you're 
not being very consistent about it. It would be tremendously helpful if you 
could add type annotations everywhere.
- I would recommend using f-strings over `%` and `.format` for string 
interpolation. They are available as of python 3.6 and the minimum version to 
use `lit` is 3.6 (if LLVM's CMakeLists.txt is to be believed).






Comment at: 
lldb/test/API/functionalities/interactive_scripted_process/TestInteractiveScriptedProcess.py:2
+"""
+Test the functionality of scripted processes
+"""

nit



Comment at: 
lldb/test/API/functionalities/interactive_scripted_process/TestInteractiveScriptedProcess.py:8
+from lldbsuite.test.lldbtest import *
+import os
+

nit: import json



Comment at: 
lldb/test/API/functionalities/interactive_scripted_process/TestInteractiveScriptedProcess.py:54
+
+mux_class = self.script_module + "." + "MultiplexerScriptedProcess"
+script_dict = {'driving_target_idx' : real_target_id}





Comment at: 
lldb/test/API/functionalities/interactive_scripted_process/TestInteractiveScriptedProcess.py:60-61
+
+self.runCmd("log enable lldb state -f /tmp/state.log")
+# self.runCmd("log enable lldb event -f /tmp/event.log")
+self.dbg.SetAsync(True)

Remove these log lines, I don't think they're needed for the test.



Comment at: 
lldb/test/API/functionalities/interactive_scripted_process/TestInteractiveScriptedProcess.py:89
+self.assertEqual(len(real_process.threads), len(mux_process.threads), 
"Same number of threads")
+for id in range(0, len(real_process.threads)):
+real_pc = real_process.threads[id].frame[0].pc

Defaults to 0 if you don't specify a start.



Comment at: 
lldb/test/API/functionalities/interactive_scripted_process/TestInteractiveScriptedProcess.py:102-103
+self.assertState(lldb.SBProcess.GetStateFromEvent(event), 
lldb.eStateRunning)
+event = lldbutil.fetch_next_event(self, mux_process_listener, 
mux_process.GetBroadcaster())
+self.assertState(lldb.SBProcess.GetStateFromEvent(event), 
lldb.eStateStopped)
+

Did you mean to get info from the `mux_process_listener` twice? Was one of 
these supposed to be for the real process?



Comment at: 
lldb/test/API/functionalities/interactive_scripted_process/interactive_scripted_process.py:9
+
+import os,json,struct,signal
+import time

nit: import these all on different lines



Comment at: 
lldb/test/API/functionalities/interactive_scripted_process/interactive_scripted_process.py:74-89
+def write_memory_at_address(self, addr, data, error):
+return self.driving_process.WriteMemory(addr,
+bytearray(data.uint8.all()),
+error)
+
+def get_loaded_images(self):
+return self.loaded_images

Some functions have their parameter types and return types annotated but others 
don't. Can you add all the annotations to be consistent?



Comment at: 
lldb/test/API/functionalities/interactive_scripted_process/interactive_scripted_process.py:89
+def get_scripted_thread_plugin(self):
+return PassthruScriptedThread.__module__ + "." + 
PassthruScriptedThread.__name__
+





Comment at: 
lldb/test/API/functionalities/interactive_scripted_process/interactive_scripted_process.py:132
+def get_scripted_thread_plugin(self):
+return MultiplexedScriptedThread.__module__ + "." + 
MultiplexedScriptedThread.__name__
+





Comment at: 
lldb/test/API/functionalities/interactive_scripted_process/interactive_scripted_process.py:165
+def get_name(self) -> str:
+return PassthruScriptedThread.__name__ + ".thread-" + str(self.idx)
+





Comment at: 
lldb/test/API/functionalities/interactive_scripted_process/interactive_scripted_process.py:168
+def get_stop_reason(self) -> Dict[str, Any]:
+stop_reason = { "type": lldb.eStopReasonInvalid, "data": {  }}
+





Comment at: 
lldb/test/API/functionalities/interactive_scripted_process/interactive_scripted_process.py:204
+
+return struct.pack("{}Q".format(len(self.register_ctx)), 
*self.register_ctx.values())
+





Comment at: 
lldb/test/API/functionalities/interactive_scripted_process/interactive_scripted_process.py:209
+parity = "Odd" if self.scripted_process.parity % 2 else "Even"
+return parity + MultiplexedScriptedThread.__name__ + ".thread-" + 
str(self.idx)
+

[Lldb-commits] [PATCH] D148401: [lldb/API] Add convenience constructor for SBError (NFC)

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.



Comment at: lldb/include/lldb/API/SBError.h:26
 
+  SBError(const char *message);
+

JDevlieghere wrote:
> Why not a StringRef?
I'm not sure we have `StringRef` type map in swig? We would have to create one 
for swig to pick up on it correctly, no?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148401

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

[Lldb-commits] [PATCH] D148401: [lldb/API] Add convenience constructor for SBError (NFC)

[Alex Langford via Phabricator via lldb-commits]

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

I like this! Reading through the call stack, it looks like we shouldn't hit any 
issues if `message` is `nullptr`. Could you add a small test to 
`TestSBError.py` constructing an error with a message directly?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148401

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

[Lldb-commits] [PATCH] D145296: [lldb/Plugin] Add breakpoint setting support to ScriptedProcesses.

[Alex Langford via Phabricator via lldb-commits]

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

Seems ok to me, one small thought. Shouldn't hold this patch up though.




Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:270-271
 
+Status ScriptedProcess::EnableBreakpointSite(BreakpointSite *bp_site) {
+  assert(bp_site != nullptr);
+

I like the assert, but I can't help but wonder if we should change the 
signature of this method? Something like `EnableBreakpointSite(BreakpointSite 
_site)` or something like this... Seems like the other overrides assume that 
it is not nullptr before using it and all the callsites verify it before 
calling it too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145296

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

[Lldb-commits] [PATCH] D148400: [lldb] Fix bug to update process public run lock with process state

[Alex Langford via Phabricator via lldb-commits]

bulbazord requested changes to this revision.
bulbazord added a comment.
This revision now requires changes to proceed.

Idea is good, few concerns about the implementation.




Comment at: lldb/source/Target/Process.cpp:404-419
+llvm::StringRef Process::GetAttachSynchronousHijackListenerName() {
+  static ConstString class_name(
+  "lldb.internal.Process.AttachSynchronous.hijack");
+  return class_name.GetCString();
+}
+
+llvm::StringRef Process::GetLaunchSynchronousHijackListenerName() {

Do these need to be ConstStrings? They will live in the string pool forever, 
and it looks like in the code below you're just manipulating `const char *` 
anyway.

Could be something like:
```
llvm::StringRef Process::GetAttachSynchronousHijackListenerName() {
  return "lldb.internal.Process.AttachSynchronous.hijack";
}
```



Comment at: lldb/source/Target/Process.cpp:1427
 if (hijacking_name &&
-strcmp(hijacking_name, g_resume_sync_name))
+strstr(hijacking_name, "lldb.internal") != hijacking_name)
   return true;

Instead of using the `strstr` function directly, I would at least do `strnstr` 
to ensure that we're not touching memory we don't have. Especially if we could 
get `hijacking_name` to be shorter than `"lldb.internal"` somehow...

We could also change this to use StringRefs and use the 
`starts_with`/`startswith` methods instead.



Comment at: lldb/source/Target/Process.cpp:1437-1438
 if (hijacking_name &&
-strcmp(hijacking_name, g_resume_sync_name) == 0)
+strcmp(hijacking_name,
+   GetResumeSynchronousHijackListenerName().data()) == 0)
   return true;

First, I would probably avoid `strcmp` directly and use `strncmp` instead. We 
know the length of the HijackListener name, that would be a good choice for `n` 
here.

But you could probably simplify this even further. You can compare 
`GetResumeSynchronousHijackListenerName` to `hijacking_name` with StringRef's 
operator==. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148400

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

[Lldb-commits] [PATCH] D148399: [lldb] Improve logging for process state change (NFC)

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

+1 to what Jonas said. `LLDB_LOG` would greatly simplify this since it puts 
`__FILE__` and `__func__` in each message, which is what these are doing 
manually.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148399

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

[Lldb-commits] [PATCH] D148397: [lldb/Utility] Add opt-in passthrough mode to event listeners

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.



Comment at: lldb/include/lldb/Utility/Broadcaster.h:520-522
+/// A optional listener that all private events get also broadcasted to,
+/// on top the hijacked / default listeners.
+lldb::ListenerSP m_passthrough_listener = nullptr;

It looks like Broadcaster can have multiple listeners. Do we want multiple 
passthrough listeners as well? If not, why only one?



Comment at: lldb/source/API/SBAttachInfo.cpp:260
+
+  return SBListener(m_opaque_sp->GetPassthroughListener());
+}

`GetPassthroughListener` can return `nullptr`. I think you want to take 
advantage of the protected `SBListener(const ListenerSP &)` constructor here 
but I think that if `GetPassthroughListener` returns `nullptr` you'll end up 
calling the constructor `SBListener(const char *)`. I don't think this is what 
you want.



Comment at: lldb/source/API/SBLaunchInfo.cpp:395
+
+  return SBListener(m_opaque_sp->GetPassthroughListener());
+}

Same issue as `SBAttachInfo::GetPassthroughListener`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148397

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

[Lldb-commits] [PATCH] D148541: [lldb] fix build issue on MSVC because of missing byte-swap builtins

[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/D148541/new/

https://reviews.llvm.org/D148541

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

[Lldb-commits] [PATCH] D148541: [lldb] fix build issue on MSVC because of missing byte-swap builtins

[Alex Langford via Phabricator via lldb-commits]

bulbazord requested changes to this revision.
bulbazord added a comment.
This revision now requires changes to proceed.

Instead of not using `__builtin_bswap{32,64}` entirely, we should check if 
they're available.

You can use the clang feature-checking macro `__has_builtin` (and if it doesn't 
exist you can define it). So you could amend this patch like so:

  #if !defined(__has_builtin)
  #define __has_builtin(x) 0
  #endif
  
  #if __has_builtin(__builtin_bswap32)
  #define bswap_32(x) __builtin_bswap32(x)
  #elif _MSC_VER
  #define bswap_32(x) _byteswap_ulong(x)
  #else
  #include 
  #endif




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148541

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

[Lldb-commits] [PATCH] D146965: [lldb] Add support for MSP430 in LLDB.

[Alex Langford via Phabricator via lldb-commits]

bulbazord accepted this revision.
bulbazord added a comment.

In D146965#4274274 , @DavidSpickett 
wrote:

> I'm sure there's something missing but LGTM.
>
> @bulbazord ?

I'm satisfied with this since there are tests. I'm not sure what might be 
missing but I'm sure we'll find it in time.


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

https://reviews.llvm.org/D146965

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

[Lldb-commits] [PATCH] D148395: [lldb] Unify default/hijack listener between Process{Attach, Launch}Info (NFC)

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

Creating a ProcessAttachInfo from a ProcessLaunchInfo with this change means 
they'll have different listeners. Is that ProcessLaunchInfo kept around for any 
other reason? Or is it just made to create the ProcessAttachInfo? This seems 
like a reasonable move to me, but I'm not sure how LaunchInfo and AttachInfo 
might interact.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148395

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

[Lldb-commits] [PATCH] D148402: [lldb] Remove use of ConstString from Args::GetShellSafeArgument

[Alex Langford via Phabricator via lldb-commits]

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

Having the names of various shells in ConstString's StringPool is not
really necessary, especially if they are otherwise not going to be there
in the first place. For example, if the person debugging uses bash on
their system, the `shell` parameter will have its `m_filename` set to a
ConstString containing "bash". However, fish, tcsh, zsh, and sh will
probably never be used and are just taking up space in the StringPool.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D148402

Files:
  lldb/source/Utility/Args.cpp


Index: lldb/source/Utility/Args.cpp
===
--- lldb/source/Utility/Args.cpp
+++ lldb/source/Utility/Args.cpp
@@ -7,7 +7,6 @@
 
//===--===//
 
 #include "lldb/Utility/Args.h"
-#include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Stream.h"
 #include "lldb/Utility/StringList.h"
@@ -385,20 +384,21 @@
 std::string Args::GetShellSafeArgument(const FileSpec ,
llvm::StringRef unsafe_arg) {
   struct ShellDescriptor {
-ConstString m_basename;
+llvm::StringRef m_basename;
 llvm::StringRef m_escapables;
   };
 
-  static ShellDescriptor g_Shells[] = {{ConstString("bash"), " '\"<>()&;"},
-   {ConstString("fish"), " '\"<>()&\\|;"},
-   {ConstString("tcsh"), " '\"<>()&;"},
-   {ConstString("zsh"), " '\"<>()&;\\|"},
-   {ConstString("sh"), " '\"<>()&;"}};
+  static ShellDescriptor g_Shells[] = {{"bash", " '\"<>()&;"},
+   {"fish", " '\"<>()&\\|;"},
+   {"tcsh", " '\"<>()&;"},
+   {"zsh", " '\"<>()&;\\|"},
+   {"sh", " '\"<>()&;"}};
 
   // safe minimal set
   llvm::StringRef escapables = " '\"";
 
-  if (auto basename = shell.GetFilename()) {
+  auto basename = shell.GetFilename().GetStringRef();
+  if (!basename.empty()) {
 for (const auto  : g_Shells) {
   if (Shell.m_basename == basename) {
 escapables = Shell.m_escapables;


Index: lldb/source/Utility/Args.cpp
===
--- lldb/source/Utility/Args.cpp
+++ lldb/source/Utility/Args.cpp
@@ -7,7 +7,6 @@
 //===--===//
 
 #include "lldb/Utility/Args.h"
-#include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Stream.h"
 #include "lldb/Utility/StringList.h"
@@ -385,20 +384,21 @@
 std::string Args::GetShellSafeArgument(const FileSpec ,
llvm::StringRef unsafe_arg) {
   struct ShellDescriptor {
-ConstString m_basename;
+llvm::StringRef m_basename;
 llvm::StringRef m_escapables;
   };
 
-  static ShellDescriptor g_Shells[] = {{ConstString("bash"), " '\"<>()&;"},
-   {ConstString("fish"), " '\"<>()&\\|;"},
-   {ConstString("tcsh"), " '\"<>()&;"},
-   {ConstString("zsh"), " '\"<>()&;\\|"},
-   {ConstString("sh"), " '\"<>()&;"}};
+  static ShellDescriptor g_Shells[] = {{"bash", " '\"<>()&;"},
+   {"fish", " '\"<>()&\\|;"},
+   {"tcsh", " '\"<>()&;"},
+   {"zsh", " '\"<>()&;\\|"},
+   {"sh", " '\"<>()&;"}};
 
   // safe minimal set
   llvm::StringRef escapables = " '\"";
 
-  if (auto basename = shell.GetFilename()) {
+  auto basename = shell.GetFilename().GetStringRef();
+  if (!basename.empty()) {
 for (const auto  : g_Shells) {
   if (Shell.m_basename == basename) {
 escapables = Shell.m_escapables;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D148380: [lldb] Lock accesses to PathMappingLists's internals

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

In D148380#4270090 , @JDevlieghere 
wrote:

> In D148380#4270085 , @bulbazord 
> wrote:
>
>> In D148380#4269876 , @bulbazord 
>> wrote:
>>
>>> In D148380#4269862 , 
>>> @JDevlieghere wrote:
>>>
 Does this actually have to be a //recursive// mutex?
>>>
>>> Good point, I don't think it does. I'll update this.
>>
>> Scratch that -- It unfortunately does need to be a recursive mutex. 
>> `PathMappingList` has a callback that may try to perform another operation 
>> on the list itself. This happens if you try to use `target modules 
>> search-paths add` -- We append to the list, the specified callback tries to 
>> read the size of the list. With a std::mutex we deadlock.
>
> I see, and I assume it doesn't make sense to unlock the mutex before the 
> callback?

I modified this patch locally to use `std::mutex` and release the mutex before 
invoking the callback. The test suite seems to like it, but it's still 
technically incorrect because of `AppendUnique`. Using std::mutex, we'd have to 
first lock the mutex, check to see if the given paths are already in `m_pairs`, 
and if they are not, unlock the mutex and call Append. If 2 things try to 
AppendUnique the same things at the same time and the stars align, they're 
going to Append the same thing twice (which is what happens without this patch 
anyway). We'd need to do some refactors if we wanted std::mutex to work 
correctly instead of std::recursive_mutex. That may be worth doing in the 
future, but I'm going to land this as-is for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148380

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

[Lldb-commits] [PATCH] D148380: [lldb] Lock accesses to PathMappingLists's internals

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

In D148380#4269876 , @bulbazord wrote:

> In D148380#4269862 , @JDevlieghere 
> wrote:
>
>> Does this actually have to be a //recursive// mutex?
>
> Good point, I don't think it does. I'll update this.

Scratch that -- It unfortunately does need to be a recursive mutex. 
`PathMappingList` has a callback that may try to perform another operation on 
the list itself. This happens if you try to use `target modules search-paths 
add` -- We append to the list, the specified callback tries to read the size of 
the list. With a std::mutex we deadlock.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148380

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

[Lldb-commits] [PATCH] D148380: [lldb] Lock accesses to PathMappingLists's internals

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

In D148380#4269862 , @JDevlieghere 
wrote:

> Does this actually have to be a //recursive// mutex?

Good point, I don't think it does. I'll update this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148380

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

[Lldb-commits] [PATCH] D148380: [lldb] Lock accesses to PathMappingLists's internals

[Alex Langford via Phabricator via lldb-commits]

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

This class is not safe in multithreaded code. It's possible for one
thread to modify a PathMappingList's `m_pair` vector while another
thread is iterating over it, effectively invalidating the iterator and
potentially leading to crashes or other difficult-to-diagnose bugs.

rdar://107695786


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D148380

Files:
  lldb/include/lldb/Target/PathMappingList.h
  lldb/source/Target/PathMappingList.cpp

Index: lldb/source/Target/PathMappingList.cpp
===
--- lldb/source/Target/PathMappingList.cpp
+++ lldb/source/Target/PathMappingList.cpp
@@ -48,6 +48,7 @@
 
 const PathMappingList ::operator=(const PathMappingList ) {
   if (this != ) {
+std::scoped_lock locks(m_mutex, rhs.m_mutex);
 m_pairs = rhs.m_pairs;
 m_callback = nullptr;
 m_callback_baton = nullptr;
@@ -60,6 +61,7 @@
 
 void PathMappingList::Append(llvm::StringRef path, llvm::StringRef replacement,
  bool notify) {
+  std::lock_guard lock(m_mutex);
   ++m_mod_id;
   m_pairs.emplace_back(pair(NormalizePath(path), NormalizePath(replacement)));
   if (notify && m_callback)
@@ -67,6 +69,7 @@
 }
 
 void PathMappingList::Append(const PathMappingList , bool notify) {
+  std::scoped_lock locks(m_mutex, rhs.m_mutex);
   ++m_mod_id;
   if (!rhs.m_pairs.empty()) {
 const_iterator pos, end = rhs.m_pairs.end();
@@ -81,6 +84,7 @@
llvm::StringRef replacement, bool notify) {
   auto normalized_path = NormalizePath(path);
   auto normalized_replacement = NormalizePath(replacement);
+  std::lock_guard lock(m_mutex);
   for (const auto  : m_pairs) {
 if (pair.first.GetStringRef().equals(normalized_path) &&
 pair.second.GetStringRef().equals(normalized_replacement))
@@ -92,6 +96,7 @@
 
 void PathMappingList::Insert(llvm::StringRef path, llvm::StringRef replacement,
  uint32_t index, bool notify) {
+  std::lock_guard lock(m_mutex);
   ++m_mod_id;
   iterator insert_iter;
   if (index >= m_pairs.size())
@@ -106,6 +111,7 @@
 
 bool PathMappingList::Replace(llvm::StringRef path, llvm::StringRef replacement,
   uint32_t index, bool notify) {
+  std::lock_guard lock(m_mutex);
   if (index >= m_pairs.size())
 return false;
   ++m_mod_id;
@@ -116,6 +122,7 @@
 }
 
 bool PathMappingList::Remove(size_t index, bool notify) {
+  std::lock_guard lock(m_mutex);
   if (index >= m_pairs.size())
 return false;
 
@@ -130,6 +137,7 @@
 // For clients which do not need the pair index dumped, pass a pair_index >= 0
 // to only dump the indicated pair.
 void PathMappingList::Dump(Stream *s, int pair_index) {
+  std::lock_guard lock(m_mutex);
   unsigned int numPairs = m_pairs.size();
 
   if (pair_index < 0) {
@@ -147,6 +155,7 @@
 
 llvm::json::Value PathMappingList::ToJSON() {
   llvm::json::Array entries;
+  std::lock_guard lock(m_mutex);
   for (const auto  : m_pairs) {
 llvm::json::Array entry{pair.first.GetStringRef().str(),
 pair.second.GetStringRef().str()};
@@ -156,6 +165,7 @@
 }
 
 void PathMappingList::Clear(bool notify) {
+  std::lock_guard lock(m_mutex);
   if (!m_pairs.empty())
 ++m_mod_id;
   m_pairs.clear();
@@ -186,6 +196,7 @@
 
 std::optional PathMappingList::RemapPath(llvm::StringRef mapping_path,
bool only_if_exists) const {
+  std::lock_guard lock(m_mutex);
   if (m_pairs.empty() || mapping_path.empty())
 return {};
   LazyBool path_is_relative = eLazyBoolCalculate;
@@ -224,6 +235,7 @@
 PathMappingList::ReverseRemapPath(const FileSpec , FileSpec ) const {
   std::string path = file.GetPath();
   llvm::StringRef path_ref(path);
+  std::lock_guard lock(m_mutex);
   for (const auto  : m_pairs) {
 llvm::StringRef removed_prefix = it.second.GetStringRef();
 if (!path_ref.consume_front(it.second.GetStringRef()))
@@ -252,6 +264,7 @@
 
 bool PathMappingList::Replace(llvm::StringRef path, llvm::StringRef new_path,
   bool notify) {
+  std::lock_guard lock(m_mutex);
   uint32_t idx = FindIndexForPath(path);
   if (idx < m_pairs.size()) {
 ++m_mod_id;
@@ -264,6 +277,7 @@
 }
 
 bool PathMappingList::Remove(ConstString path, bool notify) {
+  std::lock_guard lock(m_mutex);
   iterator pos = FindIteratorForPath(path);
   if (pos != m_pairs.end()) {
 ++m_mod_id;
@@ -277,6 +291,7 @@
 
 PathMappingList::const_iterator
 PathMappingList::FindIteratorForPath(ConstString path) const {
+  std::lock_guard lock(m_mutex);
   const_iterator pos;
   const_iterator begin = m_pairs.begin();
   const_iterator end = m_pairs.end();
@@ -290,6 +305,7 @@
 
 

[Lldb-commits] [PATCH] D148282: Fix the help for "type X delete" to make the -a & -w behaviors clear

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.



Comment at: lldb/source/Commands/CommandObjectType.cpp:112-113
+return "format";
+  }
+};
+

maybe add an `llvm_unreachable` after the switch statement? 



Comment at: lldb/source/Commands/CommandObjectType.cpp:879-885
+const char *CommandObjectTypeFormatterDelete::g_short_help_template = 
+"Delete an existing %s for a type.";
+const char *CommandObjectTypeFormatterDelete::g_long_help_template = 
+"Delete an existing %s for a type.  Unless you specify a "
+"specific category or all categories, only the "
+"'default' category is searched.  The names must be exactly as "
+"shown in the 'type %s list' output";

You could define these in the class and not out-of-line if you use constexpr. 
So for example, above in the class you can do:

```
  static constexpr const char *g_short_help_template = "Delete an existing %s 
for a type.";
```




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148282

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

[Lldb-commits] [PATCH] D148050: [lldb] Change formatter helper function parameter list to remove ConstString

[Alex Langford via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG0e22b8cba786: [lldb] Change formatter helper function 
parameter list to remove ConstString (authored by bulbazord).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148050

Files:
  lldb/include/lldb/DataFormatters/FormattersHelpers.h
  lldb/source/DataFormatters/FormatManager.cpp
  lldb/source/DataFormatters/FormattersHelpers.cpp
  lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
  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
@@ -297,23 +297,23 @@
   objc_flags.SetSkipPointers(true);
   AddCXXSummary(objc_category_sp,
 lldb_private::formatters::ObjCSELSummaryProvider,
-"SEL summary provider", ConstString("SEL"), objc_flags);
-  AddCXXSummary(
-  objc_category_sp, lldb_private::formatters::ObjCSELSummaryProvider,
-  "SEL summary provider", ConstString("struct objc_selector"), objc_flags);
-  AddCXXSummary(
-  objc_category_sp, lldb_private::formatters::ObjCSELSummaryProvider,
-  "SEL summary provider", ConstString("objc_selector"), objc_flags);
-  AddCXXSummary(
-  objc_category_sp, lldb_private::formatters::ObjCSELSummaryProvider,
-  "SEL summary provider", ConstString("objc_selector *"), objc_flags);
+"SEL summary provider", "SEL", objc_flags);
+  AddCXXSummary(objc_category_sp,
+lldb_private::formatters::ObjCSELSummaryProvider,
+"SEL summary provider", "struct objc_selector", objc_flags);
+  AddCXXSummary(objc_category_sp,
+lldb_private::formatters::ObjCSELSummaryProvider,
+"SEL summary provider", "objc_selector", objc_flags);
   AddCXXSummary(objc_category_sp,
 lldb_private::formatters::ObjCSELSummaryProvider,
-"SEL summary provider", ConstString("SEL *"), objc_flags);
+"SEL summary provider", "objc_selector *", objc_flags);
+  AddCXXSummary(objc_category_sp,
+lldb_private::formatters::ObjCSELSummaryProvider,
+"SEL summary provider", "SEL *", objc_flags);
 
   AddCXXSummary(objc_category_sp,
 lldb_private::formatters::ObjCClassSummaryProvider,
-"Class summary provider", ConstString("Class"), objc_flags);
+"Class summary provider", "Class", objc_flags);
 
   SyntheticChildren::Flags class_synth_flags;
   class_synth_flags.SetCascades(true).SetSkipPointers(false).SetSkipReferences(
@@ -321,61 +321,62 @@
 
   AddCXXSynthetic(objc_category_sp,
   lldb_private::formatters::ObjCClassSyntheticFrontEndCreator,
-  "Class synthetic children", ConstString("Class"),
-  class_synth_flags);
+  "Class synthetic children", "Class", class_synth_flags);
 
   objc_flags.SetSkipPointers(false);
   objc_flags.SetCascades(true);
   objc_flags.SetSkipReferences(false);
 
   AddStringSummary(objc_category_sp, "${var.__FuncPtr%A}",
-   ConstString("__block_literal_generic"), objc_flags);
+   "__block_literal_generic", objc_flags);
 
-  AddStringSummary(objc_category_sp, "${var.years} years, ${var.months} "
- "months, ${var.days} days, ${var.hours} "
- "hours, ${var.minutes} minutes "
- "${var.seconds} seconds",
-   ConstString("CFGregorianUnits"), objc_flags);
   AddStringSummary(objc_category_sp,
-   "location=${var.location} length=${var.length}",
-   ConstString("CFRange"), objc_flags);
+   "${var.years} years, ${var.months} "
+   "months, ${var.days} days, ${var.hours} "
+   "hours, ${var.minutes} minutes "
+   "${var.seconds} seconds",
+   "CFGregorianUnits", objc_flags);
+  AddStringSummary(objc_category_sp,
+   "location=${var.location} length=${var.length}", "CFRange",
+   objc_flags);
 
   AddStringSummary(objc_category_sp,
-   "location=${var.location}, length=${var.length}",
-   ConstString("NSRange"), objc_flags);
+   "location=${var.location}, length=${var.length}", "NSRange",
+   objc_flags);
   AddStringSummary(objc_category_sp, "(${var.origin}, ${var.size}), ...",
-   ConstString("NSRectArray"), objc_flags);
+   "NSRectArray", objc_flags);
 
-  AddOneLineSummary(objc_category_sp, ConstString("NSPoint"), objc_flags);
-  AddOneLineSummary(objc_category_sp, 

[Lldb-commits] [PATCH] D148050: [lldb] Change formatter helper function parameter list to remove ConstString

[Alex Langford via Phabricator via lldb-commits]

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

All of these functions take a ConstString for the type_name,
but this isn't really needed for two reasons:
1.) This parameter is always constructed from a static c-string

  constant.

2.) They are passed along to to `AddTypeSummary` as a StringRef anyway.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D148050

Files:
  lldb/include/lldb/DataFormatters/FormattersHelpers.h
  lldb/source/DataFormatters/FormatManager.cpp
  lldb/source/DataFormatters/FormattersHelpers.cpp
  lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
  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
@@ -297,23 +297,23 @@
   objc_flags.SetSkipPointers(true);
   AddCXXSummary(objc_category_sp,
 lldb_private::formatters::ObjCSELSummaryProvider,
-"SEL summary provider", ConstString("SEL"), objc_flags);
-  AddCXXSummary(
-  objc_category_sp, lldb_private::formatters::ObjCSELSummaryProvider,
-  "SEL summary provider", ConstString("struct objc_selector"), objc_flags);
-  AddCXXSummary(
-  objc_category_sp, lldb_private::formatters::ObjCSELSummaryProvider,
-  "SEL summary provider", ConstString("objc_selector"), objc_flags);
-  AddCXXSummary(
-  objc_category_sp, lldb_private::formatters::ObjCSELSummaryProvider,
-  "SEL summary provider", ConstString("objc_selector *"), objc_flags);
+"SEL summary provider", "SEL", objc_flags);
+  AddCXXSummary(objc_category_sp,
+lldb_private::formatters::ObjCSELSummaryProvider,
+"SEL summary provider", "struct objc_selector", objc_flags);
+  AddCXXSummary(objc_category_sp,
+lldb_private::formatters::ObjCSELSummaryProvider,
+"SEL summary provider", "objc_selector", objc_flags);
   AddCXXSummary(objc_category_sp,
 lldb_private::formatters::ObjCSELSummaryProvider,
-"SEL summary provider", ConstString("SEL *"), objc_flags);
+"SEL summary provider", "objc_selector *", objc_flags);
+  AddCXXSummary(objc_category_sp,
+lldb_private::formatters::ObjCSELSummaryProvider,
+"SEL summary provider", "SEL *", objc_flags);
 
   AddCXXSummary(objc_category_sp,
 lldb_private::formatters::ObjCClassSummaryProvider,
-"Class summary provider", ConstString("Class"), objc_flags);
+"Class summary provider", "Class", objc_flags);
 
   SyntheticChildren::Flags class_synth_flags;
   class_synth_flags.SetCascades(true).SetSkipPointers(false).SetSkipReferences(
@@ -321,61 +321,62 @@
 
   AddCXXSynthetic(objc_category_sp,
   lldb_private::formatters::ObjCClassSyntheticFrontEndCreator,
-  "Class synthetic children", ConstString("Class"),
-  class_synth_flags);
+  "Class synthetic children", "Class", class_synth_flags);
 
   objc_flags.SetSkipPointers(false);
   objc_flags.SetCascades(true);
   objc_flags.SetSkipReferences(false);
 
   AddStringSummary(objc_category_sp, "${var.__FuncPtr%A}",
-   ConstString("__block_literal_generic"), objc_flags);
+   "__block_literal_generic", objc_flags);
 
-  AddStringSummary(objc_category_sp, "${var.years} years, ${var.months} "
- "months, ${var.days} days, ${var.hours} "
- "hours, ${var.minutes} minutes "
- "${var.seconds} seconds",
-   ConstString("CFGregorianUnits"), objc_flags);
   AddStringSummary(objc_category_sp,
-   "location=${var.location} length=${var.length}",
-   ConstString("CFRange"), objc_flags);
+   "${var.years} years, ${var.months} "
+   "months, ${var.days} days, ${var.hours} "
+   "hours, ${var.minutes} minutes "
+   "${var.seconds} seconds",
+   "CFGregorianUnits", objc_flags);
+  AddStringSummary(objc_category_sp,
+   "location=${var.location} length=${var.length}", "CFRange",
+   objc_flags);
 
   AddStringSummary(objc_category_sp,
-   "location=${var.location}, length=${var.length}",
-   ConstString("NSRange"), objc_flags);
+   "location=${var.location}, length=${var.length}", "NSRange",
+   objc_flags);
   AddStringSummary(objc_category_sp, "(${var.origin}, ${var.size}), ...",
-  

[Lldb-commits] [PATCH] D147833: [lldb] Change return type of EventData::GetFlavor

[Alex Langford via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG6ebf1bc66b89: [lldb] Change return type of 
EventData::GetFlavor (authored by bulbazord).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147833

Files:
  lldb/include/lldb/Breakpoint/Breakpoint.h
  lldb/include/lldb/Breakpoint/Watchpoint.h
  lldb/include/lldb/Core/DebuggerEvents.h
  lldb/include/lldb/Target/Process.h
  lldb/include/lldb/Target/Target.h
  lldb/include/lldb/Target/Thread.h
  lldb/include/lldb/Utility/Event.h
  lldb/source/API/SBEvent.cpp
  lldb/source/Breakpoint/Breakpoint.cpp
  lldb/source/Breakpoint/Watchpoint.cpp
  lldb/source/Core/DebuggerEvents.cpp
  lldb/source/Target/Process.cpp
  lldb/source/Target/Target.cpp
  lldb/source/Target/Thread.cpp
  lldb/source/Utility/Event.cpp

Index: lldb/source/Utility/Event.cpp
===
--- lldb/source/Utility/Event.cpp
+++ lldb/source/Utility/Event.cpp
@@ -114,12 +114,9 @@
 
 EventDataBytes::~EventDataBytes() = default;
 
-ConstString EventDataBytes::GetFlavorString() {
-  static ConstString g_flavor("EventDataBytes");
-  return g_flavor;
-}
+llvm::StringRef EventDataBytes::GetFlavorString() { return "EventDataBytes"; }
 
-ConstString EventDataBytes::GetFlavor() const {
+llvm::StringRef EventDataBytes::GetFlavor() const {
   return EventDataBytes::GetFlavorString();
 }
 
@@ -182,6 +179,10 @@
   m_bytes.swap(new_bytes);
 }
 
+llvm::StringRef EventDataReceipt::GetFlavorString() {
+  return "Process::ProcessEventData";
+}
+
 #pragma mark -
 #pragma mark EventStructuredData
 
@@ -200,7 +201,7 @@
 
 // EventDataStructuredData member functions
 
-ConstString EventDataStructuredData::GetFlavor() const {
+llvm::StringRef EventDataStructuredData::GetFlavor() const {
   return EventDataStructuredData::GetFlavorString();
 }
 
@@ -280,7 +281,6 @@
 return StructuredDataPluginSP();
 }
 
-ConstString EventDataStructuredData::GetFlavorString() {
-  static ConstString s_flavor("EventDataStructuredData");
-  return s_flavor;
+llvm::StringRef EventDataStructuredData::GetFlavorString() {
+  return "EventDataStructuredData";
 }
Index: lldb/source/Target/Thread.cpp
===
--- lldb/source/Target/Thread.cpp
+++ lldb/source/Target/Thread.cpp
@@ -150,9 +150,8 @@
 
 // Thread Event Data
 
-ConstString Thread::ThreadEventData::GetFlavorString() {
-  static ConstString g_flavor("Thread::ThreadEventData");
-  return g_flavor;
+llvm::StringRef Thread::ThreadEventData::GetFlavorString() {
+  return "Thread::ThreadEventData";
 }
 
 Thread::ThreadEventData::ThreadEventData(const lldb::ThreadSP thread_sp)
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -4786,9 +4786,8 @@
 
 Target::TargetEventData::~TargetEventData() = default;
 
-ConstString Target::TargetEventData::GetFlavorString() {
-  static ConstString g_flavor("Target::TargetEventData");
-  return g_flavor;
+llvm::StringRef Target::TargetEventData::GetFlavorString() {
+  return "Target::TargetEventData";
 }
 
 void Target::TargetEventData::Dump(Stream *s) const {
Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -3929,12 +3929,11 @@
 
 Process::ProcessEventData::~ProcessEventData() = default;
 
-ConstString Process::ProcessEventData::GetFlavorString() {
-  static ConstString g_flavor("Process::ProcessEventData");
-  return g_flavor;
+llvm::StringRef Process::ProcessEventData::GetFlavorString() {
+  return "Process::ProcessEventData";
 }
 
-ConstString Process::ProcessEventData::GetFlavor() const {
+llvm::StringRef Process::ProcessEventData::GetFlavor() const {
   return ProcessEventData::GetFlavorString();
 }
 
Index: lldb/source/Core/DebuggerEvents.cpp
===
--- lldb/source/Core/DebuggerEvents.cpp
+++ lldb/source/Core/DebuggerEvents.cpp
@@ -23,12 +23,11 @@
   return nullptr;
 }
 
-ConstString ProgressEventData::GetFlavorString() {
-  static ConstString g_flavor("ProgressEventData");
-  return g_flavor;
+llvm::StringRef ProgressEventData::GetFlavorString() {
+  return "ProgressEventData";
 }
 
-ConstString ProgressEventData::GetFlavor() const {
+llvm::StringRef ProgressEventData::GetFlavor() const {
   return ProgressEventData::GetFlavorString();
 }
 
@@ -94,12 +93,11 @@
   s->Flush();
 }
 
-ConstString DiagnosticEventData::GetFlavorString() {
-  static ConstString g_flavor("DiagnosticEventData");
-  return g_flavor;
+llvm::StringRef DiagnosticEventData::GetFlavorString() {
+  return "DiagnosticEventData";
 }
 
-ConstString DiagnosticEventData::GetFlavor() const {
+llvm::StringRef DiagnosticEventData::GetFlavor() 

[Lldb-commits] [PATCH] D147841: [lldb][NFC] Update syntax description for language cplusplus demangle

[Alex Langford via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG469bdbd62ce2: [lldb][NFC] Update syntax description for 
language cplusplus demangle (authored by bulbazord).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147841

Files:
  
lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp
  lldb/test/Shell/Commands/command-language-cplusplus-demangle.test


Index: lldb/test/Shell/Commands/command-language-cplusplus-demangle.test
===
--- /dev/null
+++ lldb/test/Shell/Commands/command-language-cplusplus-demangle.test
@@ -0,0 +1,22 @@
+# RUN: %lldb -b -o "language cplusplus demangle __ZN3Foo7DoThingEv" \
+# RUN:   | FileCheck --check-prefix=DOUBLE-UNDERSCORE %s
+# RUN: %lldb -b -o "language cplusplus demangle _ZN3Foo7DoThingEv" \
+# RUN:   | FileCheck --check-prefix=SINGLE-UNDERSCORE %s
+# RUN: not %lldb -b -o "language cplusplus demangle foo" 2>&1 \
+# RUN:   | FileCheck --check-prefix=NOT-MANGLED %s
+# RUN: not %lldb -b -o "language cplusplus demangle _ZN3Foo7DoThingEv foo" 
2>&1 \
+# RUN:   | FileCheck --check-prefix=MULTI-ARG %s
+# RUN: %lldb -b -o "help language cplusplus demangle" \
+# RUN:   | FileCheck --check-prefix=HELP-MESSAGE %s
+
+# DOUBLE-UNDERSCORE: __ZN3Foo7DoThingEv ---> Foo::DoThing()
+
+# SINGLE-UNDERSCORE: _ZN3Foo7DoThingEv ---> Foo::DoThing()
+
+# NOT-MANGLED: error: foo is not a valid C++ mangled name
+
+# MULTI-ARG: _ZN3Foo7DoThingEv ---> Foo::DoThing()
+# MULTI-ARG: error: foo is not a valid C++ mangled name
+
+# HELP-MESSAGE: Demangle a C++ mangled name.
+# HELP-MESSAGE: Syntax: language cplusplus demangle [ ...] 
Index: 
lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp
===
--- 
lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp
+++ 
lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp
@@ -318,9 +318,9 @@
 class CommandObjectMultiwordItaniumABI_Demangle : public CommandObjectParsed {
 public:
   CommandObjectMultiwordItaniumABI_Demangle(CommandInterpreter )
-  : CommandObjectParsed(interpreter, "demangle",
-"Demangle a C++ mangled name.",
-"language cplusplus demangle") {
+  : CommandObjectParsed(
+interpreter, "demangle", "Demangle a C++ mangled name.",
+"language cplusplus demangle [ ...]") {
 CommandArgumentEntry arg;
 CommandArgumentData index_arg;
 


Index: lldb/test/Shell/Commands/command-language-cplusplus-demangle.test
===
--- /dev/null
+++ lldb/test/Shell/Commands/command-language-cplusplus-demangle.test
@@ -0,0 +1,22 @@
+# RUN: %lldb -b -o "language cplusplus demangle __ZN3Foo7DoThingEv" \
+# RUN:   | FileCheck --check-prefix=DOUBLE-UNDERSCORE %s
+# RUN: %lldb -b -o "language cplusplus demangle _ZN3Foo7DoThingEv" \
+# RUN:   | FileCheck --check-prefix=SINGLE-UNDERSCORE %s
+# RUN: not %lldb -b -o "language cplusplus demangle foo" 2>&1 \
+# RUN:   | FileCheck --check-prefix=NOT-MANGLED %s
+# RUN: not %lldb -b -o "language cplusplus demangle _ZN3Foo7DoThingEv foo" 2>&1 \
+# RUN:   | FileCheck --check-prefix=MULTI-ARG %s
+# RUN: %lldb -b -o "help language cplusplus demangle" \
+# RUN:   | FileCheck --check-prefix=HELP-MESSAGE %s
+
+# DOUBLE-UNDERSCORE: __ZN3Foo7DoThingEv ---> Foo::DoThing()
+
+# SINGLE-UNDERSCORE: _ZN3Foo7DoThingEv ---> Foo::DoThing()
+
+# NOT-MANGLED: error: foo is not a valid C++ mangled name
+
+# MULTI-ARG: _ZN3Foo7DoThingEv ---> Foo::DoThing()
+# MULTI-ARG: error: foo is not a valid C++ mangled name
+
+# HELP-MESSAGE: Demangle a C++ mangled name.
+# HELP-MESSAGE: Syntax: language cplusplus demangle [ ...] 
Index: lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp
===
--- lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp
+++ lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp
@@ -318,9 +318,9 @@
 class CommandObjectMultiwordItaniumABI_Demangle : public CommandObjectParsed {
 public:
   CommandObjectMultiwordItaniumABI_Demangle(CommandInterpreter )
-  : CommandObjectParsed(interpreter, "demangle",
-"Demangle a C++ mangled name.",
-"language cplusplus demangle") {
+  : CommandObjectParsed(
+interpreter, "demangle", "Demangle a C++ mangled name.",
+"language cplusplus demangle [ ...]") {
 CommandArgumentEntry arg;
 CommandArgumentData index_arg;
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org

[Lldb-commits] [PATCH] D147833: [lldb] Change return type of EventData::GetFlavor

[Alex Langford via Phabricator via lldb-commits]

bulbazord updated this revision to Diff 511954.
bulbazord added a comment.

Change return type to llvm::StringRef


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147833

Files:
  lldb/include/lldb/Breakpoint/Breakpoint.h
  lldb/include/lldb/Breakpoint/Watchpoint.h
  lldb/include/lldb/Core/DebuggerEvents.h
  lldb/include/lldb/Target/Process.h
  lldb/include/lldb/Target/Target.h
  lldb/include/lldb/Target/Thread.h
  lldb/include/lldb/Utility/Event.h
  lldb/source/API/SBEvent.cpp
  lldb/source/Breakpoint/Breakpoint.cpp
  lldb/source/Breakpoint/Watchpoint.cpp
  lldb/source/Core/DebuggerEvents.cpp
  lldb/source/Target/Process.cpp
  lldb/source/Target/Target.cpp
  lldb/source/Target/Thread.cpp
  lldb/source/Utility/Event.cpp

Index: lldb/source/Utility/Event.cpp
===
--- lldb/source/Utility/Event.cpp
+++ lldb/source/Utility/Event.cpp
@@ -114,12 +114,9 @@
 
 EventDataBytes::~EventDataBytes() = default;
 
-ConstString EventDataBytes::GetFlavorString() {
-  static ConstString g_flavor("EventDataBytes");
-  return g_flavor;
-}
+llvm::StringRef EventDataBytes::GetFlavorString() { return "EventDataBytes"; }
 
-ConstString EventDataBytes::GetFlavor() const {
+llvm::StringRef EventDataBytes::GetFlavor() const {
   return EventDataBytes::GetFlavorString();
 }
 
@@ -182,6 +179,10 @@
   m_bytes.swap(new_bytes);
 }
 
+llvm::StringRef EventDataReceipt::GetFlavorString() {
+  return "Process::ProcessEventData";
+}
+
 #pragma mark -
 #pragma mark EventStructuredData
 
@@ -200,7 +201,7 @@
 
 // EventDataStructuredData member functions
 
-ConstString EventDataStructuredData::GetFlavor() const {
+llvm::StringRef EventDataStructuredData::GetFlavor() const {
   return EventDataStructuredData::GetFlavorString();
 }
 
@@ -280,7 +281,6 @@
 return StructuredDataPluginSP();
 }
 
-ConstString EventDataStructuredData::GetFlavorString() {
-  static ConstString s_flavor("EventDataStructuredData");
-  return s_flavor;
+llvm::StringRef EventDataStructuredData::GetFlavorString() {
+  return "EventDataStructuredData";
 }
Index: lldb/source/Target/Thread.cpp
===
--- lldb/source/Target/Thread.cpp
+++ lldb/source/Target/Thread.cpp
@@ -150,9 +150,8 @@
 
 // Thread Event Data
 
-ConstString Thread::ThreadEventData::GetFlavorString() {
-  static ConstString g_flavor("Thread::ThreadEventData");
-  return g_flavor;
+llvm::StringRef Thread::ThreadEventData::GetFlavorString() {
+  return "Thread::ThreadEventData";
 }
 
 Thread::ThreadEventData::ThreadEventData(const lldb::ThreadSP thread_sp)
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -4786,9 +4786,8 @@
 
 Target::TargetEventData::~TargetEventData() = default;
 
-ConstString Target::TargetEventData::GetFlavorString() {
-  static ConstString g_flavor("Target::TargetEventData");
-  return g_flavor;
+llvm::StringRef Target::TargetEventData::GetFlavorString() {
+  return "Target::TargetEventData";
 }
 
 void Target::TargetEventData::Dump(Stream *s) const {
Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -3929,12 +3929,11 @@
 
 Process::ProcessEventData::~ProcessEventData() = default;
 
-ConstString Process::ProcessEventData::GetFlavorString() {
-  static ConstString g_flavor("Process::ProcessEventData");
-  return g_flavor;
+llvm::StringRef Process::ProcessEventData::GetFlavorString() {
+  return "Process::ProcessEventData";
 }
 
-ConstString Process::ProcessEventData::GetFlavor() const {
+llvm::StringRef Process::ProcessEventData::GetFlavor() const {
   return ProcessEventData::GetFlavorString();
 }
 
Index: lldb/source/Core/DebuggerEvents.cpp
===
--- lldb/source/Core/DebuggerEvents.cpp
+++ lldb/source/Core/DebuggerEvents.cpp
@@ -23,12 +23,11 @@
   return nullptr;
 }
 
-ConstString ProgressEventData::GetFlavorString() {
-  static ConstString g_flavor("ProgressEventData");
-  return g_flavor;
+llvm::StringRef ProgressEventData::GetFlavorString() {
+  return "ProgressEventData";
 }
 
-ConstString ProgressEventData::GetFlavor() const {
+llvm::StringRef ProgressEventData::GetFlavor() const {
   return ProgressEventData::GetFlavorString();
 }
 
@@ -94,12 +93,11 @@
   s->Flush();
 }
 
-ConstString DiagnosticEventData::GetFlavorString() {
-  static ConstString g_flavor("DiagnosticEventData");
-  return g_flavor;
+llvm::StringRef DiagnosticEventData::GetFlavorString() {
+  return "DiagnosticEventData";
 }
 
-ConstString DiagnosticEventData::GetFlavor() const {
+llvm::StringRef DiagnosticEventData::GetFlavor() const {
   return DiagnosticEventData::GetFlavorString();
 }
 
@@ 

[Lldb-commits] [PATCH] D147833: [lldb] Change return type of EventData::GetFlavor

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

In D147833#4253416 , @jasonmolenda 
wrote:

> idk maybe I'm over-thinking it, but this does make me a little uncomfortable. 
>  We have at least one instance where someone did 
> `platform_sp->GetPluginName() == "ios-simulator"` in 
> ClangExpressionSourceCode.cpp (probably to avoid a dependency on an an apple 
> platform plugin in generic code; the code should undoubtedly be done 
> differently) and the mach-o linker on darwin today will unique identical 
> c-strings from separate compilation units, but I doubt that's a guarantee on 
> Darwin or on other platforms.  Platform::GetPluginName is returning a 
> StringRef here, and we naturally see code like this and assume the c-string 
> will be converted to a StringRef or ConstString implicitly for the comparison 
> operator.  But if GetPluginName started returning a pointer to const c-string 
> and the strings aren't uniqued, the comparison fails in a tricky to see way.



In D147833#4253422 , @jasonmolenda 
wrote:

> In D147833#4253416 , @jasonmolenda 
> wrote:
>
>> idk maybe I'm over-thinking it, but this does make me a little 
>> uncomfortable.  We have at least one instance where someone did 
>> `platform_sp->GetPluginName() == "ios-simulator"` in 
>> ClangExpressionSourceCode.cpp
>
> I only looked around for the first instance of this code pattern, there may 
> be another place where we compare identification strings from objects.  And I 
> don't feel confident that I can anticipate what will be added to lldb in the 
> future.

Yes, we definitely do that in many places. I see your point though, comparing 
const c-string pointers can get pretty tricky if somebody refactors this in the 
future. In that case, instead of `GetFlavor` returning a `const char *` or a 
`ConstString`, it could return a `StringRef` and this should be handled, no? 
Just like `GetPluginName()` in your example.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147833

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

[Lldb-commits] [PATCH] D147841: [lldb][NFC] Update syntax description for language cplusplus demangle

[Alex Langford via Phabricator via lldb-commits]

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

Also added some tests because this is completely untested.

rdar://107780577


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D147841

Files:
  
lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp
  lldb/test/Shell/Commands/command-language-cplusplus-demangle.test


Index: lldb/test/Shell/Commands/command-language-cplusplus-demangle.test
===
--- /dev/null
+++ lldb/test/Shell/Commands/command-language-cplusplus-demangle.test
@@ -0,0 +1,22 @@
+# RUN: %lldb -b -o "language cplusplus demangle __ZN3Foo7DoThingEv" \
+# RUN:   | FileCheck --check-prefix=DOUBLE-UNDERSCORE %s
+# RUN: %lldb -b -o "language cplusplus demangle _ZN3Foo7DoThingEv" \
+# RUN:   | FileCheck --check-prefix=SINGLE-UNDERSCORE %s
+# RUN: not %lldb -b -o "language cplusplus demangle foo" 2>&1 \
+# RUN:   | FileCheck --check-prefix=NOT-MANGLED %s
+# RUN: not %lldb -b -o "language cplusplus demangle _ZN3Foo7DoThingEv foo" 
2>&1 \
+# RUN:   | FileCheck --check-prefix=MULTI-ARG %s
+# RUN: %lldb -b -o "help language cplusplus demangle" \
+# RUN:   | FileCheck --check-prefix=HELP-MESSAGE %s
+
+# DOUBLE-UNDERSCORE: __ZN3Foo7DoThingEv ---> Foo::DoThing()
+
+# SINGLE-UNDERSCORE: _ZN3Foo7DoThingEv ---> Foo::DoThing()
+
+# NOT-MANGLED: error: foo is not a valid C++ mangled name
+
+# MULTI-ARG: _ZN3Foo7DoThingEv ---> Foo::DoThing()
+# MULTI-ARG: error: foo is not a valid C++ mangled name
+
+# HELP-MESSAGE: Demangle a C++ mangled name.
+# HELP-MESSAGE: Syntax: language cplusplus demangle [ ...] 
Index: 
lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp
===
--- 
lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp
+++ 
lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp
@@ -318,9 +318,9 @@
 class CommandObjectMultiwordItaniumABI_Demangle : public CommandObjectParsed {
 public:
   CommandObjectMultiwordItaniumABI_Demangle(CommandInterpreter )
-  : CommandObjectParsed(interpreter, "demangle",
-"Demangle a C++ mangled name.",
-"language cplusplus demangle") {
+  : CommandObjectParsed(
+interpreter, "demangle", "Demangle a C++ mangled name.",
+"language cplusplus demangle [ ...]") {
 CommandArgumentEntry arg;
 CommandArgumentData index_arg;
 


Index: lldb/test/Shell/Commands/command-language-cplusplus-demangle.test
===
--- /dev/null
+++ lldb/test/Shell/Commands/command-language-cplusplus-demangle.test
@@ -0,0 +1,22 @@
+# RUN: %lldb -b -o "language cplusplus demangle __ZN3Foo7DoThingEv" \
+# RUN:   | FileCheck --check-prefix=DOUBLE-UNDERSCORE %s
+# RUN: %lldb -b -o "language cplusplus demangle _ZN3Foo7DoThingEv" \
+# RUN:   | FileCheck --check-prefix=SINGLE-UNDERSCORE %s
+# RUN: not %lldb -b -o "language cplusplus demangle foo" 2>&1 \
+# RUN:   | FileCheck --check-prefix=NOT-MANGLED %s
+# RUN: not %lldb -b -o "language cplusplus demangle _ZN3Foo7DoThingEv foo" 2>&1 \
+# RUN:   | FileCheck --check-prefix=MULTI-ARG %s
+# RUN: %lldb -b -o "help language cplusplus demangle" \
+# RUN:   | FileCheck --check-prefix=HELP-MESSAGE %s
+
+# DOUBLE-UNDERSCORE: __ZN3Foo7DoThingEv ---> Foo::DoThing()
+
+# SINGLE-UNDERSCORE: _ZN3Foo7DoThingEv ---> Foo::DoThing()
+
+# NOT-MANGLED: error: foo is not a valid C++ mangled name
+
+# MULTI-ARG: _ZN3Foo7DoThingEv ---> Foo::DoThing()
+# MULTI-ARG: error: foo is not a valid C++ mangled name
+
+# HELP-MESSAGE: Demangle a C++ mangled name.
+# HELP-MESSAGE: Syntax: language cplusplus demangle [ ...] 
Index: lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp
===
--- lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp
+++ lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp
@@ -318,9 +318,9 @@
 class CommandObjectMultiwordItaniumABI_Demangle : public CommandObjectParsed {
 public:
   CommandObjectMultiwordItaniumABI_Demangle(CommandInterpreter )
-  : CommandObjectParsed(interpreter, "demangle",
-"Demangle a C++ mangled name.",
-"language cplusplus demangle") {
+  : CommandObjectParsed(
+interpreter, "demangle", "Demangle a C++ mangled name.",
+"language cplusplus demangle [ ...]") {
 CommandArgumentEntry arg;
 CommandArgumentData index_arg;
 
___
lldb-commits 

[Lldb-commits] [PATCH] D147833: [lldb] Change return type of EventData::GetFlavor

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

In D147833#4252651 , @jasonmolenda 
wrote:

> Am I missing something, how does this work when we have uses like 
> `event_data->GetFlavor() == TargetEventData::GetFlavorString()` - is this 
> changing from a one-time construction of a ConstString to a construction of a 
> ConstString every time it's called by implicit construction?

`event_data->GetFlavor() == TargetEventData::GetFlavorString()` would be just a 
straight pointer comparison, no ConstString in the picture. These strings don't 
need to be in the ConstString pool -- they only exist in one place each and are 
guaranteed to be there for the lifetime of a running lldb process. There are 
other places in lldb where I'd like to do this, but I figured I'd do it in a 
few smaller patches rather than one giant patch.




Comment at: lldb/source/Utility/Event.cpp:183
+const char *EventDataReceipt::GetFlavorString() {
+  return "Process::ProcessEventData";
+}

Note: this is probably the wrong flavor string (i.e. a bug) but I'm just 
preserving existing behavior here, for better or worse.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147833

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

[Lldb-commits] [PATCH] D147833: [lldb] Change return type of EventData::GetFlavor

[Alex Langford via Phabricator via lldb-commits]

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

There's no reason these strings need to be in the ConstString
StringPool, they're already string literals with static lifetime.

I plan on addressing other similar functions in follow up commits.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D147833

Files:
  lldb/include/lldb/Breakpoint/Breakpoint.h
  lldb/include/lldb/Breakpoint/Watchpoint.h
  lldb/include/lldb/Core/DebuggerEvents.h
  lldb/include/lldb/Target/Process.h
  lldb/include/lldb/Target/Target.h
  lldb/include/lldb/Target/Thread.h
  lldb/include/lldb/Utility/Event.h
  lldb/source/API/SBEvent.cpp
  lldb/source/Breakpoint/Breakpoint.cpp
  lldb/source/Breakpoint/Watchpoint.cpp
  lldb/source/Core/DebuggerEvents.cpp
  lldb/source/Target/Process.cpp
  lldb/source/Target/Target.cpp
  lldb/source/Target/Thread.cpp
  lldb/source/Utility/Event.cpp

Index: lldb/source/Utility/Event.cpp
===
--- lldb/source/Utility/Event.cpp
+++ lldb/source/Utility/Event.cpp
@@ -114,12 +114,9 @@
 
 EventDataBytes::~EventDataBytes() = default;
 
-ConstString EventDataBytes::GetFlavorString() {
-  static ConstString g_flavor("EventDataBytes");
-  return g_flavor;
-}
+const char *EventDataBytes::GetFlavorString() { return "EventDataBytes"; }
 
-ConstString EventDataBytes::GetFlavor() const {
+const char *EventDataBytes::GetFlavor() const {
   return EventDataBytes::GetFlavorString();
 }
 
@@ -182,6 +179,10 @@
   m_bytes.swap(new_bytes);
 }
 
+const char *EventDataReceipt::GetFlavorString() {
+  return "Process::ProcessEventData";
+}
+
 #pragma mark -
 #pragma mark EventStructuredData
 
@@ -200,7 +201,7 @@
 
 // EventDataStructuredData member functions
 
-ConstString EventDataStructuredData::GetFlavor() const {
+const char *EventDataStructuredData::GetFlavor() const {
   return EventDataStructuredData::GetFlavorString();
 }
 
@@ -280,7 +281,6 @@
 return StructuredDataPluginSP();
 }
 
-ConstString EventDataStructuredData::GetFlavorString() {
-  static ConstString s_flavor("EventDataStructuredData");
-  return s_flavor;
+const char *EventDataStructuredData::GetFlavorString() {
+  return "EventDataStructuredData";
 }
Index: lldb/source/Target/Thread.cpp
===
--- lldb/source/Target/Thread.cpp
+++ lldb/source/Target/Thread.cpp
@@ -150,9 +150,8 @@
 
 // Thread Event Data
 
-ConstString Thread::ThreadEventData::GetFlavorString() {
-  static ConstString g_flavor("Thread::ThreadEventData");
-  return g_flavor;
+const char *Thread::ThreadEventData::GetFlavorString() {
+  return "Thread::ThreadEventData";
 }
 
 Thread::ThreadEventData::ThreadEventData(const lldb::ThreadSP thread_sp)
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -4786,9 +4786,8 @@
 
 Target::TargetEventData::~TargetEventData() = default;
 
-ConstString Target::TargetEventData::GetFlavorString() {
-  static ConstString g_flavor("Target::TargetEventData");
-  return g_flavor;
+const char *Target::TargetEventData::GetFlavorString() {
+  return "Target::TargetEventData";
 }
 
 void Target::TargetEventData::Dump(Stream *s) const {
Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -3929,12 +3929,11 @@
 
 Process::ProcessEventData::~ProcessEventData() = default;
 
-ConstString Process::ProcessEventData::GetFlavorString() {
-  static ConstString g_flavor("Process::ProcessEventData");
-  return g_flavor;
+const char *Process::ProcessEventData::GetFlavorString() {
+  return "Process::ProcessEventData";
 }
 
-ConstString Process::ProcessEventData::GetFlavor() const {
+const char *Process::ProcessEventData::GetFlavor() const {
   return ProcessEventData::GetFlavorString();
 }
 
Index: lldb/source/Core/DebuggerEvents.cpp
===
--- lldb/source/Core/DebuggerEvents.cpp
+++ lldb/source/Core/DebuggerEvents.cpp
@@ -23,12 +23,9 @@
   return nullptr;
 }
 
-ConstString ProgressEventData::GetFlavorString() {
-  static ConstString g_flavor("ProgressEventData");
-  return g_flavor;
-}
+const char *ProgressEventData::GetFlavorString() { return "ProgressEventData"; }
 
-ConstString ProgressEventData::GetFlavor() const {
+const char *ProgressEventData::GetFlavor() const {
   return ProgressEventData::GetFlavorString();
 }
 
@@ -94,12 +91,11 @@
   s->Flush();
 }
 
-ConstString DiagnosticEventData::GetFlavorString() {
-  static ConstString g_flavor("DiagnosticEventData");
-  return g_flavor;
+const char 

[Lldb-commits] [PATCH] D147801: [lldb] Add unittests for a few FileSpec methods

[Alex Langford via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGde5f96e99aed: [lldb] Add unittests for a few FileSpec 
methods (authored by bulbazord).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147801

Files:
  lldb/include/lldb/Utility/FileSpec.h
  lldb/unittests/Utility/FileSpecTest.cpp


Index: lldb/unittests/Utility/FileSpecTest.cpp
===
--- lldb/unittests/Utility/FileSpecTest.cpp
+++ lldb/unittests/Utility/FileSpecTest.cpp
@@ -448,3 +448,59 @@
   file.PrependPathComponent("/tmp");
   EXPECT_TRUE(file.IsAbsolute());
 }
+
+TEST(FileSpecTest, TestFileNameExtensions) {
+  FileSpec dylib = PosixSpec("/tmp/foo.dylib");
+  FileSpec exe = PosixSpec("/tmp/foo");
+  FileSpec dSYM = PosixSpec("/tmp/foo.dSYM/");
+  FileSpec just_dot = PosixSpec("/tmp/bar.");
+
+  EXPECT_TRUE(dylib.GetFileNameExtension() == ".dylib");
+  EXPECT_TRUE(exe.GetFileNameExtension() == ConstString(nullptr));
+  EXPECT_TRUE(dSYM.GetFileNameExtension() == ".dSYM");
+  EXPECT_TRUE(just_dot.GetFileNameExtension() == ".");
+
+  FileSpec dll = WindowsSpec("C:\\tmp\\foo.dll");
+  FileSpec win_noext = WindowsSpec("C:\\tmp\\foo");
+
+  EXPECT_TRUE(dll.GetFileNameExtension() == ".dll");
+  EXPECT_TRUE(win_noext.GetFileNameExtension() == ConstString(nullptr));
+}
+
+TEST(FileSpecTest, TestFileNameStrippingExtension) {
+  FileSpec dylib = PosixSpec("/tmp/foo.dylib");
+  FileSpec exe = PosixSpec("/tmp/foo");
+  FileSpec just_dot = PosixSpec("/tmp/bar.");
+
+  EXPECT_TRUE(dylib.GetFileNameStrippingExtension() == "foo");
+  EXPECT_TRUE(exe.GetFileNameStrippingExtension() == "foo");
+  EXPECT_TRUE(just_dot.GetFileNameStrippingExtension() == "bar");
+
+  FileSpec dll = WindowsSpec("C:\\tmp\\foo.dll");
+  FileSpec win_noext = WindowsSpec("C:\\tmp\\foo");
+
+  EXPECT_TRUE(dll.GetFileNameStrippingExtension() == "foo");
+  EXPECT_TRUE(win_noext.GetFileNameStrippingExtension() == "foo");
+}
+
+TEST(FileSpecTest, TestIsSourceImplementationFile) {
+  FileSpec c_src = PosixSpec("/tmp/foo.c");
+  FileSpec txt_file = PosixSpec("/tmp/foo.txt");
+  FileSpec executable = PosixSpec("/tmp/foo");
+  FileSpec just_dot = PosixSpec("/tmp/bar.");
+
+  EXPECT_TRUE(c_src.IsSourceImplementationFile());
+  EXPECT_FALSE(txt_file.IsSourceImplementationFile());
+  EXPECT_FALSE(executable.IsSourceImplementationFile());
+  EXPECT_FALSE(just_dot.IsSourceImplementationFile());
+
+  FileSpec cpp_src = WindowsSpec("C:\\tmp\\foo.cpp");
+  FileSpec dll = WindowsSpec("C:\\tmp\\foo.dll");
+  FileSpec win_noext = WindowsSpec("C:\\tmp\\foo");
+  FileSpec exe = WindowsSpec("C:\\tmp\\foo.exe");
+
+  EXPECT_TRUE(cpp_src.IsSourceImplementationFile());
+  EXPECT_FALSE(dll.IsSourceImplementationFile());
+  EXPECT_FALSE(win_noext.IsSourceImplementationFile());
+  EXPECT_FALSE(exe.IsSourceImplementationFile());
+}
Index: lldb/include/lldb/Utility/FileSpec.h
===
--- lldb/include/lldb/Utility/FileSpec.h
+++ lldb/include/lldb/Utility/FileSpec.h
@@ -253,7 +253,7 @@
   /// (files with a ".c", ".cpp", ".m", ".mm" (many more) extension).
   ///
   /// \return
-  /// \b true if the filespec represents an implementation source
+  /// \b true if the FileSpec represents an implementation source
   /// file, \b false otherwise.
   bool IsSourceImplementationFile() const;
 
@@ -327,7 +327,7 @@
   /// Returns a ConstString that represents the extension of the filename for
   /// this FileSpec object. If this object does not represent a file, or the
   /// filename has no extension, ConstString(nullptr) is returned. The dot
-  /// ('.') character is not returned as part of the extension
+  /// ('.') character is the first character in the returned string.
   ///
   /// \return Returns the extension of the file as a ConstString object.
   ConstString GetFileNameExtension() const;


Index: lldb/unittests/Utility/FileSpecTest.cpp
===
--- lldb/unittests/Utility/FileSpecTest.cpp
+++ lldb/unittests/Utility/FileSpecTest.cpp
@@ -448,3 +448,59 @@
   file.PrependPathComponent("/tmp");
   EXPECT_TRUE(file.IsAbsolute());
 }
+
+TEST(FileSpecTest, TestFileNameExtensions) {
+  FileSpec dylib = PosixSpec("/tmp/foo.dylib");
+  FileSpec exe = PosixSpec("/tmp/foo");
+  FileSpec dSYM = PosixSpec("/tmp/foo.dSYM/");
+  FileSpec just_dot = PosixSpec("/tmp/bar.");
+
+  EXPECT_TRUE(dylib.GetFileNameExtension() == ".dylib");
+  EXPECT_TRUE(exe.GetFileNameExtension() == ConstString(nullptr));
+  EXPECT_TRUE(dSYM.GetFileNameExtension() == ".dSYM");
+  EXPECT_TRUE(just_dot.GetFileNameExtension() == ".");
+
+  FileSpec dll = WindowsSpec("C:\\tmp\\foo.dll");
+  FileSpec win_noext = WindowsSpec("C:\\tmp\\foo");
+
+  EXPECT_TRUE(dll.GetFileNameExtension() == ".dll");
+  EXPECT_TRUE(win_noext.GetFileNameExtension() == 

[Lldb-commits] [PATCH] D147801: [lldb] Add unittests for a few FileSpec methods

[Alex Langford via Phabricator via lldb-commits]

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

This adds tests for:

- FileSpec::TestFileNameExtensions
- FileSpec::TestFileNameStrippingExtension
- FileSpec::IsSourceImplementationFile

This additionally updates incorrect documentation.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D147801

Files:
  lldb/include/lldb/Utility/FileSpec.h
  lldb/unittests/Utility/FileSpecTest.cpp


Index: lldb/unittests/Utility/FileSpecTest.cpp
===
--- lldb/unittests/Utility/FileSpecTest.cpp
+++ lldb/unittests/Utility/FileSpecTest.cpp
@@ -448,3 +448,59 @@
   file.PrependPathComponent("/tmp");
   EXPECT_TRUE(file.IsAbsolute());
 }
+
+TEST(FileSpecTest, TestFileNameExtensions) {
+  FileSpec dylib = PosixSpec("/tmp/foo.dylib");
+  FileSpec exe = PosixSpec("/tmp/foo");
+  FileSpec dSYM = PosixSpec("/tmp/foo.dSYM/");
+  FileSpec just_dot = PosixSpec("/tmp/bar.");
+
+  EXPECT_TRUE(dylib.GetFileNameExtension() == ".dylib");
+  EXPECT_TRUE(exe.GetFileNameExtension() == ConstString(nullptr));
+  EXPECT_TRUE(dSYM.GetFileNameExtension() == ".dSYM");
+  EXPECT_TRUE(just_dot.GetFileNameExtension() == ".");
+
+  FileSpec dll = WindowsSpec("C:\\tmp\\foo.dll");
+  FileSpec win_noext = WindowsSpec("C:\\tmp\\foo");
+
+  EXPECT_TRUE(dll.GetFileNameExtension() == ".dll");
+  EXPECT_TRUE(win_noext.GetFileNameExtension() == ConstString(nullptr));
+}
+
+TEST(FileSpecTest, TestFileNameStrippingExtension) {
+  FileSpec dylib = PosixSpec("/tmp/foo.dylib");
+  FileSpec exe = PosixSpec("/tmp/foo");
+  FileSpec just_dot = PosixSpec("/tmp/bar.");
+
+  EXPECT_TRUE(dylib.GetFileNameStrippingExtension() == "foo");
+  EXPECT_TRUE(exe.GetFileNameStrippingExtension() == "foo");
+  EXPECT_TRUE(just_dot.GetFileNameStrippingExtension() == "bar");
+
+  FileSpec dll = WindowsSpec("C:\\tmp\\foo.dll");
+  FileSpec win_noext = WindowsSpec("C:\\tmp\\foo");
+
+  EXPECT_TRUE(dll.GetFileNameStrippingExtension() == "foo");
+  EXPECT_TRUE(win_noext.GetFileNameStrippingExtension() == "foo");
+}
+
+TEST(FileSpecTest, TestIsSourceImplementationFile) {
+  FileSpec c_src = PosixSpec("/tmp/foo.c");
+  FileSpec txt_file = PosixSpec("/tmp/foo.txt");
+  FileSpec executable = PosixSpec("/tmp/foo");
+  FileSpec just_dot = PosixSpec("/tmp/bar.");
+
+  EXPECT_TRUE(c_src.IsSourceImplementationFile());
+  EXPECT_FALSE(txt_file.IsSourceImplementationFile());
+  EXPECT_FALSE(executable.IsSourceImplementationFile());
+  EXPECT_FALSE(just_dot.IsSourceImplementationFile());
+
+  FileSpec cpp_src = WindowsSpec("C:\\tmp\\foo.cpp");
+  FileSpec dll = WindowsSpec("C:\\tmp\\foo.dll");
+  FileSpec win_noext = WindowsSpec("C:\\tmp\\foo");
+  FileSpec exe = WindowsSpec("C:\\tmp\\foo.exe");
+
+  EXPECT_TRUE(cpp_src.IsSourceImplementationFile());
+  EXPECT_FALSE(dll.IsSourceImplementationFile());
+  EXPECT_FALSE(win_noext.IsSourceImplementationFile());
+  EXPECT_FALSE(exe.IsSourceImplementationFile());
+}
Index: lldb/include/lldb/Utility/FileSpec.h
===
--- lldb/include/lldb/Utility/FileSpec.h
+++ lldb/include/lldb/Utility/FileSpec.h
@@ -253,7 +253,7 @@
   /// (files with a ".c", ".cpp", ".m", ".mm" (many more) extension).
   ///
   /// \return
-  /// \b true if the filespec represents an implementation source
+  /// \b true if the FileSpec represents an implementation source
   /// file, \b false otherwise.
   bool IsSourceImplementationFile() const;
 
@@ -327,7 +327,7 @@
   /// Returns a ConstString that represents the extension of the filename for
   /// this FileSpec object. If this object does not represent a file, or the
   /// filename has no extension, ConstString(nullptr) is returned. The dot
-  /// ('.') character is not returned as part of the extension
+  /// ('.') character is the first character in the returned string.
   ///
   /// \return Returns the extension of the file as a ConstString object.
   ConstString GetFileNameExtension() const;


Index: lldb/unittests/Utility/FileSpecTest.cpp
===
--- lldb/unittests/Utility/FileSpecTest.cpp
+++ lldb/unittests/Utility/FileSpecTest.cpp
@@ -448,3 +448,59 @@
   file.PrependPathComponent("/tmp");
   EXPECT_TRUE(file.IsAbsolute());
 }
+
+TEST(FileSpecTest, TestFileNameExtensions) {
+  FileSpec dylib = PosixSpec("/tmp/foo.dylib");
+  FileSpec exe = PosixSpec("/tmp/foo");
+  FileSpec dSYM = PosixSpec("/tmp/foo.dSYM/");
+  FileSpec just_dot = PosixSpec("/tmp/bar.");
+
+  EXPECT_TRUE(dylib.GetFileNameExtension() == ".dylib");
+  EXPECT_TRUE(exe.GetFileNameExtension() == ConstString(nullptr));
+  EXPECT_TRUE(dSYM.GetFileNameExtension() == ".dSYM");
+  EXPECT_TRUE(just_dot.GetFileNameExtension() == ".");
+
+  FileSpec dll = 

[Lldb-commits] [PATCH] D147746: [lldb][NFC] Delete unused function Breakpoint::GetEventIdentifier

[Alex Langford via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGb04bc87c9edc: [lldb][NFC] Delete unused function 
Breakpoint::GetEventIdentifier (authored by bulbazord).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147746

Files:
  lldb/include/lldb/Breakpoint/Breakpoint.h
  lldb/source/Breakpoint/Breakpoint.cpp


Index: lldb/source/Breakpoint/Breakpoint.cpp
===
--- lldb/source/Breakpoint/Breakpoint.cpp
+++ lldb/source/Breakpoint/Breakpoint.cpp
@@ -37,11 +37,6 @@
 using namespace lldb_private;
 using namespace llvm;
 
-ConstString Breakpoint::GetEventIdentifier() {
-  static ConstString g_identifier("event-identifier.breakpoint.changed");
-  return g_identifier;
-}
-
 const char *Breakpoint::g_option_names[static_cast(
 Breakpoint::OptionNames::LastOptionName)]{"Names", "Hardware"};
 
Index: lldb/include/lldb/Breakpoint/Breakpoint.h
===
--- lldb/include/lldb/Breakpoint/Breakpoint.h
+++ lldb/include/lldb/Breakpoint/Breakpoint.h
@@ -80,7 +80,6 @@
 class Breakpoint : public std::enable_shared_from_this,
public Stoppoint {
 public:
-  static ConstString GetEventIdentifier();
   static const char *
   BreakpointEventTypeAsCString(lldb::BreakpointEventType type);
 


Index: lldb/source/Breakpoint/Breakpoint.cpp
===
--- lldb/source/Breakpoint/Breakpoint.cpp
+++ lldb/source/Breakpoint/Breakpoint.cpp
@@ -37,11 +37,6 @@
 using namespace lldb_private;
 using namespace llvm;
 
-ConstString Breakpoint::GetEventIdentifier() {
-  static ConstString g_identifier("event-identifier.breakpoint.changed");
-  return g_identifier;
-}
-
 const char *Breakpoint::g_option_names[static_cast(
 Breakpoint::OptionNames::LastOptionName)]{"Names", "Hardware"};
 
Index: lldb/include/lldb/Breakpoint/Breakpoint.h
===
--- lldb/include/lldb/Breakpoint/Breakpoint.h
+++ lldb/include/lldb/Breakpoint/Breakpoint.h
@@ -80,7 +80,6 @@
 class Breakpoint : public std::enable_shared_from_this,
public Stoppoint {
 public:
-  static ConstString GetEventIdentifier();
   static const char *
   BreakpointEventTypeAsCString(lldb::BreakpointEventType type);
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D147746: [lldb][NFC] Delete unused function Breakpoint::GetEventIdentifier

[Alex Langford via Phabricator via lldb-commits]

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

This is unused


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D147746

Files:
  lldb/include/lldb/Breakpoint/Breakpoint.h
  lldb/source/Breakpoint/Breakpoint.cpp


Index: lldb/source/Breakpoint/Breakpoint.cpp
===
--- lldb/source/Breakpoint/Breakpoint.cpp
+++ lldb/source/Breakpoint/Breakpoint.cpp
@@ -37,11 +37,6 @@
 using namespace lldb_private;
 using namespace llvm;
 
-ConstString Breakpoint::GetEventIdentifier() {
-  static ConstString g_identifier("event-identifier.breakpoint.changed");
-  return g_identifier;
-}
-
 const char *Breakpoint::g_option_names[static_cast(
 Breakpoint::OptionNames::LastOptionName)]{"Names", "Hardware"};
 
Index: lldb/include/lldb/Breakpoint/Breakpoint.h
===
--- lldb/include/lldb/Breakpoint/Breakpoint.h
+++ lldb/include/lldb/Breakpoint/Breakpoint.h
@@ -80,7 +80,6 @@
 class Breakpoint : public std::enable_shared_from_this,
public Stoppoint {
 public:
-  static ConstString GetEventIdentifier();
   static const char *
   BreakpointEventTypeAsCString(lldb::BreakpointEventType type);
 


Index: lldb/source/Breakpoint/Breakpoint.cpp
===
--- lldb/source/Breakpoint/Breakpoint.cpp
+++ lldb/source/Breakpoint/Breakpoint.cpp
@@ -37,11 +37,6 @@
 using namespace lldb_private;
 using namespace llvm;
 
-ConstString Breakpoint::GetEventIdentifier() {
-  static ConstString g_identifier("event-identifier.breakpoint.changed");
-  return g_identifier;
-}
-
 const char *Breakpoint::g_option_names[static_cast(
 Breakpoint::OptionNames::LastOptionName)]{"Names", "Hardware"};
 
Index: lldb/include/lldb/Breakpoint/Breakpoint.h
===
--- lldb/include/lldb/Breakpoint/Breakpoint.h
+++ lldb/include/lldb/Breakpoint/Breakpoint.h
@@ -80,7 +80,6 @@
 class Breakpoint : public std::enable_shared_from_this,
public Stoppoint {
 public:
-  static ConstString GetEventIdentifier();
   static const char *
   BreakpointEventTypeAsCString(lldb::BreakpointEventType type);
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D147736: [lldb] Add an LLDB_DEPRECATED macro similar to LLVM_DEPRECATED

[Alex Langford via Phabricator via lldb-commits]

bulbazord accepted this revision.
bulbazord added inline comments.
This revision is now accepted and ready to land.



Comment at: lldb/include/lldb/API/SBDefines.h:33
+// supports the attribute.
+#if defined(SWIG) or _cplusplus < 201300
+#undef LLDB_DEPRECATED

nit: Use `__cplusplus < 201402L` to be more specific.

I got that value from https://en.cppreference.com/w/cpp/preprocessor/replace


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

https://reviews.llvm.org/D147736

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

[Lldb-commits] [PATCH] D147669: PECOFF: enforce move semantics and consume errors properly

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:869-873
   LLDB_LOG(log,
"ObjectFilePECOFF::AppendFromExportTable - failed to get export 
"
"table entry name: {0}",
llvm::fmt_consume(std::move(err)));
+  llvm::consumeError(std::move(err));

compnerd wrote:
> compnerd wrote:
> > compnerd wrote:
> > > bulbazord wrote:
> > > > If logging is enabled, we are moving from the same object twice, no?
> > > > 
> > > > I think we should rethink the LLDB_LOG macro when it comes to 
> > > > errors :/
> > > Yeah ... I was worried about that.
> > I should mention that at least on MSVC I _can_ get away with it:
> > 
> > ```
> >  ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
> > name: RVA 0x0 for export ordinal table not found
> >  ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
> > name: RVA 0x0 for export ordinal table not found
> >  ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
> > name: RVA 0x0 for export ordinal table not found
> >  ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
> > name: RVA 0x0 for export ordinal table not found
> >  ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
> > name: RVA 0x0 for export ordinal table not found
> >  ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
> > name: RVA 0x0 for export ordinal table not found 
> >  ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
> > name: RVA 0x0 for export ordinal table not found
> >  ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
> > name: RVA 0x0 for export ordinal table not found
> >  ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
> > name: RVA 0x0 for export ordinal table not found
> >  ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
> > name: RVA 0x0 for export ordinal table not found
> >  ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
> > name: RVA 0x0 for export ordinal table not found
> >  ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
> > name: RVA 0x0 for export ordinal table not found
> >  ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
> > name: RVA 0x0 for export ordinal table not found
> >  ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
> > name: RVA 0x0 for export ordinal table not found
> >  ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
> > name: RVA 0x0 for export ordinal table not found
> >  ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
> > name: RVA 0x0 for export ordinal table not found
> > ```
> https://godbolt.org/z/nj4r7K8hb
> 
> UBSAN also seems to indicate it is permissible.
Moving from a standard library type leaves the original object in a "valid but 
unspecified" state. Do we make such guarantees about llvm::Errors? This is 
probably going to work "correctly" but I'm not sure what might go wrong.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147669

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

[Lldb-commits] [PATCH] D147669: PECOFF: enforce move semantics and consume errors properly

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:869-873
   LLDB_LOG(log,
"ObjectFilePECOFF::AppendFromExportTable - failed to get export 
"
"table entry name: {0}",
llvm::fmt_consume(std::move(err)));
+  llvm::consumeError(std::move(err));

If logging is enabled, we are moving from the same object twice, no?

I think we should rethink the LLDB_LOG macro when it comes to errors :/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147669

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

[Lldb-commits] [PATCH] D145580: [lldb] Show register fields using bitfield struct types

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

In D145580#4245911 , @DavidSpickett 
wrote:

> @bulbazord Please take a look and see if I've done the plugin/core code split 
> correctly.

This looks fine. Thanks for being flexible and working with us to find 
something that maintains the split even if the end result feels more complex.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145580

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

[Lldb-commits] [PATCH] D146965: [lldb] Add support for MSP430 in LLDB.

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.



Comment at: lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp:127
  llvm::Triple::hexagon, llvm::Triple::mips, llvm::Triple::mips64el,
- llvm::Triple::mipsel, llvm::Triple::systemz},
+ llvm::Triple::mipsel, llvm::Triple::msp430, llvm::Triple::systemz},
 llvm::Triple::Linux);

DavidSpickett wrote:
> If this is bare metal why do we have changes to Linux code here? Or is this 
> the default platform used when connecting to the msp430 debug stub.
> 
> That said, I'm not sure we A: support Hexagon or B: it runs linux either :)
Hexagon is definitely supported AFAIK but I'm not sure it runs Linux. It does 
looks like we support an SysV ABI class for Hexagon though, so perhaps that's 
somewhat indicative that it runs something with that ABI...


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

https://reviews.llvm.org/D146965

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

[Lldb-commits] [PATCH] D147587: Fix the check in StopInfoBreakpoint for "are we running an expression"

[Alex Langford via Phabricator via lldb-commits]

bulbazord accepted this revision.
bulbazord added a comment.

LGTM. It'd be nice if we could have a test that wasn't specific to Darwin or 
ObjC though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147587

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