[PATCH] D59482: [ObjectYAML] Add basic minidump generation support

[Pavel Labath via Phabricator via llvm-commits]

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


================
Comment at: include/llvm/ObjectYAML/MinidumpYAML.h:27
+struct Stream {
+  enum class StreamKind {
+    Hex,
----------------
clayborg wrote:
> Maybe "Encoding" might be a better name?
On it's own I agree Encoding would be better, but the term `Kind` is already used by the EFLYAML implementation, so I think it's better to be consistent with that.


================
Comment at: include/llvm/ObjectYAML/MinidumpYAML.h:28
+  enum class StreamKind {
+    Hex,
+    SystemInfo,
----------------
clayborg wrote:
> Maybe "Binary" instead of "Hex"? Or maybe "RawBytes"?
Makes sense. In fact ELF already uses the name `RawContent` for the equivalent functionality, so I went with that. I also changed `Text` to `TextContent`.


================
Comment at: include/llvm/ObjectYAML/MinidumpYAML.h:50
+  yaml::BinaryRef Content;
+  yaml::Hex32 Size;
+
----------------
clayborg wrote:
> Is the "Size" necessary here? Does yaml::BinaryRef not contain a size?
This is another bit I stole from ELFYAML. The idea is that you can specify only the size, in case you don't actually care about the content, or specify the size+content if you only care about the initial few bytes of the section. However, it looks like I didn't finish copying the idea, so I was missing the bits that check that `Size >= Content.size()` and the bit which pads the size when serializing. Now I fix that and add some additional tests.


================
Comment at: include/llvm/ObjectYAML/MinidumpYAML.h:81
+/// /proc/<pid> file on linux).
+struct TextStream : public Stream {
+  BlockStringRef Text;
----------------
clayborg wrote:
> Maybe further specify the content like "UTF8Stream" or "ASCIIStream"? Not sure what the encoding is expected to be, but might be better to indicate the exact text encoding here?
Unfortunately, I don't think there is a well-defined encoding here. I think the files will be mostly ASCII, but for instance the encoding of the "filename" part of /proc/PID/maps can really be anything. So, I think we should just keep this bit deliberately vague.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59482