[PATCH] D59291: Add basic minidump support to obj2yaml

[Pavel Labath via Phabricator via llvm-commits]

labath added a comment.

The BinaryFormat changes are basically just defining a bunch of structs and enums, so there isn't much to test there. The only actual functional change there is the addition of the magic recognition code in Magic.cpp. That could easily be unit-tested, but that would test just those 5 lines of changes. These would be covered by any other minidump tests anyway, which is why I guess there are no existing unit tests for the magic recognition code.

The Object changes are more interesting. These could be unit tested, but I think these tests would require "baked" binaries (perhaps in the form of inline C char arrays) since, well, the Object code is all about opening binaries, and we have no way to produce those at that point. Overall, I think a small, well-targeted set of small test binaries is useful for "rooting" the tests of code like this for two reasons:

- this ultimately needs to interoperate with other producers/consumers, and if everything goes through one (de)serialization point, there nothing to guard against accidental changes in the binary format (it has happened to me while transcribing the lldb structure definitions)
- some things (particularly various boundary conditions and malformed binaries) cannot be expressed in yaml form. For instance, the yaml format I'm proposing here will not be able to generate a minidump where one of the streams runs off the end of the file (much like the elf yaml representation cannot express truncated sections)

So, if we want to split this up more, I can propose the following:

- split off the BinaryFormat&Object changes into a separate patch, test it with some inline C char arrays
- write the yaml2obj equivalent of this patch, test it via unit tests and the Object minidump reader
- finally, come back to this patch (obj2yaml), test it via `yamlobj|obj2yaml` round-tripping

What do you think?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59291