[Lldb-commits] [PATCH] D68657: Update MinidumpYAML to use minidump::Exception for exception stream

[Joseph Tremoulet via Phabricator via lldb-commits]

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 ID and using its Context.
+  // That would require a couple changes:
+  //   1. We'd need to pass the whole dump around as a context argument.
+  //   2. We'd need to ensure that the Thread stream got processed before
+  //      the Exception stream (or make Exception's ThreadContext required
+  //      when the Exception stream is processed before the Thread stream).
----------------
labath wrote:
> I've been thinking about this for a while now, and while that idea has some appeal, I am not sure if this would ever really be a "good" idea. Currently, this format allows you to easily create syntactically-valid-but-probably-nonsensical minidumps (multiple thread list streams, multiple threads with the same ID, etc..). All of that would be more difficult if we started depending on strict "correctness" of other parts of the minidump in order to compute something here.
> 
> Even if I was doing this, I'd probably implement this differently -- make the context always optional, but then check for consistency higher up (the validate call of the entire minidump object or something). Anyway, maybe just delete this todo? :)
I've removed the TODO, but I reserve the right to think this would be a good idea :). Speficially because I think you could still model in YAML anything you could put in a minidump file, by explicitly providing the ThreadContext even when it has a default.

(This is in contrast to the other validation stuff I had in earlier revisions, where I was just misunderstanding the point of yaml validation -- so thanks for explaining it!) 


================
Comment at: llvm/unittests/ObjectYAML/MinidumpYAMLTest.cpp:310-316
+  ASSERT_FALSE(ExpectedFile);
+  auto Unhandled =
+      handleErrors(ExpectedFile.takeError(), [](const StringError &error) {
+        EXPECT_EQ(static_cast<int>(std::errc::invalid_argument),
+                  error.convertToErrorCode().value());
+      });
+  EXPECT_THAT_ERROR(std::move(Unhandled), Succeeded());
----------------
labath wrote:
> Maybe:
> ```
> EXPECT_THAT_EXPECTED(ExpectedFile, Failed<StringError>(
>     testing::Property(&StringError::convertToErrorCode,
>          make_error_code(errc::invalid_argument))));
> ```
> ?
> 
> Though, this might actually be best off as a lit test where you just FileCheck the exact error message.
Moved to lit, thanks for the suggestion.  The obvious place seemed to be llvm/test/ObjectYAML, but there weren't any minidump tests there so I added a minidump subdirectory for the new test.  Please let me know if there was a better place that I just overlooked.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68657/new/

https://reviews.llvm.org/D68657