[Lldb-commits] [PATCH] D45700: Improve LLDB's handling of non-local minidumps

[Leonard Mosescu via Phabricator via lldb-commits]

lemo marked 4 inline comments as done.
lemo added inline comments.


================
Comment at: packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py:53
+        for module in self.target.modules:
+            self.assertTrue(module.IsValid())
+
----------------
labath wrote:
> Could we strengthen these assertions a bit. Given that this is static data you are loading, I don't see why you couldn't hard-code the names of the modules you should expect.
Done.

Just curious, do we have any support for golden output files? (it would be great for tests like this)


================
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:53
+  // Creates a synthetic module section covering the whole module image
+  void CreateImageSection(const MinidumpModule *module, Target& target) {
+    const ConstString section_name(".module_image");
----------------
labath wrote:
> lemo wrote:
> > amccarth wrote:
> > > I wonder if this should just be part of the constructor.  I don't see a scenario where you'd create a PlaceholderModule and not want to create exactly one fake image section.  I know the style guide is against doing lots of work in a constructor, but that's mostly because it's hard to report failures, which you're not doing here anyway.
> > Thanks for the suggestion. I agree, this would look a bit cleaner to fold everything in the constructor (except the extra arguments to the constructor, but it will probably still be a net win)
> > 
> > The reason for a separate method is the little "shared_from_this()". It can't be done from the constructor since the object is not yet managed by a shared_ptr, so we need to do it post-construction.
> This can be solved by hiding the constructor and having a static factory function which returns a shared_ptr.
The factory is a great idea, done. (one downside with a private constructor is that we lose the benefits of std::make_shared though)


================
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:47
+//------------------------------------------------------------------
+class PlaceholderModule : public Module {
+public:
----------------
labath wrote:
> clayborg wrote:
> > I would be worth putting this class maybe in the same folder as lldb_private::Module and possibly renaming it. I can see this kind of thing being useful for symbolication in general and it won't be limited to use in minidumps. It should have enough accessors that allows an external client to modify everything.
> I concur. Besides postmortem, we can run into the situation where we cannot access the loaded modules for live debugging as well.
Thanks, I agree. I think this needs a bit more baking first though, I say let's put it through some use first (and maybe identify a 2nd use case).
(in particular we'd also need a more generic interface and I'd like to avoid creating an overly complex solution which may not even fit future use cases)


================
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:79
+private:
+  SectionList m_sections;
+};
----------------
clayborg wrote:
> Just use lldb_private::Module::m_sections_ap and put the sections in there? Any reason not to?
Thanks, done. (no good reason, I was just extra cautious not to introduce any unexpected side-effects)


https://reviews.llvm.org/D45700