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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
