[Lldb-commits] [PATCH] D57751: minidump: Add ability to attach (breakpad) symbol files to placeholder modules

[Leonard Mosescu via Phabricator via lldb-commits]

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