[Lldb-commits] [PATCH] D59482: [ObjectYAML] Add basic minidump generation support

[Pavel Labath via Phabricator via lldb-commits]

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


================
Comment at: lib/ObjectYAML/MinidumpYAML.cpp:181
+  FeaturesRef.writeAsBinary(FeaturesStream);
+  std::fill(Features.begin(), Features.end(), 0);
+  std::memcpy(Features.begin(), FeaturesStorage.begin(),
----------------
jhenderson wrote:
> Is this definitely necessary? What's the type of Info.ProcessorFeatures? Similar comments for Vendor ID below.
No, it's not necessary, it's just that I was too lazy to implement proper error checking for this (and this field isn't that important -- I don't believe we actually use it for anything right now, but I have to print it in some way). I've now implemented proper checking for this and the VendorID fields (including tests).


================
Comment at: lib/ObjectYAML/MinidumpYAML.cpp:306
+Error MinidumpYAML::writeAsBinary(StringRef Yaml, raw_ostream &OS) {
+  yaml::Input yin(Yaml);
+  Object Obj;
----------------
jhenderson wrote:
> I don't think `yin` matches our (current) variable naming guidelines. It should be `Yin` at least, though it wasn't immediately clear what was meant even then at first to me, so perhaps a different name might be better (simply `Input` would work for me).
There's definitely plenty of places that use `yin`, with the yaml library itself being the most prominent user. I think you could argue that it fits the naming guidelines due to the "emulates c++ stl interface" (i.e `cin`) exception, but I don't think the name really matters that much here, so I've just renamed it to `Input`.


================
Comment at: tools/yaml2obj/yaml2obj.cpp:61
+      if (Doc.Minidump) {
+        MinidumpYAML::writeAsBinary(*Doc.Minidump, Out);
+        return 0;
----------------
jhenderson wrote:
> I feel like it would make more sense for this to match the other versions above, i.e. I think this should be at least `return yaml2minidump(...);`
Yeah, I had the same feeling, but then I also didn't like creating a `yaml2minidump.cpp` just to put a two-line function in it (and putting this function into the ObjectYAML library also didn't feel right).

The root cause here is that I've chose to fully library-ize the minidump code, which doesn't fit in with how the other object files work (though there is a kind of a precedent for this in DWARFYAML). My reason for doing that was that I think it would be very useful to have this functionality accessible to lldb unit tests (and I've also gotten interest from a downstream user who wanted to use this functionality to synthesize minidumps in their unit tests).

Long story short, I've now put the created yaml2minidump.cpp and put the yaml2minidump in it.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59482