[Lldb-commits] [PATCH] D153626: [lldb] Use SmallVector for handling register data

[David Spickett via Phabricator via lldb-commits]

DavidSpickett created this revision.
Herald added subscribers: ctetreau, atanasyan, jrtc27, kristof.beyls.
Herald added a project: All.
DavidSpickett requested review of this revision.
Herald added subscribers: lldb-commits, alextsao1999.
Herald added a project: LLDB.

Previously lldb was using arrays of size kMaxRegisterByteSize to handle
registers. This was set to 256 because the largest possible register
we support is Arm's scalable vectors (SVE) which can be up to 256 bytes long.

This means for most operations aside from SVE, we're wasting 192 bytes
of it. Which is ok given that we don't have to pay the cost of a heap
alocation and 256 bytes isn't all that much overall.

With the introduction of the Arm Scalable Matrix extension there is a new
array storage register, ZA. This register is essentially a square made up of
SVE vectors. Therefore ZA could be up to 64kb in size.

https://developer.arm.com/documentation/ddi0616/latest/

"The Effective Streaming SVE vector length, SVL, is a power of two in the range 
128 to 2048 bits inclusive."

"The ZA storage is architectural register state consisting of a two-dimensional 
ZA array of [SVLB × SVLB] bytes."

99% of operations will never touch ZA and making every stack frame 64kb+ just
for that slim chance is a bad idea.

Instead I'm switching register handling to use SmallVector with a stack 
allocation
size of kTypicalRegisterByteSize. kMaxRegisterByteSize will be used in places
where we can't predict the size of register we're reading (in the GDB remote 
client).

The result is that the 99% of small register operations can use the stack
as before and the actual ZA operations will move to the heap as needed.

I tested this by first working out -wframe-larger-than values for all the
libraries using the arrays previously. With this change I was able to increase
kMaxRegisterByteSize to 256*256 without hitting those limits. With the
exception of the GDB server which needs to use a max size buffer.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D153626

Files:
  lldb/include/lldb/Utility/RegisterValue.h
  lldb/source/Host/common/NativeRegisterContext.cpp
  lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.cpp
  lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp
  lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
  lldb/source/Plugins/Instruction/MIPS/EmulateInstructionMIPS.cpp
  lldb/source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
  lldb/source/Target/RegisterContext.cpp
  lldb/source/Utility/RegisterValue.cpp

Index: lldb/source/Utility/RegisterValue.cpp
===
--- lldb/source/Utility/RegisterValue.cpp
+++ lldb/source/Utility/RegisterValue.cpp
@@ -48,11 +48,6 @@
 return 0;
   }
 
-  if (dst_len > kMaxRegisterByteSize) {
-error.SetErrorString("destination is too big");
-return 0;
-  }
-
   const uint32_t src_len = reg_info.byte_size;
 
   // Extract the register data into a data extractor
@@ -96,12 +91,6 @@
   //   |AABB| Address contents
   //   |AABB| Register contents [on little-endian hardware]
   //   |AABB| Register contents [on big-endian hardware]
-  if (src_len > kMaxRegisterByteSize) {
-error.SetErrorStringWithFormat(
-"register buffer is too small to receive %u bytes of data.", src_len);
-return 0;
-  }
-
   const uint32_t dst_len = reg_info.byte_size;
 
   if (src_len > dst_len) {
@@ -128,9 +117,10 @@
   case eTypeInvalid:
 break;
   case eTypeBytes: {
-DataExtractor data(buffer.bytes, buffer.length, buffer.byte_order, 1);
-if (scalar.SetValueFromData(data, lldb::eEncodingUint,
-	  buffer.length).Success())
+DataExtractor data(buffer.bytes.data(), buffer.bytes.size(),
+   buffer.byte_order, 1);
+if (scalar.SetValueFromData(data, lldb::eEncodingUint, buffer.bytes.size())
+.Success())
   return true;
   } break;
   case eTypeUInt8:
@@ -190,9 +180,6 @@
   if (src_len > reg_info.byte_size)
 src_len = reg_info.byte_size;
 
-  // Zero out the value in case we get partial data...
-  memset(buffer.bytes, 0, sizeof(buffer.bytes));
-
   type128 int128;
 
   m_type = eTypeInvalid;
@@ -232,16 +219,14 @@
 break;
   case eEncodingVector: {
 m_type = eTypeBytes;
-buffer.length = reg_info.byte_size;
+assert(reg_info.byte_size <= kMaxRegisterByteSize);
+buffer.bytes.resize(reg_info.byte_size);
 buffer.byte_order = src.GetByteOrder();
-assert(buffer.length <= kMaxRegisterByteSize);
-if (buffer.length > kMaxRegisterByteSize)
-  buffer.length = kMaxRegisterByteSize;
 if (src.CopyByteOrderedData(
-src_offset,// offset within "src" to start extracting data
-src_len,   // src length
-

[Lldb-commits] [PATCH] D153626: [lldb] Use SmallVector for handling register data

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added reviewers: omjavaid, JDevlieghere, labath.
DavidSpickett added a comment.

This assumes that the usual use case is:

- Make a small vector.
- Resize it to what you need.
- Use the content like an array.

Every case I found matched that but still, it's not the safest API ever. So 
I've tested this on arm64 of course, and did try to run the test suite with 
asan but issues with leaks in Python prevented that. No amount of filtering 
seemed to help.

I see Green Dragon runs an lldb sanitizers bot, so my plan would be to rely on 
that to catch any tricky issues prior to any SME changes going in.




Comment at: lldb/include/lldb/Utility/RegisterValue.h:36
+// Anything else we'll heap allocate storage for it.
+kMaxRegisterByteSize = kTypicalRegisterByteSize,
+  };

When the ZA register is added, this will change to 256*256. For now nothing 
should change apart from the slight overhead of SmallVector.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153626

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

[Lldb-commits] [PATCH] D153636: [LLDB] Fix the use of "platform process launch" with no extra arguments

[David Spickett via Phabricator via lldb-commits]

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

This fixes #62068.

After 8d1de7b34af46a089eb5433c700419ad9b2923ee 
 the 
following issue appeared:

  $ ./bin/lldb /tmp/test.o
  (lldb) target create "/tmp/test.o"
  Current executable set to '/tmp/test.o' (aarch64).
  (lldb) platform process launch -s
  error: Cannot launch '': Nothing to launch

Previously would call target->GetRunArguments when there were no extra
arguments, so we could find out what target.run-args might be.

Once that change started relying on the first arg being the exe,
the fact that that call clears the existing argument list caused the bug.

Instead, have it set a local arg list and append that to the existing
one. Which in this case will just contain the exe name.

Since there's no existing tests for this command I've added a new file
that covers enough to check this issue.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D153636

Files:
  lldb/source/Commands/CommandObjectPlatform.cpp
  lldb/test/API/commands/platform/process/launch/Makefile
  lldb/test/API/commands/platform/process/launch/TestPlatformProcessLaunch.py
  lldb/test/API/commands/platform/process/launch/main.c

Index: lldb/test/API/commands/platform/process/launch/main.c
===
--- /dev/null
+++ lldb/test/API/commands/platform/process/launch/main.c
@@ -0,0 +1,8 @@
+#include 
+
+int main(int argc, char const *argv[]) {
+  printf("Got %d argument(s).\n", argc);
+  for (int i = 0; i < argc; ++i)
+printf("[%d]: %s\n", i, argv[i]);
+  return 0;
+}
Index: lldb/test/API/commands/platform/process/launch/TestPlatformProcessLaunch.py
===
--- /dev/null
+++ lldb/test/API/commands/platform/process/launch/TestPlatformProcessLaunch.py
@@ -0,0 +1,59 @@
+"""
+Test platform process launch.
+"""
+
+from textwrap import dedent
+from lldbsuite.test.lldbtest import TestBase
+
+
+class ProcessLaunchTestCase(TestBase):
+NO_DEBUG_INFO_TESTCASE = True
+
+def setup(self):
+self.build()
+exe = self.getBuildArtifact("a.out")
+self.runCmd("file " + exe)
+return (exe, self.getBuildArtifact("stdio.log"))
+
+def test_process_launch_no_args(self):
+# When there are no extra arguments we just have 0, the program name.
+exe, outfile = self.setup()
+self.runCmd("platform process launch --stdout {} -s".format(outfile))
+self.runCmd("continue")
+
+with open(outfile) as f:
+   self.assertEqual(dedent("""\
+Got 1 argument(s).
+[0]: {}
+""".format(exe)), f.read())
+
+def test_process_launch_command_args(self):
+exe, outfile = self.setup()
+# Arguments given via the command override those in the settings.
+self.runCmd("settings set target.run-args D E")
+self.runCmd("platform process launch --stdout {} -s -- A B C".format(outfile))
+self.runCmd("continue")
+
+with open(outfile) as f:
+   self.assertEqual(dedent("""\
+Got 4 argument(s).
+[0]: {}
+[1]: A
+[2]: B
+[3]: C
+""".format(exe)), f.read())
+
+def test_process_launch_target_args(self):
+exe, outfile = self.setup()
+# When no arguments are passed via the command, use the setting.
+self.runCmd("settings set target.run-args D E")
+self.runCmd("platform process launch --stdout {}".format(outfile))
+self.runCmd("continue")
+
+with open(outfile) as f:
+   self.assertEqual(dedent("""\
+Got 3 argument(s).
+[0]: {}
+[1]: D
+[2]: E
+""".format(exe)), f.read())
\ No newline at end of file
Index: lldb/test/API/commands/platform/process/launch/Makefile
===
--- /dev/null
+++ lldb/test/API/commands/platform/process/launch/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+
+include Makefile.rules
Index: lldb/source/Commands/CommandObjectPlatform.cpp
===
--- lldb/source/Commands/CommandObjectPlatform.cpp
+++ lldb/source/Commands/CommandObjectPlatform.cpp
@@ -1207,8 +1207,12 @@
   if (m_options.launch_info.GetExecutableFile()) {
 Debugger &debugger = GetDebugger();
 
-if (argc == 0)
-  target->GetRunArguments(m_options.launch_info.GetArguments());
+if (argc == 0) {
+  // If no arguments were given to the command, use target.run-args.
+  Args target_run_args;
+  target->GetRu

[Lldb-commits] [PATCH] D153636: [LLDB] Fix the use of "platform process launch" with no extra arguments

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a reviewer: labath.
DavidSpickett added a comment.
Herald added a subscriber: JDevlieghere.

https://github.com/llvm/llvm-project/issues/62068


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153636

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

[Lldb-commits] [PATCH] D153644: [lldb] Print unprintable characters as unsigned

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere created this revision.
JDevlieghere added reviewers: jasonmolenda, DavidSpickett, bulbazord.
Herald added a project: All.
JDevlieghere requested review of this revision.

When specifying the C-string format for dumping memory, we treat unprintable 
characters as signed. Whether a character is signed or not is implementation 
defined, but all printable characters are signed. Therefore it's fair to assume 
that unprintable characters are unsigned.

Before this patch, the newly added unit test would print:

  "\xffcf\xfffa\xffed\xfffe\f”

With this patch, it prints what you would expect:

  “\xcf\xfa\xed\xfe\f”

rdar://26134


https://reviews.llvm.org/D153644

Files:
  lldb/source/Core/DumpDataExtractor.cpp
  lldb/unittests/Core/DumpDataExtractorTest.cpp


Index: lldb/unittests/Core/DumpDataExtractorTest.cpp
===
--- lldb/unittests/Core/DumpDataExtractorTest.cpp
+++ lldb/unittests/Core/DumpDataExtractorTest.cpp
@@ -133,6 +133,8 @@
 
   TestDump(llvm::StringRef("aardvark"), lldb::Format::eFormatCString,
"\"aardvark\"");
+  TestDump(llvm::StringRef("\xcf\xfa\xed\xfe\f"), lldb::Format::eFormatCString,
+   "\"\\xcf\\xfa\\xed\\xfe\\f\"");
   TestDump(99, lldb::Format::eFormatDecimal, "99");
   // Just prints as a signed integer.
   TestDump(-1, lldb::Format::eFormatEnum, "-1");
Index: lldb/source/Core/DumpDataExtractor.cpp
===
--- lldb/source/Core/DumpDataExtractor.cpp
+++ lldb/source/Core/DumpDataExtractor.cpp
@@ -212,7 +212,8 @@
 s.PutChar(c);
 return;
   }
-  s.Printf("\\x%2.2x", c);
+  // Non-print characters can be assumed to be unsigned.
+  s.Printf("\\x%2.2x", static_cast(c));
 }
 
 /// Dump a floating point type.


Index: lldb/unittests/Core/DumpDataExtractorTest.cpp
===
--- lldb/unittests/Core/DumpDataExtractorTest.cpp
+++ lldb/unittests/Core/DumpDataExtractorTest.cpp
@@ -133,6 +133,8 @@
 
   TestDump(llvm::StringRef("aardvark"), lldb::Format::eFormatCString,
"\"aardvark\"");
+  TestDump(llvm::StringRef("\xcf\xfa\xed\xfe\f"), lldb::Format::eFormatCString,
+   "\"\\xcf\\xfa\\xed\\xfe\\f\"");
   TestDump(99, lldb::Format::eFormatDecimal, "99");
   // Just prints as a signed integer.
   TestDump(-1, lldb::Format::eFormatEnum, "-1");
Index: lldb/source/Core/DumpDataExtractor.cpp
===
--- lldb/source/Core/DumpDataExtractor.cpp
+++ lldb/source/Core/DumpDataExtractor.cpp
@@ -212,7 +212,8 @@
 s.PutChar(c);
 return;
   }
-  s.Printf("\\x%2.2x", c);
+  // Non-print characters can be assumed to be unsigned.
+  s.Printf("\\x%2.2x", static_cast(c));
 }
 
 /// Dump a floating point type.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D153644: [lldb] Print unprintable characters as unsigned

[Alex Langford via Phabricator via lldb-commits]

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

Makes sense to me. Does it make sense to add a test case where we mix 
"printable" characters with "non-printable" ones? I see you have a `\f` at the 
end there, but that's a special escaped character. Something like 
`"\xef\xbb\xbfHello World!\n"`.


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

https://reviews.llvm.org/D153644

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

[Lldb-commits] [PATCH] D153626: [lldb] Use SmallVector for handling register data

[Jonas Devlieghere via Phabricator via lldb-commits]

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

In D153626#096 , @DavidSpickett 
wrote:

> This assumes that the usual use case is:
>
> - Make a small vector.
> - Resize it to what you need.
> - Use the content like an array.
>
> Every case I found matched that but still, it's not the safest API ever.

Yeah, I don't see how we can make this any safer without introducing something 
like the DataExtractor but for writing rather than reading, which would be 
overkill, and if we care about the cost of heap allocations, probably too slow 
as well. The current approach doesn't seem any less safe than the buffer we had 
before so I think it's fine.

In D153626#096 , @DavidSpickett 
wrote:

> I see Green Dragon runs an lldb sanitizers bot, so my plan would be to rely 
> on that to catch any tricky issues prior to any SME changes going in.

The sanitized bot on GreenDragon runs on Intel and I assume the "risky' changes 
only apply to arm64 as that's the only architecture that needs to scale beyond 
the default 256? Anyway I haven't seen the leaks issue you've mentioned locally 
so I'm happy to run a sanitized build on arm64.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153626

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

[Lldb-commits] [lldb] 1a397ec - [lldb][NFCI] Remove use of ConstString from StructuredDataPlugin

[Alex Langford via lldb-commits]

Author: Alex Langford
Date: 2023-06-23T10:29:52-07:00
New Revision: 1a397ecffdea64f5a521b4aac1fa4b98723dec22

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

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

The use of ConstString in StructuredDataPlugin is unneccessary as fast
comparisons are not neeeded for StructuredDataPlugins.

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

Added: 


Modified: 
lldb/include/lldb/Target/Process.h
lldb/include/lldb/Target/StructuredDataPlugin.h
lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp
lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.h
lldb/source/Target/Process.cpp
lldb/source/Target/StructuredDataPlugin.cpp

Removed: 




diff  --git a/lldb/include/lldb/Target/Process.h 
b/lldb/include/lldb/Target/Process.h
index 907ef50a7a61b..0f9b0c07df9bd 100644
--- a/lldb/include/lldb/Target/Process.h
+++ b/lldb/include/lldb/Target/Process.h
@@ -2833,7 +2833,7 @@ void PruneThreadPlans();
   ///
   /// virtual void
   /// HandleArrivalOfStructuredData(Process &process,
-  ///   ConstString type_name,
+  ///   llvm::StringRef type_name,
   ///   const StructuredData::ObjectSP
   ///   &object_sp)
   ///

diff  --git a/lldb/include/lldb/Target/StructuredDataPlugin.h 
b/lldb/include/lldb/Target/StructuredDataPlugin.h
index 09241d5281fb4..09111b1390505 100644
--- a/lldb/include/lldb/Target/StructuredDataPlugin.h
+++ b/lldb/include/lldb/Target/StructuredDataPlugin.h
@@ -64,7 +64,7 @@ class StructuredDataPlugin
   ///
   /// \return
   /// true if the plugin supports the feature; otherwise, false.
-  virtual bool SupportsStructuredDataType(ConstString type_name) = 0;
+  virtual bool SupportsStructuredDataType(llvm::StringRef type_name) = 0;
 
   /// Handle the arrival of asynchronous structured data from the process.
   ///
@@ -92,7 +92,7 @@ class StructuredDataPlugin
   /// key named "type" that must be a string value containing the
   /// structured data type name.
   virtual void
-  HandleArrivalOfStructuredData(Process &process, ConstString type_name,
+  HandleArrivalOfStructuredData(Process &process, llvm::StringRef type_name,
 const StructuredData::ObjectSP &object_sp) = 0;
 
   /// Get a human-readable description of the contents of the data.
@@ -124,7 +124,7 @@ class StructuredDataPlugin
   /// \param[in] type_name
   /// The name of the feature tag for the asynchronous structured data.
   /// This is needed for plugins that support more than one feature.
-  virtual bool GetEnabled(ConstString type_name) const;
+  virtual bool GetEnabled(llvm::StringRef type_name) const;
 
   /// Allow the plugin to do work related to modules that loaded in the
   /// the corresponding process.

diff  --git 
a/lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp 
b/lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp
index 611e1506d8581..60e94b89e37e9 100644
--- a/lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp
+++ b/lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp
@@ -875,9 +875,10 @@ class StatusCommand : public CommandObjectParsed {
   process_sp->GetStructuredDataPlugin(GetDarwinLogTypeName());
   stream.Printf("Availability: %s\n",
 plugin_sp ? "available" : "unavailable");
-  llvm::StringRef plugin_name = 
StructuredDataDarwinLog::GetStaticPluginName();
   const bool enabled =
-  plugin_sp ? plugin_sp->GetEnabled(ConstString(plugin_name)) : false;
+  plugin_sp ? plugin_sp->GetEnabled(
+  StructuredDataDarwinLog::GetStaticPluginName())
+: false;
   stream.Printf("Enabled: %s\n", enabled ? "true" : "false");
 }
 
@@ -1057,12 +1058,12 @@ void StructuredDataDarwinLog::Terminate() {
 // StructuredDataPlugin API
 
 bool StructuredDataDarwinLog::SupportsStructuredDataType(
-ConstString type_name) {
+llvm::StringRef type_name) {
   return type_name == GetDarwinLogTypeName();
 }
 
 void StructuredDataDarwinLog::HandleArrivalOfStructuredData(
-Process &process, ConstString type_name,
+Process &process, llvm::StringRef type_name,
 const StructuredData::ObjectSP &object_sp) {
   Log *log = GetLog(LLDBLog::Process);
   if (log) {
@@ -1086,11 +1087,9 @@ void 
StructuredDataDarwinLog::HandleArrivalOfStructuredData(
 
   // Ignore any data that isn't for us.
   if (type_name != GetDarwinLogTypeName()) {
-LLDB_LOGF(log,
-  "StructuredDataDarwinLog::%s() StructuredD

[Lldb-commits] [PATCH] D153482: [lldb][NFCI] Remove use of ConstString from StructuredDataPlugin

[Alex Langford via Phabricator via lldb-commits]

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153482

Files:
  lldb/include/lldb/Target/Process.h
  lldb/include/lldb/Target/StructuredDataPlugin.h
  lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp
  lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.h
  lldb/source/Target/Process.cpp
  lldb/source/Target/StructuredDataPlugin.cpp

Index: lldb/source/Target/StructuredDataPlugin.cpp
===
--- lldb/source/Target/StructuredDataPlugin.cpp
+++ lldb/source/Target/StructuredDataPlugin.cpp
@@ -32,7 +32,7 @@
 
 StructuredDataPlugin::~StructuredDataPlugin() = default;
 
-bool StructuredDataPlugin::GetEnabled(ConstString type_name) const {
+bool StructuredDataPlugin::GetEnabled(llvm::StringRef type_name) const {
   // By default, plugins are always enabled.  Plugin authors should override
   // this if there is an enabled/disabled state for their plugin.
   return true;
Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -6070,7 +6070,7 @@
 // For any of the remaining type names, map any that this plugin supports.
 std::vector names_to_remove;
 for (llvm::StringRef type_name : type_names) {
-  if (plugin_sp->SupportsStructuredDataType(ConstString(type_name))) {
+  if (plugin_sp->SupportsStructuredDataType(type_name)) {
 m_structured_data_plugin_map.insert(
 std::make_pair(type_name, plugin_sp));
 names_to_remove.push_back(type_name);
@@ -6110,8 +6110,7 @@
   }
 
   // Route the structured data to the plugin.
-  find_it->second->HandleArrivalOfStructuredData(*this, ConstString(type_name),
- object_sp);
+  find_it->second->HandleArrivalOfStructuredData(*this, type_name, object_sp);
   return true;
 }
 
Index: lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.h
===
--- lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.h
+++ lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.h
@@ -50,16 +50,16 @@
 
   // StructuredDataPlugin API
 
-  bool SupportsStructuredDataType(ConstString type_name) override;
+  bool SupportsStructuredDataType(llvm::StringRef type_name) override;
 
   void HandleArrivalOfStructuredData(
-  Process &process, ConstString type_name,
+  Process &process, llvm::StringRef type_name,
   const StructuredData::ObjectSP &object_sp) override;
 
   Status GetDescription(const StructuredData::ObjectSP &object_sp,
 lldb_private::Stream &stream) override;
 
-  bool GetEnabled(ConstString type_name) const override;
+  bool GetEnabled(llvm::StringRef type_name) const override;
 
   void ModulesDidLoad(Process &process, ModuleList &module_list) override;
 
Index: lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp
===
--- lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp
+++ lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp
@@ -875,9 +875,10 @@
   process_sp->GetStructuredDataPlugin(GetDarwinLogTypeName());
   stream.Printf("Availability: %s\n",
 plugin_sp ? "available" : "unavailable");
-  llvm::StringRef plugin_name = StructuredDataDarwinLog::GetStaticPluginName();
   const bool enabled =
-  plugin_sp ? plugin_sp->GetEnabled(ConstString(plugin_name)) : false;
+  plugin_sp ? plugin_sp->GetEnabled(
+  StructuredDataDarwinLog::GetStaticPluginName())
+: false;
   stream.Printf("Enabled: %s\n", enabled ? "true" : "false");
 }
 
@@ -1057,12 +1058,12 @@
 // StructuredDataPlugin API
 
 bool StructuredDataDarwinLog::SupportsStructuredDataType(
-ConstString type_name) {
+llvm::StringRef type_name) {
   return type_name == GetDarwinLogTypeName();
 }
 
 void StructuredDataDarwinLog::HandleArrivalOfStructuredData(
-Process &process, ConstString type_name,
+Process &process, llvm::StringRef type_name,
 const StructuredData::ObjectSP &object_sp) {
   Log *log = GetLog(LLDBLog::Process);
   if (log) {
@@ -1086,11 +1087,9 @@
 
   // Ignore any data that isn't for us.
   if (type_name != GetDarwinLogTypeName()) {
-LLDB_LOGF(log,
-  "StructuredDataDarwinLog::%s() StructuredData type "
-  "expected to be %s but was %s, ignoring",
-  __FUNCTION__, GetDarwinLogTypeName().str().c_str(),
-   

[Lldb-commits] [PATCH] D153648: [lldb] Remove broken shared cache duplicate class handling

[Dave Lee via Phabricator via lldb-commits]

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

The shared cache list of ObjC classes contains classes that are duplicate in 
name (which
may or may not be duplicate in definition).

Currently, lldb does not handle this list of duplicates correctly. The bugs in 
the
handling of duplicate classes are in the for loop over the duplicates, which 
has the
following issues:

1. Indexing into the unique class list (`classOffsets`), not the duplicate list 
(`duplicateClassOffsets`)
2. Not incrementing `idx`

The result is that the first N unique classes are wastefully rehashed, where N 
is
`duplicates_count` (which can be large).

Note that this change removes the loop over the duplicates altogether, to avoid 
paying
the cost of ultimately doing nothing.

Ideally, the above bugs could be addressed, however lldb doesn't know which of 
the
duplicates the ObjC runtime has chosen. When the ObjC runtime registers 
duplicate
classes, it emits the following error:

> Class  is implemented in both libA.dylib (0x1048800b8) and libB.dylib
> (0x1048700b8). One of the two will be used. Which one is undefined.

In lldb, the code that uses results of this scan does class lookup by name, and 
doesn't
handle the case where there are multiple classes for a given name. Also, lldb 
doesn't
know which of the duplicates the ObjC runtime has chosen.

The ultimate fix is to determine which duplicate the ObjC runtime has chosen, 
and have
lldb use that too.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D153648

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


Index: 
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
===
--- 
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
+++ 
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
@@ -527,48 +527,7 @@
 DEBUG_PRINTF ("duplicate_count = %u\n", duplicate_count);
 DEBUG_PRINTF ("duplicateClassOffsets = %p\n", 
duplicateClassOffsets);
 
-for (uint32_t i=0; iversion >= 12 && objc_opt->version <= 15)
 {


Index: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
===
--- lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
+++ lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
@@ -527,48 +527,7 @@
 DEBUG_PRINTF ("duplicate_count = %u\n", duplicate_count);
 DEBUG_PRINTF ("duplicateClassOffsets = %p\n", duplicateClassOffsets);
 
-for (uint32_t i=0; iversion >= 12 && objc_opt->version <= 15)
 {
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D153644: [lldb] Print unprintable characters as unsigned

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D153644#679 , @bulbazord wrote:

> Makes sense to me. Does it make sense to add a test case where we mix 
> "printable" characters with "non-printable" ones? I see you have a `\f` at 
> the end there, but that's a special escaped character. Something like 
> `"\xef\xbb\xbfHello World!\n"`.

Good idea, I'll add that before landing this.


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

https://reviews.llvm.org/D153644

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

[Lldb-commits] [lldb] 85f40fc - [lldb] Print unprintable characters as unsigned

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2023-06-23T11:00:21-07:00
New Revision: 85f40fc676df65416568750f328c71758c84264a

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

LOG: [lldb] Print unprintable characters as unsigned

When specifying the C-string format for dumping memory, we treat
unprintable characters as signed. Whether a character is signed or not
is implementation defined, but all printable characters are signed.
Therefore it's fair to assume that unprintable characters are unsigned.

Before this patch, "\xcf\xfa\xed\xfe\f" would be printed as
"\xffcf\xfffa\xffed\xfffe\f". Now we correctly print the
original string.

rdar://26134

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

Added: 


Modified: 
lldb/source/Core/DumpDataExtractor.cpp
lldb/unittests/Core/DumpDataExtractorTest.cpp

Removed: 




diff  --git a/lldb/source/Core/DumpDataExtractor.cpp 
b/lldb/source/Core/DumpDataExtractor.cpp
index 7f6108f40c59f..728297a3a5ad8 100644
--- a/lldb/source/Core/DumpDataExtractor.cpp
+++ b/lldb/source/Core/DumpDataExtractor.cpp
@@ -212,7 +212,8 @@ static void DumpCharacter(Stream &s, const char c) {
 s.PutChar(c);
 return;
   }
-  s.Printf("\\x%2.2x", c);
+  // Non-print characters can be assumed to be unsigned.
+  s.Printf("\\x%2.2x", static_cast(c));
 }
 
 /// Dump a floating point type.

diff  --git a/lldb/unittests/Core/DumpDataExtractorTest.cpp 
b/lldb/unittests/Core/DumpDataExtractorTest.cpp
index 226f2b7869664..bbe5e9e5ed9e2 100644
--- a/lldb/unittests/Core/DumpDataExtractorTest.cpp
+++ b/lldb/unittests/Core/DumpDataExtractorTest.cpp
@@ -131,8 +131,16 @@ TEST(DumpDataExtractorTest, Formats) {
   // set of bytes to match the 10 byte format but then if the test runs on a
   // machine where we don't use 10 it'll break.
 
+  // Test printable characters.
   TestDump(llvm::StringRef("aardvark"), lldb::Format::eFormatCString,
"\"aardvark\"");
+  // Test unprintable characters.
+  TestDump(llvm::StringRef("\xcf\xfa\xed\xfe\f"), lldb::Format::eFormatCString,
+   "\"\\xcf\\xfa\\xed\\xfe\\f\"");
+  // Test a mix of printable and unprintable characters.
+  TestDump(llvm::StringRef("\xcf\xfa\ffoo"), lldb::Format::eFormatCString,
+   "\"\\xcf\\xfa\\ffoo\"");
+
   TestDump(99, lldb::Format::eFormatDecimal, "99");
   // Just prints as a signed integer.
   TestDump(-1, lldb::Format::eFormatEnum, "-1");



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

[Lldb-commits] [PATCH] D153644: [lldb] Print unprintable characters as unsigned

[Jonas Devlieghere via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG85f40fc676df: [lldb] Print unprintable characters as 
unsigned (authored by JDevlieghere).
Herald added a project: LLDB.

Changed prior to commit:
  https://reviews.llvm.org/D153644?vs=533985&id=534020#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153644

Files:
  lldb/source/Core/DumpDataExtractor.cpp
  lldb/unittests/Core/DumpDataExtractorTest.cpp


Index: lldb/unittests/Core/DumpDataExtractorTest.cpp
===
--- lldb/unittests/Core/DumpDataExtractorTest.cpp
+++ lldb/unittests/Core/DumpDataExtractorTest.cpp
@@ -131,8 +131,16 @@
   // set of bytes to match the 10 byte format but then if the test runs on a
   // machine where we don't use 10 it'll break.
 
+  // Test printable characters.
   TestDump(llvm::StringRef("aardvark"), lldb::Format::eFormatCString,
"\"aardvark\"");
+  // Test unprintable characters.
+  TestDump(llvm::StringRef("\xcf\xfa\xed\xfe\f"), lldb::Format::eFormatCString,
+   "\"\\xcf\\xfa\\xed\\xfe\\f\"");
+  // Test a mix of printable and unprintable characters.
+  TestDump(llvm::StringRef("\xcf\xfa\ffoo"), lldb::Format::eFormatCString,
+   "\"\\xcf\\xfa\\ffoo\"");
+
   TestDump(99, lldb::Format::eFormatDecimal, "99");
   // Just prints as a signed integer.
   TestDump(-1, lldb::Format::eFormatEnum, "-1");
Index: lldb/source/Core/DumpDataExtractor.cpp
===
--- lldb/source/Core/DumpDataExtractor.cpp
+++ lldb/source/Core/DumpDataExtractor.cpp
@@ -212,7 +212,8 @@
 s.PutChar(c);
 return;
   }
-  s.Printf("\\x%2.2x", c);
+  // Non-print characters can be assumed to be unsigned.
+  s.Printf("\\x%2.2x", static_cast(c));
 }
 
 /// Dump a floating point type.


Index: lldb/unittests/Core/DumpDataExtractorTest.cpp
===
--- lldb/unittests/Core/DumpDataExtractorTest.cpp
+++ lldb/unittests/Core/DumpDataExtractorTest.cpp
@@ -131,8 +131,16 @@
   // set of bytes to match the 10 byte format but then if the test runs on a
   // machine where we don't use 10 it'll break.
 
+  // Test printable characters.
   TestDump(llvm::StringRef("aardvark"), lldb::Format::eFormatCString,
"\"aardvark\"");
+  // Test unprintable characters.
+  TestDump(llvm::StringRef("\xcf\xfa\xed\xfe\f"), lldb::Format::eFormatCString,
+   "\"\\xcf\\xfa\\xed\\xfe\\f\"");
+  // Test a mix of printable and unprintable characters.
+  TestDump(llvm::StringRef("\xcf\xfa\ffoo"), lldb::Format::eFormatCString,
+   "\"\\xcf\\xfa\\ffoo\"");
+
   TestDump(99, lldb::Format::eFormatDecimal, "99");
   // Just prints as a signed integer.
   TestDump(-1, lldb::Format::eFormatEnum, "-1");
Index: lldb/source/Core/DumpDataExtractor.cpp
===
--- lldb/source/Core/DumpDataExtractor.cpp
+++ lldb/source/Core/DumpDataExtractor.cpp
@@ -212,7 +212,8 @@
 s.PutChar(c);
 return;
   }
-  s.Printf("\\x%2.2x", c);
+  // Non-print characters can be assumed to be unsigned.
+  s.Printf("\\x%2.2x", static_cast(c));
 }
 
 /// Dump a floating point type.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D153648: [lldb] Remove broken shared cache duplicate class handling

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

Both issues are not present in the code that does the equivalent parsing for 
Objective-C versions 12 through 15. Most likely I accidentally introduced these 
bugs when copy/pasting this for the changes in 16. I would prefer to use that 
rather than diverging our handling of duplicates based on the Objective-C 
runtime version.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153648

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

[Lldb-commits] [PATCH] D153644: [lldb] Print unprintable characters as unsigned

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

It seems like this patch is really avoiding the sign extension of an int8_t to 
int32_t in the process of being passed to printf.  By casting it to unsigned 
we've avoided the sign extension and getting the correct result, but it seems 
like you could have used a printf formatter like `s.Printf("\\x%2.2hhx", c);`.  
I'm not opposed to solving it this way, but the description of why it's being 
cast to unsigned is not really clear imo.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153644

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

[Lldb-commits] [PATCH] D153648: [lldb] Remove broken shared cache duplicate class handling

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

There's similar code for the runtime versions 12-15 which actually looks 
correct.  It seems to claim that the duplicate classes are all stuffed after 
the capacity in the classOffsets array and that the one that won will indeed be 
in that list but will not be marked as a duplicate.  It is also a little bogus, 
I think it would only support one duplicate.  Did it not work to fix what look 
like transcription errors going from the 12-15 version of this code to the 16 
version?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153648

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

[Lldb-commits] [PATCH] D153657: Don't allow ValueObject::Cast from a smaller type to a larger

[Jim Ingham via Phabricator via lldb-commits]

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

SBValue::Cast actually allows casting from a struct type to another struct 
type.  That's a little odd, C-family languages don't allow this, but we have 
done forever, so I don't want to remove the ability altogether.  However, we 
can't support casting from a small structure to a larger one because in some 
cases - e.g. all the ConstResult types used both for expression results and for 
many synthetic children - we don't know where we should fetch the extra bits 
from.  Just zero-filling them seems odd, and error seems like a better response.

This fixes a bug where when casting an expression result from a smaller type to 
a larger, lldb would present the memory in lldb after the ValueObject's data 
buffer as the value of the cast type.  Again, I could have fixed that bug by 
expanding the data buffer to match the larger size, but I wouldn't know what to 
fill it with.

There were two places in the C++ formatters that were using this cast from 
struct to struct type to change a C++ std typedef (e.g. 
std::shared_ptr::element type *) to a type that is more useful to the user 
(pointer to the first template argument's type).  The cast from struct to 
struct in that case wasn't necessary, and looked weird since this really isn't 
an allowed C++ operation.  So I also changed those to case the pointer first, 
then dereference the cast value.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D153657

Files:
  lldb/include/lldb/Core/ValueObject.h
  lldb/include/lldb/Core/ValueObjectConstResult.h
  lldb/include/lldb/Core/ValueObjectConstResultCast.h
  lldb/include/lldb/Core/ValueObjectConstResultChild.h
  lldb/packages/Python/lldbsuite/test/lldbtest.py
  lldb/source/Core/ValueObject.cpp
  lldb/source/Core/ValueObjectConstResult.cpp
  lldb/source/Core/ValueObjectConstResultCast.cpp
  lldb/source/Core/ValueObjectConstResultChild.cpp
  lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
  lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
  lldb/test/API/python_api/value/TestValueAPI.py
  lldb/test/API/python_api/value/main.c

Index: lldb/test/API/python_api/value/main.c
===
--- lldb/test/API/python_api/value/main.c
+++ lldb/test/API/python_api/value/main.c
@@ -29,6 +29,13 @@
   int b;
 };
 
+struct MyBiggerStruct
+{
+  int a;
+  int b;
+  int c;
+};
+
 int main (int argc, char const *argv[])
 {
 uint32_t uinthex = 0xE0A35F10;
@@ -37,6 +44,7 @@
 int i;
 MyInt a = 12345;
 struct MyStruct s = { 11, 22 };
+struct MyBiggerStruct f = { 33, 44, 55 }; 
 int *my_int_ptr = &g_my_int;
 printf("my_int_ptr points to location %p\n", my_int_ptr);
 const char **str_ptr = days_of_week;
Index: lldb/test/API/python_api/value/TestValueAPI.py
===
--- lldb/test/API/python_api/value/TestValueAPI.py
+++ lldb/test/API/python_api/value/TestValueAPI.py
@@ -146,6 +146,19 @@
 self.assertTrue(val_s.GetChildMemberWithName("a").AddressOf(), VALID_VARIABLE)
 self.assertTrue(val_a.Cast(val_i.GetType()).AddressOf(), VALID_VARIABLE)
 
+# Test some other cases of the Cast API.  We allow casts from one struct type
+# to another, which is a little weird, but we don't support casting from a
+# smaller type to a larger as we often wouldn't know how to get the extra data:
+val_f = target.EvaluateExpression("f")
+bad_cast = val_s.Cast(val_f.GetType())
+self.assertFailure(bad_cast.GetError(),
+   "Can only cast to a type that is equal to or smaller than the orignal type.")
+weird_cast = val_f.Cast(val_s.GetType())
+self.assertSuccess(weird_cast.GetError(),
+"Can cast from a larger to a smaller")
+self.assertEqual(weird_cast.GetChildMemberWithName("a").GetValueAsSigned(0), 33,
+ "Got the right value")
+
 # Check that lldb.value implements truth testing.
 self.assertFalse(lldb.value(frame0.FindVariable("bogus")))
 self.assertTrue(lldb.value(frame0.FindVariable("uinthex")))
Index: lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
@@ -157,7 +157,11 @@
   }
   if (!m_node_type)
 return nullptr;
-  node_sp = node_sp->Cast(m_node_type);
+  node_sp = m_next_element->Cast(m_node_type.GetPointerType())
+  ->Dereference(error);
+  if (!node_sp || error.Fail())
+  return nullptr;
+
   value_sp = node_sp->GetChildMemberWit

[Lldb-commits] [lldb] c0045a8 - [lldb] Use format specific for unprintabe char in DumpDataExtractor

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2023-06-23T11:56:41-07:00
New Revision: c0045a8e8e0c72a0c8be3a9c333885da4d14d472

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

LOG: [lldb] Use format specific for unprintabe char in DumpDataExtractor

Addresses Jason's post-commit feedback in D153644.

Added: 


Modified: 
lldb/source/Core/DumpDataExtractor.cpp

Removed: 




diff  --git a/lldb/source/Core/DumpDataExtractor.cpp 
b/lldb/source/Core/DumpDataExtractor.cpp
index 728297a3a5ad8..cb76b118325b7 100644
--- a/lldb/source/Core/DumpDataExtractor.cpp
+++ b/lldb/source/Core/DumpDataExtractor.cpp
@@ -212,8 +212,7 @@ static void DumpCharacter(Stream &s, const char c) {
 s.PutChar(c);
 return;
   }
-  // Non-print characters can be assumed to be unsigned.
-  s.Printf("\\x%2.2x", static_cast(c));
+  s.Printf("\\x%2.2hhx", c);
 }
 
 /// Dump a floating point type.



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

[Lldb-commits] [PATCH] D153644: [lldb] Print unprintable characters as unsigned

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D153644#891 , @jasonmolenda 
wrote:

> It seems like this patch is really avoiding the sign extension of an int8_t 
> to int32_t in the process of being passed to printf.  By casting it to 
> unsigned we've avoided the sign extension and getting the correct result, but 
> it seems like you could have used a printf formatter like 
> `s.Printf("\\x%2.2hhx", c);`.  I'm not opposed to solving it this way, but 
> the description of why it's being cast to unsigned is not really clear imo.

Good point. Went with that in c0045a8e8e0c72a0c8be3a9c333885da4d14d472.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153644

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

[Lldb-commits] [PATCH] D153648: [lldb] Remove broken shared cache duplicate class handling

[Dave Lee via Phabricator via lldb-commits]

kastiglione added a comment.

In D153648#898 , @jingham wrote:

> and that the one that won will indeed be in that list but will not be marked 
> as a duplicate.

All of the duplicate classes are in the duplicate list, not a single class that 
won. For example, a duplicated class, say `ABCSomeController`, will be in the 
duplicate list 2 or more times. The result is that there's redundant hashing, 
since each of the duplicate names occurs 2 or more times. Which is maybe fine, 
or maybe causes a perf penalty. Another result is that 
`ObjCLanguageRuntime::m_hash_to_isa_map` (a `multimap`) ends up storing every 
duplicate, however the readers of `m_hash_to_isa_map` always take the first 
match found (see `ObjCLanguageRuntime::GetDescriptorIterator`). In other words, 
the redundant classes are never accessed, and the one lldb does return could 
differ from the class the objc runtime has picked. A "fix" to the surface level 
bugs identified here has the result of exposing a bunch of non-determinism.

My thought process was: "This is broken to the point that duplicate classes are 
currently being dropped altogether, and nobody has identified this as a 
problem. Maybe delete the code until we can figure out a proper fix."

I am fine with the "fix", but as you can see I am very skeptical in it being 
helpful.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153648

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

[Lldb-commits] [PATCH] D153657: Don't allow ValueObject::Cast from a smaller type to a larger

[Jonas Devlieghere via Phabricator via lldb-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153657

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

[Lldb-commits] [PATCH] D153657: Don't allow ValueObject::Cast from a smaller type to a larger

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

I think this patch is probably okay to do, but it does break a supported use 
case that I'm aware of: One way you can do "object oriented programming" in C 
is to do exactly what you're trying to prevent. Using the test, you could say 
that "MyStruct" is the base class and "MyBiggerStruct" would be a derived 
class. You might have a pointer to a "MyStruct" object, even though you called 
`malloc(sizeof(MyBiggerStruct))`, and maybe you can perform that cast if you're 
sure that you actually have a `MyBiggerStruct` object.

I know there's not necessarily a good way to support that without also 
supporting the bug you're trying to fix though. :(




Comment at: lldb/include/lldb/Core/ValueObject.h:617-619
+  lldb::ValueObjectSP Cast(const CompilerType &compiler_type);
+
+  virtual lldb::ValueObjectSP DoCast(const CompilerType &compiler_type);

I'm a little concerned about the API surface here. Maybe this is just my 
misunderstandings about ValueObject, but...

Given a `ValueObjectSP` (which may contain a `ValueObject`, 
`ValueObjectConstResult`, etc), which one of these are you supposed to call? If 
you call `Cast`, you'll get what you want. If you call `DoCast`, you might 
circumvent the safety checks that `Cast` is providing... but if you have 
something like a `ValueObjectConstResult`, this actually calls 
`ValueObject::Cast` which does the right thing. Am I understanding this 
correctly?

I also have a little confusion about which one I would call based on the 
names... Maybe it would make more sense to call `DoCast` something like 
`CastImpl`? 



Comment at: lldb/source/Core/ValueObject.cpp:2792
+  CompilerType my_type = GetCompilerType();
+  bool unused;
+

What is the purpose of this `bool unused`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153657

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

[Lldb-commits] [PATCH] D153673: [lldb][NFCI] Remove unneeded ConstString constructions for OptionValueProperties::AppendProperty

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

I removed ConstString from OptionValueProperties in 643ba926c1f6 
, but
there are a few call sites that still create a ConstString as an
argument. I did not catch these initially because ConstString has an
implicit conversion method to StringRef.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D153673

Files:
  lldb/source/Core/Debugger.cpp
  lldb/source/Core/PluginManager.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
@@ -85,11 +85,10 @@
 const bool is_global = false;
 
 auto dict_sp = std::make_shared(1 << eTypeUInt64);
-props_sp->AppendProperty(ConstString("dict"), "", is_global, dict_sp);
+props_sp->AppendProperty("dict", "", is_global, dict_sp);
 
 auto file_list_sp = std::make_shared();
-props_sp->AppendProperty(ConstString("file-list"), "", is_global,
- file_list_sp);
+props_sp->AppendProperty("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
@@ -4112,7 +4112,7 @@
 m_experimental_properties_up =
 std::make_unique();
 m_collection_sp->AppendProperty(
-ConstString(Properties::GetExperimentalSettingsName()),
+Properties::GetExperimentalSettingsName(),
 "Experimental settings - setting these won't produce "
 "errors if the setting is not present.",
 true, m_experimental_properties_up->GetValueProperties());
@@ -4123,12 +4123,12 @@
 m_experimental_properties_up =
 std::make_unique();
 m_collection_sp->AppendProperty(
-ConstString(Properties::GetExperimentalSettingsName()),
+Properties::GetExperimentalSettingsName(),
 "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"), "Settings specific to processes.", true,
+"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,7 +167,7 @@
 std::make_shared(ConstString("process"));
 m_collection_sp->Initialize(g_process_properties);
 m_collection_sp->AppendProperty(
-ConstString("thread"), "Settings specific to threads.", true,
+"thread", "Settings specific to threads.", true,
 Thread::GetGlobalProperties().GetValueProperties());
   } else {
 m_collection_sp =
@@ -180,7 +180,7 @@
   m_experimental_properties_up =
   std::make_unique();
   m_collection_sp->AppendProperty(
-  ConstString(Properties::GetExperimentalSettingsName()),
+  Properties::GetExperimentalSettingsName(),
   "Experimental settings - setting these won't produce "
   "errors if the setting is not present.",
   true, m_experimental_properties_up->GetValueProperties());
Index: lldb/source/Core/PluginManager.cpp
===
--- lldb/source/Core/PluginManager.cpp
+++ lldb/source/Core/PluginManager.cpp
@@ -1438,7 +1438,7 @@
   lldb::OptionValuePropertiesSP parent_properties_sp(
   debugger.GetValueProperties());
   if (parent_properties_sp) {
-static ConstString g_property_name("plugin");
+static constexpr llvm::StringLiteral g_property_name("plugin");
 
 OptionValuePropertiesSP plugin_properties_sp =
 parent_properties_sp->GetSubProperty(nullptr, g_property_name);
@@ -1471,7 +1471,7 @@
 static lldb::OptionValuePropertiesSP GetDebuggerPropertyForPluginsOldStyle(
 Debugger &debugger, ConstString plugin_type_name,
 llvm::StringRef plugin_type_desc, bool can_create) {
-  static ConstString g_property_name("plugin");
+  static constexpr llvm::StringLiteral g_property_name("plugin");
   lldb::OptionValuePropertiesSP parent_properties_sp(
   debugger.GetValueProperties());
   if (parent_properties_sp) {
Index: lldb/source/Core/Debugger.cpp
==

[Lldb-commits] [PATCH] D153675: [lldb][NFCI] Remove ConstString from Process::ConfigureStructuredData

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

This is a follow-up to b4827a3c0a7ef121ca376713e115b04eff0f5194 
.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D153675

Files:
  lldb/include/lldb/Target/Process.h
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
  lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp
  lldb/source/Target/Process.cpp


Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -6007,7 +6007,7 @@
 }
 
 Status
-Process::ConfigureStructuredData(ConstString type_name,
+Process::ConfigureStructuredData(llvm::StringRef type_name,
  const StructuredData::ObjectSP &config_sp) {
   // If you get this, the Process-derived class needs to implement a method to
   // enable an already-reported asynchronous structured data feature. See
Index: lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp
===
--- lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp
+++ lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp
@@ -822,8 +822,8 @@
 // Send configuration to the feature by way of the process. Construct the
 // options we will use.
 auto config_sp = m_options_sp->BuildConfigurationData(m_enable);
-const Status error = process_sp->ConfigureStructuredData(
-ConstString(GetDarwinLogTypeName()), config_sp);
+const Status error =
+process_sp->ConfigureStructuredData(GetDarwinLogTypeName(), config_sp);
 
 // Report results.
 if (!error.Success()) {
@@ -1831,8 +1831,8 @@
 
   // We can run it directly.
   // Send configuration to the feature by way of the process.
-  const Status error = process_sp->ConfigureStructuredData(
-  ConstString(GetDarwinLogTypeName()), config_sp);
+  const Status error =
+  process_sp->ConfigureStructuredData(GetDarwinLogTypeName(), config_sp);
 
   // Report results.
   if (!error.Success()) {
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
===
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
@@ -211,7 +211,7 @@
  lldb::addr_t image_count) override;
 
   Status
-  ConfigureStructuredData(ConstString type_name,
+  ConfigureStructuredData(llvm::StringRef type_name,
   const StructuredData::ObjectSP &config_sp) override;
 
   StructuredData::ObjectSP GetLoadedDynamicLibrariesInfos() override;
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
@@ -3922,7 +3922,7 @@
 }
 
 Status ProcessGDBRemote::ConfigureStructuredData(
-ConstString type_name, const StructuredData::ObjectSP &config_sp) {
+llvm::StringRef type_name, const StructuredData::ObjectSP &config_sp) {
   return m_gdb_comm.ConfigureRemoteStructuredData(type_name, config_sp);
 }
 
Index: lldb/include/lldb/Target/Process.h
===
--- lldb/include/lldb/Target/Process.h
+++ lldb/include/lldb/Target/Process.h
@@ -2537,7 +2537,7 @@
   /// \return
   /// Returns the result of attempting to configure the feature.
   virtual Status
-  ConfigureStructuredData(ConstString type_name,
+  ConfigureStructuredData(llvm::StringRef type_name,
   const StructuredData::ObjectSP &config_sp);
 
   /// Broadcasts the given structured data object from the given plugin.


Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -6007,7 +6007,7 @@
 }
 
 Status
-Process::ConfigureStructuredData(ConstString type_name,
+Process::ConfigureStructuredData(llvm::StringRef type_name,
  const StructuredData::ObjectSP &config_sp) {
   // If you get this, the Process-derived class needs to implement a method to
   // enable an already-reported asynchronous structured data feature. See
Index: lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp
===
--- lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLo

[Lldb-commits] [PATCH] D153685: [lldb] Add `source cache dump` and `source cache clear` subcommand

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere created this revision.
JDevlieghere added reviewers: bulbazord, clayborg, jingham, jasonmolenda.
Herald added a project: All.
JDevlieghere requested review of this revision.

Add two new `source` subcommands: `source cache dump` and `source cache clear`. 
As the name implies the first one dumps the source cache while the later clears 
the cache.

This patch was motivated by a handful of (internal) bug reports related to 
sources not being available. Right now those issues can be hard to diagnose. 
The new commands give users, as well as us as developers, more insight into and 
control over the source cache.


https://reviews.llvm.org/D153685

Files:
  lldb/include/lldb/Core/Debugger.h
  lldb/include/lldb/Core/SourceManager.h
  lldb/source/Commands/CommandObjectSource.cpp
  lldb/source/Core/SourceManager.cpp
  lldb/test/API/source-manager/TestSourceManager.py

Index: lldb/test/API/source-manager/TestSourceManager.py
===
--- lldb/test/API/source-manager/TestSourceManager.py
+++ lldb/test/API/source-manager/TestSourceManager.py
@@ -317,3 +317,24 @@
 "that has no source code associated " "with it.",
 ],
 )
+
+def test_source_cache_dump_and_clear(self):
+self.build()
+exe = self.getBuildArtifact("a.out")
+self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+lldbutil.run_break_set_by_file_and_line(
+self, self.file, self.line, num_expected_locations=1, loc_exact=True
+)
+self.runCmd("run", RUN_SUCCEEDED)
+
+# Make sure the main source file is in the source cache.
+self.expect(
+"source cache dump",
+substrs=["Modification time", "Lines", "Path", " 7", self.file],
+)
+
+# Clear the cache.
+self.expect("source cache clear")
+
+# Make sure the main source file is no longer in the source cache.
+self.expect("source cache dump", matching=False, substrs=[self.file])
Index: lldb/source/Core/SourceManager.cpp
===
--- lldb/source/Core/SourceManager.cpp
+++ lldb/source/Core/SourceManager.cpp
@@ -727,3 +727,15 @@
 file_sp = pos->second;
   return file_sp;
 }
+
+void SourceManager::SourceFileCache::Dump(Stream &stream) const {
+  stream << "Modification time   LinesPath\n";
+  stream << "---  \n";
+  for (auto &entry : m_file_cache) {
+if (!entry.second)
+  continue;
+FileSP file = entry.second;
+stream.Format("{0:%Y-%m-%d %H:%M:%S} {1,8:d} {2}\n", file->GetTimestamp(),
+  file->GetNumLines(), entry.first.GetPath());
+  }
+}
Index: lldb/source/Commands/CommandObjectSource.cpp
===
--- lldb/source/Commands/CommandObjectSource.cpp
+++ lldb/source/Commands/CommandObjectSource.cpp
@@ -1201,6 +1201,62 @@
   std::string m_reverse_name;
 };
 
+class CommandObjectSourceCacheDump : public CommandObjectParsed {
+public:
+  CommandObjectSourceCacheDump(CommandInterpreter &interpreter)
+  : CommandObjectParsed(interpreter, "source cache dump",
+"Dump the state of the source code cache.\n"
+"Intended to be used for debugging LLDB itself.",
+nullptr) {}
+
+  ~CommandObjectSourceCacheDump() override = default;
+
+protected:
+  bool DoExecute(Args &command, CommandReturnObject &result) override {
+SourceManager::SourceFileCache &cache = GetDebugger().GetSourceFileCache();
+cache.Dump(result.GetOutputStream());
+result.SetStatus(eReturnStatusSuccessFinishResult);
+return result.Succeeded();
+  }
+};
+
+class CommandObjectSourceCacheClear : public CommandObjectParsed {
+public:
+  CommandObjectSourceCacheClear(CommandInterpreter &interpreter)
+  : CommandObjectParsed(interpreter, "source cache clear",
+"Clear the source code cache.\n", nullptr) {}
+
+  ~CommandObjectSourceCacheClear() override = default;
+
+protected:
+  bool DoExecute(Args &command, CommandReturnObject &result) override {
+SourceManager::SourceFileCache &cache = GetDebugger().GetSourceFileCache();
+cache.Clear();
+result.SetStatus(eReturnStatusSuccessFinishNoResult);
+return result.Succeeded();
+  }
+};
+
+class CommandObjectSourceCache : public CommandObjectMultiword {
+public:
+  CommandObjectSourceCache(CommandInterpreter &interpreter)
+  : CommandObjectMultiword(interpreter, "source cache",
+   "Commands for managing the source code cache.",
+   "source cache ") {
+LoadSubCommand(
+"dump", CommandObjectSP(new CommandObjectSourceCacheDump(interpreter)));
+LoadSubCommand("clear", CommandObjectSP(new CommandObjectSourceCacheClear(
+interpreter)));

[Lldb-commits] [PATCH] D153673: [lldb][NFCI] Remove unneeded ConstString constructions for OptionValueProperties::AppendProperty

[Jonas Devlieghere via Phabricator via lldb-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153673

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