Page MenuHomePhabricator

MinidumpYAML: add support for the ThreadList stream
ClosedPublic

Authored by labath on May 2 2019, 1:42 AM.

Details

Summary

The implementation is a pretty straightforward extension of the pattern
used for (de)serializing the ModuleList stream. Since there are other
streams which use the same format (MemoryList and MemoryList64, at
least). I tried to generalize the code a bit so that adding future
streams of this type can be done with less code.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.May 2 2019, 1:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2019, 1:42 AM
jhenderson added inline comments.May 7 2019, 9:23 AM
include/llvm/ObjectYAML/MinidumpYAML.h
57 ↗(On Diff #197714)

with -> with a

58 ↗(On Diff #197714)

KindV and TypeV aren't clear names to me. What does the V stand for?

64 ↗(On Diff #197714)

explicit?

70–73 ↗(On Diff #197714)

I'm not sure how much sense it makes to go into the detail of the minidump file format versus the memory view here. I also am not convinced by the repetition of this in the comments below.

lib/ObjectYAML/MinidumpYAML.cpp
481 ↗(On Diff #197714)

Same comment as above re. names.

test/tools/obj2yaml/basic-minidump.yaml
47–49 ↗(On Diff #197714)

It would be nice if these were padded so that they all line up. Ditto in the Stack block below.

51 ↗(On Diff #197714)

I don't have a concrete suggestion, but it might be nice to have a shorter field name than "Start of Memory Range", but that's less of a concern if that's the actual minidump field name.

labath marked 9 inline comments as done.May 8 2019, 7:01 AM
labath added inline comments.
include/llvm/ObjectYAML/MinidumpYAML.h
58 ↗(On Diff #197714)

"variable" or "value" or something like that. :) Not a very good name, but I needed to differentiate that from the class member with the same name. However, just I've had an idea of how to organize this better and reduce the number of template parameters (by making these static members of the EntryT type). This also avoid the need for inventing names here.

70–73 ↗(On Diff #197714)

I've removed the memory vs. file blurb.

test/tools/obj2yaml/basic-minidump.yaml
47–49 ↗(On Diff #197714)

The microsoft structure definition calls this field just "teb" (for Thread Environment Block), but I've found that too opaque, so I expanded the acronym (sans "thread", because it is obvious we are talking about threads here). I could shorten this further to "environment" (the word "block" probably doesn't add that much value) , or even to "teb" for consistency with microsoft headers. Let me know what you think.

51 ↗(On Diff #197714)

That's how the field is called in the official microsoft documentation (https://docs.microsoft.com/en-us/windows/desktop/api/minidumpapiset/ns-minidumpapiset-minidump_memory_descriptor), which is probably the closest thing to a "spec" for this thing. It's a bit verbose, and probably "Address" would just suffice here, but otoh it's nice for this to match the official name.

labath updated this revision to Diff 198653.May 8 2019, 7:01 AM
labath marked 2 inline comments as done.

Address review comments.

jhenderson accepted this revision.May 9 2019, 7:43 AM

LGTM, with the suggested fixes.

test/tools/obj2yaml/basic-minidump.yaml
105 ↗(On Diff #198653)

Similar comment here about whitespace. Make the values of attributes within a block line up.

106 ↗(On Diff #198653)

Move this to be above the Stack block for readability.

47–49 ↗(On Diff #197714)

Environment Block is fine. I was actually referring to the number of spaces between the attribute name and value, i.e. I'd prefer this:

- Thread Id:         0x5C5D5E5F
  Priority Class:    0x60616263
  Environment Block: 0x6465666768696A6B
51 ↗(On Diff #197714)

Let's leave it as is, since it matches the Microsoft document.

This revision is now accepted and ready to land.May 9 2019, 7:43 AM
labath marked 6 inline comments as done.May 9 2019, 8:10 AM

Thanks for the review.

test/tools/obj2yaml/basic-minidump.yaml
47–49 ↗(On Diff #197714)

Ok, I see. I can do that manually here, but that won't prevent the actual output from obj2yaml from being misaligned (which is why i was trying to come up with a shorter name).

This revision was automatically updated to reflect the committed changes.
labath marked an inline comment as done.