[PATCH] D63395: [llvm-objcopy][MachO] Support load commands used in executables/shared libraries

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOWriter.h:28
+  // Points to the __LINKEDIT segment if it exists.
+  MachO::macho_load_command *LinkEditLoadCommand = nullptr;
 
----------------
alexshap wrote:
> so after some thinking about what's going on here it seems to me
> maybe it's possible to move the logic for building the layout from MachOWriter into a separate class MachOLayoutBuilder, basically it's motivated by "separation of concerns" / desire to simplify a bit what's going on here. Roughly speaking the idea is the following:  
> 
> 1. MachOLayoutBuilder will take the model (Object) and update all the offsets / sizes. In particular, it might hold some additional state internally (like MachO::macho_load_command *LinkEditLoadCommand) and will be responsible for calling the private subroutines (computeSizeOfCmds, layoutSegments, layoutRelocations, updateDySymTab, layoutTail) in the correct order, etc. The contract here is that after calling its public method build() the layout is final and one can safely use it to calculate / get the total size of the binary, all the offsets/sizes are valid etc. 
> 
> 2. MachOWriter will be responsible for managing the Buffer + writing things into it properly, but it will never modify the model (Object) directly (but internally it can make use of MachOLayoutBuilder before writing things out).
> 
> What do you think ?
> 
in 2. i meant that it would not modify the offsets/sizes directly. 

In general, i don't insist on this approach since things are quite complex here, we have to update  / rebuild many other things besides the offsets/sizes. If we could split out that logic - that would be cool imo / it would enable us to unload some complexity from the writer, this was the main point of my comment. 


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

https://reviews.llvm.org/D63395