jankratochvil created this revision.
jankratochvil added reviewers: clayborg, labath, teemperor.
jankratochvil added a project: LLDB.
Herald added a subscriber: JDevlieghere.
jankratochvil requested review of this revision.
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
<https://reviews.llvm.org/D107470> but that is `llvm::DataExtractor`).
OK to check it in?
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D107485
Files:
lldb/include/lldb/DataFormatters/StringPrinter.h
lldb/include/lldb/Utility/DataExtractor.h
lldb/source/DataFormatters/StringPrinter.cpp
lldb/source/Plugins/Language/CPlusPlus/CxxStringTypes.cpp
lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
lldb/source/Plugins/Process/elf-core/ProcessElfCore.h
lldb/source/Plugins/Process/elf-core/RegisterUtilities.cpp
lldb/unittests/DataFormatter/StringPrinterTests.cpp
Index: lldb/unittests/DataFormatter/StringPrinterTests.cpp
===================================================================
--- lldb/unittests/DataFormatter/StringPrinterTests.cpp
+++ lldb/unittests/DataFormatter/StringPrinterTests.cpp
@@ -37,9 +37,8 @@
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;
Index: lldb/source/Plugins/Process/elf-core/RegisterUtilities.cpp
===================================================================
--- lldb/source/Plugins/Process/elf-core/RegisterUtilities.cpp
+++ lldb/source/Plugins/Process/elf-core/RegisterUtilities.cpp
@@ -34,5 +34,5 @@
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);
}
Index: lldb/source/Plugins/Process/elf-core/ProcessElfCore.h
===================================================================
--- lldb/source/Plugins/Process/elf-core/ProcessElfCore.h
+++ lldb/source/Plugins/Process/elf-core/ProcessElfCore.h
@@ -148,7 +148,7 @@
// 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();
Index: lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
===================================================================
--- lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
+++ lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
@@ -519,9 +519,8 @@
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 @@
/// 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);
Index: lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
===================================================================
--- lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
@@ -686,7 +686,7 @@
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 @@
}
}
- 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);
Index: lldb/source/Plugins/Language/CPlusPlus/CxxStringTypes.cpp
===================================================================
--- lldb/source/Plugins/Language/CPlusPlus/CxxStringTypes.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/CxxStringTypes.cpp
@@ -168,7 +168,7 @@
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 @@
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 @@
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 @@
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('\'');
Index: lldb/source/DataFormatters/StringPrinter.cpp
===================================================================
--- lldb/source/DataFormatters/StringPrinter.cpp
+++ lldb/source/DataFormatters/StringPrinter.cpp
@@ -475,11 +475,9 @@
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);
Index: lldb/include/lldb/Utility/DataExtractor.h
===================================================================
--- lldb/include/lldb/Utility/DataExtractor.h
+++ lldb/include/lldb/Utility/DataExtractor.h
@@ -134,7 +134,12 @@
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 @@
/// 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 @@
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
Index: lldb/include/lldb/DataFormatters/StringPrinter.h
===================================================================
--- lldb/include/lldb/DataFormatters/StringPrinter.h
+++ lldb/include/lldb/DataFormatters/StringPrinter.h
@@ -133,9 +133,9 @@
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; }
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
