Re: [Lldb-commits] [PATCH] D24385: MinidumpParsing: pid, modules, exceptions

2016-09-13 Thread Dimitar Vlahovski via lldb-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rL281348: MinidumpParsing: pid, modules, exceptions, strings (authored by dvlahovski). Changed prior to commit: https://reviews.llvm.org/D24385?vs=71154&id=71182#toc Repository: rL LLVM https://review

Re: [Lldb-commits] [PATCH] D24385: MinidumpParsing: pid, modules, exceptions

2016-09-13 Thread Zachary Turner via lldb-commits
Looks good here too On Tue, Sep 13, 2016 at 6:21 AM Pavel Labath wrote: > labath added a comment. > > Looks good as far as I am concerned. > > > https://reviews.llvm.org/D24385 > > > > ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lis

Re: [Lldb-commits] [PATCH] D24385: MinidumpParsing: pid, modules, exceptions

2016-09-13 Thread Pavel Labath via lldb-commits
labath added a comment. Looks good as far as I am concerned. https://reviews.llvm.org/D24385 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D24385: MinidumpParsing: pid, modules, exceptions

2016-09-13 Thread Dimitar Vlahovski via lldb-commits
dvlahovski updated this revision to Diff 71154. dvlahovski added a comment. Making the LinuxProcStatus a smarter class by not parsing the pid each time when GetPid is called https://reviews.llvm.org/D24385 Files: source/Plugins/Process/minidump/MinidumpParser.cpp source/Plugins/Process/min

Re: [Lldb-commits] [PATCH] D24385: MinidumpParsing: pid, modules, exceptions

2016-09-13 Thread Dimitar Vlahovski via lldb-commits
dvlahovski updated this revision to Diff 71151. dvlahovski marked 3 inline comments as done. dvlahovski added a comment. Removed consumeString; Fixed comparisons in unittests; out-of-bounds check in MinidumpString parsing https://reviews.llvm.org/D24385 Files: source/Plugins/Process/minidump

Re: [Lldb-commits] [PATCH] D24385: MinidumpParsing: pid, modules, exceptions

2016-09-13 Thread Pavel Labath via lldb-commits
labath added a comment. Just a couple of tiny comments from me. Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:86 @@ +85,3 @@ +llvm::Optional MinidumpParser::GetMinidumpString(uint32_t rva) { + auto arr_ref = m_data_sp->GetData().drop_front(rva); + return parse

Re: [Lldb-commits] [PATCH] D24385: MinidumpParsing: pid, modules, exceptions

2016-09-12 Thread Zachary Turner via lldb-commits
zturner added inline comments. Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:222 @@ -154,1 +221,2 @@ + return MinidumpExceptionStream::Parse(data); } dvlahovski wrote: > Now that I return an ArrayRef, which is basically a reinterpret > cast of

Re: [Lldb-commits] [PATCH] D24385: MinidumpParsing: pid, modules, exceptions

2016-09-12 Thread Dimitar Vlahovski via lldb-commits
dvlahovski added inline comments. Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:222 @@ -154,1 +221,2 @@ + return MinidumpExceptionStream::Parse(data); } Now that I return an ArrayRef, which is basically a reinterpret cast of a piece of memory t

Re: [Lldb-commits] [PATCH] D24385: MinidumpParsing: pid, modules, exceptions

2016-09-12 Thread Dimitar Vlahovski via lldb-commits
dvlahovski updated this revision to Diff 71026. dvlahovski marked 17 inline comments as done. dvlahovski added a comment. Herald added subscribers: mgorny, srhines, danalbert, tberghammer. Changes regarding all of the comments. Removed llvm::Optionals where it was unneeded. Parsing also the OS fr

Re: [Lldb-commits] [PATCH] D24385: MinidumpParsing: pid, modules, exceptions

2016-09-12 Thread Adrian McCarthy via lldb-commits
> Even if it's length prefixed, the logic here basically just consumes the entire buffer, which doesn't seem right Yes, agreed. On Fri, Sep 9, 2016 at 5:45 PM, Zachary Turner wrote: > Even if it's length prefixed, the logic here basically just consumes the > entire buffer, which doesn't seem ri

Re: [Lldb-commits] [PATCH] D24385: MinidumpParsing: pid, modules, exceptions

2016-09-09 Thread Zachary Turner via lldb-commits
Even if it's length prefixed, the logic here basically just consumes the entire buffer, which doesn't seem right On Fri, Sep 9, 2016 at 5:43 PM Adrian McCarthy wrote: > amccarth added inline comments. > > > Comment at: source/Plugins/Process/minidump/MinidumpTypes.cpp:21 > @@ +20

Re: [Lldb-commits] [PATCH] D24385: MinidumpParsing: pid, modules, exceptions

2016-09-09 Thread Adrian McCarthy via lldb-commits
amccarth added inline comments. Comment at: source/Plugins/Process/minidump/MinidumpTypes.cpp:21 @@ +20,3 @@ +llvm::StringRef +lldb_private::minidump::consumeString(llvm::ArrayRef &Buffer) { + return llvm::StringRef(reinterpret_cast(Buffer.data()), zturner wrote:

Re: [Lldb-commits] [PATCH] D24385: MinidumpParsing: pid, modules, exceptions

2016-09-09 Thread Zachary Turner via lldb-commits
zturner added inline comments. Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:88-89 @@ +87,4 @@ +MinidumpParser::GetMinidumpString(uint32_t rva) { + llvm::ArrayRef arr_ref(m_data_sp->GetBytes() + rva, + m_data_sp->GetByteSize() - r

Re: [Lldb-commits] [PATCH] D24385: MinidumpParsing: pid, modules, exceptions

2016-09-09 Thread Pavel Labath via lldb-commits
labath added a comment. A collection of small remarks, mostly to do with style. Mainly, I'd like to make the tests more strict about what they accept as reasonable values. A higher level question: Given that we are already in the `minidump` namespace, do we want all (most) of these symbols to b

Re: [Lldb-commits] [PATCH] D24385: MinidumpParsing: pid, modules, exceptions

2016-09-09 Thread Dimitar Vlahovski via lldb-commits
dvlahovski added a comment. Also added parsing code for Minidump strings - the string in the file are UTF-16 encoded. I used the code from the WinMiniDump plugin and it can extract a UTF-16 string and convert it to a UTF-8 one. https://reviews.llvm.org/D24385 ___

Re: [Lldb-commits] [PATCH] D24385: MinidumpParsing: pid, modules, exceptions

2016-09-09 Thread Dimitar Vlahovski via lldb-commits
dvlahovski updated this revision to Diff 70816. dvlahovski added a comment. Forgot to run clang-format https://reviews.llvm.org/D24385 Files: source/Plugins/Process/minidump/MinidumpParser.cpp source/Plugins/Process/minidump/MinidumpParser.h source/Plugins/Process/minidump/MinidumpTypes.c

[Lldb-commits] [PATCH] D24385: MinidumpParsing: pid, modules, exceptions

2016-09-09 Thread Dimitar Vlahovski via lldb-commits
dvlahovski created this revision. dvlahovski added reviewers: labath, zturner. dvlahovski added a subscriber: lldb-commits. Herald added a subscriber: beanz. Added parsing of the MiscInfo data stream. The main member of it that we care about is the process_id On Linux generated Minidump (from brea