[Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

[Leonard Mosescu via lldb-commits]

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 <labath at google.com> wrote:

> 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 not
> require creating a parser object, maybe we could delay the parser
> creation until `LoadCore` gets called (at which point you can easily
> report errors).
>
> This will leave us with a nice MinidumpParser interface.
> ProcessMinidump will still use two-stage initialization, but this is
> nothing new, and this change will make it easier for us to change the
> initialization method of the Process objects in the future.
>
> WDYT?
>
> On Mon, 16 Jul 2018 at 18:16, Leonard Mosescu <mosescu at google.com> wrote:
> >
> > 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 to inspect the core file
> to see if it's a minidump
> > 2b. ... if it is a minidump, we need to create a ProcessMinidump (which
> calls MinidumpParser::Create())
> > 3. once the plugin is selected, Process::LoadCore() is finally called
> and this the earliest we can do minidump-specific error checking
> >
> > Note that at step 2b. we don't have a way to propagate the error since
> we're just doing the plugin lookup (the most we can pass on the lookup to
> the rest of the plugins). We can't easily defer the
> MinidumpParser::Create() as step 2b either since that only morphs into a
> different kind of two-stage initialization (having a ProcessMinidump w/o a
> parser).
> >
> > I agree that it would be nicer with a one step initialization but
> overall changing the LLDB plugin lookup is too intrusive for the relatively
> small benefit. If you have any suggestions I'd love to hear them.
> >
> >
> > On Mon, Jul 16, 2018 at 9:04 AM, Pavel Labath via Phabricator <
> reviews at reviews.llvm.org> wrote:
> >>
> >> 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 methods, then it
> should be done by the `Create` function. Ideally this would be done before
> even constructing the parser object, but if this is impractical for some
> reason then you can make the `Initialize` function private and call it
> directly from `Create`. This way a user will never be able to see an
> malformed parser object. To make sure you propagate the error, you can
> change the return type of `Create` to `llvm::Expected<MinidumpParser`
> (the only reason we did not do this back then was that there was no
> precedent for using `Expected` in LLDB, but this is no longer the case).
> >>
> >>
> >> Repository:
> >>   rL LLVM
> >>
> >> https://reviews.llvm.org/D49202
> >>
> >>
> >>
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20180716/1a374a4e/attachment-0001.html>