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

[Pavel Labath via Phabricator via llvm-commits]

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 wrote:
> labath wrote:
> > This file has a helper function for this (`mapOptional(IO, "name", value, 0)`. I'd consider changing the field name to "Number of Parameters" even though it does not match the field name, as it reads weird without that. I'm not sure why the microsoft naming is inconsistent here -- most of the other minidump structs have "of" in their name already (BaseOfImage, SizeOfImage, etc.), but at least we can be consistent.
> Updated to use the helper, and changed the name in the YAML to "Number of Parameters".  Let me know if it's important to you to also change the name of the field in the llvm::minidump::Exception type to `NumberOfParameters` -- I wasn't sure if you were suggesting that, and regardless my preference would be to leave that as-is to match breakpad aside from casing, as otherwise it's hard to know where to stop (e.g. change "ExceptionInformation" to "Parameters" to match "NumberOfParameters" and the YAML?  Reconcile the several different ways that alignment padding fields are named?  etc.)
I wasn't sure about that either -- that's why it was phrased so vaguely. While I generally tried to stick to the original naming, I have already done some renames to the field names in existing structures -- IIRC, it was limited to unifying the naming for the various "reserved"/"unused" fields. Anyway, I think that keeping the original name in this particular case is fine.


================
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).
----------------
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? :)


================
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());
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68657