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 <lab...@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 <mose...@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 < > revi...@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 > >> > >> > >> > > >
_______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
