lemo added subscribers: bgianfo, penryu.
lemo added a comment.
> It looks like nobody except me is worried about the
> module-without-an-object-file situation, so I guess we can try this out and
> see how it goes.
Sorry Pavel, I meant to respond to this: most of the code seems to
explicitly
>
> It looks like nobody except me is worried about the
> module-without-an-object-file situation, so I guess we can try this out and
> see how it goes.
>
Sorry Pavel, I meant to respond to this: most of the code seems to
explicitly handle this case (module-without-object-file), I just had to fix
labath added a comment.
It looks like nobody except me is worried about the
module-without-an-object-file situation, so I guess we can try this out and see
how it goes.
The test you've added here has been failing on windows though. I've tried to
fix this in r330314, but it meant modifying the
clayborg added a comment.
LGTM
Repository:
rL LLVM
https://reviews.llvm.org/D45700
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
amccarth accepted this revision.
amccarth added a comment.
I actually preferred the factory solution.
What I intended to express is that the modification of the target doesn't seems
like it should be inside the PlaceholderModule class, so whether you use a
factory or not doesn't really address
Greg/Pavel, does the latest revision look good to you? Thanks!
On Wed, Apr 18, 2018 at 10:31 AM, Leonard Mosescu via Phabricator <
revi...@reviews.llvm.org> wrote:
> lemo marked an inline comment as done.
> lemo added a comment.
>
> In https://reviews.llvm.org/D45700#1070491, @amccarth wrote:
>
lemo added subscribers: amccarth, clayborg, labath, zturner.
lemo marked an inline comment as done.
lemo added a comment.
Greg/Pavel, does the latest revision look good to you? Thanks!
https://reviews.llvm.org/D45700
___
lldb-commits mailing list
lemo marked an inline comment as done.
lemo added a comment.
In https://reviews.llvm.org/D45700#1070491, @amccarth wrote:
> LGTM, but consider highlighting the side effect to `target` when the factory
> makes a Placeholder module.
Good observation: taking a step back, the factory introduces
lemo updated this revision to Diff 142960.
https://reviews.llvm.org/D45700
Files:
packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py
amccarth accepted this revision.
amccarth added a comment.
This revision is now accepted and ready to land.
LGTM, but consider highlighting the side effect to `target` when the factory
makes a Placeholder module.
In the future, we need to refactor the minidump tests to get rid of the
lemo marked 2 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 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())
+
lemo wrote:
> labath wrote:
> > Could we
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())
+
lemo updated this revision to Diff 142799.
lemo edited the summary of this revision.
lemo added a comment.
Thanks for the feedback.
PS. The sample output in the description if from LLDB running on Linux, reading
minidumps captured on Windows.
https://reviews.llvm.org/D45700
Files:
clayborg added a comment.
Looks good. I questions if we want PlaceholderModule to be available for all
symbolicators/core dump plugins. See inlined comments.
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:47
lemo updated this revision to Diff 142706.
lemo edited the summary of this revision.
https://reviews.llvm.org/D45700
Files:
packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
lemo added inline comments.
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
amccarth added a comment.
Nice!
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
lemo created this revision.
lemo added reviewers: clayborg, labath, amccarth.
lemo added a project: LLDB.
Herald added subscribers: JDevlieghere, aprantl.
Normally, LLDB is creating a high-fidelity representation of a live
process, including a list of modules and sections, with the
associated
19 matches
Mail list logo
