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

[Pavel Labath via Phabricator via lldb-commits]

labath marked 9 inline comments as done.
labath added inline comments.


================
Comment at: include/llvm/ObjectYAML/MinidumpYAML.h:58
+/// streams with similar structure.
+template <typename EntryT, Stream::StreamKind KindV, minidump::StreamType TypeV>
+struct ListStream : public Stream {
----------------
jhenderson wrote:
> KindV and TypeV aren't clear names to me. What does the V stand for?
"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.


================
Comment at: include/llvm/ObjectYAML/MinidumpYAML.h:70-73
+/// A structure containing all data belonging to a single minidump module. On
+/// disk, these are placed at various places in the minidump file and
+/// cross-referenced via their offsets, but for ease of use, we group them
+/// together in the logical memory view.
----------------
jhenderson wrote:
> 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.
I've removed the memory vs. file blurb.


================
Comment at: test/tools/obj2yaml/basic-minidump.yaml:47-49
+      - Thread Id:       0x5C5D5E5F
+        Priority Class:  0x60616263
+        Environment Block: 0x6465666768696A6B
----------------
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.


================
Comment at: test/tools/obj2yaml/basic-minidump.yaml:51
+        Stack:           
+          Start of Memory Range: 0x6C6D6E6F70717273
+          Content:         7475767778797A7B
----------------
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. 


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61423