From 14f443030c1acc4346589aee3c1c532dc00eae3a Mon Sep 17 00:00:00 2001 From: Jan Kratochvil Date: Wed, 4 Aug 2021 20:34:21 +0200 Subject: [PATCH] [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 --- lldb/include/lldb/DataFormatters/StringPrinter.h | 4 ++-- lldb/include/lldb/Utility/DataExtractor.h | 16 ++++++++++++++-- lldb/source/DataFormatters/StringPrinter.cpp | 6 ++---- .../source/Plugins/Language/CPlusPlus/CxxStringTypes.cpp | 8 ++++---- lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp | 14 ++++++++------ lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp | 6 +++--- lldb/source/Plugins/Process/elf-core/ProcessElfCore.h | 2 +- .../Plugins/Process/elf-core/RegisterUtilities.cpp | 2 +- lldb/unittests/DataFormatter/StringPrinterTests.cpp | 5 ++--- 9 files changed, 37 insertions(+), 26 deletions(-) diff --git a/lldb/include/lldb/DataFormatters/StringPrinter.h b/lldb/include/lldb/DataFormatters/StringPrinter.h index 4a6e2e9..cb6cba1 100644 --- 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 --- 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 --- 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 --- 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 --- 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 --- 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 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 --- 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 --- 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 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 --- a/lldb/unittests/DataFormatter/StringPrinterTests.cpp +++ b/lldb/unittests/DataFormatter/StringPrinterTests.cpp @@ -37,9 +37,8 @@ static Optional 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(opts); if (!success) return llvm::None; -- 2.7.4