This revision was automatically updated to reflect the committed changes.
Closed by commit rL283259: Adding a new Minidump post-mortem debugging plugin
(authored by dvlahovski).
Changed prior to commit:
https://reviews.llvm.org/D25196?vs=73541&id=73551#toc
Repository:
rL LLVM
https://review
labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.
lgtm
https://reviews.llvm.org/D25196
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lld
dvlahovski updated this revision to Diff 73541.
dvlahovski marked 2 inline comments as done.
dvlahovski added a comment.
Added a sanity check for loc_descr
https://reviews.llvm.org/D25196
Files:
packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/Makefile
packages/Pytho
labath added a comment.
Thanks for fixing all the comments. Unfortunately, on my last pass, I found one
more case of unverified input (I think).
> MinidumpParser.cpp:252
> +if (range_start <= addr && addr < range_start + range_size) {
> + return Range(range_start, GetData().slice(loc_
dvlahovski marked an inline comment as done.
dvlahovski added inline comments.
> zturner wrote in MinidumpParser.cpp:81-82
> Change this line to `return GetData().slice(iter->second.rva,
> iter->second.data_size);`
Nice! :)
> zturner wrote in MinidumpParser.h:35-36
> If the comment is long en
dvlahovski updated this revision to Diff 73526.
dvlahovski marked 6 inline comments as done.
dvlahovski added a comment.
Changes after Zachary's comments
https://reviews.llvm.org/D25196
Files:
packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/Makefile
packages/Python/
zturner added inline comments.
> MinidumpParser.cpp:81-82
>
> - return llvm::ArrayRef(m_data_sp->GetBytes() + iter->second.rva,
> + return llvm::ArrayRef(GetData().data() + iter->second.rva,
> iter->second.data_size);
> }
Change this line to `return GetDat
dvlahovski updated this revision to Diff 73516.
dvlahovski marked 5 inline comments as done.
dvlahovski added a comment.
Second iteration over CL - regarded Pavel's comments and encapsulated m_data_sp
more in MinidumpParser
https://reviews.llvm.org/D25196
Files:
packages/Python/lldbsuite/te
labath added a comment.
Just a couple more details and I think we're ready.
> MinidumpParser.cpp:105
> +MinidumpParser::GetThreadContext(const MinidumpThread &td) {
> + return td.GetContext(GetData().data());
> +}
I think you have made it over-encapsulated now. :)
Either a Parser function whi
dvlahovski updated this revision to Diff 73504.
dvlahovski marked 7 inline comments as done.
dvlahovski added a comment.
Updated the CL with regard to Pavel's comments
https://reviews.llvm.org/D25196
Files:
packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/Makefile
pa
dvlahovski marked 5 inline comments as done.
dvlahovski added inline comments.
> labath wrote in TestMiniDumpNew.py:19
> They are not building any code, so the would behave the same way anyway. You
> would be just running the test 2--3 times for nothing.
Aah I understand now. Ok thanks :)
http
labath added inline comments.
> dvlahovski wrote in TestMiniDumpNew.py:19
> But, should `test_deeper_stack_in_mini_dump` and
> `test_local_variables_in_mini_dump` not have this decorator ?
They are not building any code, so the would behave the same way anyway. You
would be just running the te
dvlahovski added inline comments.
> labath wrote in TestMiniDumpNew.py:19
> You can replace these with `NO_DEBUG_INFO_TESTCASE = True` at class level.
But, should `test_deeper_stack_in_mini_dump` and
`test_local_variables_in_mini_dump` not have this decorator ?
https://reviews.llvm.org/D25196
dvlahovski added a comment.
In https://reviews.llvm.org/D25196#559368, @amccarth wrote:
> I was hoping that, with your new mini dump parser, you'd be able to eliminate
> the need for the Windows-specific minidump process plugin.
>
> When I wrote the Windows mini dump plugin, I tried to isolate t
dvlahovski added inline comments.
> labath wrote in ProcessMinidump.cpp:67
> It already is return true.
>
> Is that correct?
What I meant here is: is there functionality that needs to be added here.
In the WinMinidump plugin, this method is only returning true (as mine) and has
a TODO saying t
amccarth added a comment.
I was hoping that, with your new mini dump parser, you'd be able to eliminate
the need for the Windows-specific minidump process plugin.
When I wrote the Windows mini dump plugin, I tried to isolate the Windows
API-specific bits using the pimpl idiom. Now that you've
labath requested changes to this revision.
labath added a comment.
This revision now requires changes to proceed.
I have a bunch of small comments. I'll have another look through this once they
are done.
The high-level change we need is to avoid choosing the plugin to use at
compile-time (your
dvlahovski added a comment.
So, there will be changes to the way I handle the case where there is no
ExceptionStream - I will probably create a StopReason = None. But I have to
investigate if in that case lldb still hangs because it's trying to resume a
process (that does not exists). I will as
dvlahovski created this revision.
dvlahovski added reviewers: labath, zturner.
dvlahovski added subscribers: lldb-commits, amccarth.
Herald added subscribers: modocache, mgorny, beanz.
This plugin resembles the already existing Windows-only Minidump plugin.
The WinMinidumpPlugin uses the Windows A
19 matches
Mail list logo
