clayborg added a comment.
In https://reviews.llvm.org/D49202#1165455, @lemo wrote:
> Great timing! ARM support would be most welcome. Are you planning to
> support the Breakpad flavor or ARM minidumps or the Microsoft one? (Mark
> Mentovai just reminded me that the ARM support was added
lemo added a comment.
Great timing! ARM support would be most welcome. Are you planning to
support the Breakpad flavor or ARM minidumps or the Microsoft one? (Mark
Mentovai just reminded me that the ARM support was added independently and
some of the structures are different)
Regarding the
Great timing! ARM support would be most welcome. Are you planning to
support the Breakpad flavor or ARM minidumps or the Microsoft one? (Mark
Mentovai just reminded me that the ARM support was added independently and
some of the structures are different)
Regarding the invalid minidumps, I used my
clayborg added a comment.
I have an upcoming patch for adding ARM and ARM64 support to the minidump
parser and i was curious how you created the invalid minidump files that are
part of this patch?
Repository:
rL LLVM
https://reviews.llvm.org/D49202
That sounds reasonable to me. I'll make a note to revisit this (I don't the
the cycles to do it right away but I'm planning a few more changes in the
area soon).
On Mon, Jul 16, 2018 at 10:36 AM, Pavel Labath wrote:
> Ok, I see what you mean now.
>
> Looking at the other core file plugins (elf,
Ok, I see what you mean now.
Looking at the other core file plugins (elf, mach-o), it looks like
they do only very basic verification before the accept the file. The
pretty much just check the magic numbers, which would be more-or-less
equivalent to our `MinidumpHeader::Parse` call. As this does
lemo added subscribers: amccarth, bgianfo, labath, penryu.
lemo added a comment.
The problem is not returning an error from Minidump::Create() - if that was
the case this could easily be improved indeed. The two-phase initialization
is a consequence of the LLDB plugin lookup:
1.
The problem is not returning an error from Minidump::Create() - if that was
the case this could easily be improved indeed. The two-phase initialization
is a consequence of the LLDB plugin lookup:
1. Target::CreateProcess() calls Process::FindPlugin()
2. ProcessMinidump::CreateInstance() then has
labath added a comment.
I don't agree with the two-stage initialization of the MinidumpParser class
being introduced here. We deliberately introduced the `Create` static function
to avoid this. If this `Initialize` function in checking invariants which are
assumed to be hold by other parser
This revision was automatically updated to reflect the committed changes.
Closed by commit rL336918: Restructure the minidump loading path and add early
explicit consistency… (authored by lemo, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
lemo added a comment.
Thanks Adrian for the thorough review.
Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:52
ASSERT_GT(parser->GetData().size(), 0UL);
+auto result = parser->Initialize();
+if (expect_failure)
amccarth wrote:
>
amccarth accepted this revision.
amccarth added a comment.
This revision is now accepted and ready to land.
LGTM if you don't want to consider my remaining suggestion for this patch.
Thanks for the extra tests.
Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:52
lemo updated this revision to Diff 155080.
Herald added a subscriber: mgorny.
https://reviews.llvm.org/D49202
Files:
source/Plugins/Process/minidump/MinidumpParser.cpp
source/Plugins/Process/minidump/MinidumpParser.h
source/Plugins/Process/minidump/MinidumpTypes.cpp
lemo updated this revision to Diff 155076.
lemo added a comment.
Adding a few ill-formed minidumps for testing the new checks
https://reviews.llvm.org/D49202
Files:
source/Plugins/Process/minidump/MinidumpParser.cpp
source/Plugins/Process/minidump/MinidumpParser.h
lemo added a comment.
Regarding test for the other checks, I'll try to fabricate a few invalid
minidumps (although it would obviously provide limited coverage)
Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:52
ASSERT_GT(parser->GetData().size(), 0UL);
+
amccarth added inline comments.
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:172
+
+ // Do we support the minidump's architecture?
+ ArchSpec arch = GetArchitecture();
lemo wrote:
> amccarth wrote:
> > Should the architecture check be in the
amccarth added a comment.
Also, I'm not seeing tests for the other consistency checks you're adding (like
whether there are any streams or whether the streams overlap). Is it difficult
to create such malformed minidumps?
Comment at:
lemo updated this revision to Diff 155069.
lemo marked 9 inline comments as done.
lemo added a comment.
Incorporating CR feedback
https://reviews.llvm.org/D49202
Files:
source/Plugins/Process/minidump/MinidumpParser.cpp
source/Plugins/Process/minidump/MinidumpParser.h
lemo added inline comments.
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:23
#include
+#include
+#include
amccarth wrote:
> Why add ``? It looks like your new map is just a vector.
Good catch
Comment at:
amccarth added inline comments.
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:23
#include
+#include
+#include
Why add ``? It looks like your new map is just a vector.
Comment at:
lemo created this revision.
lemo added reviewers: amccarth, labath.
lemo added a project: LLDB.
Herald added a subscriber: mgrang.
Corrupted minidumps was leading to unpredictable behavior.
This change adds explicit consistency checks for the minidump early on. The
checks are not comprehensive
21 matches
Mail list logo
