lemo accepted this revision.
lemo added a comment.
Looks good - a few comments inline.
================
Comment at: include/lldb/Core/Module.h:178
+ module_sp->m_file = module_sp->m_objfile_sp->GetFileSpec();
+ return std::move(module_sp);
}
----------------
nit: return std::move(x) is almost never a good idea. It's not needed, verbose
and most importantly it actually inhibits NRVO so it's likely less efficient.
================
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:51
public:
- PlaceholderModule(const ModuleSpec &module_spec) :
- Module(module_spec.GetFileSpec(), module_spec.GetArchitecture()) {
- if (module_spec.GetUUID().IsValid())
- SetUUID(module_spec.GetUUID());
- }
-
- // Creates a synthetic module section covering the whole module image (and
- // sets the section load address as well)
- void CreateImageSection(const MinidumpModule *module, Target& target) {
- const ConstString section_name(".module_image");
- lldb::SectionSP section_sp(new Section(
- shared_from_this(), // Module to which this section belongs.
- nullptr, // ObjectFile
- 0, // Section ID.
- section_name, // Section name.
- eSectionTypeContainer, // Section type.
- module->base_of_image, // VM address.
- module->size_of_image, // VM size in bytes of this section.
- 0, // Offset of this section in the file.
- module->size_of_image, // Size of the section as found in the file.
- 12, // Alignment of the section (log2)
- 0, // Flags for this section.
- 1)); // Number of host bytes per target byte
- section_sp->SetPermissions(ePermissionsExecutable | ePermissionsReadable);
- GetSectionList()->AddSection(section_sp);
- target.GetSectionLoadList().SetSectionLoadAddress(
- section_sp, module->base_of_image);
+ PlaceholderObjectFile(const lldb::ModuleSP &module_sp,
+ const ModuleSpec &module_spec, lldb::offset_t base,
----------------
Can we avoid passing both the module_sp and module_spec? I try to avoid
interfaces which allow for inconsistent states, ex: what if module_sp and
module_spec describe completely different modules?
At very least I'd add a few asserts, but if we can change the interface to
avoid it, even better.
================
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:52
-//------------------------------------------------------------------
-class PlaceholderModule : public Module {
public:
----------------
Nice - so we can do without this in all scenarios? What about PDBs?
================
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:66
+ Strata CalculateStrata() override { return eStrataUnknown; }
+ void Dump(Stream *s) override {}
+ uint32_t GetDependentModules(FileSpecList &file_list) override { return 0; }
----------------
should we dump something meaningful? I don't remember the exact command but I
think this surfaces in user output
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57751/new/
https://reviews.llvm.org/D57751
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
