labath marked an inline comment as done.
labath added inline comments.
================
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:348-349
+ bool is_complete;
+ std::tie(*m_memory_regions, is_complete) =
+ m_minidump_parser->BuildMemoryRegions();
+
----------------
clayborg wrote:
> Might be nice to just assign memory regions and then ask the minidump parser
> what its source of memory regions was? I am thinking of:
>
> ```
> m_memory_regions = m_minidump_parser->BuildMemoryRegions();
> switch (m_minidump_parser->GetMemoryRegionsSource()) {
> case MemoryInfoList:
> case LinuxProcMaps:
> break;
> case MemoryList:
> // Memory list is not always a exhaustive list of memory regions in a
> process...
> // Insert code below the "if (is_complete)" in switch?
> ```
>
> Actually thinking of this a bit more, we should really just do this work
> inside "m_minidump_parser->BuildMemoryRegions();". The minidump itself can
> then make the call and we don't need to return the "is_complete". Otherwise
> if anyone else uses the info from "m_minidump_parser->BuildMemoryRegions();"
> they would have to do the same thing as this code which would be bad.
>
I don't think that doing this work in the MinidumpParser is right, as the main
reason for it's existence was to isolate minidump parsing from the stuff
happening elsewhere in lldb (to enable unit testing and potentially be able to
reuse it elsewhere). Now that a lot of the parsing has moved to llvm the second
reason probably doesn't apply, but the unit testing separation is still nice to
have. Even if we ignore that (and say have unit tests pass in an empty
ModuleList), it still doesn't seem fully right because it's ProcessMinidump who
creates the Module objects (MinidumpParser only works with minidump "modules"),
so it seems natural to me that it should be the one who handles their
regionizing too.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69105/new/
https://reviews.llvm.org/D69105
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
