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



_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to