labath added inline comments.
================
Comment at: lldb/include/lldb/Target/MemoryRegionInfo.h:43-45
+ if (m_dirty_pages.hasValue()) {
+ m_dirty_pages.getValue().clear();
+ }
----------------
This bit is unnecessary. In fact, I'd implement the entire function as `*this =
MemoryRegionInfo();`
================
Comment at: lldb/include/lldb/Target/MemoryRegionInfo.h:118
+ /// detail this.
+ llvm::Optional<std::vector<lldb::addr_t>> GetDirtyPageList() {
+ return m_dirty_pages;
----------------
`const Optional<...> &` to avoid a copy.
================
Comment at: lldb/include/lldb/Target/MemoryRegionInfo.h:125-127
+ if (m_dirty_pages.hasValue())
+ m_dirty_pages.getValue().clear();
+ m_dirty_pages = pagelist;
----------------
`m_dirty_pages = std::move(pagelist);`
================
Comment at: lldb/include/lldb/lldb-private-interfaces.h:58-59
+ const FileSpec &outfile,
+ lldb::SaveCoreStyle requested_core_style,
+ lldb::SaveCoreStyle &created_core_style,
+ Status &error);
----------------
Maybe just have one argument which will be set on exit to reflect the actual
style that was used?
================
Comment at: lldb/source/API/SBMemoryRegionInfo.cpp:142
+ addr_t dirty_page_addr = LLDB_INVALID_ADDRESS;
+ llvm::Optional<std::vector<addr_t>> dirty_page_list =
+ m_opaque_up->GetDirtyPageList();
----------------
`const T &`
================
Comment at: lldb/source/Commands/CommandObjectMemory.cpp:1742
name, section_name ? " " : "", section_name);
+ llvm::Optional<std::vector<addr_t>> dirty_page_list =
+ range_info.GetDirtyPageList();
----------------
const T &
================
Comment at: lldb/source/Commands/CommandObjectMemory.cpp:1750-1760
+ bool print_comma = false;
+ result.AppendMessageWithFormat("Dirty pages: ");
+ for (size_t i = 0; i < page_count; i++) {
+ if (print_comma)
+ result.AppendMessageWithFormat(", ");
+ else
+ print_comma = true;
----------------
I think something like this ought to do:
`AppendMessageWithFormatv("Dirty pages: {0:@[x]}.\n",
llvm::make_range(dirty_page_list->begin(), dirty_page_list->end()));`
================
Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:225
lldb_private::Status &error) {
+ created_core_style = SaveCoreStyle::eSaveCoreMiniDump;
return SaveMiniDump(process_sp, outfile, error);
----------------
I don't think this is a good use of that constant. Minidumps can also come in
"full" styles, which includes all memory, or some smaller ones, where only the
heap (or it's modified portion, or just stack, etc.) is included.
In essence, "minidump" is just a format, just like "elf" and "macho". It's
peculiarity is that it is different from the native object file format, but
that's not relevant here.
I think that the best way to handle this is to make the choice of format should
be orthogonal to the choice of what goes into the dump. Currently we don't
allow the user to pick a format, and that's fine -- we'll just pick whatever is
the native format for the given platform. But I don't think we should start
mixing the two.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D88387/new/
https://reviews.llvm.org/D88387
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
