dvlahovski added inline comments.
Comment at: lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp:146
@@ +145,3 @@
+break;
+}
+
labath wrote:
> You have a "enumeration not handled in a switch" warning here. Could you do
> something about
labath added inline comments.
Comment at: lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp:146
@@ +145,3 @@
+break;
+}
+
You have a "enumeration not handled in a switch" warning here. Could you do
something about that?
Repository:
This revision was automatically updated to reflect the committed changes.
Closed by commit rL280356: Minidump parsing (authored by dvlahovski).
Changed prior to commit:
https://reviews.llvm.org/D23545?vs=69340&id=69988#toc
Repository:
rL LLVM
https://reviews.llvm.org/D23545
Files:
lldb/tr
amccarth added a comment.
LGTM.
https://reviews.llvm.org/D23545
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
zturner accepted this revision.
zturner added a reviewer: zturner.
This revision is now accepted and ready to land.
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:60
@@ +59,3 @@
+MinidumpParser parser(data_buf_sp, header, directory_map);
+return llvm::Optio
dvlahovski updated this revision to Diff 69340.
dvlahovski added a comment.
Making MinidumpParser constuctor private
https://reviews.llvm.org/D23545
Files:
cmake/LLDBDependencies.cmake
source/Plugins/Process/CMakeLists.txt
source/Plugins/Process/minidump/CMakeLists.txt
source/Plugins/Pr
labath added a comment.
Looks fine to me. Adrian, Zachary, any more thoughts here?
Comment at: source/Plugins/Process/minidump/MinidumpParser.h:42
@@ +41,3 @@
+public:
+explicit MinidumpParser(const lldb::DataBufferSP &data_buf_sp, const
MinidumpHeader *header,
+
dvlahovski added a comment.
I changed all of the `llvm::Optional` returning functions to
return only `const type *`
and signalize for 'failure' by returning a `nullptr`. In the cases where I
return objects (e.g. vector of threads)
I'm still using the `llvm::Optional` pattern.
I also think that u
dvlahovski updated this revision to Diff 69254.
dvlahovski added a comment.
Changed the constructing pattern of MinidumpParser
Now there is a static Create method that returns
and llvm::Optional MinidumpParser.
https://reviews.llvm.org/D23545
Files:
cmake/LLDBDependencies.cmake
source/Plug
zturner added a comment.
Were you still going to change all the `Optionals` to raw pointers (or even
better, `llvm::Errors`)
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:21
@@ +20,3 @@
+MinidumpParser::MinidumpParser(const lldb::DataBufferSP &data_buf_sp)
+
dvlahovski updated this revision to Diff 68690.
dvlahovski added a comment.
Move creation of ArrayRef after sanity check
Add a test that checks that the thread list is not present in a
truncated minidump file
https://reviews.llvm.org/D23545
Files:
cmake/LLDBDependencies.cmake
source/Plugin
labath added a comment.
Looks good as far as I am concerned. You might want to add a test or two that
makes sure we handle invalid data reasonably. E.g., load only the first X kb of
the minidump (so that the thread list is not present in the loaded block), and
make sure getting a thread list fa
dvlahovski added a comment.
I'm thinking of doing the following approach regarding the plugin
implementation:
Start writing a new plugin "from scratch", which will be inspired strongly by
ProcessWinMiniDump and also borrowing stuff from ProcessElfCore.
Also my plan is to make a single cross-plat
dvlahovski updated this revision to Diff 68670.
dvlahovski added a comment.
Adding sanity checks whether the data buffer
contains the amound of bytes that we want to read.
All of the functions that were returning llvm::Optional
now return just const something *
and indicate for failure with the r
labath added a comment.
> > Do you have a suggestion where the parsing code to be, if not in the same
> > directory?
>
>
> The parsing code will end up being used by multiple plugins--the new one
> you're building for Linux and the existing one that's Windows-specific.
I was hoping that w
dvlahovski added inline comments.
Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:260
@@ +259,3 @@
+
+uint8_t number_of_processors;
+uint8_t product_type;
amccarth wrote:
> I'll concede that the static_assert is useful. Given that, showing the
zturner added inline comments.
Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:133
@@ +132,3 @@
+// This matches the Linux kernel definitions from
+enum MinidumpPCPUInformationARMElfHwCaps
+{
dvlahovski wrote:
> amccarth wrote:
> > zturner wrote:
> >
dvlahovski added inline comments.
Comment at: source/Plugins/Process/minidump/MinidumpTypes.cpp:21
@@ +20,3 @@
+
+llvm::Optional
+MinidumpHeader::Parse(llvm::ArrayRef &data)
zturner wrote:
> I think these can all just return the pointer instead of `llvm::Optional<
amccarth added a comment.
In https://reviews.llvm.org/D23545#519675, @dvlahovski wrote:
> In https://reviews.llvm.org/D23545#516808, @amccarth wrote:
>
> > Are we putting this code in the right place? I wouldn't expect minidump
> > parsing to fall under Plugins/Process.
> >
> > I assume the eve
dvlahovski added a comment.
Also FYI I copied the implementation of `consumeObject` to my header.
https://reviews.llvm.org/D23545
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
zturner added inline comments.
Comment at: source/Plugins/Process/minidump/MinidumpParser.h:67
@@ +66,3 @@
+bool m_valid_data;
+llvm::DenseMap m_directory_map;
+};
dvlahovski wrote:
> zturner wrote:
> > Can this be `llvm::DenseMap > MinidumpLocationDescrip
dvlahovski added inline comments.
Comment at: source/Plugins/Process/minidump/MinidumpParser.h:67
@@ +66,3 @@
+bool m_valid_data;
+llvm::DenseMap m_directory_map;
+};
zturner wrote:
> Can this be `llvm::DenseMap`?
>
> No point erasing the type information
zturner added inline comments.
Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:259
@@ +258,3 @@
+static_assert(sizeof(MinidumpSystemInfo) == MINIDUMP_SYSTEM_INFO_SIZE, "sizeof
MinidumpSystemInfo is not correct!");
+
+struct MinidumpMiscInfo
I think th
zturner added inline comments.
Comment at: source/Plugins/Process/minidump/MinidumpParser.h:67
@@ +66,3 @@
+bool m_valid_data;
+llvm::DenseMap m_directory_map;
+};
Can this be `llvm::DenseMap`?
No point erasing the type information of the enum.
=
dvlahovski added a comment.
In https://reviews.llvm.org/D23545#516808, @amccarth wrote:
> Are we putting this code in the right place? I wouldn't expect minidump
> parsing to fall under Plugins/Process.
>
> I assume the eventual intent is to turn the Windows-specific code into a user
> of your
dvlahovski added inline comments.
Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:258
@@ +257,3 @@
+llvm::support::ulittle16_t processor_level;
+llvm::support::ulittle16_t processor_revision;
+
The idea of this is to be sure that the compiler d
dvlahovski updated this revision to Diff 68557.
dvlahovski added a comment.
Update of the code, regarding the remarks
of zturner and amccarth.
https://reviews.llvm.org/D23545
Files:
cmake/LLDBDependencies.cmake
source/Plugins/Process/CMakeLists.txt
source/Plugins/Process/minidump/CMakeLi
zturner added inline comments.
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:29
@@ +28,3 @@
+
+lldb::ByteOrder byteorder = m_data.GetByteOrder();
+lldb::offset_t directory_list_offset = m_header->stream_directory_rva;
I asked some people f
Btw the endian aware types are usable, just include
"llvm/Support/Endian.h". It's just the consumeObject functions that are not
On Tue, Aug 16, 2016 at 10:01 AM Zachary Turner wrote:
> zturner added a comment.
>
> LLVM does have something similar to DataExtractor, but unfortunately it's
> not pre
zturner added a comment.
LLVM does have something similar to DataExtractor, but unfortunately it's not
present in the correct library for you to easily be able to reuse it. I
actually own the code in question, so I'm willing to fix that for you if you're
interested, but at the same time the co
amccarth added a comment.
Are we putting this code in the right place? I wouldn't expect minidump
parsing to fall under Plugins/Process.
I assume the eventual intent is to turn the Windows-specific code into a user
of your new code? I look forward to seeing that happen.
Com
I see. Let me follow up with the breakpad developers to ask them about
that. If you only have to support one endianness the code can be much
cleaner.
On Tue, Aug 16, 2016 at 9:44 AM Dimitar Vlahovski
wrote:
> dvlahovski added inline comments.
>
>
> Comment at: source/Plugins/P
dvlahovski added inline comments.
Comment at: source/Plugins/Process/minidump/MinidumpTypes.cpp:31-35
@@ +30,7 @@
+// the signature might be byte swapped
+data.SetByteOrder(lldb::eByteOrderBig);
+*offset -= 4;
+signature = data.GetU32(offset);
+if (signature ==
zturner added a subscriber: zturner.
Comment at: source/Plugins/Process/minidump/MinidumpTypes.cpp:31-35
@@ +30,7 @@
+// the signature might be byte swapped
+data.SetByteOrder(lldb::eByteOrderBig);
+*offset -= 4;
+signature = data.GetU32(offset);
+if (signature
Thanks for the comment. Do you have anything particular in mind from the
llvm types? Is there something similar to DataExtractor?
On Tue, Aug 16, 2016 at 4:21 PM, Zachary Turner wrote:
> I would prefer to use llvm types to do the parsing wherever possible. Why
> do we need DataExtractor? If the
I would prefer to use llvm types to do the parsing wherever possible. Why
do we need DataExtractor? If the only reason is to force little endian,
just use the types in llvm/Endian.h
On Tue, Aug 16, 2016 at 8:13 AM Dimitar Vlahovski via lldb-commits <
lldb-commits@lists.llvm.org> wrote:
> dvlahovsk
dvlahovski updated this revision to Diff 68180.
dvlahovski added a comment.
Fixing a little bug - should get the byteorder
after calling SignatureMatchAndSetByteOrder
https://reviews.llvm.org/D23545
Files:
cmake/LLDBDependencies.cmake
source/Plugins/Process/CMakeLists.txt
source/Plugins/
dvlahovski updated this revision to Diff 68177.
dvlahovski added a comment.
Resolving Pavel's remarks
https://reviews.llvm.org/D23545
Files:
cmake/LLDBDependencies.cmake
source/Plugins/Process/CMakeLists.txt
source/Plugins/Process/minidump/CMakeLists.txt
source/Plugins/Process/minidump/
labath added a comment.
The implementation looks nice and clean. I have only some stylistic comments
giving it finishing touches..
Comment at: source/Plugins/Process/minidump/CMakeLists.txt:6
@@ +5,3 @@
+ MinidumpParser.cpp
+ #ProcessMinidump.cpp
+ #ThreadMinidump.cpp
--
39 matches
Mail list logo
