Page MenuHomePhabricator

Update MinidumpYAML to use minidump::Exception for exception stream
ClosedPublic

Authored by JosephTremoulet on Oct 8 2019, 11:53 AM.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptOct 8 2019, 11:53 AM
labath added subscribers: MaskRay, grimar.

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

llvm/lib/ObjectYAML/MinidumpEmitter.cpp
127

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.

  • Change Exception Information format per feedback
  • Add test with extraneous parameter

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

Ok. Updated. I agree the code is simpler this way and the YAML just as readable, plus it's nice not having to worry about buffer overrun. Added testcases with missing and extraneous parameters.

  • Update test input yaml Exception stream
Herald added a project: Restricted Project. · View Herald TranscriptOct 10 2019, 2:02 PM

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?

llvm/include/llvm/ObjectYAML/MinidumpYAML.h
181–200

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.

llvm/lib/ObjectYAML/MinidumpYAML.cpp
392

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.

406–412

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

Address review feedback

  - Add Exception stream to minidump-basic.yaml to test obj2yaml
    direction
  - Reorder ExceptionStream definition and constructors
  - Use the mapOptional helper
  - Replace "Number Parameters" with "Number of Parameters" in YAML
  - Stop using mapping traits validation for number of parameters,
    update test to ensure correctly de-yamlizing an exception record
    with an out-of-bounds number of parameters
JosephTremoulet marked 4 inline comments as done.Oct 11 2019, 9:00 AM

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

llvm/lib/ObjectYAML/MinidumpYAML.cpp
392

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

grimar added inline comments.Oct 12 2019, 4:16 AM
llvm/include/llvm/ObjectYAML/MinidumpYAML.h
113–114

I'd avoid memset:

ExceptionStream()
    : Stream(StreamKind::Exception, minidump::StreamType::Exception),
      MDExceptionStream({}) {
llvm/lib/ObjectYAML/MinidumpYAML.cpp
530

We often avoid using auto when return type is not obvious.

llvm/unittests/ObjectYAML/MinidumpYAMLTest.cpp
143

I'd add a comment for each test to describe what they do.

JosephTremoulet marked an inline comment as done.
  • Apply review feedback (-auto, -memset, +comments)
JosephTremoulet marked 3 inline comments as done.Oct 13 2019, 5:55 PM
  • Fix Expected<> types
  • ...and fix namespace...
labath added inline comments.Oct 14 2019, 2:36 AM
llvm/lib/ObjectYAML/MinidumpYAML.cpp
376–382

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

392

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.

llvm/unittests/ObjectYAML/MinidumpYAMLTest.cpp
310–316

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.

  • Remove TODO, lit-ify negative test and tighten check
JosephTremoulet marked 4 inline comments as done.Oct 14 2019, 6:55 AM
JosephTremoulet added inline comments.
llvm/lib/ObjectYAML/MinidumpYAML.cpp
376–382

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!)

llvm/unittests/ObjectYAML/MinidumpYAMLTest.cpp
310–316

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.

labath added inline comments.Oct 14 2019, 7:01 AM
llvm/unittests/ObjectYAML/MinidumpYAMLTest.cpp
310–316

there are some in test/tools/yaml2obj. I'd put it next to those.

JosephTremoulet marked 2 inline comments as done.
  • Move test

Thanks. I think this is great. @grimar, do you have any more comments?

llvm/lib/ObjectYAML/MinidumpYAML.cpp
376–382

Fair enough. :)

grimar accepted this revision.Oct 15 2019, 12:10 AM

LGTM with 2 nits.

llvm/test/tools/yaml2obj/minidump-exception-missing-parameter.yaml
4

nit: It is became common to use ## for comments in yaml2obj and actually many other LLVM tools test cases.
(here and below)

llvm/unittests/ObjectYAML/MinidumpYAMLTest.cpp
143

nit: Missing a full stop (here and below).

This revision is now accepted and ready to land.Oct 15 2019, 12:10 AM
JosephTremoulet marked an inline comment as done.Oct 15 2019, 7:54 AM
JosephTremoulet added inline comments.
llvm/test/tools/yaml2obj/minidump-exception-missing-parameter.yaml
4

Sorry, when you say "here and below", do you just mean lines 3 and 4-5, or do you also mean the CHECK line (line 20)? Grepping in test/tools it appears to be very uncommon to use ## for CHECK lines, but that's what I'd have otherwise assumed "and below" referred to...

  • punctuation fixes
grimar added inline comments.Oct 15 2019, 10:48 AM
llvm/test/tools/yaml2obj/minidump-exception-missing-parameter.yaml
4

Sorry, I think I misread the line 20, i.e. I read it as a comment. Indeed we do not use ## for CHECK lines.

MaskRay added inline comments.Oct 16 2019, 1:24 AM
llvm/lib/ObjectYAML/MinidumpYAML.cpp
388

You may use ("Parameter " + Twine(Index)).str() to get rid of the "llvm/Support/FormatVariadic.h" dependency.

JosephTremoulet marked an inline comment as done.Oct 16 2019, 4:32 AM
JosephTremoulet added inline comments.
llvm/lib/ObjectYAML/MinidumpYAML.cpp
388

Is #including FormatVariadic.h a bad thing? I did it this way to avoid heap allocation.

JosephTremoulet marked an inline comment as done.
  • Rebase
  • Use Twine instead of formatv
JosephTremoulet marked an inline comment as done.Oct 17 2019, 10:33 AM
JosephTremoulet added inline comments.
llvm/lib/ObjectYAML/MinidumpYAML.cpp
388

...finally realized that Twine avoids the heap too :). Thanks for the suggestion, updated.

This revision was automatically updated to reflect the committed changes.