[PATCH] D42222: [llvm-objcopy] Refactor llvm-objcopy to use reader and writer objects

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich created this revision.
jakehehrlich added reviewers: phosek, jhenderson.

While writing code for input and output formats in llvm-objcopy it became apparent that there was a code health problem. This change attempts to solve that problem by refactoring the code to use Reader and Writer objects that can read in different objects in different formats, convert them to a single shared internal representation, and then write them to any other representation.

New classes:
Reader: the base class used to construct instances of the internal representation
Writer: the base class used to write out instances of the internal representation
ELFBuilder: a helper class for ELFWriter that takes an ELFFile and converts it to a Object
SectionVisitor: it became necessary to remove writeSection from SectionBase because, under the new Reader/Writer scheme, it's possible to convert between ELF Types such as ELF32LE and ELF32BE. This isn't possible with writeSection because it (dynamically) depends on the underlying section type *and* (statically) depends on the ELF type. Bad things would happen if the underlying sections for ELF32LE were used for writing to ELF64BE. To avoid this code smell (which would have compiled, run, and output some nonsesnse) I decoupled writing of sections from a class.
SectionWriter: This is just the ELFT templated implementation of SectionVisitor. Many classes now have this class as a friend so that the writing methods in this class can write out private data.
ELFWriter: This is the Writer that outputs to ELF
BinaryWriter: This is the Writer that outputs to Binary
ElfType: Because the ELF Type is not a part of the Object anymore we need a way to construct the correct default Writer based on properties of the Reader. This enum just keeps track of the ELF type of the input so it can be used as the default output type as well.

Object has correspondingly undergone some serious changes as well. It now has more generic methods for building and manipulating ELF binaries. This interface makes ELFBuilder easy enough to use and will make the BinaryReader/Builder easy to create as well. Most changes in this diff are cosmetic and deal with the fact that a method has been moved from one class to another or a change from a pointer to a reference. Almost no changes should result in a functional difference (this is after all a refactor). One minor functional change was made and the result can be seen in remove-shstrtab-error.test. The fact that it fails hasn't changed but the error message has changed because that failure is detected at a later point in the code now (because WriteSectionHeaders is a property of the ElfWriter *not* a property of the Object). I'd say roughly 80-90% of this code is cosmetically different, 10-19% is different but functionally the same, and 1-5% is functionally different despite not causing a change in tests.

Known Code Smells:

I'm actully aware of a number of issues I'd personally like to fix before this is submitted. This code is still rough and in need of some polish. For instance there's a bit of code duplication that I haven't removed. There are several "if(auto *o = dyn_cast<ELFObjectFile<...>(...))" blocks around that I'd like to remove. Also the handling of BufPtr and SectionWriter in BianryWriter and ElfWriter is duplicated. I'd like to remove that (I think that one can be accomplished by just folding it into the Writer base class). Also it might be possible to split this up into separate diffs. Any recommendations on how to split it up would be welcome.  I figured now that it a) compiles and b) appears to pass all tests that I could start getting some feedback.


Repository:
  rL LLVM

https://reviews.llvm.org/D42222

Files:
  test/tools/llvm-objcopy/remove-shstrtab-error.test
  tools/llvm-objcopy/Object.cpp
  tools/llvm-objcopy/Object.h
  tools/llvm-objcopy/llvm-objcopy.cpp
  tools/llvm-objcopy/llvm-objcopy.h

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D42222.130334.patch
Type: text/x-patch
Size: 58618 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180118/dd1104a8/attachment.bin>