amccarth accepted this revision.
amccarth added a reviewer: amccarth.
amccarth added a comment.
LGTM. Thanks!
================
Comment at: source/Plugins/Process/Windows/MiniDump/ProcessWinMiniDump.cpp:338
@@ -315,4 +337,3 @@
// truncated.
- error.SetErrorString("address is not in a known range");
return error;
}
----------------
hhellyer wrote:
> amccarth wrote:
> > hhellyer wrote:
> > > Asking for an address outside a known range is not actually an error, you
> > > just get back an unmapped range that specifies how far it is to the next
> > > mapped range.
> > > The original discussion about that happened here:
> > > https://reviews.llvm.org/D21751 - GetMemoryRegionInfo should only need to
> > > return an error if it is unimplemented. (I should probably have
> > > referenced that in the summary.)
> > Got it. Thanks for the explanation!
> >
> > Given that, should the condition on 284 ("the mini dump contains no memory
> > range information") actually return an error or just an unmapped range?
> I thought the most likely cause of having no memory info would be not passing
> the MiniDumpWithFullMemoryInfo flag to MiniDumpWriteDump will create a dump
> without the MemoryInfoListStream present. Saying that memory range
> information from a dump like that is "unsupported" seemed like the right
> option. I think MiniDumpWithFullMemoryInfo is part of the default set of
> flags used when you create a dump through task manager so most dumps should
> have the stream.
>
> I won't land the patch immediately, I'm happy to change it if you'd prefer it
> to return an unmapped range. I think it would have to be one unmapped range
> that ran from load_addr to LLDB_INVALID_ADDRESS to match the expected
> behaviour.
Sounds like you've thought it through more thoroughly than I, and I agree with
your conclusions, so I have no objections. Thanks for the explanations.
https://reviews.llvm.org/D22352
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
