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

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap accepted this revision.
alexshap added inline comments.


================
Comment at: tools/llvm-objcopy/COFF/Writer.cpp:43
 
+Error COFFWriter::finalizeSectionNumbers() {
+  for (Symbol &Sym : Obj.getMutableSymbols()) {
----------------
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.


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

https://reviews.llvm.org/D56683