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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

The part I am not sure about here is that you are creating a Module which has no associated object file, but it still has some sections. That's not how any of our current modules/object files work, and I worry that this may cause problems down the line (and plainly put, having sections without an object file feels weird). I am wondering whether it wouldn't be better to go all the way and create a "PlaceholderObjectFile" as well (or instead of PlaceholderModule).

I don't know what the right answer here is, but it is something to think about...



================
Comment at: packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py:53
+        for module in self.target.modules:
+            self.assertTrue(module.IsValid())
+
----------------
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.


================
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");
----------------
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.


================
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:47
+//------------------------------------------------------------------
+class PlaceholderModule : public Module {
+public:
----------------
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.


https://reviews.llvm.org/D45700