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
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
