[PATCH] D36558: [llvm-objcopy] Add support for nested and overlapping segments

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In https://reviews.llvm.org/D36558#857165, @jakehehrlich wrote:

> Are you aware of some way of adding unit tests in llvm?


As @efriedma mentioned, there are plenty of unit tests in llvm/unittests (I don't know if you're developing on Windows or Linux, but at least in the Visual Studio solution, most of the projects under the "Test" group are unit test projects). I have a little experience with adding unit tests to existing tests, but no experience with setting up a whole new set of unit tests for a new program, such as llvm-objcopy. I do believe that it would require moving all the code that we want to test this way into a separate support library. In general, my experience has been that writing unit tests has been easier to generate sufficient coverage than regression tests, but that was with an internal non-LLVM product, so the principle may not so readily apply here.

I think if the alternative is large numbers of pre-built binaries, I think we have to unit test this, if we want sensible coverage. However, I also think this a broader problem with llvm-objcopy, so I think it's not part of this change. However, I do think it's something that should be looked at sooner rather than later (from experience, the bigger a project grows, the harder it is to unit test, because certain design decisions that make unit testing hard get harder to undo). This all might be something to ask about on the mailing list (i.e. how to start unit testing the program), as I haven't discovered any documentation on it anywhere.

I'm happy with the code as is now, but I'm not sure how we can exercise the parts of the code where the bugs were from a Lit perspective until we add the ability to actually do things to the ELF to llvm-objcopy (e.g. strip sections). I'd therefore like the unit testing discussion/investigation to happen before I accept this review (although if you don't think it's worth it, I could probably be persuaded otherwise).


Repository:
  rL LLVM

https://reviews.llvm.org/D36558