This revision was automatically updated to reflect the committed changes.
Closed by commit rGa50272f8261f: Update MinidumpYAML to use minidump::Exception
for exception stream (authored by JosephTremoulet).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
JosephTremoulet updated this revision to Diff 225459.
JosephTremoulet marked an inline comment as done.
JosephTremoulet added a comment.
- Rebase
- Use Twine instead of formatv
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68657/new/
JosephTremoulet marked an inline comment as done.
JosephTremoulet added inline comments.
Comment at: llvm/lib/ObjectYAML/MinidumpYAML.cpp:389
+ for (size_t Index = 0; Index < Exception.MaxParameters; ++Index) {
+SmallString<16> Name = formatv("Parameter {0}", Index);
+
JosephTremoulet marked an inline comment as done.
JosephTremoulet added inline comments.
Comment at: llvm/lib/ObjectYAML/MinidumpYAML.cpp:389
+ for (size_t Index = 0; Index < Exception.MaxParameters; ++Index) {
+SmallString<16> Name = formatv("Parameter {0}", Index);
+
MaskRay added inline comments.
Comment at: llvm/lib/ObjectYAML/MinidumpYAML.cpp:389
+ for (size_t Index = 0; Index < Exception.MaxParameters; ++Index) {
+SmallString<16> Name = formatv("Parameter {0}", Index);
+support::ulittle64_t =
grimar added inline comments.
Comment at: llvm/test/tools/yaml2obj/minidump-exception-missing-parameter.yaml:3
+
+# Test that we report an error for an ExceptionStream where the specified
+# number of parameters is greater than the number of ExceptionInformation
JosephTremoulet updated this revision to Diff 225044.
JosephTremoulet added a comment.
- punctuation fixes
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68657/new/
https://reviews.llvm.org/D68657
Files:
JosephTremoulet marked an inline comment as done.
JosephTremoulet added inline comments.
Comment at: llvm/test/tools/yaml2obj/minidump-exception-missing-parameter.yaml:3
+
+# Test that we report an error for an ExceptionStream where the specified
+# number of parameters is
grimar accepted this revision.
grimar added a comment.
This revision is now accepted and ready to land.
LGTM with 2 nits.
Comment at: llvm/test/tools/yaml2obj/minidump-exception-missing-parameter.yaml:3
+
+# Test that we report an error for an ExceptionStream where the
labath added a comment.
Thanks. I think this is great. @grimar, do you have any more comments?
Comment at: llvm/lib/ObjectYAML/MinidumpYAML.cpp:377-383
+ // TODO: We could provide a reasonable default for ThreadContext by searching
+ // the Thread stream for a thread with
JosephTremoulet updated this revision to Diff 224857.
JosephTremoulet marked 2 inline comments as done.
JosephTremoulet added a comment.
- Move test
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68657/new/
https://reviews.llvm.org/D68657
Files:
labath added inline comments.
Comment at: llvm/unittests/ObjectYAML/MinidumpYAMLTest.cpp:310-316
+ ASSERT_FALSE(ExpectedFile);
+ auto Unhandled =
+ handleErrors(ExpectedFile.takeError(), [](const StringError ) {
+EXPECT_EQ(static_cast(std::errc::invalid_argument),
JosephTremoulet marked 4 inline comments as done.
JosephTremoulet added inline comments.
Comment at: llvm/lib/ObjectYAML/MinidumpYAML.cpp:377-383
+ // TODO: We could provide a reasonable default for ThreadContext by searching
+ // the Thread stream for a thread with the given
JosephTremoulet updated this revision to Diff 224853.
JosephTremoulet added a comment.
- Remove TODO, lit-ify negative test and tighten check
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68657/new/
https://reviews.llvm.org/D68657
Files:
labath added inline comments.
Comment at: llvm/lib/ObjectYAML/MinidumpYAML.cpp:394
+ mapOptionalHex(IO, "Exception Address", Exception.ExceptionAddress, 0);
+ IO.mapOptional("Number Parameters", Exception.NumberParameters,
+ support::ulittle32_t(0u));
JosephTremoulet updated this revision to Diff 224805.
JosephTremoulet added a comment.
- ...and fix namespace...
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68657/new/
https://reviews.llvm.org/D68657
Files:
JosephTremoulet updated this revision to Diff 224804.
JosephTremoulet added a comment.
- Fix Expected<> types
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68657/new/
https://reviews.llvm.org/D68657
Files:
JosephTremoulet updated this revision to Diff 224800.
JosephTremoulet marked an inline comment as done.
JosephTremoulet added a comment.
- Apply review feedback (-auto, -memset, +comments)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68657/new/
grimar added inline comments.
Comment at: llvm/include/llvm/ObjectYAML/MinidumpYAML.h:113-114
+ ExceptionStream()
+ : Stream(StreamKind::Exception, minidump::StreamType::Exception) {
+memset(, 0, sizeof(minidump::ExceptionStream));
+ }
I'd avoid
JosephTremoulet updated this revision to Diff 224607.
JosephTremoulet added a comment.
Address review feedback
- Add Exception stream to minidump-basic.yaml to test obj2yaml
direction
- Reorder ExceptionStream definition and constructors
- Use the mapOptional helper
-
JosephTremoulet marked 4 inline comments as done.
JosephTremoulet added a comment.
Added Exception stream to minidump-basic.yaml as suggested.
Comment at: llvm/lib/ObjectYAML/MinidumpYAML.cpp:394
+ mapOptionalHex(IO, "Exception Address", Exception.ExceptionAddress, 0);
+
labath added a comment.
Thanks. IIUC, all the existing tests just cover the yaml2obj direction. Could
you add something for the other direction too? Maybe add an exception stream to
`test/tools/obj2yaml/basic-minidump.yaml`?
Comment at:
JosephTremoulet updated this revision to Diff 224469.
JosephTremoulet added a comment.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
- Update test input yaml Exception stream
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
23 matches
Mail list logo
