[nfc] [lldb] Prevent needless copies of DataExtractor

[jankratochvil]

Summary

This patch was created accidentally but I find it useful when I already wrote it.
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).
OK to check it in?

lldb/include/lldb/DataFormatters/StringPrinter.h

 //===-- StringPrinter.h -----------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_DATAFORMATTERS_STRINGPRINTER_H
 #define LLDB_DATAFORMATTERS_STRINGPRINTER_H
 
 #include <functional>
 #include <string>
 
 #include "lldb/lldb-forward.h"
 
 #include "lldb/Utility/DataExtractor.h"
 
 namespace lldb_private {
 namespace formatters {
 class StringPrinter {
 public:
   enum class StringElementType { ASCII, UTF8, UTF16, UTF32 };
public:
   public:
     ReadBufferAndDumpToStreamOptions() = default;
 
     ReadBufferAndDumpToStreamOptions(ValueObject &valobj);
 
     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; }
 
     bool GetIsTruncated() const { return m_is_truncated; }
   private:
     DataExtractor m_data;
     bool m_is_truncated = false;
   };

lldb/include/lldb/Utility/DataExtractor.h

 //===-- DataExtractor.h -----------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_UTILITY_DATAEXTRACTOR_H
 #define LLDB_UTILITY_DATAEXTRACTOR_H
 
 #include "lldb/Utility/Endian.h"
 #include "lldb/lldb-defines.h"
 #include "lldb/lldb-enumerations.h"
 #include "lldb/lldb-forward.h"
 #include "lldb/lldb-types.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/Support/DataExtractor.h"
 #include "llvm/Support/SwapByteOrder.h"
 
public:
   /// \param[in] length
   ///     The length in bytes of the subset of data.
   ///
   /// \param[in] target_byte_size
   ///     A size of a target byte in 8-bit host bytes
   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.
   ///
   /// Copies all data, byte order and address size settings from \a rhs into
   /// this object. If \a rhs contains shared data, a reference to that shared
   /// data will be added.
   ///
   /// \param[in] rhs
   ///     Another DataExtractor object to copy.
   ///
   /// \return
   ///     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
   /// count on the data will be decremented, and if zero, the data will be
   /// freed.
   virtual ~DataExtractor();
 
   uint32_t getTargetByteSize() const { return m_target_byte_size; }
protected:
   const uint8_t *m_start = nullptr; ///< A pointer to the first byte of data.
   const uint8_t *m_end =
       nullptr; ///< A pointer to the byte that is past the end of the data.
   lldb::ByteOrder
       m_byte_order;     ///< The byte order of the data we are extracting from.
   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
 
 #endif // LLDB_UTILITY_DATAEXTRACTOR_H

lldb/source/DataFormatters/StringPrinter.cpp

 //===-- StringPrinter.cpp -------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/DataFormatters/StringPrinter.h"
 
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/ValueObject.h"
 #include "lldb/Target/Language.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/Status.h"
 
static bool ReadEncodedBufferAndDumpToStream(
   else
     process_sp->ReadMemoryFromInferior(options.GetLocation(), buffer,
                                        bufferSPSize, error);
   if (error.Fail()) {
     options.GetStream()->Printf("unable to read data");
     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);
   if (needs_zero_terminator)
     dump_options.SetBinaryZeroIsTerminator(true);
 
   GetPrintableElementType print_style = (elem_type == StringElementType::ASCII)
                                             ? GetPrintableElementType::ASCII

lldb/source/Plugins/Language/CPlusPlus/CxxStringTypes.cpp

if (error.Fail())
     return false;
 
   std::string value;
   valobj.GetValueAsCString(lldb::eFormatUnicode8, value);
   if (!value.empty())
     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('\'');
   options.SetSourceSize(1);
   options.SetBinaryZeroIsTerminator(false);
 
   return StringPrinter::ReadBufferAndDumpToStream<
       StringPrinter::StringElementType::UTF8>(options);
if (error.Fail())
     return false;
 
   std::string value;
   valobj.GetValueAsCString(lldb::eFormatUnicode16, value);
   if (!value.empty())
     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('\'');
   options.SetSourceSize(1);
   options.SetBinaryZeroIsTerminator(false);
 
   return StringPrinter::ReadBufferAndDumpToStream<
       StringPrinter::StringElementType::UTF16>(options);
if (error.Fail())
     return false;
 
   std::string value;
   valobj.GetValueAsCString(lldb::eFormatUnicode32, value);
   if (!value.empty())
     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('\'');
   options.SetSourceSize(1);
   options.SetBinaryZeroIsTerminator(false);
 
   return StringPrinter::ReadBufferAndDumpToStream<
       StringPrinter::StringElementType::UTF32>(options);
if (!wchar_compiler_type)
 
     // Safe to pass nullptr for exe_scope here.
   llvm::Optional<uint64_t> size = wchar_compiler_type.GetBitSize(nullptr);
   if (!size)
     return false;
   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('\'');
   options.SetSourceSize(1);
   options.SetBinaryZeroIsTerminator(false);
 
   switch (wchar_size) {
   case 8:

lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp

bool lldb_private::formatters::LibcxxWStringSummaryProvider(
   if (!ast_context)
     return false;
 
   auto wchar_t_size =
       ast_context->GetBasicType(lldb::eBasicTypeWChar).GetByteSize(nullptr);
   if (!wchar_t_size)
     return false;
 
-  options.SetData(extractor);
+  options.SetData(std::move(extractor));
   options.SetStream(&stream);
   options.SetPrefixToken("L");
   options.SetQuote('"');
   options.SetSourceSize(size);
   options.SetBinaryZeroIsTerminator(false);
 
   switch (*wchar_t_size) {
   case 1:
bool LibcxxStringSummaryProvider(ValueObject &valobj, Stream &stream,
   if (summary_options.GetCapping() == TypeSummaryCapping::eTypeSummaryCapped) {
     const auto max_size = valobj.GetTargetSP()->GetMaximumSizeOfStringSummary();
     if (size > max_size) {
       size = max_size;
       options.SetIsTruncated(true);
     }
   }
 
+  {
     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);
   else
     options.SetPrefixToken(prefix_token);
   options.SetQuote('"');
   options.SetSourceSize(size);
   options.SetBinaryZeroIsTerminator(false);

lldb/source/Plugins/Process/elf-core/ProcessElfCore.h

 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_SOURCE_PLUGINS_PROCESS_ELF_CORE_PROCESSELFCORE_H
 #define LLDB_SOURCE_PLUGINS_PROCESS_ELF_CORE_PROCESSELFCORE_H
 
 #include <list>
 #include <vector>
 
 #include "lldb/Target/PostMortemProcess.h"
 #include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/Status.h"
 
 #include "Plugins/ObjectFile/ELF/ELFHeader.h"
 #include "Plugins/Process/elf-core/RegisterUtilities.h"
 
 struct ThreadData;
 
private:
   VMRangeToPermissions m_core_range_infos;
 
   // NT_FILE entries found from the NOTE segment
   std::vector<NT_FILE_Entry> m_nt_file_entries;
 
   // 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();
 
   // Parse a contiguous address range of the process from LOAD segment
   lldb::addr_t
   AddAddressRangeFromLoadSegment(const elf::ELFProgramHeader &header);
 

lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp

 //===-- ProcessElfCore.cpp ------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include <cstdlib>
 
 #include <memory>
 #include <mutex>
 
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/Section.h"
 #include "lldb/Target/DynamicLoader.h"
 #include "lldb/Target/MemoryRegionInfo.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Target/UnixSignals.h"
 #include "lldb/Utility/DataBufferHeap.h"
ProcessElfCore::parseSegment(const DataExtractor &segment) {
   while (offset < segment.GetByteSize()) {
     ELFNote note = ELFNote();
     if (!note.Parse(segment, &offset))
       return llvm::make_error<llvm::StringError>(
           "Unable to parse note segment", llvm::inconvertibleErrorCode());
 
     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;
   }
 
   return std::move(result);
 }
 
 llvm::Error ProcessElfCore::parseFreeBSDNotes(llvm::ArrayRef<CoreNote> notes) {
   ArchSpec arch = GetArchitecture();
if (have_prstatus)
     m_thread_data.push_back(thread_data);
   return llvm::Error::success();
 }
 
 /// Parse Thread context from PT_NOTE segment and store it in the thread list
 /// 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);
   if(!notes_or_error)
     return notes_or_error.takeError();
   switch (GetArchitecture().GetTriple().getOS()) {
   case llvm::Triple::FreeBSD:
     return parseFreeBSDNotes(*notes_or_error);

lldb/source/Plugins/Process/elf-core/RegisterUtilities.cpp

 //===-- RegisterUtilities.cpp ---------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "Plugins/Process/elf-core/RegisterUtilities.h"
 #include "llvm/ADT/STLExtras.h"
 
 using namespace lldb_private;
 
 static llvm::Optional<uint32_t>
 getNoteType(const llvm::Triple &Triple,
             llvm::ArrayRef<RegsetDesc> RegsetDescs) {
   for (const auto &Entry : RegsetDescs) {
DataExtractor lldb_private::getRegset(llvm::ArrayRef<CoreNote> Notes,
                                       const llvm::Triple &Triple,
                                       llvm::ArrayRef<RegsetDesc> RegsetDescs) {
   auto TypeOr = getNoteType(Triple, RegsetDescs);
   if (!TypeOr)
     return DataExtractor();
   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);
 }

lldb/unittests/DataFormatter/StringPrinterTests.cpp

 //===-- StringPrinterTests.cpp --------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/DataFormatters/StringPrinter.h"
 #include "lldb/Utility/DataExtractor.h"
 #include "lldb/Utility/Endian.h"
 #include "lldb/Utility/StreamString.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/raw_ostream.h"
 #include "gtest/gtest.h"
 #include <string>
static Optional<std::string> format(StringRef input,
   StreamString out;
   StringPrinter::ReadBufferAndDumpToStreamOptions opts;
   opts.SetStream(&out);
   opts.SetSourceSize(input.size());
   opts.SetNeedsZeroTermination(true);
   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;
   return out.GetString().str();
 }
 
 // Test ASCII formatting for C++. This behaves exactly like UTF8 formatting for
 // C++, although that's questionable (see FIXME in StringPrinter.cpp).