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

Fri Oct 11 09:01:05 PDT 2019

JosephTremoulet marked 4 inline comments as done.
JosephTremoulet added a comment.

Added Exception stream to minidump-basic.yaml as suggested.

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68657