[PATCH] D56683: [llvm-objcopy] [COFF] Add support for removing sections

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added inline comments.


================
Comment at: tools/llvm-objcopy/COFF/Writer.cpp:43
 
+Error COFFWriter::finalizeSectionNumbers() {
+  for (Symbol &Sym : Obj.getMutableSymbols()) {
----------------
alexshap wrote:
> mstorsjo wrote:
> > alexshap wrote:
> > > mstorsjo wrote:
> > > > alexshap wrote:
> > > > > general comment (plus maybe others (Jake, James, Jordan) add their opinion): I know that smth like this ("finalize*" methods) has been going on in the llvm-objcopy for ELF for a while (in particular, the abstract class Writer has had the method "finalize" from the early beginning), however, this brings up the following question: what is the contract between Object and Writer ? in particular, to what extent an instance of Object should be "ready" to be consumable by a writer. 
> > > > > 
> > > > Well, it's not very strictly typed out, but in general, while the Object class tries to maintain internal consistency as far as possible, some of the fields in the file format level structs have broken-out forms in the Symbol/Section/Relocation classes. In these cases, the broken-out field is the correct one that is maintained, and the corresponding field in the file format level struct is only set once the final layout of the file is known.
> > > so this is still a concern for me (and the existing ELF code too). So basically several people have mentioned interest in building a library from the code in llvm-objcopy, I'm thinking more and more about the current interfaces & design. Let me try to explain what makes me worried: so basically the idea of having these separate abstractions (Reader, Object, Writer) provides two important "extension points": we can read multiple input formats and we can write multiple output formats (at least potentially). For example, if one day smb decides to implement YAMLWriter i think it'll be unfortunate if he has to reimplement all this finalize* logic. Another thought - this introduces more subtle coupling between these classes (the assumptions which Writer makes about Object are non-obvious, at least for me). I think the same applies to ELF. @jakehehrlich , @jhenderson, @rupprecht - maybe you have some other thoughts / I'm missing something ?
> > Very fair points, and it might be good to move some amount of finalization back from Writer into Object.
> > 
> > Are you ok with me moving forward with this patch though, and we can look at that as a separate refactoring concern at a later point?
> yeah, I'm okay with that (don't want to block this diff), I just wanted to take advantage of this discussion to bring this question up / get some feedback from other people.
Sure, there's definitely opportunity for cleanup...
 - The split between write() and finalize() doesn't seem that important -- finalize() is always followed by write(), I don't see why we can't just combine them.
 - Having some sort of finalize/write logic in writers seems necessary; different ways of outputting the object is going to have different requirements (e.g. final bookkeeping)
 - But, perhaps Object itself should have a finalize() method for generic things (e.g. assigning sections indices). The balance of what goes in an Object vs Writer finalize method is going to be more of an art than a science, I think.
 - I think intra-coupling is actually the worse culprit, e.g. look at BinaryELFBuilder (to use my own code as a bad example), which requires its own private methods to be called in a specific error.
 - Also I'm not sure how much (if at all) we can generalize this -- creating a generic Object super interface that is unique across ELF/COFF/MachO will likely be too restrictive. Probably something we should just enforce in code review.
All that said, I don't think anything blocks this patch, but it's good to bring it up.


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

https://reviews.llvm.org/D56683