<div dir="ltr">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).</div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jul 16, 2018 at 10:36 AM, Pavel Labath <span dir="ltr"><<a href="mailto:labath@google.com" target="_blank">labath@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Ok, I see what you mean now.<br>
<br>
Looking at the other core file plugins (elf, mach-o), it looks like<br>
they do only very basic verification before the accept the file. The<br>
pretty much just check the magic numbers, which would be more-or-less<br>
equivalent to our `MinidumpHeader::Parse` call. As this does not<br>
require creating a parser object, maybe we could delay the parser<br>
creation until `LoadCore` gets called (at which point you can easily<br>
report errors).<br>
<br>
This will leave us with a nice MinidumpParser interface.<br>
ProcessMinidump will still use two-stage initialization, but this is<br>
nothing new, and this change will make it easier for us to change the<br>
initialization method of the Process objects in the future.<br>
<br>
WDYT?<br>
<div class="HOEnZb"><div class="h5"><br>
On Mon, 16 Jul 2018 at 18:16, Leonard Mosescu <<a href="mailto:mosescu@google.com">mosescu@google.com</a>> wrote:<br>
><br>
> 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:<br>
><br>
> 1. Target::CreateProcess() calls Process::FindPlugin()<br>
> 2. ProcessMinidump::<wbr>CreateInstance() then has to inspect the core file to see if it's a minidump<br>
> 2b. ... if it is a minidump, we need to create a ProcessMinidump (which calls MinidumpParser::Create())<br>
> 3. once the plugin is selected, Process::LoadCore() is finally called and this the earliest we can do minidump-specific error checking<br>
><br>
> 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).<br>
><br>
> 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.<br>
><br>
><br>
> On Mon, Jul 16, 2018 at 9:04 AM, Pavel Labath via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br>
>><br>
>> labath added a comment.<br>
>><br>
>> 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<<wbr>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).<br>
>><br>
>><br>
>> Repository:<br>
>>   rL LLVM<br>
>><br>
>> <a href="https://reviews.llvm.org/D49202" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D49202</a><br>
>><br>
>><br>
>><br>
><br>
</div></div></blockquote></div><br></div>