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

[Pavel Labath via Phabricator via llvm-commits]

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: llvm/include/llvm/ObjectYAML/MinidumpYAML.h:162-181
+/// ExceptionStream minidump stream.
+struct ExceptionStream : public Stream {
+  minidump::ExceptionStream MDExceptionStream;
+  yaml::BinaryRef ThreadContext;
+
+  explicit ExceptionStream(const minidump::ExceptionStream &MDExceptionStream,
+                           ArrayRef<uint8_t> ThreadContext)
----------------
I've been trying to keep this somewhat sorted. Could you move this before the `MemoryInfoListStream` class? Also, in the previous patch we've moved the default constructors to front. It would be good to make this consistent with that.


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


================
Comment at: llvm/lib/ObjectYAML/MinidumpYAML.cpp:408-414
+StringRef yaml::MappingTraits<minidump::Exception>::validate(
+    yaml::IO &IO, minidump::Exception &Exception) {
+  if (Exception.NumberParameters > Exception::MaxParameters)
+    return "Exception reports too many parameters";
+  return "";
+}
+
----------------
Could you remove this bit too? While it is technically invalid, this is not something that yaml2obj needs to care about (as it does not prevent successful serialization), and it would be nice to be able to use it to generate a test case with an invalid number (because that is something lldb should care about and expect/handle)..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68657