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