[Lldb-commits] [PATCH] D61423: MinidumpYAML: add support for the ThreadList stream

[James Henderson via Phabricator via lldb-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

LGTM, with the suggested fixes.



================
Comment at: test/tools/obj2yaml/basic-minidump.yaml:47-49
+      - Thread Id:       0x5C5D5E5F
+        Priority Class:  0x60616263
+        Environment Block: 0x6465666768696A6B
----------------
labath wrote:
> jhenderson wrote:
> > It would be nice if these were padded so that they all line up. Ditto in the Stack block below.
> 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.
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
```


================
Comment at: test/tools/obj2yaml/basic-minidump.yaml:51
+        Stack:           
+          Start of Memory Range: 0x6C6D6E6F70717273
+          Content:         7475767778797A7B
----------------
labath wrote:
> jhenderson wrote:
> > 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.
> 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. 
Let's leave it as is, since it matches the Microsoft document.


================
Comment at: test/tools/obj2yaml/basic-minidump.yaml:105
+# CHECK-NEXT:           Start of Memory Range: 0x6C6D6E6F70717273
+# CHECK-NEXT:           Content:         7475767778797A7B
+# CHECK-NEXT:         Context:         7C7D7E7F80818283
----------------
Similar comment here about whitespace. Make the values of attributes within a block line up.


================
Comment at: test/tools/obj2yaml/basic-minidump.yaml:106
+# CHECK-NEXT:           Content:         7475767778797A7B
+# CHECK-NEXT:         Context:         7C7D7E7F80818283
 # CHECK-NEXT: ...
----------------
Move this to be above the Stack block for readability.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61423