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
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
