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

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: include/lldb/Core/Module.h:176-177
+
+    // Also update the module FileSpec.
+    module_sp->m_file = module_sp->m_objfile_sp->GetFileSpec();
+    return std::move(module_sp);
----------------
clayborg wrote:
> Why do we need to update the m_file? m_platform_file is the path of the module as it is known the the remote system, so if m_file differs from the object file's file spec, we might want to shuffle, m_file into m_platform_file first and then overwrite it?
I used the word "update", because the previous comment did, but this isn't really "updating" anything. It is just "setting" the FileSpec because it was previously empty (the module we're setting this on is created a couple of lines back (line 163) as an empty module. The entire reason for existence of this function is to perform the articulate dance of creating the objects in the right order (because ObjectFile stores a shared_ptr to the module, it needs to be created after the ModuleSP is fully constructed).

So, there isn't to shuffle here. We just set m_file, to whatever the ObjectFile tells us is the file it was created from. 


================
Comment at: include/lldb/Core/Module.h:178
+    module_sp->m_file = module_sp->m_objfile_sp->GetFileSpec();
+    return std::move(module_sp);
   }
----------------
lemo wrote:
> 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. 
Yep, I should have known that. I'll remove it.


================
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,
----------------
lemo wrote:
> 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.
Yeah, I agree this interface is weird, but this is kind of necessary to make the `CreateModuleFromObjectFile` flow work. The idea there is to create an empty module, then place an ObjectFile inside of it, and then use the data from the ObjectFile to initialize the module.

So, at the point when this function is called, the module_sp object will be empty (so there isn't anything to assert). It's only present here to satisfy the ObjectFile contract and set up the weak_ptr backlink. The module_spec contains the actual data, which is used to initialize the PlaceholderObjectFile (and which will then in turn initialize the Module object).


================
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:52
-//------------------------------------------------------------------
-class PlaceholderModule : public Module {
 public:
----------------
lemo wrote:
> Nice - so we can do without this in all scenarios? What about PDBs?
I haven't tried anything special besides running existing tests, but I don't think this should hamper anything. The modules I create here will be fairly realistic, they will have UUIDs, ObjectFiles and everything. So if the PDBs were able to attach to the PlaceholderModules, they should certainly be able to do the same with regular modules + PlaceholderObjectFiles.


================
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; }
----------------
lemo wrote:
> should we dump something meaningful? I don't remember the exact command but I think this surfaces in user output
I guess you meant "image dump objfile". It's kind of weird because that command prefixes the output with "outputting headers for <N> modules", and I have no idea what would be the "headers" for this object file. However I suppose a short one line summary of the file is better than just printing nothing.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57751/new/

https://reviews.llvm.org/D57751