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

Fri Jun 26 01:36:35 PDT 2020

jhenderson added a comment.

My inline comment aside, I'm happy with the code change. Not personally going to LGTM the patch though because I don't feel confident in reviewing the test.

================
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:
----------------
alexshap wrote:
> 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 ?
As a developer, I may not have access to Mach-O using hardware to verify whether something runs or to run it in a debugger. It's quite possible however that I might be making changes in a related area, and accidentally break this test. With such a verbose input, it makes it harder for me to figure out what the characteristics of this input are that might be causing my change to fail. A more concise output makes it easier to spot the difference. Obviously, if the behaviour interacts closely with multiple areas of an object, each of those areas will need to be appropriately fleshed out.

As for inspecting what's wrong with a binary, I'd expect to be able to do that with tools such as llvm-readobj, even if it's malformed in some way. I'd expect such tools to be tolerant of things being malformed, reporting warnings as necessary, so that I can look at the bit that is really important. This is something that has been changed a lot in the ELF side of llvm-readobj, but less so in other tools.

I think there is a place for tests that take a binary produced by the tools and makes sure its runnable. I just don't think that place is in the low-level LLVM lit tests. Perhaps it belongs in a different suite.

FWIW, I think the state of Mach-O yaml2obj doesn't help, since it requires very verbose output, much of which could probably be omitted with a bit of improvement to yaml2obj. I think taking the time to do that could be a worthwhile investment, but it isn't something I'll be doing.

That all being said, I don't actually maintain the Mach-O port, I just help with the high-level reviewing, so I'd defer to others like @smeenai and yourself, as to what your preference is.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82503