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