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 han
>
> 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
This revision was automatically updated to reflect the committed changes.
Closed by commit rL330302: Improve LLDB's handling of non-local minidumps
(authored by lemo, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D45700?vs=142960&id=1
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
ll
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 to
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
source/Commands/CommandObjectTarge
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
redundancy
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())
+
laba
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())
+
laba
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:
packages
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,
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
packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDu
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 section_name("
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 section_
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 mem
21 matches
Mail list logo
