[PATCH] D82503: [llvm-objcopy][MachO] Fix segment size alignment

Thu Jun 25 02:30:32 PDT 2020

alexshap marked an inline comment as done.
alexshap added inline comments.

================
Comment at: llvm/test/tools/llvm-objcopy/MachO/segment-size-alignment.test:59
+## xcrun -sdk iphoneos clang -arch armv7 main.c -o main.exe
+--- !mach-o
+FileHeader:
----------------
jhenderson wrote:
> Do we really need such verbose inputs for the test case? This test is purely about the segment address/size values. Everything else seems to be noise to me, and makes it hard to identify what in the input YAML is actually important.
> 
> I'm not convinced it's important for the input to be a fully runnable executable.
Testing real executables makes several aspects easier:
the binaries are valid / they can be run, the issue with misaligned segments doesn't actually manifest itself until smb runs the binary (in particular, all the tools would perfectly consume it without any issues). 
More importantly, If the test fails one has to inspect the input, and if the input is invalid it prevents you from using a debugger and other tools.
This is the main reason why I'm advocating for having normal binaries for this test rather than something artificial. 
A few other considerations: segments (including __LINKEDIT) have to be non-trivial to actually verify those computations / catch issues => their content essentially shouldn't be removed. What can be removed (relatively easy): some load commands (e.g. LC_DYLD_INFO_ONLY, LC_LOAD_DYLINKER, LC_ENCRYPTION_INFO, LC_LOAD_DYLIB), this comes at the cost of the ability to debug / inspect the binaries mentioned above. What do you think @jhenderson ?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82503