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

[Pavel Labath via Phabricator via llvm-commits]

labath added reviewers: MaskRay, grimar.
labath added subscribers: MaskRay, grimar.
labath added a comment.

Adding @MaskRay and @grimar, as @jhenderson is out.

This ExceptionInformation stuff is pretty impressive, but I am wondering if it's really needed, particularly as lldb does not even use those fields right now. Also, while technically an array, the Parameter stuff isn't really used as an array, as each exception type (I actually know of only one: EXCEPTION_ACCESS_VIOLATION) defines fixed meanings for each position.

Therefore I think it would make sense to just spell out each member of that array as a separate member in the yaml representation, which should be a much simpler endeavour. We can use the "actual parameter count" field to suppress the fields that don't contain any value, if they really are zero, which should make the yaml output concise in the usual cases. I think something like this should be sufficient:

  IO.mapOptional("Number of Parameters", Record.NumberParameters, 0);
  for(unsigned I = 0; I < Exception::MaxParameters ++I) {
    if (I <Record.NumberParameters)
      mapRequiredHex(IO, "Parameter " + to_string(I),  Record.ExceptionInformation[I]);
    else
      mapOptionalHex(IO, "Parameter " + to_string(I),  Record.ExceptionInformation[I], 0);
  }

I think that would strike a good balance between code complexity, output brevity, and being able to generate interesting and potentially invalid inputs for other tools (which is one of the main goals of yaml2obj, and so interpreting the input too strictly is not desired/helpful).



================
Comment at: llvm/lib/ObjectYAML/MinidumpEmitter.cpp:127
+  // Lay out the thread context data, (which is not a part of the stream).
+  // TODO: This usually (always?) matches the thread context of the
+  // corresponding thread, and may overlap memory regions as well.  We could
----------------
Yeah, this isn't an issue specific to just this stream. There are other places which suffer from the same problem too (e.g. the "stack" field of a thread will usually match/point to the same blob as one of the MemoryList entries. So far, I have been ignoring that because this does not generally matter for the consumers, and it capturing that in yaml in a form which would still be easily editable/reducable would be tricky.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68657