[nfc] [lldb] Prevent needless copies of DataExtractor

lldb_private::DataExtractor contains DataBufferSP m_data_sp which is
relatively expensive to copy (due to multi-threading locking).

llvm::DataExtractor does not have this problem as it uses StringRef
instead.

The copy constructor is explicit as otherwise it is easy to make
unintended modification of a local copy instead of a caller's instance
(D107470 but that is llvm::DataExtractor).

Reviewed By: clayborg

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

diff --git a/lldb/include/lldb/DataFormatters/StringPrinter.h b/lldb/include/lldb/DataFormatters/StringPrinter.h
index 4a6e2e9..cb6cba1 100644 (file)
--- a/lldb/include/lldb/DataFormatters/StringPrinter.h
+++ b/lldb/include/lldb/DataFormatters/StringPrinter.h
@@ -133,9 +133,9 @@ public:
     ReadBufferAndDumpToStreamOptions(
         const ReadStringAndDumpToStreamOptions &options);
 
-    void SetData(DataExtractor d) { m_data = d; }
+    void SetData(DataExtractor &&d) { m_data = std::move(d); }
 
-    lldb_private::DataExtractor GetData() const { return m_data; }
+    const lldb_private::DataExtractor &GetData() const { return m_data; }
 
     void SetIsTruncated(bool t) { m_is_truncated = t; }
 

diff --git a/lldb/include/lldb/Utility/DataExtractor.h b/lldb/include/lldb/Utility/DataExtractor.h
index 0923e52..dbf0bce 100644 (file)
--- a/lldb/include/lldb/Utility/DataExtractor.h
+++ b/lldb/include/lldb/Utility/DataExtractor.h
@@ -134,7 +134,12 @@ public:
   DataExtractor(const DataExtractor &data, lldb::offset_t offset,
                 lldb::offset_t length, uint32_t target_byte_size = 1);
 
-  DataExtractor(const DataExtractor &rhs);
+  /// Copy constructor.
+  ///
+  /// The copy constructor is explicit as otherwise it is easy to make
+  /// unintended modification of a local copy instead of a caller's instance.
+  /// Also a needless copy of the \a m_data_sp shared pointer is/ expensive.
+  explicit DataExtractor(const DataExtractor &rhs);
 
   /// Assignment operator.
   ///
@@ -149,6 +154,12 @@ public:
   ///     A const reference to this object.
   const DataExtractor &operator=(const DataExtractor &rhs);
 
+  /// Move constructor and move assignment operators to complete the rule of 5.
+  ///
+  /// They would get deleted as we already defined those of rule of 3.
+  DataExtractor(DataExtractor &&rhs) = default;
+  DataExtractor &operator=(DataExtractor &&rhs) = default;
+
   /// Destructor
   ///
   /// If this object contains a valid shared data reference, the reference
@@ -1005,7 +1016,8 @@ protected:
   uint32_t m_addr_size; ///< The address size to use when extracting addresses.
   /// The shared pointer to data that can be shared among multiple instances
   lldb::DataBufferSP m_data_sp;
-  const uint32_t m_target_byte_size = 1;
+  /// Making it const would require implementation of move assignment operator.
+  uint32_t m_target_byte_size = 1;
 };
 
 } // namespace lldb_private

diff --git a/lldb/source/DataFormatters/StringPrinter.cpp b/lldb/source/DataFormatters/StringPrinter.cpp
index 0c6438f..6def893 100644 (file)
--- a/lldb/source/DataFormatters/StringPrinter.cpp
+++ b/lldb/source/DataFormatters/StringPrinter.cpp
@@ -475,11 +475,9 @@ static bool ReadEncodedBufferAndDumpToStream(
     return true;
   }
 
-  DataExtractor data(buffer_sp, process_sp->GetByteOrder(),
-                     process_sp->GetAddressByteSize());
-
   StringPrinter::ReadBufferAndDumpToStreamOptions dump_options(options);
-  dump_options.SetData(data);
+  dump_options.SetData(DataExtractor(buffer_sp, process_sp->GetByteOrder(),
+                                     process_sp->GetAddressByteSize()));
   dump_options.SetSourceSize(sourceSize);
   dump_options.SetIsTruncated(is_truncated);
   dump_options.SetNeedsZeroTermination(needs_zero_terminator);

diff --git a/lldb/source/Plugins/Language/CPlusPlus/CxxStringTypes.cpp b/lldb/source/Plugins/Language/CPlusPlus/CxxStringTypes.cpp
index 41bbd2b..45c0cd2 100644 (file)
--- a/lldb/source/Plugins/Language/CPlusPlus/CxxStringTypes.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/CxxStringTypes.cpp
@@ -168,7 +168,7 @@ bool lldb_private::formatters::Char8SummaryProvider(
     stream.Printf("%s ", value.c_str());
 
   StringPrinter::ReadBufferAndDumpToStreamOptions options(valobj);
-  options.SetData(data);
+  options.SetData(std::move(data));
   options.SetStream(&stream);
   options.SetPrefixToken("u8");
   options.SetQuote('\'');
@@ -194,7 +194,7 @@ bool lldb_private::formatters::Char16SummaryProvider(
     stream.Printf("%s ", value.c_str());
 
   StringPrinter::ReadBufferAndDumpToStreamOptions options(valobj);
-  options.SetData(data);
+  options.SetData(std::move(data));
   options.SetStream(&stream);
   options.SetPrefixToken("u");
   options.SetQuote('\'');
@@ -220,7 +220,7 @@ bool lldb_private::formatters::Char32SummaryProvider(
     stream.Printf("%s ", value.c_str());
 
   StringPrinter::ReadBufferAndDumpToStreamOptions options(valobj);
-  options.SetData(data);
+  options.SetData(std::move(data));
   options.SetStream(&stream);
   options.SetPrefixToken("U");
   options.SetQuote('\'');
@@ -254,7 +254,7 @@ bool lldb_private::formatters::WCharSummaryProvider(
   const uint32_t wchar_size = *size;
 
   StringPrinter::ReadBufferAndDumpToStreamOptions options(valobj);
-  options.SetData(data);
+  options.SetData(std::move(data));
   options.SetStream(&stream);
   options.SetPrefixToken("L");
   options.SetQuote('\'');

diff --git a/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp b/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
index 8eda422..b9aef0a 100644 (file)
--- a/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
@@ -686,7 +686,7 @@ bool lldb_private::formatters::LibcxxWStringSummaryProvider(
   if (!wchar_t_size)
     return false;
 
-  options.SetData(extractor);
+  options.SetData(std::move(extractor));
   options.SetStream(&stream);
   options.SetPrefixToken("L");
   options.SetQuote('"');
@@ -743,12 +743,14 @@ bool LibcxxStringSummaryProvider(ValueObject &valobj, Stream &stream,
     }
   }
 
-  DataExtractor extractor;
-  const size_t bytes_read = location_sp->GetPointeeData(extractor, 0, size);
-  if (bytes_read < size)
-    return false;
+  {
+    DataExtractor extractor;
+    const size_t bytes_read = location_sp->GetPointeeData(extractor, 0, size);
+    if (bytes_read < size)
+      return false;
 
-  options.SetData(extractor);
+    options.SetData(std::move(extractor));
+  }
   options.SetStream(&stream);
   if (prefix_token.empty())
     options.SetPrefixToken(nullptr);

diff --git a/lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp b/lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
index 12bc739..79da46d 100644 (file)
--- a/lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
+++ b/lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
@@ -519,9 +519,8 @@ ProcessElfCore::parseSegment(const DataExtractor &segment) {
 
     size_t note_start = offset;
     size_t note_size = llvm::alignTo(note.n_descsz, 4);
-    DataExtractor note_data(segment, note_start, note_size);
 
-    result.push_back({note, note_data});
+    result.push_back({note, DataExtractor(segment, note_start, note_size)});
     offset += note_size;
   }
 
@@ -897,7 +896,8 @@ llvm::Error ProcessElfCore::parseLinuxNotes(llvm::ArrayRef<CoreNote> notes) {
 /// A note segment consists of one or more NOTE entries, but their types and
 /// meaning differ depending on the OS.
 llvm::Error ProcessElfCore::ParseThreadContextsFromNoteSegment(
-    const elf::ELFProgramHeader &segment_header, DataExtractor segment_data) {
+    const elf::ELFProgramHeader &segment_header,
+    const DataExtractor &segment_data) {
   assert(segment_header.p_type == llvm::ELF::PT_NOTE);
 
   auto notes_or_error = parseSegment(segment_data);

diff --git a/lldb/source/Plugins/Process/elf-core/ProcessElfCore.h b/lldb/source/Plugins/Process/elf-core/ProcessElfCore.h
index d8e3cc9..39a7394 100644 (file)
--- a/lldb/source/Plugins/Process/elf-core/ProcessElfCore.h
+++ b/lldb/source/Plugins/Process/elf-core/ProcessElfCore.h
@@ -148,7 +148,7 @@ private:
   // Parse thread(s) data structures(prstatus, prpsinfo) from given NOTE segment
   llvm::Error ParseThreadContextsFromNoteSegment(
       const elf::ELFProgramHeader &segment_header,
-      lldb_private::DataExtractor segment_data);
+      const lldb_private::DataExtractor &segment_data);
 
   // Returns number of thread contexts stored in the core file
   uint32_t GetNumThreadContexts();

diff --git a/lldb/source/Plugins/Process/elf-core/RegisterUtilities.cpp b/lldb/source/Plugins/Process/elf-core/RegisterUtilities.cpp
index 0c21c0f..6f3bf02 100644 (file)
--- a/lldb/source/Plugins/Process/elf-core/RegisterUtilities.cpp
+++ b/lldb/source/Plugins/Process/elf-core/RegisterUtilities.cpp
@@ -34,5 +34,5 @@ DataExtractor lldb_private::getRegset(llvm::ArrayRef<CoreNote> Notes,
   uint32_t Type = *TypeOr;
   auto Iter = llvm::find_if(
       Notes, [Type](const CoreNote &Note) { return Note.info.n_type == Type; });
-  return Iter == Notes.end() ? DataExtractor() : Iter->data;
+  return Iter == Notes.end() ? DataExtractor() : DataExtractor(Iter->data);
 }

diff --git a/lldb/unittests/DataFormatter/StringPrinterTests.cpp b/lldb/unittests/DataFormatter/StringPrinterTests.cpp
index 84a9372..a7fa6e8 100644 (file)
--- a/lldb/unittests/DataFormatter/StringPrinterTests.cpp
+++ b/lldb/unittests/DataFormatter/StringPrinterTests.cpp
@@ -37,9 +37,8 @@ static Optional<std::string> format(StringRef input,
   opts.SetEscapeNonPrintables(true);
   opts.SetIgnoreMaxLength(false);
   opts.SetEscapeStyle(escape_style);
-  DataExtractor extractor(input.data(), input.size(),
-                          endian::InlHostByteOrder(), sizeof(void *));
-  opts.SetData(extractor);
+  opts.SetData(DataExtractor(input.data(), input.size(),
+                             endian::InlHostByteOrder(), sizeof(void *)));
   const bool success = StringPrinter::ReadBufferAndDumpToStream<elem_ty>(opts);
   if (!success)
     return llvm::None;